Merge pull request #1129 from itrycodinganditsbad/fix-902

fix(menus): native menu follows command enablement (#902)

apps/filmcraft/src/main.rs

@@ -335,7 +335,7 @@ fn app_creator(launch: Launch, graphics: graphics::GraphicsStart, created: Arc<A
         {
             let (rx, update) = native_menu::install(&app, cc.egui_ctx.clone());
             app.command_inbox = Some(rx);
-            app.hooks.shortcuts_changed = Some(update);
+            app.hooks.menu_changed = Some(update);
             app.ui.show_menu_bar = false;
         }
         if let Some(port) = control_port {

apps/filmcraft/src/native_menu.rs

@@ -19,12 +19,13 @@ fn accel(s: &str) -> Option<Accelerator> {
     mapped.parse().ok()
 }
 
-/// Updates native key equivalents when the active keyboard shortcuts change.
-pub type ShortcutUpdater = Box<dyn FnMut(&[Item])>;
+/// Updates native item labels, key equivalents and enablement when the menu tree changes.
+pub type MenuUpdater = Box<dyn FnMut(&[Item])>;
 
-pub fn install(app: &FilmcraftApp, ctx: egui::Context) -> (Receiver<String>, ShortcutUpdater) {
+pub fn install(app: &FilmcraftApp, ctx: egui::Context) -> (Receiver<String>, MenuUpdater) {
     let items = menu_items(app);
-    let mut native: Vec<(String, MenuItem)> = Vec::new();
+    // each native item with the menu entry it currently shows, so updates touch only what changed
+    let mut native: Vec<(Item, MenuItem)> = Vec::new();
     let mut titles: Vec<(String, Submenu)> = Vec::new();
     let bar = Menu::new();
     let app_menu = Submenu::new("FilmCraft", true);
@@ -34,16 +35,20 @@ pub fn install(app: &FilmcraftApp, ctx: egui::Context) -> (Receiver<String>, Sho
     titles.push((settings_label.into(), settings.clone()));
     for it in items.iter().filter(|i| i.id.starts_with("app.settings.")) {
         let label = it.label.trim_end_matches('…').to_string();
-        let mi = MenuItem::with_id(it.id.clone(), label, true, it.shortcut.as_deref().and_then(accel));
-        native.push((it.id.clone(), mi.clone()));
+        let mi = MenuItem::with_id(it.id.clone(), label, it.enabled, it.shortcut.as_deref().and_then(accel));
+        native.push((it.clone(), mi.clone()));
         let _ = settings.append(&mi);
     }
     // About and Discord repeat Help menu commands: same translated labels, updated with them
     let label = |id: &str, english: &str| items.iter().find(|i| i.id == id).map_or_else(|| english.to_string(), |i| i.label.clone());
     let about = MenuItem::with_id("app.about", label("app.about", "About FilmCraft"), true, None);
     let discord = MenuItem::with_id("help.discord", label("help.discord", "Join the ArtCraft Discord…"), true, None);
-    native.push(("app.about".into(), about.clone()));
-    native.push(("help.discord".into(), discord.clone()));
+    for (id, mi) in [("app.about", &about), ("help.discord", &discord)] {
+        if let Some(it) = items.iter().find(|i| i.id == id) {
+            // recorded as built (enabled, no shortcut), so the first update applies the real state
+            native.push((Item { enabled: true, shortcut: None, ..it.clone() }, mi.clone()));
+        }
+    }
     // the predefined items keep their native actions; our text lets them follow the language
     let predefined = [
         (hide_label, PredefinedMenuItem::hide(Some(app.ui.language.tr(hide_label)))),
@@ -71,8 +76,8 @@ pub fn install(app: &FilmcraftApp, ctx: egui::Context) -> (Receiver<String>, Sho
         // their first item appears
         let mut subs: Vec<(Vec<String>, Submenu)> = Vec::new();
         for it in mine {
-            let mi = MenuItem::with_id(it.id.clone(), &it.label, true, it.shortcut.as_deref().and_then(accel));
-            native.push((it.id.clone(), mi.clone()));
+            let mi = MenuItem::with_id(it.id.clone(), &it.label, it.enabled, it.shortcut.as_deref().and_then(accel));
+            native.push(((*it).clone(), mi.clone()));
             let mut parent = sub.clone();
             for depth in 1..it.path.len() {
                 let key = it.path[..=depth].to_vec();
@@ -105,7 +110,8 @@ pub fn install(app: &FilmcraftApp, ctx: egui::Context) -> (Receiver<String>, Sho
             ctx.request_repaint();
         }
     });
-    let update: ShortcutUpdater = Box::new(move |items: &[Item]| {
+    let mut shown_language = app.ui.language;
+    let update: MenuUpdater = Box::new(move |items: &[Item]| {
         let checked = |id: &str| items.iter().any(|it| it.id == id && it.checked == Some(true));
         let language = if checked("app.language.japanese") {
             filmcraft_ui_egui::i18n::Language::Ja
@@ -128,17 +134,28 @@ pub fn install(app: &FilmcraftApp, ctx: egui::Context) -> (Receiver<String>, Sho
         } else {
             filmcraft_ui_egui::i18n::Language::En
         };
+        if language != shown_language {
+            shown_language = language;
             for (title, submenu) in &titles {
                 submenu.set_text(language.tr(title));
             }
+        }
         for (label, item) in &predefined {
             item.set_text(language.tr(label));
         }
-        for (id, mi) in &native {
-            if let Some(it) = items.iter().find(|i| &i.id == id) {
-                mi.set_text(&it.label);
+        for (shown, mi) in &mut native {
+            let Some(it) = items.iter().find(|i| i.id == shown.id) else { continue };
+            if it.label != shown.label {
+                // Settings entries drop the trailing ellipsis, as when the menu was built
+                mi.set_text(if it.id.starts_with("app.settings.") { it.label.trim_end_matches('…') } else { &it.label });
+            }
+            if it.shortcut != shown.shortcut {
                 let _ = mi.set_accelerator(it.shortcut.as_deref().and_then(accel));
             }
+            if it.enabled != shown.enabled {
+                mi.set_enabled(it.enabled);
+            }
+            *shown = it.clone();
         }
     });
     (rx, update)

crates/ui-egui/src/lib.rs

@@ -129,8 +129,9 @@ pub struct HostHooks {
     /// Save dialog with a filter: (filter name, extensions, suggested file name) → path. The
     /// suggestion may be a full path, and the dialog then starts in its folder.
     pub pick_save_as: Option<Box<dyn FnMut(&str, &[&str], &str) -> Option<String>>>,
-    /// The active keyboard shortcuts changed: update native menu key equivalents.
-    pub shortcuts_changed: Option<Box<dyn FnMut(&[menus::MenuItem])>>,
+    /// The menu tree changed (labels, shortcuts, enablement, checkmarks): update the native menu
+    /// bar. Called from the frame loop only when something differs from the last call.
+    pub menu_changed: Option<Box<dyn FnMut(&[menus::MenuItem])>>,
     /// The user's preferred languages (locale tags, most preferred first) for Interface Language ▸
     /// System Language. Without it, System Language is English.
     pub system_languages: Option<Box<dyn Fn() -> Vec<String>>>,
@@ -349,8 +350,10 @@ pub struct FilmcraftApp {
     pub fps: f32,
     last_time: f64,
     bindings: Vec<menus::KeyBinding>,
-    /// Shortcut-set revision `bindings` (and the native menu) were built from.
+    /// Shortcut-set revision `bindings` were built from.
     bindings_rev: u64,
+    /// The menu tree last sent to `hooks.menu_changed` (the native menu bar).
+    menu_sent: Vec<menus::MenuItem>,
     /// Keyboard Shortcuts dialog state.
     pub shortcut_editor: panels::shortcuts_dialog::EditorState,
     pub toast: Option<(String, f64)>,
@@ -576,6 +579,7 @@ impl FilmcraftApp {
             last_time: 0.0,
             bindings: Vec::new(),
             bindings_rev: 0,
+            menu_sent: Vec::new(),
             shortcut_editor: Default::default(),
             toast: None,
             graphics_notice: None,
@@ -710,10 +714,6 @@ impl FilmcraftApp {
                 self.ui.language = language;
             }
             i18n::set_current(self.ui.language);
-            let items = menus::menu_items(self);
-            if let Some(hook) = self.hooks.shortcuts_changed.as_mut() {
-                hook(&items);
-            }
         }
         self.frames.set_cache_budget(p.memory.frame_cache_mb as usize * (1 << 20));
         self.ui.play_after_render = p.timeline.play_after_rendering;
@@ -1412,16 +1412,29 @@ impl FilmcraftApp {
 
     // ---------------------------------------------------------------- input
 
+    /// Sends the menu tree to the native menu bar when it changed, so its items show the same
+    /// labels, shortcuts and enablement as the in-window menus (Undo with nothing to undo is
+    /// disabled). Without a native menu bar this does nothing.
+    fn sync_native_menu(&mut self) {
+        if self.hooks.menu_changed.is_none() {
+            return;
+        }
+        let items = menus::menu_items(self);
+        if items == self.menu_sent {
+            return;
+        }
+        if let Some(hook) = self.hooks.menu_changed.as_mut() {
+            hook(&items);
+        }
+        self.menu_sent = items;
+    }
+
     fn handle_shortcuts(&mut self, ctx: &egui::Context) {
         let workspaces = (dock::names(&self.workspaces), self.ui.workspace.clone());
         if self.bindings_rev != self.session.shortcuts.revision || workspaces != self.menu_workspaces {
             self.menu_workspaces = workspaces;
             self.bindings = menus::bindings(self);
             self.bindings_rev = self.session.shortcuts.revision;
-            let items = menus::menu_items(self);
-            if let Some(hook) = self.hooks.shortcuts_changed.as_mut() {
-                hook(&items);
-            }
         }
         if ctx.egui_wants_keyboard_input() || self.dialog == Some(Dialog::Shortcuts) {
             return;
@@ -1720,6 +1733,7 @@ impl FilmcraftApp {
         }
         self.begin_press(&ctx);
         self.handle_shortcuts(&ctx);
+        self.sync_native_menu();
         self.advance_playback(&ctx);
         self.advance_source_playback(&ctx);
         self.scrub_audio(&ctx);
@@ -2597,3 +2611,43 @@ mod screen_px_tests {
         }
     }
 }
+
+#[cfg(test)]
+mod native_menu_tests {
+    use super::*;
+    use std::cell::RefCell;
+    use std::rc::Rc;
+
+    fn enabled(items: &[menus::MenuItem], id: &str) -> Option<bool> {
+        items.iter().find(|it| it.id == id).map(|it| it.enabled)
+    }
+
+    /// The native menu bar (macOS) gets the menu tree whenever it changes, so Edit ▸ Undo and Redo
+    /// follow the history instead of staying enabled (#902).
+    #[test]
+    fn native_menu_follows_undo_and_redo_availability() {
+        let sent: Rc<RefCell<Vec<Vec<menus::MenuItem>>>> = Rc::default();
+        let mut app = FilmcraftApp::new(Session::default());
+        let sink = sent.clone();
+        app.hooks.menu_changed = Some(Box::new(move |items: &[menus::MenuItem]| sink.borrow_mut().push(items.to_vec())));
+
+        app.sync_native_menu();
+        let last = |sent: &Rc<RefCell<Vec<Vec<menus::MenuItem>>>>, id: &str| sent.borrow().last().and_then(|items| enabled(items, id));
+        assert_eq!(last(&sent, "edit.undo"), Some(false), "nothing to undo in an empty project");
+        assert_eq!(last(&sent, "edit.redo"), Some(false), "nothing to redo in an empty project");
+        assert_eq!(last(&sent, "file.newProject"), Some(true));
+
+        app.sync_native_menu();
+        assert_eq!(sent.borrow().len(), 1, "an unchanged menu is not sent again");
+
+        app.session.execute("file.newSequence", json!({"width":16,"height":16})).unwrap();
+        app.sync_native_menu();
+        assert_eq!(last(&sent, "edit.undo"), Some(true), "an edit can be undone");
+        assert_eq!(last(&sent, "edit.redo"), Some(false));
+
+        app.session.execute("edit.undo", json!({})).unwrap();
+        app.sync_native_menu();
+        assert_eq!(last(&sent, "edit.undo"), Some(false));
+        assert_eq!(last(&sent, "edit.redo"), Some(true), "the undone edit can be redone");
+    }
+}

crates/ui-egui/src/menus.rs

@@ -224,10 +224,6 @@ pub fn invoke(app: &mut FilmcraftApp, ctx: &egui::Context, id: &str, mut params:
         if let Err(e) = saved {
             app.ui.status = tlf!("The language changed but could not be saved: {e}", e);
         }
-        let items = menu_items(app);
-        if let Some(hook) = app.hooks.shortcuts_changed.as_mut() {
-            hook(&items);
-        }
         ctx.request_repaint();
         return Ok(json!(app.ui.language));
     }
@@ -641,7 +637,7 @@ fn encode_png(width: usize, height: usize, rgba: Vec<u8>) -> Result<Vec<u8>, Str
 }
 
 /// A menu tree entry for display / `ui.menu.list`.
-#[derive(Clone, Debug, serde::Serialize)]
+#[derive(Clone, Debug, PartialEq, serde::Serialize)]
 pub struct MenuItem {
     pub id: String,
     pub label: String,