Merge pull request #12245 from hectorviov/fix/brush-shift-alt-sampler

Painting tools: Shift+Alt+click adds a colour sampler

crates/engine/src/cmd/sampler_cmds.rs

@@ -16,6 +16,12 @@ use crate::{DocState, EngineError, Result, Session};
 /// Photoshop's Color Sampler limit: at most 10 points per document.
 pub const MAX_SAMPLERS: usize = 10;
 
+/// The refusal `view.colorSamplers.add` gives at [`MAX_SAMPLERS`], so a caller can tell it from
+/// other failures.
+pub fn limit_message() -> String {
+    format!("Color Sampler limit reached: at most {MAX_SAMPLERS} points per document")
+}
+
 fn bad(cmd: &str, msg: impl Into<String>) -> EngineError {
     EngineError::BadParams { cmd: cmd.into(), msg: msg.into() }
 }
@@ -103,7 +109,7 @@ fn add(s: &mut Session, p: &Value) -> Result<Value> {
     change(s, |st| {
         let pos = pixel(&st.doc, x, y).ok_or_else(|| EngineError::Other("the sample point is outside the canvas".into()))?;
         if st.color_samplers.len() >= MAX_SAMPLERS {
-            return Err(EngineError::Other(format!("Color Sampler limit reached: at most {MAX_SAMPLERS} points per document")));
+            return Err(EngineError::Other(limit_message()));
         }
         st.color_samplers.push(pos);
         let i = st.color_samplers.len() - 1;

crates/ui-egui/src/brush_shift_alt_sampler_tests.rs

@@ -0,0 +1,170 @@
+//! #11890: Shift+Alt+click with a painting tool that has the Alt eyedropper adds a colour sampler
+//! at the clicked pixel, as the Color Sampler tool places one: the foreground colour stays and
+//! nothing is painted. Alt+click without Shift still sets the foreground; plain clicks paint.
+
+use egui::Modifiers;
+use serde_json::{Value, json};
+
+use crate::PhotocraftApp;
+use crate::canvas::{ToolEvent, tool_event};
+use crate::state::Tool;
+
+const SHIFT_ALT: Modifiers = Modifiers { alt: true, ctrl: false, shift: true, mac_cmd: false, command: false };
+
+/// A 400 × 300 document, a red layer to sample, blue foreground.
+fn app(tool: Tool) -> PhotocraftApp {
+    let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), crate::Services::default());
+    app.run("file.new", json!({"width": 400, "height": 300})).unwrap();
+    app.run("edit.fill", json!({"color": "#ff0000"})).unwrap();
+    app.run("tools.setColors", json!({"foreground": "#0000ff"})).unwrap();
+    app.ui.tool = tool;
+    app.sync_views();
+    app
+}
+
+fn click(app: &mut PhotocraftApp, x: f64, y: f64, mods: Modifiers) {
+    tool_event(app, ToolEvent::Down { x, y, pressure: 1.0 }, mods);
+    // A click's own jitter: a move or two on the spot.
+    tool_event(app, ToolEvent::Move { x, y, pressure: 1.0 }, mods);
+    tool_event(app, ToolEvent::Up { x, y }, mods);
+}
+
+fn samplers(app: &mut PhotocraftApp) -> Vec<Value> {
+    let r = app.run("view.colorSamplers.list", json!({})).unwrap();
+    r.get("samplers").and_then(Value::as_array).cloned().unwrap_or_default()
+}
+
+fn foreground(app: &PhotocraftApp) -> [f32; 4] {
+    app.session.tools.foreground
+}
+
+fn steps(app: &PhotocraftApp) -> usize {
+    app.session.active().unwrap().history.past_len()
+}
+
+#[test]
+fn shift_alt_click_adds_a_sampler_with_every_alt_eyedropper_tool() {
+    for tool in [Tool::Brush, Tool::Pencil, Tool::Gradient, Tool::PaintBucket, Tool::BackgroundEraser] {
+        let mut app = app(tool);
+        let fg = foreground(&app);
+        let before = steps(&app);
+        click(&mut app, 199.7, 120.2, SHIFT_ALT);
+        let got = samplers(&mut app);
+        // The point the Color Sampler tool's command places for the same click.
+        let mut twin = self::app(tool);
+        twin.run("view.colorSamplers.add", json!({"x": 199.7, "y": 120.2})).unwrap();
+        assert_eq!(got, samplers(&mut twin), "{tool:?}: one sampler at the clicked pixel");
+        assert_eq!(got.len(), 1, "{tool:?}");
+        assert_eq!(foreground(&app), fg, "{tool:?}: the foreground stays");
+        assert_eq!(steps(&app), before, "{tool:?}: nothing painted");
+        assert!(!app.alt_sampling, "{tool:?}: the gesture ended");
+    }
+}
+
+#[test]
+fn at_the_limit_the_click_adds_nothing_and_says_so() {
+    let mut app = app(Tool::Brush);
+    for i in 0..10 {
+        app.run("view.colorSamplers.add", json!({"x": 10 + i * 20, "y": 10})).unwrap();
+    }
+    let (fg, before) = (foreground(&app), steps(&app));
+    click(&mut app, 200.0, 150.0, SHIFT_ALT);
+    assert_eq!(samplers(&mut app).len(), 10);
+    assert_eq!(steps(&app), before, "nothing painted");
+    assert!(app.ui.status.contains("limit"), "the Color Sampler tool's limit message: {:?}", app.ui.status);
+    assert!(!app.ui.status_error, "reported as the Color Sampler tool does");
+    assert_eq!(foreground(&app), fg);
+}
+
+#[test]
+fn alt_click_still_samples_and_a_plain_click_still_paints() {
+    let mut app = app(Tool::Brush);
+    click(&mut app, 50.0, 50.0, Modifiers::ALT);
+    assert_eq!(foreground(&app)[..3], [1.0, 0.0, 0.0], "Alt+click picks the red under the pointer");
+    assert!(samplers(&mut app).is_empty());
+    let before = steps(&app);
+    click(&mut app, 80.0, 80.0, Modifiers::NONE);
+    assert_eq!(steps(&app), before + 1, "a plain click paints a dab");
+    assert!(samplers(&mut app).is_empty());
+}
+
+#[test]
+fn a_shift_alt_click_off_the_canvas_adds_nothing_and_never_panics() {
+    let mut app = app(Tool::Brush);
+    let fg = foreground(&app);
+    for (x, y) in [(-20.0, 50.0), (450.0, 50.0), (f64::NAN, 10.0), (10.0, f64::INFINITY)] {
+        click(&mut app, x, y, SHIFT_ALT);
+    }
+    assert!(samplers(&mut app).is_empty());
+    assert_eq!(foreground(&app), fg);
+}
+
+#[test]
+fn shift_alt_click_on_a_sampler_samples_the_foreground_as_before() {
+    // A press on an existing point wasn't measured, so it keeps the Alt eyedropper's sample.
+    let mut app = app(Tool::Brush);
+    app.run("view.colorSamplers.add", json!({"x": 120, "y": 80})).unwrap();
+    let before = samplers(&mut app);
+    click(&mut app, 121.0, 80.5, SHIFT_ALT);
+    assert_eq!(samplers(&mut app), before, "no second point on the first");
+    assert_eq!(foreground(&app)[..3], [1.0, 0.0, 0.0], "the red under the pointer, as an Alt+click");
+}
+
+#[test]
+fn a_shift_alt_click_off_the_canvas_reports_nothing() {
+    // The Color Sampler tool places no point off the canvas and says nothing; so does this press.
+    let mut app = app(Tool::Brush);
+    app.ui.status = "Ready".into();
+    for (x, y) in [(-20.0, 50.0), (450.0, 50.0), (f64::NAN, 10.0)] {
+        click(&mut app, x, y, SHIFT_ALT);
+        assert_eq!(app.ui.status, "Ready", "({x}, {y}): the status bar is left as it was");
+    }
+}
+
+#[test]
+fn a_shift_alt_click_refused_for_another_reason_keeps_the_error_styling() {
+    // Only the limit is the Color Sampler tool's plain report. A Pen draft whose commit failed,
+    // left in another document, refuses every command until it is dealt with: that stays an error.
+    let mut app = app(Tool::Pen);
+    for x in [30.0, 1e40] {
+        tool_event(&mut app, ToolEvent::Down { x, y: 60.0, pressure: 1.0 }, Modifiers::NONE);
+        tool_event(&mut app, ToolEvent::Up { x, y: 60.0 }, Modifiers::NONE);
+    }
+    crate::vector_ui::pen_commit(&mut app, false);
+    assert!(app.ui.pen.as_ref().is_some_and(|p| p.commit_failed), "a failed Pen draft");
+    app.session.execute("file.new", json!({"width": 400, "height": 300})).unwrap();
+    app.ui.tool = Tool::Brush;
+    // The Pen failure styled the status bar as an error already: only the click may set it again.
+    app.ui.status_error = false;
+    click(&mut app, 200.0, 150.0, SHIFT_ALT);
+    assert!(samplers_of(&app).is_empty(), "nothing added");
+    assert!(!app.ui.status.is_empty(), "the refusal is reported");
+    assert!(app.ui.status_error, "and keeps its error styling: {:?}", app.ui.status);
+}
+
+#[test]
+fn a_full_document_refused_for_another_reason_keeps_the_error_styling() {
+    // Ten points and a failed Pen draft in another document: the refusal is the Pen's, not the
+    // limit's, so it stays an error.
+    let mut app = app(Tool::Pen);
+    for x in [30.0, 1e40] {
+        tool_event(&mut app, ToolEvent::Down { x, y: 60.0, pressure: 1.0 }, Modifiers::NONE);
+        tool_event(&mut app, ToolEvent::Up { x, y: 60.0 }, Modifiers::NONE);
+    }
+    crate::vector_ui::pen_commit(&mut app, false);
+    assert!(app.ui.pen.as_ref().is_some_and(|p| p.commit_failed), "a failed Pen draft");
+    app.session.execute("file.new", json!({"width": 400, "height": 300})).unwrap();
+    for i in 0..10 {
+        app.session.execute("view.colorSamplers.add", json!({"x": 10 + i * 20, "y": 10})).unwrap();
+    }
+    app.ui.tool = Tool::Brush;
+    app.ui.status_error = false;
+    click(&mut app, 200.0, 150.0, SHIFT_ALT);
+    assert_eq!(samplers_of(&app).len(), 10, "nothing added");
+    assert!(!app.ui.status.contains("limit"), "the Pen's refusal, not the limit: {:?}", app.ui.status);
+    assert!(app.ui.status_error, "and keeps its error styling: {:?}", app.ui.status);
+}
+
+fn samplers_of(app: &PhotocraftApp) -> Vec<[f64; 2]> {
+    app.session.active().map(|d| d.color_samplers.clone()).unwrap_or_default()
+}

crates/ui-egui/src/canvas/cursors.rs

@@ -212,6 +212,24 @@ pub(super) fn alt_eyedropper(app: &mut PhotocraftApp, ev: ToolEvent, mods: egui:
         return false;
     }
     match ev {
+        // ⇧⌥-click adds a colour sampler at the clicked pixel instead, as the Color Sampler tool
+        // places one: no foreground change, nothing painted (#11890). A refused point (the
+        // document's limit) reports in the status bar as that tool's does; off the canvas, as that
+        // tool, it places none and says nothing. A press on an existing point wasn't measured, so
+        // it samples the foreground as before.
+        ToolEvent::Down { x, y, .. } if mods.shift && !on_sampler(app, x, y) => {
+            let size = app.session.active().map(|d| d.doc.size);
+            let inside = size.is_some_and(|s| (0.0..f64::from(s.width)).contains(&x) && (0.0..f64::from(s.height)).contains(&y));
+            if inside && let Err(e) = app.run("view.colorSamplers.add", json!({"x": x, "y": y})) {
+                // Only the limit is a plain report; any other refusal (a failed Pen draft left in
+                // another document) stays an error, also when the document is full.
+                let limit = e == photocraft_engine::sampler_cmds::limit_message();
+                app.ui.status = e;
+                app.ui.status_error = !limit;
+            }
+        }
+        // While ⇧⌥ stay held after that press, moves sample nothing.
+        ToolEvent::Move { .. } if mods.shift => {}
         // Without ⌥: the sample sets the foreground colour, whatever the Eyedropper would do.
         ToolEvent::Down { x, y, .. } | ToolEvent::Move { x, y, .. } => sample_eyedropper(app, x, y, egui::Modifiers::NONE),
         ToolEvent::Up { .. } => app.alt_sampling = false,
@@ -219,6 +237,17 @@ pub(super) fn alt_eyedropper(app: &mut PhotocraftApp, ev: ToolEvent, mods: egui:
     true
 }
 
+/// Is document point (x, y) on a colour sampler, within the Color Sampler tool's reach of one (7
+/// screen pixels)?
+fn on_sampler(app: &PhotocraftApp, x: f64, y: f64) -> bool {
+    let tol = 7.0 / f64::from(app.point_zoom().max(0.01));
+    app.session.active().is_some_and(|d| d.color_samplers.iter().any(|q| (q[0] - x).hypot(q[1] - y) <= tol))
+}
+
+#[cfg(test)]
+#[path = "../brush_shift_alt_sampler_tests.rs"]
+mod brush_shift_alt_sampler_tests;
+
 /// Sample the image or targeted raster mask at (x, y) using the configured Sample Size.
 /// Write a valid sample to the foreground swatch, or to the background swatch with Alt.
 pub(super) fn sample_eyedropper(app: &mut PhotocraftApp, x: f64, y: f64, mods: egui::Modifiers) {