Merge pull request #12843 from rmsff/fix/red-eye-history-state-12573

Red Eye: name the step Red Eye Removal Tool and record a click that changes nothing

crates/engine/src/cmd/redeye_cmds.rs

@@ -4,6 +4,8 @@
 //! runs on a window around each click plus the mask-filter halo.
 
 use photocraft_algo::redeye::{self, MAX_POINTS, MAX_WINDOW};
+use photocraft_color::ColorMode;
+use photocraft_doc::{Document, LayerId};
 use photocraft_geom::Rect;
 use photocraft_raster::{Surface, from_rgba_into};
 use serde_json::{Value, json};
@@ -198,6 +200,17 @@ fn apply_click(surf: &mut Surface, sel: Option<&Surface>, lock: bool, click: (i3
     Ok((area, changed))
 }
 
+/// Whether a click that changed no pixel still ran the tool, leaving one unchanged history state
+/// as in Photoshop. Photoshop refuses non-RGB/Lab images, hidden layers and a click that starts
+/// outside the selection; alpha-channel, Quick Mask and layer/filter-mask targets, off-canvas
+/// clicks and clicks on locked transparency were not measured and keep the miss.
+fn runs_unchanged(doc: &Document, id: Option<LayerId>, p: &Value, first: (i32, i32)) -> bool {
+    let pixels = crate::channel_cmds::target_of(p) == crate::channel_cmds::Target::Pixels;
+    let shown = id.and_then(|id| doc.path_of(id)).is_some_and(|path| doc.shown_at(&path));
+    let selected = doc.selection.as_ref().is_none_or(|m| m.sample_channel(first.0, first.1, 0) > 0.0);
+    matches!(doc.mode, ColorMode::Rgb | ColorMode::Lab) && pixels && shown && selected && doc.bounds().contains(first.0, first.1)
+}
+
 fn enabled(s: &Session) -> std::result::Result<(), String> {
     crate::commands::has_paintable(s)?;
     if crate::channel_cmds::edits_channel(s) {
@@ -223,10 +236,14 @@ fn red_eye(s: &mut Session, p: &Value) -> Result<Value> {
     let pupil = pupil_raw.clamp(1.0, 100.0);
     let darken = darken.clamp(0.0, 100.0);
     let id = if p.get("layer").is_some() { Some(crate::commands::layer_param(s, p)?) } else { s.active().and_then(|d| d.active_layer) };
-    let (damage, total) = s.edit("Red Eye", |doc, active| {
+    let (damage, total) = s.edit("Red Eye Removal Tool", |doc, active| {
         let canvas = doc.bounds();
         let sel = doc.selection.clone();
-        let (surf, lock) = crate::channel_cmds::target_surface(doc, id.or(*active), p)?;
+        let layer = id.or(*active);
+        let (surf, lock) = crate::channel_cmds::target_surface(doc, layer, p)?;
+        let Some(&first) = pts.first() else { return Err(EngineError::Other(MISS.into())) };
+        // The Background always locks transparency, so only a click on a clear pixel counts.
+        let on_locked_clear = lock && surf.rgba(first.0, first.1)[3] <= 1.0e-5;
         let mut total = 0u32;
         let mut damage = Rect::EMPTY;
         for click in &pts {
@@ -235,7 +252,7 @@ fn red_eye(s: &mut Session, p: &Value) -> Result<Value> {
             damage = if damage.is_empty() { area } else { damage.union(&area) };
         }
         if total == 0 {
-            return Err(EngineError::Other(MISS.into()));
+            return if !on_locked_clear && runs_unchanged(doc, layer, p, first) { Ok((Rect::EMPTY, 0)) } else { Err(EngineError::Other(MISS.into())) };
         }
         Ok((damage, total))
     })?;
@@ -337,8 +354,8 @@ mod tests {
     fn miss_and_bad_params_are_errors() {
         let mut s = session(32, 32, 8, "rgb");
         paint_layer(&mut s, |_, _| [0.1, 0.2, 0.8, 1.0]);
-        let err = s.execute("paint.redEye", json!({"x": 16, "y": 16})).unwrap_err().to_string();
-        assert!(err.contains(MISS), "{err}");
+        let r = s.execute("paint.redEye", json!({"x": 16, "y": 16})).unwrap();
+        assert_eq!(r["pixels"], 0, "a miss runs without changing a pixel: {r}");
         assert!(s.execute("paint.redEye", json!({})).is_err());
         assert!(s.execute("paint.redEye", json!({"x": null, "y": 1})).is_err());
         let pts: Vec<[i32; 2]> = (0..65).map(|i| [i, 0]).collect();
@@ -439,3 +456,7 @@ mod tests {
         assert!(s.execute("paint.redEye", json!({"x": 12, "y": 12})).is_err());
     }
 }
+
+#[cfg(test)]
+#[path = "redeye_history_state_tests.rs"]
+mod redeye_history_state_tests;

crates/engine/src/cmd/redeye_history_state_tests.rs

@@ -0,0 +1,165 @@
+//! #12573: every Red Eye click that runs adds one History state named "Red Eye Removal Tool",
+//! also when it changes no pixel; clicks the tool refuses add none and stay errors.
+
+use serde_json::{Value, json};
+
+use super::*;
+
+const STEP: &str = "Red Eye Removal Tool";
+
+fn session(mode: &str) -> Session {
+    session_at(mode, 8)
+}
+
+fn session_at(mode: &str, depth: u64) -> Session {
+    let mut s = Session::new();
+    s.execute("file.new", json!({"width": 64, "height": 64, "mode": mode, "depth": depth})).unwrap();
+    s
+}
+
+fn last(s: &Session) -> (usize, Option<String>) {
+    let h = &s.active().unwrap().history;
+    (h.past_len(), h.entries().last().map(|e| e.to_string()))
+}
+
+/// The active layer's pixels, if it has any (a Multichannel document may not).
+fn pixels(s: &Session) -> Option<Vec<f32>> {
+    let d = s.active().unwrap();
+    let surf = d.doc.layer(d.active_layer?)?.surface()?;
+    Some(surf.read_region(d.doc.bounds()))
+}
+
+fn red_dot(s: &mut Session) {
+    s.execute("paint.pencil", json!({"points": [[32, 32]], "size": 14, "hardness": 100, "color": "#f21818"})).unwrap();
+}
+
+/// Runs `params`, which must change no pixel, and checks it leaves one unchanged state that
+/// Undo takes back and Redo puts back.
+fn unchanged_state(s: &mut Session, params: Value, case: &str) {
+    let (n, _) = last(s);
+    let before = pixels(s);
+    let r = s.execute("paint.redEye", params).unwrap_or_else(|e| panic!("{case}: {e}"));
+    assert_eq!(r["pixels"], 0, "{case}: {r}");
+    assert_eq!(last(s), (n + 1, Some(STEP.to_string())), "{case}");
+    assert!(pixels(s) == before, "{case}: no pixel changes");
+    assert!(s.undo(), "{case}");
+    assert_eq!(last(s).0, n, "{case}: undo removes the state");
+    assert!(pixels(s) == before, "{case}");
+    assert!(s.redo(), "{case}");
+    assert_eq!(last(s), (n + 1, Some(STEP.to_string())), "{case}: redo");
+    assert!(pixels(s) == before, "{case}: redo");
+}
+
+fn refused(s: &mut Session, params: Value, case: &str) {
+    let (n, _) = last(s);
+    let before = pixels(s);
+    assert!(s.execute("paint.redEye", params).is_err(), "{case}");
+    assert_eq!(last(s).0, n, "{case}: no state");
+    assert!(pixels(s) == before, "{case}");
+}
+
+#[test]
+fn a_correcting_click_is_one_red_eye_removal_tool_state_that_undoes() {
+    let mut s = session("rgb");
+    s.execute("layer.new.layer", json!({})).unwrap();
+    red_dot(&mut s);
+    let (n, _) = last(&s);
+    let before = pixels(&s);
+    let r = s.execute("paint.redEye", json!({"x": 32, "y": 32})).unwrap();
+    assert!(r["pixels"].as_u64().unwrap() > 0, "{r}");
+    assert_eq!(last(&s), (n + 1, Some(STEP.to_string())));
+    let after = pixels(&s);
+    assert!(after != before);
+    // A second click on the fixed eye is a second state.
+    s.execute("paint.redEye", json!({"x": 32, "y": 32})).unwrap();
+    assert_eq!(last(&s), (n + 2, Some(STEP.to_string())));
+    assert!(s.undo() && s.undo());
+    assert!(pixels(&s) == before, "undo restores the pixels");
+    assert!(s.redo());
+    assert!(pixels(&s) == after, "redo applies the click again");
+}
+
+#[test]
+fn a_click_that_changes_no_pixel_still_adds_one_state() {
+    for depth in [8, 16, 32] {
+        let mut s = session_at("rgb", depth);
+        // On the Background, which locks transparency.
+        s.execute("edit.fill", json!({"color": "#283c78"})).unwrap();
+        unchanged_state(&mut s, json!({"x": 32, "y": 32}), &format!("dark blue pupil, {depth} bit"));
+        s.execute("layer.new.layer", json!({})).unwrap();
+        unchanged_state(&mut s, json!({"x": 32, "y": 32}), &format!("empty transparent layer, {depth} bit"));
+        red_dot(&mut s);
+        unchanged_state(&mut s, json!({"x": 6, "y": 6, "pupilSize": 1}), &format!("transparent area of a layer, {depth} bit"));
+        unchanged_state(&mut s, json!({"x": 32, "y": 32, "darken": 0}), &format!("Darken Amount 0, {depth} bit"));
+    }
+    let mut s = session("rgb");
+    s.execute("layer.new.layer", json!({})).unwrap();
+    red_dot(&mut s);
+    unchanged_state(&mut s, json!({"points": [[6, 6], [58, 6], [6, 58]], "pupilSize": 1}), "a points batch");
+    let mut lab = session("lab");
+    lab.execute("edit.fill", json!({"color": "#283c78"})).unwrap();
+    unchanged_state(&mut lab, json!({"x": 32, "y": 32}), "Lab document");
+}
+
+#[test]
+fn a_points_batch_is_one_state() {
+    let mut s = session("rgb");
+    s.execute("layer.new.layer", json!({})).unwrap();
+    red_dot(&mut s);
+    let (n, _) = last(&s);
+    let before = pixels(&s);
+    s.execute("paint.redEye", json!({"points": [[6, 6], [32, 32]], "pupilSize": 15})).unwrap();
+    assert_eq!(last(&s), (n + 1, Some(STEP.to_string())));
+    assert!(pixels(&s) != before);
+}
+
+#[test]
+fn refused_clicks_add_no_state() {
+    let mut s = session("rgb");
+    s.execute("layer.new.layer", json!({})).unwrap();
+    for (params, case) in [
+        (json!({}), "no click"),
+        (json!({"x": "nope", "y": 1}), "bad coordinates"),
+        (json!({"points": []}), "empty batch"),
+        (json!({"x": 8, "y": 8, "pupilSize": 10_000.0}), "window too large"),
+        (json!({"x": 80, "y": 8}), "off the canvas"),
+    ] {
+        refused(&mut s, params, case);
+    }
+    s.execute("select.rect", json!({"x": 40, "y": 40, "width": 10, "height": 10})).unwrap();
+    refused(&mut s, json!({"x": 8, "y": 8}), "a click outside the selection");
+    unchanged_state(&mut s, json!({"x": 44, "y": 44}), "a click inside the selection");
+    s.execute("select.deselect", json!({})).unwrap();
+    let id = s.active().unwrap().active_layer.unwrap();
+    s.execute("layer.setProps", json!({"layer": id.0, "visible": false})).unwrap();
+    refused(&mut s, json!({"x": 8, "y": 8}), "a hidden layer");
+    s.execute("layer.setProps", json!({"layer": id.0, "visible": true})).unwrap();
+    s.execute("layer.lockLayers", json!({"transparency": true})).unwrap();
+    refused(&mut s, json!({"x": 8, "y": 8}), "a clear pixel under Lock transparent pixels");
+    s.execute("layer.lockLayers", json!({"transparency": false})).unwrap();
+    s.execute("layer.setProps", json!({"locks": {"pixels": true}})).unwrap();
+    refused(&mut s, json!({"x": 8, "y": 8}), "a locked layer");
+    for mode in ["gray", "cmyk"] {
+        let mut s = session(mode);
+        s.execute("edit.fill", json!({"color": "#283c78"})).unwrap();
+        refused(&mut s, json!({"x": 32, "y": 32}), mode);
+    }
+    for (convert, params) in
+        [("image.mode.indexedColor", json!({"palette": "exact", "dither": "none", "forced": "none"})), ("image.mode.multichannel", json!({}))]
+    {
+        let mut s = session("rgb");
+        s.execute("edit.fill", json!({"color": "#283c78"})).unwrap();
+        s.execute(convert, params).unwrap();
+        refused(&mut s, json!({"x": 32, "y": 32}), convert);
+    }
+}
+
+#[test]
+fn channel_and_quick_mask_targets_keep_the_miss() {
+    let mut s = session("rgb");
+    s.execute("channel.new", json!({})).unwrap();
+    refused(&mut s, json!({"x": 32, "y": 32, "target": {"channel": 0}}), "an alpha channel");
+    let mut s = session("rgb");
+    s.execute("select.editInQuickMaskMode", json!({"on": true})).unwrap();
+    refused(&mut s, json!({"x": 32, "y": 32, "target": "quickMask"}), "the Quick Mask");
+}

crates/engine/tests/multi_channel_editing.rs

@@ -1539,8 +1539,8 @@ fn named_red_eye_target_does_not_bypass_channel_protection_on_the_active_layer()
     .unwrap();
     s.execute("channel.target", json!({"colors":[1,2]})).unwrap();
     let before = surface(&s);
-    let error = s.execute("paint.redEye", json!({"x":32,"y":32,"layer":"other"})).unwrap_err();
-    assert!(error.to_string().contains("no red-eye pixels"));
+    let result = s.execute("paint.redEye", json!({"x":32,"y":32,"layer":"other"})).unwrap();
+    assert_eq!(result["pixels"], 0, "{result}");
     assert!(surface(&s) == before);
 }
 

crates/ui-egui/src/tools/retouch_ui/tests.rs

@@ -725,7 +725,7 @@ fn content_aware_move_transform_on_drop_turns_the_patch_on_enter() {
 }
 
 #[test]
-fn red_eye_click_corrects_a_pupil_and_errors_on_a_miss() {
+fn red_eye_click_corrects_a_pupil_and_a_miss_adds_an_unchanged_state() {
     let mut app = app();
     app.run("paint.pencil", json!({"points": [[50, 30]], "size": 18, "hardness": 100, "color": "#f21818"})).unwrap();
     let before = active(&app).surface().unwrap().rgba(50, 30);
@@ -739,10 +739,14 @@ fn red_eye_click_corrects_a_pupil_and_errors_on_a_miss() {
     assert_eq!(app.session.journal.last().map(|(id, _)| id.as_str()), Some("paint.redEye"));
     let after = active(&app).surface().unwrap().rgba(50, 30);
     assert!(after[0] < before[0] - 0.15, "before {before:?} after {after:?}");
+    // A miss changes nothing but still adds a state, as in Photoshop (#12573).
+    let steps = app.session.active().unwrap().history.past_len();
     tool_event(&mut app, ToolEvent::Down { x: 8.0, y: 8.0, pressure: 1.0 }, egui::Modifiers::NONE);
     tool_event(&mut app, ToolEvent::Up { x: 8.0, y: 8.0 }, egui::Modifiers::NONE);
-    assert!(app.ui.status_error);
-    assert!(app.ui.status.contains("no red-eye pixels"), "{}", app.ui.status);
+    assert!(!app.ui.status_error, "{}", app.ui.status);
+    let history = &app.session.active().unwrap().history;
+    assert_eq!(history.past_len(), steps + 1);
+    assert_eq!(history.entries().last().map(|e| e.to_string()).as_deref(), Some("Red Eye Removal Tool"));
 }
 
 #[test]