From efd7f3e97a27d22c82d6d89144b99bc619012599 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Thu, 29 Oct 2026 03:38:35 -0700 Subject: [PATCH 1/3] Blend: a pending pick starts over when the document's history changes The Blend tool kept its first pick, its clicked anchor and the blend it was adding to through edits made outside the tool. Picking A, recoloring other art and clicking B made a blend of A and B; undoing A's creation left the next click doing nothing. History::step names the current step by the serial numbers of the newest undo step and the next redo step. The session keeps the active document's uid and step from the tool's last pointer event and calls the new Tool::history_moved at the next pointer event, or before drawing overlays, when they differ. The tool's own edits are recorded after its actions; a selection tool lent by Cmd is not told. BlendTool clears its pick and chain there, as Escape does. A selection change keeps them. --- crates/engine/src/history.rs | 28 +++++++++- crates/engine/src/lib.rs | 4 ++ crates/engine/src/session/documents.rs | 1 + crates/engine/src/tests/blendopts.rs | 74 +++++++++++++++++++++++++- crates/engine/src/tooling.rs | 28 ++++++++++ crates/tools/src/lib.rs | 4 ++ crates/tools/src/meshblend.rs | 5 ++ 7 files changed, 141 insertions(+), 3 deletions(-) diff --git a/crates/engine/src/history.rs b/crates/engine/src/history.rs index ec53acd5ba..ee6adccc49 100644 --- a/crates/engine/src/history.rs +++ b/crates/engine/src/history.rs @@ -19,6 +19,14 @@ pub struct HistoryEntry { next: usize, /// The template and name `label` was made from ([`Session::edit_named`]). pub(crate) named: Option, + /// A number no other step takes ([`History::step`]). + serial: u64, +} + +/// The next [`HistoryEntry::serial`]: 1 for the first step made, so 0 never names a step. +fn next_serial() -> u64 { + static NEXT: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(1); + NEXT.fetch_add(1, std::sync::atomic::Ordering::Relaxed) } /// A step name made by filling the `{name}` placeholder of a template with a name @@ -45,7 +53,7 @@ impl HistoryEntry { pub(crate) fn kept(label: String, doc: Arc, selection: Arc, next: &Document) -> Self { doc.drop_lookup_cache(); let (bytes, next_bytes) = doc.retained_bytes_each(next); - HistoryEntry { label, doc, selection, bytes, next_bytes, next: std::ptr::from_ref(next) as usize, named: None } + HistoryEntry { label, doc, selection, bytes, next_bytes, next: std::ptr::from_ref(next) as usize, named: None, serial: next_serial() } } /// The template and name the step's label was made from, while the label is still the one @@ -66,7 +74,16 @@ impl HistoryEntry { return Self::kept(label, doc, selection, now); } doc.drop_lookup_cache(); - HistoryEntry { label, doc, selection, bytes: costs.next_bytes, next_bytes: costs.bytes, next: Arc::as_ptr(now) as usize, named: None } + HistoryEntry { + label, + doc, + selection, + bytes: costs.next_bytes, + next_bytes: costs.bytes, + next: Arc::as_ptr(now) as usize, + named: None, + serial: next_serial(), + } } /// The figures [`HistoryEntry::turned`] reuses. @@ -111,6 +128,13 @@ impl History { self.undo.iter().chain(&self.redo).map(|e| e.bytes).sum() } + /// Which step the document is at: the newest undo step and the next redo step, by numbers no + /// other step takes (0 for none). Every new step, Undo and Redo changes it; an edit merged into + /// the newest step (an undo group) and a selection change do not. + pub fn step(&self) -> (u64, u64) { + (self.undo.last().map_or(0, |e| e.serial), self.redo.last().map_or(0, |e| e.serial)) + } + /// Let the oldest steps go while over the step limit or the memory budget: undo steps first /// (the oldest), then redo steps (the furthest ahead). pub fn trim(&mut self) { diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 7f32ca9948..226c391d36 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -60,6 +60,10 @@ pub struct Session { /// While Cmd lends `tool` (a selection tool) for a drag: the tool it was lent to, which comes /// back as it was at the release ([`Session::pointer`]). pub(crate) lender: Option>, + /// The active document (its uid) and its [`History::step`] when the tool last handled a pointer + /// event or drew its overlays: when they differ at the next one, the tool is told first + /// ([`vectorcraft_tools::Tool::history_moved`]). None until the first. + pub(crate) tool_step: Option<(u64, (u64, u64))>, /// The selection tool chosen last (Selection, Direct Selection or Group Selection): the one Cmd /// lends the other tools. None until one is chosen. pub(crate) last_selection_tool: Option<&'static str>, diff --git a/crates/engine/src/session/documents.rs b/crates/engine/src/session/documents.rs index 89d846330b..c073369aa8 100644 --- a/crates/engine/src/session/documents.rs +++ b/crates/engine/src/session/documents.rs @@ -31,6 +31,7 @@ impl Session { journal: Journal::default(), tool: vectorcraft_tools::create("selection"), lender: None, + tool_step: None, last_selection_tool: None, last_view: ViewInfo::default(), preview_memo: Default::default(), diff --git a/crates/engine/src/tests/blendopts.rs b/crates/engine/src/tests/blendopts.rs index 97f3560787..ec05893f8e 100644 --- a/crates/engine/src/tests/blendopts.rs +++ b/crates/engine/src/tests/blendopts.rs @@ -1,6 +1,6 @@ //! Blend Options (defaults with nothing selected, `object.blend.info`) and the Blend tool: //! start points from clicked anchors, adding keys to a blend instead of nesting it (M8.18), and -//! Escape dropping the first pick. +//! Escape, or an edit made outside the tool, dropping the first pick. use serde_json::{Value, json}; use vectorcraft_doc::live::{BlendOrientation, BlendSpacing, BlendSpec}; @@ -167,6 +167,78 @@ fn escape_drops_the_blend_tools_first_pick() { assert_eq!(spec(&s, g).0, vec![b, c], "a was not added"); } +#[test] +fn an_edit_or_undo_made_outside_the_blend_tool_drops_its_first_pick() { + for undo in [false, true] { + let mut s = session(); + let (a, b, c) = (rect(&mut s, 0.0, 0.0, 40.0), rect(&mut s, 200.0, 0.0, 40.0), rect(&mut s, 400.0, 0.0, 40.0)); + s.select_tool("blend", ViewInfo::default()).unwrap(); + let click = |s: &mut Session, x: f64, y: f64| s.pointer(&PointerEvent::new(PointerKind::Down, x, y), ViewInfo::default()).unwrap(); + // Undo takes away c, the object picked first; an edit recolors c, which is not picked. + click(&mut s, if undo { 420.0 } else { 20.0 }, 20.0); + assert!(!s.overlays(ViewInfo::default()).is_empty()); + if undo { + s.execute("edit.undo", &json!({})).unwrap(); + } else { + s.execute("paint.setFill", &json!({"ids": [c.0], "color": "#ff0000"})).unwrap(); + } + assert!(s.overlays(ViewInfo::default()).is_empty(), "undo {undo}: the pick is not marked"); + let (layers, depth) = (s.doc().unwrap().doc.layers.clone(), s.doc().unwrap().history.undo.len()); + click(&mut s, 220.0, 20.0); + assert_eq!(s.doc().unwrap().selection.objects, vec![b], "undo {undo}: b is picked first"); + assert_eq!((&s.doc().unwrap().doc.layers, s.doc().unwrap().history.undo.len()), (&layers, depth), "undo {undo}: the click changes no art"); + click(&mut s, if undo { 20.0 } else { 420.0 }, 20.0); + let g = s.doc().unwrap().selection.objects[0]; + assert_eq!(spec(&s, g).0, if undo { vec![a, b] } else { vec![b, c] }, "undo {undo}"); + } +} + +#[test] +fn removing_the_blend_the_blend_tool_adds_to_ends_adding() { + let mut s = session(); + let (a, b, c) = (rect(&mut s, 0.0, 0.0, 40.0), rect(&mut s, 200.0, 0.0, 40.0), rect(&mut s, 400.0, 0.0, 40.0)); + let (d, e) = (rect(&mut s, 0.0, 300.0, 40.0), rect(&mut s, 200.0, 300.0, 40.0)); + let other = id_of(&s.execute("object.blend.make", &json!({"ids": [d.0, e.0]})).unwrap()); + s.select_tool("blend", ViewInfo::default()).unwrap(); + let click = |s: &mut Session, x: f64, y: f64| s.pointer(&PointerEvent::new(PointerKind::Down, x, y), ViewInfo::default()).unwrap(); + click(&mut s, 20.0, 20.0); + click(&mut s, 220.0, 20.0); + let g = s.doc().unwrap().selection.objects[0]; + s.execute("edit.clear", &json!({"ids": [g.0]})).unwrap(); + sel(&mut s, &[other]); + click(&mut s, 420.0, 20.0); + assert_eq!(s.doc().unwrap().selection.objects, vec![c], "c is picked first"); + assert_eq!(spec(&s, other).0, vec![d, e], "c is not added to the selected blend"); + assert!([a, b, g].iter().all(|id| s.doc().unwrap().doc.node(*id).is_none()), "the blend went with its keys"); +} + +#[test] +fn a_selection_change_keeps_the_blend_tools_first_pick() { + for drag in [false, true] { + let mut s = session(); + let (a, b, c) = (rect(&mut s, 0.0, 0.0, 40.0), rect(&mut s, 200.0, 0.0, 40.0), rect(&mut s, 400.0, 0.0, 40.0)); + let view = ViewInfo::default(); + s.select_tool("blend", view).unwrap(); + s.pointer(&PointerEvent::new(PointerKind::Down, 20.0, 20.0), view).unwrap(); + sel(&mut s, &[c]); + // Cmd lends the selection tool: a click on c selects it, a drag moves it (an edit). + let cmd = Mods { cmd: true, ..Mods::default() }; + let to = if drag { 440.0 } else { 420.0 }; + s.pointer(&PointerEvent::new(PointerKind::Down, 420.0, 20.0).with_mods(cmd), view).unwrap(); + s.pointer(&PointerEvent::new(PointerKind::Drag, to, 20.0).with_mods(cmd), view).unwrap(); + s.pointer(&PointerEvent::new(PointerKind::Up, to, 20.0).with_mods(cmd), view).unwrap(); + assert_eq!(s.tool_id(), "blend"); + assert_eq!(s.overlays(view).is_empty(), drag, "drag {drag}"); + s.pointer(&PointerEvent::new(PointerKind::Down, 220.0, 20.0), view).unwrap(); + if drag { + assert_eq!(s.doc().unwrap().selection.objects, vec![b], "the drag dropped the pick"); + } else { + let g = s.doc().unwrap().selection.objects[0]; + assert_eq!(spec(&s, g).0, vec![a, b]); + } + } +} + #[test] fn start_points_round_trip_and_old_blends_load() { let mut s = session(); diff --git a/crates/engine/src/tooling.rs b/crates/engine/src/tooling.rs index 81f8616a80..94d2d6742e 100644 --- a/crates/engine/src/tooling.rs +++ b/crates/engine/src/tooling.rs @@ -328,6 +328,25 @@ impl Session { self.tool_panic.take().map_or(Ok(()), Err) } + /// The active document (its uid) and its [`crate::History::step`]. + fn history_step(&self) -> Option<(u64, (u64, u64))> { + self.active().map(|d| (d.uid, d.history.step())) + } + + /// Tell the active tool when the history moved, or another document became active, since it + /// last handled a pointer event or was last told ([`vectorcraft_tools::Tool::history_moved`]). + /// A lent selection tool is not told. + fn tell_tool_history_moved(&mut self) { + if self.lender.is_some() { + return; + } + let now = self.history_step(); + if self.tool_step.is_some_and(|seen| Some(seen) != now) { + self.tool.history_moved(); + } + self.tool_step = now; + } + /// Feed a pointer event to the active tool. Returns requests for the UI. pub fn pointer(&mut self, ev: &PointerEvent, view: ViewInfo) -> Result> { self.last_view = view; @@ -341,7 +360,9 @@ impl Session { return r; } self.leave_draw_inside(ev, view)?; + self.tell_tool_history_moved(); let ev = &self.lend_selection_tool(ev); + let lent = self.lender.is_some(); let acts = self.with_tool_cx(view, |t, cx| t.pointer(cx, ev)); let r = self.take_tool_panic().and_then(|()| { // A gesture may change options (Alt-drag sizes a Liquify brush): keep them. @@ -350,6 +371,11 @@ impl Session { } self.apply_actions(acts) }); + // The tool's own edits do not move the history for it. A selection tool that Cmd lends + // edits for the lender, which hears of the edit at its next event. + if !lent { + self.tool_step = self.history_step(); + } if ev.kind == PointerKind::Up { self.give_back_tool(view)?; } @@ -553,6 +579,8 @@ impl Session { } pub fn overlays(&mut self, view: ViewInfo) -> Vec { + // The Blend tool's first pick is not marked after an edit that dropped it. + self.tell_tool_history_moved(); let mut v = self.with_tool_cx(view, |t, cx| t.overlays(cx)); // The Type tools draw the ports of selected type themselves. if !matches!(self.tool.id(), "type" | "areaType" | "typeOnPath" | "verticalType" | "verticalAreaType" | "verticalTypeOnPath") { diff --git a/crates/tools/src/lib.rs b/crates/tools/src/lib.rs index e55bb4069a..686e004081 100644 --- a/crates/tools/src/lib.rs +++ b/crates/tools/src/lib.rs @@ -224,6 +224,10 @@ pub trait Tool: Send { fn after_command(&mut self, _cx: &ToolContext) -> Vec { vec![] } + /// The undo history moved outside the tool (an edit, Undo or Redo the tool did not make), or + /// another document became active. The Blend tool drops its first pick and stops adding to the + /// blend it just made. + fn history_moved(&mut self) {} /// Time passes while the pointer button is held (`dt` seconds since the last tick, whether or /// not the pointer moved): tools that keep working while the brush holds still (Twirl, Pucker, /// Bloat) act on it. The host supplies the time, so tests and agents drive it exactly. diff --git a/crates/tools/src/meshblend.rs b/crates/tools/src/meshblend.rs index d6a46faf05..e881d54c41 100644 --- a/crates/tools/src/meshblend.rs +++ b/crates/tools/src/meshblend.rs @@ -148,6 +148,11 @@ impl Tool for BlendTool { (self.first, self.chained) = (None, false); vec![] } + /// An edit, Undo or Redo made outside the tool drops the object picked first and stops adding to + /// the blend just made, so the next click picks a new first object. + fn history_moved(&mut self) { + (self.first, self.chained) = (None, false); + } } #[derive(Default)] From 4b663906e9935b5b7fe97756cf3c0b31ea181aa1 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sun, 1 Nov 2026 06:53:56 -0800 Subject: [PATCH 2/3] Blend: tests for a pick dropped by Redo and for the history step A Redo made outside the Blend tool drops its first pick, as an edit or an Undo does, and two picks with no edit between them blend. History::step changes with each new step, Undo and Redo, also when an Undo and a new edit leave as many steps as before, and stays the same for a selection change and for an edit merged into an undo group. --- crates/engine/src/tests/blendopts.rs | 43 +++++++++++++++++++------- crates/engine/src/tests/history_mem.rs | 37 ++++++++++++++++++++++ 2 files changed, 68 insertions(+), 12 deletions(-) diff --git a/crates/engine/src/tests/blendopts.rs b/crates/engine/src/tests/blendopts.rs index 7332e18ec0..366a19241f 100644 --- a/crates/engine/src/tests/blendopts.rs +++ b/crates/engine/src/tests/blendopts.rs @@ -234,31 +234,50 @@ fn escape_drops_the_blend_tools_first_pick() { } #[test] -fn an_edit_or_undo_made_outside_the_blend_tool_drops_its_first_pick() { - for undo in [false, true] { +fn an_edit_undo_or_redo_made_outside_the_blend_tool_drops_its_first_pick() { + for case in ["edit", "undo", "redo"] { + let undo = case == "undo"; let mut s = session(); let (a, b, c) = (rect(&mut s, 0.0, 0.0, 40.0), rect(&mut s, 200.0, 0.0, 40.0), rect(&mut s, 400.0, 0.0, 40.0)); + if case == "redo" { + s.execute("paint.setFill", &json!({"ids": [c.0], "color": "#ff0000"})).unwrap(); + s.execute("edit.undo", &json!({})).unwrap(); + } s.select_tool("blend", ViewInfo::default()).unwrap(); let click = |s: &mut Session, x: f64, y: f64| s.pointer(&PointerEvent::new(PointerKind::Down, x, y), ViewInfo::default()).unwrap(); - // Undo takes away c, the object picked first; an edit recolors c, which is not picked. + // Undo takes away c, the object picked first; an edit or a redo recolors c, which is not picked. click(&mut s, if undo { 420.0 } else { 20.0 }, 20.0); assert!(!s.overlays(ViewInfo::default()).is_empty()); - if undo { - s.execute("edit.undo", &json!({})).unwrap(); - } else { - s.execute("paint.setFill", &json!({"ids": [c.0], "color": "#ff0000"})).unwrap(); - } - assert!(s.overlays(ViewInfo::default()).is_empty(), "undo {undo}: the pick is not marked"); + match case { + "undo" => s.execute("edit.undo", &json!({})).unwrap(), + "redo" => s.execute("edit.redo", &json!({})).unwrap(), + _ => s.execute("paint.setFill", &json!({"ids": [c.0], "color": "#ff0000"})).unwrap(), + }; + assert!(s.overlays(ViewInfo::default()).is_empty(), "{case}: the pick is not marked"); let (layers, depth) = (s.doc().unwrap().doc.layers.clone(), s.doc().unwrap().history.undo.len()); click(&mut s, 220.0, 20.0); - assert_eq!(s.doc().unwrap().selection.objects, vec![b], "undo {undo}: b is picked first"); - assert_eq!((&s.doc().unwrap().doc.layers, s.doc().unwrap().history.undo.len()), (&layers, depth), "undo {undo}: the click changes no art"); + assert_eq!(s.doc().unwrap().selection.objects, vec![b], "{case}: b is picked first"); + assert_eq!((&s.doc().unwrap().doc.layers, s.doc().unwrap().history.undo.len()), (&layers, depth), "{case}: the click changes no art"); click(&mut s, if undo { 20.0 } else { 420.0 }, 20.0); let g = s.doc().unwrap().selection.objects[0]; - assert_eq!(spec(&s, g).0, if undo { vec![a, b] } else { vec![b, c] }, "undo {undo}"); + assert_eq!(spec(&s, g).0, if undo { vec![a, b] } else { vec![b, c] }, "{case}"); } } +#[test] +fn two_picks_with_no_edit_between_them_blend() { + let mut s = session(); + let (a, b) = (rect(&mut s, 0.0, 0.0, 40.0), rect(&mut s, 200.0, 0.0, 40.0)); + s.select_tool("blend", ViewInfo::default()).unwrap(); + let click = |s: &mut Session, x: f64, y: f64| s.pointer(&PointerEvent::new(PointerKind::Down, x, y), ViewInfo::default()).unwrap(); + click(&mut s, 20.0, 20.0); + // Drawing the overlays between the picks does not drop the first one. + assert!(!s.overlays(ViewInfo::default()).is_empty()); + click(&mut s, 220.0, 20.0); + let g = s.doc().unwrap().selection.objects[0]; + assert_eq!(spec(&s, g).0, vec![a, b]); +} + #[test] fn removing_the_blend_the_blend_tool_adds_to_ends_adding() { let mut s = session(); diff --git a/crates/engine/src/tests/history_mem.rs b/crates/engine/src/tests/history_mem.rs index af5d495528..23665d9eca 100644 --- a/crates/engine/src/tests/history_mem.rs +++ b/crates/engine/src/tests/history_mem.rs @@ -179,3 +179,40 @@ fn redo_during_an_interaction_keeps_the_history_and_the_document_in_step() { s.execute("edit.undo", &json!({})).unwrap(); assert_eq!(first_x(&s), start); } + +/// `History::step` changes with every new step, Undo and Redo, also when an undo and a new edit +/// leave as many steps as before, and stays the same for an edit merged into an undo group and for +/// a selection change. +#[test] +fn the_history_step_changes_with_each_step_undo_and_redo() { + let mut s = session_with_rects(2); + let step = |s: &Session| s.doc().unwrap().history.step(); + let start = step(&s); + assert!(start.0 != 0 && start.1 == 0, "the Add step, and nothing to redo: {start:?}"); + s.execute("object.move", &json!({"dx": 1, "dy": 0})).unwrap(); + let moved = step(&s); + assert!(moved.0 != start.0 && moved.1 == 0, "{moved:?}"); + s.execute("edit.undo", &json!({})).unwrap(); + let undone = step(&s); + assert!(undone.0 == start.0 && undone.1 != 0, "{undone:?}"); + s.execute("edit.redo", &json!({})).unwrap(); + let redone = step(&s); + assert!(![start, moved, undone].contains(&redone) && redone.1 == 0, "{redone:?}"); + // An undo and a new edit: as many undo steps as after the move, at a different step. + s.execute("edit.undo", &json!({})).unwrap(); + let depth = s.doc().unwrap().history.undo.len(); + s.execute("object.move", &json!({"dx": 0, "dy": 1})).unwrap(); + let other = step(&s); + assert_eq!(s.doc().unwrap().history.undo.len(), depth + 1); + assert!(![start, moved, undone, redone].contains(&other), "{other:?}"); + s.execute("select.none", &json!({})).unwrap(); + assert_eq!(step(&s), other, "a selection change is no step"); + s.execute("select.all", &json!({})).unwrap(); + s.begin_undo_group(); + s.execute("object.move", &json!({"dx": 1, "dy": 0})).unwrap(); + let grouped = step(&s); + s.execute("object.move", &json!({"dx": 1, "dy": 0})).unwrap(); + assert_eq!(step(&s), grouped, "the second move is merged into the first"); + s.end_undo_group(false); + assert_ne!(grouped, other); +} From baeef1ec05717908707e646224ada0458b02a664 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sun, 1 Nov 2026 07:12:43 -0800 Subject: [PATCH 3/3] Blend: an edit merged into an open undo group drops the first pick An edit merged into the newest undo step (an undo group) left History::step unchanged, so a first pick made while the group was open survived the edit. The newest step now takes a new number whenever it is extended. --- crates/engine/src/history.rs | 7 ++++--- crates/engine/src/tests/blendopts.rs | 7 ++++++- crates/engine/src/tests/history_mem.rs | 16 ++++++++++------ 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/crates/engine/src/history.rs b/crates/engine/src/history.rs index ee6adccc49..4ce4e4599a 100644 --- a/crates/engine/src/history.rs +++ b/crates/engine/src/history.rs @@ -129,8 +129,8 @@ impl History { } /// Which step the document is at: the newest undo step and the next redo step, by numbers no - /// other step takes (0 for none). Every new step, Undo and Redo changes it; an edit merged into - /// the newest step (an undo group) and a selection change do not. + /// other step takes (0 for none). Every new step, Undo, Redo and edit merged into the newest + /// step (an undo group) changes it; a selection change does not. pub fn step(&self) -> (u64, u64) { (self.undo.last().map_or(0, |e| e.serial), self.redo.last().map_or(0, |e| e.serial)) } @@ -161,11 +161,12 @@ impl History { } /// Estimate the newest undo step again, its next state now being `now` (the steps after it were - /// merged into it, or an undo group extended it). + /// merged into it, or an undo group extended it). It takes a new number ([`History::step`]). pub(crate) fn recost_last(&mut self, now: &Document) { if let Some(e) = self.undo.last_mut() { (e.bytes, e.next_bytes) = e.doc.retained_bytes_each(now); e.next = std::ptr::from_ref(now) as usize; + e.serial = next_serial(); } } } diff --git a/crates/engine/src/tests/blendopts.rs b/crates/engine/src/tests/blendopts.rs index 366a19241f..059357ddb3 100644 --- a/crates/engine/src/tests/blendopts.rs +++ b/crates/engine/src/tests/blendopts.rs @@ -235,7 +235,7 @@ fn escape_drops_the_blend_tools_first_pick() { #[test] fn an_edit_undo_or_redo_made_outside_the_blend_tool_drops_its_first_pick() { - for case in ["edit", "undo", "redo"] { + for case in ["edit", "undo", "redo", "group"] { let undo = case == "undo"; let mut s = session(); let (a, b, c) = (rect(&mut s, 0.0, 0.0, 40.0), rect(&mut s, 200.0, 0.0, 40.0), rect(&mut s, 400.0, 0.0, 40.0)); @@ -243,6 +243,11 @@ fn an_edit_undo_or_redo_made_outside_the_blend_tool_drops_its_first_pick() { s.execute("paint.setFill", &json!({"ids": [c.0], "color": "#ff0000"})).unwrap(); s.execute("edit.undo", &json!({})).unwrap(); } + // In an open undo group, the edit after the pick is merged into the group's step. + if case == "group" { + s.begin_undo_group(); + s.execute("paint.setFill", &json!({"ids": [c.0], "color": "#00ff00"})).unwrap(); + } s.select_tool("blend", ViewInfo::default()).unwrap(); let click = |s: &mut Session, x: f64, y: f64| s.pointer(&PointerEvent::new(PointerKind::Down, x, y), ViewInfo::default()).unwrap(); // Undo takes away c, the object picked first; an edit or a redo recolors c, which is not picked. diff --git a/crates/engine/src/tests/history_mem.rs b/crates/engine/src/tests/history_mem.rs index 23665d9eca..c14ea4aca4 100644 --- a/crates/engine/src/tests/history_mem.rs +++ b/crates/engine/src/tests/history_mem.rs @@ -181,8 +181,8 @@ fn redo_during_an_interaction_keeps_the_history_and_the_document_in_step() { } /// `History::step` changes with every new step, Undo and Redo, also when an undo and a new edit -/// leave as many steps as before, and stays the same for an edit merged into an undo group and for -/// a selection change. +/// leave as many steps as before, and with an edit merged into an undo group's step. It stays the +/// same for a selection change. #[test] fn the_history_step_changes_with_each_step_undo_and_redo() { let mut s = session_with_rects(2); @@ -210,9 +210,13 @@ fn the_history_step_changes_with_each_step_undo_and_redo() { s.execute("select.all", &json!({})).unwrap(); s.begin_undo_group(); s.execute("object.move", &json!({"dx": 1, "dy": 0})).unwrap(); - let grouped = step(&s); - s.execute("object.move", &json!({"dx": 1, "dy": 0})).unwrap(); - assert_eq!(step(&s), grouped, "the second move is merged into the first"); - s.end_undo_group(false); + let (grouped, depth) = (step(&s), s.doc().unwrap().history.undo.len()); assert_ne!(grouped, other); + s.execute("object.move", &json!({"dx": 1, "dy": 0})).unwrap(); + assert_eq!(s.doc().unwrap().history.undo.len(), depth, "the second move is merged into the first"); + let merged = step(&s); + assert!(merged != grouped && merged.1 == 0, "a merged edit moves the step: {merged:?}"); + s.execute("select.none", &json!({})).unwrap(); + assert_eq!(step(&s), merged); + s.end_undo_group(false); }