diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 8614bc094..588a8a2a2 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -97,6 +97,17 @@ 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. +#[derive(Debug)] +struct GroupStep { + doc: u64, + after: std::sync::Weak, +} + /// The clipboard: a block of cells (with formulas and styles) or marching-ants reference. #[derive(Clone, Debug)] pub struct Clipboard { @@ -405,8 +416,9 @@ 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 made: document, its undo length after the step, and the epoch. - group_step: Option<(u64, usize, u64)>, + /// The step the last `group_end` made, so `execute` can tell that a command's step is exactly + /// its group's. + group_step: Option, /// Measures cell text for AutoFit Column Width. Without it (CLI, MCP) AutoFit uses /// [`cmd::format::approx_text_width`]. pub text_width: Option, @@ -567,12 +579,14 @@ impl Session { let mut label = spec.label.trim_end_matches('…').to_string(); if own_groups && let Some((_, len)) = start { // A command whose only step is its group's keeps that name ("Run Macro: Totals"). - // The step must have been made by a group this command ended (its epoch is - // past the one the command started with): an older record at the same length - // belongs to an earlier command, undone since. + // 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, + // a group step of an earlier command (undone since), or a group's step that + // the command edited past all name the step after the command instead. if d.undo.len() == len + 1 - && self.group_step.is_some_and(|(doc, n, epoch)| doc == uid && n == len + 1 && epoch > group_epoch) && 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))) { label = last.label.clone(); } @@ -646,7 +660,7 @@ 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((g.doc, d.undo.len(), self.group_epoch)); + self.group_step = Some(GroupStep { doc: g.doc, after: Arc::downgrade(&d.wb) }); d.redo.clear(); d.revision += 1; } @@ -679,10 +693,6 @@ impl Session { d.undo.truncate(g.start); d.redo = g.redo; d.revision += 1; - // A step the rollback dropped is no longer anyone's to name. - if self.group_step.is_some_and(|(doc, n, _)| doc == g.doc && n > g.start) { - self.group_step = None; - } Ok(()) } diff --git a/crates/engine/src/tests/seams.rs b/crates/engine/src/tests/seams.rs index b3397c724..a598bb7c7 100644 --- a/crates/engine/src/tests/seams.rs +++ b/crates/engine/src/tests/seams.rs @@ -226,6 +226,27 @@ fn register_group_commands() { }); // A command that is one group keeps the group's name. register("test.onlyGroup", |s, _| s.grouped("Named Step", |s| put(s, "D1", "x")).map(|_| json!({}))); + // A group, then an edit the command makes itself (no nested command, so no undo entry). + register("test.groupThenEdit", |s, _| { + s.grouped("Named Step", |s| put(s, "D1", "x"))?; + crate::cmd::edit(s, |cx| { + let at = gridcraft_core::CellRef::parse("D2").ok_or_else(|| EngineError::Other("D2".into()))?; + cx.sheet_mut(0)?.set_value(at, gridcraft_core::Value::Text("y".into())); + cx.touch(0, at); + Ok(()) + })?; + Ok(json!({})) + }); + // A group with a change, then one that is rolled back: the step is still the first group's. + register("test.groupThenRolledBack", |s, _| { + s.grouped("Named Step", |s| put(s, "D1", "x"))?; + let failed: crate::Result<()> = s.grouped("Doomed", |s| { + put(s, "D3", "gone")?; + Err(EngineError::Other("no".into())) + }); + assert!(failed.is_err()); + Ok(json!({})) + }); // An edit, then the rollback of the innermost group that was open before the command. register("test.rollbackOpenGroup", |s, _| { put(s, "E1", "gone")?; @@ -266,9 +287,9 @@ fn a_command_that_runs_its_own_group_makes_one_step() { #[test] fn a_stale_group_step_does_not_name_a_later_command() { - // The record of the last group step (document, length, epoch) survives the Undo of that step. - // A later command that ends a group of its own at the same history length, after rolling - // another back, must not take the name of the nested edit it made. + // The record of the last group step survives the Undo of that step. A later command that + // ends a group of its own at the same history length, after rolling another back, must not + // take the name of the nested edit it made. register_group_commands(); let mut s = s(); s.execute("test.onlyGroup", json!({})).unwrap(); @@ -285,6 +306,27 @@ fn a_stale_group_step_does_not_name_a_later_command() { assert!(v(&s, "B2").is_empty() && v(&s, "D1").is_empty()); } +#[test] +fn a_command_keeps_its_groups_name_only_for_exactly_that_step() { + // The name comes from the group only when the group's step is the command's whole step: an + // edit after the group (no nested command, so the history length matches) makes it the + // command's own, while a later group that rolled back changes nothing. + register_group_commands(); + let mut s = s(); + s.execute("test.groupThenEdit", json!({})).unwrap(); + assert_eq!(undo_labels(&s), ["test.groupThenEdit"]); + assert_eq!((v(&s, "D1").display(), v(&s, "D2").display()), ("x".into(), "y".into())); + s.execute("edit.undo", json!({})).unwrap(); + assert!(v(&s, "D1").is_empty() && v(&s, "D2").is_empty()); + assert_eq!(undo_labels(&s), Vec::::new()); + s.execute("test.groupThenRolledBack", json!({})).unwrap(); + assert_eq!(undo_labels(&s), ["Named Step"]); + assert_eq!(v(&s, "D1").display(), "x"); + assert!(v(&s, "D3").is_empty()); + s.execute("edit.undo", json!({})).unwrap(); + assert!(v(&s, "D1").is_empty()); +} + #[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