From 114088d589bf661ecff010a9aa162470bbd2e11e Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Fri, 23 Oct 2026 16:33:42 -0400 Subject: [PATCH] Engine: a command's changes after ending a group, or before opening one it leaves open, are not lost or a step of their own Two boundaries of the group rules were wrong. A command that ended a group open before it was treated as grouped whatever it did next: with an unchanged group (no step) its later edit had no undo entry at all, and with nested edits after the end the entry was the nested command's. The group's step now stays the group's, and the command makes one step of its own, under its name, for what changed after the end, from the workbook the group ended at. A command that edited and then opened a group it leaves open (a debugger's start) left its nested entries in the history, so one Undo after the group ended brought back a state from inside the command. Such a group now takes the command's start as its own, and its step covers the command's earlier edits. The record of where the last group ended now holds the workbook and selection it ended at, the history length then and whether it made a step; it lives until the outermost command finishes. A rollback records where it ended too, unless the document is back exactly where the last recorded step left it. --- crates/engine/src/lib.rs | 73 ++++++++++++++++++++++++-------- crates/engine/src/tests/seams.rs | 63 +++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 18 deletions(-) diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 588a8a2a2..b44ed0852 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -97,15 +97,19 @@ struct OpenGroup { redo: Vec, } -/// The undo step the last [`Session::group_end`] made: its document and the workbook it led to. -/// The entry itself holds the workbook it started from, so a command can check that the step is -/// exactly its own (the same start and end as the command's) and keep the group's name for it. -/// A weak reference: the record keeps no workbook alive, and a workbook that is gone can't be -/// a command's result. +/// Where the last [`Session::group_end`] or [`Session::group_rollback`] left its document: the +/// workbook and selection it ended at, the undo length then, and whether it made a step. A +/// command that ended the group tells its own later changes from the group's step by it, and +/// keeps the group's name only when the group's step is exactly its own (the entry holds the +/// workbook the step started from). Dropped when the outermost command finishes, so it keeps no +/// workbook alive between commands. #[derive(Debug)] struct GroupStep { doc: u64, - after: std::sync::Weak, + after: Arc, + selection: Selection, + len: usize, + stepped: bool, } /// The clipboard: a block of cells (with formulas and styles) or marching-ants reference. @@ -416,8 +420,7 @@ pub struct Session { next_group: u64, /// Counts group begins and ends, so `execute` can tell that a command opened or closed one. group_epoch: u64, - /// The step the last `group_end` made, so `execute` can tell that a command's step is exactly - /// its group's. + /// Where the last group ended, so `execute` can tell a command's own changes from its group's. group_step: Option, /// Measures cell text for AutoFit Column Width. Without it (CLI, MCP) AutoFit uses /// [`cmd::format::approx_text_width`]. @@ -549,6 +552,7 @@ impl Session { } if self.depth == 0 { self.trim_history(); + self.group_step = None; } return Err(EngineError::Internal(id.to_string(), msg)); } @@ -563,21 +567,44 @@ impl Session { } self.push_journal(id, cmd::journal_params(¶ms)); } - // Inside an undo group, or when the command ended one that was open before it, the group - // owns the step. A command that opened and closed its own groups makes one step of its - // own, which replaces the steps those groups made. + // Inside an undo group the group owns the step. A command that ended a group open before + // it leaves the group its step and makes one of its own for what it changed after the end. + // A command that opened and closed its own groups makes one step of its own, which + // replaces the steps those groups made. let epoch_changed = self.group_epoch != group_epoch; let in_group_now = before.as_ref().is_some_and(|(uid, ..)| self.in_group(*uid)); - let own_groups = epoch_changed && !in_group_now && start.is_some_and(|(was_open, _)| !was_open); - let grouped = !own_groups && (epoch_changed || in_group_now); + let was_open = start.is_some_and(|(was_open, _)| was_open); if spec.undoable && let Some((uid, wb, sel)) = before && let Some(d) = self.docs.iter_mut().find(|d| d.uid == uid) && !Arc::ptr_eq(&d.wb, &wb) { - if !grouped { - let mut label = spec.label.trim_end_matches('…').to_string(); - if own_groups && let Some((_, len)) = start { + let label = spec.label.trim_end_matches('…').to_string(); + if in_group_now { + // A group this command opened and left open (a debugger's start) takes the + // command's start as its own, so what the command changed before opening it is + // part of the group's step and not a step of its own. + if !was_open + && let Some((_, len)) = start + && let Some(g) = self.groups.iter_mut().filter(|g| g.doc == uid && g.id >= first_group).min_by_key(|g| g.id) + { + g.start = len; + g.wb = wb; + g.selection = sel; + } + } else if epoch_changed && was_open { + // The group it ended made its step; changes after the end, whether direct or by + // nested commands, are the command's own step from where the group ended. + if let Some(step) = self.group_step.as_ref().filter(|g| g.doc == uid) + && (d.undo.len() > step.len || !Arc::ptr_eq(&step.after, &d.wb)) + { + d.undo.truncate(step.len); + push_entry(&mut d.undo, label, step.after.clone(), &d.wb, step.selection.clone()); + d.redo.clear(); + } + } else { + let mut label = label; + if epoch_changed && let Some((_, len)) = start { // A command whose only step is its group's keeps that name ("Run Macro: Totals"). // That is so only when the one entry the command added is a group's step from // the command's own start to its own result: an entry a nested command made, @@ -586,7 +613,7 @@ impl Session { if d.undo.len() == len + 1 && let Some(last) = d.undo.last() && Arc::ptr_eq(&last.wb, &wb) - && self.group_step.as_ref().is_some_and(|g| g.doc == uid && g.after.upgrade().is_some_and(|after| Arc::ptr_eq(&after, &d.wb))) + && self.group_step.as_ref().is_some_and(|g| g.doc == uid && g.stepped && Arc::ptr_eq(&g.after, &d.wb)) { label = last.label.clone(); } @@ -603,6 +630,7 @@ impl Session { // lengths it compared stay true. if self.depth == 0 { self.trim_history(); + self.group_step = None; } result } @@ -660,10 +688,13 @@ impl Session { let changed = !Arc::ptr_eq(&d.wb, &g.wb); if changed && !outer { push_entry(&mut d.undo, g.label, g.wb, &d.wb, g.selection); - self.group_step = Some(GroupStep { doc: g.doc, after: Arc::downgrade(&d.wb) }); d.redo.clear(); d.revision += 1; } + if !outer { + self.group_step = + Some(GroupStep { doc: g.doc, after: d.wb.clone(), selection: d.selection.clone(), len: d.undo.len(), stepped: changed }); + } // Inside a command, the outermost command applies the history limit when it finishes. if self.depth == 0 { self.trim_history(); @@ -693,6 +724,12 @@ impl Session { d.undo.truncate(g.start); d.redo = g.redo; d.revision += 1; + // The record of the last group step stays when the document is back exactly where that + // step left it (this group began there); otherwise the rollback is where the group ended. + let kept = self.group_step.as_ref().is_some_and(|st| st.doc == g.doc && st.len == g.start && Arc::ptr_eq(&st.after, &d.wb)); + if !kept && !self.groups.iter().any(|o| o.doc == g.doc) { + self.group_step = Some(GroupStep { doc: g.doc, after: d.wb.clone(), selection: d.selection.clone(), len: g.start, stepped: false }); + } Ok(()) } diff --git a/crates/engine/src/tests/seams.rs b/crates/engine/src/tests/seams.rs index c524b397d..1f83bd67d 100644 --- a/crates/engine/src/tests/seams.rs +++ b/crates/engine/src/tests/seams.rs @@ -261,6 +261,20 @@ fn register_group_commands() { s.group_end(token.ok_or_else(|| EngineError::Other("no group".into()))?)?; Ok(json!({})) }); + // The end of the innermost group that was open before the command, then an edit. + register("test.endOpenGroupThenEdit", |s, _| { + let token = s.groups.last().map(|g| crate::GroupToken { id: g.id, doc: g.doc }); + s.group_end(token.ok_or_else(|| EngineError::Other("no group".into()))?)?; + put(s, "E1", "after")?; + Ok(json!({})) + }); + // An edit, then a group the command leaves open (a debugger's start). + register("test.editThenOpenGroup", |s, _| { + put(s, "A1", "first")?; + let _open = s.group_begin("Debug")?; + put(s, "A2", "second")?; + Ok(json!({})) + }); } #[test] @@ -327,6 +341,55 @@ fn a_command_keeps_its_groups_name_only_for_exactly_that_step() { assert!(v(&s, "D1").is_empty()); } +#[test] +fn what_a_command_changes_after_ending_a_group_is_its_own_step() { + // Ending a group that was open before the command leaves the group its step (if it made one) + // and makes a step of the command's own for what it changed after the end. + register_group_commands(); + let mut s = s(); + // The group made no step: the command's edit must still be undoable, under its own name. + let _unchanged = s.group_begin("Debug").unwrap(); + s.execute("test.endOpenGroupThenEdit", json!({})).unwrap(); + assert_eq!(undo_labels(&s), ["test.endOpenGroupThenEdit"]); + assert_eq!(v(&s, "E1").display(), "after"); + s.execute("edit.undo", json!({})).unwrap(); + assert!(v(&s, "E1").is_empty()); + s.execute("edit.redo", json!({})).unwrap(); + assert_eq!(v(&s, "E1").display(), "after"); + // The group made a step: two steps, the group's and the command's. + s.execute("edit.undo", json!({})).unwrap(); + let _changed = s.group_begin("Debug").unwrap(); + set(&mut s, "B1", "inside"); + s.execute("test.endOpenGroupThenEdit", json!({})).unwrap(); + assert_eq!(undo_labels(&s), ["Debug", "test.endOpenGroupThenEdit"]); + s.execute("edit.undo", json!({})).unwrap(); + assert!(v(&s, "E1").is_empty()); + assert_eq!(v(&s, "B1").display(), "inside"); + s.execute("edit.undo", json!({})).unwrap(); + assert!(v(&s, "B1").is_empty()); +} + +#[test] +fn a_group_a_command_leaves_open_starts_where_the_command_did() { + // A command that edits and then opens a group it leaves open has no undo step of its own: + // the group's step, made when it ends, covers the command's edit too, so no state from + // inside the command can come back. + register_group_commands(); + let mut s = s(); + set(&mut s, "C1", "base"); + s.execute("test.editThenOpenGroup", json!({})).unwrap(); + let d = s.doc().unwrap(); + assert!(s.in_group(d.uid)); + let token = s.groups.last().map(|g| crate::GroupToken { id: g.id, doc: g.doc }).unwrap(); + assert!(s.group_end(token).unwrap()); + assert_eq!(undo_labels(&s), ["Enter Cell", "Debug"]); + s.execute("edit.undo", json!({})).unwrap(); + assert!(v(&s, "A1").is_empty() && v(&s, "A2").is_empty()); + assert_eq!(v(&s, "C1").display(), "base"); + s.execute("edit.redo", json!({})).unwrap(); + assert_eq!((v(&s, "A1").display(), v(&s, "A2").display()), ("first".into(), "second".into())); +} + #[test] fn a_group_rolled_back_inside_a_command_keeps_the_redo_history() { // The debugger's Stop with Undo Changes rolls its group back inside a command. The rollback