Merge pull request #12268 from hectorviov/fix/color-replacement-tolerance-field

Color Replacement: whole-number Tolerance 1-100 with the alert, and the bar's tooltips

crates/engine/src/cmd/brush_cmds.rs

@@ -861,7 +861,8 @@ fn color_replacement_setup(s: &Session, p: &Value) -> Result<(Stroke, ReplaceSet
         mode: parse(p, cmd, "mode")?.unwrap_or(ReplaceMode::Color),
         sampling: parse(p, cmd, "sampling")?.unwrap_or(Sampling::Continuous),
         limits: parse(p, cmd, "limits")?.unwrap_or(Limits::Contiguous),
-        tolerance: pct(p, "tolerance", 30.0),
+        // 1..100 %, as the reference's field takes it: 0 and below run as 1 (#11930).
+        tolerance: pct(p, "tolerance", 30.0).max(0.01),
         anti_alias: flag(p, "antiAlias", true),
         color: brush.color,
         background: s.tools.background,
@@ -1361,7 +1362,7 @@ pub fn specs() -> Vec<CommandSpec> {
         spec!(
             "paint.colorReplacement",
             "Color Replacement Tool",
-            r##"{"points":[…],"brush":{…}?,"size":px?,"mode":"hue|saturation|color|luminosity"="color","sampling":"continuous|once|backgroundSwatch"="continuous","limits":"contiguous|discontiguous|findEdges"="contiguous","tolerance":0..100=30,"antiAlias":bool=true,"color":"#rrggbb"?=foreground,"target":"pixels"="pixels","seed":u64?}"##,
+            r##"{"points":[…],"brush":{…}?,"size":px?,"mode":"hue|saturation|color|luminosity"="color","sampling":"continuous|once|backgroundSwatch"="continuous","limits":"contiguous|discontiguous|findEdges"="contiguous","tolerance":1..100=30,"antiAlias":bool=true,"color":"#rrggbb"?=foreground,"target":"pixels"="pixels","seed":u64?}"##,
             crate::commands::has_paintable,
             color_replacement,
             true

crates/ui-egui/src/color_replacement_tolerance_field_tests.rs

@@ -0,0 +1,173 @@
+//! #11930: the Color Replacement tool's Tolerance takes whole numbers 1..100; a value typed past the
+//! range becomes the closest one with the reference's alert, text that isn't a number changes
+//! nothing; `paint.colorReplacement` runs tolerance 0 and below as 1; the bar's controls carry the
+//! reference's tooltips.
+
+use egui::accesskit::Role;
+use egui::vec2;
+use egui_kittest::Harness;
+use egui_kittest::kittest::{NodeT, Queryable};
+use serde_json::json;
+
+use crate::PhotocraftApp;
+use crate::eraser_ui::TOLERANCE_RANGE_ALERT;
+use crate::state::{DialogKind, Tool};
+
+fn bar() -> Harness<'static, PhotocraftApp> {
+    let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), crate::Services::default());
+    app.run("file.new", json!({"width": 80, "height": 80})).unwrap();
+    app.ui.tool = Tool::ColorReplacement;
+    let mut h = Harness::builder().with_size(vec2(1600.0, 300.0)).build_ui_state(|ui, app| crate::panels::options_bar(app, ui), app);
+    PhotocraftApp::setup_context(&h.ctx, Default::default());
+    h.run_steps(4);
+    h
+}
+
+/// Types `text` into the Tolerance field, then Tabs out of it.
+fn type_tolerance(h: &mut Harness<'static, PhotocraftApp>, text: &str) {
+    h.query_all_by_role(Role::SpinButton).find(|n| n.accesskit_node().label().as_deref() == Some("Tolerance:")).expect("the Tolerance field").click();
+    h.run_steps(1);
+    h.key_press_modifiers(egui::Modifiers::COMMAND, egui::Key::A);
+    h.event(egui::Event::Text(text.into()));
+    h.run_steps(1);
+    h.key_press(egui::Key::Tab);
+    h.run_steps(3);
+}
+
+fn alert(h: &Harness<'static, PhotocraftApp>) -> Option<String> {
+    h.state().ui.dialogs.iter().find(|d| d.kind == DialogKind::Error).and_then(|d| d.fields.get("message")).and_then(|m| m.as_str()).map(str::to_owned)
+}
+
+fn tolerance(h: &Harness<'static, PhotocraftApp>) -> f32 {
+    h.state().ui.tool_options.cr_tolerance
+}
+
+#[test]
+fn past_the_range_takes_the_closest_value_with_the_alert() {
+    for (typed, want) in [("0", 1.0), ("-5", 1.0), ("150", 100.0)] {
+        let mut h = bar();
+        type_tolerance(&mut h, typed);
+        assert_eq!(tolerance(&h), want, "{typed}");
+        assert_eq!(alert(&h).as_deref(), Some(TOLERANCE_RANGE_ALERT), "{typed}");
+    }
+}
+
+#[test]
+fn a_fraction_stores_a_whole_number_and_text_changes_nothing() {
+    let mut h = bar();
+    type_tolerance(&mut h, "12.5");
+    assert_eq!(tolerance(&h), 13.0, "rounded to the nearest whole number, as the Magic Wand's");
+    assert_eq!(alert(&h), None, "in range: no alert");
+    let mut h = bar();
+    let before = tolerance(&h);
+    type_tolerance(&mut h, "abc");
+    assert_eq!(tolerance(&h), before, "not a number: unchanged");
+    assert_eq!(alert(&h), None, "and no alert");
+}
+
+/// Types `text` into the Tolerance field and leaves it there, with the keyboard.
+fn type_without_leaving(h: &mut Harness<'static, PhotocraftApp>, text: &str) {
+    h.query_all_by_role(Role::SpinButton).find(|n| n.accesskit_node().label().as_deref() == Some("Tolerance:")).expect("the Tolerance field").click();
+    h.run_steps(1);
+    h.key_press_modifiers(egui::Modifiers::COMMAND, egui::Key::A);
+    h.event(egui::Event::Text(text.into()));
+    h.run_steps(1);
+}
+
+#[test]
+fn text_typed_over_a_value_past_the_range_does_not_alert() {
+    let mut h = bar();
+    type_without_leaving(&mut h, "150");
+    h.key_press_modifiers(egui::Modifiers::COMMAND, egui::Key::A);
+    h.event(egui::Event::Text("abc".into()));
+    h.run_steps(1);
+    h.key_press(egui::Key::Tab);
+    h.run_steps(3);
+    assert_eq!(alert(&h), None, "the field is left holding text that is no number: no alert");
+}
+
+#[test]
+fn a_value_past_the_range_typed_again_after_text_alerts() {
+    let mut h = bar();
+    type_without_leaving(&mut h, "150");
+    for text in ["abc", "150"] {
+        h.key_press_modifiers(egui::Modifiers::COMMAND, egui::Key::A);
+        h.event(egui::Event::Text(text.into()));
+        h.run_steps(1);
+    }
+    h.key_press(egui::Key::Tab);
+    h.run_steps(3);
+    assert_eq!(tolerance(&h), 100.0);
+    assert_eq!(alert(&h).as_deref(), Some(TOLERANCE_RANGE_ALERT), "the field is left holding 150 again");
+}
+
+#[test]
+fn a_tool_change_while_typing_past_the_range_still_alerts() {
+    let mut h = bar();
+    type_without_leaving(&mut h, "150");
+    assert_eq!(alert(&h), None, "not yet: the field still has the keyboard");
+    // A tool shortcut the field passes by (a custom function key) changes the tool before the
+    // field is left, so the field is never drawn again.
+    h.state_mut().ui.tool = Tool::Brush;
+    h.run_steps(4);
+    assert_eq!(tolerance(&h), 100.0);
+    assert_eq!(alert(&h).as_deref(), Some(TOLERANCE_RANGE_ALERT), "the alert the field owed");
+}
+
+#[test]
+fn the_bar_has_the_reference_tooltips() {
+    for (role, label, tip) in [
+        (Role::ComboBox, "Mode:", "Painting Mode"),
+        (Role::ComboBox, "Limits:", "Determines the spread of replaced color"),
+        (Role::SpinButton, "Tolerance:", "Tolerance for choosing related colors"),
+        (Role::CheckBox, "Anti-alias", "Anti-alias brush application"),
+    ] {
+        let mut h = bar();
+        assert!(h.query_by_label(tip).is_none(), "{label}: no tooltip before the hover");
+        let at = h.query_all_by_role(role).find(|n| n.accesskit_node().label().as_deref() == Some(label)).expect(label).rect().center();
+        h.hover_at(at);
+        h.run_steps(8);
+        assert!(h.query_by_label(tip).is_some(), "{label}: resting on it shows {tip:?}");
+    }
+}
+
+/// A pixel 2 levels off the sampled colour (red 222 against 220) after a Color Replacement click
+/// sampling the 220 red, with `tolerance`: 1 % (2.55 levels) reaches it, 0 % would not.
+fn near_red(tolerance: serde_json::Value) -> [f32; 4] {
+    let mut s = photocraft_engine::Session::new();
+    s.execute("file.new", json!({"width": 80, "height": 80})).unwrap();
+    s.execute("layer.new.layer", json!({})).unwrap();
+    s.execute("edit.fill", json!({"color": "#dc1e1e"})).unwrap();
+    s.execute("select.rect", json!({"x": 40, "y": 0, "width": 40, "height": 80})).unwrap();
+    s.execute("edit.fill", json!({"color": "#de1e1e"})).unwrap();
+    s.execute("select.deselect", json!({})).unwrap();
+    s.execute("tools.setColors", json!({"foreground": "#0000ff"})).unwrap();
+    let p = json!({"points": [[30, 40]], "size": 30, "hardness": 100, "tolerance": tolerance, "limits": "discontiguous", "sampling": "once"});
+    s.execute("paint.colorReplacement", p).unwrap();
+    let st = s.active().unwrap();
+    st.doc.layer(st.active_layer.unwrap()).unwrap().surface().unwrap().rgba(42, 40)
+}
+
+#[test]
+fn the_command_runs_tolerance_below_one_as_one() {
+    let untouched = [222.0 / 255.0, 30.0 / 255.0, 30.0 / 255.0];
+    let one = near_red(json!(1));
+    assert!((one[2] - untouched[2]).abs() > 0.05, "1 % reaches a colour 2 levels away: {one:?}");
+    for t in [json!(0), json!(-5)] {
+        assert_eq!(near_red(t.clone()), one, "tolerance {t} runs as 1");
+    }
+    let spec = photocraft_engine::command_specs().iter().find(|s| s.id == "paint.colorReplacement").unwrap();
+    assert!(spec.params.contains("\"tolerance\":1..100"), "{}", spec.params);
+}
+
+#[test]
+fn a_restored_fraction_reaches_the_stroke_as_a_whole_number() {
+    // The options bar need not have drawn: the stroke's params round it themselves, so a stroke
+    // replaces the same pixels before and after the bar shows the setting.
+    let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), crate::Services::default());
+    for (saved, want) in [(12.5, 13.0), (23.4, 23.0), (0.0, 1.0), (150.0, 100.0)] {
+        app.ui.tool_options.cr_tolerance = saved;
+        let p = crate::retouch_ui::color_replacement_params(&app);
+        assert_eq!(p["tolerance"].as_f64(), Some(want), "{saved}");
+    }
+}

