keymap_ui: Add ability to delete user created bindings (#34248)

Closes #ISSUE

Adds an action and special handling in `KeymapFile::update_keybinding`
for removals. If the binding being removed is the last in a keymap
section, the keymap section will be removed entirely instead of left
empty.

Still to do is the ability to unbind/remove non-user created bindings
such as those in the default keymap by binding them to `NoAction`,
however, this will be done in a follow up PR.

Release Notes:

- N/A *or* Added/Fixed/Improved ...

crates/settings/src/keymap_file.rs

@@ -623,49 +623,55 @@ impl KeymapFile {
         // We don't want to modify the file if it's invalid.
         let keymap = Self::parse(&keymap_contents).context("Failed to parse keymap")?;
 
+        if let KeybindUpdateOperation::Remove {
+            target,
+            target_keybind_source,
+        } = operation
+        {
+            if target_keybind_source != KeybindSource::User {
+                anyhow::bail!("Cannot remove non-user created keybinding. Not implemented yet");
+            }
+            let target_action_value = target
+                .action_value()
+                .context("Failed to generate target action JSON value")?;
+            let Some((index, keystrokes_str)) =
+                find_binding(&keymap, &target, &target_action_value)
+            else {
+                anyhow::bail!("Failed to find keybinding to remove");
+            };
+            let is_only_binding = keymap.0[index]
+                .bindings
+                .as_ref()
+                .map_or(true, |bindings| bindings.len() == 1);
+            let key_path: &[&str] = if is_only_binding {
+                &[]
+            } else {
+                &["bindings", keystrokes_str]
+            };
+            let (replace_range, replace_value) = replace_top_level_array_value_in_json_text(
+                &keymap_contents,
+                key_path,
+                None,
+                None,
+                index,
+                tab_size,
+            )
+            .context("Failed to remove keybinding")?;
+            keymap_contents.replace_range(replace_range, &replace_value);
+            return Ok(keymap_contents);
+        }
+
         if let KeybindUpdateOperation::Replace { source, target, .. } = operation {
-            let mut found_index = None;
             let target_action_value = target
                 .action_value()
                 .context("Failed to generate target action JSON value")?;
             let source_action_value = source
                 .action_value()
                 .context("Failed to generate source action JSON value")?;
-            'sections: for (index, section) in keymap.sections().enumerate() {
-                if section.context != target.context.unwrap_or("") {
-                    continue;
-                }
-                if section.use_key_equivalents != target.use_key_equivalents {
-                    continue;
-                }
-                let Some(bindings) = &section.bindings else {
-                    continue;
-                };
-                for (keystrokes, action) in bindings {
-                    let Ok(keystrokes) = keystrokes
-                        .split_whitespace()
-                        .map(Keystroke::parse)
-                        .collect::<Result<Vec<_>, _>>()
-                    else {
-                        continue;
-                    };
-                    if keystrokes.len() != target.keystrokes.len()
-                        || !keystrokes
-                            .iter()
-                            .zip(target.keystrokes)
-                            .all(|(a, b)| a.should_match(b))
-                    {
-                        continue;
-                    }
-                    if action.0 != target_action_value {
-                        continue;
-                    }
-                    found_index = Some(index);
-                    break 'sections;
-                }
-            }
 
-            if let Some(index) = found_index {
+            if let Some((index, keystrokes_str)) =
+                find_binding(&keymap, &target, &target_action_value)
+            {
                 if target.context == source.context {
                     // if we are only changing the keybinding (common case)
                     // not the context, etc. Then just update the binding in place
@@ -673,7 +679,7 @@ impl KeymapFile {
                     let (replace_range, replace_value) =
                         replace_top_level_array_value_in_json_text(
                             &keymap_contents,
-                            &["bindings", &target.keystrokes_unparsed()],
+                            &["bindings", keystrokes_str],
                             Some(&source_action_value),
                             Some(&source.keystrokes_unparsed()),
                             index,
@@ -695,7 +701,7 @@ impl KeymapFile {
                     let (replace_range, replace_value) =
                         replace_top_level_array_value_in_json_text(
                             &keymap_contents,
-                            &["bindings", &target.keystrokes_unparsed()],
+                            &["bindings", keystrokes_str],
                             Some(&source_action_value),
                             Some(&source.keystrokes_unparsed()),
                             index,
@@ -725,7 +731,7 @@ impl KeymapFile {
                     let (replace_range, replace_value) =
                         replace_top_level_array_value_in_json_text(
                             &keymap_contents,
-                            &["bindings", &target.keystrokes_unparsed()],
+                            &["bindings", keystrokes_str],
                             None,
                             None,
                             index,
@@ -771,6 +777,46 @@ impl KeymapFile {
             keymap_contents.replace_range(replace_range, &replace_value);
         }
         return Ok(keymap_contents);
+
+        fn find_binding<'a, 'b>(
+            keymap: &'b KeymapFile,
+            target: &KeybindUpdateTarget<'a>,
+            target_action_value: &Value,
+        ) -> Option<(usize, &'b str)> {
+            for (index, section) in keymap.sections().enumerate() {
+                if section.context != target.context.unwrap_or("") {
+                    continue;
+                }
+                if section.use_key_equivalents != target.use_key_equivalents {
+                    continue;
+                }
+                let Some(bindings) = &section.bindings else {
+                    continue;
+                };
+                for (keystrokes_str, action) in bindings {
+                    let Ok(keystrokes) = keystrokes_str
+                        .split_whitespace()
+                        .map(Keystroke::parse)
+                        .collect::<Result<Vec<_>, _>>()
+                    else {
+                        continue;
+                    };
+                    if keystrokes.len() != target.keystrokes.len()
+                        || !keystrokes
+                            .iter()
+                            .zip(target.keystrokes)
+                            .all(|(a, b)| a.should_match(b))
+                    {
+                        continue;
+                    }
+                    if &action.0 != target_action_value {
+                        continue;
+                    }
+                    return Some((index, &keystrokes_str));
+                }
+            }
+            None
+        }
     }
 }
 
@@ -783,6 +829,10 @@ pub enum KeybindUpdateOperation<'a> {
         target_keybind_source: KeybindSource,
     },
     Add(KeybindUpdateTarget<'a>),
+    Remove {
+        target: KeybindUpdateTarget<'a>,
+        target_keybind_source: KeybindSource,
+    },
 }
 
 pub struct KeybindUpdateTarget<'a> {
@@ -1300,5 +1350,118 @@ mod tests {
             ]"#
             .unindent(),
         );
+
+        check_keymap_update(
+            r#"[
+                {
+                    "context": "SomeContext",
+                    "bindings": {
+                        "a": "foo::bar",
+                        "c": "foo::baz",
+                    }
+                },
+            ]"#
+            .unindent(),
+            KeybindUpdateOperation::Remove {
+                target: KeybindUpdateTarget {
+                    context: Some("SomeContext"),
+                    keystrokes: &parse_keystrokes("a"),
+                    action_name: "foo::bar",
+                    use_key_equivalents: false,
+                    input: None,
+                },
+                target_keybind_source: KeybindSource::User,
+            },
+            r#"[
+                {
+                    "context": "SomeContext",
+                    "bindings": {
+                        "c": "foo::baz",
+                    }
+                },
+            ]"#
+            .unindent(),
+        );
+
+        check_keymap_update(
+            r#"[
+                {
+                    "context": "SomeContext",
+                    "bindings": {
+                        "a": ["foo::bar", true],
+                        "c": "foo::baz",
+                    }
+                },
+            ]"#
+            .unindent(),
+            KeybindUpdateOperation::Remove {
+                target: KeybindUpdateTarget {
+                    context: Some("SomeContext"),
+                    keystrokes: &parse_keystrokes("a"),
+                    action_name: "foo::bar",
+                    use_key_equivalents: false,
+                    input: Some("true"),
+                },
+                target_keybind_source: KeybindSource::User,
+            },
+            r#"[
+                {
+                    "context": "SomeContext",
+                    "bindings": {
+                        "c": "foo::baz",
+                    }
+                },
+            ]"#
+            .unindent(),
+        );
+
+        check_keymap_update(
+            r#"[
+                {
+                    "context": "SomeContext",
+                    "bindings": {
+                        "b": "foo::baz",
+                    }
+                },
+                {
+                    "context": "SomeContext",
+                    "bindings": {
+                        "a": ["foo::bar", true],
+                    }
+                },
+                {
+                    "context": "SomeContext",
+                    "bindings": {
+                        "c": "foo::baz",
+                    }
+                },
+            ]"#
+            .unindent(),
+            KeybindUpdateOperation::Remove {
+                target: KeybindUpdateTarget {
+                    context: Some("SomeContext"),
+                    keystrokes: &parse_keystrokes("a"),
+                    action_name: "foo::bar",
+                    use_key_equivalents: false,
+                    input: Some("true"),
+                },
+                target_keybind_source: KeybindSource::User,
+            },
+            r#"[
+                {
+                    "context": "SomeContext",
+                    "bindings": {
+                        "b": "foo::baz",
+                    }
+                },
+                {
+                    "context": "SomeContext",
+                    "bindings": {
+                        "c": "foo::baz",
+                    }
+                },
+            ]"#
+            .unindent(),
+        );
     }
 }

