From 87062ef0c000c23c61ec55552cafd8fec5532328 Mon Sep 17 00:00:00 2001 From: Trevor Brown Date: Thu, 5 Nov 2026 14:12:50 -0500 Subject: [PATCH 1/2] The healing slot starts on Spot Healing Brush and the brush slot lists Color Replacement before Mixer Brush (#11375) TOOL_SECTIONS and Tool::ALL now order the healing slot as Spot Healing, Remove, Healing, Patch, Content-Aware Move, Red Eye and the brush slot as Brush, Pencil, Color Replacement, Mixer Brush. A fresh healing slot shows Spot Healing Brush and J selects it. Existing tests that expected the old orders (B and J cycles, the brush flyout click, the shortcut override test) are updated. The engine's shortcut table keeps the same order as Tool::ALL, so its two pairs are swapped to match. Co-Authored-By: Claude Fable 5.1 --- crates/engine/src/shell_commands.rs | 4 +- crates/ui-egui/src/lib.rs | 2 + crates/ui-egui/src/panels.rs | 2 +- crates/ui-egui/src/panels/toolbar.rs | 8 +- .../ui-egui/src/panels/type_flyout_tests.rs | 12 +-- crates/ui-egui/src/pencil_tests.rs | 12 +-- .../prefs/prefs_ui/shortcut_capture_tests.rs | 4 +- crates/ui-egui/src/state/tool.rs | 4 +- .../ui-egui/src/toolbar_group_order_tests.rs | 96 +++++++++++++++++++ crates/ui-egui/src/tools/retouch_ui/tests.rs | 6 +- 10 files changed, 124 insertions(+), 26 deletions(-) create mode 100644 crates/ui-egui/src/toolbar_group_order_tests.rs diff --git a/crates/engine/src/shell_commands.rs b/crates/engine/src/shell_commands.rs index 3b52c03a03..6e8eeac1d7 100644 --- a/crates/engine/src/shell_commands.rs +++ b/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")), diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 54edda6264..602bc3cd75 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -792,6 +792,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; diff --git a/crates/ui-egui/src/panels.rs b/crates/ui-egui/src/panels.rs index ab5475ff9b..c872d7f69d 100644 --- a/crates/ui-egui/src/panels.rs +++ b/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; diff --git a/crates/ui-egui/src/panels/toolbar.rs b/crates/ui-egui/src/panels/toolbar.rs index c8f62a602e..933b67c04b 100644 --- a/crates/ui-egui/src/panels/toolbar.rs +++ b/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) } diff --git a/crates/ui-egui/src/panels/type_flyout_tests.rs b/crates/ui-egui/src/panels/type_flyout_tests.rs index 5144379a3e..9e13f66169 100644 --- a/crates/ui-egui/src/panels/type_flyout_tests.rs +++ b/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 = 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); diff --git a/crates/ui-egui/src/pencil_tests.rs b/crates/ui-egui/src/pencil_tests.rs index aea5743215..6cc618e8fa 100644 --- a/crates/ui-egui/src/pencil_tests.rs +++ b/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)); } diff --git a/crates/ui-egui/src/prefs/prefs_ui/shortcut_capture_tests.rs b/crates/ui-egui/src/prefs/prefs_ui/shortcut_capture_tests.rs index f59bf278e8..20a2695a97 100644 --- a/crates/ui-egui/src/prefs/prefs_ui/shortcut_capture_tests.rs +++ b/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] diff --git a/crates/ui-egui/src/state/tool.rs b/crates/ui-egui/src/state/tool.rs index cd96f525a1..5adedaf3c8 100644 --- a/crates/ui-egui/src/state/tool.rs +++ b/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, diff --git a/crates/ui-egui/src/toolbar_group_order_tests.rs b/crates/ui-egui/src/toolbar_group_order_tests.rs new file mode 100644 index 0000000000..caafa1247c --- /dev/null +++ b/crates/ui-egui/src/toolbar_group_order_tests.rs @@ -0,0 +1,96 @@ +//! 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}; +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 { + 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 = 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 = BRUSH.iter().chain(&BRUSH[..1]).copied().collect(); + assert_eq!(shift_sequence(Key::B, 5), want); +} diff --git a/crates/ui-egui/src/tools/retouch_ui/tests.rs b/crates/ui-egui/src/tools/retouch_ui/tests.rs index f2293d6bc7..4a084bc80a 100644 --- a/crates/ui-egui/src/tools/retouch_ui/tests.rs +++ b/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::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] From d9e983549e932ffaa693571d1b8fec92660c7a96 Mon Sep 17 00:00:00 2001 From: Trevor Brown Date: Thu, 5 Nov 2026 15:11:04 -0500 Subject: [PATCH 2/2] Alt-click on the reordered healing and brush slots is covered through the pointer (#11375) The new test pointer_alt_click_steps_the_healing_and_brush_slots_in_the_new_order drives Alt-click on each slot button, following alt_click_cycles_the_tool_flyout_slot, and checks the full wrapped sequence. Co-Authored-By: Claude Fable 5.1 --- .../ui-egui/src/toolbar_group_order_tests.rs | 51 ++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) diff --git a/crates/ui-egui/src/toolbar_group_order_tests.rs b/crates/ui-egui/src/toolbar_group_order_tests.rs index caafa1247c..46db63bcb3 100644 --- a/crates/ui-egui/src/toolbar_group_order_tests.rs +++ b/crates/ui-egui/src/toolbar_group_order_tests.rs @@ -5,7 +5,7 @@ use egui::{Key, Modifiers}; use serde_json::json; use crate::PhotocraftApp; -use crate::panels::toolbar::{TOOL_SECTIONS, next_slot_tool}; +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]; @@ -94,3 +94,52 @@ fn shift_b_follows_the_reference_sequence() { let want: Vec = 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) { + 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 = 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; + } + } +}