Merge pull request #4599 from dotCooCoo/fix/blend-pick-resets-on-history-step

Blend: a pending pick starts over when the document's history changes

crates/engine/src/history.rs

@@ -19,12 +19,20 @@ pub struct HistoryEntry {
     next: usize,
     /// The template and name `label` was made from ([`Session::edit_named`]).
     pub(crate) named: Option<NamedStep>,
+    /// A number no other step takes ([`History::step`]).
+    serial: u64,
     /// Original and converted new-art colors, only for steps that change document color mode.
     pub(crate) color_mode_paints: Option<(vectorcraft_tools::PaintDefaults, vectorcraft_tools::PaintDefaults)>,
     /// Owned new-art snapshots included in both directional estimates.
     color_mode_bytes: usize,
 }
 
+/// 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
 /// ([`Session::edit_named`]).
 #[derive(Clone, Debug, PartialEq, Eq)]
@@ -57,6 +65,7 @@ impl HistoryEntry {
             next_bytes,
             next: std::ptr::from_ref(next) as usize,
             named: None,
+            serial: next_serial(),
             color_mode_paints: None,
             color_mode_bytes: 0,
         }
@@ -88,6 +97,7 @@ impl HistoryEntry {
             next_bytes: costs.bytes,
             next: Arc::as_ptr(now) as usize,
             named: None,
+            serial: next_serial(),
             color_mode_paints: None,
             color_mode_bytes: 0,
         }
@@ -155,6 +165,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, 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))
+    }
+
     /// The document at row `row` of the History panel, `now` being the document now: the rows before
     /// it are the undo steps, oldest first, and the rows after it the redo steps, nearest first.
     pub(crate) fn state<'a>(&'a self, now: &'a Arc<Document>, row: usize) -> Option<&'a Arc<Document>> {
@@ -192,13 +209,14 @@ 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() {
             let (before, after) = e.doc.retained_bytes_each(now);
             e.bytes = before.saturating_add(e.color_mode_bytes);
             e.next_bytes = after.saturating_add(e.color_mode_bytes);
             e.next = std::ptr::from_ref(now) as usize;
+            e.serial = next_serial();
         }
     }
 }

crates/engine/src/lib.rs

@@ -62,6 +62,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<Box<dyn Tool>>,
+    /// 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 of the active document (its uid) as Cmd lent `tool`: what the lent tool's
     /// gesture found, so that one that leaves it as it was picks nothing.
     pub(crate) selection_at_lend: Option<(u64, vectorcraft_doc::Selection)>,

crates/engine/src/session/documents.rs

@@ -46,6 +46,7 @@ impl Session {
             journal: Journal::default(),
             tool: vectorcraft_tools::create("selection"),
             lender: None,
+            tool_step: None,
             selection_at_lend: None,
             lent_selection: None,
             last_selection_tool: None,

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};
@@ -233,6 +233,102 @@ 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_undo_or_redo_made_outside_the_blend_tool_drops_its_first_pick() {
+    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));
+        if case == "redo" {
+            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.
+        click(&mut s, if undo { 420.0 } else { 20.0 }, 20.0);
+        assert!(!s.overlays(ViewInfo::default()).is_empty());
+        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], "{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] }, "{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();
+    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();

crates/engine/src/tests/history_mem.rs

@@ -180,6 +180,47 @@ fn redo_during_an_interaction_keeps_the_history_and_the_document_in_step() {
     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 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);
+    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, 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);
+}
+
 #[test]
 fn color_mode_paint_snapshots_count_toward_the_history_budget() {
     let mut s = Session::new();

crates/engine/src/tooling.rs

@@ -402,6 +402,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<Vec<UiRequest>> {
         self.pointer_event(ev, view, true)
@@ -495,7 +514,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 mut acts = self.with_tool_cx(view, |t, cx| t.pointer(cx, ev));
         if !keep {
             acts.retain(|a| matches!(a, Action::Cancel | Action::Notify(_)));
@@ -507,6 +528,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();
+        }
         match ev.kind {
             PointerKind::Down => self.press_point = Some(self.freeform_point),
             PointerKind::Up => self.press_point = None,
@@ -764,6 +790,8 @@ impl Session {
     }
 
     pub fn overlays(&mut self, view: ViewInfo) -> Vec<Overlay> {
+        // 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") {

crates/tools/src/lib.rs

@@ -239,6 +239,10 @@ pub trait Tool: Send {
     fn after_command(&mut self, _cx: &ToolContext) -> Vec<Action> {
         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.

crates/tools/src/meshblend.rs

@@ -180,6 +180,11 @@ impl Tool for BlendTool {
     fn chosen_again(&mut self, cx: &ToolContext) -> Vec<Action> {
         self.deactivate(cx)
     }
+    /// 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)]