Handle contexts correctly for disabled key bindings

crates/gpui/src/key_dispatch.rs

@@ -188,15 +188,13 @@ impl DispatchTree {
         action: &dyn Action,
         context_stack: &Vec<KeyContext>,
     ) -> Vec<KeyBinding> {
-        self.keymap
+        let keymap = self.keymap.lock();
-            .lock()
+        keymap
-            .bindings_for_action(action.type_id())
+            .bindings_for_action(action)
-            .filter(|candidate| {
+            .filter(|binding| {
-                if !candidate.action.partial_eq(action) {
-                    return false;
-                }
                 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;
                     }
                 }

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 {
+    pub fn match_keystrokes(&self, pending_keystrokes: &[Keystroke]) -> KeyMatch {
-        self.context_predicate
+        if self.keystrokes.as_ref().starts_with(pending_keystrokes) {
-            .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)
-        {
             // 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<SmallVec<[Keystroke; 2]>> {
-        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()
+    }
+}

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<Item = &'_ KeyBinding> {
-        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<T: IntoIterator<Item = KeyBinding>>(&mut self, bindings: T) {
-        let no_action_id = &(NoAction {}).type_id();
+        let no_action_id = (NoAction {}).type_id();
-        let mut new_bindings = Vec::new();
+
-        let mut has_new_disabled_keystrokes = false;
         for binding in bindings {
-            if binding.action.type_id() == *no_action_id {
+            let action_id = binding.action().as_any().type_id();
-                has_new_disabled_keystrokes |= self
+            if action_id == no_action_id {
-                    .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().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> {
+    /// Iterate over all bindings, in the order they were added.
-        self.bindings
+    pub fn bindings(&self) -> impl Iterator<Item = &KeyBinding> + DoubleEndedIterator {
-            .iter()
+        self.bindings.iter()
-            .filter(|binding| !self.binding_disabled(binding))
-            .collect()
     }
 
-    fn binding_disabled(&self, binding: &KeyBinding) -> bool {
+    /// Iterate over all bindings for the given action, in the order they were added.
-        match self.disabled_keystrokes.get(&binding.keystrokes) {
+    pub fn bindings_for_action<'a>(
-            Some(disabled_predicates) => disabled_predicates.contains(&binding.context_predicate),
+        &'a self,
-            None => false,
+        action: &'a dyn Action,
+    ) -> impl 'a + Iterator<Item = &'a KeyBinding> + 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)]
+#[cfg(test)]
-// mod tests {
+mod tests {
-//     use crate::actions;
+    use super::*;
+    use crate as gpui;
+    use gpui::actions;
 
-//     use super::*;
+    actions!(
+        keymap_test,
+        [ActionAlpha, ActionBeta, ActionGamma, ActionDelta,]
+    );
 
-//     actions!(
+    #[test]
-//         keymap_test,
+    fn test_keymap() {
-//         [Present1, Present2, Present3, Duplicate, Missing]
+        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]
+        let mut keymap = Keymap::default();
-//     fn regular_keymap() {
+        keymap.add_bindings(bindings.clone());
-//         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();
+        // global bindings are enabled in all contexts
-//         assert_absent(&keymap, &all_bindings);
+        assert!(keymap.binding_enabled(&bindings[0], &[]));
-//         assert!(keymap.bindings().is_empty());
+        assert!(keymap.binding_enabled(&bindings[0], &[KeyContext::parse("terminal").unwrap()]));
 
-//         keymap.add_bindings([present_1.clone(), present_2.clone(), present_3.clone()]);
+        // contextual bindings are enabled in contexts that match their predicate
-//         assert_absent(&keymap, &[&keystroke_duplicate_to_1, &missing]);
+        assert!(!keymap.binding_enabled(&bindings[1], &[KeyContext::parse("barf x=y").unwrap()]));
-//         assert_present(
+        assert!(keymap.binding_enabled(&bindings[1], &[KeyContext::parse("pane x=y").unwrap()]));
-//             &keymap,
-//             &[(&present_1, "q"), (&present_2, "w"), (&present_3, "e")],
-//         );
 
-//         keymap.add_bindings([
+        assert!(!keymap.binding_enabled(&bindings[2], &[KeyContext::parse("editor").unwrap()]));
-//             keystroke_duplicate_to_1.clone(),
+        assert!(keymap.binding_enabled(
-//             full_duplicate_to_2.clone(),
+            &bindings[2],
-//         ]);
+            &[KeyContext::parse("editor mode=full").unwrap()]
-//         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!(
+    #[test]
-//             keymap
+    fn test_keymap_disabled() {
-//                 .bindings_for_action(keystroke_duplicate_to_1.action().id())
+        let bindings = [
-//                 .map(|binding| &binding.keystrokes)
+            KeyBinding::new("ctrl-a", ActionAlpha {}, Some("editor")),
-//                 .flatten()
+            KeyBinding::new("ctrl-b", ActionAlpha {}, Some("editor")),
-//                 .collect::<Vec<_>>(),
+            KeyBinding::new("ctrl-a", NoAction {}, Some("editor && mode==full")),
-//             vec![&Keystroke {
+            KeyBinding::new("ctrl-b", NoAction {}, None),
-//                 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<_>>(),
-//             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 mut keymap = Keymap::default();
-//         let expected_updated_bindings = vec![
+        keymap.add_bindings(bindings.clone());
-//             &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();
+        // binding is only enabled in a specific context
-//         assert_absent(&keymap, &all_bindings);
+        assert!(!keymap.binding_enabled(&bindings[0], &[KeyContext::parse("barf").unwrap()]));
-//         assert!(keymap.bindings().is_empty());
+        assert!(keymap.binding_enabled(&bindings[0], &[KeyContext::parse("editor").unwrap()]));
-//     }
 
-//     #[test]
+        // binding is disabled in a more specific context
-//     fn keymap_with_ignored() {
+        assert!(!keymap.binding_enabled(
-//         let present_1 = Binding::new("ctrl-q", Present1 {}, None);
+            &bindings[0],
-//         let present_2 = Binding::new("ctrl-w", Present2 {}, Some("pane"));
+            &[KeyContext::parse("editor mode=full").unwrap()]
-//         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();
+        // binding is globally disabled
-
+        assert!(!keymap.binding_enabled(&bindings[1], &[KeyContext::parse("barf").unwrap()]));
-//         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<_>>(),
-//                 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"
-//             );
-//         }
-//     }
-// }

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<SmallVec<[Keystroke; 2]>> {
-        self.keymap
-            .lock()
-            .bindings()
-            .iter()
-            .rev()
-            .find_map(|binding| binding.keystrokes_for_action(action, contexts))
-    }
 }
 
 #[derive(Debug)]

crates/gpui/src/platform/mac/platform.rs

@@ -260,8 +260,8 @@ impl MacPlatform {
                 os_action,
             } => {
                 let keystrokes = keymap
-                    .bindings_for_action(action.type_id())
+                    .bindings_for_action(action.as_ref())
-                    .find(|binding| binding.action().partial_eq(action.as_ref()))
+                    .next()
                     .map(|binding| binding.keystrokes());
 
                 let selector = match os_action {