From 4d8dbddb86b451b783e3735436a16414007667d1 Mon Sep 17 00:00:00 2001 From: Nathaniel Ledford Date: Thu, 22 Oct 2026 16:40:34 -0400 Subject: [PATCH 1/4] MENU-BAR: a menu item really gives its key up to a text field or shortcut recording on macOS --- apps/lightcraft/src/native_menu.rs | 135 ++++++++++++++++++++++++++--- docs/parity-checklist.md | 3 +- docs/whats-new.md | 10 +++ 3 files changed, 137 insertions(+), 11 deletions(-) diff --git a/apps/lightcraft/src/native_menu.rs b/apps/lightcraft/src/native_menu.rs index 4ac40bcac..a60354bd1 100644 --- a/apps/lightcraft/src/native_menu.rs +++ b/apps/lightcraft/src/native_menu.rs @@ -39,18 +39,86 @@ 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, + /// The accelerator the item's shortcut maps to. accel: Option, + /// The accelerator the native item has now: `accel`, or `None` while it yields. + current: Option, label: String, enabled: bool, checked: Option, + /// 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, never per frame. + fn set_accelerator(&mut self, want: Option) { + if self.current == want { + return; + } + self.current = want; + if cfg!(target_os = "macos") && want.is_none() && self.swappable && self.replace_without_accelerator() { + return; + } + let _ = match &self.handle { + Handle::Plain(h) => h.set_accelerator(want), + Handle::Check(h) => h.set_accelerator(want), + }; + } + + 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)), + }; + if self.parent.remove(old).is_err() { + return false; + } + let new_ref: &dyn muda::IsMenuItem = match &new { + Handle::Plain(h) => h, + Handle::Check(h) => h, + }; + if self.parent.insert(new_ref, position).is_err() { + // the old item is already gone: an item at the end beats a missing one + let _ = self.parent.append(new_ref); + } + self.handle = new; + true + } +} + +/// 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, sc: &str, text_focus: bool, capturing: bool) -> Option { + if capturing || (text_focus && yields_to_text(sc)) { None } else { accel } } pub struct NativeMenu { menu: Menu, items: HashMap, + /// 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, rx: Receiver, /// Structure of the installed menu (ids, kinds, submenu names); a change rebuilds it. structure: String, @@ -216,7 +284,15 @@ 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, + }; m.rebuild(app); app.native_menu = true; m @@ -227,6 +303,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 +323,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 +357,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(&[ @@ -290,7 +385,8 @@ impl NativeMenu { 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 +394,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 +430,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, @@ -421,16 +520,13 @@ impl NativeMenu { if focus != self.text_focus || capturing != self.capturing { self.text_focus = focus; self.capturing = capturing; - for it in self.items.values() { + 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); } self.publish_shortcuts(app); } @@ -463,6 +559,25 @@ 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"); + } + #[test] fn shortcuts_map_to_accelerators() { assert_eq!(accelerator("Cmd+Shift+Z"), Some(Accelerator::new(Modifiers::META | Modifiers::SHIFT, Code::KeyZ))); diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 99f17561e..3e741604a 100644 --- a/docs/parity-checklist.md +++ b/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 (✅) | diff --git a/docs/whats-new.md b/docs/whats-new.md index cffff83e2..cd48a7204 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -1,5 +1,15 @@ # What's new in LightCraft +## 2026-10-22 + +### 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 From 7fc8161fc96f2e334e2cbc85a250b079d6e710a8 Mon Sep 17 00:00:00 2001 From: Nathaniel Ledford Date: Fri, 23 Oct 2026 07:00:52 -0400 Subject: [PATCH 2/4] MENU-BAR: a rebuilt native menu applies the keys' yield state in the same frame --- apps/lightcraft/src/native_menu.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/apps/lightcraft/src/native_menu.rs b/apps/lightcraft/src/native_menu.rs index a60354bd1..031976b23 100644 --- a/apps/lightcraft/src/native_menu.rs +++ b/apps/lightcraft/src/native_menu.rs @@ -478,8 +478,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) { for n in nodes { From 2c73431db7d7e4fd2efc9402f2b92a21c90433b9 Mon Sep 17 00:00:00 2001 From: Nathaniel Ledford Date: Fri, 23 Oct 2026 07:52:07 -0400 Subject: [PATCH 3/4] MENU-BAR: a failed menu item swap keeps the old item and its cached accelerator --- apps/lightcraft/src/native_menu.rs | 108 ++++++++++++++++++++++++++--- 1 file changed, 100 insertions(+), 8 deletions(-) diff --git a/apps/lightcraft/src/native_menu.rs b/apps/lightcraft/src/native_menu.rs index 031976b23..853cf549c 100644 --- a/apps/lightcraft/src/native_menu.rs +++ b/apps/lightcraft/src/native_menu.rs @@ -69,16 +69,23 @@ impl Item { if self.current == want { return; } - self.current = want; - if cfg!(target_os = "macos") && want.is_none() && self.swappable && self.replace_without_accelerator() { + 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 next time + if self.replace_without_accelerator() { + self.current = want; + } return; } + self.current = want; let _ = match &self.handle { Handle::Plain(h) => h.set_accelerator(want), Handle::Check(h) => h.set_accelerator(want), }; } + /// 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), @@ -89,22 +96,54 @@ impl Item { 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)), }; - if self.parent.remove(old).is_err() { - return false; - } let new_ref: &dyn muda::IsMenuItem = match &new { Handle::Plain(h) => h, Handle::Check(h) => h, }; - if self.parent.insert(new_ref, position).is_err() { - // the old item is already gone: an item at the end beats a missing one - let _ = self.parent.append(new_ref); + if !swap_item(&self.parent, old, new_ref, position) { + return false; } self.handle = new; true } } +/// 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(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, sc: &str, text_focus: bool, capturing: bool) -> Option { @@ -579,6 +618,59 @@ mod tests { 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>, + fail_remove: bool, + /// The new item (9) cannot be inserted / appended. + reject_new_insert: bool, + reject_new_append: 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 { + 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) { + *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" + ); + } + #[test] fn shortcuts_map_to_accelerators() { assert_eq!(accelerator("Cmd+Shift+Z"), Some(Accelerator::new(Modifiers::META | Modifiers::SHIFT, Code::KeyZ))); From 6889fe223ba49a44a5caeaad6a404b23e2d27ed2 Mon Sep 17 00:00:00 2001 From: Nathaniel Ledford Date: Fri, 23 Oct 2026 08:21:57 -0400 Subject: [PATCH 4/4] MENU-BAR: a menu item's cached accelerator follows the native call, and a failed one is retried --- apps/lightcraft/src/native_menu.rs | 59 +++++++++++++++++++++++++----- 1 file changed, 49 insertions(+), 10 deletions(-) diff --git a/apps/lightcraft/src/native_menu.rs b/apps/lightcraft/src/native_menu.rs index 853cf549c..3a491b2ec 100644 --- a/apps/lightcraft/src/native_menu.rs +++ b/apps/lightcraft/src/native_menu.rs @@ -64,24 +64,25 @@ impl Item { /// 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, never per frame. + /// 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) { 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 next time - if self.replace_without_accelerator() { - self.current = want; - } + // 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; } - self.current = want; - let _ = match &self.handle { + 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 @@ -108,6 +109,18 @@ impl Item { } } +/// 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, want: Option, native_call_worked: bool) -> Option { + 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 { @@ -165,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). @@ -331,6 +346,7 @@ impl NativeMenu { structure: String::new(), text_focus: false, capturing: false, + accelerators_pending: false, }; m.rebuild(app); app.native_menu = true; @@ -421,6 +437,7 @@ impl NativeMenu { self.menu.init_for_nsapp(); self.text_focus = false; self.capturing = false; + self.accelerators_pending = false; self.publish_shortcuts(app); } @@ -557,9 +574,10 @@ 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; + 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() { @@ -567,7 +585,9 @@ impl NativeMenu { } 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 @@ -627,6 +647,8 @@ mod tests { /// 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; @@ -638,7 +660,7 @@ mod tests { true } fn insert(&self, row: &u32, position: usize) -> bool { - if self.reject_new_insert && *row == 9 { + if (self.reject_new_insert && *row == 9) || (self.reject_old_insert && *row == 2) { return false; } self.rows.borrow_mut().insert(position, *row); @@ -669,6 +691,23 @@ mod tests { (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]