Merge pull request #3294 from dotCooCoo/fix/undo-group-cancel-cuts-the-earliest-journal-entry

Undo groups: a cancel drops only the canceled groups' entries from the journal

crates/engine/src/journal.rs

@@ -81,7 +81,7 @@ impl Journal {
         self.bytes += size;
         self.trim();
     }
-    /// Drop the entries from number `len` on (a cancelled undo group's).
+    /// Drop the entries from number `len` on.
     pub fn truncate(&mut self, len: usize) {
         let keep = len.saturating_sub(self.dropped);
         self.entries.truncate(keep);
@@ -89,11 +89,49 @@ impl Journal {
         self.bytes = self.sizes.iter().sum();
         self.dropped = self.dropped.min(len);
     }
+    /// Drop the entries numbered `numbers` (sorted, a number may repeat; the entries of canceled undo
+    /// groups). The entries after them are numbered down to close the gaps.
+    pub fn remove(&mut self, numbers: &[usize]) {
+        let dropped = self.dropped;
+        let gone = |i: usize| numbers.binary_search(&(dropped + i)).is_ok();
+        let mut i = 0;
+        self.entries.retain(|_| {
+            i += 1;
+            !gone(i - 1)
+        });
+        let mut i = 0;
+        self.sizes.retain(|_| {
+            i += 1;
+            !gone(i - 1)
+        });
+        self.bytes = self.sizes.iter().sum();
+        // How many of the numbers are below `n`, each counted once.
+        let below = |n: usize| {
+            let mut last = None;
+            numbers.iter().take_while(|&&m| m < n).filter(|&&m| last.replace(m) != Some(m)).count()
+        };
+        self.held = self.held.map(|h| h.saturating_sub(below(h)));
+        self.dropped = dropped.saturating_sub(below(dropped));
+    }
+    /// Put `e` in place of entry number `i`, if kept.
+    pub fn replace(&mut self, i: usize, e: JournalEntry) {
+        let Some(k) = i.checked_sub(self.dropped) else { return };
+        if let (Some(old), Some(size)) = (self.entries.get_mut(k), self.sizes.get_mut(k)) {
+            *size = entry_bytes(&e);
+            *old = e;
+            self.bytes = self.sizes.iter().sum();
+        }
+    }
     /// Keep every entry from number `from` on (a recording in progress), or stop (`None`).
     pub fn hold(&mut self, from: Option<usize>) {
         self.held = from;
         self.trim();
     }
+    /// The number the entries are kept from ([`Journal::hold`]). [`Journal::remove`] moves it down
+    /// with the entries it numbers down.
+    pub fn held(&self) -> Option<usize> {
+        self.held
+    }
 
     /// Let the oldest entries go while over the bounds (in batches: a quarter of the entry bound
     /// at a time, so a push doesn't shift the whole list every time).
@@ -194,4 +232,25 @@ mod tests {
         j.truncate(29_990);
         assert_eq!((j.len(), j.last().map(|e| e.1["dx"].clone())), (29_990, Some(json!(29_989))));
     }
+
+    #[test]
+    fn removed_entries_leave_no_gap_in_the_numbers() {
+        let mut j = Journal::default();
+        for i in 0..30_000 {
+            j.push(("object.move".into(), json!({"dx": i})));
+        }
+        let first = j.first_kept();
+        // A recording from entry 29,500 on: the entries removed before it move its start down.
+        j.hold(Some(29_500));
+        // One entry that was let go already and two kept ones, two of them given twice.
+        j.remove(&[first - 1, first - 1, 29_000, 29_000, 29_998]);
+        assert_eq!((j.len(), j.first_kept(), j.held()), (29_997, first - 1, Some(29_498)));
+        let dx = |j: &Journal, i: usize| j.get(i).map(|e| e.1["dx"].clone());
+        assert_eq!((dx(&j, 28_998), dx(&j, 28_999)), (Some(json!(28_999)), Some(json!(29_001))));
+        assert_eq!(j.last().map(|e| e.1["dx"].clone()), Some(json!(29_999)));
+        assert_eq!(j.bytes, j.sizes.iter().sum::<usize>());
+        // A replaced entry is counted at its new size.
+        j.replace(29_000, big(0));
+        assert_eq!(j.bytes, j.iter().map(entry_bytes).sum::<usize>());
+    }
 }

