Move gear: every active-layer change counts, not only the ones through execute (#11381)

Session::note_layer_change(before) now backs the change count and is called by execute, start, finished jobs, select_layer, restore_layer_selection, layer_multi_cmds::set_selection, undo, redo, rollback and a new Session::set_active_layer; document switches (note_active_change) count too. The Pen tool's direct write of the active layer goes through set_active_layer.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

crates/engine/src/cmd/layer_multi_cmds/selection.rs

@@ -49,6 +49,7 @@ pub(crate) fn top_level(doc: &Document, ids: &[LayerId]) -> Vec<LayerId> {
 
 /// Replace the layer selection without a history step (like [`Session::select_layer`]).
 pub fn set_selection(s: &mut Session, ids: Vec<LayerId>, active: Option<LayerId>, anchor: Option<LayerId>) -> Result<()> {
+    let before = s.active_layer_stamp();
     let st = s.active_mut().ok_or(EngineError::NoDocument)?;
     if let Some(bad) = ids.iter().chain(active.iter()).find(|id| st.doc.layer(**id).is_none()) {
         return Err(EngineError::NoLayer(*bad));
@@ -70,6 +71,7 @@ pub fn set_selection(s: &mut Session, ids: Vec<LayerId>, active: Option<LayerId>
         st.saved_revision = st.revision;
     }
     st.last_damage = Some(Rect::EMPTY);
+    s.note_layer_change(before);
     Ok(())
 }
 

crates/engine/src/jobs.rs

@@ -924,7 +924,10 @@ impl Session {
     /// Run a command, in the background when it supports it (see [`jobs`](crate::jobs)). Errors
     /// like [`Session::execute`] for unknown, disabled or failing commands.
     pub fn start(&mut self, id: &str, params: Value) -> Result<Started> {
-        self.dispatch(id, params, true)
+        let before = self.active_layer_stamp();
+        let started = self.dispatch(id, params, true);
+        self.note_layer_change(before);
+        started
     }
 
     pub const MAX_PENDING_PREFLIGHTS: usize = 32;
@@ -1740,7 +1743,13 @@ impl Session {
     }
 
     /// Apply (or report the failure of) a job whose worker has delivered `r`.
-    fn finish_job(&mut self, mut job: Running, r: Result<Payload>) {
+    fn finish_job(&mut self, job: Running, r: Result<Payload>) {
+        let before = self.active_layer_stamp();
+        self.finish_job_step(job, r);
+        self.note_layer_change(before);
+    }
+
+    fn finish_job_step(&mut self, mut job: Running, r: Result<Payload>) {
         let apply = job.apply.take();
         // Keep the typed error beside its display text, so `wait_job` and `JobEvent` can still
         // report a hard budget's required/limit or an approval request's limits.

crates/engine/src/lib.rs

@@ -216,9 +216,11 @@ pub struct Session {
     active_generation: u64,
     /// The document id [`Self::active_generation`] was last bumped for.
     active_doc_for_generation: Option<photocraft_doc::DocId>,
-    /// Counts the [`Session::execute`] calls that changed the active document or layer (selection
-    /// commands, Action playback, undo, a canvas pick). A caller that remembers how a change came
-    /// about compares it to see that nothing changed the active layer since.
+    /// Counts the changes of the active document or layer made through the `Session` API: commands
+    /// (`execute`, `start`, finished jobs), `select_layer`, `restore_layer_selection`,
+    /// `layer_multi_cmds::set_selection`, `set_active_layer`, undo / redo / rollback, and document
+    /// switches. A caller that remembers how a change came about compares it to see that nothing
+    /// changed the active layer since. (Writing `DocState::active_layer` directly bypasses it.)
     active_layer_changes: u64,
     pub tools: ToolState,
     /// Log of executed commands (for action recording and debugging).

crates/engine/src/session/documents.rs

@@ -1,7 +1,7 @@
 use crate::{EngineError, Result};
 
 use super::history::fix_selection;
-use super::{DocState, Document, Session, channel_cmds, smart_cmds};
+use super::{DocState, Document, LayerId, Session, channel_cmds, smart_cmds};
 
 /// Move item `i` of `v` to position `to`, clamped to the end. Returns where it went; `None` when
 /// `i` is out of range.
@@ -53,11 +53,35 @@ impl Session {
     pub(crate) fn note_active_change(&mut self) {
         let id = self.active().map(|st| st.doc.id);
         if id != self.active_doc_for_generation {
+            self.active_layer_changes = self.active_layer_changes.wrapping_add(1);
             self.active_doc_for_generation = id;
             self.active_generation = self.active_generation.wrapping_add(1);
         }
     }
 
+    /// The active document and its active layer, for [`Self::note_layer_change`].
+    pub(crate) fn active_layer_stamp(&self) -> Option<(photocraft_doc::DocId, Option<LayerId>)> {
+        self.active().map(|st| (st.doc.id, st.active_layer))
+    }
+
+    /// Count a change of the active document or layer since `before` (from
+    /// [`Self::active_layer_stamp`]). Every path that can change either calls this after it.
+    pub(crate) fn note_layer_change(&mut self, before: Option<(photocraft_doc::DocId, Option<LayerId>)>) {
+        if self.active_layer_stamp() != before {
+            self.active_layer_changes = self.active_layer_changes.wrapping_add(1);
+        }
+    }
+
+    /// Make `layer` the active document's active layer and nothing else (the selection list and
+    /// anchor are left as they are), counted in [`Self::active_layer_changes`].
+    pub fn set_active_layer(&mut self, layer: Option<LayerId>) {
+        let before = self.active_layer_stamp();
+        if let Some(st) = self.active_mut() {
+            st.active_layer = layer;
+        }
+        self.note_layer_change(before);
+    }
+
     pub fn set_active(&mut self, index: usize) -> bool {
         if index < self.docs.len() {
             self.active = Some(index);

crates/engine/src/session/execute.rs

@@ -15,11 +15,9 @@ impl Session {
     /// Runs synchronously, including job-capable commands (see [`jobs`]; [`Session::start`] runs
     /// those in the background).
     pub fn execute(&mut self, id: &str, params: Value) -> Result<Value> {
-        let before = self.active().map(|st| (st.doc.id, st.active_layer));
+        let before = self.active_layer_stamp();
         let result = self.dispatch(id, params, false);
-        if self.active().map(|st| (st.doc.id, st.active_layer)) != before {
-            self.active_layer_changes = self.active_layer_changes.wrapping_add(1);
-        }
+        self.note_layer_change(before);
         match result? {
             jobs::Started::Done(v) => Ok(v),
             // Not reached: inline dispatch never starts a job. Waiting is still correct.

crates/engine/src/session/history.rs

@@ -4,6 +4,13 @@ use super::{Arc, DocState, Document, LayerId, LayerTarget, Session, channel_cmds
 impl Session {
     /// Undo a document step or restore Clear History's rows and source without editing pixels.
     pub fn undo(&mut self) -> bool {
+        let before = self.active_layer_stamp();
+        let taken = self.undo_step();
+        self.note_layer_change(before);
+        taken
+    }
+
+    fn undo_step(&mut self) -> bool {
         // A background job is computing from the current state: it must not move under it.
         if self.active_job().is_some() {
             return false;
@@ -55,6 +62,13 @@ impl Session {
 
     /// Restore the next document state and its selection/visibility bookkeeping through the ordinary history path.
     pub fn redo(&mut self) -> bool {
+        let before = self.active_layer_stamp();
+        let taken = self.redo_step();
+        self.note_layer_change(before);
+        taken
+    }
+
+    fn redo_step(&mut self) -> bool {
         if self.active_job().is_some() {
             return false;
         }
@@ -92,6 +106,13 @@ impl Session {
     /// last of them). Falls back to Undo plus forgetting the redo states when the history
     /// changed since that step. Returns whether a step was taken back.
     pub fn rollback(&mut self) -> bool {
+        let before = self.active_layer_stamp();
+        let taken = self.rollback_step();
+        self.note_layer_change(before);
+        taken
+    }
+
+    fn rollback_step(&mut self) -> bool {
         if self.active_job().is_some() {
             return false;
         }

crates/engine/src/session/layers.rs

@@ -5,6 +5,7 @@ use crate::TypeLayerRef;
 impl Session {
     /// Replace the active document's active layer without creating a history step.
     pub fn select_layer(&mut self, id: LayerId) -> Result<()> {
+        let before = self.active_layer_stamp();
         let st = self.active_mut().ok_or(EngineError::NoDocument)?;
         if st.doc.layer(id).is_none() {
             return Err(EngineError::NoLayer(id));
@@ -20,6 +21,7 @@ impl Session {
             st.saved_revision = st.revision;
         }
         st.last_damage = Some(photocraft_geom::Rect::EMPTY);
+        self.note_layer_change(before);
         Ok(())
     }
 
@@ -28,6 +30,7 @@ impl Session {
     /// is no layer selected. Selected layers that are gone are dropped; an active layer that is
     /// gone is an error and changes nothing. Like [`Self::select_layer`], not an edit.
     pub fn restore_layer_selection(&mut self, active: Option<LayerId>, selected: &[LayerId], anchor: Option<LayerId>) -> Result<()> {
+        let before = self.active_layer_stamp();
         let st = self.active_mut().ok_or(EngineError::NoDocument)?;
         if let Some(id) = active
             && st.doc.layer(id).is_none()
@@ -45,6 +48,7 @@ impl Session {
             st.saved_revision = st.revision;
         }
         st.last_damage = Some(photocraft_geom::Rect::EMPTY);
+        self.note_layer_change(before);
         Ok(())
     }
 

crates/engine/src/tests.rs

@@ -2004,3 +2004,30 @@ fn the_active_layer_change_count_moves_only_when_a_command_changes_the_active_la
     s.execute("layer.select", json!({"layer": a.0})).unwrap();
     assert!(s.active_layer_changes() >= m + 2, "a round trip back to the same layer still counts");
 }
+
+#[test]
+fn direct_selection_calls_count_active_layer_changes_only_when_the_layer_changes() {
+    let mut s = session_with_doc();
+    s.execute("layer.new.layer", json!({})).unwrap();
+    let a = s.active().unwrap().active_layer.unwrap();
+    s.execute("layer.new.layer", json!({})).unwrap();
+    let b = s.active().unwrap().active_layer.unwrap();
+    let count = |s: &Session| s.active_layer_changes();
+    let n = count(&s);
+    s.select_layer(b).unwrap();
+    assert_eq!(count(&s), n, "select_layer of the active layer");
+    s.select_layer(a).unwrap();
+    assert_eq!(count(&s), n + 1, "select_layer of another layer");
+    s.restore_layer_selection(Some(a), &[a], Some(a)).unwrap();
+    assert_eq!(count(&s), n + 1, "restore_layer_selection of the same active layer");
+    s.restore_layer_selection(Some(b), &[b], Some(b)).unwrap();
+    assert_eq!(count(&s), n + 2, "restore_layer_selection of another layer");
+    layer_multi_cmds::set_selection(&mut s, vec![b], Some(b), Some(b)).unwrap();
+    assert_eq!(count(&s), n + 2, "set_selection of the same active layer");
+    layer_multi_cmds::set_selection(&mut s, vec![a], Some(a), Some(a)).unwrap();
+    assert_eq!(count(&s), n + 3, "set_selection of another layer");
+    s.set_active_layer(Some(a));
+    assert_eq!(count(&s), n + 3, "set_active_layer of the same layer");
+    s.set_active_layer(Some(b));
+    assert_eq!(count(&s), n + 4, "set_active_layer of another layer");
+}

crates/ui-egui/src/move_bar_distribute_gear_tests.rs

@@ -425,3 +425,14 @@ fn a_pick_marker_is_read_once_and_governs_the_next_change_only() {
     assert_eq!(crate::layer_reveal::track(&mut app, &ctx), Some(inner));
     assert!(expanded(&app, group), "a later command change opens the group");
 }
+
+#[test]
+fn direct_selection_calls_after_a_pick_make_it_a_command_change() {
+    let (mut app, inner, group, ctx) = closed_group_doc(Tool::Brush, false);
+    app.ui.tool_options.move_auto_select = false;
+    ctrl_click(&mut app);
+    app.session.select_layer(group).unwrap();
+    app.session.select_layer(inner).unwrap();
+    assert_eq!(crate::layer_reveal::track(&mut app, &ctx), Some(inner));
+    assert!(expanded(&app, group), "a direct selection away and back is not a pick");
+}

crates/ui-egui/src/tools/vector_ui/pen.rs

@@ -787,9 +787,7 @@ fn commit_path(app: &mut PhotocraftApp, pen: &PenPath, closed: bool) -> Result<(
         super::PenTarget::Path { name, append } => append.then(|| name.clone()),
         _ => None,
     };
-    if let Some(st) = app.session.active_mut() {
-        st.active_layer = owner.layer;
-    }
+    app.session.set_active_layer(owner.layer);
     let foreground = std::mem::replace(&mut app.session.tools.foreground, owner.foreground);
     let background = std::mem::replace(&mut app.session.tools.background, owner.background);
     let channel = app.session.active().map(|st| st.channel_view.target);