Keyboard Shortcuts: lists made for removed plug-in commands are freed

The Keyboard Shortcuts entries and the default keymap were made again
for each set of installed plug-in commands and kept for the rest of the
process. They are now shared values kept only for the current set of
plug-in commands, so an older one is freed once nothing holds it.

The test that compares the keymap's catalog with the entry list reads
both for one set of plug-in commands, so a test that installs a plug-in
alongside it no longer makes it fail.

crates/ui-egui/src/dialogs/shortcut_import.rs

@@ -377,12 +377,12 @@ pub fn candidates() -> Vec<Candidate> {
 
 /// Keyboard Shortcuts' entry `key`. The commands of installed plug-ins are looked up in
 /// [`shortcut_editor::entries`] as it is now.
-fn entry(key: &str) -> Option<&'static Entry> {
-    static BY_KEY: OnceLock<HashMap<&'static str, &'static Entry>> = OnceLock::new();
+fn entry(key: &str) -> Option<Entry> {
+    static BY_KEY: OnceLock<HashMap<String, Entry>> = OnceLock::new();
     if vectorcraft_engine::cmd::plugin::is_command_id(key) {
-        return shortcut_editor::entries().iter().find(|e| e.key == key);
+        return shortcut_editor::entry(key);
     }
-    BY_KEY.get_or_init(|| shortcut_editor::entries().iter().map(|e| (e.key.as_str(), e)).collect()).get(key).copied()
+    BY_KEY.get_or_init(|| shortcut_editor::entries().iter().map(|e| (e.key.clone(), e.clone())).collect()).get(key).cloned()
 }
 
 /// A tool, command or panel that Keyboard Shortcuts can give a shortcut.
@@ -400,7 +400,7 @@ fn target_label(target: Option<&str>) -> String {
     match target {
         None => tl!("Choose…").to_string(),
         Some("") => tl!("No equivalent").to_string(),
-        Some(t) => entry(t).map_or_else(|| t.to_string(), shown),
+        Some(t) => entry(t).map_or_else(|| t.to_string(), |e| shown(&e)),
     }
 }
 
@@ -948,22 +948,27 @@ pub(super) fn target_picker(ui: &mut egui::Ui, i: usize, r: &Row) -> Option<Stri
                 }
                 let q = widgets::search_field(ui, field, tl!("Search")).to_lowercase();
                 let mut picked = option(ui, "", tl!("No equivalent"));
-                let list: Vec<&Entry> = if q.is_empty() {
+                let all = shortcut_editor::entries();
+                let list: Vec<usize> = if q.is_empty() {
                     for e in r.choices.iter().filter_map(|c| entry(c)) {
-                        picked = option(ui, &e.key, &shown(e)).or(picked);
+                        picked = option(ui, &e.key, &shown(&e)).or(picked);
                     }
                     ui.separator();
-                    shortcut_editor::entries().iter().collect()
+                    (0..all.len()).collect()
                 } else {
-                    shortcut_editor::entries().iter().filter(|e| shown(e).to_lowercase().contains(&q) || e.key.to_lowercase().contains(&q)).collect()
+                    all.iter()
+                        .enumerate()
+                        .filter(|(_, e)| shown(e).to_lowercase().contains(&q) || e.key.to_lowercase().contains(&q))
+                        .map(|(i, _)| i)
+                        .collect()
                 };
                 ui.data_mut(|d| d.insert_temp(header_id, ui.min_rect().height()));
-                (list, picked)
+                (all, list, picked)
             },
             ROW_H,