crates/engine/src/lib.rs

@@ -297,8 +297,10 @@ pub(crate) struct PreviewCopies {
 pub struct UndoGroup {
     /// The document before the group's first edit, as the undo step that edit recorded keeps it.
     first: Option<Arc<Document>>,
-    /// The journal's length when the group began: a cancelled group drops the entries after it.
-    journal: usize,
+    /// The numbers of the journal entries made in the group's document while it was open: a canceled
+    /// group drops them. An entry that changed the active document has the index of the document it
+    /// left active, and a canceled group turns it into that change.
+    journal: Vec<(usize, Option<usize>)>,
     /// The undo step's name; else the first edit's.
     label: Option<String>,
 }
@@ -1594,6 +1596,9 @@ impl Session {
             }
             (spec.run)(s, params)
         };
+        // The entries the command journals belong to the undo groups of the documents it edits, and to
+        // the group of the document it starts in when it ends there too.
+        let (doc, journal, grouped) = (self.active().map(|d| d.uid), self.journal.len(), self.undo_group_docs());
         let r = if self.depth == 0 { self.run_guarded(id, run) } else { self.run_nested(run) };
         let r = r?;
         if self.depth == 0 {
@@ -1604,6 +1609,7 @@ impl Session {
             self.journal.push((id.to_string(), p));
         }
         if self.depth == 0 {
+            self.journal_in_undo_groups(doc, &grouped, journal);
             self.after_command();
         }
         Ok(r)
@@ -1933,7 +1939,9 @@ impl Session {
         if perspective_again.is_some() {
             st.last_perspective = perspective_again;
         }
+        let (doc, journal, grouped) = (self.active().map(|d| d.uid), self.journal.len(), self.undo_group_docs());
         self.journal.push(preview);
+        self.journal_in_undo_groups(doc, &grouped, journal);
         Ok(())
     }
 
@@ -1972,18 +1980,48 @@ impl Session {
 
     /// [`Session::begin_undo_group`] with the undo step named `label` (else after its first edit).
     pub fn begin_named_undo_group(&mut self, label: Option<String>) {
-        let journal = self.journal.len();
         if let Some(st) = self.active_mut()
             && st.undo_group.is_none()
         {
-            st.undo_group = Some(UndoGroup { first: None, journal, label });
+            st.undo_group = Some(UndoGroup { first: None, journal: vec![], label });
+        }
+    }
+
+    /// The documents with an undo group open ([`DocState::uid`]), each with the document it holds.
+    fn undo_group_docs(&self) -> Vec<(u64, *const Document)> {
+        self.docs.iter().filter(|st| st.undo_group.is_some()).map(|st| (st.uid, Arc::as_ptr(&st.doc))).collect()
+    }
+
+    /// Note the journal entries from number `from` on in the undo groups they belong to: the group of
+    /// each document in `grouped` ([`Session::undo_group_docs`] before the entries) that they changed,
+    /// each group they opened and edited, and the group of the document with [`DocState::uid`] `doc`
+    /// when they started in it and it is still the active one. A cancel drops them. The last entry of a command that changed the
+    /// active document keeps the index of the document it left active.
+    fn journal_in_undo_groups(&mut self, doc: Option<u64>, grouped: &[(u64, *const Document)], from: usize) {
+        let to = self.journal.len();
+        let active = self.active().map(|d| d.uid);
+        let switched = if active == doc { None } else { self.active };
+        for st in &mut self.docs {
+            let Some(g) = &mut st.undo_group else { continue };
+            let started = doc == Some(st.uid) && active == doc;
+            // A group the entries opened has an edit of theirs once it has a first edit.
+            let edited = match grouped.iter().find(|&&(uid, _)| uid == st.uid) {
+                Some(&(_, d)) => !std::ptr::eq(d, Arc::as_ptr(&st.doc)),
+                None => g.first.is_some(),
+            };
+            if started || edited {
+                // A committed interaction inside the command noted its entry already.
+                let noted = g.journal.partition_point(|&(n, _)| n < from);
+                g.journal.truncate(noted);
+                g.journal.extend((from..to).map(|n| (n, switched.filter(|_| n + 1 == to))));
+            }
         }
     }
 
     /// Close the open undo groups: their edits stay one undo step or, `cancel`led (Escape), are
     /// undone and dropped from the journal.
     pub fn end_undo_group(&mut self, cancel: bool) {
-        let mut journal = None;
+        let mut journal: Vec<(usize, Option<usize>)> = vec![];
         for st in &mut self.docs {
             let Some(g) = st.undo_group.take() else { continue };
             // Only while the group's step is still the newest one.
@@ -1998,12 +2036,22 @@ impl Session {
                     deselect_guides(&st.doc, &mut st.selection);
                 }
                 st.revision += 1;
-                journal = Some(g.journal);
+                // The journal drops only the canceled groups' own entries and keeps the ones made in other documents meanwhile.
+                journal.extend(g.journal);
             }
         }
-        if let Some(len) = journal {
-            self.journal.truncate(len);
+        // A batch that edited two of the documents is in both groups.
+        journal.sort_unstable();
+        journal.dedup_by_key(|&mut (n, _)| n);
+        // An entry that changed the active document becomes that change, so the entries after it run
+        // in the same document when the journal is replayed.
+        for &(n, index) in &journal {
+            if let Some(index) = index {
+                self.journal.replace(n, ("document.activate".to_string(), serde_json::json!({ "index": index })));
+            }
         }
+        let gone: Vec<usize> = journal.into_iter().filter(|(_, index)| index.is_none()).map(|(n, _)| n).collect();
+        self.journal.remove(&gone);
     }
 
     /// Commands with enablement (for menus, palette, MCP `list_commands`).

crates/engine/src/tests_journal.rs

@@ -343,6 +343,136 @@ fn a_failed_batch_gives_back_the_highlighted_rows_and_the_current_layer() {
     assert_eq!(documents(&r), documents(&s));
 }
 
+/// Canceling undo groups open in two documents undoes the edits of both and drops the entries of
+/// both from the journal, whichever document comes first in the list.
+#[test]
+fn canceling_undo_groups_in_two_documents_drops_the_edits_of_both_from_the_journal() {
+    let mut s = Session::new();
+    s.execute("file.new", &json!({"created": null})).unwrap();
+    s.execute("file.new", &json!({"created": null})).unwrap();
+    let objects = |s: &Session| s.documents().iter().map(|d| d.doc.node_count()).collect::<Vec<_>>();
+    let before = objects(&s);
+    // The first document's group begins first, and the second document's group begins after it.
+    let journal = s.journal.len();
+    assert!(s.set_active(0));
+    s.begin_undo_group();
+    s.execute("shape.rectangle", &rect_step(10)["params"]).unwrap();
+    assert!(s.set_active(1));
+    s.begin_undo_group();
+    s.execute("shape.rectangle", &rect_step(50)["params"]).unwrap();
+    s.end_undo_group(true);
+    assert_eq!(objects(&s), before, "both documents' edits are undone");
+    assert_eq!(s.journal.len(), journal, "the journal holds neither group's edits");
+    assert_eq!(documents(&replayed(&s)), documents(&s));
+
+    // The commands do the same: neither group's begin nor its edit stays in the journal.
+    let journal = s.journal.len();
+    for (id, p) in [
+        ("document.activate", json!({"index": 0})),
+        ("edit.undoGroup.begin", json!({})),
+        ("shape.rectangle", rect_step(10)["params"].clone()),
+        ("document.activate", json!({"index": 1})),
+        ("edit.undoGroup.begin", json!({})),
+        ("shape.rectangle", rect_step(50)["params"].clone()),
+        ("edit.undoGroup.end", json!({"cancel": true})),
+    ] {
+        s.execute(id, &p).unwrap();
+    }
+    assert_eq!(objects(&s), before);
+    let ids = s.journal.since(journal).iter().map(|(id, _)| id.as_str()).collect::<Vec<_>>();
+    assert!(!ids.iter().any(|id| matches!(*id, "edit.undoGroup.begin" | "shape.rectangle")), "{ids:?}");
+    assert_eq!(ids.last(), Some(&"edit.undoGroup.end"));
+}
+
+/// A cancel drops only the canceled groups' own entries from the journal: an edit made in another
+/// document while a group was open stays in that document and in the journal.
+#[test]
+fn canceling_undo_groups_keeps_the_journal_entries_made_outside_them() {
+    let mut s = Session::new();
+    s.execute("file.new", &json!({"created": null})).unwrap();
+    s.execute("file.new", &json!({"created": null})).unwrap();
+    let objects = |s: &Session| s.documents().iter().map(|d| d.doc.node_count()).collect::<Vec<_>>();
+    let before = objects(&s);
+    // The second document's rectangle at 30 is drawn between the two groups and belongs to neither.
+    for (id, p) in [
+        ("document.activate", json!({"index": 0})),
+        ("edit.undoGroup.begin", json!({})),
+        ("shape.rectangle", rect_step(10)["params"].clone()),
+        ("document.activate", json!({"index": 1})),
+        ("shape.rectangle", rect_step(30)["params"].clone()),
+        ("edit.undoGroup.begin", json!({})),
+        ("shape.rectangle", rect_step(50)["params"].clone()),
+        ("edit.undoGroup.end", json!({"cancel": true})),
+    ] {
+        s.execute(id, &p).unwrap();
+    }
+    assert_eq!(objects(&s), vec![before[0], before[1] + 1], "only the groups' edits are undone");
+    let ids = s.journal.iter().map(|(id, _)| id.as_str()).collect::<Vec<_>>();
+    assert_eq!(ids.iter().filter(|id| **id == "shape.rectangle").count(), 1, "{ids:?}");
+    let r = replayed(&s);
+    assert_eq!(documents(&r), documents(&s));
+    assert_eq!(r.active_index(), s.active_index());
+
+    // With one group open, a rectangle drawn in the other document stays in the journal too. A live
+    // drag committed in the group goes with the group, and one committed in the other document stays.
+    let before = objects(&s);
+    let opacity = |s: &mut Session, opacity: i32| {
+        s.begin_interaction("Opacity").unwrap();
+        s.preview("transparency.set", &json!({ "opacity": opacity })).unwrap();
+        s.commit_interaction().unwrap();
+    };
+    for (id, p) in
+        [("document.activate", json!({"index": 0})), ("edit.undoGroup.begin", json!({})), ("shape.rectangle", rect_step(10)["params"].clone())]
+    {
+        s.execute(id, &p).unwrap();
+    }
+    opacity(&mut s, 50);
+    for (id, p) in [("document.activate", json!({"index": 1})), ("shape.rectangle", rect_step(70)["params"].clone())] {
+        s.execute(id, &p).unwrap();
+    }
+    opacity(&mut s, 40);
+    for (id, p) in [("document.activate", json!({"index": 0})), ("edit.undoGroup.end", json!({"cancel": true}))] {
+        s.execute(id, &p).unwrap();
+    }
+    assert_eq!(objects(&s), vec![before[0], before[1] + 1]);
+    let opacities = s.journal.iter().filter(|(id, _)| id == "transparency.set").map(|(_, p)| p["opacity"].clone()).collect::<Vec<_>>();
+    assert_eq!(opacities, vec![json!(40)]);
+    let r = replayed(&s);
+    assert_eq!(documents(&r), documents(&s));
+    assert_eq!(r.active_index(), s.active_index());
+
+    // A batch that edits the group's document and then switches documents goes with the group, and
+    // only its switch stays, so the rectangle drawn after it replays in the second document.
+    let before = objects(&s);
+    let steps = json!([rect_step(90), {"command": "document.activate", "params": {"index": 1}}]);
+    for (id, p) in [
+        ("document.activate", json!({"index": 0})),
+        ("edit.undoGroup.begin", json!({})),
+        ("command.batch", json!({ "commands": steps })),
+        ("shape.rectangle", rect_step(110)["params"].clone()),
+        ("edit.undoGroup.end", json!({"cancel": true})),
+    ] {
+        s.execute(id, &p).unwrap();
+    }
+    assert_eq!(objects(&s), vec![before[0], before[1] + 1]);
+    assert!(!s.journal.iter().any(|(id, _)| id == "command.batch"));
+    let r = replayed(&s);
+    assert_eq!(documents(&r), documents(&s));
+    assert_eq!(r.active_index(), s.active_index());
+
+    // So does a batch that opens the group itself before it switches documents.
+    let before = objects(&s);
+    s.execute("document.activate", &json!({"index": 0})).unwrap();
+    let steps = json!([{"command": "edit.undoGroup.begin"}, rect_step(130), {"command": "document.activate", "params": {"index": 1}}]);
+    s.execute("command.batch", &json!({ "commands": steps })).unwrap();
+    s.end_undo_group(true);
+    assert_eq!(objects(&s), before);
+    assert!(!s.journal.iter().any(|(id, _)| id == "command.batch"));
+    let r = replayed(&s);
+    assert_eq!(documents(&r), documents(&s));
+    assert_eq!(r.active_index(), s.active_index());
+}
+
 /// The edits made in an undo group (a scrubbed numeric field, #400) are one undo step, journaled
 /// one by one; a cancelled group undoes them and leaves the journal as it was.
 #[test]

crates/ui-egui/src/panels/actions.rs

@@ -118,6 +118,8 @@ pub fn show(app: &mut VectorcraftApp, ui: &mut egui::Ui) {
         if widgets::icon_button(ui, "square", tl!("Stop Playing/Recording"), false, 24.0).clicked()
             && let Some((set, name, start)) = app.ui.recording.take()
         {
+            // A canceled undo group numbers the steps down when it drops entries made before them.
+            let start = app.session.journal.held().unwrap_or(start);
             let steps = app.session.journal_for_action(start);
             app.session.journal.hold(None);
             if let Some(s) = app.ui.action_sets.get_mut(set) {
@@ -211,6 +213,37 @@ mod tests {
         assert_eq!(names(&app), before[1..], "nothing selected after a delete: nothing more goes");
     }
 
+    /// A recording keeps its steps when a canceled undo group drops journal entries made before the
+    /// recording began.
+    #[test]
+    fn a_recording_keeps_its_steps_when_a_canceled_undo_group_drops_earlier_entries() {
+        use super::super::tests_appearance::{click, frame_events};
+        let mut app = VectorcraftApp::new(Session::new(), Default::default());
+        let rect = |x: i32| json!({"x": x, "y": 10, "width": 20, "height": 20});
+        for (id, p) in [
+            ("file.new", json!({"width": 200, "height": 100})),
+            ("file.new", json!({"width": 200, "height": 100})),
+            ("document.activate", json!({"index": 0})),
+            ("edit.undoGroup.begin", json!({})),
+            ("shape.rectangle", rect(10)),
+        ] {
+            app.run(id, p).unwrap();
+        }
+        let ctx = egui::Context::default();
+        frame_events(&ctx, &mut app, vec![], show);
+        // The bottom bar's 24-point buttons, left to right: stop, record, play, delete.
+        let mut bar = crate::toolbar::tests::widget_rects(&ctx, egui::vec2(24.0, 24.0));
+        bar.sort_by(|a, b| a.left().total_cmp(&b.left()));
+        let [stop, record, _, _] = bar[..] else { panic!("the bottom bar: {bar:?}") };
+        click(&ctx, &mut app, record.center(), show);
+        for (id, p) in [("document.activate", json!({"index": 1})), ("shape.rectangle", rect(50)), ("edit.undoGroup.end", json!({"cancel": true}))] {
+            app.run(id, p).unwrap();
+        }
+        click(&ctx, &mut app, stop.center(), show);
+        let steps = app.ui.action_sets[0].actions.last().map(|a| a.steps.iter().map(|(c, _)| c.clone()).collect::<Vec<_>>());
+        assert_eq!(steps, Some(vec!["document.activate".to_string(), "shape.rectangle".into(), "edit.undoGroup.end".into()]));
+    }
+
     /// Actions recorded with Align to Artboard and Paste in Place, and the default Center on
     /// Artboard, play on the artboard active when they play, not on the one active when they were
     /// recorded.