From f4e05534a3aa46d28a686db12d5649c2e6d97b40 Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Wed, 21 Oct 2026 15:20:29 -0400 Subject: [PATCH] Engine: a command that runs its own undo groups makes one undo step for itself execute treated any change in the group epoch as 'the group owns the step'. A command that edited outside its own group, or whose group rolled back before it edited, left that edit with no undo step or only part of it. Now only a group that was open before the command (or is still open after) owns the step; a command that opened and closed its own groups replaces their steps with one entry for the command. --- crates/engine/src/cmd/mod.rs | 17 ++++++++++++ crates/engine/src/lib.rs | 27 +++++++++++++++++--- crates/engine/src/tests/seams.rs | 44 ++++++++++++++++++++++++++++++++ docs/architecture.md | 4 ++- 4 files changed, 88 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index 1db65c20c..fadb72b6c 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -187,6 +187,23 @@ pub fn register_test_command(id: &'static str, run: Run) { test_commands().push(spec); } +/// Test-only: an undoable command, to exercise how `execute` records its step. +#[cfg(test)] +pub(crate) fn register_undoable_test_command(id: &'static str, run: Run) { + let spec: &'static CommandSpec = Box::leak(Box::new(CommandSpec { + id, + label: id, + menu: &[], + shortcut: None, + params: "", + enabled: |_| Ok(()), + run, + journal: false, + undoable: true, + })); + test_commands().push(spec); +} + fn test_commands() -> std::sync::MutexGuard<'static, Vec<&'static CommandSpec>> { #[allow(clippy::unwrap_used)] // a poisoned test registry has no meaningful recovery match TEST_COMMANDS.lock() { diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index b57c474a1..e12335e88 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -327,6 +327,8 @@ 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)>, } impl Session { @@ -396,6 +398,8 @@ impl Session { let before = self.active().map(|d| (d.uid, d.wb.clone(), d.selection.clone())); let params = if params.is_null() { Json::Object(Default::default()) } else { params }; let (first_group, group_epoch) = (self.next_group, self.group_epoch); + // Whether a group was open on the document before the command, and how long its history was. + let start = before.as_ref().and_then(|(uid, ..)| self.docs.iter().find(|d| d.uid == *uid).map(|d| (self.in_group(*uid), d.undo.len()))); // The recorder keeps top-level calls with the place they ran in. let place = (self.depth == 0 && self.recorder.is_some()).then(|| match self.active() { Some(d) => (d.uid, d.wb.active_sheet, d.wb.active().map(|sh| sh.name.clone()).unwrap_or_default(), d.selection.clone()), @@ -434,15 +438,31 @@ impl Session { self.journal.drain(..5_000); } } - // Inside an undo group, or when the command opened or closed one, the group owns the step. - let grouped = before.as_ref().is_some_and(|(uid, ..)| self.group_epoch != group_epoch || self.in_group(*uid)); + // 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. + 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); 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 { - d.undo.push(HistoryEntry { label: spec.label.trim_end_matches('…').to_string(), wb, selection: sel }); + 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"). + 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() + { + label = last.label.clone(); + } + d.undo.truncate(len); + } + d.undo.push(HistoryEntry { label, wb, selection: sel }); if d.undo.len() > UNDO_LIMIT { d.undo.remove(0); } @@ -509,6 +529,7 @@ impl Session { while d.undo.len() > UNDO_LIMIT { d.undo.remove(0); } + self.group_step = Some((g.doc, d.undo.len(), self.group_epoch)); d.redo.clear(); d.revision += 1; } diff --git a/crates/engine/src/tests/seams.rs b/crates/engine/src/tests/seams.rs index cc082258e..60f84dcf2 100644 --- a/crates/engine/src/tests/seams.rs +++ b/crates/engine/src/tests/seams.rs @@ -202,6 +202,50 @@ fn a_group_owns_the_steps_of_commands_that_run_commands() { assert_eq!(v(&s, "A1").display(), "kept"); } +fn put(s: &mut Session, cell: &str, text: &str) -> crate::Result<()> { + s.execute("cell.set", json!({"cell": cell, "input": text})).map(|_| ()) +} + +#[test] +fn a_command_that_runs_its_own_group_makes_one_step() { + use crate::cmd::register_undoable_test_command as register; + // Edits before its group are part of the step. + register("test.editThenGroup", |s, _| { + put(s, "A1", "outside")?; + s.grouped("Inner", |s| put(s, "A2", "inside"))?; + Ok(json!({})) + }); + // A group that fails and rolls back, then an edit outside it. + register("test.failThenEdit", |s, _| { + let failed: crate::Result<()> = s.grouped("Doomed", |s| { + put(s, "B1", "gone")?; + Err(EngineError::Other("no".into())) + }); + assert!(failed.is_err()); + put(s, "B2", "kept")?; + Ok(json!({})) + }); + // 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!({}))); + let mut s = s(); + s.execute("test.onlyGroup", json!({})).unwrap(); + assert_eq!(undo_labels(&s), ["Named Step"]); + s.execute("edit.undo", json!({})).unwrap(); + set(&mut s, "C1", "base"); + s.execute("test.editThenGroup", json!({})).unwrap(); + assert_eq!(undo_labels(&s), ["Enter Cell", "test.editThenGroup"]); + 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("test.failThenEdit", json!({})).unwrap(); + assert!(v(&s, "B1").is_empty()); + assert_eq!(v(&s, "B2").display(), "kept"); + assert_eq!(undo_labels(&s), ["Enter Cell", "test.failThenEdit"]); + s.execute("edit.undo", json!({})).unwrap(); + assert!(v(&s, "B2").is_empty()); +} + // ---------------------------------------------------------------- recording buffer fn recorded_ids(s: &Session) -> Vec { diff --git a/docs/architecture.md b/docs/architecture.md index 446cbe432..0f97f857b 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -51,7 +51,9 @@ Actions made of many commands (macros, a debugger that runs one step per call) u - **Undo groups.** `Session::grouped(label, f)` runs `f` as one undo step and rolls every change back if `f` fails; `group_begin`, `group_end` and `group_rollback` do the same across calls. While a group is open on a document, its commands add no undo steps of their own and Undo and - Redo are disabled there. Groups nest; the outermost one makes the step. + Redo are disabled there. Groups nest; the outermost one makes the step. A command that opens and + closes its own groups still makes one undo step for itself, covering its edits inside and outside + the groups. - **Recording buffer** (`record.rs`). `Session::start_recording` collects each successful top-level command with the document, sheet and selection it ran in, without secret parameters. Commands run by other commands are not recorded, and the buffer is independent of the journal