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
This commit is contained in:
Dylan M. Taylor authored and GitHub committed 2026-11-11 14:37:29 -05:00
1 parent 82ea2e7c3e
commit 20ed10da5f
3 files changed
+41 -5

No files matched your search

+16 -1
View File
@@ -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(),
+20
View File
@@ -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.
+5 -4
View File
@@ -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<String, (bool, bool)> = 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<String> =
table.bindings().into_iter().filter(|(sc, _)| split(sc) == (mods, name.to_string())).map(|(_, b)| b.label.clone()).collect();
let who: Vec<String> = 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);