Merge pull request #13003 from rmsff/fix/sharpen-refusal-alerts-12952

Sharpen: alert on a hidden, locked or group layer

crates/ui-egui/src/brush_refusal_alert_tests.rs

@@ -9,7 +9,7 @@ use super::*;
 use crate::canvas::{ToolEvent, tool_event};
 
 /// A 64 × 64 document with a painted layer active, black foreground, `tool`.
-fn app(tool: Tool) -> PhotocraftApp {
+pub(super) fn app(tool: Tool) -> PhotocraftApp {
     let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), crate::Services::default());
     app.run("file.new", json!({"width": 64, "height": 64})).unwrap();
     app.run("layer.new.layer", json!({})).unwrap();
@@ -20,26 +20,26 @@ fn app(tool: Tool) -> PhotocraftApp {
     app
 }
 
-fn active(app: &PhotocraftApp) -> u64 {
+pub(super) fn active(app: &PhotocraftApp) -> u64 {
     app.session.active().unwrap().active_layer.unwrap().0
 }
 
-fn steps(app: &PhotocraftApp) -> usize {
+pub(super) fn steps(app: &PhotocraftApp) -> usize {
     app.session.active().unwrap().history.past_len()
 }
 
 /// A press, a short drag and the release with the active tool.
-fn stroke(app: &mut PhotocraftApp) {
+pub(super) fn stroke(app: &mut PhotocraftApp) {
     tool_event(app, ToolEvent::Down { x: 20.0, y: 32.0, pressure: 1.0 }, Modifiers::NONE);
     tool_event(app, ToolEvent::Move { x: 44.0, y: 32.0, pressure: 1.0 }, Modifiers::NONE);
     tool_event(app, ToolEvent::Up { x: 44.0, y: 32.0 }, Modifiers::NONE);
 }
 
-fn alerts(app: &PhotocraftApp) -> Vec<String> {
+pub(super) fn alerts(app: &PhotocraftApp) -> Vec<String> {
     app.ui.dialogs.iter().filter(|d| d.kind == DialogKind::Error).filter_map(|d| d.fields.get("message").and_then(Value::as_str).map(str::to_string)).collect()
 }
 
-fn refused(app: &mut PhotocraftApp, want: &str) {
+pub(super) fn refused(app: &mut PhotocraftApp, want: &str) {
     let before = steps(app);
     stroke(app);
     assert_eq!(alerts(app), [want], "the alert, once");

crates/ui-egui/src/sharpen_refusal_alert_tests.rs

@@ -0,0 +1,95 @@
+//! #12952: when the Sharpen tool can't work on the layer (hidden target layer, Lock All, Lock Image
+//! Pixels, a group's pixels), a press opens the reference application's alert with OK and changes
+//! nothing; Lock transparent pixels still sharpens. The Brush keeps its alerts, the Dodge tool its
+//! colour-mode alert only.
+
+use serde_json::json;
+
+use super::brush_refusal_alert_tests::{active, alerts, app, refused, steps, stroke};
+use super::*;
+use crate::control::{ControlRequest, Outcome, handle};
+
+#[test]
+fn a_hidden_target_layer() {
+    let mut app = app(Tool::Sharpen);
+    let id = active(&app);
+    app.run("layer.setProps", json!({"layer": id, "visible": false})).unwrap();
+    refused(&mut app, "Could not use the sharpen tool because the target layer is hidden.");
+    assert!(app.ui.status.is_empty() || !app.ui.status_error, "no status-bar error as well: {:?}", app.ui.status);
+}
+
+#[test]
+fn lock_all_and_lock_image_pixels() {
+    for lock in ["all", "pixels"] {
+        let mut app = app(Tool::Sharpen);
+        app.run("layer.lockLayers", json!({ lock: true })).unwrap();
+        refused(&mut app, "Could not use the sharpen tool because the layer is locked.");
+    }
+}
+
+#[test]
+fn a_group_is_active() {
+    let mut app = app(Tool::Sharpen);
+    app.run("layer.new.group", json!({})).unwrap();
+    refused(&mut app, "Could not use the sharpen tool because the pixels in a group cannot be edited without merging the group.");
+}
+
+#[test]
+fn the_alert_is_listed_and_confirmed_over_the_control_channel() {
+    let ctx = egui::Context::default();
+    let mut app = app(Tool::Sharpen);
+    app.run("layer.lockLayers", json!({"pixels": true})).unwrap();
+    stroke(&mut app);
+    let mut call = |method: &str, params: serde_json::Value| {
+        let (req, _rx) = ControlRequest::new(method, params);
+        match handle(&mut app, &ctx, &req) {
+            Outcome::Done(v) => v,
+            _ => panic!("{method}: expected an immediate reply"),
+        }
+    };
+    let inspect = call("ui.inspect", json!({}));
+    let dialog = &inspect["result"]["dialogs"][0];
+    assert_eq!(dialog["fields"]["message"], "Could not use the sharpen tool because the layer is locked.", "{inspect}");
+    assert_eq!(call("ui.dialog.confirm", json!({"dialog": dialog["id"]}))["ok"], true);
+    assert!(alerts(&app).is_empty());
+}
+
+#[test]
+fn lock_transparent_pixels_still_sharpens() {
+    let mut app = app(Tool::Sharpen);
+    app.run("layer.lockLayers", json!({"transparency": true})).unwrap();
+    let before = steps(&app);
+    stroke(&mut app);
+    assert!(alerts(&app).is_empty());
+    assert_eq!(steps(&app), before + 1, "the stroke was applied");
+}
+
+#[test]
+fn the_colour_mode_alert_comes_first() {
+    // A hidden layer in an Indexed Color document: the mode alert, as before this change.
+    let mut app = app(Tool::Sharpen);
+    app.run("image.mode.indexedColor", json!({"palette": "exact", "dither": "none", "forced": "none"})).unwrap();
+    let id = active(&app);
+    app.run("layer.setProps", json!({"layer": id, "visible": false})).unwrap();
+    refused(&mut app, "Could not use the sharpen tool because it does not work with Indexed Color images (convert image to RGB Color mode to edit).");
+}
+
+#[test]
+fn the_brush_and_the_dodge_tool_keep_todays_behaviour() {
+    // The Brush: no alert for Lock Image Pixels.
+    let mut app = app(Tool::Brush);
+    app.run("layer.lockLayers", json!({"pixels": true})).unwrap();
+    stroke(&mut app);
+    assert!(alerts(&app).is_empty());
+    // The Dodge tool in an RGB document: no layer alerts, the status bar says a hidden target.
+    let mut app = self::app(Tool::Dodge);
+    let id = active(&app);
+    app.run("layer.setProps", json!({"layer": id, "visible": false})).unwrap();
+    stroke(&mut app);
+    assert!(alerts(&app).is_empty());
+    assert_eq!(app.ui.status, photocraft_engine::hidden_target::HIDDEN);
+    let mut app = self::app(Tool::Dodge);
+    app.run("layer.lockLayers", json!({"all": true})).unwrap();
+    assert_eq!(alert(&app, Tool::Dodge, false), None);
+    assert_eq!(tool_name(Tool::Dodge), None);
+}

crates/ui-egui/src/tools/brush_refusal.rs

@@ -19,8 +19,8 @@ pub const INDEXED: &str = "Could not use the {tool} because it does not work wit
 pub const BITMAP: &str = "Could not use the {tool} because it does not work with Bitmap images (convert image to Grayscale mode to edit).";
 
 /// The tool's name in the colour-mode alerts, for the tools whose wording was measured: the Dodge
-/// and Sharpen tools. Kept apart from [`tool_name`]: the layer alerts aren't wired for these tools
-/// yet (#12685, #12952).
+/// and Sharpen tools. Kept apart from [`tool_name`]: the layer alerts aren't wired for the Dodge
+/// tool yet (#12685).
 fn mode_tool_name(tool: Tool) -> Option<&'static str> {
     match tool {
         Tool::Dodge => Some(tl!("dodge tool")),
@@ -29,10 +29,12 @@ fn mode_tool_name(tool: Tool) -> Option<&'static str> {
     }
 }
 
-/// The tool's name in the alerts, for the tools whose wording was measured: the Brush.
+/// The tool's name in the alerts, for the tools whose wording was measured: the Brush and the
+/// Sharpen tool (#12952).
 pub fn tool_name(tool: Tool) -> Option<&'static str> {
     match tool {
         Tool::Brush => Some(tl!("brush tool")),
+        Tool::Sharpen => Some(tl!("sharpen tool")),
         _ => None,
     }
 }
@@ -40,14 +42,15 @@ pub fn tool_name(tool: Tool) -> Option<&'static str> {
 /// The alert a press with `tool` gets, if any. `hidden` is the press-time hidden-target refusal
 /// (`canvas::tool_input`), which this only words; the lock and the group are checked here. Lock
 /// transparent pixels is no refusal, and a group's targeted mask paints. A tool with a
-/// [`mode_tool_name`] gets only its colour-mode alert, in an Indexed Color or Bitmap document.
+/// [`mode_tool_name`] gets its colour-mode alert first, in an Indexed Color or Bitmap document.
 pub fn alert(app: &PhotocraftApp, tool: Tool, hidden: bool) -> Option<String> {
-    if let Some(name) = mode_tool_name(tool) {
-        let template = match app.session.active()?.doc.mode {
-            photocraft_doc::ColorMode::Indexed => INDEXED,
-            photocraft_doc::ColorMode::Bitmap => BITMAP,
-            _ => return None,
-        };
+    if let Some(name) = mode_tool_name(tool)
+        && let Some(template) = match app.session.active()?.doc.mode {
+            photocraft_doc::ColorMode::Indexed => Some(INDEXED),
+            photocraft_doc::ColorMode::Bitmap => Some(BITMAP),
+            _ => None,
+        }
+    {
         return Some(crate::i18n::fmt(tl!(template), &[("tool", name)]));
     }
     let name = tool_name(tool)?;
@@ -61,7 +64,7 @@ pub fn alert(app: &PhotocraftApp, tool: Tool, hidden: bool) -> Option<String> {
         }
         if layer.is_group() {
             GROUP
-        } else if st.doc.effective_locks(layer.id).all {
+        } else if locked(st.doc.effective_locks(layer.id), tool) {
             LOCKED
         } else {
             return None;
@@ -70,6 +73,12 @@ pub fn alert(app: &PhotocraftApp, tool: Tool, hidden: bool) -> Option<String> {
     Some(crate::i18n::fmt(tl!(template), &[("tool", name)]))
 }
 
+/// Whether `locks` refuse `tool`: Lock All, and for the Sharpen tool Lock Image Pixels as well
+/// (measured for it; the Brush keeps today's behaviour there).
+fn locked(locks: photocraft_doc::Locks, tool: Tool) -> bool {
+    locks.all || (tool == Tool::Sharpen && locks.pixels)
+}
+
 /// Opens the alert: ordinary dialog state, listed by `ui.inspect` and closed by `ui.dialog.confirm`.
 pub fn prompt(app: &mut PhotocraftApp, message: &str) {
     app.ui.open_dialog(DialogKind::Error, json!({ "message": message }).as_object().cloned().unwrap_or_default());
@@ -78,3 +87,7 @@ pub fn prompt(app: &mut PhotocraftApp, message: &str) {
 #[cfg(test)]
 #[path = "../brush_refusal_alert_tests.rs"]
 mod brush_refusal_alert_tests;
+
+#[cfg(test)]
+#[path = "../sharpen_refusal_alert_tests.rs"]
+mod sharpen_refusal_alert_tests;