From 59b366126e9d99f6cf1c8a81580ae4c6901d257e Mon Sep 17 00:00:00 2001 From: "zed-zippy[bot]" <234243425+zed-zippy[bot]@users.noreply.github.com> Date: Mon, 23 Mar 2026 14:59:12 +0000 Subject: [PATCH] Feat unbind (#52047) (cherry-pick to preview) (#52216) Cherry-pick of #52047 to preview ---- ## Context This PR adds an `Unbind` action, as well as syntax sugar in the keymaps for declaring it ``` { "unbind": { "tab: "editor::AcceptEditPrediction" } } ``` Is equivalent to ``` { "bindings": { "tab: ["zed::Unbind", "editor::AcceptEditPrediction"] } } ``` In the keymap, unbind is always parsed first, so that you can unbind and rebind something in the same block. The semantics of `Unbind` differ from `NoAction` in that `NoAction` is treated _as an action_, `Unbind` is treated as a filter. In practice this means that when resolving bindings, we stop searching when we hit a `NoAction` (because we found a matching binding), but we keep looking when we hit an `Unbind` and filter out keystroke:action pairs that match previous unbindings. In essence `Unbind` is only an action so that it fits cleanly in the existing logic. It is really just a storage of deleted bindings. The plan is to rework the edit predictions key bindings on top of this, as well as use `Unbind` rather than `NoAction` in the keymap UI. Both will be done in follow up PRs. Additionally, in this initial implementation unbound actions are matched by name only. The assumption is that actions with arguments are bound to different keys in general. However, the current syntax allows providing arguments to the unbound actions. Both so that copy-paste works, and so that in the future if this functionality is added, keymaps will not break. ## How to Review - The dispatch logic in GPUI - The parsing logic in `keymap_file.rs` ## Self-Review Checklist - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Release Notes: - Added support for unbinding key bindings from the default keymaps. You can now remove default bindings you don't want, without having to re-declare default bindings that use the same keys. For instance, to unbind `tab` from `editor::AcceptEditPrediction`, you can put the following in your `keymap.json` ``` [ { "context": "Editor && edit_prediction", "unbind": { "tab": "editor::AcceptEditPrediction" } } ] ``` Co-authored-by: Ben Kunkle --- crates/gpui/src/action.rs | 35 +-- crates/gpui/src/key_dispatch.rs | 114 +++++---- crates/gpui/src/keymap.rs | 150 ++++++++++-- crates/settings/src/keymap_file.rs | 375 +++++++++++++++++++++++++++-- 4 files changed, 580 insertions(+), 94 deletions(-) diff --git a/crates/gpui/src/action.rs b/crates/gpui/src/action.rs index 1ab619ff171dbeab8a0843393874e7184320e0db..a47ebe69f0d825c6e2c347ea2881180cb5b04573 100644 --- a/crates/gpui/src/action.rs +++ b/crates/gpui/src/action.rs @@ -1,7 +1,7 @@ use anyhow::{Context as _, Result}; use collections::HashMap; pub use gpui_macros::Action; -pub use no_action::{NoAction, is_no_action}; +pub use no_action::{NoAction, Unbind, is_no_action, is_unbind}; use serde_json::json; use std::{ any::{Any, TypeId}, @@ -290,19 +290,6 @@ impl ActionRegistry { } } - #[cfg(test)] - pub(crate) fn load_action(&mut self) { - self.insert_action(MacroActionData { - name: A::name_for_type(), - type_id: TypeId::of::(), - build: A::build, - json_schema: A::action_json_schema, - deprecated_aliases: A::deprecated_aliases(), - deprecation_message: A::deprecation_message(), - documentation: A::documentation(), - }); - } - fn insert_action(&mut self, action: MacroActionData) { let name = action.name; if self.by_name.contains_key(name) { @@ -432,7 +419,8 @@ pub fn generate_list_of_all_registered_actions() -> impl Iterator bool { - action.as_any().type_id() == (NoAction {}).type_id() + action.as_any().is::() + } + + /// Returns whether or not this action represents an unbind marker. + pub fn is_unbind(action: &dyn gpui::Action) -> bool { + action.as_any().is::() } } diff --git a/crates/gpui/src/key_dispatch.rs b/crates/gpui/src/key_dispatch.rs index 03c7eaaaae6e16f8a9c3f486b0a7b863e0c86416..fee75d5dad39df5cb6c2df2729811a1d942d2fe8 100644 --- a/crates/gpui/src/key_dispatch.rs +++ b/crates/gpui/src/key_dispatch.rs @@ -629,66 +629,99 @@ mod tests { use std::{cell::RefCell, ops::Range, rc::Rc}; use crate::{ - Action, ActionRegistry, App, Bounds, Context, DispatchTree, FocusHandle, InputHandler, - IntoElement, KeyBinding, KeyContext, Keymap, Pixels, Point, Render, Subscription, - TestAppContext, UTF16Selection, Window, + ActionRegistry, App, Bounds, Context, DispatchTree, FocusHandle, InputHandler, IntoElement, + KeyBinding, KeyContext, Keymap, Pixels, Point, Render, Subscription, TestAppContext, + UTF16Selection, Unbind, Window, }; - #[derive(PartialEq, Eq)] - struct TestAction; + actions!(dispatch_test, [TestAction, SecondaryTestAction]); - impl Action for TestAction { - fn name(&self) -> &'static str { - "test::TestAction" - } - - fn name_for_type() -> &'static str - where - Self: ::std::marker::Sized, - { - "test::TestAction" - } - - fn partial_eq(&self, action: &dyn Action) -> bool { - action.as_any().downcast_ref::() == Some(self) - } - - fn boxed_clone(&self) -> std::boxed::Box { - Box::new(TestAction) - } + fn test_dispatch_tree(bindings: Vec) -> DispatchTree { + let registry = ActionRegistry::default(); - fn build(_value: serde_json::Value) -> anyhow::Result> - where - Self: Sized, - { - Ok(Box::new(TestAction)) - } + DispatchTree::new( + Rc::new(RefCell::new(Keymap::new(bindings))), + Rc::new(registry), + ) } #[test] fn test_keybinding_for_action_bounds() { - let keymap = Keymap::new(vec![KeyBinding::new( + let tree = test_dispatch_tree(vec![KeyBinding::new( "cmd-n", TestAction, Some("ProjectPanel"), )]); - let mut registry = ActionRegistry::default(); + let contexts = vec![ + KeyContext::parse("Workspace").unwrap(), + KeyContext::parse("ProjectPanel").unwrap(), + ]; + + let keybinding = tree.bindings_for_action(&TestAction, &contexts); + + assert!(keybinding[0].action.partial_eq(&TestAction)) + } + + #[test] + fn test_bindings_for_action_hides_targeted_unbind_in_active_context() { + let tree = test_dispatch_tree(vec![ + KeyBinding::new("tab", TestAction, Some("Editor")), + KeyBinding::new( + "tab", + Unbind("dispatch_test::TestAction".into()), + Some("Editor && edit_prediction"), + ), + KeyBinding::new( + "tab", + SecondaryTestAction, + Some("Editor && showing_completions"), + ), + ]); + + let contexts = vec![ + KeyContext::parse("Workspace").unwrap(), + KeyContext::parse("Editor showing_completions edit_prediction").unwrap(), + ]; - registry.load_action::(); + let bindings = tree.bindings_for_action(&TestAction, &contexts); + assert!(bindings.is_empty()); - let keymap = Rc::new(RefCell::new(keymap)); + let highest = tree.highest_precedence_binding_for_action(&TestAction, &contexts); + assert!(highest.is_none()); + + let fallback_bindings = tree.bindings_for_action(&SecondaryTestAction, &contexts); + assert_eq!(fallback_bindings.len(), 1); + assert!(fallback_bindings[0].action.partial_eq(&SecondaryTestAction)); + } - let tree = DispatchTree::new(keymap, Rc::new(registry)); + #[test] + fn test_bindings_for_action_keeps_targeted_binding_outside_unbind_context() { + let tree = test_dispatch_tree(vec![ + KeyBinding::new("tab", TestAction, Some("Editor")), + KeyBinding::new( + "tab", + Unbind("dispatch_test::TestAction".into()), + Some("Editor && edit_prediction"), + ), + KeyBinding::new( + "tab", + SecondaryTestAction, + Some("Editor && showing_completions"), + ), + ]); let contexts = vec![ KeyContext::parse("Workspace").unwrap(), - KeyContext::parse("ProjectPanel").unwrap(), + KeyContext::parse("Editor").unwrap(), ]; - let keybinding = tree.bindings_for_action(&TestAction, &contexts); + let bindings = tree.bindings_for_action(&TestAction, &contexts); + assert_eq!(bindings.len(), 1); + assert!(bindings[0].action.partial_eq(&TestAction)); - assert!(keybinding[0].action.partial_eq(&TestAction)) + let highest = tree.highest_precedence_binding_for_action(&TestAction, &contexts); + assert!(highest.is_some_and(|binding| binding.action.partial_eq(&TestAction))); } #[test] @@ -698,10 +731,7 @@ mod tests { KeyBinding::new("space", TestAction, Some("ContextA")), KeyBinding::new("space f g", TestAction, Some("ContextB")), ]; - let keymap = Rc::new(RefCell::new(Keymap::new(bindings))); - let mut registry = ActionRegistry::default(); - registry.load_action::(); - let mut tree = DispatchTree::new(keymap, Rc::new(registry)); + let mut tree = test_dispatch_tree(bindings); type DispatchPath = SmallVec<[super::DispatchNodeId; 32]>; fn dispatch( diff --git a/crates/gpui/src/keymap.rs b/crates/gpui/src/keymap.rs index d5398ff0447849ca5bfcdbbb5a838af0cbc22836..eaf582a0074d4e8d21d46fdeadf44141182405a6 100644 --- a/crates/gpui/src/keymap.rs +++ b/crates/gpui/src/keymap.rs @@ -4,7 +4,7 @@ mod context; pub use binding::*; pub use context::*; -use crate::{Action, AsKeystroke, Keystroke, is_no_action}; +use crate::{Action, AsKeystroke, Keystroke, Unbind, is_no_action, is_unbind}; use collections::{HashMap, HashSet}; use smallvec::SmallVec; use std::any::TypeId; @@ -19,7 +19,7 @@ pub struct KeymapVersion(usize); pub struct Keymap { bindings: Vec, binding_indices_by_action_id: HashMap>, - no_action_binding_indices: Vec, + disabled_binding_indices: Vec, version: KeymapVersion, } @@ -27,6 +27,26 @@ pub struct Keymap { #[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd)] pub struct BindingIndex(usize); +fn disabled_binding_matches_context(disabled_binding: &KeyBinding, binding: &KeyBinding) -> bool { + match ( + &disabled_binding.context_predicate, + &binding.context_predicate, + ) { + (None, _) => true, + (Some(_), None) => false, + (Some(disabled_predicate), Some(predicate)) => disabled_predicate.is_superset(predicate), + } +} + +fn binding_is_unbound(disabled_binding: &KeyBinding, binding: &KeyBinding) -> bool { + disabled_binding.keystrokes == binding.keystrokes + && disabled_binding + .action() + .as_any() + .downcast_ref::() + .is_some_and(|unbind| unbind.0.as_ref() == binding.action.name()) +} + impl Keymap { /// Create a new keymap with the given bindings. pub fn new(bindings: Vec) -> Self { @@ -44,8 +64,8 @@ impl Keymap { pub fn add_bindings>(&mut self, bindings: T) { for binding in bindings { let action_id = binding.action().as_any().type_id(); - if is_no_action(&*binding.action) { - self.no_action_binding_indices.push(self.bindings.len()); + if is_no_action(&*binding.action) || is_unbind(&*binding.action) { + self.disabled_binding_indices.push(self.bindings.len()); } else { self.binding_indices_by_action_id .entry(action_id) @@ -62,7 +82,7 @@ impl Keymap { pub fn clear(&mut self) { self.bindings.clear(); self.binding_indices_by_action_id.clear(); - self.no_action_binding_indices.clear(); + self.disabled_binding_indices.clear(); self.version.0 += 1; } @@ -90,21 +110,22 @@ impl Keymap { return None; } - for null_ix in &self.no_action_binding_indices { - if null_ix > ix { - let null_binding = &self.bindings[*null_ix]; - if null_binding.keystrokes == binding.keystrokes { - let null_binding_matches = - match (&null_binding.context_predicate, &binding.context_predicate) { - (None, _) => true, - (Some(_), None) => false, - (Some(null_predicate), Some(predicate)) => { - null_predicate.is_superset(predicate) - } - }; - if null_binding_matches { + for disabled_ix in &self.disabled_binding_indices { + if disabled_ix > ix { + let disabled_binding = &self.bindings[*disabled_ix]; + if disabled_binding.keystrokes != binding.keystrokes { + continue; + } + + if is_no_action(&*disabled_binding.action) { + if disabled_binding_matches_context(disabled_binding, binding) { return None; } + } else if is_unbind(&*disabled_binding.action) + && disabled_binding_matches_context(disabled_binding, binding) + && binding_is_unbound(disabled_binding, binding) + { + return None; } } } @@ -170,6 +191,7 @@ impl Keymap { let mut bindings: SmallVec<[_; 1]> = SmallVec::new(); let mut first_binding_index = None; + let mut unbound_bindings: Vec<&KeyBinding> = Vec::new(); for (_, ix, binding) in matched_bindings { if is_no_action(&*binding.action) { @@ -186,6 +208,19 @@ impl Keymap { // For non-user NoAction bindings, continue searching for user overrides continue; } + + if is_unbind(&*binding.action) { + unbound_bindings.push(binding); + continue; + } + + if unbound_bindings + .iter() + .any(|disabled_binding| binding_is_unbound(disabled_binding, binding)) + { + continue; + } + bindings.push(binding.clone()); first_binding_index.get_or_insert(ix); } @@ -197,7 +232,7 @@ impl Keymap { { continue; } - if is_no_action(&*binding.action) { + if is_no_action(&*binding.action) || is_unbind(&*binding.action) { pending.remove(&&binding.keystrokes); continue; } @@ -232,7 +267,10 @@ impl Keymap { match pending { None => None, Some(is_pending) => { - if !is_pending || is_no_action(&*binding.action) { + if !is_pending + || is_no_action(&*binding.action) + || is_unbind(&*binding.action) + { return None; } Some((depth, BindingIndex(ix), binding)) @@ -256,7 +294,7 @@ impl Keymap { mod tests { use super::*; use crate as gpui; - use gpui::NoAction; + use gpui::{NoAction, Unbind}; actions!( test_only, @@ -720,6 +758,76 @@ mod tests { } } + #[test] + fn test_targeted_unbind_ignores_target_context() { + let bindings = [ + KeyBinding::new("tab", ActionAlpha {}, Some("Editor")), + KeyBinding::new("tab", ActionBeta {}, Some("Editor && showing_completions")), + KeyBinding::new( + "tab", + Unbind("test_only::ActionAlpha".into()), + Some("Editor && edit_prediction"), + ), + ]; + + let mut keymap = Keymap::default(); + keymap.add_bindings(bindings); + + let (result, pending) = keymap.bindings_for_input( + &[Keystroke::parse("tab").unwrap()], + &[KeyContext::parse("Editor showing_completions edit_prediction").unwrap()], + ); + + assert!(!pending); + assert_eq!(result.len(), 1); + assert!(result[0].action.partial_eq(&ActionBeta {})); + } + + #[test] + fn test_bindings_for_action_keeps_binding_for_narrower_targeted_unbind() { + let bindings = [ + KeyBinding::new("tab", ActionAlpha {}, Some("Editor")), + KeyBinding::new( + "tab", + Unbind("test_only::ActionAlpha".into()), + Some("Editor && edit_prediction"), + ), + KeyBinding::new("tab", ActionBeta {}, Some("Editor && showing_completions")), + ]; + + let mut keymap = Keymap::default(); + keymap.add_bindings(bindings); + + assert_bindings(&keymap, &ActionAlpha {}, &["tab"]); + assert_bindings(&keymap, &ActionBeta {}, &["tab"]); + + #[track_caller] + fn assert_bindings(keymap: &Keymap, action: &dyn Action, expected: &[&str]) { + let actual = keymap + .bindings_for_action(action) + .map(|binding| binding.keystrokes[0].inner().unparse()) + .collect::>(); + assert_eq!(actual, expected, "{:?}", action); + } + } + + #[test] + fn test_bindings_for_action_removes_binding_for_broader_targeted_unbind() { + let bindings = [ + KeyBinding::new("tab", ActionAlpha {}, Some("Editor && edit_prediction")), + KeyBinding::new( + "tab", + Unbind("test_only::ActionAlpha".into()), + Some("Editor"), + ), + ]; + + let mut keymap = Keymap::default(); + keymap.add_bindings(bindings); + + assert!(keymap.bindings_for_action(&ActionAlpha {}).next().is_none()); + } + #[test] fn test_source_precedence_sorting() { // KeybindSource precedence: User (0) > Vim (1) > Base (2) > Default (3) diff --git a/crates/settings/src/keymap_file.rs b/crates/settings/src/keymap_file.rs index 0bc7c45afb6870c772c5963aebcf9807988ac359..79713bdb5a20250a7b98b81bf73408cd63f55c60 100644 --- a/crates/settings/src/keymap_file.rs +++ b/crates/settings/src/keymap_file.rs @@ -4,7 +4,7 @@ use fs::Fs; use gpui::{ Action, ActionBuildError, App, InvalidKeystrokeError, KEYSTROKE_PARSE_EXPECTED_MESSAGE, KeyBinding, KeyBindingContextPredicate, KeyBindingMetaIndex, KeybindingKeystroke, Keystroke, - NoAction, SharedString, generate_list_of_all_registered_actions, register_action, + NoAction, SharedString, Unbind, generate_list_of_all_registered_actions, register_action, }; use schemars::{JsonSchema, json_schema}; use serde::Deserialize; @@ -73,6 +73,10 @@ pub struct KeymapSection { /// on macOS. See the documentation for more details. #[serde(default)] use_key_equivalents: bool, + /// This keymap section's unbindings, as a JSON object mapping keystrokes to actions. These are + /// parsed before `bindings`, so bindings later in the same section can still take precedence. + #[serde(default)] + unbind: Option>, /// This keymap section's bindings, as a JSON object mapping keystrokes to actions. The /// keystrokes key is a string representing a sequence of keystrokes to type, where the /// keystrokes are separated by whitespace. Each keystroke is a sequence of modifiers (`ctrl`, @@ -135,6 +139,20 @@ impl JsonSchema for KeymapAction { } } +#[derive(Debug, Deserialize, Default, Clone)] +#[serde(transparent)] +pub struct UnbindTargetAction(Value); + +impl JsonSchema for UnbindTargetAction { + fn schema_name() -> Cow<'static, str> { + "UnbindTargetAction".into() + } + + fn json_schema(_: &mut schemars::SchemaGenerator) -> schemars::Schema { + json_schema!(true) + } +} + #[derive(Debug)] #[must_use] pub enum KeymapFileLoadResult { @@ -231,6 +249,7 @@ impl KeymapFile { for KeymapSection { context, use_key_equivalents, + unbind, bindings, unrecognized_fields, } in keymap_file.0.iter() @@ -244,7 +263,7 @@ impl KeymapFile { // Leading space is to separate from the message indicating which section // the error occurred in. errors.push(( - context, + context.clone(), format!(" Parse error in section `context` field: {}", err), )); continue; @@ -263,6 +282,38 @@ impl KeymapFile { .unwrap(); } + if let Some(unbind) = unbind { + for (keystrokes, action) in unbind { + let result = Self::load_unbinding( + keystrokes, + action, + context_predicate.clone(), + *use_key_equivalents, + cx, + ); + match result { + Ok(key_binding) => { + key_bindings.push(key_binding); + } + Err(err) => { + let mut lines = err.lines(); + let mut indented_err = lines.next().unwrap().to_string(); + for line in lines { + indented_err.push_str(" "); + indented_err.push_str(line); + indented_err.push_str("\n"); + } + write!( + section_errors, + "\n\n- In unbind {}, {indented_err}", + MarkdownInlineCode(&format!("\"{}\"", keystrokes)) + ) + .unwrap(); + } + } + } + } + if let Some(bindings) = bindings { for (keystrokes, action) in bindings { let result = Self::load_keybinding( @@ -296,7 +347,7 @@ impl KeymapFile { } if !section_errors.is_empty() { - errors.push((context, section_errors)) + errors.push((context.clone(), section_errors)) } } @@ -332,7 +383,17 @@ impl KeymapFile { use_key_equivalents: bool, cx: &App, ) -> std::result::Result { - let (action, action_input_string) = Self::build_keymap_action(action, cx)?; + Self::load_keybinding_action_value(keystrokes, &action.0, context, use_key_equivalents, cx) + } + + fn load_keybinding_action_value( + keystrokes: &str, + action: &Value, + context: Option>, + use_key_equivalents: bool, + cx: &App, + ) -> std::result::Result { + let (action, action_input_string) = Self::build_keymap_action_value(action, cx)?; let key_binding = match KeyBinding::load( keystrokes, @@ -362,23 +423,70 @@ impl KeymapFile { } } + fn load_unbinding( + keystrokes: &str, + action: &UnbindTargetAction, + context: Option>, + use_key_equivalents: bool, + cx: &App, + ) -> std::result::Result { + let key_binding = Self::load_keybinding_action_value( + keystrokes, + &action.0, + context, + use_key_equivalents, + cx, + )?; + + if key_binding.action().partial_eq(&NoAction) { + return Err("expected action name string or [name, input] array.".to_string()); + } + + if key_binding.action().name() == Unbind::name_for_type() { + return Err(format!( + "can't use {} as an unbind target.", + MarkdownInlineCode(&format!("\"{}\"", Unbind::name_for_type())) + )); + } + + KeyBinding::load( + keystrokes, + Box::new(Unbind(key_binding.action().name().into())), + key_binding.predicate(), + use_key_equivalents, + key_binding.action_input(), + cx.keyboard_mapper().as_ref(), + ) + .map_err(|InvalidKeystrokeError { keystroke }| { + format!( + "invalid keystroke {}. {}", + MarkdownInlineCode(&format!("\"{}\"", &keystroke)), + KEYSTROKE_PARSE_EXPECTED_MESSAGE + ) + }) + } + pub fn parse_action( action: &KeymapAction, ) -> Result)>, String> { - let name_and_input = match &action.0 { + Self::parse_action_value(&action.0) + } + + fn parse_action_value(action: &Value) -> Result)>, String> { + let name_and_input = match action { Value::Array(items) => { if items.len() != 2 { return Err(format!( "expected two-element array of `[name, input]`. \ Instead found {}.", - MarkdownInlineCode(&action.0.to_string()) + MarkdownInlineCode(&action.to_string()) )); } let serde_json::Value::String(ref name) = items[0] else { return Err(format!( "expected two-element array of `[name, input]`, \ but the first element is not a string in {}.", - MarkdownInlineCode(&action.0.to_string()) + MarkdownInlineCode(&action.to_string()) )); }; Some((name, Some(&items[1]))) @@ -389,7 +497,7 @@ impl KeymapFile { return Err(format!( "expected two-element array of `[name, input]`. \ Instead found {}.", - MarkdownInlineCode(&action.0.to_string()) + MarkdownInlineCode(&action.to_string()) )); } }; @@ -400,7 +508,14 @@ impl KeymapFile { action: &KeymapAction, cx: &App, ) -> std::result::Result<(Box, Option), String> { - let (build_result, action_input_string) = match Self::parse_action(action)? { + Self::build_keymap_action_value(&action.0, cx) + } + + fn build_keymap_action_value( + action: &Value, + cx: &App, + ) -> std::result::Result<(Box, Option), String> { + let (build_result, action_input_string) = match Self::parse_action_value(action)? { Some((name, action_input)) if name.as_str() == ActionSequence::name_for_type() => { match action_input { Some(action_input) => ( @@ -583,9 +698,15 @@ impl KeymapFile { "minItems": 2, "maxItems": 2 }); - let mut keymap_action_alternatives = vec![empty_action_name, empty_action_name_with_input]; + let mut keymap_action_alternatives = vec![ + empty_action_name.clone(), + empty_action_name_with_input.clone(), + ]; + let mut unbind_target_action_alternatives = + vec![empty_action_name, empty_action_name_with_input]; let mut empty_schema_action_names = vec![]; + let mut empty_schema_unbind_target_action_names = vec![]; for (name, action_schema) in action_schemas.into_iter() { let deprecation = if name == NoAction.name() { Some("null") @@ -593,6 +714,9 @@ impl KeymapFile { deprecations.get(name).copied() }; + let include_in_unbind_target_schema = + name != NoAction.name() && name != Unbind::name_for_type(); + // Add an alternative for plain action names. let mut plain_action = json_schema!({ "type": "string", @@ -607,7 +731,10 @@ impl KeymapFile { if let Some(description) = &description { add_description(&mut plain_action, description); } - keymap_action_alternatives.push(plain_action); + keymap_action_alternatives.push(plain_action.clone()); + if include_in_unbind_target_schema { + unbind_target_action_alternatives.push(plain_action); + } // Add an alternative for actions with data specified as a [name, data] array. // @@ -633,9 +760,15 @@ impl KeymapFile { "minItems": 2, "maxItems": 2 }); - keymap_action_alternatives.push(action_with_input); + keymap_action_alternatives.push(action_with_input.clone()); + if include_in_unbind_target_schema { + unbind_target_action_alternatives.push(action_with_input); + } } else { empty_schema_action_names.push(name); + if include_in_unbind_target_schema { + empty_schema_unbind_target_action_names.push(name); + } } } @@ -659,20 +792,44 @@ impl KeymapFile { keymap_action_alternatives.push(actions_with_empty_input); } + if !empty_schema_unbind_target_action_names.is_empty() { + let action_names = json_schema!({ "enum": empty_schema_unbind_target_action_names }); + let no_properties_allowed = json_schema!({ + "type": "object", + "additionalProperties": false + }); + let mut actions_with_empty_input = json_schema!({ + "type": "array", + "items": [action_names, no_properties_allowed], + "minItems": 2, + "maxItems": 2 + }); + add_deprecation( + &mut actions_with_empty_input, + "This action does not take input - just the action name string should be used." + .to_string(), + ); + unbind_target_action_alternatives.push(actions_with_empty_input); + } + // Placing null first causes json-language-server to default assuming actions should be // null, so place it last. keymap_action_alternatives.push(json_schema!({ "type": "null" })); - // The `KeymapSection` schema will reference the `KeymapAction` schema by name, so setting - // the definition of `KeymapAction` results in the full action schema being used. generator.definitions_mut().insert( KeymapAction::schema_name().to_string(), json!({ "anyOf": keymap_action_alternatives }), ); + generator.definitions_mut().insert( + UnbindTargetAction::schema_name().to_string(), + json!({ + "anyOf": unbind_target_action_alternatives + }), + ); generator.root_schema_for::().to_value() } @@ -1260,7 +1417,8 @@ impl Action for ActionSequence { #[cfg(test)] mod tests { - use gpui::{DummyKeyboardMapper, KeybindingKeystroke, Keystroke}; + use gpui::{Action, App, DummyKeyboardMapper, KeybindingKeystroke, Keystroke, Unbind}; + use serde_json::Value; use unindent::Unindent; use crate::{ @@ -1268,6 +1426,8 @@ mod tests { keymap_file::{KeybindUpdateOperation, KeybindUpdateTarget}, }; + gpui::actions!(test_keymap_file, [StringAction, InputAction]); + #[test] fn can_deserialize_keymap_with_trailing_comma() { let json = indoc::indoc! {"[ @@ -1283,6 +1443,191 @@ mod tests { KeymapFile::parse(json).unwrap(); } + #[gpui::test] + fn keymap_section_unbinds_are_loaded_before_bindings(cx: &mut App) { + let key_bindings = match KeymapFile::load( + indoc::indoc! {r#" + [ + { + "unbind": { + "ctrl-a": "test_keymap_file::StringAction", + "ctrl-b": ["test_keymap_file::InputAction", {}] + }, + "bindings": { + "ctrl-c": "test_keymap_file::StringAction" + } + } + ] + "#}, + cx, + ) { + crate::keymap_file::KeymapFileLoadResult::Success { key_bindings } => key_bindings, + crate::keymap_file::KeymapFileLoadResult::SomeFailedToLoad { + error_message, .. + } => { + panic!("{error_message}"); + } + crate::keymap_file::KeymapFileLoadResult::JsonParseFailure { error } => { + panic!("JSON parse error: {error}"); + } + }; + + assert_eq!(key_bindings.len(), 3); + assert!( + key_bindings[0] + .action() + .partial_eq(&Unbind("test_keymap_file::StringAction".into())) + ); + assert_eq!(key_bindings[0].action_input(), None); + assert!( + key_bindings[1] + .action() + .partial_eq(&Unbind("test_keymap_file::InputAction".into())) + ); + assert_eq!( + key_bindings[1] + .action_input() + .as_ref() + .map(ToString::to_string), + Some("{}".to_string()) + ); + assert_eq!( + key_bindings[2].action().name(), + "test_keymap_file::StringAction" + ); + } + + #[gpui::test] + fn keymap_unbind_loads_valid_target_action_with_input(cx: &mut App) { + let key_bindings = match KeymapFile::load( + indoc::indoc! {r#" + [ + { + "unbind": { + "ctrl-a": ["test_keymap_file::InputAction", {}] + } + } + ] + "#}, + cx, + ) { + crate::keymap_file::KeymapFileLoadResult::Success { key_bindings } => key_bindings, + other => panic!("expected Success, got {other:?}"), + }; + + assert_eq!(key_bindings.len(), 1); + assert!( + key_bindings[0] + .action() + .partial_eq(&Unbind("test_keymap_file::InputAction".into())) + ); + assert_eq!( + key_bindings[0] + .action_input() + .as_ref() + .map(ToString::to_string), + Some("{}".to_string()) + ); + } + + #[gpui::test] + fn keymap_unbind_rejects_null(cx: &mut App) { + match KeymapFile::load( + indoc::indoc! {r#" + [ + { + "unbind": { + "ctrl-a": null + } + } + ] + "#}, + cx, + ) { + crate::keymap_file::KeymapFileLoadResult::SomeFailedToLoad { + key_bindings, + error_message, + } => { + assert!(key_bindings.is_empty()); + assert!( + error_message + .0 + .contains("expected action name string or [name, input] array.") + ); + } + other => panic!("expected SomeFailedToLoad, got {other:?}"), + } + } + + #[gpui::test] + fn keymap_unbind_rejects_unbind_action(cx: &mut App) { + match KeymapFile::load( + indoc::indoc! {r#" + [ + { + "unbind": { + "ctrl-a": ["zed::Unbind", "test_keymap_file::StringAction"] + } + } + ] + "#}, + cx, + ) { + crate::keymap_file::KeymapFileLoadResult::SomeFailedToLoad { + key_bindings, + error_message, + } => { + assert!(key_bindings.is_empty()); + assert!( + error_message + .0 + .contains("can't use `\"zed::Unbind\"` as an unbind target.") + ); + } + other => panic!("expected SomeFailedToLoad, got {other:?}"), + } + } + + #[test] + fn keymap_schema_for_unbind_excludes_null_and_unbind_action() { + fn schema_allows(schema: &Value, expected: &Value) -> bool { + match schema { + Value::Object(object) => { + if object.get("const") == Some(expected) { + return true; + } + if object.get("type") == Some(&Value::String("null".to_string())) + && expected == &Value::Null + { + return true; + } + object.values().any(|value| schema_allows(value, expected)) + } + Value::Array(items) => items.iter().any(|value| schema_allows(value, expected)), + _ => false, + } + } + + let schema = KeymapFile::generate_json_schema_from_inventory(); + let unbind_schema = schema + .pointer("/$defs/UnbindTargetAction") + .expect("missing UnbindTargetAction schema"); + + assert!(!schema_allows(unbind_schema, &Value::Null)); + assert!(!schema_allows( + unbind_schema, + &Value::String(Unbind::name_for_type().to_string()) + )); + assert!(schema_allows( + unbind_schema, + &Value::String("test_keymap_file::StringAction".to_string()) + )); + assert!(schema_allows( + unbind_schema, + &Value::String("test_keymap_file::InputAction".to_string()) + )); + } + #[track_caller] fn check_keymap_update( input: impl ToString,