diff --git a/crates/engine/src/cmd/layer_multi_cmds/selection.rs b/crates/engine/src/cmd/layer_multi_cmds/selection.rs index e5e361fb1c..cb3dad8c75 100644 --- a/crates/engine/src/cmd/layer_multi_cmds/selection.rs +++ b/crates/engine/src/cmd/layer_multi_cmds/selection.rs @@ -49,6 +49,7 @@ pub(crate) fn top_level(doc: &Document, ids: &[LayerId]) -> Vec { /// Replace the layer selection without a history step (like [`Session::select_layer`]). pub fn set_selection(s: &mut Session, ids: Vec, active: Option, anchor: Option) -> 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, active: Option st.saved_revision = st.revision; } st.last_damage = Some(Rect::EMPTY); + s.note_layer_change(before); Ok(()) } diff --git a/crates/engine/src/jobs.rs b/crates/engine/src/jobs.rs index 15ae84a291..cd39e13d55 100644 --- a/crates/engine/src/jobs.rs +++ b/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 { - 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) { + fn finish_job(&mut self, job: Running, r: Result) { + 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) { 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. diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 81b0fdb8b9..18b34f86c1 100644 --- a/crates/engine/src/lib.rs +++ b/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, - /// 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). diff --git a/crates/engine/src/session/documents.rs b/crates/engine/src/session/documents.rs index ead3894e48..02755acc9e 100644 --- a/crates/engine/src/session/documents.rs +++ b/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)> { + 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)>) { + 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) { + 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); diff --git a/crates/engine/src/session/execute.rs b/crates/engine/src/session/execute.rs index 53212bce3d..5522f5fc5c 100644 --- a/crates/engine/src/session/execute.rs +++ b/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 { - 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. diff --git a/crates/engine/src/session/history.rs b/crates/engine/src/session/history.rs index c5f9cd301c..01a90a169f 100644 --- a/crates/engine/src/session/history.rs +++ b/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; } diff --git a/crates/engine/src/session/layers.rs b/crates/engine/src/session/layers.rs index 4511b7fbf1..d7f8ed9997 100644 --- a/crates/engine/src/session/layers.rs +++ b/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, selected: &[LayerId], anchor: Option) -> 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(()) } diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 3cf1950b55..8ed2445f8d 100644 --- a/crates/engine/src/tests.rs +++ b/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"); +} diff --git a/crates/ui-egui/src/move_bar_distribute_gear_tests.rs b/crates/ui-egui/src/move_bar_distribute_gear_tests.rs index 1026009b09..45ebb91969 100644 --- a/crates/ui-egui/src/move_bar_distribute_gear_tests.rs +++ b/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"); +} diff --git a/crates/ui-egui/src/tools/vector_ui/pen.rs b/crates/ui-egui/src/tools/vector_ui/pen.rs index baa536f3b8..15cff9430f 100644 --- a/crates/ui-egui/src/tools/vector_ui/pen.rs +++ b/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);