Merge pull request #104 from storytold/pr/m4-ft3

M14.4: flaky paste fuzz test and batch journaling

crates/engine/src/cmd/fileio/batch.rs

@@ -1,37 +1,89 @@
-//! `command.batch`: several commands as one undo step.
+//! `command.batch`: several commands as one undo step (in each document they edit).
 
 use serde_json::{Value, json};
 
 use super::super::*;
-use crate::EngineError;
+use crate::{DocState, EngineError};
 
 pub(super) fn batch(s: &mut Session, p: &Value) -> Result<Value> {
-    let cmds = p.get("commands").and_then(Value::as_array).ok_or_else(|| bad("command.batch", "missing commands"))?.clone();
+    let mut cmds = p.get("commands").and_then(Value::as_array).ok_or_else(|| bad("command.batch", "missing commands"))?.clone();
     let label = str_param(p, "label").unwrap_or("Batch").to_string();
-    s.begin_interaction(&label)?;
-    // Session-level state a step may change (paint defaults, drawing mode) rolls back too.
-    let saved = (s.paint.clone(), s.fill_active, s.draw_mode, s.draw_inside, s.clipboard.clone());
-    let mut results = vec![];
-    for c in &cmds {
-        let id = c.get("command").and_then(Value::as_str).unwrap_or("");
-        if id == "command.batch" {
-            s.cancel_interaction()?;
-            return Err(bad("command.batch", "batches cannot nest"));
+    // What an error rolls back besides the documents: session-level state a step may change
+    // (paint defaults, drawing mode, clipboard, the Untitled-N count).
+    let saved = (s.paint.clone(), s.fill_active, s.draw_mode, s.draw_inside, s.clipboard.clone(), s.untitled_counter);
+    let (start, active): (Vec<u64>, _) = (s.docs.iter().map(|d| d.uid).collect(), s.active);
+    s.batch_stash = Some(vec![]);
+    let r = run_steps(s, &label, &mut cmds);
+    let stash = s.batch_stash.take().unwrap_or_default();
+    let results = match r {
+        Ok(results) => results,
+        Err(e) => {
+            roll_back(s, &start, stash);
+            s.active = active;
+            (s.paint, s.fill_active, s.draw_mode, s.draw_inside, s.clipboard, s.untitled_counter) = saved;
+            s.pending_paint = None;
+            return Err(e);
         }
-        let params = c.get("params").cloned().unwrap_or(json!({}));
-        match s.execute(id, &params) {
-            Ok(v) => results.push(v),
-            Err(e) => {
-                s.cancel_interaction()?;
-                (s.paint, s.fill_active, s.draw_mode, s.draw_inside, s.clipboard) = saved;
-                return Err(EngineError::Other(format!("batch step {} (`{id}`) failed: {e}", results.len())));
-            }
+    };
+    // Each document the steps edited keeps them as one undo step.
+    s.remember_pending_paint();
+    for st in &mut s.docs {
+        if let Some(it) = st.interaction.take() {
+            st.keep_interaction(it);
         }
     }
-    // Commit as one undo step (the interaction's "preview" is the batch itself).
-    if let Some(it) = s.doc_mut()?.interaction.as_mut() {
-        it.preview = Some(("command.batch".into(), p.clone()));
+    // The batch journals itself, whichever document it ends in.
+    let mut entry = p.clone();
+    if let Some(o) = entry.as_object_mut() {
+        o.insert("commands".into(), Value::Array(cmds));
     }
-    s.commit_interaction()?;
+    s.journal.push(("command.batch".into(), entry));
     Ok(json!({ "results": results }))
 }
+
+/// Run the steps → their results. Each runs inside an interaction of the document active when it
+/// starts (one for all the steps in that document), so a step may open, switch or close documents.
+fn run_steps(s: &mut Session, label: &str, cmds: &mut [Value]) -> Result<Vec<Value>> {
+    let mut results = Vec::with_capacity(cmds.len());
+    for c in cmds {
+        let id = c.get("command").and_then(Value::as_str).unwrap_or("").to_string();
+        if id == "command.batch" {
+            return Err(bad("command.batch", "batches cannot nest"));
+        }
+        if s.active().is_some() {
+            s.begin_interaction(label)?;
+        }
+        let params = c.get("params").cloned().unwrap_or(json!({}));
+        let (v, noted) =
+            s.execute_step(&id, &params).map_err(|e| EngineError::Other(format!("batch step {} (`{id}`) failed: {e}", results.len())))?;
+        // The step's journal entry records what it took from the preferences or the clock (a
+        // save's date, a new document's), as a command of its own would.
+        if noted != params
+            && let Some(c) = c.as_object_mut()
+        {
+            c.insert("params".into(), noted);
+        }
+        results.push(v);
+    }
+    Ok(results)
+}
+
+/// The documents as they were before the batch (`start`: their uids in order): the ones it opened
+/// close, the ones it closed or replaced (`stash`) come back, and each one's interaction is undone.
+fn roll_back(s: &mut Session, start: &[u64], mut stash: Vec<DocState>) {
+    let mut open = std::mem::take(&mut s.docs);
+    for &uid in start {
+        // The newest revision the document reached, so the restored one redraws.
+        let newest = open.iter().chain(&stash).filter(|d| d.uid == uid).map(|d| d.revision).max().unwrap_or_default();
+        // Its state before the batch: the first one a step closed or replaced, else the open one.
+        let st = match stash.iter().position(|d| d.uid == uid) {
+            Some(i) => Some(stash.remove(i)),
+            None => open.iter().position(|d| d.uid == uid).map(|i| open.remove(i)),
+        };
+        if let Some(mut st) = st {
+            st.undo_interaction();
+            st.revision = newest.saturating_add(1);
+            s.docs.push(st);
+        }
+    }
+}

crates/engine/src/cmd/fileio/mod.rs

@@ -108,7 +108,7 @@ pub fn specs() -> Vec<CommandSpec> {
             has_doc,
             screens::export_for_screens
         ),
-        cmd!(query "command.batch", "Batch", [], None, "{label?, commands: [{command, params}]} run several commands as ONE undo step; stops at the first error and rolls back", has_doc, batch::batch),
+        cmd!(query "command.batch", "Batch", [], None, "{label?, commands: [{command, params}]} run several commands as ONE undo step (one in each document they edit: steps may open, switch, close or revert documents); stops at the first error and rolls everything back, documents included", has_doc, batch::batch),
         cmd!(
             query "document.formats",
             "File Formats",

crates/engine/src/lib.rs

@@ -168,6 +168,24 @@ impl DocState {
     pub fn mark_saved_as(&mut self, snapshot: &Arc<Document>) {
         self.saved_doc = snapshot.clone();
     }
+    /// Keep what interaction `it` (taken from this document) changed, as one undo step.
+    pub(crate) fn keep_interaction(&mut self, it: Interaction) {
+        if !Arc::ptr_eq(&it.doc, &self.doc) {
+            self.history.undo.push(HistoryEntry { label: it.label, doc: it.doc, selection: it.selection });
+            self.history.redo.clear();
+            self.revision += 1;
+        }
+    }
+    /// End the interaction in progress, undoing what it changed.
+    pub(crate) fn undo_interaction(&mut self) {
+        if let Some(it) = self.interaction.take() {
+            self.doc = it.doc;
+            self.selection = it.selection;
+            self.active_layer = it.active_layer;
+            self.isolation = it.isolation;
+            self.revision += 1;
+        }
+    }
     pub fn title(&self) -> String {
         let name = self
             .path
@@ -207,6 +225,27 @@ pub const MAX_COORD: f64 = 4.0e6;
 /// ([`Session::journal_for_action`]).
 const REPLAY_ONLY: [(&str, &str); 3] = [("file.new", "created"), ("document.save", "modified"), ("file.saveAs", "modified")];
 
+/// `p`, the params of command `id`, without its [`REPLAY_ONLY`] values (a batch: its steps').
+fn strip_replay_only(id: &str, p: &mut Value) {
+    let strip = |id: &str, p: &mut Value| {
+        if let Value::Object(m) = p {
+            m.retain(|k, _| !REPLAY_ONLY.iter().any(|&(c, key)| c == id && key == k));
+        }
+    };
+    strip(id, p);
+    // Batches don't nest: one level of steps.
+    if id == "command.batch"
+        && let Some(Value::Array(steps)) = p.get_mut("commands")
+    {
+        for step in steps {
+            let id = step.get("command").and_then(Value::as_str).unwrap_or_default().to_string();
+            if let Some(p) = step.get_mut("params") {
+                strip(&id, p);
+            }
+        }
+    }
+}
+
 /// Cheap sanity check after an edit: artboards and the objects just touched (the selection) must
 /// have finite, in-range geometry, so saved files always reload and renderers never see NaN/∞.
 fn doc_sane(d: &Document, sel: &Selection) -> bool {
@@ -587,6 +626,12 @@ pub struct Session {
     /// Parameters the running top-level command resolved from the preferences or the clock, added
     /// to its journal entry so a replay does the same ([`Session::note_journal`]).
     journal_note: serde_json::Map<String, Value>,
+    /// The depth of the command whose values [`Session::note_journal`] keeps: 1 (the top-level
+    /// command), or a step's while a batch runs it ([`Session::execute_step`]).
+    note_depth: u32,
+    /// While `command.batch` runs: the documents its steps closed or replaced (Revert), which an
+    /// error brings back; `None` otherwise.
+    pub(crate) batch_stash: Option<Vec<DocState>>,
 }
 
 impl Default for Session {
@@ -624,6 +669,8 @@ impl Session {
             style_libraries: Default::default(),
             recent_urls: vec![],
             journal_note: Default::default(),
+            note_depth: 1,
+            batch_stash: None,
         }
     }
 
@@ -658,7 +705,11 @@ impl Session {
         let view = self.last_view;
         let acts = self.with_tool_cx(view, |t, cx| t.deactivate(cx));
         let _ = self.apply_actions(acts);
-        let _ = self.cancel_interaction();
+        // A batch leaves its interaction open in the document it leaves, to keep or roll back with
+        // the rest of the batch; anything else in progress (a drag) is cancelled.
+        if self.batch_stash.is_none() {
+            let _ = self.cancel_interaction();
+        }
         self.tool = vectorcraft_tools::create(self.tool.id());
     }
     pub fn set_active(&mut self, index: usize) -> bool {
@@ -729,7 +780,10 @@ impl Session {
         st.save_options = old.save_options.clone();
         st.converted = old.converted;
         st.view = old.view.clone();
-        self.docs[index] = st;
+        let old = std::mem::replace(&mut self.docs[index], st);
+        if let Some(stash) = &mut self.batch_stash {
+            stash.push(old);
+        }
         true
     }
     pub fn close_document(&mut self, index: usize) -> bool {
@@ -737,7 +791,10 @@ impl Session {
             return false;
         }
         self.reset_tool_for_doc_switch();
-        self.docs.remove(index);
+        let old = self.docs.remove(index);
+        if let Some(stash) = &mut self.batch_stash {
+            stash.push(old);
+        }
         self.active = if self.docs.is_empty() { None } else { Some(index.min(self.docs.len() - 1)) };
         true
     }
@@ -752,6 +809,8 @@ impl Session {
         // be recorded twice and replay differently).
         if self.depth == 0 {
             self.journal_note.clear();
+            self.note_depth = 1;
+            self.batch_stash = None;
         }
         let r = if self.depth == 0 { self.run_guarded(id, |s| (spec.run)(s, params)) } else { self.run_nested(|s| (spec.run)(s, params)) };
         let r = r?;
@@ -766,13 +825,26 @@ impl Session {
     }
 
     /// Record `key: value` in the running top-level command's journal entry (or its interaction's
-    /// preview) unless its params give `key`: a value it resolved from the preferences or the clock.
+    /// preview, or its step in a batch) unless its params give `key`: a value it resolved from the
+    /// preferences or the clock.
     pub fn note_journal(&mut self, key: &str, value: Value) {
-        if self.depth == 1 {
+        if self.depth == self.note_depth {
             self.journal_note.insert(key.to_string(), value);
         }
     }
 
+    /// Run `id` as a step of the running command, which journals its steps (`command.batch`):
+    /// → the step's result and its params with what it noted ([`Session::note_journal`]), for the
+    /// step in the running command's journal entry.
+    pub(crate) fn execute_step(&mut self, id: &str, params: &Value) -> Result<(Value, Value)> {
+        let outer = (std::mem::take(&mut self.journal_note), self.note_depth);
+        self.note_depth = self.depth + 1;
+        let r = self.execute(id, params);
+        let noted = self.noted(params);
+        (self.journal_note, self.note_depth) = outer;
+        Ok((r?, noted))
+    }
+
     /// The journal from entry `start` on, as an action records it: without the params that only
     /// pin a replay to the original run ([`REPLAY_ONLY`]), so playing the action later acts now.
     pub fn journal_for_action(&self, start: usize) -> Vec<(String, Value)> {
@@ -781,11 +853,7 @@ impl Session {
             .skip(start)
             .cloned()
             .map(|(id, mut p)| {
-                if let Value::Object(m) = &mut p {
-                    for (_, key) in REPLAY_ONLY.iter().filter(|(c, _)| *c == id) {
-                        m.remove(*key);
-                    }
-                }
+                strip_replay_only(&id, &mut p);
                 (id, p)
             })
             .collect()
@@ -934,17 +1002,11 @@ impl Session {
     }
 
     pub fn commit_interaction(&mut self) -> Result<()> {
-        if let Some(p) = self.pending_paint.take() {
-            self.remember_paint_now(&p);
-        }
+        self.remember_pending_paint();
         let st = self.doc_mut()?;
-        let Some(it) = st.interaction.take() else { return Ok(()) };
-        let Some(preview) = it.preview else { return Ok(()) };
-        if !Arc::ptr_eq(&it.doc, &st.doc) {
-            st.history.undo.push(HistoryEntry { label: it.label, doc: it.doc, selection: it.selection });
-            st.history.redo.clear();
-            st.revision += 1;
-        }
+        let Some(mut it) = st.interaction.take() else { return Ok(()) };
+        let Some(preview) = it.preview.take() else { return Ok(()) };
+        st.keep_interaction(it);
         if preview.0 == "object.transform" {
             let m = cmd::matrix_param(&preview.1, "matrix");
             let copy = preview.1.get("copy").and_then(Value::as_bool).unwrap_or(false);
@@ -956,16 +1018,16 @@ impl Session {
         Ok(())
     }
 
+    /// Remember the paint a live preview applied (as its interaction is kept).
+    pub(crate) fn remember_pending_paint(&mut self) {
+        if let Some(p) = self.pending_paint.take() {
+            self.remember_paint_now(&p);
+        }
+    }
+
     pub fn cancel_interaction(&mut self) -> Result<()> {
         self.pending_paint = None;
-        let st = self.doc_mut()?;
-        if let Some(it) = st.interaction.take() {
-            st.doc = it.doc;
-            st.selection = it.selection;
-            st.active_layer = it.active_layer;
-            st.isolation = it.isolation;
-            st.revision += 1;
-        }
+        self.doc_mut()?.undo_interaction();
         Ok(())
     }
 

crates/engine/src/tests_journal.rs

@@ -97,3 +97,137 @@ fn a_save_records_its_date_and_replays_it() {
     assert_eq!(r.doc().unwrap().doc.metadata, s.doc().unwrap().doc.metadata);
     assert!(s.journal_for_action(0).iter().all(|(_, p)| p.get("modified").is_none() && p.get("created").is_none()));
 }
+
+/// The document `s` ends on after replaying the journal of `s` in a new session.
+fn replayed(s: &Session) -> Session {
+    let mut r = Session::new();
+    for (id, p) in &s.journal {
+        r.execute(id, p).unwrap();
+    }
+    r
+}
+
+#[test]
+fn a_save_in_a_batch_records_its_date_and_replays_it() {
+    let path = vectorcraft_testkit::temp_dir("journal-batch").join("a.vectorcraft").to_string_lossy().into_owned();
+    let mut s = Session::new();
+    s.execute("file.new", &json!({"created": 1_000_000_000})).unwrap();
+    let rect = json!({"x": 10, "y": 10, "width": 50, "height": 40});
+    let steps = json!([{"command": "shape.rectangle", "params": rect}, {"command": "document.save", "params": {"path": path}}]);
+    s.execute("command.batch", &json!({ "commands": steps })).unwrap();
+    let modified = s.doc().unwrap().doc.metadata.modified;
+    assert!(modified.is_some(), "the save in the batch dates the document");
+    // The step's entry records the date it used; steps that take nothing from elsewhere stay as given.
+    let (id, p) = s.journal.last().unwrap().clone();
+    assert_eq!(id, "command.batch");
+    assert_eq!(p["commands"][0], steps[0]);
+    assert_eq!(p["commands"][1]["params"], json!({"path": path, "modified": modified}));
+    // As if the original run happened at another time: the replay lands on the recorded date.
+    s.journal.last_mut().unwrap().1["commands"][1]["params"]["modified"] = json!(1_234_567_890);
+    let r = replayed(&s);
+    assert_eq!(r.doc().unwrap().doc.metadata.modified, Some(1_234_567_890));
+    assert_eq!(r.doc().unwrap().doc.metadata.created, Some(1_000_000_000));
+    assert_eq!(r.doc().unwrap().doc.node_count(), s.doc().unwrap().doc.node_count());
+    // An action leaves the step's date out, so playing it later dates the document then.
+    let action = s.journal_for_action(0);
+    assert_eq!(action[1].1["commands"][1]["params"], json!({ "path": path }));
+    assert_eq!(action[1].1["commands"][0], steps[0]);
+}
+
+#[test]
+fn a_batch_step_records_what_it_took_from_the_preferences() {
+    let mut s = Session::new();
+    s.execute("file.new", &json!({"created": null})).unwrap();
+    s.prefs.scale_strokes = true;
+    let steps = json!([
+        {"command": "shape.rectangle", "params": {"x": 10, "y": 10, "width": 50, "height": 40}},
+        {"command": "stroke.set", "params": {"weight": 2}},
+        {"command": "object.scale", "params": {"sx": 300}},
+    ]);
+    s.execute("command.batch", &json!({ "commands": steps })).unwrap();
+    let p = &s.journal.last().unwrap().1;
+    assert_eq!(p["commands"][2]["params"], json!({"sx": 300, "strokes": true, "corners": false}));
+    // Replayed with other preferences, the scale still scales the stroke.
+    let r = replayed(&s);
+    assert!(!r.prefs.scale_strokes);
+    let doc = |s: &Session| serde_json::to_value(&*s.doc().unwrap().doc).unwrap();
+    assert_eq!(doc(&r), doc(&s));
+}
+
+/// Every open document of `s`, as JSON.
+fn documents(s: &Session) -> Vec<serde_json::Value> {
+    s.documents().iter().map(|d| serde_json::to_value(&*d.doc).unwrap()).collect()
+}
+
+fn rect_step(x: i32) -> serde_json::Value {
+    json!({"command": "shape.rectangle", "params": {"x": x, "y": 10, "width": 20, "height": 20}})
+}
+
+#[test]
+fn a_batch_that_opens_a_new_document_is_journaled_and_replays() {
+    let mut s = Session::new();
+    s.execute("file.new", &json!({"created": null})).unwrap();
+    let steps = json!([rect_step(10), {"command": "file.new", "params": {"width": 300, "height": 200}}, rect_step(50), rect_step(90)]);
+    s.execute("command.batch", &json!({"label": "Two documents", "commands": steps})).unwrap();
+    // The steps before the new document stay in the first one and the rest go in the new one: one
+    // undo step in each, nothing left in progress.
+    assert_eq!(s.documents().len(), 2);
+    assert_eq!(s.active_index(), Some(1));
+    for (st, objects) in s.documents().iter().zip([1, 2]) {
+        assert_eq!(st.doc.node_count(), 1 + objects);
+        assert_eq!(st.history.undo.iter().map(|h| h.label.as_str()).collect::<Vec<_>>(), ["Two documents"]);
+        assert!(st.interaction.is_none());
+    }
+    // The batch is journaled, with the new document's date, and replays to the same documents.
+    let (id, p) = s.journal.last().unwrap().clone();
+    assert_eq!(id, "command.batch");
+    assert!(p["commands"][1]["params"]["created"].is_i64(), "{p}");
+    let r = replayed(&s);
+    assert_eq!(documents(&r), documents(&s));
+    assert_eq!(r.active_index(), s.active_index());
+    // Undo in the new document takes its part of the batch back.
+    s.execute("edit.undo", &json!({})).unwrap();
+    assert_eq!(s.doc().unwrap().doc.node_count(), 1);
+}
+
+#[test]
+fn a_failed_batch_closes_the_documents_it_opened_and_restores_the_ones_it_closed() {
+    let mut s = Session::new();
+    s.execute("file.new", &json!({"created": null})).unwrap();
+    s.execute("file.new", &json!({"created": null})).unwrap();
+    s.execute("shape.ellipse", &json!({"x": 0, "y": 0, "width": 30, "height": 30})).unwrap();
+    let (before, active, journal, next) = (documents(&s), s.active_index(), s.journal.len(), s.peek_untitled());
+    let steps = json!([
+        rect_step(10),
+        {"command": "file.close", "params": {}},
+        rect_step(30),
+        {"command": "file.new", "params": {}},
+        rect_step(50),
+        {"command": "no.such.command", "params": {}},
+    ]);
+    let e = s.execute("command.batch", &json!({ "commands": steps })).unwrap_err();
+    assert!(e.to_string().contains("batch step 5"), "{e}");
+    assert_eq!(documents(&s), before);
+    assert_eq!(s.active_index(), active);
+    assert_eq!(s.journal.len(), journal, "a failed batch isn't journaled");
+    assert_eq!(s.peek_untitled(), next, "the next new document keeps its name");
+    assert!(s.documents().iter().all(|d| d.interaction.is_none()));
+    assert_eq!(s.documents()[1].history.undo.len(), 1, "only the ellipse drawn before the batch");
+}
+
+#[test]
+fn a_failed_batch_brings_back_a_document_it_reverted() {
+    let path = vectorcraft_testkit::temp_dir("journal-batch").join("revert.vectorcraft").to_string_lossy().into_owned();
+    let mut s = Session::new();
+    s.execute("file.new", &json!({"created": null})).unwrap();
+    s.execute("document.save", &json!({"path": path, "modified": null})).unwrap();
+    s.execute("shape.ellipse", &json!({"x": 0, "y": 0, "width": 30, "height": 30})).unwrap();
+    let (before, revision) = (documents(&s), s.doc().unwrap().revision);
+    let steps = json!([rect_step(10), {"command": "file.revert", "params": {}}, rect_step(30), {"command": "no.such.command"}]);
+    assert!(s.execute("command.batch", &json!({ "commands": steps })).is_err());
+    assert_eq!(documents(&s), before, "the unsaved ellipse is back");
+    let st = s.doc().unwrap();
+    assert_eq!(st.history.undo.len(), 1);
+    assert!(st.is_dirty() && st.interaction.is_none());
+    assert!(st.revision > revision, "the restored document redraws");
+}

crates/engine/tests/import_fuzz.rs

@@ -727,7 +727,9 @@ fn paste_from_elsewhere(cmd: &str, p: Value) -> Result<(), TestCaseError> {
         let mut s = vectorcraft_engine::Session::new();
         s.execute("file.new", &json!({"width": 100, "height": 100})).unwrap();
         if s.execute(cmd, &p).is_ok() {
-            s.execute("edit.paste", &json!({"center": [50, 50]})).unwrap();
+            // Paste may refuse what it was given, e.g. a damaged PDF whose art spans more than the
+            // canvas ("result would exceed the canvas"): an error is a fine outcome, a panic isn't.
+            let _ = s.execute("edit.paste", &json!({"center": [50, 50]}));
         }
     });
     prop_assert!(r.is_ok(), "{cmd}: panicked: {:?}", r.err());

docs/mcp.md

@@ -979,7 +979,11 @@ creation date) as text chunks, and SVG export writes the description as `<desc>`
 all of it as Dublin Core. `file.new {created}` and a save to a file (`document.save` or `file.saveAs` with
 `{path, modified}`) use the given date (Unix seconds, or `null` for none) instead of now. Their journal entries always
 record the date they used, so replaying the journal makes the same document whenever it runs (an action recorded in
-the Actions panel leaves the date out: playing it later dates the document then).
+the Actions panel leaves the date out: playing it later dates the document then). As a step of `command.batch`, a
+command records the date it used (and any value it takes from the preferences) in its step of the batch's journal entry.
+A batch may open, switch, close or revert documents: each document it edits gets one undo step, the batch is
+journaled whichever document it ends in, and an error rolls every document back (the ones it opened close, the ones
+it closed or reverted come back).
 
 ```json
 {"name":"run_command","arguments":{"command":"file.info","params":{"author":"Ada","keywords":"poster, fair","rating":4,"copyrightStatus":"copyrighted","copyrightNotice":"© 2026 Ada"}}}