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.

Cargo.lock

@@ -9516,7 +9516,7 @@ dependencies = [
 
 [[package]]
 name = "zed"
-version = "0.120.0"
+version = "0.119.3"
 dependencies = [
  "activity_indicator",
  "ai",

crates/gpui/src/key_dispatch.rs

@@ -188,15 +188,13 @@ impl DispatchTree {
         action: &dyn Action,
         context_stack: &Vec<KeyContext>,
     ) -> Vec<KeyBinding> {
-        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;
                     }
                 }

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<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 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<Item = &KeyBinding> + 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<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)]
-// 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<_>>(),
-//             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<_>>(),
-//             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<_>>(),
-//                 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()]));
+    }
+}

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())
-                    .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 {

crates/zed/Cargo.toml

@@ -2,7 +2,7 @@
 description = "The fast, collaborative code editor."
 edition = "2021"
 name = "zed"
-version = "0.120.0"
+version = "0.119.3"
 publish = false
 
 [lib]

crates/zed/RELEASE_CHANNEL

@@ -1 +1 @@
-dev
+preview