Merge PR #2621

# Conflicts:
#	docs/whats-new.md

apps/lightcraft/src/native_menu.rs

@@ -39,18 +39,138 @@ enum Handle {
 
 struct Item {
     handle: Handle,
+    /// The submenu the item sits in, to swap the item when its accelerator has to go (macOS).
+    parent: Submenu,
     cmd: String,
     params: Value,
     shortcut: Option<String>,
+    /// The accelerator the item's shortcut maps to.
     accel: Option<Accelerator>,
+    /// The accelerator the native item has now: `accel`, or `None` while it yields.
+    current: Option<Accelerator>,
     label: String,
     enabled: bool,
     checked: Option<bool>,
+    /// Whether the item may be replaced to drop its key. Not in the Help menu: a swap there left
+    /// the old row and added a second one (muda 0.21, macOS 27), so those keys stay.
+    swappable: bool,
+}
+
+impl Item {
+    fn native_label(&self) -> String {
+        lightcraft_ui_egui::menubar::display_item_label(&self.cmd, &self.params, &self.label).replace('&', "&&")
+    }
+
+    /// Puts `want` on the native item. muda's `set_accelerator(None)` does nothing on macOS (the
+    /// old key equivalent stays and the menu still takes the key), so there the item is replaced
+    /// by an identical one without an accelerator, at the same position and with the same id,
+    /// text, enabled and checked state. Only done when the accelerator changes (and again while a
+    /// native call keeps failing), never per frame otherwise.
+    fn set_accelerator(&mut self, want: Option<Accelerator>) {
+        if self.current == want {
+            return;
+        }
+        if cfg!(target_os = "macos") && want.is_none() && self.swappable {
+            // `current` follows the native item: it changes only when the swap went through, so a
+            // failed swap (the old item, key and all, is still there) is tried again
+            let done = self.replace_without_accelerator();
+            self.current = settled(self.current, want, done);
+            return;
+        }
+        let done = match &self.handle {
+            Handle::Plain(h) => h.set_accelerator(want),
+            Handle::Check(h) => h.set_accelerator(want),
+        }
+        .is_ok();
+        self.current = settled(self.current, want, done);
+    }
+
+    /// Replaces the native item by one without an accelerator. `self.handle` changes only when the
+    /// new item is attached; on failure the old item is still the one in the submenu.
+    fn replace_without_accelerator(&mut self) -> bool {
+        let (id, old): (_, &dyn muda::IsMenuItem) = match &self.handle {
+            Handle::Plain(h) => (h.id().clone(), h),
+            Handle::Check(h) => (h.id().clone(), h),
+        };
+        let Some(position) = self.parent.items().iter().position(|k| *k.id() == id) else { return false };
+        let new = match &self.handle {
+            Handle::Plain(_) => Handle::Plain(MenuItem::with_id(id, self.native_label(), self.enabled, None)),
+            Handle::Check(h) => Handle::Check(CheckMenuItem::with_id(id, self.native_label(), self.enabled, h.is_checked(), None)),
+        };
+        let new_ref: &dyn muda::IsMenuItem = match &new {
+            Handle::Plain(h) => h,
+            Handle::Check(h) => h,
+        };
+        if !swap_item(&self.parent, old, new_ref, position) {
+            return false;
+        }
+        self.handle = new;
+        true
+    }
+}
+
+/// The accelerator the native item has after trying to put `want` on it: `want` when the native
+/// call worked, else what it had, so the caller sees the mismatch and tries again.
+fn settled(had: Option<Accelerator>, want: Option<Accelerator>, native_call_worked: bool) -> Option<Accelerator> {
+    if native_call_worked { want } else { had }
+}
+
+/// Whether the per-frame accelerator sync has to run: the focus or recording state changed, or an
+/// earlier sync left an item whose native accelerator is not the wanted one (a failed swap).
+fn sync_needed(focus: bool, capturing: bool, synced_focus: bool, synced_capturing: bool, pending: bool) -> bool {
+    focus != synced_focus || capturing != synced_capturing || pending
+}
+
+/// The three submenu calls a swap needs, so the failure paths can be tested without a main-thread
+/// `muda::Menu`.
+trait SwapTarget {
+    type Row: ?Sized;
+    fn remove(&self, row: &Self::Row) -> bool;
+    fn insert(&self, row: &Self::Row, position: usize) -> bool;
+    fn append(&self, row: &Self::Row) -> bool;
+}
+
+impl SwapTarget for Submenu {
+    type Row = dyn muda::IsMenuItem;
+    fn remove(&self, row: &Self::Row) -> bool {
+        Submenu::remove(self, row).is_ok()
+    }
+    fn insert(&self, row: &Self::Row, position: usize) -> bool {
+        Submenu::insert(self, row, position).is_ok()
+    }
+    fn append(&self, row: &Self::Row) -> bool {
+        Submenu::append(self, row).is_ok()
+    }
+}
+
+/// Puts `new` where `old` is. Returns whether `new` is attached; when it is not, `old` is back in
+/// the menu (at its position, else at the end), so the caller keeps `old` as its handle.
+fn swap_item<T: SwapTarget + ?Sized>(target: &T, old: &T::Row, new: &T::Row, position: usize) -> bool {
+    if !target.remove(old) {
+        return false;
+    }
+    // (an item at the end beats a missing one)
+    if target.insert(new, position) || target.append(new) {
+        return true;
+    }
+    let _ = target.insert(old, position) || target.append(old);
+    false
+}
+
+/// The accelerator a native item has: none while a shortcut is being recorded, and none for the
+/// keys a focused text field needs.
+fn wanted_accelerator(accel: Option<Accelerator>, sc: &str, text_focus: bool, capturing: bool) -> Option<Accelerator> {
+    if capturing || (text_focus && yields_to_text(sc)) { None } else { accel }
 }
 
 pub struct NativeMenu {
     menu: Menu,
     items: HashMap<String, Item>,
+    /// Settings… and Quit in the application submenu: ordinary items, so they give up ⌘, and ⌘Q
+    /// to the shortcut recorder like the rest. (The system's own items there and in File/Window,
+    /// such as Hide ⌘H, Close Window ⌘W and Minimize ⌘M, are muda predefined items whose key
+    /// cannot be changed or swapped.)
+    app_items: Vec<Item>,
     rx: Receiver<String>,
     /// Structure of the installed menu (ids, kinds, submenu names); a change rebuilds it.
     structure: String,
@@ -58,6 +178,8 @@ pub struct NativeMenu {
     /// The keyboard shortcuts editor is recording a key: every accelerator is off so the key
     /// press reaches it.
     capturing: bool,
+    /// An item's native accelerator is not the wanted one (its swap or accelerator call failed): sync again next frame.
+    accelerators_pending: bool,
 }
 
 /// `Cmd+Shift+Z` → a muda accelerator (`None` for keys we leave to egui, e.g. Escape).
@@ -216,7 +338,16 @@ impl NativeMenu {
             let _ = tx.send(e.id.0);
             repaint.request_repaint();
         }));
