From f5c2e155939fb988ec3e8eded5017f64b800f2e6 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Fri, 5 Jan 2024 13:51:47 -0800 Subject: [PATCH] Restore the ability to disable key bindings by setting them to `null` in your keymap (#3921) * Fix an incorrect use of `Any::type_id` that prevented the disabling of key bindings * Restructured the representation of disabled key bindings so that they handle context predicates correctly. Previously, to disable key binding, you needed to supply the exact same context predicate (e.g. `Editor && mode == "full"`) as the binding that you are trying to disable. Now, the context predicates of disabled key bindings are evaluated just like any other context predicate (with the current context) to see if they apply. Release Notes: - Fixed an issue where disabling key bindings didn't work. To disable a key binding, set it to `null` in your keymap. --- crates/gpui/src/key_dispatch.rs | 14 +- crates/gpui/src/keymap/binding.rs | 41 +-- crates/gpui/src/keymap/keymap.rs | 448 ++++++----------------- crates/gpui/src/keymap/matcher.rs | 22 +- crates/gpui/src/platform/mac/platform.rs | 4 +- 5 files changed, 136 insertions(+), 393 deletions(-) diff --git a/crates/gpui/src/key_dispatch.rs b/crates/gpui/src/key_dispatch.rs index ade52a13143a8ba8ea6ecaa41434f53b721262a0..22c4dffc03a78df8fde5530a3059887e91a2b876 100644 --- a/crates/gpui/src/key_dispatch.rs +++ b/crates/gpui/src/key_dispatch.rs @@ -188,15 +188,13 @@ impl DispatchTree { action: &dyn Action, context_stack: &Vec, ) -> Vec { - self.keymap - .lock() - .bindings_for_action(action.type_id()) - .filter(|candidate| { - if !candidate.action.partial_eq(action) { - return false; - } + let keymap = self.keymap.lock(); + keymap + .bindings_for_action(action) + .filter(|binding| { for i in 1..context_stack.len() { - if candidate.matches_context(&context_stack[0..=i]) { + let context = &context_stack[0..i]; + if keymap.binding_enabled(binding, context) { return true; } } diff --git a/crates/gpui/src/keymap/binding.rs b/crates/gpui/src/keymap/binding.rs index 24394107849e24ae80d827768573972d36b21cb3..766e54f4734c0075e40a4ff4699a1735eb483c80 100644 --- a/crates/gpui/src/keymap/binding.rs +++ b/crates/gpui/src/keymap/binding.rs @@ -1,4 +1,4 @@ -use crate::{Action, KeyBindingContextPredicate, KeyContext, KeyMatch, Keystroke}; +use crate::{Action, KeyBindingContextPredicate, KeyMatch, Keystroke}; use anyhow::Result; use smallvec::SmallVec; @@ -42,21 +42,8 @@ impl KeyBinding { }) } - pub fn matches_context(&self, contexts: &[KeyContext]) -> bool { - self.context_predicate - .as_ref() - .map(|predicate| predicate.eval(contexts)) - .unwrap_or(true) - } - - pub fn match_keystrokes( - &self, - pending_keystrokes: &[Keystroke], - contexts: &[KeyContext], - ) -> KeyMatch { - if self.keystrokes.as_ref().starts_with(pending_keystrokes) - && self.matches_context(contexts) - { + pub fn match_keystrokes(&self, pending_keystrokes: &[Keystroke]) -> KeyMatch { + if self.keystrokes.as_ref().starts_with(pending_keystrokes) { // If the binding is completed, push it onto the matches list if self.keystrokes.as_ref().len() == pending_keystrokes.len() { KeyMatch::Some(vec![self.action.boxed_clone()]) @@ -68,18 +55,6 @@ impl KeyBinding { } } - pub fn keystrokes_for_action( - &self, - action: &dyn Action, - contexts: &[KeyContext], - ) -> Option> { - if self.action.partial_eq(action) && self.matches_context(contexts) { - Some(self.keystrokes.clone()) - } else { - None - } - } - pub fn keystrokes(&self) -> &[Keystroke] { self.keystrokes.as_slice() } @@ -88,3 +63,13 @@ impl KeyBinding { self.action.as_ref() } } + +impl std::fmt::Debug for KeyBinding { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("KeyBinding") + .field("keystrokes", &self.keystrokes) + .field("context_predicate", &self.context_predicate) + .field("action", &self.action.name()) + .finish() + } +} diff --git a/crates/gpui/src/keymap/keymap.rs b/crates/gpui/src/keymap/keymap.rs index 8152693c07a5ab316ce03ab3600ed855595b2ccd..8c74e12e08e8e67e9e0784521b6df164ec25044b 100644 --- a/crates/gpui/src/keymap/keymap.rs +++ b/crates/gpui/src/keymap/keymap.rs @@ -1,4 +1,4 @@ -use crate::{KeyBinding, KeyBindingContextPredicate, Keystroke, NoAction}; +use crate::{Action, KeyBinding, KeyBindingContextPredicate, KeyContext, Keystroke, NoAction}; use collections::HashSet; use smallvec::SmallVec; use std::{ @@ -29,54 +29,22 @@ impl Keymap { self.version } - pub fn bindings_for_action(&self, action_id: TypeId) -> impl Iterator { - self.binding_indices_by_action_id - .get(&action_id) - .map(SmallVec::as_slice) - .unwrap_or(&[]) - .iter() - .map(|ix| &self.bindings[*ix]) - .filter(|binding| !self.binding_disabled(binding)) - } - pub fn add_bindings>(&mut self, bindings: T) { - let no_action_id = &(NoAction {}).type_id(); - let mut new_bindings = Vec::new(); - let mut has_new_disabled_keystrokes = false; + let no_action_id = (NoAction {}).type_id(); + for binding in bindings { - if binding.action.type_id() == *no_action_id { - has_new_disabled_keystrokes |= self - .disabled_keystrokes + let action_id = binding.action().as_any().type_id(); + if action_id == no_action_id { + 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().as_any().type_id()) + .entry(action_id) .or_default() .push(self.bindings.len()); - self.bindings.push(new_binding); + self.bindings.push(binding); } } @@ -90,311 +58,113 @@ impl Keymap { self.version.0 += 1; } - pub fn bindings(&self) -> Vec<&KeyBinding> { - self.bindings - .iter() - .filter(|binding| !self.binding_disabled(binding)) - .collect() + /// Iterate over all bindings, in the order they were added. + pub fn bindings(&self) -> impl Iterator + DoubleEndedIterator { + self.bindings.iter() } - fn binding_disabled(&self, binding: &KeyBinding) -> bool { - match self.disabled_keystrokes.get(&binding.keystrokes) { - Some(disabled_predicates) => disabled_predicates.contains(&binding.context_predicate), - None => false, - } + /// Iterate over all bindings for the given action, in the order they were added. + pub fn bindings_for_action<'a>( + &'a self, + action: &'a dyn Action, + ) -> impl 'a + Iterator + DoubleEndedIterator { + let action_id = action.type_id(); + self.binding_indices_by_action_id + .get(&action_id) + .map_or(&[] as _, SmallVec::as_slice) + .iter() + .map(|ix| &self.bindings[*ix]) + .filter(move |binding| binding.action().partial_eq(action)) } -} - -// #[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(), -// ime_key: None, -// }], -// "{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(), -// ime_key: None, -// }, -// &Keystroke { -// ctrl: true, -// alt: false, -// shift: false, -// cmd: false, -// function: false, -// key: "w".to_string(), -// ime_key: None, -// } -// ], -// "{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()); -// } - -// #[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(); + pub fn binding_enabled(&self, binding: &KeyBinding, context: &[KeyContext]) -> bool { + // If binding has a context predicate, it must match the current context, + if let Some(predicate) = &binding.context_predicate { + if !predicate.eval(context) { + return false; + } + } -// 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(); -// } + if let Some(disabled_predicates) = self.disabled_keystrokes.get(&binding.keystrokes) { + for disabled_predicate in disabled_predicates { + match disabled_predicate { + // The binding must not be globally disabled. + None => return false, -// #[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(), -// ime_key: None, -// }], -// "{expected:?} should have the expected keystroke with key '{expected_key}' in the keymap for element {i}" -// ); + // The binding must not be disabled in the current context. + Some(predicate) => { + if predicate.eval(context) { + return false; + } + } + } + } + } -// 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:?}" -// ); -// } -// } + true + } +} -// #[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" -// ); -// } -// } +#[cfg(test)] +mod tests { + use super::*; + use crate as gpui; + use gpui::actions; + + actions!( + keymap_test, + [ActionAlpha, ActionBeta, ActionGamma, ActionDelta,] + ); + + #[test] + fn test_keymap() { + let bindings = [ + KeyBinding::new("ctrl-a", ActionAlpha {}, None), + KeyBinding::new("ctrl-a", ActionBeta {}, Some("pane")), + KeyBinding::new("ctrl-a", ActionGamma {}, Some("editor && mode==full")), + ]; + + let mut keymap = Keymap::default(); + keymap.add_bindings(bindings.clone()); + + // global bindings are enabled in all contexts + assert!(keymap.binding_enabled(&bindings[0], &[])); + assert!(keymap.binding_enabled(&bindings[0], &[KeyContext::parse("terminal").unwrap()])); + + // contextual bindings are enabled in contexts that match their predicate + assert!(!keymap.binding_enabled(&bindings[1], &[KeyContext::parse("barf x=y").unwrap()])); + assert!(keymap.binding_enabled(&bindings[1], &[KeyContext::parse("pane x=y").unwrap()])); + + assert!(!keymap.binding_enabled(&bindings[2], &[KeyContext::parse("editor").unwrap()])); + assert!(keymap.binding_enabled( + &bindings[2], + &[KeyContext::parse("editor mode=full").unwrap()] + )); + } -// #[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" -// ); -// } -// } -// } + #[test] + fn test_keymap_disabled() { + let bindings = [ + KeyBinding::new("ctrl-a", ActionAlpha {}, Some("editor")), + KeyBinding::new("ctrl-b", ActionAlpha {}, Some("editor")), + KeyBinding::new("ctrl-a", NoAction {}, Some("editor && mode==full")), + KeyBinding::new("ctrl-b", NoAction {}, None), + ]; + + let mut keymap = Keymap::default(); + keymap.add_bindings(bindings.clone()); + + // binding is only enabled in a specific context + assert!(!keymap.binding_enabled(&bindings[0], &[KeyContext::parse("barf").unwrap()])); + assert!(keymap.binding_enabled(&bindings[0], &[KeyContext::parse("editor").unwrap()])); + + // binding is disabled in a more specific context + assert!(!keymap.binding_enabled( + &bindings[0], + &[KeyContext::parse("editor mode=full").unwrap()] + )); + + // binding is globally disabled + assert!(!keymap.binding_enabled(&bindings[1], &[KeyContext::parse("barf").unwrap()])); + } +} diff --git a/crates/gpui/src/keymap/matcher.rs b/crates/gpui/src/keymap/matcher.rs index 9d74975c5635415ea3cf08dc8007f735fca981c3..ab42f1278c3a837c2895d77fdf388a842ed2433b 100644 --- a/crates/gpui/src/keymap/matcher.rs +++ b/crates/gpui/src/keymap/matcher.rs @@ -1,6 +1,5 @@ use crate::{Action, KeyContext, Keymap, KeymapVersion, Keystroke}; use parking_lot::Mutex; -use smallvec::SmallVec; use std::sync::Arc; pub struct KeystrokeMatcher { @@ -51,10 +50,14 @@ impl KeystrokeMatcher { let mut pending_key = None; let mut found_actions = Vec::new(); - for binding in keymap.bindings().iter().rev() { + for binding in keymap.bindings().rev() { + if !keymap.binding_enabled(binding, context_stack) { + continue; + } + for candidate in keystroke.match_candidates() { self.pending_keystrokes.push(candidate.clone()); - match binding.match_keystrokes(&self.pending_keystrokes, context_stack) { + match binding.match_keystrokes(&self.pending_keystrokes) { KeyMatch::Some(mut actions) => { found_actions.append(&mut actions); } @@ -82,19 +85,6 @@ impl KeystrokeMatcher { KeyMatch::Pending } } - - pub fn keystrokes_for_action( - &self, - action: &dyn Action, - contexts: &[KeyContext], - ) -> Option> { - self.keymap - .lock() - .bindings() - .iter() - .rev() - .find_map(|binding| binding.keystrokes_for_action(action, contexts)) - } } #[derive(Debug)] diff --git a/crates/gpui/src/platform/mac/platform.rs b/crates/gpui/src/platform/mac/platform.rs index 70455767933f5de1df61603dbfce621c56162bc8..ff89f91730ae5d33e6720f1c9723ed8f2741f0ad 100644 --- a/crates/gpui/src/platform/mac/platform.rs +++ b/crates/gpui/src/platform/mac/platform.rs @@ -260,8 +260,8 @@ impl MacPlatform { os_action, } => { let keystrokes = keymap - .bindings_for_action(action.type_id()) - .find(|binding| binding.action().partial_eq(action.as_ref())) + .bindings_for_action(action.as_ref()) + .next() .map(|binding| binding.keystrokes()); let selector = match os_action {