From 20ed10da5fe2923215956812d583f4982603de91 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 14:37:29 -0500 Subject: [PATCH] Shortcuts: list each binding once and cap the conflict list (#2997) * fix(shortcuts): list each binding once and cap the conflict list * address review: compute the shortcut bindings once per keyboard draw --- crates/engine/src/settings/shortcuts.rs | 17 ++++++++++++++++- crates/engine/src/tests/settings.rs | 20 ++++++++++++++++++++ crates/ui-egui/src/panels/shortcut_editor.rs | 9 +++++---- 3 files changed, 41 insertions(+), 5 deletions(-) diff --git a/crates/engine/src/settings/shortcuts.rs b/crates/engine/src/settings/shortcuts.rs index 44e2261a2..2d3996d57 100644 --- a/crates/engine/src/settings/shortcuts.rs +++ b/crates/engine/src/settings/shortcuts.rs @@ -550,6 +550,10 @@ impl Keymaps { } } +/// Most conflicts [`ShortcutTable::all_conflicts`] lists: a preset binding every command to one +/// key would otherwise list every pair of them. +pub const MAX_CONFLICTS: usize = 10_000; + /// The active preset resolved against every bindable command. #[derive(Clone, Debug, Default)] pub struct ShortcutTable { @@ -584,8 +588,15 @@ impl ShortcutTable { } /// Every active binding: (keys, bindable). + /// Each bindable's keys once: an imported preset can repeat a binding any number of times. pub fn bindings(&self) -> Vec<(&str, &Bindable)> { - self.bindables.iter().flat_map(|b| self.keys.get(&b.key).into_iter().flatten().map(move |k| (k.as_str(), b))).collect() + self.bindables + .iter() + .flat_map(|b| { + let mut seen = std::collections::HashSet::new(); + self.keys.get(&b.key).into_iter().flatten().filter(move |k| seen.insert(k.as_str())).map(move |k| (k.as_str(), b)) + }) + .collect() } /// Other bindables using `keys` in an overlapping scope. @@ -607,6 +618,7 @@ impl ShortcutTable { /// Every pair of commands sharing keys in the same scope, or in two contexts (which can be /// active at once): only one of the two can run, or neither ([`pick`]). (Other pairs in /// different scopes are intended, see [`Conflict::overrides`].) + /// At most [`MAX_CONFLICTS`] are listed. pub fn all_conflicts(&self) -> Vec<(String, Conflict)> { let mut by_keys: BTreeMap<&str, Vec<&Bindable>> = BTreeMap::new(); for (k, b) in self.bindings() { @@ -618,6 +630,9 @@ impl ShortcutTable { for (k, bs) in by_keys { for (i, a) in bs.iter().enumerate() { for b in &bs[i + 1..] { + if out.len() >= MAX_CONFLICTS { + return out; + } if a.scope == b.scope || (is_context(&a.scope) && is_context(&b.scope)) { out.push(( a.key.clone(), diff --git a/crates/engine/src/tests/settings.rs b/crates/engine/src/tests/settings.rs index 0f8d6ffa2..42bcefc97 100644 --- a/crates/engine/src/tests/settings.rs +++ b/crates/engine/src/tests/settings.rs @@ -788,6 +788,26 @@ fn context_shortcuts_on_the_same_keys_conflict() { assert!(t.conflicts(b, "F9").iter().any(|c| c.key == "test.paint" && !c.overrides)); } +/// A preset repeating one binding many times lists it once, and conflicts stay bounded. +#[test] +fn repeated_bindings_are_one_binding() { + let mut s = Session::default(); + s.set_ui_commands(vec![ + crate::shortcuts::UiCommand { id: "test.a".into(), label: "A".into(), shortcut: Some("F9".into()), scope: Some("Timeline".into()) }, + crate::shortcuts::UiCommand { id: "test.b".into(), label: "B".into(), shortcut: Some("F9".into()), scope: Some("Timeline".into()) }, + ]); + let mut t = s.shortcuts().clone(); + t.keys.insert("test.a".into(), vec!["F9".into(); 10_000]); + assert_eq!(t.bindings().iter().filter(|(_, b)| b.key == "test.a").count(), 1); + assert_eq!(t.all_conflicts().iter().filter(|(a, c)| a == "test.a" && c.key == "test.b").count(), 1); + // Many commands on one key: the listing stops at its cap. + for i in 0..200 { + t.bindables.push(crate::shortcuts::Bindable { key: format!("test.many{i}"), ..t.find("test.b").unwrap().clone() }); + t.keys.insert(format!("test.many{i}"), vec!["F9".into()]); + } + assert_eq!(t.all_conflicts().len(), crate::shortcuts::MAX_CONFLICTS); +} + // ---------------------------------------------------------------- auto-save / recovery /// The names in `d`, sorted. diff --git a/crates/ui-egui/src/panels/shortcut_editor.rs b/crates/ui-egui/src/panels/shortcut_editor.rs index e4bf313ac..80011613d 100644 --- a/crates/ui-egui/src/panels/shortcut_editor.rs +++ b/crates/ui-egui/src/panels/shortcut_editor.rs @@ -244,11 +244,13 @@ pub fn show(app: &mut EffectcraftApp, ctx: &egui::Context, t: &Tokens) { let mods = if held.iter().any(|m| *m) && !st.recording { held } else { st.mods }; let table = app.session.shortcuts().clone(); + // Once per frame: the keyboard draw reads it for every key. + let bindings = table.bindings(); let presets = app.session.keymaps.names(); let active = app.session.keymaps.active.clone(); // key name → (app?, panel?) for the current modifiers. let mut keymap: std::collections::HashMap = Default::default(); - for (sc, b) in table.bindings() { + for (sc, b) in bindings.iter().copied() { let (m, k) = split(sc); if m == mods { let e = keymap.entry(k).or_default(); @@ -395,8 +397,7 @@ pub fn show(app: &mut EffectcraftApp, ctx: &egui::Context, t: &Tokens) { } } if !name.starts_with('#') { - let who: Vec = - table.bindings().into_iter().filter(|(sc, _)| split(sc) == (mods, name.to_string())).map(|(_, b)| b.label.clone()).collect(); + let who: Vec = bindings.iter().copied().filter(|(sc, _)| split(sc) == (mods, name.to_string())).map(|(_, b)| b.label.clone()).collect(); if !who.is_empty() { resp.on_hover_text(who.join("\n")); } @@ -428,7 +429,7 @@ pub fn show(app: &mut EffectcraftApp, ctx: &egui::Context, t: &Tokens) { p.text(info.left_top() + vec2(8.0, 6.0), Align2::LEFT_TOP, tr_args("Key: {}", &[&k]), Tokens::semibold(12.0), t.tab_text_active); let mut yy = info.min.y + 26.0; let mut list: Vec<(String, String, String)> = - table.bindings().into_iter().filter(|(sc, _)| split(sc).1 == *k).map(|(sc, b)| (sc.to_string(), b.label.clone(), b.scope.clone())).collect(); + bindings.iter().copied().filter(|(sc, _)| split(sc).1 == *k).map(|(sc, b)| (sc.to_string(), b.label.clone(), b.scope.clone())).collect(); list.sort(); if list.is_empty() { p.text(pos2(info.min.x + 8.0, yy), Align2::LEFT_TOP, tr("Not assigned"), Tokens::ui(11.5), t.text_dim);