Merge PR #11535: The healing slot starts on Spot Healing Brush and the brush slot lists Color Replacement before Mixer Brush (#11375)

crates/engine/src/shell_commands.rs

@@ -122,8 +122,8 @@ pub const TOOLS: &[(&str, &str, Option<&str>)] = &[
     ("tools.select.count", "Count Tool", Some("I")),
     ("tools.select.brush", "Brush Tool", Some("B")),
     ("tools.select.pencil", "Pencil Tool", Some("B")),
-    ("tools.select.mixerBrush", "Mixer Brush Tool", Some("B")),
     ("tools.select.colorReplacement", "Color Replacement Tool", Some("B")),
+    ("tools.select.mixerBrush", "Mixer Brush Tool", Some("B")),
     ("tools.select.eraser", "Eraser Tool", Some("E")),
     ("tools.select.backgroundEraser", "Background Eraser Tool", Some("E")),
     ("tools.select.magicEraser", "Magic Eraser Tool", Some("E")),
@@ -136,8 +136,8 @@ pub const TOOLS: &[(&str, &str, Option<&str>)] = &[
     ("tools.select.hand", "Hand Tool", Some("H")),
     ("tools.select.rotateView", "Rotate View Tool", Some("R")),
     ("tools.select.zoom", "Zoom Tool", Some("Z")),
-    ("tools.select.remove", "Remove Tool", Some("J")),
     ("tools.select.spotHealing", "Spot Healing Brush Tool", Some("J")),
+    ("tools.select.remove", "Remove Tool", Some("J")),
     ("tools.select.healing", "Healing Brush Tool", Some("J")),
     ("tools.select.patch", "Patch Tool", Some("J")),
     ("tools.select.contentAwareMove", "Content-Aware Move Tool", Some("J")),

crates/ui-egui/src/lib.rs

@@ -799,6 +799,8 @@ mod pencil_tests;
 mod tool_options_theme_tests;
 #[cfg(test)]
 mod tool_shortcut_tests;
+#[cfg(test)]
+mod toolbar_group_order_tests;
 
 #[cfg(test)]
 mod pixel_aspect_tests;

crates/ui-egui/src/panels.rs

@@ -60,7 +60,7 @@ pub mod swatches_ui;
 pub(super) mod timeline_ui;
 pub(crate) mod title_bar;
 pub(super) mod titlebar;
-mod toolbar;
+pub(crate) mod toolbar;
 pub mod type_panels_ui;
 pub(super) mod variables_ui;
 

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

@@ -6,7 +6,7 @@ use super::{Align2, Color32, PhotocraftApp, Pos2, Rect, Sense, Stroke, StrokeKin
 /// Tool groups separated by hairlines, Photoshop order.
 /// Photoshop toolbar: divider-separated sections of slots; each slot is a flyout group whose button
 /// shows the group's last-used tool.
-pub(super) const TOOL_SECTIONS: &[&[&[Tool]]] = &[
+pub(crate) const TOOL_SECTIONS: &[&[&[Tool]]] = &[
     &[&[Tool::Move, Tool::Artboard]],
     &[
         &[Tool::RectMarquee, Tool::EllipseMarquee, Tool::SingleRowMarquee, Tool::SingleColumnMarquee],
@@ -17,8 +17,8 @@ pub(super) const TOOL_SECTIONS: &[&[&[Tool]]] = &[
         &[Tool::Eyedropper, Tool::ColorSampler, Tool::Ruler, Tool::Note, Tool::Count],
     ],
     &[
-        &[Tool::Remove, Tool::SpotHealing, Tool::Healing, Tool::Patch, Tool::ContentAwareMove, Tool::RedEye],
-        &[Tool::Brush, Tool::Pencil, Tool::MixerBrush, Tool::ColorReplacement],
+        &[Tool::SpotHealing, Tool::Remove, Tool::Healing, Tool::Patch, Tool::ContentAwareMove, Tool::RedEye],
+        &[Tool::Brush, Tool::Pencil, Tool::ColorReplacement, Tool::MixerBrush],
         &[Tool::CloneStamp, Tool::PatternStamp],
         &[Tool::HistoryBrush, Tool::ArtHistoryBrush],
         &[Tool::Eraser, Tool::BackgroundEraser, Tool::MagicEraser],
@@ -46,7 +46,7 @@ fn slot_tool(ui: &egui::Ui, current: Tool, slot: &[Tool], key: egui::Id) -> Tool
 
 /// Next tool in a toolbar flyout slot after `from`, wrapping (Photoshop Alt/Option-click). An
 /// empty slot returns `from` unchanged rather than dividing by zero.
-pub(super) fn next_slot_tool(slot: &[Tool], from: Tool) -> Tool {
+pub(crate) fn next_slot_tool(slot: &[Tool], from: Tool) -> Tool {
     let i = slot.iter().position(|&t| t == from).unwrap_or(0);
     i.checked_add(1).and_then(|n| n.checked_rem(slot.len())).and_then(|n| slot.get(n)).copied().unwrap_or(from)
 }

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

@@ -293,8 +293,8 @@ fn red_eye_is_in_the_j_flyout() {
     let j = TOOL_SECTIONS.iter().flat_map(|section| section.iter()).find(|slot| slot.contains(&Tool::SpotHealing)).expect("J group");
     assert!(j.contains(&Tool::RedEye), "{j:?}");
     assert_eq!(j.last(), Some(&Tool::RedEye));
-    // Remove leads the flyout, so the slot shows it until another J tool is picked.
-    assert_eq!(j.first(), Some(&Tool::Remove));
+    // Spot Healing Brush leads the flyout, so the slot shows it until another J tool is picked.
+    assert_eq!(j.first(), Some(&Tool::SpotHealing));
 }
 
 #[test]
@@ -348,8 +348,8 @@ fn long_press_brush_selects_color_replacement() {
     frame(&mut app, &ctx, 0.1, vec![]);
     let index = TOOL_SECTIONS.iter().flat_map(|section| section.iter()).position(|slot| slot.contains(&Tool::Brush)).unwrap();
     let slot = TOOL_SECTIONS.iter().flat_map(|section| section.iter()).nth(index).copied().unwrap();
-    // Photoshop's Brush group, Color Replacement after the Mixer Brush.
-    assert_eq!(slot, &[Tool::Brush, Tool::Pencil, Tool::MixerBrush, Tool::ColorReplacement]);
+    // Photoshop's Brush group, Color Replacement before the Mixer Brush.
+    assert_eq!(slot, &[Tool::Brush, Tool::Pencil, Tool::ColorReplacement, Tool::MixerBrush]);
     let bx = if Tokens::get(&ctx).pro { 30.0 } else { 36.0 };
     let mut buttons: Vec<Rect> = ctx.viewport(|v| {
         v.prev_pass
@@ -369,8 +369,8 @@ fn long_press_brush_selects_color_replacement() {
     frame(&mut app, &ctx, 1.45, vec![]);
     let key = egui::Id::unique(("tool-slot", index));
     let menu = ctx.memory(|m| m.area_rect(key.with("flyout"))).unwrap();
-    // The last of the four 26-point rows.
-    let row = egui::pos2(menu.left() + 65.0, menu.bottom() - 20.0);
+    // The third of the four 26-point rows.
+    let row = egui::pos2(menu.left() + 65.0, menu.bottom() - 46.0);
     frame(&mut app, &ctx, 2.0, vec![egui::Event::PointerMoved(row), pointer(row, true)]);
     frame(&mut app, &ctx, 2.05, vec![pointer(row, false)]);
     assert_eq!(app.ui.tool, Tool::ColorReplacement);

crates/ui-egui/src/pencil_tests.rs

@@ -259,7 +259,7 @@ fn the_eraser_in_block_mode_erases_a_16_screen_px_square() {
 }
 
 #[test]
-fn b_cycles_brush_pencil_mixer_brush_and_color_replacement() {
+fn b_cycles_brush_pencil_color_replacement_and_mixer_brush() {
     let mut h = Harness::builder().with_size(vec2(1280.0, 800.0)).with_max_steps(64).build_eframe(|cc| {
         PhotocraftApp::setup_context(&cc.egui_ctx, Default::default());
         let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), Default::default());
@@ -279,9 +279,9 @@ fn b_cycles_brush_pencil_mixer_brush_and_color_replacement() {
     press(&mut h, Modifiers::NONE);
     assert_eq!(h.state().ui.tool, Tool::Pencil);
     press(&mut h, Modifiers::NONE);
-    assert_eq!(h.state().ui.tool, Tool::MixerBrush);
-    press(&mut h, Modifiers::NONE);
     assert_eq!(h.state().ui.tool, Tool::ColorReplacement);
+    press(&mut h, Modifiers::NONE);
+    assert_eq!(h.state().ui.tool, Tool::MixerBrush);
     // Use Shift Key for Tool Switch: ⇧B cycles, B keeps the group's tool.
     h.state_mut().session.edit_prefs(|p| p.tools.use_shift_key_for_tool_switch = true);
     press(&mut h, Modifiers::SHIFT);
@@ -289,11 +289,11 @@ fn b_cycles_brush_pencil_mixer_brush_and_color_replacement() {
     press(&mut h, Modifiers::SHIFT);
     assert_eq!(h.state().ui.tool, Tool::Pencil);
     press(&mut h, Modifiers::SHIFT);
-    assert_eq!(h.state().ui.tool, Tool::MixerBrush);
+    assert_eq!(h.state().ui.tool, Tool::ColorReplacement);
     press(&mut h, Modifiers::SHIFT);
-    assert_eq!(h.state().ui.tool, Tool::ColorReplacement);
+    assert_eq!(h.state().ui.tool, Tool::MixerBrush);
     press(&mut h, Modifiers::NONE);
-    assert_eq!(h.state().ui.tool, Tool::ColorReplacement);
+    assert_eq!(h.state().ui.tool, Tool::MixerBrush);
     assert_eq!(Tool::from_name("pencil"), Some(Tool::Pencil));
     assert_eq!(Tool::from_name("mixerBrush"), Some(Tool::MixerBrush));
 }

crates/ui-egui/src/prefs/prefs_ui/shortcut_capture_tests.rs

@@ -106,7 +106,7 @@ fn toolbar_shortcut_overrides_remove_old_keys_and_skip_removed_group_members() {
     press(&mut app, Key::B, Modifiers::NONE);
     assert_eq!(app.ui.tool, Tool::Brush, "a removed tool cannot return through group memory");
     press(&mut app, Key::B, Modifiers::SHIFT);
-    assert_eq!(app.ui.tool, Tool::MixerBrush, "Shift cycling skips a removed tool");
+    assert_eq!(app.ui.tool, Tool::ColorReplacement, "Shift cycling skips a removed tool");
 
     app.run("edit.keyboardShortcuts", json!({"reset": true})).unwrap();
     app.ui.tool = Tool::Brush;
@@ -114,7 +114,7 @@ fn toolbar_shortcut_overrides_remove_old_keys_and_skip_removed_group_members() {
     assert_eq!(app.ui.tool, Tool::Pencil, "reset restores the default group");
     app.session.edit_prefs(|p| p.tools.use_shift_key_for_tool_switch = false);
     press(&mut app, Key::B, Modifiers::NONE);
-    assert_eq!(app.ui.tool, Tool::MixerBrush, "plain-key cycling still follows the preference");
+    assert_eq!(app.ui.tool, Tool::ColorReplacement, "plain-key cycling still follows the preference");
 }
 
 #[test]

crates/ui-egui/src/state/tool.rs

@@ -99,8 +99,8 @@ impl Tool {
         Tool::Count,
         Tool::Brush,
         Tool::Pencil,
-        Tool::MixerBrush,
         Tool::ColorReplacement,
+        Tool::MixerBrush,
         Tool::Eraser,
         Tool::BackgroundEraser,
         Tool::MagicEraser,
@@ -113,8 +113,8 @@ impl Tool {
         Tool::Hand,
         Tool::RotateView,
         Tool::Zoom,
-        Tool::Remove,
         Tool::SpotHealing,
+        Tool::Remove,
         Tool::Healing,
         Tool::Patch,
         Tool::ContentAwareMove,

crates/ui-egui/src/toolbar_group_order_tests.rs

@@ -0,0 +1,145 @@
+//! The healing and brush slots list their tools in the reference order, the healing slot starts on
+//! Spot Healing Brush, and J, B, Alt-click and Shift+letter all follow that order.
+
+use egui::{Key, Modifiers};
+use serde_json::json;
+
+use crate::PhotocraftApp;
+use crate::panels::toolbar::{TOOL_SECTIONS, next_slot_tool, toolbar};
+use crate::state::Tool;
+
+const HEALING: [Tool; 6] = [Tool::SpotHealing, Tool::Remove, Tool::Healing, Tool::Patch, Tool::ContentAwareMove, Tool::RedEye];
+const BRUSH: [Tool; 4] = [Tool::Brush, Tool::Pencil, Tool::ColorReplacement, Tool::MixerBrush];
+
+fn app() -> PhotocraftApp {
+    let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), crate::Services::default());
+    app.run("file.new", json!({"width": 20, "height": 10})).unwrap();
+    app.run("layer.new.layer", json!({})).unwrap();
+    app
+}
+
+fn press(app: &mut PhotocraftApp, key: Key, modifiers: Modifiers) {
+    let ctx = egui::Context::default();
+    let raw = egui::RawInput {
+        events: vec![egui::Event::ModifiersChanged(modifiers), egui::Event::Key { key, physical_key: None, pressed: true, repeat: false, modifiers }],
+        ..Default::default()
+    };
+    ctx.begin_pass(raw);
+    crate::shortcuts::handle(app, &ctx);
+    ctx.end_pass().textures_delta.clear();
+}
+
+fn slot_with(tool: Tool) -> &'static [Tool] {
+    TOOL_SECTIONS.iter().flat_map(|s| s.iter()).find(|slot| slot.contains(&tool)).copied().unwrap()
+}
+
+fn shift_sequence(key: Key, steps: usize) -> Vec<Tool> {
+    let mut app = app();
+    app.ui.tool = Tool::Hand;
+    (0..steps)
+        .map(|_| {
+            press(&mut app, key, Modifiers::SHIFT);
+            app.ui.tool
+        })
+        .collect()
+}
+
+#[test]
+fn the_healing_slot_lists_spot_healing_first() {
+    assert_eq!(slot_with(Tool::Healing), &HEALING);
+}
+
+#[test]
+fn the_brush_slot_lists_color_replacement_before_mixer_brush() {
+    assert_eq!(slot_with(Tool::Brush), &BRUSH);
+}
+
+#[test]
+fn a_fresh_healing_slot_shows_spot_healing() {
+    assert_eq!(slot_with(Tool::Remove)[0], Tool::SpotHealing);
+}
+
+#[test]
+fn j_on_fresh_settings_selects_spot_healing() {
+    let mut app = app();
+    app.ui.tool = Tool::Hand;
+    press(&mut app, Key::J, Modifiers::NONE);
+    assert_eq!(app.ui.tool, Tool::SpotHealing);
+}
+
+#[test]
+fn tool_all_orders_the_two_pairs_like_the_slots() {
+    let pos = |t: Tool| Tool::ALL.iter().position(|x| *x == t).unwrap();
+    assert!(pos(Tool::SpotHealing) < pos(Tool::Remove));
+    assert!(pos(Tool::ColorReplacement) < pos(Tool::MixerBrush));
+}
+
+#[test]
+fn alt_click_steps_both_slots_in_the_new_order_and_wraps() {
+    for order in [&HEALING[..], &BRUSH[..]] {
+        for (i, from) in order.iter().enumerate() {
+            assert_eq!(next_slot_tool(slot_with(*from), *from), order[(i + 1) % order.len()], "from {from:?}");
+        }
+    }
+}
+
+#[test]
+fn shift_j_follows_the_reference_sequence() {
+    let want: Vec<Tool> = HEALING.iter().chain(&HEALING[..1]).copied().collect();
+    assert_eq!(shift_sequence(Key::J, 7), want);
+}
+
+#[test]
+fn shift_b_follows_the_reference_sequence() {
+    let want: Vec<Tool> = BRUSH.iter().chain(&BRUSH[..1]).copied().collect();
+    assert_eq!(shift_sequence(Key::B, 5), want);
+}
+
+fn frame(app: &mut PhotocraftApp, ctx: &egui::Context, time: f64, events: Vec<egui::Event>) {
+    let mut out = ctx.run_ui(
+        egui::RawInput {
+            time: Some(time),
+            events,
+            screen_rect: Some(egui::Rect::from_min_size(egui::Pos2::ZERO, egui::vec2(1200.0, 1800.0))),
+            ..Default::default()
+        },
+        |ui| toolbar(app, ui),
+    );
+    out.textures_delta.clear();
+}
+
+/// Alt-click on the slot button, through the pointer, steps the reordered healing and brush slots.
+#[test]
+fn pointer_alt_click_steps_the_healing_and_brush_slots_in_the_new_order() {
+    let mut app = app();
+    let ctx = egui::Context::default();
+    PhotocraftApp::setup_context(&ctx, crate::theme::ThemeKind::ALL[0]);
+    frame(&mut app, &ctx, 0.0, vec![]);
+    frame(&mut app, &ctx, 0.1, vec![]);
+    let bx = if crate::theme::Tokens::get(&ctx).pro { 30.0 } else { 36.0 };
+    let mut buttons: Vec<egui::Rect> = ctx.viewport(|v| {
+        v.prev_pass
+            .widgets
+            .layers()
+            .flat_map(|(_, w)| w.iter())
+            .filter(|w| w.rect.size() == egui::Vec2::splat(bx) && w.sense.senses_click())
+            .map(|w| w.rect)
+            .collect()
+    });
+    buttons.sort_by(|a, b| a.top().total_cmp(&b.top()));
+    let alt = Modifiers { alt: true, ..Default::default() };
+    let pointer = |pos, pressed, modifiers| egui::Event::PointerButton { pos, button: egui::PointerButton::Primary, pressed, modifiers };
+    let mut t = 1.0;
+    for order in [&HEALING[..], &BRUSH[..]] {
+        let index = TOOL_SECTIONS.iter().flat_map(|s| s.iter()).position(|slot| slot.contains(&order[0])).unwrap();
+        let at = buttons[index].center();
+        app.ui.tool = order[0];
+        for want in order.iter().cycle().skip(1).take(order.len()) {
+            frame(&mut app, &ctx, t, vec![egui::Event::ModifiersChanged(alt), egui::Event::PointerMoved(at), pointer(at, true, alt)]);
+            frame(&mut app, &ctx, t + 0.05, vec![egui::Event::ModifiersChanged(Default::default()), pointer(at, false, alt)]);
+            frame(&mut app, &ctx, t + 0.1, vec![]);
+            assert_eq!(app.ui.tool, *want, "alt-click at {t}");
+            t += 0.5;
+        }
+    }
+}

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

@@ -762,16 +762,16 @@ fn create_a_new_layer_sends_new_layer_to_the_command() {
 
 #[test]
 fn red_eye_is_in_the_j_flyout() {
-    let j = [Tool::Remove, Tool::SpotHealing, Tool::Healing, Tool::Patch, Tool::ContentAwareMove, Tool::RedEye];
+    let j = [Tool::SpotHealing, Tool::Remove, Tool::Healing, Tool::Patch, Tool::ContentAwareMove, Tool::RedEye];
     assert!(j.contains(&Tool::RedEye));
     assert!(j.iter().all(|t| t.key() == 'J'));
 }
 
 #[test]
-fn j_cycles_from_remove() {
+fn j_cycles_from_spot_healing() {
     // J from another group picks the first J tool, and repeated J goes on in this order.
     let group: Vec<Tool> = Tool::ALL.iter().copied().filter(|t| t.key() == 'J').collect();
-    assert_eq!(group, [Tool::Remove, Tool::SpotHealing, Tool::Healing, Tool::Patch, Tool::ContentAwareMove, Tool::RedEye]);
+    assert_eq!(group, [Tool::SpotHealing, Tool::Remove, Tool::Healing, Tool::Patch, Tool::ContentAwareMove, Tool::RedEye]);
 }
 
 #[test]