diff --git a/crates/engine/src/journal.rs b/crates/engine/src/journal.rs index 96cfe7ae3..230bcfba0 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,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) { 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 { + 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::()); + // 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::()); + } } diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 4c848ecbc..34a9c86da 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,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 = 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 2de23ea0c..713ad4b78 100644 --- a/crates/engine/src/tests_journal.rs +++ b/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::>(); + 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::>(); + 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::>(); + 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] diff --git a/crates/ui-egui/src/panels/actions.rs b/crates/ui-egui/src/panels/actions.rs index 5eb641217..c7f6974ae 100644 --- a/crates/ui-egui/src/panels/actions.rs +++ b/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::>()); + 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.