-        let mut m = NativeMenu { menu: Menu::new(), items: HashMap::new(), rx, structure: String::new(), text_focus: false, capturing: false };
+        let mut m = NativeMenu {
+            menu: Menu::new(),
+            items: HashMap::new(),
+            app_items: Vec::new(),
+            rx,
+            structure: String::new(),
+            text_focus: false,
+            capturing: false,
+            accelerators_pending: false,
+        };
         m.rebuild(app);
         app.native_menu = true;
         m
@@ -227,6 +358,7 @@ impl NativeMenu {
         self.structure = structure_of(&bar);
         self.menu = Menu::new();
         self.items.clear();
+        self.app_items.clear();
 
         // the application menu
         let app_menu = Submenu::new("LightCraft", true);
@@ -246,6 +378,22 @@ impl NativeMenu {
             &PredefinedMenuItem::separator(),
             &quit,
         ]);
+        for (handle, cmd, shortcut, text) in [(&settings, SETTINGS, SETTINGS_KEY, "Settings…"), (&quit, QUIT, "Cmd+Q", "Quit LightCraft")] {
+            let accel = accelerator(shortcut);
+            self.app_items.push(Item {
+                handle: Handle::Plain(handle.clone()),
+                parent: app_menu.clone(),
+                swappable: true,
+                cmd: cmd.to_string(),
+                params: Value::Null,
+                shortcut: Some(shortcut.to_string()),
+                accel,
+                current: accel,
+                label: text.to_string(),
+                enabled: true,
+                checked: None,
+            });
+        }
         let _ = self.menu.append(&app_menu);
 
         for (title, nodes) in &bar {
@@ -264,7 +412,9 @@ impl NativeMenu {
                 .cloned()
                 .collect();
             let nodes = tidy_separators(nodes);
-            self.append_nodes(&sub, &nodes);
+            // Swapping an item in the Help menu leaves the old row in place and adds a second one
+            // (measured on macOS 27 with muda 0.21): the Help menu is not addressed row for row.
+            self.append_nodes(&sub, &nodes, title != "Help");
             if title == "File" {
                 // ⌘W, the system's own item
                 let _ = sub.append_items(&[
@@ -287,10 +437,12 @@ impl NativeMenu {
         self.menu.init_for_nsapp();
         self.text_focus = false;
         self.capturing = false;
+        self.accelerators_pending = false;
         self.publish_shortcuts(app);
     }
 
-    fn append_nodes(&mut self, sub: &Submenu, nodes: &[MenuNode]) {
+    /// `swappable`: whether its items may be replaced to drop their key (see `Item::swappable`).
+    fn append_nodes(&mut self, sub: &Submenu, nodes: &[MenuNode], swappable: bool) {
         for n in nodes {
             match n {
                 MenuNode::Separator => {
@@ -298,7 +450,7 @@ impl NativeMenu {
                 }
                 MenuNode::Submenu { label, children } => {
                     let s = Submenu::new(label_text(label), true);
-                    self.append_nodes(&s, children);
+                    self.append_nodes(&s, children, swappable);
                     let _ = sub.append(&s);
                 }
                 MenuNode::Item { id, params, label, shortcut, enabled, checked } => {
@@ -334,10 +486,13 @@ impl NativeMenu {
                         key,
                         Item {
                             handle,
+                            parent: sub.clone(),
+                            swappable,
                             cmd: id.clone(),
                             params: params.clone(),
                             shortcut: shortcut.clone(),
                             accel,
+                            current: accel,
                             label: label.clone(),
                             enabled: *enabled,
                             checked: *checked,
@@ -379,8 +534,9 @@ impl NativeMenu {
         }
         let bar = menu_bar(app);
         if structure_of(&bar) != self.structure {
+            // (no early return: a rebuild installs every accelerator and resets the state they were synced
+            // to, so the sync below runs in this same frame and no key reaches the menu in between)
             self.rebuild(app);
-            return quit;
         }
         fn walk(nodes: &[MenuNode], items: &mut HashMap<String, Item>) {
             for n in nodes {
@@ -418,20 +574,20 @@ impl NativeMenu {
 
         let focus = ctx.egui_wants_keyboard_input();
         let capturing = app.recording_shortcut.is_some();
-        if focus != self.text_focus || capturing != self.capturing {
+        if sync_needed(focus, capturing, self.text_focus, self.capturing, self.accelerators_pending) {
             self.text_focus = focus;
             self.capturing = capturing;
-            for it in self.items.values() {
+            let mut pending = false;
+            for it in self.items.values_mut().chain(self.app_items.iter_mut()) {
                 let Some(sc) = it.shortcut.as_deref() else { continue };
                 if it.accel.is_none() {
                     continue;
                 }
-                let accel = if capturing || (focus && yields_to_text(sc)) { None } else { it.accel };
-                let _ = match &it.handle {
-                    Handle::Plain(h) => h.set_accelerator(accel),
-                    Handle::Check(h) => h.set_accelerator(accel),
-                };
+                let want = wanted_accelerator(it.accel, sc, focus, capturing);
+                it.set_accelerator(want);
+                pending |= it.current != want;
             }
+            self.accelerators_pending = pending;
             self.publish_shortcuts(app);
         }
         quit
@@ -459,6 +615,97 @@ mod tests {
         set_language(Locale::En);
     }
 
+    #[test]
+    fn accelerators_yield_to_text_focus_and_shortcut_recording() {
+        let g = accelerator("G");
+        let cmd_s = accelerator("Cmd+S");
+        assert_eq!(wanted_accelerator(g, "G", false, false), g);
+        assert_eq!(wanted_accelerator(g, "G", true, false), None, "typing a G in a field");
+        assert_eq!(wanted_accelerator(cmd_s, "Cmd+S", true, false), cmd_s, "⌘S still saves while typing");
+        assert_eq!(wanted_accelerator(cmd_s, "Cmd+S", false, true), None, "the editor records the key");
+        assert_eq!(wanted_accelerator(cmd_s, "Cmd+S", true, true), None, "recording wins over typing");
+        // the application menu's Settings… and Quit are tracked like the other items
+        for sc in [SETTINGS_KEY, "Cmd+Q"] {
+            let a = accelerator(sc);
+            assert!(a.is_some());
+            assert_eq!(wanted_accelerator(a, sc, true, false), a, "{sc} stays while typing");
+            assert_eq!(wanted_accelerator(a, sc, false, true), None, "{sc} goes while recording");
+        }
+        assert_eq!(wanted_accelerator(None, "Escape", false, false), None, "no accelerator stays none");
+    }
+
+    /// A submenu of numbers whose calls can be made to fail, to run `swap_item`'s failure paths.
+    /// Row 2 is the old item and row 9 its replacement.
+    #[derive(Default)]
+    struct Fake {
+        rows: std::cell::RefCell<Vec<u32>>,
+        fail_remove: bool,
+        /// The new item (9) cannot be inserted / appended.
+        reject_new_insert: bool,
+        reject_new_append: bool,
+        /// The old item (2) cannot be inserted back.
+        reject_old_insert: bool,
+    }
+    impl SwapTarget for Fake {
+        type Row = u32;
+        fn remove(&self, row: &u32) -> bool {
+            if self.fail_remove {
+                return false;
+            }
+            self.rows.borrow_mut().retain(|x| x != row);
+            true
+        }
+        fn insert(&self, row: &u32, position: usize) -> bool {
+            if (self.reject_new_insert && *row == 9) || (self.reject_old_insert && *row == 2) {
+                return false;
+            }
+            self.rows.borrow_mut().insert(position, *row);
+            true
+        }
+        fn append(&self, row: &u32) -> bool {
+            if self.reject_new_append && *row == 9 {
+                return false;
+            }
+            self.rows.borrow_mut().push(*row);
+            true
+        }
+    }
+
+    fn swap(fake: Fake) -> (bool, Vec<u32>) {
+        *fake.rows.borrow_mut() = vec![1, 2, 3];
+        let done = swap_item(&fake, &2, &9, 1);
+        (done, fake.rows.into_inner())
+    }
+
+    #[test]
+    fn a_swap_changes_the_item_only_when_the_new_one_is_attached() {
+        assert_eq!(swap(Fake::default()), (true, vec![1, 9, 3]), "in place");
+        assert_eq!(swap(Fake { fail_remove: true, ..Default::default() }), (false, vec![1, 2, 3]), "remove fails: the old item stays");
+        assert_eq!(swap(Fake { reject_new_insert: true, ..Default::default() }), (true, vec![1, 3, 9]), "insert fails: the new item goes last");
+        assert_eq!(
+            swap(Fake { reject_new_insert: true, reject_new_append: true, ..Default::default() }),
+            (false, vec![1, 2, 3]),
+            "neither works: the old item is back where it was"
+        );
+        assert_eq!(
+            swap(Fake { reject_new_insert: true, reject_new_append: true, reject_old_insert: true, ..Default::default() }),
+            (false, vec![1, 3, 2]),
+            "the old item cannot go back in place either: it is appended"
+        );
+    }
+
+    #[test]
+    fn a_failed_native_call_is_retried_while_the_state_holds() {
+        let g = accelerator("G");
+        assert_eq!(settled(g, None, true), None, "the swap worked");
+        assert_eq!(settled(g, None, false), g, "it failed: the native item still has its key");
+        assert_eq!(settled(None, g, false), None, "restoring a key failed");
+        assert!(!sync_needed(true, false, true, false, false), "nothing changed");
+        assert!(sync_needed(true, false, false, false, false), "focus changed");
+        assert!(sync_needed(false, true, false, false, false), "recording changed");
+        assert!(sync_needed(true, false, true, false, true), "a swap failed last time: try again with the state unchanged");
+    }
+
     #[test]
     fn shortcuts_map_to_accelerators() {
         assert_eq!(accelerator("Cmd+Shift+Z"), Some(Accelerator::new(Modifiers::META | Modifiers::SHIFT, Code::KeyZ)));

docs/parity-checklist.md

@@ -1,6 +1,6 @@
 # Lightroom parity checklist
 
-> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-21 · **Change:** minor (LR-MERGE-HDR / LR-MERGE-PANO: Lanczos-3 warps; LR-IMP-LOCAL: browsing from the sidebar records the folder; LR-VIEW-ZOOM: only a full-size drag within 20 ms clears a slow-drag verdict; LR-IMP-ADD-DIALOG: hierarchical import album destination reconciled with main, #2362; KEY-SLIDER-NUDGE: vertical wheel nudges need a click or drag in the row; leaving it disarms them; LR-EDIT-OPTICS-PROFILE: Fujifilm nine-knot lens tables read at the plain radius; LR-VIEW-ZOOM: only a full-size drag within 20 ms clears a slow-drag verdict; LRC-LIB-LIBFOLDERS, LR-BEHAV-BG-TASKS: Synchronize Folder's scan shows its folder listing and stops it on ✕; LRC-LIB-LIBFOLDERS: Synchronize Folder says a folder is up to date; LR-LIB-STACK: filmstrip stack badge click; LRC-LIB-REFVIEW / CATALOG-SWITCH: Escape preserves edits; catalog-local pins; deleted and malformed references refused; LRC-LIB-COMPARE: the Info panel labels the active photo while culling, #2398; KEYC-RATING: `\` shows / hides the Filter Bar in the grids; LR-VIEW-PHOTOGRID: the grid reads only the photos an import batch or an edit touched, not the whole list; earlier: LRC-PRINT-PAGE: Print Page Options; LR-META-XMP: fewer false unimported-XMP warnings) · **Target:** Adobe Lightroom Classic 15.5 / Lightroom 9.6
+> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-22 · **Change:** minor (MENU-BAR: on macOS a menu item that yields its key to a focused text field or to shortcut recording really lets it go; LR-MERGE-HDR / LR-MERGE-PANO: Lanczos-3 warps; LR-IMP-LOCAL: browsing from the sidebar records the folder; LR-VIEW-ZOOM: only a full-size drag within 20 ms clears a slow-drag verdict; LR-IMP-ADD-DIALOG: hierarchical import album destination reconciled with main, #2362; KEY-SLIDER-NUDGE: vertical wheel nudges need a click or drag in the row; leaving it disarms them; LR-EDIT-OPTICS-PROFILE: Fujifilm nine-knot lens tables read at the plain radius; LR-VIEW-ZOOM: only a full-size drag within 20 ms clears a slow-drag verdict; LRC-LIB-LIBFOLDERS, LR-BEHAV-BG-TASKS: Synchronize Folder's scan shows its folder listing and stops it on ✕; LRC-LIB-LIBFOLDERS: Synchronize Folder says a folder is up to date; LR-LIB-STACK: filmstrip stack badge click; LRC-LIB-REFVIEW / CATALOG-SWITCH: Escape preserves edits; catalog-local pins; deleted and malformed references refused; LRC-LIB-COMPARE: the Info panel labels the active photo while culling, #2398; KEYC-RATING: `\` shows / hides the Filter Bar in the grids; LR-VIEW-PHOTOGRID: the grid reads only the photos an import batch or an edit touched, not the whole list; earlier: LRC-PRINT-PAGE: Print Page Options; LR-META-XMP: fewer false unimported-XMP warnings) · **Target:** Adobe Lightroom Classic 15.5 / Lightroom 9.6
 
 The measured, row-by-row checklist: one row per Lightroom feature, menu item and shortcut, what we
 have and what is missing. It answers *does the feature exist?* (feature breadth). Whether it is good
@@ -927,3 +927,4 @@ Latin stays upright. Advanced Japanese composition remains open. Regression cove
 | 2026-10-16 | minor | LR-VIEW-PHOTOGRID: when the photos shown change by those an edit touched (an import's batch, a capture time, a crop), the grid reads only those photos' dates and shapes again instead of every photo of the list (a 16-photo batch at 181,749 photos: grid frame 101 ms → 7 ms); status unchanged |
 | 2026-10-19 | minor | LRC-LIB-LIBFOLDERS: the sidebar trees indent by a narrower step (14 / 12 / 10 / 8 pt) when the deepest open row would not fit the panel, instead of cutting its name or scrolling sideways at once (issue #2298); status unchanged (✅) |
 | 2026-10-15 | patch | LR-EXP-DIM: Width & Height (`fit: widthHeight`) limits the width and the height as the photo stands, beside Dimensions (W × H, either orientation), as in Lightroom (#1620); status unchanged (✅) |
+| 2026-10-22 | minor | MENU-BAR: on macOS the native menu gives a key back to the window while a text field has focus or a shortcut is being recorded (muda's `set_accelerator(None)` does nothing there, so the menu item is now swapped for one without a key, and the key is put back on that item afterwards); status unchanged (✅) |

docs/whats-new.md

@@ -7,6 +7,14 @@
   is removed and a new note made in its place, so the file it pointed to is never changed (as for the catalog's
   temporary files, issue #1502).
 
+### Keys reach text fields and the shortcut editor on macOS
+- While a text field has focus, the native menu now lets go of the keys the field needs (`G`, the digits `0`–`9`,
+  `P`, `U` and the like), and while Keyboard Shortcuts is recording a key it lets go of all of them (⌘, and ⌘Q in
+  the LightCraft menu too), except the system's own items (Hide LightCraft ⌘H, Hide Others ⌥⌘H, Close Window ⌘W
+  and Minimize ⌘M) and the two keys of the Help menu (⌘/ and F1). Before, the menu item kept its key on macOS, so the
+  key could still run the menu command instead of reaching the field or the editor. The keys come back on the menu
+  items when the field loses focus or recording ends; the menu's order, checkmarks and enabled state are unchanged.
+
 ## 2026-10-21
 
 ### Sharper resampling in HDR and panorama merges