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]