collective
Browse / github.com/storytold/filmcraft / patch
macOS menu bar: keep the menu tree between frames, don't rebuild it every frame
macOS menu bar: keep the menu tree between frames, don't rebuild it every frame ### Problem `sync_native_menu` runs every frame. It rebuilt the whole tree with `menus::menu_items`: 387 rows of translated labels, a `Shortcuts::primary` scan of all 308 bindings for each row, and a few thousand small allocations. It did that only to compare the result with the tree it sent last time. On an Apple M2 Max that costs **about 0.4 ms a frame**, every frame the app draws: playback, scrubbing, drags and the meter drop after Stop. That is 5% of a 120 Hz frame's budget, and on `fa550ce2` it was a fifth of the CPU time of a frame that only animated the meters (1.74 → 1.40 ms; 39% of i
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- ZJHeepfixer (imported from pull request #3285, open)
- Base
a504f3c5adc4cd60d9c1120916cddb50c9b24b86- Size
- +263 −31 in 5 files
- Review
- pending
- Shared
- 2026-10-28
- Id
5515488b4477fd89e83bdc40
How it went
No other operator has reported using it yet.
Diff
raw · crates/engine/src/commands.rs crates/engine/src/lib.rs crates/ui-egui/src/lib.rs crates/ui-egui/src/menus.rs docs/performance.md
@@ -35,6 +35,14 @@ pub struct CommandSpec { pub journal: bool, } +impl CommandSpec {+ /// Whether the command can run on `s`'s current selection ([`Session::is_enabled`], for a+ /// caller that already holds the spec: the menus ask several hundred of them every frame).+ pub fn is_enabled(&self, s: &Session) -> bool {+ (self.enabled)(s).is_ok()+ }+}+ macro_rules! cmd { ($id:literal, $label:literal, [$($m:literal),*], $sc:expr, $params:literal, $en:expr, $run:expr) => { CommandSpec { id: $id, label: $label, menu: &[$($m),*], shortcut: $sc, params: $params, enabled: $en, run: $run, journal: true }@@ -829,7 +829,7 @@ impl Session { /// Whether the command can run on the current selection (what the menus show). pub fn is_enabled(&self, id: &str) -> bool {- commands::find(id).is_some_and(|c| (c.enabled)(self).is_ok())+ commands::find(id).is_some_and(|c| c.is_enabled(self)) } /// What is open, to store beside the project when it is saved.@@ -374,8 +374,8 @@ pub struct FilmcraftApp { bindings: Vec<menus::KeyBinding>, /// 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>,+ /// The menu tree for `hooks.menu_changed` (the native menu bar), kept between frames.+ menu_cache: menus::MenuCache, /// Keyboard Shortcuts dialog state. pub shortcut_editor: panels::shortcuts_dialog::EditorState, pub toast: Option<(String, f64)>,@@ -604,7 +604,7 @@ impl FilmcraftApp { last_time: 0.0, bindings: Vec::new(), bindings_rev: 0,- menu_sent: Vec::new(),+ menu_cache: menus::MenuCache::default(), shortcut_editor: Default::default(), toast: None, graphics_notice: None,@@ -1531,19 +1531,19 @@ impl FilmcraftApp { /// 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.+ /// disabled). Runs every frame, so the tree is not rebuilt each time: see [`menus::MenuCache`].+ /// 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);+ let mut cache = std::mem::take(&mut self.menu_cache);+ if let Some(items) = cache.changed(self)+ && let Some(hook) = self.hooks.menu_changed.as_mut()+ {+ hook(items); }- self.menu_sent = items;+ self.menu_cache = cache; } /// Whether keyboard focus is on a dock divider or a floating panel's resize grip, as drawn last pass.@@ -2983,4 +2983,99 @@ mod native_menu_tests { assert_eq!(last(&sent, "edit.undo"), Some(false)); assert_eq!(last(&sent, "edit.redo"), Some(true), "the undone edit can be redone"); }++ /// The native menu is no longer rebuilt every frame (it cost ~0.5 ms a frame), so every kind+ /// of change it shows must still reach it, on the next frame: enablement, checkmarks, labels+ /// (also a language switched at runtime), shortcuts, workspaces and recent projects. After+ /// each change the tree sent must be exactly what a full rebuild gives.+ #[test]+ fn native_menu_follows_every_change_it_shows() {+ let sent: Rc<RefCell<Vec<Vec<menus::MenuItem>>>> = Rc::default();+ let mut session = Session::default();+ session.execute("file.openDemoProject", json!({})).unwrap();+ let mut app = FilmcraftApp::new(session);+ let sink = sent.clone();+ app.hooks.menu_changed = Some(Box::new(move |items: &[menus::MenuItem]| sink.borrow_mut().push(items.to_vec())));+ let ctx = egui::Context::default();+ let row = |sent: &Rc<RefCell<Vec<Vec<menus::MenuItem>>>>, id: &str| sent.borrow().last().and_then(|items| items.iter().find(|it| it.id == id).cloned());+ app.session.execute("edit.deselectAll", json!({})).unwrap();+ app.sync_native_menu();+ let ids: Vec<(String, Vec<String>)> = sent.borrow()[0].iter().map(|it| (it.id.clone(), it.path.clone())).collect();+ assert_eq!(row(&sent, "edit.copy").map(|it| it.enabled), Some(false), "nothing selected");++ type Change = Box<dyn Fn(&mut FilmcraftApp, &egui::Context)>;+ let menu = |id: &'static str| -> Change {+ Box::new(move |app, ctx| {+ let _ = menus::invoke(app, ctx, id, json!({}));+ })+ };+ let changes: Vec<(&str, Change)> = vec![+ ("select every clip", menu("edit.selectAll")),+ ("quarter playback resolution", menu("view.playbackRes.quarter")),+ ("a guide on the Program monitor", Box::new(|app, _| app.ui.program.guides.push(state::Guide { vertical: true, position: 10.0 }))),+ ("focus the Source monitor", Box::new(|app, _| app.ui.focused = dock::PanelKind::Source)),+ ("dynamic waveforms toggled", Box::new(|app, _| app.ui.extras.dynamic_waveforms = !app.ui.extras.dynamic_waveforms)),+ ("German at runtime", menu("app.language.german")),+ ("rename a label", Box::new(|app, _| app.session.prefs.labels.colors.entry("yellow".into()).or_default().name = "B-Roll".into())),+ ("rebind Save", Box::new(|app, _| assert!(app.session.shortcuts.set("file.save", "Cmd+Shift+9", None, false, true).is_ok()))),+ ("a recent project", Box::new(|app, _| app.session.prefs.general.recent_projects.insert(0, "/tmp/Trailer.fcproj".into()))),+ ("a saved workspace", Box::new(|app, _| app.workspaces.saved.push(dock::SavedWorkspace { name: "Grading".into(), ..Default::default() }))),+ ("the Audio workspace", menu("window.workspace.audio")),+ ("English again", menu("app.language.english")),+ ];+ for (what, change) in &changes {+ let before = menus::menu_items(&app);+ let count = sent.borrow().len();+ change(&mut app, &ctx);+ let expected = menus::menu_items(&app);+ assert_ne!(before, expected, "{what}: the change must show in the menu, or this step tests nothing");+ app.sync_native_menu();+ assert_eq!(sent.borrow().len(), count + 1, "{what}: the changed menu was not sent");+ assert!(sent.borrow().last() == Some(&expected), "{what}: the menu sent differs from a full rebuild");+ }+ assert_eq!(row(&sent, "edit.copy").map(|it| it.enabled), Some(true));+ assert_eq!(row(&sent, "view.playbackRes.quarter").and_then(|it| it.checked), Some(false), "Source monitor focused: its resolution");+ assert_eq!(row(&sent, "edit.label.yellow").map(|it| it.label), Some("B-Roll".to_string()));+ assert_eq!(row(&sent, "file.save").and_then(|it| it.shortcut), Some("Cmd+Shift+9".to_string()));+ assert_eq!(row(&sent, "file.openRecent.0").map(|it| it.label), Some("Trailer.fcproj".to_string()));+ assert_eq!(row(&sent, "file.clearRecent").map(|it| it.enabled), Some(true));+ assert_eq!(row(&sent, "window.workspace.audio").and_then(|it| it.checked), Some(true));+ assert_eq!(row(&sent, "window.workspace.grading").map(|it| it.label), Some("Grading".to_string()));+ assert_eq!(row(&sent, "view.lockGuides").map(|it| it.enabled), Some(false), "no guides on the focused Source monitor");+ assert_eq!(row(&sent, "app.language.english").and_then(|it| it.checked), Some(true));+ // German was shown in between: translated labels and the checkmark, on the same rows+ let german = sent.borrow().iter().find(|items| items.iter().any(|it| it.id == "app.language.german" && it.checked == Some(true))).cloned();+ let german = german.expect("the German menu was sent");+ let save = german.iter().find(|it| it.id == "file.save").map(|it| it.label.clone());+ assert_eq!(save.as_deref(), Some(i18n::Language::De.tr("Save")));+ assert_ne!(save.as_deref(), Some("Save"));+ // The native items were made once, at launch, and are matched by id: a language switch+ // must keep every id and path (designcraft#1548 lost its menu commands that way).+ let german_ids: Vec<(String, Vec<String>)> = german.iter().map(|it| (it.id.clone(), it.path.clone())).collect();+ let without_recent = |v: &[(String, Vec<String>)]| v.iter().filter(|(id, _)| !id.starts_with("file.openRecent.")).cloned().collect::<Vec<_>>();+ assert_eq!(without_recent(&german_ids), without_recent(&ids));+ }++ /// Frames where nothing it shows changed neither rebuild the rows nor resend them; a change+ /// of enablement alone refreshes them without a rebuild.+ #[test]+ fn an_unchanged_native_menu_is_not_rebuilt() {+ let sent = Rc::new(RefCell::new(0usize));+ let mut app = FilmcraftApp::new(Session::default());+ let sink = sent.clone();+ app.hooks.menu_changed = Some(Box::new(move |_: &[menus::MenuItem]| *sink.borrow_mut() += 1));+ for _ in 0..100 {+ app.sync_native_menu();+ }+ assert_eq!((*sent.borrow(), app.menu_cache.builds), (1, 1));+ app.session.execute("file.newSequence", json!({"width":16,"height":16})).unwrap();+ app.sync_native_menu();+ assert_eq!((*sent.borrow(), app.menu_cache.builds), (2, 1), "Undo became enabled: sent, from the same rows");+ app.ui.language = i18n::Language::Ja;+ app.sync_native_menu();+ assert_eq!((*sent.borrow(), app.menu_cache.builds), (3, 2), "new labels: rebuilt");+ let mut none = FilmcraftApp::new(Session::default());+ none.sync_native_menu();+ assert_eq!(none.menu_cache.builds, 0, "no native menu bar, no menu tree");+ } }@@ -696,34 +696,156 @@ pub const MENUS: [&str; 9] = ["File", "Edit", "Clip", "Sequence", "Markers", "Gr pub const NATIVE_APP_MENU: [&str; 4] = ["Settings", "Hide FilmCraft", "Hide Others", "Quit FilmCraft"]; pub fn menu_items(app: &FilmcraftApp) -> Vec<MenuItem> {+ let mut v = menu_rows(app);+ let live: Vec<Live> = v.iter().map(|it| Live::of(&it.id)).collect();+ menu_state(app, &mut v, &live);+ v+}++/// The rows of the menu tree: ids, translated labels, menu paths and shortcuts, with Window ▸+/// Workspaces and File ▸ Open Recent spliced in. They are read only from what [`RowsFrom`] keeps+/// a copy of, so [`MenuCache`] can tell when they are out of date. Enablement and checkmarks+/// follow the selection, the playhead, the focused monitor and more: [`menu_state`] sets them.+fn menu_rows(app: &FilmcraftApp) -> Vec<MenuItem> { let mut v = menu_items_for(&app.session); for it in &mut v { it.label = app.ui.language.tr(&it.label).to_string();- match it.id.as_str() {- "app.language.english" => it.checked = Some(app.ui.language == crate::i18n::Language::En),- "app.language.japanese" => it.checked = Some(app.ui.language == crate::i18n::Language::Ja),- "app.language.spanish" => it.checked = Some(app.ui.language == crate::i18n::Language::Es),- "app.language.portuguese" => it.checked = Some(app.ui.language == crate::i18n::Language::PtBr),- "app.language.ukrainian" => it.checked = Some(app.ui.language == crate::i18n::Language::Uk),- "app.language.chinese" => it.checked = Some(app.ui.language == crate::i18n::Language::ZhCn),- "app.language.german" => it.checked = Some(app.ui.language == crate::i18n::Language::De),- "app.language.russian" => it.checked = Some(app.ui.language == crate::i18n::Language::Ru),- "app.language.french" => it.checked = Some(app.ui.language == crate::i18n::Language::Fr),- "app.language.indonesian" => it.checked = Some(app.ui.language == crate::i18n::Language::Id),- "app.language.italian" => it.checked = Some(app.ui.language == crate::i18n::Language::It),- "app.language.korean" => it.checked = Some(app.ui.language == crate::i18n::Language::Ko),- _ => {}- }- if it.id.starts_with("view.") {- it.checked = crate::panels::monitor_view::checked(app, &it.id).or(crate::panels::menu_dialogs::checked(app, &it.id));- it.enabled &= crate::panels::monitor_view::enabled(app, &it.id);- } } crate::panels::workspaces::menu(app, &mut v); open_recent(&mut v, &app.session, |n| app.ui.language.tr(n).to_string()); v } +/// Where a row's enablement and checkmark come from when they change without the rows changing.+#[derive(Clone, Copy)]+enum Live {+ /// Nothing: set with the rows (frontend commands, panels, workspaces, recent projects).+ Fixed,+ /// An engine command: enabled when it can run now.+ Command(&'static filmcraft_engine::CommandSpec),+ /// A View item, engine command or not: also enabled and checked by the monitors' state.+ View(Option<&'static filmcraft_engine::CommandSpec>),+ /// Edit ▸ Language: checked for the interface language.+ Language(crate::i18n::Language),+}++impl Live {+ fn of(id: &str) -> Self {+ use crate::i18n::Language;+ let command = filmcraft_engine::find_command(id);+ if id.starts_with("view.") {+ return Self::View(command);+ }+ if let Some(c) = command {+ return Self::Command(c);+ }+ let language = match id {+ "app.language.english" => Language::En,+ "app.language.japanese" => Language::Ja,+ "app.language.spanish" => Language::Es,+ "app.language.portuguese" => Language::PtBr,+ "app.language.ukrainian" => Language::Uk,+ "app.language.chinese" => Language::ZhCn,+ "app.language.german" => Language::De,+ "app.language.russian" => Language::Ru,+ "app.language.french" => Language::Fr,+ "app.language.indonesian" => Language::Id,+ "app.language.italian" => Language::It,+ "app.language.korean" => Language::Ko,+ _ => return Self::Fixed,+ };+ Self::Language(language)+ }+}++/// The live part of each row (`live` says which part that is): whether an engine command can run+/// now, the View items' enablement and checkmarks, the Language checkmarks. Each value is set+/// afresh, never from what the row held before, so the same rows can be refreshed every frame.+fn menu_state(app: &FilmcraftApp, items: &mut [MenuItem], live: &[Live]) {+ for (it, live) in items.iter_mut().zip(live) {+ match *live {+ Live::Fixed => {}+ Live::Command(c) => it.enabled = c.is_enabled(&app.session),+ Live::View(c) => {+ it.enabled = c.is_none_or(|c| c.is_enabled(&app.session)) && crate::panels::monitor_view::enabled(app, &it.id);+ it.checked = crate::panels::monitor_view::checked(app, &it.id).or(crate::panels::menu_dialogs::checked(app, &it.id));+ }+ Live::Language(language) => it.checked = Some(app.ui.language == language),+ }+ }+}++/// Copies of everything [`menu_rows`] reads: the interface language (labels), the shortcut set+/// (by its revision), Settings ▸ Labels (Edit ▸ Label names), the saved workspaces and the one in+/// use (Window ▸ Workspaces and its checkmark) and the recent projects (File ▸ Open Recent).+#[derive(PartialEq)]+struct RowsFrom {+ language: crate::i18n::Language,+ shortcuts: u64,+ labels: filmcraft_engine::settings::LabelPrefs,+ workspaces: Vec<String>,+ workspace: String,+ recent: Vec<String>,+}++impl RowsFrom {+ fn of(app: &FilmcraftApp) -> Self {+ Self {+ language: app.ui.language,+ shortcuts: app.session.shortcuts.revision,+ labels: app.session.prefs.labels.clone(),+ workspaces: app.workspaces.saved.iter().map(|w| w.name.clone()).collect(),+ workspace: app.ui.workspace.clone(),+ recent: app.session.prefs.general.recent_projects.clone(),+ }+ }++ /// Whether rows built from `self` are still `app`'s rows (compared in place, no copies).+ fn matches(&self, app: &FilmcraftApp) -> bool {+ self.language == app.ui.language+ && self.shortcuts == app.session.shortcuts.revision+ && self.labels == app.session.prefs.labels+ && self.workspaces.iter().eq(app.workspaces.saved.iter().map(|w| &w.name))+ && self.workspace == app.ui.workspace+ && self.recent == app.session.prefs.general.recent_projects+ }+}++/// The menu tree kept between frames for the native menu bar. Building it from scratch (several+/// hundred rows of translated labels, shortcut lookups and small allocations) took about half a+/// millisecond, every frame. The rows are rebuilt only when what they are made from changed;+/// their enablement and checkmarks are still read every time, so nothing they show can lag.+#[derive(Default)]+pub struct MenuCache {+ from: Option<RowsFrom>,+ rows: Vec<MenuItem>,+ /// Which part of each row [`menu_state`] refreshes, worked out when the rows are built.+ live: Vec<Live>,+ /// The tree as last handed out by [`MenuCache::changed`].+ sent: Vec<MenuItem>,+ /// How often the rows were built (tests).+ pub(crate) builds: u64,+}++impl MenuCache {+ /// The menu tree ([`menu_items`]) when it differs from the one this returned last time, else+ /// `None`.+ pub fn changed(&mut self, app: &FilmcraftApp) -> Option<&[MenuItem]> {+ if !self.from.as_ref().is_some_and(|from| from.matches(app)) {+ self.rows = menu_rows(app);+ self.live = self.rows.iter().map(|it| Live::of(&it.id)).collect();+ self.from = Some(RowsFrom::of(app));+ self.builds += 1;+ }+ menu_state(app, &mut self.rows, &self.live);+ if self.rows == self.sent {+ return None;+ }+ self.sent.clone_from(&self.rows);+ Some(&self.sent)+ }+}+ /// File ▸ Open Recent, after Open Project…: the recent projects (General ▸ `recentProjects`), /// newest first, then Clear Recent Files. Clear is always there, so the submenu exists (in the /// native menu too, built once at launch) before the first project is saved.@@ -18,6 +18,13 @@ finishes. Transcript words and paragraphs are cached by project snapshot and act search highlights by query. Playhead movement reuses them; project edits, undo/redo and view changes refresh them. Highlight lookup is constant work per word, including searches with many matches. +The native macOS menu bar is brought up to date every frame, but its rows (translated labels, menu+paths, shortcuts, Window ▸ Workspaces and File ▸ Open Recent) are kept between frames and rebuilt+only when the language, the shortcut set, Settings ▸ Labels, the workspaces or the recent projects+change (`menus::MenuCache`). Enablement and checkmarks are still read every frame. Rebuilding the+whole tree took about 0.4 ms a frame on an Apple M2 Max (5% of a 120 Hz frame, every frame of+playback, scrubbing or a drag); the frame-by-frame check now takes about 0.01 ms.+ To measure a long transcript, run `cargo test -p filmcraft-ui-egui --test it transcript_ui::long_transcript_redraw_benchmark -- --ignored --nocapture` alone. It reports median and p95 redraw time for 10,000 words with an empty query and a query matching