Merge pull request #11852 from yoriswind/codex/toolbar-first-w-11757

Use toolbar order for fresh tool shortcuts

crates/ui-egui/src/input/shortcuts.rs

@@ -907,8 +907,9 @@ fn handle_escape_and_view_keys(app: &mut PhotocraftApp, ctx: &egui::Context) {
     crate::opacity_keys::handle(app, ctx);
 }
 
-/// Selects a tool from its letter key. Pressing the key of the current group cycles within it; with
-/// the Shift-switch preference only ⇧+key cycles. Returns true when a tool key was handled.
+/// Selects a tool from its letter key. Entering a group uses its remembered eligible tool or the
+/// first eligible tool in toolbar order. Pressing the key of the current group cycles within it.
+/// With the Shift-switch preference, only ⇧+key cycles. Returns true when a tool key was handled.
 fn select_tool_by_key(app: &mut PhotocraftApp, ctx: &egui::Context, pressed: &impl Fn(Key) -> bool) -> bool {
     let shift_switch = app.session.prefs().tools.use_shift_key_for_tool_switch;
     for t in Tool::ALL {
@@ -920,15 +921,20 @@ fn select_tool_by_key(app: &mut PhotocraftApp, ctx: &egui::Context, pressed: &im
         if cycle_shift || pressed(k) {
             let before = app.ui.tool;
             let mut group: Vec<Tool> = Tool::ALL.iter().copied().filter(|x| x.key() == t.key() && crate::tool_shortcuts::uses_default(app, *x)).collect();
-            // Shift+key (the cycle and the fallback to the group's first tool) uses the toolbar
-            // flyout's order; plain keys keep the `Tool::ALL` order.
+            // Shift+key cycles in toolbar flyout order.
             if cycle_shift {
                 group.sort_by_key(|x| crate::panels::toolbar::toolbar_position(*x));
             }
             app.ui.tool = match group.iter().position(|x| *x == app.ui.tool) {
                 Some(i) if shift_switch && !cycle_shift => group[i],
                 Some(i) => group[(i + 1) % group.len()],
-                None => app.ui.group_tools.iter().copied().find(|last| group.contains(last)).unwrap_or(group[0]),
+                None => app
+                    .ui
+                    .group_tools
+                    .iter()
+                    .copied()
+                    .find(|last| group.contains(last))
+                    .unwrap_or_else(|| group.iter().copied().min_by_key(|tool| crate::panels::toolbar::toolbar_position(*tool)).unwrap_or(t)),
             };
             crate::tool_memory::ended_by_tool(app);
             // `consume_key` ignores extra ⇧ and ⌥: a ⇧/⌥ switch is never spring-loaded.
@@ -946,3 +952,7 @@ fn select_tool_by_key(app: &mut PhotocraftApp, ctx: &egui::Context, pressed: &im
 
 #[cfg(test)]
 mod tests;
+
+#[cfg(test)]
+#[path = "tool_key_default_tests.rs"]
+mod tool_key_default_tests;

crates/ui-egui/src/input/tool_key_default_tests.rs

@@ -0,0 +1,89 @@
+use super::{Key, Modifiers, PhotocraftApp, Tool, handle};
+use crate::panels::toolbar::TOOL_SECTIONS;
+use crate::spring_tools::key_of;
+use std::collections::HashSet;
+
+fn app() -> PhotocraftApp {
+    PhotocraftApp::new(photocraft_engine::Session::new(), Default::default())
+}
+
+fn press(app: &mut PhotocraftApp, ctx: &egui::Context, key: Key, modifiers: Modifiers) {
+    let raw = egui::RawInput {
+        events: vec![
+            egui::Event::ModifiersChanged(modifiers),
+            egui::Event::Key { key, physical_key: None, pressed: true, repeat: false, modifiers },
+            egui::Event::Key { key, physical_key: None, pressed: false, repeat: false, modifiers },
+        ],
+        ..Default::default()
+    };
+    ctx.begin_pass(raw);
+    handle(app, ctx);
+    ctx.end_pass().textures_delta.clear();
+}
+
+/// A fresh app enters each default letter group at its first toolbar tool under either preference.
+#[test]
+fn fresh_tool_letters_pick_the_first_tool_in_toolbar_order() {
+    for shift_switch in [true, false] {
+        let mut checked = HashSet::new();
+        for tool in TOOL_SECTIONS.iter().flat_map(|section| section.iter()).flat_map(|slot| slot.iter()).copied() {
+            let Some(key) = key_of(tool.key()) else { continue };
+            if !checked.insert(key) {
+                continue;
+            }
+
+            let mut app = app();
+            app.session.edit_prefs(|prefs| prefs.tools.use_shift_key_for_tool_switch = shift_switch);
+            if app.ui.tool.key() == tool.key() {
+                app.ui.tool = Tool::Brush;
+            }
+            press(&mut app, &egui::Context::default(), key, Modifiers::NONE);
+            assert_eq!(app.ui.tool, tool, "{key:?}, Shift switch: {shift_switch}");
+        }
+        assert_eq!(checked, Tool::ALL.into_iter().filter_map(|tool| key_of(tool.key())).collect::<HashSet<_>>());
+    }
+}
+
+/// A remembered Magic Wand takes precedence over the fresh W default, even when hidden.
+#[test]
+fn plain_w_returns_to_the_remembered_magic_wand() {
+    for hidden in [false, true] {
+        let mut app = app();
+        let ctx = egui::Context::default();
+        if hidden {
+            app.session.edit_prefs(|prefs| prefs.toolbar.hidden = vec!["MagicWand".to_owned()]);
+        }
+        app.ui.tool = Tool::MagicWand;
+        press(&mut app, &ctx, Key::B, Modifiers::NONE);
+        assert_eq!(app.ui.tool, Tool::Brush);
+        press(&mut app, &ctx, Key::W, Modifiers::NONE);
+        assert_eq!(app.ui.tool, Tool::MagicWand, "hidden: {hidden}");
+    }
+}
+
+/// Shift+W follows the flyout order and wraps. With the default preference, plain W keeps
+/// the current tool when it already belongs to the group.
+#[test]
+fn shift_w_cycles_in_toolbar_order() {
+    let mut app = app();
+    let ctx = egui::Context::default();
+    app.ui.tool = Tool::ObjectSelection;
+
+    for expected in [Tool::QuickSelection, Tool::MagicWand, Tool::ObjectSelection] {
+        press(&mut app, &ctx, Key::W, Modifiers::SHIFT);
+        assert_eq!(app.ui.tool, expected);
+    }
+    press(&mut app, &ctx, Key::W, Modifiers::NONE);
+    assert_eq!(app.ui.tool, Tool::ObjectSelection);
+}
+
+#[test]
+fn remapped_first_tool_is_skipped_by_its_default_letter() {
+    let mut app = app();
+    let ctx = egui::Context::default();
+    app.run("prefs.set", serde_json::json!({"path": "shortcuts.tools.select.objectSelection", "value": "F6"})).unwrap();
+    press(&mut app, &ctx, Key::W, Modifiers::NONE);
+    assert_eq!(app.ui.tool, Tool::QuickSelection);
+    press(&mut app, &ctx, Key::F6, Modifiers::NONE);
+    assert_eq!(app.ui.tool, Tool::ObjectSelection);
+}

crates/ui-egui/src/tool_shift_cycle_order_tests.rs

@@ -160,5 +160,5 @@ fn a_plain_w_from_outside_the_group_keeps_selecting_what_it_selected_before() {
     ctx.begin_pass(raw);
     crate::shortcuts::handle(&mut app, &ctx);
     ctx.end_pass().textures_delta.clear();
-    assert_eq!(app.ui.tool, Tool::MagicWand, "the first W tool in Tool::ALL, not the first in the flyout");
+    assert_eq!(app.ui.tool, Tool::ObjectSelection, "fresh W selects the first tool in the flyout");
 }