From 4cc06748c909dced565de925c1198bff95bff911 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Mon, 17 Jul 2023 14:22:50 +0300 Subject: [PATCH] Ignore keybindings with NoAction in config overrides --- crates/gpui/src/keymap_matcher/binding.rs | 4 +- crates/gpui/src/keymap_matcher/keymap.rs | 372 +++++++++++++++++- .../gpui/src/keymap_matcher/keymap_context.rs | 2 +- crates/gpui/src/keymap_matcher/keystroke.rs | 2 +- 4 files changed, 355 insertions(+), 25 deletions(-) diff --git a/crates/gpui/src/keymap_matcher/binding.rs b/crates/gpui/src/keymap_matcher/binding.rs index 4d8334128ba5b0f419e095b173e2568207d18560..527052c85d3668d0716da84c1445880d81faca47 100644 --- a/crates/gpui/src/keymap_matcher/binding.rs +++ b/crates/gpui/src/keymap_matcher/binding.rs @@ -7,8 +7,8 @@ use super::{KeymapContext, KeymapContextPredicate, Keystroke}; pub struct Binding { action: Box, - keystrokes: SmallVec<[Keystroke; 2]>, - context_predicate: Option, + pub(super) keystrokes: SmallVec<[Keystroke; 2]>, + pub(super) context_predicate: Option, } impl std::fmt::Debug for Binding { diff --git a/crates/gpui/src/keymap_matcher/keymap.rs b/crates/gpui/src/keymap_matcher/keymap.rs index bc02ae10facb5e2d67cf07598e627f19068cb5b1..7cb95cab3a28aac3a41f7515e1551d22b1a776f5 100644 --- a/crates/gpui/src/keymap_matcher/keymap.rs +++ b/crates/gpui/src/keymap_matcher/keymap.rs @@ -1,28 +1,24 @@ +use collections::HashSet; use smallvec::SmallVec; use std::{any::TypeId, collections::HashMap}; -use super::Binding; +use crate::{Action, NoAction}; + +use super::{Binding, KeymapContextPredicate, Keystroke}; #[derive(Default)] pub struct Keymap { bindings: Vec, binding_indices_by_action_id: HashMap>, + disabled_keystrokes: HashMap, HashSet>>, } impl Keymap { - pub fn new(bindings: Vec) -> Self { - let mut binding_indices_by_action_id = HashMap::new(); - for (ix, binding) in bindings.iter().enumerate() { - binding_indices_by_action_id - .entry(binding.action().id()) - .or_insert_with(SmallVec::new) - .push(ix); - } - - Self { - binding_indices_by_action_id, - bindings, - } + #[cfg(test)] + pub(super) fn new(bindings: Vec) -> Self { + let mut this = Self::default(); + this.add_bindings(bindings); + this } pub(crate) fn bindings_for_action( @@ -35,24 +31,358 @@ impl Keymap { .unwrap_or(&[]) .iter() .map(|ix| &self.bindings[*ix]) + .filter(|binding| !self.binding_disabled(binding)) } pub(crate) fn add_bindings>(&mut self, bindings: T) { + let no_action_id = (NoAction {}).id(); + let mut new_bindings = Vec::new(); + let mut has_new_disabled_keystrokes = false; for binding in bindings { - self.binding_indices_by_action_id - .entry(binding.action().id()) - .or_default() - .push(self.bindings.len()); - self.bindings.push(binding); + if binding.action().id() == no_action_id { + has_new_disabled_keystrokes |= self + .disabled_keystrokes + .entry(binding.keystrokes) + .or_default() + .insert(binding.context_predicate); + } else { + new_bindings.push(binding); + } + } + + if has_new_disabled_keystrokes { + self.binding_indices_by_action_id.retain(|_, indices| { + indices.retain(|ix| { + let binding = &self.bindings[*ix]; + match self.disabled_keystrokes.get(&binding.keystrokes) { + Some(disabled_predicates) => { + !disabled_predicates.contains(&binding.context_predicate) + } + None => true, + } + }); + !indices.is_empty() + }); + } + + for new_binding in new_bindings { + if !self.binding_disabled(&new_binding) { + self.binding_indices_by_action_id + .entry(new_binding.action().id()) + .or_default() + .push(self.bindings.len()); + self.bindings.push(new_binding); + } } } pub(crate) fn clear(&mut self) { self.bindings.clear(); self.binding_indices_by_action_id.clear(); + self.disabled_keystrokes.clear(); + } + + pub fn bindings(&self) -> Vec<&Binding> { + self.bindings + .iter() + .filter(|binding| !self.binding_disabled(binding)) + .collect() + } + + fn binding_disabled(&self, binding: &Binding) -> bool { + match self.disabled_keystrokes.get(&binding.keystrokes) { + Some(disabled_predicates) => disabled_predicates.contains(&binding.context_predicate), + None => false, + } + } +} + +#[cfg(test)] +mod tests { + use crate::actions; + + use super::*; + + actions!( + keymap_test, + [Present1, Present2, Present3, Duplicate, Missing] + ); + + #[test] + fn regular_keymap() { + let present_1 = Binding::new("ctrl-q", Present1 {}, None); + let present_2 = Binding::new("ctrl-w", Present2 {}, Some("pane")); + let present_3 = Binding::new("ctrl-e", Present3 {}, Some("editor")); + let keystroke_duplicate_to_1 = Binding::new("ctrl-q", Duplicate {}, None); + let full_duplicate_to_2 = Binding::new("ctrl-w", Present2 {}, Some("pane")); + let missing = Binding::new("ctrl-r", Missing {}, None); + let all_bindings = [ + &present_1, + &present_2, + &present_3, + &keystroke_duplicate_to_1, + &full_duplicate_to_2, + &missing, + ]; + + let mut keymap = Keymap::default(); + assert_absent(&keymap, &all_bindings); + assert!(keymap.bindings().is_empty()); + + keymap.add_bindings([present_1.clone(), present_2.clone(), present_3.clone()]); + assert_absent(&keymap, &[&keystroke_duplicate_to_1, &missing]); + assert_present( + &keymap, + &[(&present_1, "q"), (&present_2, "w"), (&present_3, "e")], + ); + + keymap.add_bindings([ + keystroke_duplicate_to_1.clone(), + full_duplicate_to_2.clone(), + ]); + assert_absent(&keymap, &[&missing]); + assert!( + !keymap.binding_disabled(&keystroke_duplicate_to_1), + "Duplicate binding 1 was added and should not be disabled" + ); + assert!( + !keymap.binding_disabled(&full_duplicate_to_2), + "Duplicate binding 2 was added and should not be disabled" + ); + + assert_eq!( + keymap + .bindings_for_action(keystroke_duplicate_to_1.action().id()) + .map(|binding| &binding.keystrokes) + .flatten() + .collect::>(), + vec![&Keystroke { + ctrl: true, + alt: false, + shift: false, + cmd: false, + function: false, + key: "q".to_string() + }], + "{keystroke_duplicate_to_1:?} should have the expected keystroke in the keymap" + ); + assert_eq!( + keymap + .bindings_for_action(full_duplicate_to_2.action().id()) + .map(|binding| &binding.keystrokes) + .flatten() + .collect::>(), + vec![ + &Keystroke { + ctrl: true, + alt: false, + shift: false, + cmd: false, + function: false, + key: "w".to_string() + }, + &Keystroke { + ctrl: true, + alt: false, + shift: false, + cmd: false, + function: false, + key: "w".to_string() + } + ], + "{full_duplicate_to_2:?} should have a duplicated keystroke in the keymap" + ); + + let updated_bindings = keymap.bindings(); + let expected_updated_bindings = vec![ + &present_1, + &present_2, + &present_3, + &keystroke_duplicate_to_1, + &full_duplicate_to_2, + ]; + assert_eq!( + updated_bindings.len(), + expected_updated_bindings.len(), + "Unexpected updated keymap bindings {updated_bindings:?}" + ); + for (i, expected) in expected_updated_bindings.iter().enumerate() { + let keymap_binding = &updated_bindings[i]; + assert_eq!( + keymap_binding.context_predicate, expected.context_predicate, + "Unexpected context predicate for keymap {i} element: {keymap_binding:?}" + ); + assert_eq!( + keymap_binding.keystrokes, expected.keystrokes, + "Unexpected keystrokes for keymap {i} element: {keymap_binding:?}" + ); + } + + keymap.clear(); + assert_absent(&keymap, &all_bindings); + assert!(keymap.bindings().is_empty()); } - pub fn bindings(&self) -> &Vec { - &self.bindings + #[test] + fn keymap_with_ignored() { + let present_1 = Binding::new("ctrl-q", Present1 {}, None); + let present_2 = Binding::new("ctrl-w", Present2 {}, Some("pane")); + let present_3 = Binding::new("ctrl-e", Present3 {}, Some("editor")); + let keystroke_duplicate_to_1 = Binding::new("ctrl-q", Duplicate {}, None); + let full_duplicate_to_2 = Binding::new("ctrl-w", Present2 {}, Some("pane")); + let ignored_1 = Binding::new("ctrl-q", NoAction {}, None); + let ignored_2 = Binding::new("ctrl-w", NoAction {}, Some("pane")); + let ignored_3_with_other_context = + Binding::new("ctrl-e", NoAction {}, Some("other_context")); + + let mut keymap = Keymap::default(); + + keymap.add_bindings([ + ignored_1.clone(), + ignored_2.clone(), + ignored_3_with_other_context.clone(), + ]); + assert_absent(&keymap, &[&present_3]); + assert_disabled( + &keymap, + &[ + &present_1, + &present_2, + &ignored_1, + &ignored_2, + &ignored_3_with_other_context, + ], + ); + assert!(keymap.bindings().is_empty()); + keymap.clear(); + + keymap.add_bindings([ + present_1.clone(), + present_2.clone(), + present_3.clone(), + ignored_1.clone(), + ignored_2.clone(), + ignored_3_with_other_context.clone(), + ]); + assert_present(&keymap, &[(&present_3, "e")]); + assert_disabled( + &keymap, + &[ + &present_1, + &present_2, + &ignored_1, + &ignored_2, + &ignored_3_with_other_context, + ], + ); + keymap.clear(); + + keymap.add_bindings([ + present_1.clone(), + present_2.clone(), + present_3.clone(), + ignored_1.clone(), + ]); + assert_present(&keymap, &[(&present_2, "w"), (&present_3, "e")]); + assert_disabled(&keymap, &[&present_1, &ignored_1]); + assert_absent(&keymap, &[&ignored_2, &ignored_3_with_other_context]); + keymap.clear(); + + keymap.add_bindings([ + present_1.clone(), + present_2.clone(), + present_3.clone(), + keystroke_duplicate_to_1.clone(), + full_duplicate_to_2.clone(), + ignored_1.clone(), + ignored_2.clone(), + ignored_3_with_other_context.clone(), + ]); + assert_present(&keymap, &[(&present_3, "e")]); + assert_disabled( + &keymap, + &[ + &present_1, + &present_2, + &keystroke_duplicate_to_1, + &full_duplicate_to_2, + &ignored_1, + &ignored_2, + &ignored_3_with_other_context, + ], + ); + keymap.clear(); + } + + #[track_caller] + fn assert_present(keymap: &Keymap, expected_bindings: &[(&Binding, &str)]) { + let keymap_bindings = keymap.bindings(); + assert_eq!( + expected_bindings.len(), + keymap_bindings.len(), + "Unexpected keymap bindings {keymap_bindings:?}" + ); + for (i, (expected, expected_key)) in expected_bindings.iter().enumerate() { + assert!( + !keymap.binding_disabled(expected), + "{expected:?} should not be disabled as it was added into keymap for element {i}" + ); + assert_eq!( + keymap + .bindings_for_action(expected.action().id()) + .map(|binding| &binding.keystrokes) + .flatten() + .collect::>(), + vec![&Keystroke { + ctrl: true, + alt: false, + shift: false, + cmd: false, + function: false, + key: expected_key.to_string() + }], + "{expected:?} should have the expected keystroke with key '{expected_key}' in the keymap for element {i}" + ); + + let keymap_binding = &keymap_bindings[i]; + assert_eq!( + keymap_binding.context_predicate, expected.context_predicate, + "Unexpected context predicate for keymap {i} element: {keymap_binding:?}" + ); + assert_eq!( + keymap_binding.keystrokes, expected.keystrokes, + "Unexpected keystrokes for keymap {i} element: {keymap_binding:?}" + ); + } + } + + #[track_caller] + fn assert_absent(keymap: &Keymap, bindings: &[&Binding]) { + for binding in bindings.iter() { + assert!( + !keymap.binding_disabled(binding), + "{binding:?} should not be disabled in the keymap where was not added" + ); + assert_eq!( + keymap.bindings_for_action(binding.action().id()).count(), + 0, + "{binding:?} should have no actions in the keymap where was not added" + ); + } + } + + #[track_caller] + fn assert_disabled(keymap: &Keymap, bindings: &[&Binding]) { + for binding in bindings.iter() { + assert!( + keymap.binding_disabled(binding), + "{binding:?} should be disabled in the keymap" + ); + assert_eq!( + keymap.bindings_for_action(binding.action().id()).count(), + 0, + "{binding:?} should have no actions in the keymap where it was disabled" + ); + } } } diff --git a/crates/gpui/src/keymap_matcher/keymap_context.rs b/crates/gpui/src/keymap_matcher/keymap_context.rs index be61fea531656fb5a3163ba2804dfce1b4fb0e63..fd60a8f4b5d385eb94b7edf0bfeb407a9dce8c20 100644 --- a/crates/gpui/src/keymap_matcher/keymap_context.rs +++ b/crates/gpui/src/keymap_matcher/keymap_context.rs @@ -44,7 +44,7 @@ impl KeymapContext { } } -#[derive(Clone, Debug, Eq, PartialEq)] +#[derive(Clone, Debug, Eq, PartialEq, Hash)] pub enum KeymapContextPredicate { Identifier(String), Equal(String, String), diff --git a/crates/gpui/src/keymap_matcher/keystroke.rs b/crates/gpui/src/keymap_matcher/keystroke.rs index ed3c3f69140f2da02268c5d44ac9ecd77d813761..164dea8aba145aec263dd1c95afbbb79824bb83c 100644 --- a/crates/gpui/src/keymap_matcher/keystroke.rs +++ b/crates/gpui/src/keymap_matcher/keystroke.rs @@ -3,7 +3,7 @@ use std::fmt::Write; use anyhow::anyhow; use serde::Deserialize; -#[derive(Clone, Debug, Eq, PartialEq, Default, Deserialize)] +#[derive(Clone, Debug, Eq, PartialEq, Default, Deserialize, Hash)] pub struct Keystroke { pub ctrl: bool, pub alt: bool,