From c0e40f576ebb16ab92221bd952d64bd8c50d213e Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Wed, 11 Nov 2026 17:38:50 -0800 Subject: [PATCH 1/2] Keyboard Shortcuts: a plug-in installed while the menus are walked no longer leaves the keymap a row short The shortcut entries are built from the menus under the plug-in revision read before the walk. A plug-in installed or removed during the walk left entries that don't match that revision, while the keymap cached for the revision was built from the entries before the change, so the catalogue and the entries differed by a row (the_default_shortcuts_do_not_conflict failed when a test beside it installed a plug-in). The menus are now walked again when the revision changes during the walk, and a snapshot that still doesn't match is not cached. --- crates/ui-egui/src/shortcut_editor.rs | 68 +++++++++++++++++++++++---- 1 file changed, 60 insertions(+), 8 deletions(-) diff --git a/crates/ui-egui/src/shortcut_editor.rs b/crates/ui-egui/src/shortcut_editor.rs index b17a4c47a..9a2344515 100644 --- a/crates/ui-egui/src/shortcut_editor.rs +++ b/crates/ui-egui/src/shortcut_editor.rs @@ -374,12 +374,38 @@ pub(crate) struct Snapshot { /// a live-effect or filter plug-in installed later has its row, and one removed has none. pub(crate) fn snapshot() -> Arc { static LAST: Mutex>> = Mutex::new(None); - let revision = menus::plugin_revision(); + let mut revision = menus::plugin_revision(); let last = LAST.lock().unwrap_or_else(|e| e.into_inner()).clone(); if let Some(s) = last.filter(|s| s.revision == revision) { return s; } + // When a plug-in is installed or removed during the walk, the entries may not match + // `revision`, and the keymap cached for it would lack rows or hold extra ones. The menus are + // walked again at the new revision, and a snapshot that still doesn't match is not cached. + let mut s = build_snapshot(revision); + for _ in 0..8 { + let now = menus::plugin_revision(); + if now == revision { + let mut last = LAST.lock().unwrap_or_else(|e| e.into_inner()); + // A newer snapshot another thread stored meanwhile stays. + if last.as_ref().is_none_or(|l| l.revision <= revision) { + *last = Some(s.clone()); + } + return s; + } + revision = now; + s = build_snapshot(revision); + } + s +} + +/// The snapshot of the menus as they are now, under plug-in revision `revision`. +fn build_snapshot(revision: u64) -> Arc { // Built without the lock held: the Effect menu is walked anew. + #[cfg(test)] + if let Some(f) = tests::WHILE_WALKING.take() { + f(); + } let effects: Arc<[EffectItem]> = build_effect_items().into(); let filters: Arc<[FilterItem]> = build_filter_items().into(); let (head, tail) = fixed_entries(); @@ -396,13 +422,7 @@ pub(crate) fn snapshot() -> Arc { })) .chain(tail.iter().cloned()) .collect(); - let s = Arc::new(Snapshot { revision, effects, filters, entries }); - let mut last = LAST.lock().unwrap_or_else(|e| e.into_inner()); - // A newer snapshot another thread stored meanwhile stays. - if last.as_ref().is_none_or(|l| l.revision <= revision) { - *last = Some(s.clone()); - } - s + Arc::new(Snapshot { revision, effects, filters, entries }) } /// Every tool, command and panel the editor can give a shortcut; [`listed`] says which it lists. @@ -1496,6 +1516,38 @@ mod tests { panic!("the installed plug-ins kept changing"); } + thread_local! { + /// Run by [`snapshot`] on this thread once, after it reads the plug-in revision and before + /// it walks the menus. + pub(super) static WHILE_WALKING: std::cell::Cell>> = const { std::cell::Cell::new(None) }; + } + + /// A plug-in installed while the menus are walked doesn't leave a snapshot under the revision + /// read before the walk: the keymap built for that revision would then lack the plug-in's row. + #[test] + fn a_plugin_installed_while_the_menus_are_walked_gets_a_new_snapshot() { + use vectorcraft_plugins::wat as tpl; + let install = |id: &str| { + let manifest = format!(r#"{{"id":"{id}","name":"{id}","version":"1","kind":"filter","params":{{}}}}"#); + vectorcraft_plugins::registry::install_bytes(&wat::parse_str(tpl::returning(&manifest, r#"{"objects":[]}"#)).unwrap()).unwrap(); + }; + let (before, during) = ("test.ui.walk-before", "test.ui.walk-during"); + // A new revision, so the next snapshot walks the menus. + install(before); + WHILE_WALKING.set(Some(Box::new(move || { + // Another caller builds the snapshot and keymap of the revision read before the walk. + crate::keymap::template_for(&snapshot()); + install(during); + }))); + let s = snapshot(); + let keys: Vec<&str> = s.entries.iter().map(|e| e.key.as_str()).collect(); + let catalog = crate::keymap::template_for(&s).catalog().len(); + vectorcraft_plugins::registry::remove(before); + vectorcraft_plugins::registry::remove(during); + assert!(keys.contains(&"plugin:test.ui.walk-during"), "the walk lists the plug-in"); + assert_eq!(catalog, s.entries.len(), "the keymap of the snapshot's revision has every entry"); + } + #[test] fn format_parse_round_trip_for_every_default() { for e in entries().iter() { From 6a1354455bc0b0b26ad27a04661dca582f6dff5c Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Wed, 11 Nov 2026 18:24:10 -0800 Subject: [PATCH 2/2] Keyboard Shortcuts: the walk test runs its hook when the snapshot is already stored The test that installs a plug-in during the menu walk failed when another test thread had already stored the snapshot of the same revision: the stored snapshot was returned and the hook never ran. In tests, a set walk hook now bypasses the stored snapshot, and the test stores that snapshot itself first. --- crates/ui-egui/src/shortcut_editor.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/crates/ui-egui/src/shortcut_editor.rs b/crates/ui-egui/src/shortcut_editor.rs index 9a2344515..7ada99ee5 100644 --- a/crates/ui-egui/src/shortcut_editor.rs +++ b/crates/ui-egui/src/shortcut_editor.rs @@ -376,7 +376,17 @@ pub(crate) fn snapshot() -> Arc { static LAST: Mutex>> = Mutex::new(None); let mut revision = menus::plugin_revision(); let last = LAST.lock().unwrap_or_else(|e| e.into_inner()).clone(); - if let Some(s) = last.filter(|s| s.revision == revision) { + // A test's walk hook runs even when a snapshot of this revision is already stored. + #[cfg(test)] + let hooked = { + let f = tests::WHILE_WALKING.take(); + let hooked = f.is_some(); + tests::WHILE_WALKING.set(f); + hooked + }; + #[cfg(not(test))] + let hooked = false; + if let Some(s) = last.filter(|s| s.revision == revision && !hooked) { return s; } // When a plug-in is installed or removed during the walk, the entries may not match @@ -1532,8 +1542,9 @@ mod tests { vectorcraft_plugins::registry::install_bytes(&wat::parse_str(tpl::returning(&manifest, r#"{"objects":[]}"#)).unwrap()).unwrap(); }; let (before, during) = ("test.ui.walk-before", "test.ui.walk-during"); - // A new revision, so the next snapshot walks the menus. install(before); + // Stores the snapshot of this revision, as another test thread may already have done. + snapshot(); WHILE_WALKING.set(Some(Box::new(move || { // Another caller builds the snapshot and keymap of the revision read before the walk. crate::keymap::template_for(&snapshot());