From 8a96562adfa1a0fc9eba19bdd245d1cac7eed6fe Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Fri, 5 Jan 2024 12:07:20 -0800 Subject: [PATCH] Handle contexts correctly for disabled key bindings --- crates/gpui/src/key_dispatch.rs | 14 +- crates/gpui/src/keymap/binding.rs | 41 +-- crates/gpui/src/keymap/keymap.rs | 432 ++++++----------------- crates/gpui/src/keymap/matcher.rs | 22 +- crates/gpui/src/platform/mac/platform.rs | 4 +- 5 files changed, 128 insertions(+), 385 deletions(-) diff --git a/crates/gpui/src/key_dispatch.rs b/crates/gpui/src/key_dispatch.rs index ade52a1314..22c4dffc03 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 2439410784..766e54f473 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 8152693c07..8c74e12e08 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)) + } + + 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; + } } + + 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, + + // The binding must not be disabled in the current context. + Some(predicate) => { + if predicate.eval(context) { + return false; + } + } + } + } + } + + true } } -// #[cfg(test)] -// mod tests { -// use crate::actions; +#[cfg(test)] +mod tests { + use super::*; + use crate as gpui; + use gpui::actions; -// use super::*; + actions!( + keymap_test, + [ActionAlpha, ActionBeta, ActionGamma, ActionDelta,] + ); -// actions!( -// keymap_test, -// [Present1, Present2, Present3, Duplicate, Missing] -// ); + #[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")), + ]; -// #[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(); + keymap.add_bindings(bindings.clone()); -// let mut keymap = Keymap::default(); -// assert_absent(&keymap, &all_bindings); -// assert!(keymap.bindings().is_empty()); + // global bindings are enabled in all contexts + assert!(keymap.binding_enabled(&bindings[0], &[])); + assert!(keymap.binding_enabled(&bindings[0], &[KeyContext::parse("terminal").unwrap()])); -// 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")], -// ); + // 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()])); -// 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!(!keymap.binding_enabled(&bindings[2], &[KeyContext::parse("editor").unwrap()])); + assert!(keymap.binding_enabled( + &bindings[2], + &[KeyContext::parse("editor mode=full").unwrap()] + )); + } -// 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" -// ); + #[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 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:?}" -// ); -// } + let mut keymap = Keymap::default(); + keymap.add_bindings(bindings.clone()); -// keymap.clear(); -// assert_absent(&keymap, &all_bindings); -// assert!(keymap.bindings().is_empty()); -// } + // 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()])); -// #[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")); + // binding is disabled in a more specific context + assert!(!keymap.binding_enabled( + &bindings[0], + &[KeyContext::parse("editor mode=full").unwrap()] + )); -// 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(), -// ime_key: None, -// }], -// "{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" -// ); -// } -// } -// } + // 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 9d74975c56..ab42f1278c 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 7045576793..ff89f91730 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 {