From 21b4a2ecdd9cae24fcc20b73c35eee278b51bbe3 Mon Sep 17 00:00:00 2001 From: Ben Kunkle Date: Wed, 16 Jul 2025 09:49:16 -0500 Subject: [PATCH] keymap_ui: Infer use key equivalents (#34498) Closes #ISSUE This PR attempts to add workarounds for `use_key_equivalents` in the keymap UI. First of all it makes it so that `use_key_equivalents` is ignored when searching for a binding to replace so that replacing a keybind with `use_key_equivalents` set to true does not result in a new binding. Second, it attempts to infer the value of `use_key_equivalents` off of a base binding when adding a binding by adding an optional `from` parameter to the `KeymapUpdateOperation::Add` variant. Neither workaround will work when the `from` binding for an add or the `target` binding for a replace are not in the user keymap. cc: @Anthony-Eid Release Notes: - N/A *or* Added/Fixed/Improved ... --- crates/settings/src/keymap_file.rs | 178 +++++++++++++++++--------- crates/settings/src/settings_json.rs | 25 +++- crates/settings_ui/src/keybindings.rs | 117 +++++++++-------- 3 files changed, 201 insertions(+), 119 deletions(-) diff --git a/crates/settings/src/keymap_file.rs b/crates/settings/src/keymap_file.rs index 470c5faf78..b61d30e405 100644 --- a/crates/settings/src/keymap_file.rs +++ b/crates/settings/src/keymap_file.rs @@ -10,6 +10,7 @@ use serde::Deserialize; use serde_json::{Value, json}; use std::borrow::Cow; use std::{any::TypeId, fmt::Write, rc::Rc, sync::Arc, sync::LazyLock}; +use util::ResultExt as _; use util::{ asset_str, markdown::{MarkdownEscaped, MarkdownInlineCode, MarkdownString}, @@ -612,19 +613,26 @@ impl KeymapFile { KeybindUpdateOperation::Replace { target_keybind_source: target_source, source, - .. + target, } if target_source != KeybindSource::User => { - operation = KeybindUpdateOperation::Add(source); + operation = KeybindUpdateOperation::Add { + source, + from: Some(target), + }; } // if trying to remove a keybinding that is not user-defined, treat it as creating a binding // that binds it to `zed::NoAction` KeybindUpdateOperation::Remove { - mut target, + target, target_keybind_source, } if target_keybind_source != KeybindSource::User => { - target.action_name = gpui::NoAction.name(); - target.action_arguments.take(); - operation = KeybindUpdateOperation::Add(target); + let mut source = target.clone(); + source.action_name = gpui::NoAction.name(); + source.action_arguments.take(); + operation = KeybindUpdateOperation::Add { + source, + from: Some(target), + }; } _ => {} } @@ -742,7 +750,10 @@ impl KeymapFile { ) .context("Failed to replace keybinding")?; keymap_contents.replace_range(replace_range, &replace_value); - operation = KeybindUpdateOperation::Add(source); + operation = KeybindUpdateOperation::Add { + source, + from: Some(target), + }; } } else { log::warn!( @@ -752,16 +763,28 @@ impl KeymapFile { source.keystrokes, source_action_value, ); - operation = KeybindUpdateOperation::Add(source); + operation = KeybindUpdateOperation::Add { + source, + from: Some(target), + }; } } - if let KeybindUpdateOperation::Add(keybinding) = operation { + if let KeybindUpdateOperation::Add { + source: keybinding, + from, + } = operation + { let mut value = serde_json::Map::with_capacity(4); if let Some(context) = keybinding.context { value.insert("context".to_string(), context.into()); } - if keybinding.use_key_equivalents { + let use_key_equivalents = from.and_then(|from| { + let action_value = from.action_value().context("Failed to serialize action value. `use_key_equivalents` on new keybinding may be incorrect.").log_err()?; + let (index, _) = find_binding(&keymap, &from, &action_value)?; + Some(keymap.0[index].use_key_equivalents) + }).unwrap_or(false); + if use_key_equivalents { value.insert("use_key_equivalents".to_string(), true.into()); } @@ -794,9 +817,6 @@ impl KeymapFile { if section_context_parsed != target_context_parsed { continue; } - if section.use_key_equivalents != target.use_key_equivalents { - continue; - } let Some(bindings) = §ion.bindings else { continue; }; @@ -835,19 +855,27 @@ pub enum KeybindUpdateOperation<'a> { target: KeybindUpdateTarget<'a>, target_keybind_source: KeybindSource, }, - Add(KeybindUpdateTarget<'a>), + Add { + source: KeybindUpdateTarget<'a>, + from: Option>, + }, Remove { target: KeybindUpdateTarget<'a>, target_keybind_source: KeybindSource, }, } -#[derive(Debug)] +impl<'a> KeybindUpdateOperation<'a> { + pub fn add(source: KeybindUpdateTarget<'a>) -> Self { + Self::Add { source, from: None } + } +} + +#[derive(Debug, Clone)] pub struct KeybindUpdateTarget<'a> { pub context: Option<&'a str>, pub keystrokes: &'a [Keystroke], pub action_name: &'a str, - pub use_key_equivalents: bool, pub action_arguments: Option<&'a str>, } @@ -933,6 +961,7 @@ impl From for KeyBindingMetaIndex { #[cfg(test)] mod tests { + use gpui::Keystroke; use unindent::Unindent; use crate::{ @@ -955,37 +984,35 @@ mod tests { KeymapFile::parse(json).unwrap(); } + #[track_caller] + fn check_keymap_update( + input: impl ToString, + operation: KeybindUpdateOperation, + expected: impl ToString, + ) { + let result = KeymapFile::update_keybinding(operation, input.to_string(), 4) + .expect("Update succeeded"); + pretty_assertions::assert_eq!(expected.to_string(), result); + } + + #[track_caller] + fn parse_keystrokes(keystrokes: &str) -> Vec { + return keystrokes + .split(' ') + .map(|s| Keystroke::parse(s).expect("Keystrokes valid")) + .collect(); + } + #[test] fn keymap_update() { - use gpui::Keystroke; - zlog::init_test(); - #[track_caller] - fn check_keymap_update( - input: impl ToString, - operation: KeybindUpdateOperation, - expected: impl ToString, - ) { - let result = KeymapFile::update_keybinding(operation, input.to_string(), 4) - .expect("Update succeeded"); - pretty_assertions::assert_eq!(expected.to_string(), result); - } - - #[track_caller] - fn parse_keystrokes(keystrokes: &str) -> Vec { - return keystrokes - .split(' ') - .map(|s| Keystroke::parse(s).expect("Keystrokes valid")) - .collect(); - } check_keymap_update( "[]", - KeybindUpdateOperation::Add(KeybindUpdateTarget { + KeybindUpdateOperation::add(KeybindUpdateTarget { keystrokes: &parse_keystrokes("ctrl-a"), action_name: "zed::SomeAction", context: None, - use_key_equivalents: false, action_arguments: None, }), r#"[ @@ -1007,11 +1034,10 @@ mod tests { } ]"# .unindent(), - KeybindUpdateOperation::Add(KeybindUpdateTarget { + KeybindUpdateOperation::add(KeybindUpdateTarget { keystrokes: &parse_keystrokes("ctrl-b"), action_name: "zed::SomeOtherAction", context: None, - use_key_equivalents: false, action_arguments: None, }), r#"[ @@ -1038,11 +1064,10 @@ mod tests { } ]"# .unindent(), - KeybindUpdateOperation::Add(KeybindUpdateTarget { + KeybindUpdateOperation::add(KeybindUpdateTarget { keystrokes: &parse_keystrokes("ctrl-b"), action_name: "zed::SomeOtherAction", context: None, - use_key_equivalents: false, action_arguments: Some(r#"{"foo": "bar"}"#), }), r#"[ @@ -1074,11 +1099,10 @@ mod tests { } ]"# .unindent(), - KeybindUpdateOperation::Add(KeybindUpdateTarget { + KeybindUpdateOperation::add(KeybindUpdateTarget { keystrokes: &parse_keystrokes("ctrl-b"), action_name: "zed::SomeOtherAction", context: Some("Zed > Editor && some_condition = true"), - use_key_equivalents: true, action_arguments: Some(r#"{"foo": "bar"}"#), }), r#"[ @@ -1089,7 +1113,6 @@ mod tests { }, { "context": "Zed > Editor && some_condition = true", - "use_key_equivalents": true, "bindings": { "ctrl-b": [ "zed::SomeOtherAction", @@ -1117,14 +1140,12 @@ mod tests { keystrokes: &parse_keystrokes("ctrl-a"), action_name: "zed::SomeAction", context: None, - use_key_equivalents: false, action_arguments: None, }, source: KeybindUpdateTarget { keystrokes: &parse_keystrokes("ctrl-b"), action_name: "zed::SomeOtherAction", context: None, - use_key_equivalents: false, action_arguments: Some(r#"{"foo": "bar"}"#), }, target_keybind_source: KeybindSource::Base, @@ -1163,14 +1184,12 @@ mod tests { keystrokes: &parse_keystrokes("a"), action_name: "zed::SomeAction", context: None, - use_key_equivalents: false, action_arguments: None, }, source: KeybindUpdateTarget { keystrokes: &parse_keystrokes("ctrl-b"), action_name: "zed::SomeOtherAction", context: None, - use_key_equivalents: false, action_arguments: Some(r#"{"foo": "bar"}"#), }, target_keybind_source: KeybindSource::User, @@ -1204,14 +1223,12 @@ mod tests { keystrokes: &parse_keystrokes("ctrl-a"), action_name: "zed::SomeNonexistentAction", context: None, - use_key_equivalents: false, action_arguments: None, }, source: KeybindUpdateTarget { keystrokes: &parse_keystrokes("ctrl-b"), action_name: "zed::SomeOtherAction", context: None, - use_key_equivalents: false, action_arguments: None, }, target_keybind_source: KeybindSource::User, @@ -1247,14 +1264,12 @@ mod tests { keystrokes: &parse_keystrokes("ctrl-a"), action_name: "zed::SomeAction", context: None, - use_key_equivalents: false, action_arguments: None, }, source: KeybindUpdateTarget { keystrokes: &parse_keystrokes("ctrl-b"), action_name: "zed::SomeOtherAction", context: None, - use_key_equivalents: false, action_arguments: Some(r#"{"foo": "bar"}"#), }, target_keybind_source: KeybindSource::User, @@ -1292,14 +1307,12 @@ mod tests { keystrokes: &parse_keystrokes("a"), action_name: "foo::bar", context: Some("SomeContext"), - use_key_equivalents: false, action_arguments: None, }, source: KeybindUpdateTarget { keystrokes: &parse_keystrokes("c"), action_name: "foo::baz", context: Some("SomeOtherContext"), - use_key_equivalents: false, action_arguments: None, }, target_keybind_source: KeybindSource::User, @@ -1336,14 +1349,12 @@ mod tests { keystrokes: &parse_keystrokes("a"), action_name: "foo::bar", context: Some("SomeContext"), - use_key_equivalents: false, action_arguments: None, }, source: KeybindUpdateTarget { keystrokes: &parse_keystrokes("c"), action_name: "foo::baz", context: Some("SomeOtherContext"), - use_key_equivalents: false, action_arguments: None, }, target_keybind_source: KeybindSource::User, @@ -1375,7 +1386,6 @@ mod tests { context: Some("SomeContext"), keystrokes: &parse_keystrokes("a"), action_name: "foo::bar", - use_key_equivalents: false, action_arguments: None, }, target_keybind_source: KeybindSource::User, @@ -1407,7 +1417,6 @@ mod tests { context: Some("SomeContext"), keystrokes: &parse_keystrokes("a"), action_name: "foo::bar", - use_key_equivalents: false, action_arguments: Some("true"), }, target_keybind_source: KeybindSource::User, @@ -1450,7 +1459,6 @@ mod tests { context: Some("SomeContext"), keystrokes: &parse_keystrokes("a"), action_name: "foo::bar", - use_key_equivalents: false, action_arguments: Some("true"), }, target_keybind_source: KeybindSource::User, @@ -1472,4 +1480,54 @@ mod tests { .unindent(), ); } + + #[test] + fn test_append() { + check_keymap_update( + r#"[ + { + "context": "SomeOtherContext", + "use_key_equivalents": true, + "bindings": { + "b": "foo::bar", + } + }, + ]"# + .unindent(), + KeybindUpdateOperation::Add { + source: KeybindUpdateTarget { + context: Some("SomeContext"), + keystrokes: &parse_keystrokes("a"), + action_name: "foo::baz", + action_arguments: Some("true"), + }, + from: Some(KeybindUpdateTarget { + context: Some("SomeOtherContext"), + keystrokes: &parse_keystrokes("b"), + action_name: "foo::bar", + action_arguments: None, + }), + }, + r#"[ + { + "context": "SomeOtherContext", + "use_key_equivalents": true, + "bindings": { + "b": "foo::bar", + } + }, + { + "context": "SomeContext", + "use_key_equivalents": true, + "bindings": { + "a": [ + "foo::baz", + true + ] + } + } + ]"# + .unindent(), + ); + } } diff --git a/crates/settings/src/settings_json.rs b/crates/settings/src/settings_json.rs index 1aed18b44a..a448eb2737 100644 --- a/crates/settings/src/settings_json.rs +++ b/crates/settings/src/settings_json.rs @@ -437,17 +437,19 @@ pub fn append_top_level_array_value_in_json_text( ); debug_assert_eq!(cursor.node().kind(), "]"); let close_bracket_start = cursor.node().start_byte(); - cursor.goto_previous_sibling(); - while (cursor.node().is_extra() || cursor.node().is_missing()) && cursor.goto_previous_sibling() - { - } + while cursor.goto_previous_sibling() + && (cursor.node().is_extra() || cursor.node().is_missing()) + && !cursor.node().is_error() + {} let mut comma_range = None; let mut prev_item_range = None; - if cursor.node().kind() == "," { + if cursor.node().kind() == "," || is_error_of_kind(&mut cursor, ",") { comma_range = Some(cursor.node().byte_range()); - while cursor.goto_previous_sibling() && cursor.node().is_extra() {} + while cursor.goto_previous_sibling() + && (cursor.node().is_extra() || cursor.node().is_missing()) + {} debug_assert_ne!(cursor.node().kind(), "["); prev_item_range = Some(cursor.node().range()); @@ -514,6 +516,17 @@ pub fn append_top_level_array_value_in_json_text( replace_value.push('\n'); } return Ok((replace_range, replace_value)); + + fn is_error_of_kind(cursor: &mut tree_sitter::TreeCursor<'_>, kind: &str) -> bool { + if cursor.node().kind() != "ERROR" { + return false; + } + + let descendant_index = cursor.descendant_index(); + let res = cursor.goto_first_child() && cursor.node().kind() == kind; + cursor.goto_descendant(descendant_index); + return res; + } } pub fn to_pretty_json( diff --git a/crates/settings_ui/src/keybindings.rs b/crates/settings_ui/src/keybindings.rs index 5b2cca92bb..4526b7fcc8 100644 --- a/crates/settings_ui/src/keybindings.rs +++ b/crates/settings_ui/src/keybindings.rs @@ -1,5 +1,5 @@ use std::{ - ops::{Not, Range}, + ops::{Not as _, Range}, sync::Arc, }; @@ -1602,32 +1602,45 @@ impl KeybindingEditorModal { Ok(action_arguments) } - fn save(&mut self, cx: &mut Context) { - let existing_keybind = self.editing_keybind.clone(); - let fs = self.fs.clone(); + fn validate_keystrokes(&self, cx: &App) -> anyhow::Result> { let new_keystrokes = self .keybind_editor .read_with(cx, |editor, _| editor.keystrokes().to_vec()); - if new_keystrokes.is_empty() { - self.set_error(InputError::error("Keystrokes cannot be empty"), cx); - return; - } - let tab_size = cx.global::().json_tab_size(); + anyhow::ensure!(!new_keystrokes.is_empty(), "Keystrokes cannot be empty"); + Ok(new_keystrokes) + } + + fn validate_context(&self, cx: &App) -> anyhow::Result> { let new_context = self .context_editor .read_with(cx, |input, cx| input.editor().read(cx).text(cx)); - let new_context = new_context.is_empty().not().then_some(new_context); - let new_context_err = new_context.as_deref().and_then(|context| { - gpui::KeyBindingContextPredicate::parse(context) - .context("Failed to parse key context") - .err() - }); - if let Some(err) = new_context_err { - // TODO: store and display as separate error - // TODO: also, should be validating on keystroke - self.set_error(InputError::error(err.to_string()), cx); - return; - } + let Some(context) = new_context.is_empty().not().then_some(new_context) else { + return Ok(None); + }; + gpui::KeyBindingContextPredicate::parse(&context).context("Failed to parse key context")?; + + Ok(Some(context)) + } + + fn save(&mut self, cx: &mut Context) { + let existing_keybind = self.editing_keybind.clone(); + let fs = self.fs.clone(); + let tab_size = cx.global::().json_tab_size(); + let new_keystrokes = match self.validate_keystrokes(cx) { + Err(err) => { + self.set_error(InputError::error(err.to_string()), cx); + return; + } + Ok(keystrokes) => keystrokes, + }; + + let new_context = match self.validate_context(cx) { + Err(err) => { + self.set_error(InputError::error(err.to_string()), cx); + return; + } + Ok(context) => context, + }; let new_action_args = match self.validate_action_arguments(cx) { Err(input_err) => { @@ -2064,46 +2077,45 @@ async fn save_keybinding_update( .await .context("Failed to load keymap file")?; - let operation = if !create { - let existing_keystrokes = existing.keystrokes().unwrap_or_default(); - let existing_context = existing - .context - .as_ref() - .and_then(KeybindContextString::local_str); - let existing_args = existing - .action_arguments - .as_ref() - .map(|args| args.text.as_ref()); + let existing_keystrokes = existing.keystrokes().unwrap_or_default(); + let existing_context = existing + .context + .as_ref() + .and_then(KeybindContextString::local_str); + let existing_args = existing + .action_arguments + .as_ref() + .map(|args| args.text.as_ref()); + let target = settings::KeybindUpdateTarget { + context: existing_context, + keystrokes: existing_keystrokes, + action_name: &existing.action_name, + action_arguments: existing_args, + }; + + let source = settings::KeybindUpdateTarget { + context: new_context, + keystrokes: new_keystrokes, + action_name: &existing.action_name, + action_arguments: new_args, + }; + + let operation = if !create { settings::KeybindUpdateOperation::Replace { - target: settings::KeybindUpdateTarget { - context: existing_context, - keystrokes: existing_keystrokes, - action_name: &existing.action_name, - use_key_equivalents: false, - action_arguments: existing_args, - }, + target, target_keybind_source: existing .source .as_ref() .map(|(source, _name)| *source) .unwrap_or(KeybindSource::User), - source: settings::KeybindUpdateTarget { - context: new_context, - keystrokes: new_keystrokes, - action_name: &existing.action_name, - use_key_equivalents: false, - action_arguments: new_args, - }, + source, } } else { - settings::KeybindUpdateOperation::Add(settings::KeybindUpdateTarget { - context: new_context, - keystrokes: new_keystrokes, - action_name: &existing.action_name, - use_key_equivalents: false, - action_arguments: new_args, - }) + settings::KeybindUpdateOperation::Add { + source, + from: Some(target), + } }; let updated_keymap_contents = settings::KeymapFile::update_keybinding(operation, keymap_contents, tab_size) @@ -2137,7 +2149,6 @@ async fn remove_keybinding( .and_then(KeybindContextString::local_str), keystrokes, action_name: &existing.action_name, - use_key_equivalents: false, action_arguments: existing .action_arguments .as_ref()