-            |(list, _)| list.len(),
-            |ui, range, (list, mut picked)| {
-                for e in range.filter_map(|k| list.get(k)) {
+            |(_, list, _)| list.len(),
+            |ui, range, (all, list, mut picked)| {
+                for e in range.filter_map(|k| list.get(k).and_then(|&i| all.get(i))) {
                     picked = option(ui, &e.key, &shown(e)).or(picked);
                 }
                 if picked.is_some() {

crates/ui-egui/src/keymap.rs

@@ -7,7 +7,7 @@
 //! outside it are VectorCraft's too (`Cmd+Shift+]`, `Comma`, `Period`, `~` for the backtick key,
 //! `=` for Plus with modifiers). Inside an [`Overrides`] keys and texts are the crate's.
 
-use std::sync::{Mutex, OnceLock, RwLock};
+use std::sync::{Arc, Mutex, OnceLock, RwLock};
 
 use craft_keymap::{Bindable, Binding, Chord, CommandRef, Key, Keymap, Kind, Lookup, Mods, Overrides, Platform, Scope, Scopes};
 use serde_json::{Value, json};
@@ -142,7 +142,8 @@ fn catalogue() -> Vec<Bindable> {
             0
         }
     };
-    let mut entries: Vec<&Entry> = shortcut_editor::entries().iter().collect();
+    let all = shortcut_editor::entries();
+    let mut entries: Vec<&Entry> = all.iter().collect();
     entries.sort_by_key(|e| rank(e));
     entries
         .into_iter()
@@ -212,25 +213,43 @@ pub(crate) fn per_platform<T>(cells: &'static [OnceLock<T>; 3], p: Platform, mak
     cell.get_or_init(make)
 }
 
-/// The keymap with the default shortcuts for [`platform`], built once per platform, and again for
-/// each set of plug-in commands installed ([`vectorcraft_engine::cmd::plugin::commands_generation`]).
-pub fn template() -> &'static Keymap {
-    static TEMPLATES: [OnceLock<Keymap>; 3] = [OnceLock::new(), OnceLock::new(), OnceLock::new()];
-    static WITH_PLUGINS: Mutex<Vec<(Platform, u64, &'static Keymap)>> = Mutex::new(Vec::new());
+/// Values made for one set of installed plug-in commands
+/// ([`vectorcraft_engine::cmd::plugin::commands_generation`]), one for each key. Only the values of
+/// the generation asked for last are kept; an older one is freed once its last holder drops it.
+pub(crate) struct Latest<K, T: ?Sized> {
+    made: Mutex<Vec<(K, u64, Arc<T>)>>,
+}
+
+impl<K: PartialEq, T: ?Sized> Latest<K, T> {
+    pub(crate) const fn new() -> Self {
+        Latest { made: Mutex::new(Vec::new()) }
+    }
+
+    /// The value for `key` at `generation`, made with `make` when there is none yet.
+    pub(crate) fn get(&self, key: K, generation: u64, make: impl FnOnce() -> Arc<T>) -> Arc<T> {
+        let mut made = self.made.lock().unwrap_or_else(|e| e.into_inner());
+        if let Some((_, _, v)) = made.iter().find(|(k, g, _)| *k == key && *g == generation) {
+            return v.clone();
+        }
+        made.retain(|(_, g, _)| *g == generation);
+        let v = make();
+        made.push((key, generation, v.clone()));
+        v
+    }
+}
+
+/// The keymap with the default shortcuts for [`platform`], built once per platform, and again when
+/// the installed plug-in commands change ([`vectorcraft_engine::cmd::plugin::commands_generation`]).
+/// Only the keymaps of the current plug-in commands are kept ([`Latest`]).
+pub fn template() -> Arc<Keymap> {
+    static TEMPLATES: [OnceLock<Arc<Keymap>>; 3] = [OnceLock::new(), OnceLock::new(), OnceLock::new()];
+    static WITH_PLUGINS: Latest<Platform, Keymap> = Latest::new();
     let p = platform();
     let generation = vectorcraft_engine::cmd::plugin::commands_generation();
     if generation == 0 {
-        return per_platform(&TEMPLATES, p, || keymap_for(p));
+        return per_platform(&TEMPLATES, p, || Arc::new(keymap_for(p))).clone();
     }
-    let mut made = WITH_PLUGINS.lock().unwrap_or_else(|e| e.into_inner());
-    if let Some((_, _, km)) = made.iter().find(|(q, g, _)| *q == p && *g == generation) {
-        return km;
-    }
-    made.retain(|(_, g, _)| *g == generation);
-    // Kept for the rest of the process: one keymap for each set of plug-in commands installed.
-    let km: &'static Keymap = Box::leak(Box::new(keymap_for(p)));
-    made.push((p, generation, km));
-    km
+    WITH_PLUGINS.get(p, generation, || Arc::new(keymap_for(p)))
 }
 
 /// The default keymap of `p`. The chords the operating system takes can't be assigned.
@@ -240,7 +259,7 @@ fn keymap_for(p: Platform) -> Keymap {
 
 /// The keymap with `overrides` applied.
 pub fn keymap_with(overrides: &Overrides) -> Keymap {
-    template().clone().with_overrides(overrides.clone())
+    Keymap::clone(&template()).with_overrides(overrides.clone())
 }
 
 /// The command a chord runs, the first for tools sharing it.
@@ -409,7 +428,8 @@ mod tests {
 
     #[test]
     fn curvature_keeps_shift_tilde_and_plus_is_one_key() {
-        let km = template();
+        let template = template();
+        let km = &*template;
         let curvature = vc_chord("Shift+~").unwrap();
         assert_eq!(curvature, Chord::parse("Shift+`").unwrap());
         assert_eq!(lookup(km, &curvature), Some(CommandRef::new("tool:curvature")));
@@ -431,9 +451,17 @@ mod tests {
     fn the_default_shortcuts_do_not_conflict() {
         assert_eq!(template().conflicts(), vec![]);
         assert_eq!(template().problems(), vec![]);
-        assert_eq!(template().catalog().len(), shortcut_editor::entries().len(), "every entry is bindable");
+        // Both lists for one set of plug-in commands: a test running alongside may install one.
+        let (km, entries) = loop {
+            let before = vectorcraft_engine::cmd::plugin::commands_generation();
+            let both = (template(), shortcut_editor::entries());
+            if vectorcraft_engine::cmd::plugin::commands_generation() == before {
+                break both;
+            }
+        };
+        assert_eq!(km.catalog().len(), entries.len(), "every entry is bindable");
         // A default the bridge can't read would show in the menus but never run.
-        let unbound: Vec<(&str, &str)> = shortcut_editor::entries()
+        let unbound: Vec<(&str, &str)> = entries
             .iter()
             .filter_map(|e| shortcut_editor::default_of(&e.key).map(|d| (e.key.as_str(), d)))
             .filter(|(key, d)| {
@@ -445,6 +473,22 @@ mod tests {
         assert_eq!(unbound, vec![], "every default text becomes the entry's binding");
     }
 
+    /// A value made for an older set of plug-in commands is freed once a newer one is asked for;
+    /// within one set each key keeps its own value, made once.
+    #[test]
+    fn only_the_latest_plugin_commands_are_kept() {
+        let cache: Latest<u8, str> = Latest::new();
+        let first = cache.get(0, 1, || Arc::from("a"));
+        assert!(Arc::ptr_eq(&first, &cache.get(0, 1, || Arc::from("b"))), "made once");
+        let other = Arc::downgrade(&cache.get(1, 1, || Arc::from("c")));
+        assert!(other.upgrade().is_some(), "each key has its own");
+        let old = Arc::downgrade(&first);
+        drop(first);
+        assert_eq!(&*cache.get(0, 2, || Arc::from("d")), "d");
+        assert!(old.upgrade().is_none(), "the older value is freed");
+        assert!(other.upgrade().is_none(), "the older value of the other key is freed");
+    }
+
     #[test]
     fn paint_defaults_have_two_bindings_and_reset_restores_both() {
         for (key, plain, shifted) in [("paint.lastColor", ",", "Shift+,"), ("paint.lastGradient", ".", "Shift+.")] {
@@ -452,10 +496,10 @@ mod tests {
             let defaults = template().default_bindings(&command);
             assert_eq!(defaults, vec![vc_binding(plain).unwrap(), vc_binding(shifted).unwrap()], "{key}");
             assert_eq!(
-                shortcut_editor::entry(key).map(|e| e.label.as_str()),
-                Some(if key == "paint.lastColor" { "Apply Last Color" } else { "Apply Last Gradient" })
+                shortcut_editor::entry(key).map(|e| e.label),
+                Some(if key == "paint.lastColor" { "Apply Last Color" } else { "Apply Last Gradient" }.to_string())
             );
-            assert_eq!(effective_with(template(), key).as_deref(), Some(plain));
+            assert_eq!(effective_with(&template(), key).as_deref(), Some(plain));
             let mut overrides = Overrides::new();
             set_override(&mut overrides, key, "Shift+;");
             assert_eq!(keymap_with(&overrides).bindings(&command), vec![vc_binding("Shift+;").unwrap()]);
@@ -494,7 +538,8 @@ mod tests {
         assert_eq!(entry_key(&c), "panel:layers");
         assert_eq!(entry_of_stored(&stored_key("panel:layers")), "panel:layers");
         assert_eq!(stored_key("tool:pen"), "tool:pen");
-        let paths = &template().bindable(&c).unwrap().paths;
+        let template = template();
+        let paths = &template.bindable(&c).unwrap().paths;
         assert_eq!(paths, &vec![vec!["Window".to_string(), "Layers".to_string()]], "import matches panels by name");
     }
 
@@ -506,8 +551,7 @@ mod tests {
         for id in TOGGLES {
             let label = crate::menus::dynamic_label(&app, id, "");
             assert!(label.starts_with("Show ") || label.starts_with("Hide "), "{id}: {label}");
-            let listed = template().bindable(&CommandRef::new(*id));
-            assert!(listed.is_none_or(|b| b.is_toggle), "{id}");
+            assert!(template().bindable(&CommandRef::new(*id)).is_none_or(|b| b.is_toggle), "{id}");
         }
     }
 }

crates/ui-egui/src/shortcut_editor.rs

@@ -73,7 +73,7 @@ pub(crate) struct Mirror {
 impl Mirror {
     fn new(overrides: Overrides) -> Mirror {
         let plugins = vectorcraft_engine::cmd::plugin::commands_generation();
-        let keymap = crate::keymap::template().clone().with_overrides(overrides);
+        let keymap = Keymap::clone(&crate::keymap::template()).with_overrides(overrides);
         let text = keymap
             .overrides()
             .iter()
@@ -360,36 +360,30 @@ pub struct Entry {
 
 /// Every tool, command and panel the editor can give a shortcut, followed by the commands of the
 /// installed plug-ins; [`listed`] says which it lists. The list is made again when the plug-in
-/// commands change ([`vectorcraft_engine::cmd::plugin::commands_generation`]).
-pub fn entries() -> &'static [Entry] {
-    static WITH_PLUGINS: Mutex<Option<(u64, &'static [Entry])>> = Mutex::new(None);
+/// commands change ([`vectorcraft_engine::cmd::plugin::commands_generation`]). Only the list of the
+/// current plug-in commands is kept ([`crate::keymap::Latest`]).
+pub fn entries() -> Arc<[Entry]> {
+    static WITH_PLUGINS: crate::keymap::Latest<(), [Entry]> = crate::keymap::Latest::new();
     let base = built_in_entries();
     let generation = vectorcraft_engine::cmd::plugin::commands_generation();
     if generation == 0 {
-        return base;
+        return base.clone();
     }
-    let mut with = WITH_PLUGINS.lock().unwrap_or_else(|e| e.into_inner());
-    if let Some((g, e)) = *with
-        && g == generation
-    {
-        return e;
-    }
-    let mut v = base.to_vec();
-    v.extend(vectorcraft_engine::cmd::plugin::command_specs().into_iter().map(|c| Entry {
-        key: c.id.into(),
-        label: c.label.into(),
-        group: c.menu.join(" › "),
-        is_tool: false,
-    }));
-    // Kept for the rest of the process: one list for each set of plug-in commands installed.
-    let e: &'static [Entry] = Box::leak(v.into_boxed_slice());
-    *with = Some((generation, e));
-    e
+    WITH_PLUGINS.get((), generation, || {
+        let mut v = base.to_vec();
+        v.extend(vectorcraft_engine::cmd::plugin::command_specs().into_iter().map(|c| Entry {
+            key: c.id.into(),
+            label: c.label.into(),
+            group: c.menu.join(" › "),
+            is_tool: false,
+        }));
+        v.into()
+    })
 }
 
 /// [`entries`] without the plug-in commands.
-fn built_in_entries() -> &'static [Entry] {
-    static E: OnceLock<Vec<Entry>> = OnceLock::new();
+fn built_in_entries() -> &'static Arc<[Entry]> {
+    static E: OnceLock<Arc<[Entry]>> = OnceLock::new();
     E.get_or_init(|| {
         let mut v: Vec<Entry> = vectorcraft_tools::catalog::all_tools()
             .map(|t| Entry { key: format!("tool:{}", t.id), label: t.label.to_string(), group: "Tools".into(), is_tool: true })
@@ -430,9 +424,8 @@ fn built_in_entries() -> &'static [Entry] {
             }
             v.push(Entry { key: id.to_string(), label: label.to_string(), group: id_group(id), is_tool: false });
         }
-        v
+        v.into()
     })
-    .as_slice()
 }
 
 /// One Effect menu item a shortcut can be given (#3296): the effect's id, the command its item runs
@@ -621,8 +614,8 @@ fn names_a_required_field(params: &str) -> bool {
     false
 }
 
-pub fn entry(key: &str) -> Option<&'static Entry> {
-    entries().iter().find(|e| e.key == key)
+pub fn entry(key: &str) -> Option<Entry> {
+    entries().iter().find(|e| e.key == key).cloned()
 }
 
 /// Entries (other than `except`) whose effective shortcut equals `chord`.
@@ -1217,7 +1210,7 @@ fn export_from_dialog(app: &mut VectorcraftApp) {
 fn drop_hidden(d: &mut Dialog, overrides: &Overrides) {
     for field in ["__recording", "__selected", "__conflict"] {
         let key = d.str(field);
-        if !key.is_empty() && entry(&key).is_none_or(|e| !listed(overrides, e)) {
+        if !key.is_empty() && entry(&key).is_none_or(|e| !listed(overrides, &e)) {
             d.fields.insert(field.into(), json!(""));
         }
     }
@@ -1259,7 +1252,7 @@ mod tests {
 
     #[test]
     fn format_parse_round_trip_for_every_default() {
-        for e in entries() {
+        for e in entries().iter() {
             if let Some(s) = default_of(&e.key)
                 && let Some(sc) = crate::shortcuts::parse(s)
             {
@@ -1574,7 +1567,7 @@ mod tests {
     #[test]
     fn duplicate_can_be_given_a_shortcut() {
         // Duplicate has no default chord (#1539): it must still be listed so one can be set.
-        assert_eq!(entry("edit.duplicate").map(|e| e.group.as_str()), Some("Edit"));
+        assert_eq!(entry("edit.duplicate").map(|e| e.group).as_deref(), Some("Edit"));
         let mut ov = Overrides::new();
         assign(&mut ov, "edit.duplicate", Some("Cmd+Alt+Shift+F8"), false).unwrap();
         assert_eq!(effective_in(&ov, "edit.duplicate").as_deref(), Some("Cmd+Alt+Shift+F8"));
@@ -1647,10 +1640,11 @@ mod tests {
         // whose params are all optional, one the menu bar runs without params (Rotate… opens its
         // dialog) and one that opens a dialog for its values (New Variable).
         let none = Overrides::new();
-        let dropped: Vec<&str> = entries().iter().filter(|e| default_of(&e.key).is_some() && !listed(&none, e)).map(|e| e.key.as_str()).collect();
+        let all = entries();
+        let dropped: Vec<&str> = all.iter().filter(|e| default_of(&e.key).is_some() && !listed(&none, e)).map(|e| e.key.as_str()).collect();
         assert!(dropped.is_empty(), "{dropped:?}");
         for id in ["edit.duplicate", "object.pathfinder.unite", "object.rotate", "variable.define", "object.distributeSpacing"] {
-            assert!(entry(id).is_some_and(|e| listed(&none, e)), "{id}");
+            assert!(entry(id).is_some_and(|e| listed(&none, &e)), "{id}");
         }
         for (doc, required) in [
             ("{}", false),
@@ -1679,12 +1673,12 @@ mod tests {
         let others = ["swatch.unused", "graphicStyle.unused", "document.info", "view.proofSetup", "magicWand.set", "edit.findNext"];
         for id in NEEDS_A_PARAM.iter().chain(&others) {
             let e = entry(id).unwrap_or_else(|| panic!("{id} has an entry"));
-            assert!(!listed(&none, e), "{id}");
+            assert!(!listed(&none, &e), "{id}");
             assert_eq!(default_of(id), None, "{id}");
         }
         let mut ov = Overrides::new();
         crate::keymap::set_override(&mut ov, "transparency.set", "Cmd+Alt+Shift+F10");
-        assert!(entry("transparency.set").is_some_and(|e| listed(&ov, e)));
+        assert!(entry("transparency.set").is_some_and(|e| listed(&ov, &e)));
         let mut app = VectorcraftApp::new(vectorcraft_engine::Session::new(), Default::default());
         for id in ["transparency.set", "transparency.setOpacityMask"] {
             let list = run_command(&mut app, "shortcuts.list", &json!({ "query": id })).unwrap().unwrap();
@@ -1699,7 +1693,7 @@ mod tests {
             "addon.color.printFixes",
             "edit.findReplace",
         ] {
-            assert!(entry(id).is_some_and(|e| listed(&none, e)), "{id}");
+            assert!(entry(id).is_some_and(|e| listed(&none, &e)), "{id}");
         }
         assert_eq!(menu_target("object.path.cleanUp").map(|(id, _)| id).as_deref(), Some("ui.paramDialog"));
         assert_eq!(

crates/ui-egui/src/shortcuts/tests.rs

@@ -510,7 +510,7 @@ fn cmd_alt_k_and_cmd_alt_shift_f_highlight_the_character_fields() {
     assert!(app.ui.dialog.is_none(), "not Preferences");
     // Keyboard Shortcuts lists all three under Type; Highlight Tracking has no default key.
     for id in ["type.highlightFontName", "type.highlightKerning", "type.highlightTracking"] {
-        assert_eq!(crate::shortcut_editor::entry(id).map(|e| e.group.as_str()), Some("Type"), "{id}");
+        assert_eq!(crate::shortcut_editor::entry(id).map(|e| e.group).as_deref(), Some("Type"), "{id}");
     }
     assert_eq!(crate::menus::shortcut_of("type.highlightTracking"), None);
 }
@@ -2347,7 +2347,7 @@ fn all_registered_shortcuts_parse() {
 /// dialog for the selection.
 #[test]
 fn an_effect_menu_item_takes_a_shortcut_of_its_own() {
-    let entry = crate::shortcut_editor::entries().iter().find(|e| e.key == "effect:distort.transform").expect("Transform is listed");
+    let entry = crate::shortcut_editor::entry("effect:distort.transform").expect("Transform is listed");
     assert_eq!(entry.group, "Effect › Distort & Transform");
     assert_eq!(crate::keymap::command_of("effect:distort.transform").id, "effect.dialog");
     let mut app = VectorcraftApp::new(vectorcraft_engine::Session::new(), Default::default());

crates/ui-egui/src/tests/plugins.rs

@@ -220,7 +220,7 @@ fn plugin_commands_are_menu_items_and_shortcut_entries() {
     assert!(!entries.iter().any(|e| e.command.as_deref() == Some(size) && e.path.first().map(String::as_str) != Some("Object")), "not in File");
     assert!(!menus::menu_strings().contains("Cmd Test"), "the plug-in's name is not text to translate");
 
-    let listed = crate::shortcut_editor::entries().iter().find(|e| e.key == grow).expect("in Keyboard Shortcuts");
+    let listed = crate::shortcut_editor::entry(grow).expect("in Keyboard Shortcuts");
     assert_eq!((listed.label.as_str(), listed.group.as_str()), ("Grow", "Object › Plug-ins › Cmd Test"));
     assert_eq!(crate::shortcut_editor::default_of(grow), None, "a manifest gives no shortcut");
     assert!(crate::keymap::template().bindable(&crate::keymap::command_of(grow)).is_some(), "a shortcut can be given to it");
@@ -238,6 +238,7 @@ fn plugin_commands_are_menu_items_and_shortcut_entries() {
     app.run("plugin.remove", json!({"id": id})).unwrap();
     assert!(menus::menu_entries(&app).iter().all(|e| e.command.as_deref() != Some(grow)));
     assert!(crate::shortcut_editor::entries().iter().all(|e| e.key != grow));
+    assert!(crate::keymap::template().bindable(&crate::keymap::command_of(grow)).is_none());
 }
 
 /// A shortcut the user gives a plug-in command runs it.