From 7915b9f93f5c096dad26a5e535ebd793e84ad964 Mon Sep 17 00:00:00 2001 From: Ben Kunkle Date: Thu, 10 Jul 2025 18:23:26 -0500 Subject: [PATCH] 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 | 239 ++++++++++++++++++++++---- crates/settings/src/settings_json.rs | 161 ++++++++++++----- crates/settings_ui/src/keybindings.rs | 91 ++++++++-- 3 files changed, 401 insertions(+), 90 deletions(-) diff --git a/crates/settings/src/keymap_file.rs b/crates/settings/src/keymap_file.rs index 19bc58ea23..98dbe4d02a 100644 --- a/crates/settings/src/keymap_file.rs +++ b/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) = §ion.bindings else { - continue; - }; - for (keystrokes, action) in bindings { - let Ok(keystrokes) = keystrokes - .split_whitespace() - .map(Keystroke::parse) - .collect::, _>>() - 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) = §ion.bindings else { + continue; + }; + for (keystrokes_str, action) in bindings { + let Ok(keystrokes) = keystrokes_str + .split_whitespace() + .map(Keystroke::parse) + .collect::, _>>() + 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(), + ); } } diff --git a/crates/settings/src/settings_json.rs b/crates/settings/src/settings_json.rs index f569a18769..1aed18b44a 100644 --- a/crates/settings/src/settings_json.rs +++ b/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) = - replace_value_in_json_text(value_str, key_path, tab_size, new_value, replace_key); - - 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'); + if new_value.is_none() && key_path.is_empty() { + let mut remove_range = text_range.clone(); + if index == 0 { + while cursor.goto_next_sibling() + && (cursor.node().is_extra() || cursor.node().is_missing()) + {} + if cursor.node().kind() == "," { + remove_range.end = cursor.node().range().end_byte; + } + 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 ") { - replace_value.remove(idx + 1); - } - while let Some(idx) = replace_value.find("\n") { - replace_value.replace_range(idx..idx + 1, " "); - } - } + let (mut replace_range, mut replace_value) = + replace_value_in_json_text(value_str, key_path, tab_size, new_value, replace_key); - 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, 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]"#, 2, &[], 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,]"#, 100, &[], json!(2), r#"[1, 3, 3, 2]"#); + check_array_replace(r#"[1, 3, 3]"#, 1, &[], Some(json!(2)), r#"[1, 2, 3]"#); + check_array_replace(r#"[1, 3, 3]"#, 2, &[], Some(json!(2)), r#"[1, 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, &[], 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] diff --git a/crates/settings_ui/src/keybindings.rs b/crates/settings_ui/src/keybindings.rs index 1f6c0ba8c7..58c2ac8f8e 100644 --- a/crates/settings_ui/src/keybindings.rs +++ b/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) { + 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::().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, + 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, 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)) - .action("Create", Box::new(CreateBinding)) - .action("Copy action", Box::new(CopyAction)) - .action_disabled_when( - selected_binding_has_no_context, - "Copy Context", - Box::new(CopyContext), - ) + menu.action_disabled_when( + selected_binding_is_unbound_action, + "Edit", + Box::new(EditBinding), + ) + .action("Create", Box::new(CreateBinding)) + .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), + ) }) }