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

@@ -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();
+                let mut source = target.clone();
-                target.action_arguments.take();
+                source.action_name = gpui::NoAction.name();
-                operation = KeybindUpdateOperation::Add(target);
+                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) = &section.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<KeybindUpdateTarget<'a>>,
+    },
     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<KeybindSource> 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<Keystroke> {
+        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<Keystroke> {
-            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(),
+        );
+    }
 }

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.goto_previous_sibling()
-    while (cursor.node().is_extra() || cursor.node().is_missing()) && 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(

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<Self>) {
+    fn validate_keystrokes(&self, cx: &App) -> anyhow::Result<Vec<Keystroke>> {
-        let existing_keybind = self.editing_keybind.clone();
-        let fs = self.fs.clone();
         let new_keystrokes = self
             .keybind_editor
             .read_with(cx, |editor, _| editor.keystrokes().to_vec());
-        if new_keystrokes.is_empty() {
+        anyhow::ensure!(!new_keystrokes.is_empty(), "Keystrokes cannot be empty");
-            self.set_error(InputError::error("Keystrokes cannot be empty"), cx);
+        Ok(new_keystrokes)
-            return;
+    }
-        }
+
-        let tab_size = cx.global::<settings::SettingsStore>().json_tab_size();
+    fn validate_context(&self, cx: &App) -> anyhow::Result<Option<String>> {
         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 Some(context) = new_context.is_empty().not().then_some(new_context) else {
-        let new_context_err = new_context.as_deref().and_then(|context| {
+            return Ok(None);
-            gpui::KeyBindingContextPredicate::parse(context)
+        };
-                .context("Failed to parse key context")
+        gpui::KeyBindingContextPredicate::parse(&context).context("Failed to parse key context")?;
-                .err()
+
-        });
+        Ok(Some(context))
-        if let Some(err) = new_context_err {
+    }
-            // TODO: store and display as separate error
+
-            // TODO: also, should be validating on keystroke
+    fn save(&mut self, cx: &mut Context<Self>) {
-            self.set_error(InputError::error(err.to_string()), cx);
+        let existing_keybind = self.editing_keybind.clone();
-            return;
+        let fs = self.fs.clone();
-        }
+        let tab_size = cx.global::<settings::SettingsStore>().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_keystrokes = existing.keystrokes().unwrap_or_default();
+    let existing_context = existing
-        let existing_context = existing
+        .context
-            .context
+        .as_ref()
-            .as_ref()
+        .and_then(KeybindContextString::local_str);
-            .and_then(KeybindContextString::local_str);
+    let existing_args = existing
-        let existing_args = existing
+        .action_arguments
-            .action_arguments
+        .as_ref()
-            .as_ref()
+        .map(|args| args.text.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 {
+            target,
-                context: existing_context,
-                keystrokes: existing_keystrokes,
-                action_name: &existing.action_name,
-                use_key_equivalents: false,
-                action_arguments: existing_args,
-            },
             target_keybind_source: existing
                 .source
                 .as_ref()
                 .map(|(source, _name)| *source)
                 .unwrap_or(KeybindSource::User),
-            source: settings::KeybindUpdateTarget {
+            source,
-                context: new_context,
-                keystrokes: new_keystrokes,
-                action_name: &existing.action_name,
-                use_key_equivalents: false,
-                action_arguments: new_args,
-            },
         }
     } else {
-        settings::KeybindUpdateOperation::Add(settings::KeybindUpdateTarget {
+        settings::KeybindUpdateOperation::Add {
-            context: new_context,
+            source,
-            keystrokes: new_keystrokes,
+            from: Some(target),
-            action_name: &existing.action_name,
+        }
-            use_key_equivalents: false,
-            action_arguments: new_args,
-        })
     };
     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()