From ad173006f7c0b444a19ab1e38414d0dbb6cc963d Mon Sep 17 00:00:00 2001 From: noah1400 <48360809+noah1400@users.noreply.github.com> Date: Sun, 11 Oct 2026 02:16:19 +0200 Subject: [PATCH] Undo: a command that runs another is one undo step (#406) Cut, Duplicate, Paste in Place, the Arrange commands and pasting text run a second command inside themselves, and both recorded an undo step: the second Undo did nothing. Only the outer command records now. A command that fails after changing the document records its step too; a script that stopped at a bad step left changes that could not be undone. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/scripts.rs | 4 +- crates/engine/src/guard.rs | 17 +++++ crates/engine/src/lib.rs | 33 ++++++++-- crates/engine/src/tests.rs | 105 +++++++++++++++++++++++++++++++ 4 files changed, 152 insertions(+), 7 deletions(-) diff --git a/crates/engine/src/cmd/scripts.rs b/crates/engine/src/cmd/scripts.rs index 0fa89c598..d4a03606b 100644 --- a/crates/engine/src/cmd/scripts.rs +++ b/crates/engine/src/cmd/scripts.rs @@ -8,7 +8,9 @@ use crate::Result; fn run(s: &mut crate::Session, p: &Value) -> Result { let text = str_param(p, "text").ok_or_else(|| bad("script.run", "`text` required"))?; let steps = crate::script::parse(text).map_err(|e| bad("script.run", e))?; - // The steps' own undo entries collapse into this command's single one. + // The steps record undo entries of their own while the script runs (one of them may be + // Undo); they collapse into this command's single one. + s.in_undo_step = false; let uid = s.active().map(|d| d.uid); let before = s.active().map_or(0, |d| d.history.undo.len()); let report = s.run_script(&steps); diff --git a/crates/engine/src/guard.rs b/crates/engine/src/guard.rs index 9a1df3eef..57cc575fa 100644 --- a/crates/engine/src/guard.rs +++ b/crates/engine/src/guard.rs @@ -54,6 +54,8 @@ impl Session { { log::error!("crash recovery save after `{id}` failed: {e}"); } + // The command never got to say that it is done. + self.in_undo_step = false; log::error!("command `{id}` panicked: {msg}"); Err(EngineError::Internal(id.to_string(), msg)) } @@ -86,4 +88,19 @@ mod tests { // The session keeps working. s.execute("frame.create", &json!({"rect": [36, 236, 200, 400]})).unwrap(); } + + /// An undoable command that panics is no longer running: the next command records its own + /// undo step. + #[test] + fn a_panicking_command_does_not_swallow_later_undo_steps() { + let mut s = Session::new(); + s.execute("file.new", &json!({})).unwrap(); + let panicked = s.guarded("test.panic", |s| { + s.in_undo_step = true; + panic!("boom"); + }); + assert!(panicked.is_err()); + s.execute("frame.create", &json!({"rect": [36, 36, 200, 200]})).unwrap(); + assert_eq!(s.active().unwrap().history.undo.len(), 1); + } } diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 771c89b13..735d3f687 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -247,6 +247,8 @@ impl Default for Prefs { pub struct Session { docs: Vec, active: Option, + /// An undoable command is running: the commands it runs are part of its undo step. + in_undo_step: bool, pub prefs: Prefs, pub cache: Arc, pub(crate) tool: Box, @@ -287,6 +289,7 @@ impl Session { Session { docs: vec![], active: None, + in_undo_step: false, prefs: Prefs::default(), cache: Arc::new(Cache::new()), tool: designcraft_tools::create("selection"), @@ -409,7 +412,19 @@ impl Session { self.stop_data_preview(); } let before = self.active().map(|d| (d.uid, d.doc.clone())); - let r = (spec.run)(self, params)?; + // What an undoable command runs is part of its undo step. + let nested = self.in_undo_step; + self.in_undo_step = nested || spec.undoable; + let ran = (spec.run)(self, params); + self.in_undo_step = nested; + let r = match ran { + Ok(r) => r, + Err(e) => { + // What it did before it failed can be undone too. + self.record_undo_step(spec, before, nested); + return Err(e); + } + }; // Undo, redo or deleting layers can take the active layer away: new objects would land on // a layer that isn't there (invisible, not exported). Fall back to the top layer. if let Some(st) = self.active_mut() @@ -432,20 +447,26 @@ impl Session { st.doc = Arc::new(d); } } - // Record undo if the document changed (and we're not previewing an interaction). + self.record_undo_step(spec, before, nested); + if spec.journal { + self.journal.push((id.to_string(), params.clone())); + } + Ok(r) + } + + /// Record undo if the document changed (and we're not previewing an interaction). A command + /// run by an undoable one leaves the step to it: one command is one undo step. + fn record_undo_step(&mut self, spec: &CommandSpec, before: Option<(u64, Arc)>, nested: bool) { if let (Some((uid, old)), Some(st)) = (before, self.active_mut()) && st.uid == uid && !Arc::ptr_eq(&old, &st.doc) && st.interaction.is_none() && spec.undoable + && !nested { let entry = HistoryEntry { label: spec.label.to_string(), doc: old, selection: st.selection.clone() }; push_undo(st, entry); } - if spec.journal { - self.journal.push((id.to_string(), params.clone())); - } - Ok(r) } /// Smart Text Reflow for the primary story: add threaded pages while it oversets; remove diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index c1032bccf..0d94c8660 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -75,6 +75,111 @@ fn tool_gesture_creates_one_undo_step() { assert!((it.visible_bounds().x0 - 36.0).abs() < 1e-6, "visible {:?} path {:?}", it.visible_bounds(), it.bounds()); } +/// A command that runs another one (Cut runs Clear) recorded an undo step for each, so the +/// second Undo did nothing. One command is one step, whatever it runs inside. +#[test] +fn a_command_that_runs_another_is_one_undo_step() { + let mut s = session(); + let one_step = |s: &mut Session, id: &str, p: Value, label: &str| { + let (before, steps) = { + let st = s.doc().unwrap(); + (st.doc.clone(), st.history.undo.len()) + }; + s.execute(id, &p).unwrap(); + let st = s.doc().unwrap(); + assert_ne!(st.doc, before, "{id} changes the document"); + assert_eq!(st.history.undo.len(), steps + 1, "{id}"); + assert_eq!(st.history.undo.last().unwrap().label, label); + let after = st.doc.clone(); + s.execute("edit.undo", &json!({})).unwrap(); + assert_eq!(s.doc().unwrap().doc, before, "one Undo takes {id} back"); + s.execute("edit.redo", &json!({})).unwrap(); + assert_eq!(s.doc().unwrap().doc, after, "one Redo brings {id} back"); + s.execute("edit.undo", &json!({})).unwrap(); + }; + // Pasting text runs Type. + let story = s.execute("frame.create", &json!({"rect": [36, 300, 300, 400], "content": "text", "text": "ab"})).unwrap()["story"].clone(); + s.execute("text.select", &json!({"story": story, "anchor": 2})).unwrap(); + one_step(&mut s, "edit.paste", json!({"text": "c"}), "Paste"); + let a = s.execute("frame.create", &json!({"rect": [36, 36, 136, 136]})).unwrap()["id"].clone(); + s.execute("frame.create", &json!({"rect": [100, 100, 200, 200]})).unwrap(); + for (id, label) in + [("edit.duplicate", "Duplicate"), ("object.bringToFront", "Bring to Front"), ("edit.cut", "Cut"), ("edit.pasteInPlace", "Paste in Place")] + { + s.execute("selection.set", &json!({"ids": [a]})).unwrap(); + one_step(&mut s, id, json!({}), label); + } + // A script that runs Cut is three commands deep and still one step. + let script = format!("selection.set {{\"ids\": [{a}]}}\nedit.cut {{}}"); + one_step(&mut s, "script.run", json!({"text": script}), "Run Script"); +} + +/// What a command did before it failed is an undo step too: a script that stopped at a bad +/// step left its earlier steps in the document with nothing to undo. +#[test] +fn a_command_that_fails_partway_can_be_undone() { + let mut s = session(); + let before = s.doc().unwrap().doc.clone(); + let script = "frame.create {\"rect\": [36, 36, 136, 136]}\nnope.command"; + assert!(s.execute("script.run", &json!({"text": script})).is_err()); + let st = s.doc().unwrap(); + assert_eq!(st.doc.spreads[0].items.len(), 1, "the first step ran"); + assert_eq!(st.history.undo.len(), 1); + s.execute("edit.undo", &json!({})).unwrap(); + assert_eq!(s.doc().unwrap().doc, before); + // The next command records its step as usual. + s.execute("frame.create", &json!({"rect": [36, 36, 136, 136]})).unwrap(); + assert_eq!(s.doc().unwrap().history.undo.len(), 1); +} + +/// The same for a command that fails after a command it ran changed the document: that one +/// leaves the step to it, so it has to record one. +#[test] +fn a_command_that_fails_after_the_one_it_ran_can_be_undone() { + let mut s = session(); + let empty = s.execute("frame.create", &json!({"rect": [36, 36, 136, 136], "content": "unassigned"})).unwrap()["id"].clone(); + let text = s.execute("frame.create", &json!({"rect": [36, 300, 300, 400], "content": "text", "text": "abc"})).unwrap()["id"].clone(); + s.execute("selection.set", &json!({"ids": [text]})).unwrap(); + let (before, steps) = { + let st = s.doc().unwrap(); + (st.doc.clone(), st.history.undo.len()) + }; + // Fill with Placeholder Text turns the empty frame into a text frame, then finds no such story. + assert!(s.execute("type.fillWithPlaceholder", &json!({"frame": empty, "story": 99999})).is_err()); + let st = s.doc().unwrap(); + assert_ne!(st.doc, before, "the frame was converted"); + assert_eq!(st.history.undo.len(), steps + 1); + assert_eq!(st.history.undo.last().unwrap().label, "Fill with Placeholder Text"); + s.execute("edit.undo", &json!({})).unwrap(); + assert_eq!(s.doc().unwrap().doc, before); +} + +/// A script's steps still record as it runs: an Undo step takes back the step before it, not +/// what was done before the script. The script stays one step. +#[test] +fn a_script_that_undoes_its_own_step_is_one_undo_step() { + let mut s = session(); + s.execute("frame.create", &json!({"rect": [36, 36, 136, 136]})).unwrap(); + let script = "frame.create {\"rect\": [200, 36, 300, 136]}\nframe.create {\"rect\": [400, 36, 500, 136]}\nedit.undo {}"; + s.execute("script.run", &json!({"text": script})).unwrap(); + let st = s.doc().unwrap(); + assert_eq!(st.doc.spreads[0].items.len(), 2, "the frame from before the script and the script's first"); + assert_eq!(st.history.undo.len(), 2); + s.execute("edit.undo", &json!({})).unwrap(); + assert_eq!(s.doc().unwrap().doc.spreads[0].items.len(), 1); +} + +/// A command that is no undo step itself (placing the caret) leaves the steps of the commands +/// it runs alone: turning the empty frame into a text frame can be undone. +#[test] +fn a_command_without_an_undo_step_keeps_the_steps_of_those_it_runs() { + let mut s = session(); + let id = s.execute("frame.create", &json!({"rect": [36, 36, 300, 200], "content": "unassigned"})).unwrap()["id"].clone(); + s.execute("text.placeCaret", &json!({"frame": id, "point": [50, 50]})).unwrap(); + let labels: Vec<&str> = s.doc().unwrap().history.undo.iter().map(|e| e.label.as_str()).collect(); + assert_eq!(labels, ["Create Frame", "Content"]); +} + #[test] fn shift_move_does_not_take_the_other_axis() { use designcraft_tools::{PointerEvent, PointerKind};