From 01cbf8f2eb5ec22be59bffb488346cd567683d08 Mon Sep 17 00:00:00 2001 From: tylerphotos <93777540+tylerphotos@users.noreply.github.com> Date: Thu, 22 Oct 2026 07:13:14 -0500 Subject: [PATCH] KEY-GRID: refresh native handles when shortcut ownership changes Include effective accelerators in the shared native menu cache key, with a regression for same-text direct-to-cycle key transfer. Document the touched menu helpers. Co-Authored-By: Codex --- apps/lightcraft/src/native_menu.rs | 41 ++++++----------------- crates/ui-egui/src/menubar.rs | 52 ++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 32 deletions(-) diff --git a/apps/lightcraft/src/native_menu.rs b/apps/lightcraft/src/native_menu.rs index 67e20e31c..c52999b80 100644 --- a/apps/lightcraft/src/native_menu.rs +++ b/apps/lightcraft/src/native_menu.rs @@ -16,7 +16,9 @@ use std::collections::{HashMap, HashSet}; use std::sync::mpsc::{Receiver, channel}; use lightcraft_ui_egui::LightcraftApp; -use lightcraft_ui_egui::menubar::{MenuNode, menu_bar, native_item_presentation, native_menu_delivers as menu_delivers}; +use lightcraft_ui_egui::menubar::{ + MenuNode, menu_bar, native_item_presentation, native_menu_delivers as menu_delivers, native_menu_structure as structure_of, +}; use muda::accelerator::{Accelerator, Code, Modifiers}; use muda::{CheckMenuItem, Menu, MenuEvent, MenuItem, PredefinedMenuItem, Submenu}; use serde_json::Value; @@ -164,34 +166,6 @@ fn label_text(s: &str) -> String { lightcraft_ui_egui::i18n::tr(s).replace('&', "&&") } -fn structure_of(bar: &[(String, Vec)]) -> String { - fn walk(n: &[MenuNode], out: &mut String) { - for x in n { - match x { - MenuNode::Item { id, params, checked, shortcut, .. } => { - out.push_str(&MenuNode::key(id, params)); - out.push(if checked.is_some() { 'c' } else { 'i' }); - out.push_str(shortcut.as_deref().unwrap_or("")); - } - MenuNode::Separator => out.push('-'), - MenuNode::Submenu { label, children } => { - out.push('['); - out.push_str(label); - walk(children, out); - out.push(']'); - } - } - out.push(';'); - } - } - let mut s = format!("{};", lightcraft_ui_egui::i18n::language().code()); - for (t, items) in bar { - s.push_str(t); - walk(items, &mut s); - } - s -} - /// Drop a trailing separator (left behind when an item moved to the app menu). fn tidy_separators(mut nodes: Vec) -> Vec { while matches!(nodes.last(), Some(MenuNode::Separator)) { @@ -215,9 +189,10 @@ impl NativeMenu { m } + /// Recreate native handles and shortcut ownership from the current shared menu model. fn rebuild(&mut self, app: &mut LightcraftApp) { let bar = menu_bar(app); - self.structure = structure_of(&bar); + self.structure = structure_of(&bar, &app.ui.settings.keymap); self.menu = Menu::new(); self.items.clear(); @@ -283,6 +258,7 @@ impl NativeMenu { self.publish_shortcuts(app); } + /// Install translated rows with their effective accelerators, preserving hint-only keys. fn append_nodes(&mut self, sub: &Submenu, nodes: &[MenuNode], keymap: &lightcraft_ui_egui::shortcuts::Keymap) { for n in nodes { match n { @@ -361,10 +337,11 @@ impl NativeMenu { } } let bar = menu_bar(app); - if structure_of(&bar) != self.structure { + if structure_of(&bar, &app.ui.settings.keymap) != self.structure { self.rebuild(app); return quit; } + /// Synchronize live labels and states; accelerator ownership changes already triggered a rebuild. fn walk(nodes: &[MenuNode], items: &mut HashMap, keymap: &lightcraft_ui_egui::shortcuts::Keymap) { for n in nodes { match n { @@ -438,7 +415,7 @@ mod tests { let mut seen = std::collections::BTreeSet::new(); for language in Locale::ALL { set_language(*language); - assert!(seen.insert(structure_of(&bar)), "{language:?} shares another language's menu structure key"); + assert!(seen.insert(structure_of(&bar, &app.ui.settings.keymap)), "{language:?} shares another language's menu structure key"); } set_language(Locale::ZhHant); assert_eq!(label_text("File"), "檔案"); diff --git a/crates/ui-egui/src/menubar.rs b/crates/ui-egui/src/menubar.rs index 278ef178b..6b39bf7ea 100644 --- a/crates/ui-egui/src/menubar.rs +++ b/crates/ui-egui/src/menubar.rs @@ -81,6 +81,41 @@ pub fn native_item_presentation<'a>( } } +/// Cache key for native menu structure and effective accelerator ownership. A transfer from +/// a direct grid binding to a borrowed cycle hint must rebuild native handles and republish +/// their owned shortcuts even when the visible menu model is identical. +pub fn native_menu_structure(bar: &[(String, Vec)], keymap: &crate::shortcuts::Keymap) -> String { + /// Add stable identities and effective native keys; live checked values stay outside the cache key. + fn walk(n: &[MenuNode], out: &mut String, keymap: &crate::shortcuts::Keymap) { + for x in n { + match x { + MenuNode::Item { id, params, label, checked, shortcut, .. } => { + out.push_str(&MenuNode::key(id, params)); + out.push(if checked.is_some() { 'c' } else { 'i' }); + out.push_str(shortcut.as_deref().unwrap_or("")); + // A key can change owners while both visible hints stay identical. + out.push('|'); + out.push_str(native_item_presentation(keymap, id, params, label, shortcut.as_deref()).1.unwrap_or("")); + } + MenuNode::Separator => out.push('-'), + MenuNode::Submenu { label, children } => { + out.push('['); + out.push_str(label); + walk(children, out, keymap); + out.push(']'); + } + } + out.push(';'); + } + } + let mut s = format!("{};", crate::i18n::language().code()); + for (t, items) in bar { + s.push_str(t); + walk(items, &mut s, keymap); + } + s +} + /// Top-level menus in order (the macOS app menu is added by the host). pub const MENUS: &[&str] = &["File", "Edit", "View", "Photo", "Window", "Help"]; @@ -760,6 +795,7 @@ pub(crate) fn expanded(app: &LightcraftApp, name: &str) -> Option> }) } +/// Resolve a live menu row, including contextual keys and borrowed grid-cycle hints. fn node(app: &LightcraftApp, e: &MenuEntry) -> MenuNode { // Shift+P picks and advances in Library; don't advertise it on Presets in those views. let shortcut = if e.id == "panel.presets" && crate::shortcuts::library_grid(app) { None } else { e.shortcut.as_deref() }; @@ -1179,6 +1215,22 @@ mod tests { } } + /// #2513 review: moving a direct-grid key to the cycle command leaves the visible model + /// unchanged, but must invalidate the native cache so its old accelerator is removed. + #[test] + fn grid_menu_shortcut_hints_invalidate_native_ownership_cache() { + let mut a = app(); + crate::shortcuts::assign(&mut a.ui.settings.keymap, "view.gridToggle", None).unwrap(); + crate::shortcuts::assign(&mut a.ui.settings.keymap, "view.photoGrid", Some("Cmd+Shift+G")).unwrap(); + crate::shortcuts::assign(&mut a.ui.settings.keymap, "view.squareGrid", Some("Cmd+Shift+2")).unwrap(); + let before = menu_bar(&a); + let direct = native_menu_structure(&before, &a.ui.settings.keymap); + crate::shortcuts::assign(&mut a.ui.settings.keymap, "view.gridToggle", Some("Cmd+Shift+G")).unwrap(); + let after = menu_bar(&a); + assert_eq!(before, after, "visible hint text does not expose the ownership change"); + assert_ne!(direct, native_menu_structure(&after, &a.ui.settings.keymap), "rebuild removes the stale direct-grid accelerator"); + } + /// #2500: grid view rows show the separate cycle command's current shortcut. #[test] fn grid_menu_shortcut_hints_follow_the_cycle_key() {