crates/settings/src/settings_json.rs

@@ -353,29 +353,58 @@ pub fn replace_top_level_array_value_in_json_text(
     let range = cursor.node().range();
     let indent_width = range.start_point.column;
     let offset = range.start_byte;
-    let value_str = &text[range.start_byte..range.end_byte];
+    let text_range = range.start_byte..range.end_byte;
+    let value_str = &text[text_range.clone()];
     let needs_indent = range.start_point.row > 0;
 
-    let (mut replace_range, mut replace_value) =
+    if new_value.is_none() && key_path.is_empty() {
-        replace_value_in_json_text(value_str, key_path, tab_size, new_value, replace_key);
+        let mut remove_range = text_range.clone();
-
+        if index == 0 {
-    replace_range.start += offset;
+            while cursor.goto_next_sibling()
-    replace_range.end += offset;
+                && (cursor.node().is_extra() || cursor.node().is_missing())
-
+            {}
-    if needs_indent {
+            if cursor.node().kind() == "," {
-        let increased_indent = format!("\n{space:width$}", space = ' ', width = indent_width);
+                remove_range.end = cursor.node().range().end_byte;
-        replace_value = replace_value.replace('\n', &increased_indent);
+            }
-        // replace_value.push('\n');
+            if let Some(next_newline) = &text[remove_range.end + 1..].find('\n') {
+                if text[remove_range.end + 1..remove_range.end + next_newline]
+                    .chars()
+                    .all(|c| c.is_ascii_whitespace())
+                {
+                    remove_range.end = remove_range.end + next_newline;
+                }
+            }
+        } else {
+            while cursor.goto_previous_sibling()
+                && (cursor.node().is_extra() || cursor.node().is_missing())
+            {}
+            if cursor.node().kind() == "," {
+                remove_range.start = cursor.node().range().start_byte;
+            }
+        }
+        return Ok((remove_range, String::new()));
     } else {
-        while let Some(idx) = replace_value.find("\n ") {
+        let (mut replace_range, mut replace_value) =
-            replace_value.remove(idx + 1);
+            replace_value_in_json_text(value_str, key_path, tab_size, new_value, replace_key);
-        }
-        while let Some(idx) = replace_value.find("\n") {
-            replace_value.replace_range(idx..idx + 1, " ");
-        }
-    }
 
-    return Ok((replace_range, replace_value));
+        replace_range.start += offset;
+        replace_range.end += offset;
+
+        if needs_indent {
+            let increased_indent = format!("\n{space:width$}", space = ' ', width = indent_width);
+            replace_value = replace_value.replace('\n', &increased_indent);
+            // replace_value.push('\n');
+        } else {
+            while let Some(idx) = replace_value.find("\n ") {
+                replace_value.remove(idx + 1);
+            }
+            while let Some(idx) = replace_value.find("\n") {
+                replace_value.replace_range(idx..idx + 1, " ");
+            }
+        }
+
+        return Ok((replace_range, replace_value));
+    }
 }
 
 pub fn append_top_level_array_value_in_json_text(
@@ -1005,14 +1034,14 @@ mod tests {
             input: impl ToString,
             index: usize,
             key_path: &[&str],
-            value: Value,
+            value: Option<Value>,
             expected: impl ToString,
         ) {
             let input = input.to_string();
             let result = replace_top_level_array_value_in_json_text(
                 &input,
                 key_path,
-                Some(&value),
+                value.as_ref(),
                 None,
                 index,
                 4,
@@ -1023,10 +1052,10 @@ mod tests {
             pretty_assertions::assert_eq!(expected.to_string(), result_str);
         }
 
-        check_array_replace(r#"[1, 3, 3]"#, 1, &[], json!(2), r#"[1, 2, 3]"#);
+        check_array_replace(r#"[1, 3, 3]"#, 1, &[], Some(json!(2)), r#"[1, 2, 3]"#);
-        check_array_replace(r#"[1, 3, 3]"#, 2, &[], json!(2), r#"[1, 3, 2]"#);
+        check_array_replace(r#"[1, 3, 3]"#, 2, &[], Some(json!(2)), r#"[1, 3, 2]"#);
-        check_array_replace(r#"[1, 3, 3,]"#, 3, &[], json!(2), r#"[1, 3, 3, 2]"#);
+        check_array_replace(r#"[1, 3, 3,]"#, 3, &[], Some(json!(2)), r#"[1, 3, 3, 2]"#);
-        check_array_replace(r#"[1, 3, 3,]"#, 100, &[], json!(2), r#"[1, 3, 3, 2]"#);
+        check_array_replace(r#"[1, 3, 3,]"#, 100, &[], Some(json!(2)), r#"[1, 3, 3, 2]"#);
         check_array_replace(
             r#"[
                 1,
@@ -1036,7 +1065,7 @@ mod tests {
             .unindent(),
             1,
             &[],
-            json!({"foo": "bar", "baz": "qux"}),
+            Some(json!({"foo": "bar", "baz": "qux"})),
             r#"[
                 1,
                 {
@@ -1051,7 +1080,7 @@ mod tests {
             r#"[1, 3, 3,]"#,
             1,
             &[],
-            json!({"foo": "bar", "baz": "qux"}),
+            Some(json!({"foo": "bar", "baz": "qux"})),
             r#"[1, { "foo": "bar", "baz": "qux" }, 3,]"#,
         );
 
@@ -1059,7 +1088,7 @@ mod tests {
             r#"[1, { "foo": "bar", "baz": "qux" }, 3,]"#,
             1,
             &["baz"],
-            json!({"qux": "quz"}),
+            Some(json!({"qux": "quz"})),
             r#"[1, { "foo": "bar", "baz": { "qux": "quz" } }, 3,]"#,
         );
 
@@ -1074,7 +1103,7 @@ mod tests {
             ]"#,
             1,
             &["baz"],
-            json!({"qux": "quz"}),
+            Some(json!({"qux": "quz"})),
             r#"[
                 1,
                 {
@@ -1100,7 +1129,7 @@ mod tests {
             ]"#,
             1,
             &["baz"],
-            json!("qux"),
+            Some(json!("qux")),
             r#"[
                 1,
                 {
@@ -1127,7 +1156,7 @@ mod tests {
             ]"#,
             1,
             &["baz"],
-            json!("qux"),
+            Some(json!("qux")),
             r#"[
                 1,
                 {
@@ -1151,7 +1180,7 @@ mod tests {
             ]"#,
             2,
             &[],
-            json!("replaced"),
+            Some(json!("replaced")),
             r#"[
                 1,
                 // This is element 2
@@ -1169,7 +1198,7 @@ mod tests {
             .unindent(),
             0,
             &[],
-            json!("first"),
+            Some(json!("first")),
             r#"[
                 // Empty array with comment
                 "first"
@@ -1180,7 +1209,7 @@ mod tests {
             r#"[]"#.unindent(),
             0,
             &[],
-            json!("first"),
+            Some(json!("first")),
             r#"[
                 "first"
             ]"#
@@ -1197,7 +1226,7 @@ mod tests {
             ]"#,
             0,
             &[],
-            json!({"new": "object"}),
+            Some(json!({"new": "object"})),
             r#"[
                 // Leading comment
                 // Another leading comment
@@ -1217,7 +1246,7 @@ mod tests {
                     ]"#,
             1,
             &[],
-            json!("deep"),
+            Some(json!("deep")),
             r#"[
                         1,
                         "deep",
@@ -1230,7 +1259,7 @@ mod tests {
             r#"[1,2,   3,    4]"#,
             2,
             &[],
-            json!("spaced"),
+            Some(json!("spaced")),
             r#"[1,2,   "spaced",    4]"#,
         );
 
@@ -1243,7 +1272,7 @@ mod tests {
             ]"#,
             1,
             &[],
-            json!(["a", "b", "c", "d"]),
+            Some(json!(["a", "b", "c", "d"])),
             r#"[
                 [1, 2, 3],
                 [
@@ -1268,7 +1297,7 @@ mod tests {
             ]"#,
             0,
             &[],
-            json!("updated"),
+            Some(json!("updated")),
             r#"[
                 /*
                  * This is a
@@ -1284,7 +1313,7 @@ mod tests {
             r#"[true, false, true]"#,
             1,
             &[],
-            json!(null),
+            Some(json!(null)),
             r#"[true, null, true]"#,
         );
 
@@ -1293,7 +1322,7 @@ mod tests {
             r#"[42]"#,
             0,
             &[],
-            json!({"answer": 42}),
+            Some(json!({"answer": 42})),
             r#"[{ "answer": 42 }]"#,
         );
 
@@ -1307,7 +1336,7 @@ mod tests {
             .unindent(),
             10,
             &[],
-            json!(123),
+            Some(json!(123)),
             r#"[
                 // Comment 1
                 // Comment 2
@@ -1316,6 +1345,54 @@ mod tests {
             ]"#
             .unindent(),
         );
+
+        check_array_replace(
+            r#"[
+                {
+                    "key": "value"
+                },
+                {
+                    "key": "value2"
+                }
+            ]"#
+            .unindent(),
+            0,
+            &[],
+            None,
+            r#"[
+                {
+                    "key": "value2"
+                }
+            ]"#
+            .unindent(),
+        );
+
+        check_array_replace(
+            r#"[
+                {
+                    "key": "value"
+                },
+                {
+                    "key": "value2"
+                },
+                {
+                    "key": "value3"
+                },
+            ]"#
+            .unindent(),
+            1,
+            &[],
+            None,
+            r#"[
+                {
+                    "key": "value"
+                },
+                {
+                    "key": "value3"
+                },
+            ]"#
+            .unindent(),
+        );
     }
 
     #[test]

crates/settings_ui/src/keybindings.rs

@@ -23,7 +23,10 @@ use ui::{
     ActiveTheme as _, App, Banner, BorrowAppContext, ContextMenu, ParentElement as _, Render,
     SharedString, Styled as _, Tooltip, Window, prelude::*, right_click_menu,
 };
-use workspace::{Item, ModalView, SerializableItem, Workspace, register_serializable_item};
+use workspace::{
+    Item, ModalView, SerializableItem, Workspace, notifications::NotifyTaskExt as _,
+    register_serializable_item,
+};
 
 use crate::{
     SettingsUiFeatureFlag,
@@ -49,6 +52,8 @@ actions!(
         EditBinding,
         /// Creates a new key binding for the selected action.
         CreateBinding,
+        /// Deletes the selected key binding.
+        DeleteBinding,
         /// Copies the action name to clipboard.
         CopyAction,
         /// Copies the context predicate to clipboard.
@@ -613,6 +618,21 @@ impl KeymapEditor {
         self.open_edit_keybinding_modal(true, window, cx);
     }
 
+    fn delete_binding(&mut self, _: &DeleteBinding, window: &mut Window, cx: &mut Context<Self>) {
+        let Some(to_remove) = self.selected_binding().cloned() else {
+            return;
+        };
+        let Ok(fs) = self
+            .workspace
+            .read_with(cx, |workspace, _| workspace.app_state().fs.clone())
+        else {
+            return;
+        };
+        let tab_size = cx.global::<settings::SettingsStore>().json_tab_size();
+        cx.spawn(async move |_, _| remove_keybinding(to_remove, &fs, tab_size).await)
+            .detach_and_notify_err(window, cx);
+    }
+
     fn copy_context_to_clipboard(
         &mut self,
         _: &CopyContext,
@@ -740,6 +760,7 @@ impl Render for KeymapEditor {
             .on_action(cx.listener(Self::confirm))
             .on_action(cx.listener(Self::edit_binding))
             .on_action(cx.listener(Self::create_binding))
+            .on_action(cx.listener(Self::delete_binding))
             .on_action(cx.listener(Self::copy_action_to_clipboard))
             .on_action(cx.listener(Self::copy_context_to_clipboard))
             .size_full()
@@ -1458,6 +1479,47 @@ async fn save_keybinding_update(
     Ok(())
 }
 
+async fn remove_keybinding(
+    existing: ProcessedKeybinding,
+    fs: &Arc<dyn Fs>,
+    tab_size: usize,
+) -> anyhow::Result<()> {
+    let Some(ui_key_binding) = existing.ui_key_binding else {
+        anyhow::bail!("Cannot remove a keybinding that does not exist");
+    };
+    let keymap_contents = settings::KeymapFile::load_keymap_file(fs)
+        .await
+        .context("Failed to load keymap file")?;
+
+    let operation = settings::KeybindUpdateOperation::Remove {
+        target: settings::KeybindUpdateTarget {
+            context: existing
+                .context
+                .as_ref()
+                .and_then(KeybindContextString::local_str),
+            keystrokes: &ui_key_binding.keystrokes,
+            action_name: &existing.action_name,
+            use_key_equivalents: false,
+            input: existing
+                .action_input
+                .as_ref()
+                .map(|input| input.text.as_ref()),
+        },
+        target_keybind_source: existing
+            .source
+            .map(|(source, _name)| source)
+            .unwrap_or(KeybindSource::User),
+    };
+
+    let updated_keymap_contents =
+        settings::KeymapFile::update_keybinding(operation, keymap_contents, tab_size)
+            .context("Failed to update keybinding")?;
+    fs.atomic_write(paths::keymap_file().clone(), updated_keymap_contents)
+        .await
+        .context("Failed to write keymap file")?;
+    Ok(())
+}
+
 struct KeystrokeInput {
     keystrokes: Vec<Keystroke>,
     focus_handle: FocusHandle,
@@ -1667,16 +1729,25 @@ fn build_keybind_context_menu(
             .and_then(KeybindContextString::local)
             .is_none();
 
-        let selected_binding_is_unbound = selected_binding.ui_key_binding.is_none();
+        let selected_binding_is_unbound_action = selected_binding.ui_key_binding.is_none();
 
-        menu.action_disabled_when(selected_binding_is_unbound, "Edit", Box::new(EditBinding))
+        menu.action_disabled_when(
-            .action("Create", Box::new(CreateBinding))
+            selected_binding_is_unbound_action,
-            .action("Copy action", Box::new(CopyAction))
+            "Edit",
-            .action_disabled_when(
+            Box::new(EditBinding),
-                selected_binding_has_no_context,
+        )
-                "Copy Context",
+        .action("Create", Box::new(CreateBinding))
-                Box::new(CopyContext),
+        .action_disabled_when(
-            )
+            selected_binding_is_unbound_action,
+            "Delete",
+            Box::new(DeleteBinding),
+        )
+        .action("Copy action", Box::new(CopyAction))
+        .action_disabled_when(
+            selected_binding_has_no_context,
+            "Copy Context",
+            Box::new(CopyContext),
+        )
     })
 }