Merge #411: Undo: a command that runs another is one undo step

crates/engine/src/cmd/scripts.rs

@@ -8,7 +8,9 @@ use crate::Result;
 fn run(s: &mut crate::Session, p: &Value) -> Result<Value> {
     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);

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);
+    }
 }

crates/engine/src/lib.rs

@@ -271,6 +271,8 @@ impl Default for Prefs {
 pub struct Session {
     docs: Vec<DocState>,
     active: Option<usize>,
+    /// 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<Cache>,
     pub(crate) tool: Box<dyn designcraft_tools::Tool>,
@@ -321,6 +323,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"),
@@ -463,7 +466,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()
@@ -486,20 +501,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<Document>)>, 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

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};