crates/ui-egui/src/panels/options_bar.rs

@@ -43,6 +43,7 @@ pub(super) fn options_bar_height(pro: bool, narrow: bool) -> f32 {
 
 pub fn options_bar(app: &mut PhotocraftApp, ui: &mut egui::Ui) {
     crate::vector_ui::track_arrow_default_selection(app, ui.ctx());
+    crate::eraser_ui::alert_for_a_field_gone(app, ui.ctx());
     let t = Tokens::get(ui.ctx());
     let narrow = app.session.prefs().workspace.narrow_options_bar();
     egui::Panel::top("options_bar")

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

@@ -208,22 +208,31 @@ pub const TOLERANCE_RANGE_ALERT: &str = "An integer between 1% and 100% is requi
 /// closest one. Returns true when the field is left with a value typed out of range, so the
 /// caller shows [`TOLERANCE_RANGE_ALERT`].
 fn tolerance_field(ui: &mut egui::Ui, value: &mut f32) -> bool {
+    whole_tolerance_field(ui, value, "bg-eraser-tolerance-typed", tl!("Erase range of similar colors"))
+}
+
+/// [`tolerance_field`]'s field for any tool's whole-number Tolerance (the Color Replacement tool's
+/// too, #11930): `draft` keys its typed-but-not-left text, `hover` is its tooltip.
+pub(crate) fn whole_tolerance_field(ui: &mut egui::Ui, value: &mut f32, draft: &str, hover: &str) -> bool {
     // A restored setting (an older 0, a fraction) shows and erases as the field would store it.
     *value = stored_tolerance(*value);
     // While the field has the keyboard it gets back what was typed, not the stored value, so
     // its text is left alone mid-entry (`0` on the way to `05` must not turn into `1`).
-    let draft_id = ui.make_persistent_id(egui::IdSalt::new("bg-eraser-tolerance-typed"));
+    let draft_id = ui.make_persistent_id(egui::IdSalt::new(draft));
     // The field's own range is wide, so the typed value reaches here unclamped.
     // A draft only stands while the setting is still the one it stored: a value set elsewhere
     // meanwhile (a tool preset while the bar was hidden) wins.
     let draft = ui.ctx().data(|d| d.get_temp::<(f32, f32)>(draft_id));
     let mut typed = draft.filter(|(_, stored)| *stored == *value).map_or(*value, |(typed, _)| typed);
-    let r = widgets::value_field(ui, &mut typed, -1.0e9..=1.0e9, "%", 62.0).on_hover_text(tl!("Erase range of similar colors"));
+    let r = widgets::value_field(ui, &mut typed, -1.0e9..=1.0e9, "%", 62.0).on_hover_text(hover);
     let out_id = r.id.with("out-of-range");
     // An edit the field never got to finish (the options bar was hidden while it had the
     // keyboard) has nothing left to say once the field is entered again.
     if r.gained_focus() {
-        ui.ctx().data_mut(|d| d.remove_temp::<bool>(out_id));
+        ui.ctx().data_mut(|d| {
+            d.remove_temp::<bool>(out_id);
+            d.remove_temp::<bool>(r.id.with("parses"));
+        });
     }
     if r.changed() && typed.is_finite() {
         *value = typed.round().clamp(1.0, 100.0);
@@ -239,21 +248,73 @@ fn tolerance_field(ui: &mut egui::Ui, value: &mut f32) -> bool {
             ui.ctx().data_mut(|d| d.remove_temp::<(f32, f32)>(draft_id));
         }
     }
+    // Text replaced by something that is no number (`150`, then `abc`) leaves the value alone
+    // when the field is left, with no alert (#12268). A number typed again after it (`150` once
+    // more) owes the alert again, even though the value did not change.
+    let no_number_id = r.id.with("no-number-typed");
+    if r.has_focus() {
+        match widgets::typed_text_parses(ui.ctx(), r.id) {
+            Some(false) => ui.ctx().data_mut(|d| {
+                d.remove_temp::<bool>(out_id);
+                d.insert_temp(no_number_id, true);
+            }),
+            Some(true) if ui.ctx().data_mut(|d| d.remove_temp::<bool>(no_number_id)).unwrap_or(false) => {
+                let out = typed.is_finite() && !(1.0..=100.0).contains(&typed);
+                ui.ctx().data_mut(|d| d.insert_temp(out_id, out));
+            }
+            _ => {}
+        }
+    } else {
+        ui.ctx().data_mut(|d| d.remove_temp::<bool>(no_number_id));
+    }
     if !r.has_focus() {
         ui.ctx().data_mut(|d| d.remove_temp::<(f32, f32)>(draft_id));
     }
-    r.lost_focus() && ui.ctx().data_mut(|d| d.remove_temp::<bool>(out_id)).unwrap_or(false)
+    let alert = r.lost_focus() && ui.ctx().data_mut(|d| d.remove_temp::<bool>(out_id)).unwrap_or(false);
+    // While a value typed out of range waits for the field to be left, the bar notes the pass, so
+    // a field that goes away instead (a tool shortcut that passes the field by) still alerts.
+    let owed = r.has_focus() && ui.ctx().data(|d| d.get_temp::<bool>(out_id)).unwrap_or(false);
+    let pass = ui.ctx().cumulative_pass_nr();
+    ui.ctx().data_mut(|d| {
+        if owed {
+            d.insert_temp(range_alert_owed_id(), pass);
+        } else {
+            d.remove_temp::<u64>(range_alert_owed_id());
+        }
+    });
+    alert
+}
+
+/// Where a whole-number Tolerance field notes the pass in which it last owed the range alert.
+fn range_alert_owed_id() -> egui::Id {
+    egui::Id::unique("whole-tolerance-range-alert-owed")
+}
+
+/// Shows [`TOLERANCE_RANGE_ALERT`] for a whole-number Tolerance field that went away in the last
+/// pass while it held a value typed out of range (the tool changed by a shortcut, #12268). The
+/// options bar calls it before drawing a tool's options. An edit older than that (the bar was
+/// hidden meanwhile) is dropped, as the field drops it once entered again.
+pub(crate) fn alert_for_a_field_gone(app: &mut PhotocraftApp, ctx: &egui::Context) {
+    let Some(seen) = ctx.data(|d| d.get_temp::<u64>(range_alert_owed_id())) else { return };
+    let now = ctx.cumulative_pass_nr();
+    if seen.saturating_add(1) >= now {
+        return;
+    }
+    ctx.data_mut(|d| d.remove_temp::<u64>(range_alert_owed_id()));
+    if seen.saturating_add(2) == now {
+        open_range_alert(app);
+    }
 }
 
 /// A Tolerance as the field stores it: a whole number in 1..=100, the default 50 for a value
 /// that is no number. Strokes use it too, so a restored setting erases the same before and after
 /// the options bar shows it.
-fn stored_tolerance(v: f32) -> f32 {
+pub(crate) fn stored_tolerance(v: f32) -> f32 {
     if v.is_finite() { v.round().clamp(1.0, 100.0) } else { 50.0 }
 }
 
 /// Shows [`TOLERANCE_RANGE_ALERT`]: a modal with OK, once even if more than one pass asks.
-fn open_range_alert(app: &mut PhotocraftApp) {
+pub(crate) fn open_range_alert(app: &mut PhotocraftApp) {
     let message = tl!(TOLERANCE_RANGE_ALERT).to_owned();
     let shown = app.ui.dialogs.iter().any(|d| d.kind == crate::state::DialogKind::Error && d.fields.get("message").and_then(|m| m.as_str()) == Some(&message));
     if !shown {

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

@@ -84,7 +84,9 @@ pub(crate) fn color_replacement_params(app: &PhotocraftApp) -> Value {
         "mode": o.cr_mode,
         "sampling": o.cr_sampling,
         "limits": o.cr_limits,
-        "tolerance": o.cr_tolerance,
+        // As the field stores it (#11930), so a restored fraction replaces the same pixels before
+        // and after the options bar shows it.
+        "tolerance": crate::eraser_ui::stored_tolerance(o.cr_tolerance),
         "antiAlias": o.cr_anti_alias,
         // A mask or channel target is refused: the tool recolours layer pixels only.
         "target": crate::canvas::paint_target(app),
@@ -861,6 +863,11 @@ pub fn options_bar(app: &mut PhotocraftApp, ui: &mut egui::Ui, tool: Tool) -> bo
                 ("luminosity".to_string(), tl!("Luminosity")),
             ];
             crate::widgets::dropdown(ui, "color-replacement-mode", tl!("Mode"), &mut o.cr_mode, &modes, 100.0);
+            // The reference application's tooltips (#11930), on the dropdowns' buttons by the ids their
+            // combo boxes take.
+            if let Some(mode) = ui.ctx().read_response(ui.make_persistent_id(egui::IdSalt::new("color-replacement-mode"))) {
+                mode.on_hover_text(tl!("Painting Mode"));
+            }
             crate::widgets::vline(ui, 22.0);
             ui.spacing_mut().item_spacing.x = 2.0;
             for (k, icon, tip) in [
@@ -881,9 +888,21 @@ pub fn options_bar(app: &mut PhotocraftApp, ui: &mut egui::Ui, tool: Tool) -> bo
                 ("findEdges".to_string(), tl!("Find Edges")),
             ];
             crate::widgets::dropdown(ui, "color-replacement-limits", tl!("Limits:"), &mut o.cr_limits, &limits, 110.0);
+            if let Some(limits) = ui.ctx().read_response(ui.make_persistent_id(egui::IdSalt::new("color-replacement-limits"))) {
+                limits.on_hover_text(tl!("Determines the spread of replaced color"));
+            }
             opt(ui, tl!("Tolerance:"));
-            crate::widgets::value_field(ui, &mut o.cr_tolerance, 0.0..=100.0, "%", 62.0);
-            crate::widgets::checkbox(ui, &mut o.cr_anti_alias, tl!("Anti-alias"));
+            // Whole numbers 1..=100 with the reference's alert, as the Background Eraser's (#11930).
+            let out_of_range = crate::eraser_ui::whole_tolerance_field(
+                ui,
+                &mut o.cr_tolerance,
+                "color-replacement-tolerance-typed",
+                tl!("Tolerance for choosing related colors"),
+            );
+            crate::widgets::checkbox(ui, &mut o.cr_anti_alias, tl!("Anti-alias")).on_hover_text(tl!("Anti-alias brush application"));
+            if out_of_range {
+                crate::eraser_ui::open_range_alert(app);
+            }
         }
         Tool::HistoryBrush => {
             let label = app.session.active().and_then(|st| match photocraft_engine::history_source_cmds::source(st)? {
@@ -987,6 +1006,10 @@ fn pattern_stamp_options(app: &mut PhotocraftApp, ui: &mut egui::Ui) {
 #[cfg(test)]
 mod tests;
 
+#[cfg(test)]
+#[path = "../color_replacement_tolerance_field_tests.rs"]
+mod color_replacement_tolerance_field_tests;
+
 #[cfg(test)]
 #[path = "../cam_options_bar_tests.rs"]
 mod cam_options_bar_tests;

crates/ui-egui/src/widgets.rs

@@ -50,8 +50,8 @@ pub(crate) use fields::compact_wrapping_value_field;
 #[cfg(test)]
 use fields::{FieldOpts, NUMERIC_INPUT_LIMIT, value_field_in};
 pub use fields::{
-    POPUP_ARROW_W, PopupFieldResponse, VALUE_FIELD_H, compact_value_field, elided_label, elided_label_sensed, popup_value_field, value_field,
-    value_field_named, value_field_whole_alerting,
+    POPUP_ARROW_W, PopupFieldResponse, VALUE_FIELD_H, compact_value_field, elided_label, elided_label_sensed, popup_value_field, typed_text_parses,
+    value_field, value_field_named, value_field_whole_alerting,
 };
 pub(crate) use panel::body_margin;
 pub use panel::{

crates/ui-egui/src/widgets/fields.rs

@@ -538,6 +538,8 @@ fn number_edit(
                 };
                 let s = ready.as_str();
                 let v = if hold_typed { zoom_number(s, units) } else { parse_num_in(s, units) };
+                // Whether the text being typed is a number at all, for [`typed_text_parses`].
+                ctx.data_mut(|d| d.insert_temp(id.with("parses"), v.is_some()));
                 if ctx.memory(|m| m.has_focus(id)) && plain(s).is_none() {
                     // Still typing arithmetic: hold it, and stop applying keystrokes once it parses.
                     ctx.data_mut(|d| d.insert_temp(held, v.is_some()));
@@ -570,6 +572,12 @@ fn number_edit(
     resp
 }
 
+/// Whether the text last typed into the number field `id` (a [`value_field`]'s response id) reads
+/// as a number, an arithmetic entry included once it is complete: `None` before anything was typed.
+pub fn typed_text_parses(ctx: &egui::Context, id: egui::Id) -> Option<bool> {
+    ctx.data(|d| d.get_temp::<bool>(id.with("parses")))
+}
+
 /// The zoom a zoom field's text stands for, with at most one decimal separator (`,` or `.`): a
 /// number, or a ratio (`1/2` is 50 %, `5/2` is 250 %). `None` when it is neither, or holds a
 /// second separator (`zoom_field::read` takes that apart).