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]