From fdd4ca7cbcf33fdb2c20044134453fb24fdac015 Mon Sep 17 00:00:00 2001 From: Nigel Ihza Firdausy <106657905+Ni-zav@users.noreply.github.com> Date: Mon, 19 Oct 2026 03:56:39 +0700 Subject: [PATCH] Remove: cancel queued strokes when the paint target changes (#7380) (#7464) * fix(remove): cancel waiting strokes when their target changes * fix(remove): store pending target identity with waiting strokes * test(remove): cover queued strokes across layer and channel switches * Tests: format retouch_ui.rs and drop the unreachable! arm clippy denies Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Brandon Thomas Co-authored-by: Claude Opus 5.5 --- crates/ui-egui/src/lib.rs | 6 +-- crates/ui-egui/src/retouch_ui.rs | 81 ++++++++++++++++++++++++++++---- 2 files changed, 76 insertions(+), 11 deletions(-) diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 2c23a29e36..5948bd4edf 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -605,9 +605,9 @@ pub struct PhotocraftApp { /// Footprint trail of a retouching drag (see `stroke_trail`). trail: Option, /// Remove tool strokes waiting to be applied together (Remove after each stroke off), the - /// document they belong to, and their tint on the canvas (`retouch_ui`). + /// document, layer and channel they belong to, and their tint on the canvas (`retouch_ui`). pub(crate) remove_pending: Vec>, - pub(crate) remove_pending_doc: Option, + pub(crate) remove_pending_target: Option, pub(crate) remove_trail: Option, /// The open menus' items, reused between frames (`menus::ItemCache`). pub(crate) menu_cache: Option, @@ -879,7 +879,7 @@ impl PhotocraftApp { committed_stroke: None, trail: None, remove_pending: Vec::new(), - remove_pending_doc: None, + remove_pending_target: None, remove_trail: None, menu_cache: None, move_preview: None, diff --git a/crates/ui-egui/src/retouch_ui.rs b/crates/ui-egui/src/retouch_ui.rs index 417df92b6e..d79a5385e3 100644 --- a/crates/ui-egui/src/retouch_ui.rs +++ b/crates/ui-egui/src/retouch_ui.rs @@ -166,15 +166,31 @@ fn remove_params(o: &crate::state::ToolOptions) -> Value { json!({"sampleAllLayers": o.sample_all_layers, "newLayer": o.remove_new_layer}) } +/// The selection a queued Remove stroke was drawn against. A later selection must not +/// redirect the queued strokes to a different layer or colour component. +#[derive(Clone, PartialEq)] +pub(crate) struct RemoveTarget { + doc: photocraft_doc::DocId, + layer: Option, + channel: photocraft_engine::channel_cmds::ChannelTarget, + paint: Value, +} + +fn remove_target(app: &PhotocraftApp) -> Option { + let st = app.session.active()?; + Some(RemoveTarget { doc: st.doc.id, layer: st.active_layer, channel: st.channel_view.target, paint: crate::canvas::paint_target(app) }) +} + /// Remove after each stroke off: a finished stroke waits with the others, tinted on the canvas, /// until ↩ or the options bar's ✓ applies them all as one removal (Esc or ⊘ drops them). pub fn remove_pending_add(app: &mut PhotocraftApp, points: &[[f64; 3]]) { + let Some(target) = remove_target(app) else { return }; let Some(st) = app.session.active() else { return }; - let (doc, size) = (st.doc.id, [st.doc.size.width, st.doc.size.height]); - if app.remove_pending_doc != Some(doc) { + let size = [st.doc.size.width, st.doc.size.height]; + if app.remove_pending_target.as_ref() != Some(&target) { app.remove_pending.clear(); app.remove_trail = None; - app.remove_pending_doc = Some(doc); + app.remove_pending_target = Some(target); } let diameter = app.session.tools.brush.size; app.remove_trail.get_or_insert_with(|| crate::stroke_trail::Trail::new(size)).feed_stroke(points, diameter); @@ -185,12 +201,13 @@ pub fn remove_pending_add(app: &mut PhotocraftApp, points: &[[f64; 3]]) { /// Apply the waiting strokes as one removal (one job, one undo step). False when none wait. pub fn remove_pending_commit(app: &mut PhotocraftApp) -> bool { - if app.remove_pending.is_empty() { + // Enter can arrive before the next canvas frame checks pending-stroke lifetime. + if !remove_pending_live(app) { return false; } let strokes = std::mem::take(&mut app.remove_pending); app.remove_trail = None; - app.remove_pending_doc = None; + app.remove_pending_target = None; let mut p = remove_params(&app.ui.tool_options); p["strokes"] = json!(strokes); p["target"] = crate::canvas::paint_target(app); @@ -209,7 +226,7 @@ pub fn remove_pending_cancel(app: &mut PhotocraftApp) -> bool { } app.remove_pending.clear(); app.remove_trail = None; - app.remove_pending_doc = None; + app.remove_pending_target = None; app.ui.status.clear(); app.ui.status_error = false; true @@ -221,8 +238,7 @@ pub fn remove_pending_live(app: &mut PhotocraftApp) -> bool { if app.remove_pending.is_empty() { return false; } - let doc = app.session.active().map(|st| st.doc.id); - if app.ui.tool == Tool::Remove && doc.is_some() && app.remove_pending_doc == doc { + if app.ui.tool == Tool::Remove && remove_target(app).as_ref() == app.remove_pending_target.as_ref() { return true; } remove_pending_cancel(app); @@ -1181,6 +1197,55 @@ mod tests { assert!(!remove_pending_live(&mut app) && app.remove_trail.is_none()); } + #[test] + fn queued_remove_cannot_commit_after_layer_or_channel_target_changes() { + for change in ["layer", "color", "mask"] { + let mut app = app(); + app.run("layer.new.layer", json!({"name": "Original"})).unwrap(); + if change == "color" { + app.run("channel.target", json!({"channel": "red"})).unwrap(); + } + app.ui.tool = Tool::Remove; + app.ui.tool_options.remove_after_each_stroke = false; + remove_pending_add(&mut app, &[[12.0, 10.0, 1.0], [16.0, 12.0, 1.0]]); + assert_eq!(app.remove_pending.len(), 1); + assert!(remove_pending_live(&mut app)); + + match change { + "layer" => { + app.run("layer.new.layer", json!({"name": "Other"})).unwrap(); + } + "color" => { + app.run("channel.target", json!({"channel": "blue"})).unwrap(); + } + _ => { + app.run("layer.layerMask.revealAll", json!({})).unwrap(); + app.ui.mask_target = true; + } + } + + let journal = app.session.journal.len(); + assert!(!remove_pending_commit(&mut app), "{change}: Enter must not retarget a queued stroke"); + assert_eq!(app.session.journal.len(), journal, "{change}: no Remove command ran"); + assert!(app.remove_pending.is_empty() && app.remove_trail.is_none(), "{change}: queue cleared"); + assert!(app.remove_pending_target.is_none()); + } + } + + #[test] + fn queued_remove_starts_a_new_queue_after_target_switch() { + let mut app = app(); + app.run("layer.new.layer", json!({"name": "Original"})).unwrap(); + app.ui.tool = Tool::Remove; + app.ui.tool_options.remove_after_each_stroke = false; + remove_pending_add(&mut app, &[[12.0, 10.0, 1.0], [16.0, 12.0, 1.0]]); + app.run("layer.new.layer", json!({"name": "Other"})).unwrap(); + remove_pending_add(&mut app, &[[40.0, 10.0, 1.0], [44.0, 12.0, 1.0]]); + assert_eq!(app.remove_pending.len(), 1, "strokes from different layers must not be combined"); + assert_eq!(app.remove_pending[0][0][0], 40.0); + assert!(remove_pending_live(&mut app)); + } + #[test] fn remove_strokes_waiting_are_dropped_when_the_tool_changes() { let mut app = app();