Accept null as a valid action, to disable a keystroke (#2678)

Deals with https://github.com/zed-industries/community/issues/772
Closes
https://linear.app/zed-industries/issue/Z-1518/allow-keybindings-to-be-removed

Now, configuration like 
```json5
[
    {
        "context": "Editor",
        "bindings": {
            "alt-v": null,
        }
    }
]
```

will make `alt+v` to print `√` instead of moving the caret one page up.

Release Notes:

- Added a way to disable keybindings with `null` value

crates/gpui/src/app/window.rs

@@ -14,8 +14,8 @@ use crate::{
     text_layout::TextLayoutCache,
     util::post_inc,
     Action, AnyView, AnyViewHandle, AppContext, BorrowAppContext, BorrowWindowContext, Effect,
-    Element, Entity, Handle, LayoutContext, MouseRegion, MouseRegionId, SceneBuilder, Subscription,
-    View, ViewContext, ViewHandle, WindowInvalidation,
+    Element, Entity, Handle, LayoutContext, MouseRegion, MouseRegionId, NoAction, SceneBuilder,
+    Subscription, View, ViewContext, ViewHandle, WindowInvalidation,
 };
 use anyhow::{anyhow, bail, Result};
 use collections::{HashMap, HashSet};
@@ -434,7 +434,11 @@ impl<'a> WindowContext<'a> {
                 MatchResult::None => false,
                 MatchResult::Pending => true,
                 MatchResult::Matches(matches) => {
+                    let no_action_id = (NoAction {}).id();
                     for (view_id, action) in matches {
+                        if action.id() == no_action_id {
+                            return false;
+                        }
                         if self.dispatch_action(Some(*view_id), action.as_ref()) {
                             self.keystroke_matcher.clear_pending();
                             handled_by = Some(action.boxed_clone());

crates/gpui/src/gpui.rs

@@ -31,3 +31,5 @@ pub use window::{Axis, SizeConstraint, Vector2FExt, WindowContext};
 
 pub use anyhow;
 pub use serde_json;
+
+actions!(zed, [NoAction]);

crates/settings/src/keymap_file.rs

@@ -1,7 +1,7 @@
 use crate::{settings_store::parse_json_with_comments, SettingsAssets};
 use anyhow::{anyhow, Context, Result};
 use collections::BTreeMap;
-use gpui::{keymap_matcher::Binding, AppContext};
+use gpui::{keymap_matcher::Binding, AppContext, NoAction};
 use schemars::{
     gen::{SchemaGenerator, SchemaSettings},
     schema::{InstanceType, Schema, SchemaObject, SingleOrVec, SubschemaValidation},
@@ -11,18 +11,18 @@ use serde::Deserialize;
 use serde_json::Value;
 use util::{asset_str, ResultExt};
 
-#[derive(Deserialize, Default, Clone, JsonSchema)]
+#[derive(Debug, Deserialize, Default, Clone, JsonSchema)]
 #[serde(transparent)]
 pub struct KeymapFile(Vec<KeymapBlock>);
 
-#[derive(Deserialize, Default, Clone, JsonSchema)]
+#[derive(Debug, Deserialize, Default, Clone, JsonSchema)]
 pub struct KeymapBlock {
     #[serde(default)]
     context: Option<String>,
     bindings: BTreeMap<String, KeymapAction>,
 }
 
-#[derive(Deserialize, Default, Clone)]
+#[derive(Debug, Deserialize, Default, Clone)]
 #[serde(transparent)]
 pub struct KeymapAction(Value);
 
@@ -61,21 +61,22 @@ impl KeymapFile {
                     // We want to deserialize the action data as a `RawValue` so that we can
                     // deserialize the action itself dynamically directly from the JSON
                     // string. But `RawValue` currently does not work inside of an untagged enum.
-                    if let Value::Array(items) = action {
-                        let Ok([name, data]): Result<[serde_json::Value; 2], _> = items.try_into() else {
-                            return Some(Err(anyhow!("Expected array of length 2")));
-                        };
-                        let serde_json::Value::String(name) = name else {
-                            return Some(Err(anyhow!("Expected first item in array to be a string.")))
-                        };
-                        cx.deserialize_action(
-                            &name,
-                            Some(data),
-                        )
-                    } else if let Value::String(name) = action {
-                        cx.deserialize_action(&name, None)
-                    } else {
-                        return Some(Err(anyhow!("Expected two-element array, got {:?}", action)));
+                    match action {
+                        Value::Array(items) => {
+                            let Ok([name, data]): Result<[serde_json::Value; 2], _> = items.try_into() else {
+                                return Some(Err(anyhow!("Expected array of length 2")));
+                            };
+                            let serde_json::Value::String(name) = name else {
+                                return Some(Err(anyhow!("Expected first item in array to be a string.")))
+                            };
+                            cx.deserialize_action(
+                                &name,
+                                Some(data),
+                            )
+                        },
+                        Value::String(name) => cx.deserialize_action(&name, None),
+                        Value::Null => Ok(no_action()),
+                        _ => return Some(Err(anyhow!("Expected two-element array, got {action:?}"))),
                     }
                     .with_context(|| {
                         format!(
@@ -115,6 +116,10 @@ impl KeymapFile {
                         instance_type: Some(SingleOrVec::Single(Box::new(InstanceType::Array))),
                         ..Default::default()
                     }),
+                    Schema::Object(SchemaObject {
+                        instance_type: Some(SingleOrVec::Single(Box::new(InstanceType::Null))),
+                        ..Default::default()
+                    }),
                 ]),
                 ..Default::default()
             })),
@@ -129,6 +134,10 @@ impl KeymapFile {
     }
 }
 
+fn no_action() -> Box<dyn gpui::Action> {
+    Box::new(NoAction {})
+}
+
 #[cfg(test)]
 mod tests {
     use crate::KeymapFile;

crates/zed/src/zed.rs

@@ -2074,6 +2074,167 @@ mod tests {
             line!(),
         );
 
+        #[track_caller]
+        fn assert_key_bindings_for<'a>(
+            window_id: usize,
+            cx: &TestAppContext,
+            actions: Vec<(&'static str, &'a dyn Action)>,
+            line: u32,
+        ) {
+            for (key, action) in actions {
+                // assert that...
+                assert!(
+                    cx.available_actions(window_id, 0)
+                        .into_iter()
+                        .any(|(_, bound_action, b)| {
+                            // action names match...
+                            bound_action.name() == action.name()
+                        && bound_action.namespace() == action.namespace()
+                        // and key strokes contain the given key
+                        && b.iter()
+                            .any(|binding| binding.keystrokes().iter().any(|k| k.key == key))
+                        }),
+                    "On {} Failed to find {} with key binding {}",
+                    line,
+                    action.name(),
+                    key
+                );
+            }
+        }
+    }
+
+    #[gpui::test]
+    async fn test_disabled_keymap_binding(cx: &mut gpui::TestAppContext) {
+        struct TestView;
+
+        impl Entity for TestView {
+            type Event = ();
+        }
+
+        impl View for TestView {
+            fn ui_name() -> &'static str {
+                "TestView"
+            }
+
+            fn render(&mut self, _: &mut ViewContext<Self>) -> AnyElement<Self> {
+                Empty::new().into_any()
+            }
+        }
+
+        let executor = cx.background();
+        let fs = FakeFs::new(executor.clone());
+
+        actions!(test, [A, B]);
+        // From the Atom keymap
+        actions!(workspace, [ActivatePreviousPane]);
+        // From the JetBrains keymap
+        actions!(pane, [ActivatePrevItem]);
+
+        fs.save(
+            "/settings.json".as_ref(),
+            &r#"
+            {
+                "base_keymap": "Atom"
+            }
+            "#
+            .into(),
+            Default::default(),
+        )
+        .await
+        .unwrap();
+
+        fs.save(
+            "/keymap.json".as_ref(),
+            &r#"
+            [
+                {
+                    "bindings": {
+                        "backspace": "test::A"
+                    }
+                }
+            ]
+            "#
+            .into(),
+            Default::default(),
+        )
+        .await
+        .unwrap();
+
+        cx.update(|cx| {
+            cx.set_global(SettingsStore::test(cx));
+            theme::init(Assets, cx);
+            welcome::init(cx);
+
+            cx.add_global_action(|_: &A, _cx| {});
+            cx.add_global_action(|_: &B, _cx| {});
+            cx.add_global_action(|_: &ActivatePreviousPane, _cx| {});
+            cx.add_global_action(|_: &ActivatePrevItem, _cx| {});
+
+            let settings_rx = watch_config_file(
+                executor.clone(),
+                fs.clone(),
+                PathBuf::from("/settings.json"),
+            );
+            let keymap_rx =
+                watch_config_file(executor.clone(), fs.clone(), PathBuf::from("/keymap.json"));
+
+            handle_keymap_file_changes(keymap_rx, cx);
+            handle_settings_file_changes(settings_rx, cx);
+        });
+
+        cx.foreground().run_until_parked();
+
+        let (window_id, _view) = cx.add_window(|_| TestView);
+
+        // Test loading the keymap base at all
+        assert_key_bindings_for(
+            window_id,
+            cx,
+            vec![("backspace", &A), ("k", &ActivatePreviousPane)],
+            line!(),
+        );
+
+        // Test disabling the key binding for the base keymap
+        fs.save(
+            "/keymap.json".as_ref(),
+            &r#"
+            [
+                {
+                    "bindings": {
+                        "backspace": null
+                    }
+                }
+            ]
+            "#
+            .into(),
+            Default::default(),
+        )
+        .await
+        .unwrap();
+
+        cx.foreground().run_until_parked();
+
+        assert_key_bindings_for(window_id, cx, vec![("k", &ActivatePreviousPane)], line!());
+
+        // Test modifying the base, while retaining the users keymap
+        fs.save(
+            "/settings.json".as_ref(),
+            &r#"
+            {
+                "base_keymap": "JetBrains"
+            }
+            "#
+            .into(),
+            Default::default(),
+        )
+        .await
+        .unwrap();
+
+        cx.foreground().run_until_parked();
+
+        assert_key_bindings_for(window_id, cx, vec![("[", &ActivatePrevItem)], line!());
+
+        #[track_caller]
         fn assert_key_bindings_for<'a>(
             window_id: usize,
             cx: &TestAppContext,