From ae41ddb9a503bc52daab28d38e75bfebf5a55c19 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Tue, 20 Oct 2026 11:37:56 -0700 Subject: [PATCH] Undo groups: library styles and field scrubs close only the group they open Using a library's character or paragraph style, and scrubbing a numeric field, closed their undo step with Session::end_undo_group, which closes the open undo group of every document. An undo group open in another document, such as one begun with edit.undoGroup.begin for an MCP command_batch with atomic, was closed too, and that document's next edits became undo steps of their own. When the style failed to apply, or Escape ended the scrub, the other document's group was canceled as well and the edits made in it were undone. Inside an undo group already open in the same document, a scrub closed that group, Escape undid the edits made in it before the drag, and a style that failed to apply left the style it had added. Session::begin_undo_scope and end_undo_scope make the edits in between one undo step of the active document: the scope opens an undo group there, or joins the one already open. Ending a scope closes only the group it opened, in its own document, even when another document became active in the meantime, and a canceled scope undoes only its own edits, also inside a group that was already open. library.use and the scrub use the scope, so Escape inside an open group also takes back a scrub of the Transform panel's Shear field. edit.undoGroup.end still closes the open group of every document. --- crates/engine/src/cmd/library.rs | 13 ++--- crates/engine/src/lib.rs | 79 +++++++++++++++++++++++++++++ crates/engine/src/tests_journal.rs | 74 +++++++++++++++++++++++++++ crates/engine/src/tests_library.rs | 60 ++++++++++++++++++++++ crates/ui-egui/src/lib.rs | 3 ++ crates/ui-egui/src/scrub.rs | 15 ++++-- crates/ui-egui/src/tests_scrub.rs | 81 +++++++++++++++++++++++++++++- 7 files changed, 311 insertions(+), 14 deletions(-) diff --git a/crates/engine/src/cmd/library.rs b/crates/engine/src/cmd/library.rs index 74a70fd28..88d4faad2 100644 --- a/crates/engine/src/cmd/library.rs +++ b/crates/engine/src/cmd/library.rs @@ -932,16 +932,13 @@ fn use_item(s: &mut Session, p: &Value) -> Result { } } -/// Run `f`'s commands as one undo step (undone together when it fails), unless an undo group is -/// already open (its own step then holds them). +/// Run `f`'s commands as one undo step, or as part of the step of an undo group open in the active +/// document, and undo them together when it fails ([`Session::begin_undo_scope`]). fn in_one_step(s: &mut Session, f: impl FnOnce(&mut Session) -> Result) -> Result { - let open = s.active().is_some_and(|st| st.undo_group.is_some()); - if !open { - s.begin_undo_group(); - } + let scope = s.begin_undo_scope(); let r = f(s); - if !open { - s.end_undo_group(r.is_err()); + if let Some(scope) = scope { + s.end_undo_scope(scope, r.is_err()); } r } diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 34a9c86da..535818524 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -305,6 +305,19 @@ pub struct UndoGroup { label: Option, } +/// One undo step begun with [`Session::begin_undo_scope`], which [`Session::end_undo_scope`] ends. +#[derive(Clone, Debug)] +pub struct UndoScope { + /// The document ([`DocState::uid`]). + uid: u64, + /// The journal's length when the scope began. + journal: usize, + /// When the document's undo group was open already: the document, its selection and the document + /// that the group's undo step keeps, as they were when the scope began; `None` when the scope + /// opened the group. + joined: Option<(Arc, Selection, Option>)>, +} + /// Per-document editing state. #[derive(Clone, Debug)] pub struct DocState { @@ -2054,6 +2067,72 @@ impl Session { self.journal.remove(&gone); } + /// [`Session::end_undo_group`] for the open document with [`DocState::uid`] `uid` only (it may + /// have closed since): its open undo group closes, and undo groups open in other documents stay + /// open. + fn end_document_undo_group(&mut self, uid: u64, cancel: bool) { + // The other documents' groups are set aside while `end_undo_group` closes this one's. + let others: Vec<(u64, UndoGroup)> = + self.docs.iter_mut().filter(|st| st.uid != uid).filter_map(|st| Some((st.uid, st.undo_group.take()?))).collect(); + self.end_undo_group(cancel); + for (other, g) in others { + if let Some(st) = self.document_mut(other) { + st.undo_group = Some(g); + } + } + } + + /// Make the edits that follow in the active document one undo step, until + /// [`Session::end_undo_scope`]: open an undo group there or, when one is open already, join it, + /// so that the edits are part of its step. `None` without a document. + pub fn begin_undo_scope(&mut self) -> Option { + let journal = self.journal.len(); + let st = self.active()?; + let uid = st.uid; + let joined = st.undo_group.as_ref().map(|g| (st.doc.clone(), st.selection.clone(), g.first.clone())); + if joined.is_none() { + self.begin_undo_group(); + } + Some(UndoScope { uid, journal, joined }) + } + + /// End `scope` in its own document, whichever document is active now; undo groups open in other + /// documents stay open. The undo group the scope opened closes as [`Session::end_undo_group`] + /// closes it. A scope that joined an open group leaves the group open; with `cancel`, it puts the + /// document and its selection back as they were when the scope began and cuts the journal back to + /// where the scope began (only while the group's step is still the newest one), and an undo step + /// that the scope's first edit recorded goes too. + pub fn end_undo_scope(&mut self, scope: UndoScope, cancel: bool) { + let Some((doc, selection, first)) = scope.joined else { + self.end_document_undo_group(scope.uid, cancel); + return; + }; + let guides_locked = self.menu.guides_locked; + let Some(st) = self.document_mut(scope.uid) else { return }; + let Some(g) = &mut st.undo_group else { return }; + let newest = g.first.as_ref().is_some_and(|f| st.history.undo.last().is_some_and(|e| Arc::ptr_eq(&e.doc, f))); + if !cancel || !newest { + return; + } + match &first { + // The scope's first edit recorded the group's step. + None => { + st.history.undo.pop(); + g.first = None; + } + Some(f) if g.first.as_ref().is_some_and(|now| Arc::ptr_eq(now, f)) => {} + Some(_) => return, + } + st.doc = doc; + st.selection = selection; + if guides_locked { + deselect_guides(&st.doc, &mut st.selection); + } + st.history.recost_last(&st.doc); + st.revision += 1; + self.journal.truncate(scope.journal); + } + /// Commands with enablement (for menus, palette, MCP `list_commands`). pub fn commands(&self) -> Vec { command_specs().iter().map(|c| c.info(self)).collect() diff --git a/crates/engine/src/tests_journal.rs b/crates/engine/src/tests_journal.rs index 713ad4b78..04d2bb71d 100644 --- a/crates/engine/src/tests_journal.rs +++ b/crates/engine/src/tests_journal.rs @@ -520,6 +520,80 @@ fn an_undo_group_is_one_undo_step() { assert_eq!(undo_len(&s), undo + 2); } +/// An undo scope ([`Session::begin_undo_scope`]) in a document without an open undo group opens +/// one, and [`Session::end_undo_scope`] closes that group only, whichever document is active, and +/// leaves another document's group open; canceled, it undoes only that group's edits and cuts the +/// journal back to where the group began. +#[test] +fn an_undo_scope_closes_only_its_own_documents_group() { + let mut s = Session::new(); + let rect = json!({"x": 10, "y": 10, "width": 100, "height": 50}); + let objects = |d: &DocState| d.doc.layers[0].children().map_or(0, |c| c.len()); + s.execute("file.new", &json!({"width": 400, "height": 300})).unwrap(); + s.begin_undo_group(); + s.execute("shape.rectangle", &rect).unwrap(); + s.execute("file.new", &json!({"width": 400, "height": 300})).unwrap(); + let journal = s.journal.len(); + let scope = s.begin_undo_scope().unwrap(); + assert!(s.doc().unwrap().undo_group.is_some(), "the scope opens a group"); + s.execute("shape.rectangle", &rect).unwrap(); + s.execute("shape.rectangle", &rect).unwrap(); + s.end_undo_scope(scope, true); + assert_eq!(objects(s.doc().unwrap()), 0, "canceled: the document's edits are undone"); + assert_eq!(s.journal.len(), journal, "and dropped from the journal"); + assert!(s.documents()[0].undo_group.is_some(), "the other document's group stays open"); + assert_eq!(objects(&s.documents()[0]), 1, "with its edit"); + + let undo = s.doc().unwrap().history.undo.len(); + let scope = s.begin_undo_scope().unwrap(); + s.execute("shape.rectangle", &rect).unwrap(); + s.execute("shape.rectangle", &rect).unwrap(); + s.set_active(0); + s.end_undo_scope(scope, false); + let second = &s.documents()[1]; + assert_eq!(second.history.undo.len(), undo + 1, "closed: one undo step"); + assert!(second.undo_group.is_none(), "closed while another document is active"); + assert!(s.documents()[0].undo_group.is_some(), "the active document's group stays open"); +} + +/// An undo scope begun while the document's undo group is open joins the group: ended, its edits +/// are part of the group's step and the group stays open; canceled, the document goes back to +/// where it stood when the scope began, with the edits made in the group before it, the journal +/// drops the scope's entries, and a step that the scope's first edit recorded goes too. +#[test] +fn an_undo_scope_in_an_open_group_takes_back_only_its_own_edits() { + let mut s = Session::new(); + let rect = json!({"x": 10, "y": 10, "width": 100, "height": 50}); + let objects = |s: &Session| s.doc().unwrap().doc.layers[0].children().map_or(0, |c| c.len()); + let undo_len = |s: &Session| s.doc().unwrap().history.undo.len(); + s.execute("file.new", &json!({"width": 400, "height": 300})).unwrap(); + let undo = undo_len(&s); + s.begin_undo_group(); + let journal = s.journal.len(); + let scope = s.begin_undo_scope().unwrap(); + s.execute("shape.rectangle", &rect).unwrap(); + s.end_undo_scope(scope, true); + assert_eq!((objects(&s), undo_len(&s), s.journal.len()), (0, undo, journal), "the group had no step: none is left"); + assert!(s.doc().unwrap().undo_group.is_some(), "the group stays open"); + + s.execute("shape.rectangle", &rect).unwrap(); + let journal = s.journal.len(); + let scope = s.begin_undo_scope().unwrap(); + s.execute("shape.rectangle", &rect).unwrap(); + s.execute("shape.rectangle", &rect).unwrap(); + s.end_undo_scope(scope, true); + assert_eq!((objects(&s), undo_len(&s), s.journal.len()), (1, undo + 1, journal), "the group's own edit stays"); + + let scope = s.begin_undo_scope().unwrap(); + s.execute("shape.rectangle", &rect).unwrap(); + s.end_undo_scope(scope, false); + assert_eq!((objects(&s), undo_len(&s)), (2, undo + 1), "ended: the edit joins the group's step"); + assert!(s.doc().unwrap().undo_group.is_some(), "the group stays open"); + s.end_undo_group(false); + s.execute("edit.undo", &json!({})).unwrap(); + assert_eq!(objects(&s), 0, "one undo takes back the group's edits"); +} + /// A move drag is named for what its last preview did: Alt pressed or released during the drag /// turns it into a copy or back (the Selection and Direct Selection tools). #[test] diff --git a/crates/engine/src/tests_library.rs b/crates/engine/src/tests_library.rs index 15c8d9be8..31398944c 100644 --- a/crates/engine/src/tests_library.rs +++ b/crates/engine/src/tests_library.rs @@ -143,6 +143,66 @@ fn text_styles_come_from_the_selected_type_and_apply_in_one_step() { assert_eq!(r["style"], format!("{name} 2")); } +/// A library with the character and paragraph styles of 31 pt type, an undo group opened in the +/// first document with a rectangle drawn in it, and a second document, active, with type +/// selected → (session, library, character style, paragraph style). +fn group_open_in_another_document() -> (Session, String, String, String) { + let mut s = session(); + let lib = run(&mut s, "library.create", json!({}))["id"].as_str().unwrap().to_string(); + let t = run(&mut s, "text.create", json!({"x": 10, "y": 50, "text": "Heading", "size": 31}))["id"].clone(); + run(&mut s, "select.set", json!({"ids": [t]})); + let char_style = run(&mut s, "library.add", json!({"library": lib, "kind": "charStyle"}))["name"].as_str().unwrap().to_string(); + let para_style = run(&mut s, "library.add", json!({"library": lib, "kind": "paraStyle"}))["name"].as_str().unwrap().to_string(); + run(&mut s, "edit.undoGroup.begin", json!({})); + run(&mut s, "shape.rectangle", json!({"x": 0, "y": 0, "width": 20, "height": 20})); + run(&mut s, "file.new", json!({"width": 200, "height": 200})); + let u = run(&mut s, "text.create", json!({"x": 10, "y": 50, "text": "Body", "size": 9}))["id"].clone(); + run(&mut s, "select.set", json!({"ids": [u]})); + (s, lib, char_style, para_style) +} + +/// A library style used in one document does not close an undo group that is open in another +/// document, so that document's next edit still joins the group's undo step. +#[test] +fn a_library_style_leaves_another_documents_undo_group_open() { + let (mut s, lib, name, _) = group_open_in_another_document(); + run(&mut s, "library.use", json!({"library": lib, "kind": "charStyle", "item": name})); + assert!(s.documents()[0].undo_group.is_some(), "the first document's undo group stays open"); + s.set_active(0); + let undo = s.doc().unwrap().history.undo.len(); + run(&mut s, "shape.rectangle", json!({"x": 30, "y": 0, "width": 20, "height": 20})); + assert_eq!(s.doc().unwrap().history.undo.len(), undo, "the edit joins the group's step"); +} + +/// A library style that fails to apply takes back only the edits it made in its own document: an +/// undo group open in another document stays open, and the edits made in it stay. +#[test] +fn a_library_style_that_fails_takes_back_only_its_own_edits() { + let (mut s, lib, _, name) = group_open_in_another_document(); + run(&mut s, "select.none", json!({})); + assert!(s.execute("library.use", &json!({"library": lib, "kind": "paraStyle", "item": name})).is_err(), "no type selected"); + assert!(s.doc().unwrap().doc.para_styles.iter().all(|p| p.name != name), "the style it added is taken back"); + let first = &s.documents()[0]; + assert_eq!(first.doc.layers[0].children().map_or(0, |c| c.len()), 2, "the type and the rectangle drawn in the group stay"); + assert!(first.undo_group.is_some(), "the first document's undo group stays open"); +} + +/// A library style that fails to apply inside an undo group open in its own document takes back +/// the style it added, and the group stays open with the edits made in it before. +#[test] +fn a_library_style_that_fails_in_an_open_group_takes_back_its_own_edits() { + let (mut s, lib, _, name) = group_open_in_another_document(); + s.set_active(0); + run(&mut s, "select.none", json!({})); + let undo = s.doc().unwrap().history.undo.len(); + assert!(s.execute("library.use", &json!({"library": lib, "kind": "paraStyle", "item": name})).is_err(), "no type selected"); + let d = s.doc().unwrap(); + assert!(d.doc.para_styles.iter().all(|p| p.name != name), "the style it added is taken back"); + assert_eq!(d.doc.layers[0].children().map_or(0, |c| c.len()), 2, "the type and the rectangle drawn in the group stay"); + assert_eq!(d.history.undo.len(), undo, "still the group's one step"); + assert!(d.undo_group.is_some(), "the group stays open"); +} + /// A text style that a library file names with spaces around it comes into the document with the /// name trimmed, as New Character Style and New Paragraph Style trim a name, and applies. #[test] diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index e45553480..4a665906a 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -509,6 +509,8 @@ pub struct VectorcraftApp { /// A numeric field is being scrubbed: the document's edits meanwhile are one undo step /// ([`scrub::begin_frame`]). scrub_group: bool, + /// The undo step of the scrub in progress ([`vectorcraft_engine::Session::begin_undo_scope`]). + scrub_scope: Option, /// The folders control requests, and the input they inject, may read and write /// (`--automation-read-root`, `--automation-write-root`); `None`: anywhere. See /// [`Self::with_automation_roots`]. @@ -610,6 +612,7 @@ impl VectorcraftApp { ime_discard: false, prefs_changed: false, scrub_group: false, + scrub_scope: None, automation_roots: None, synthetic_frame: false, } diff --git a/crates/ui-egui/src/scrub.rs b/crates/ui-egui/src/scrub.rs index 58d3405f7..91b7f10c1 100644 --- a/crates/ui-egui/src/scrub.rs +++ b/crates/ui-egui/src/scrub.rs @@ -80,23 +80,28 @@ pub(crate) fn phase(ctx: &Context) -> Phase { } } -/// Before the panels draw: while a field is scrubbed, the edits its values make are one undo step. -/// (The drag's first frame applies nothing, so the group is open before its first value.) +/// Before the panels draw: while a field is scrubbed, the edits its values make are one undo step, +/// or part of the step of an undo group open in the active document +/// ([`vectorcraft_engine::Session::begin_undo_scope`]). (The drag's first frame applies nothing, so +/// the step begins before its first value.) pub(crate) fn begin_frame(app: &mut VectorcraftApp, ctx: &Context) { if !app.scrub_group && matches!(phase(ctx), Phase::Dragging | Phase::Released) { - app.session.begin_undo_group(); + app.scrub_scope = app.session.begin_undo_scope(); app.scrub_group = true; } } -/// After the panels: a finished drag closes its undo group; Escape undoes it. +/// After the panels: a finished drag ends its undo step in the document it began in, even when +/// another document is active by then; Escape undoes the drag's edits. pub(crate) fn end_frame(app: &mut VectorcraftApp, ctx: &Context) { let phase = phase(ctx); if !app.scrub_group || phase == Phase::Dragging { return; } app.scrub_group = false; - app.session.end_undo_group(phase == Phase::Cancelled); + if let Some(scope) = app.scrub_scope.take() { + app.session.end_undo_scope(scope, phase == Phase::Cancelled); + } app.sync_views(); } diff --git a/crates/ui-egui/src/tests_scrub.rs b/crates/ui-egui/src/tests_scrub.rs index fa00aeee4..adb6b161d 100644 --- a/crates/ui-egui/src/tests_scrub.rs +++ b/crates/ui-egui/src/tests_scrub.rs @@ -6,7 +6,7 @@ use std::cell::Cell; use egui::{CursorIcon, Event, Key, Modifiers, PointerButton, Pos2, Rect, vec2}; use serde_json::json; use vectorcraft_doc::Unit; -use vectorcraft_engine::Session; +use vectorcraft_engine::{DocState, Session}; use crate::{VectorcraftApp, scrub, widgets}; @@ -239,6 +239,10 @@ fn undo_len(app: &VectorcraftApp) -> usize { app.session.doc().unwrap().history.undo.len() } +fn objects(doc: &DocState) -> usize { + doc.doc.layers[0].children().map_or(0, |c| c.len()) +} + /// Drag the W label by `dx` in four moves (`escape`: Escape before the release). fn drag_w(app: &mut VectorcraftApp, ctx: &egui::Context, dx: f32, escape: bool) { let mut w = None; @@ -280,3 +284,78 @@ fn scrubbing_w_in_the_app_is_one_undo_step() { drag_w(&mut app, &ctx, 40.0, false); assert!((width(&app) - 100.0).abs() < 1e-6, "off: {}", width(&app)); } + +/// A scrub does not close an undo group that is open in another document, and that document keeps +/// its edits, when Escape ends the drag and when the drag is released. +#[test] +fn a_scrub_leaves_another_documents_undo_group_open() { + let mut app = VectorcraftApp::new(Session::new(), Default::default()); + app.session.execute("file.new", &json!({"width": 400, "height": 300})).unwrap(); + app.session.execute("edit.undoGroup.begin", &json!({})).unwrap(); + app.session.execute("shape.rectangle", &json!({"x": 200, "y": 10, "width": 100, "height": 50})).unwrap(); + app.session.execute("file.new", &json!({"width": 400, "height": 300})).unwrap(); + app.session.execute("shape.rectangle", &json!({"x": 10, "y": 10, "width": 100, "height": 50})).unwrap(); + let ctx = egui::Context::default(); + let other = |app: &VectorcraftApp| (app.session.documents()[0].undo_group.is_some(), objects(&app.session.documents()[0])); + drag_w(&mut app, &ctx, 40.0, true); + assert!((width(&app) - 100.0).abs() < 1e-6, "Escape: {}", width(&app)); + assert_eq!(other(&app), (true, 1), "Escape"); + drag_w(&mut app, &ctx, 40.0, false); + assert!((width(&app) - (100.0 + 40.0 / scrub::PX_PER_STEP as f64)).abs() < 1e-6, "{}", width(&app)); + assert_eq!(other(&app), (true, 1), "released"); +} + +/// A scrub in a document whose undo group is already open joins that group's undo step and +/// leaves the group open; Escape puts the document back as it was before the drag, with the edits +/// made in the group, and drops the drag's journal entries. +#[test] +fn a_scrub_inside_an_open_undo_group_joins_its_step() { + let (mut app, ctx) = app_with_rect(); + app.run("edit.undoGroup.begin", json!({})).unwrap(); + app.run("shape.rectangle", json!({"x": 200, "y": 10, "width": 100, "height": 50})).unwrap(); + let undo = undo_len(&app); + let (before, journal) = (app.session.doc().unwrap().doc.clone(), app.session.journal.len()); + drag_w(&mut app, &ctx, 40.0, true); + assert!((width(&app) - 100.0).abs() < 1e-6, "Escape: {}", width(&app)); + assert!(std::sync::Arc::ptr_eq(&app.session.doc().unwrap().doc, &before), "Escape puts the document back"); + assert_eq!(app.session.journal.len(), journal, "and drops the drag's journal entries"); + assert_eq!(objects(app.session.doc().unwrap()), 2, "the rectangle drawn in the group stays"); + drag_w(&mut app, &ctx, 40.0, false); + assert!((width(&app) - (100.0 + 40.0 / scrub::PX_PER_STEP as f64)).abs() < 1e-6, "{}", width(&app)); + assert!(app.session.doc().unwrap().undo_group.is_some(), "the group stays open"); + assert_eq!(undo_len(&app), undo, "the drag joins the group's step"); + app.run("edit.undoGroup.end", json!({})).unwrap(); + app.run("edit.undo", json!({})).unwrap(); + assert_eq!(objects(app.session.doc().unwrap()), 1, "one undo takes back the rectangle and the drag"); +} + +/// A scrub closes the undo group it opened in its own document when another document becomes +/// active during the drag (as `document.activate` does), and leaves that document's group open. +#[test] +fn a_scrub_closes_its_group_in_its_own_document_after_a_switch() { + let mut app = VectorcraftApp::new(Session::new(), Default::default()); + app.session.execute("file.new", &json!({"width": 400, "height": 300})).unwrap(); + app.session.execute("edit.undoGroup.begin", &json!({})).unwrap(); + app.session.execute("file.new", &json!({"width": 400, "height": 300})).unwrap(); + app.session.execute("shape.rectangle", &json!({"x": 10, "y": 10, "width": 100, "height": 50})).unwrap(); + let ctx = egui::Context::default(); + let undo = undo_len(&app); + let mut w = None; + for _ in 0..4 { + w = app_frame(&mut app, &ctx, vec![], "W:"); + } + let at = w.expect("the W label is shown").center(); + let button = |pos, pressed| Event::PointerButton { pos, button: PointerButton::Primary, pressed, modifiers: Modifiers::NONE }; + app_frame(&mut app, &ctx, vec![Event::PointerMoved(at)], ""); + app_frame(&mut app, &ctx, vec![button(at, true)], ""); + for i in 1..=4 { + app_frame(&mut app, &ctx, vec![Event::PointerMoved(at + vec2(10.0 * i as f32, 0.0))], ""); + } + app.session.set_active(0); + app_frame(&mut app, &ctx, vec![button(at + vec2(40.0, 0.0), false)], ""); + app_frame(&mut app, &ctx, vec![], ""); + let second = &app.session.documents()[1]; + assert!(second.undo_group.is_none(), "the scrub's group is closed"); + assert_eq!(second.history.undo.len(), undo + 1, "one undo step for the drag"); + assert!(app.session.documents()[0].undo_group.is_some(), "the active document's group stays open"); +}