From 1565c96123a80d053ffc71bd36b920998c401d28 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Tue, 20 Oct 2026 19:49:43 -0700 Subject: [PATCH] Undo groups: a cancel keeps the journal entries made outside the canceled groups A cancel cut the journal back to where the first canceled group began. That also removed the commands run in other documents after it began, which the cancel does not undo, so a replay of the journal lost them. Each undo group now keeps the numbers of its own journal entries: the commands that start and end in its document and the commands that edit it. A cancel removes only those entries, and the later entries are numbered down to close the gaps (`Journal::remove`). An entry of a canceled group that also switched to another document, such as a batch that draws and then activates another document, is replaced by a `document.activate` of that document, so the commands after it replay in the same document. --- crates/engine/src/journal.rs | 39 ++++++++++++- crates/engine/src/lib.rs | 67 ++++++++++++++++++--- crates/engine/src/tests_journal.rs | 93 +++++++++++++++++++++++++++++- 3 files changed, 187 insertions(+), 12 deletions(-) diff --git a/crates/engine/src/journal.rs b/crates/engine/src/journal.rs index 96cfe7ae3..b57697d69 100644 --- a/crates/engine/src/journal.rs +++ b/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,6 +89,27 @@ 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(); + // Each entry that was let go already counts once. + let mut last = None; + let let_go = numbers.iter().take_while(|&&n| n < dropped).filter(|&&n| last.replace(n) != Some(n)).count(); + self.dropped = dropped.saturating_sub(let_go); + } /// Keep every entry from number `from` on (a recording in progress), or stop (`None`). pub fn hold(&mut self, from: Option) { self.held = from; @@ -194,4 +215,20 @@ 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(); + // 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()), (29_997, first - 1)); + 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::()); + } } diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 423f69f13..76ecc1ba0 100644 --- a/crates/engine/src/lib.rs +++ b/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>, - /// 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)>, /// The undo step's name; else the first edit's. label: Option, } @@ -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) { - 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, 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)> = 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,13 +2036,24 @@ impl Session { deselect_guides(&st.doc, &mut st.selection); } st.revision += 1; - // The journal goes back to where the first of the canceled groups began, whatever the order of the documents. - journal = Some(journal.map_or(g.journal, |len: usize| len.min(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 + && let Some(e) = self.journal.get_mut(n) + { + *e = ("document.activate".to_string(), serde_json::json!({ "index": index })); + } } + let gone: Vec = 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`). diff --git a/crates/engine/src/tests_journal.rs b/crates/engine/src/tests_journal.rs index a32cf801b..713ad4b78 100644 --- a/crates/engine/src/tests_journal.rs +++ b/crates/engine/src/tests_journal.rs @@ -343,8 +343,8 @@ 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 cuts the journal back -/// to where the first of them began, whichever document comes first in the list. +/// 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(); @@ -384,6 +384,95 @@ fn canceling_undo_groups_in_two_documents_drops_the_edits_of_both_from_the_journ 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::>(); + 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::>(); + 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::>(); + 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]