From 206cce67831c4dd387e9e67887ef3c4f1e438ae9 Mon Sep 17 00:00:00 2001 From: Ben Kunkle Date: Fri, 11 Jul 2025 16:23:14 -0500 Subject: [PATCH] keymap_ui: Support unbinding non-user defined keybindings (#34318) Closes #ISSUE Makes it so that `KeymapFile::update_keybinding` treats removals of bindings that weren't user-defined as creating a new binding to `zed::NoAction`. Release Notes: - N/A *or* Added/Fixed/Improved ... --- Cargo.lock | 1 + crates/settings/src/keymap_file.rs | 21 +++--- crates/settings_ui/Cargo.toml | 1 + crates/settings_ui/src/keybindings.rs | 97 ++++++++++++++++----------- 4 files changed, 73 insertions(+), 47 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 624126c163..0539328576 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -14677,6 +14677,7 @@ dependencies = [ "schemars", "search", "serde", + "serde_json", "settings", "theme", "tree-sitter-json", diff --git a/crates/settings/src/keymap_file.rs b/crates/settings/src/keymap_file.rs index 102522f71d..78e306ed63 100644 --- a/crates/settings/src/keymap_file.rs +++ b/crates/settings/src/keymap_file.rs @@ -607,8 +607,8 @@ impl KeymapFile { mut keymap_contents: String, tab_size: usize, ) -> Result { - // if trying to replace a keybinding that is not user-defined, treat it as an add operation match operation { + // if trying to replace a keybinding that is not user-defined, treat it as an add operation KeybindUpdateOperation::Replace { target_keybind_source: target_source, source, @@ -616,6 +616,16 @@ impl KeymapFile { } if target_source != KeybindSource::User => { operation = KeybindUpdateOperation::Add(source); } + // 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_keybind_source, + } if target_keybind_source != KeybindSource::User => { + target.action_name = gpui::NoAction.name(); + target.input.take(); + operation = KeybindUpdateOperation::Add(target); + } _ => {} } @@ -623,14 +633,7 @@ 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"); - } + if let KeybindUpdateOperation::Remove { target, .. } = operation { let target_action_value = target .action_value() .context("Failed to generate target action JSON value")?; diff --git a/crates/settings_ui/Cargo.toml b/crates/settings_ui/Cargo.toml index 7af240bd74..2791876117 100644 --- a/crates/settings_ui/Cargo.toml +++ b/crates/settings_ui/Cargo.toml @@ -31,6 +31,7 @@ project.workspace = true schemars.workspace = true search.workspace = true serde.workspace = true +serde_json.workspace = true settings.workspace = true theme.workspace = true tree-sitter-json.workspace = true diff --git a/crates/settings_ui/src/keybindings.rs b/crates/settings_ui/src/keybindings.rs index 1718f3d283..7fc2e07087 100644 --- a/crates/settings_ui/src/keybindings.rs +++ b/crates/settings_ui/src/keybindings.rs @@ -397,11 +397,10 @@ impl KeymapEditor { SearchMode::KeyStroke => { matches.retain(|item| { this.keybindings[item.candidate_id] - .ui_key_binding - .as_ref() - .is_some_and(|binding| { + .keystrokes() + .is_some_and(|keystrokes| { keystroke_query.iter().all(|key| { - binding.keystrokes.iter().any(|keystroke| { + keystrokes.iter().any(|keystroke| { keystroke.key == key.key && keystroke.modifiers == key.modifiers }) @@ -623,7 +622,7 @@ impl KeymapEditor { .and_then(KeybindContextString::local) .is_none(); - let selected_binding_is_unbound = selected_binding.ui_key_binding.is_none(); + let selected_binding_is_unbound = selected_binding.keystrokes().is_none(); let context_menu = ContextMenu::build(window, cx, |menu, _window, _cx| { menu.action_disabled_when( @@ -876,6 +875,12 @@ impl ProcessedKeybinding { .cloned(), ) } + + fn keystrokes(&self) -> Option<&[Keystroke]> { + self.ui_key_binding + .as_ref() + .map(|binding| binding.keystrokes.as_slice()) + } } #[derive(Clone, Debug, IntoElement, PartialEq, Eq, Hash)] @@ -1303,16 +1308,8 @@ impl KeybindingEditorModal { window: &mut Window, cx: &mut App, ) -> Self { - let keybind_editor = cx.new(|cx| { - KeystrokeInput::new( - editing_keybind - .ui_key_binding - .as_ref() - .map(|keybinding| keybinding.keystrokes.clone()), - window, - cx, - ) - }); + let keybind_editor = cx + .new(|cx| KeystrokeInput::new(editing_keybind.keystrokes().map(Vec::from), window, cx)); let context_editor = cx.new(|cx| { let mut editor = Editor::single_line(window, cx); @@ -1398,6 +1395,24 @@ impl KeybindingEditorModal { } } + fn validate_action_input(&self, cx: &App) -> anyhow::Result> { + let input = self + .input_editor + .as_ref() + .map(|editor| editor.read(cx).text(cx)); + + let value = input + .as_ref() + .map(|input| { + serde_json::from_str(input).context("Failed to parse action input as JSON") + }) + .transpose()?; + + cx.build_action(&self.editing_keybind.action_name, value) + .context("Failed to validate action input")?; + Ok(input) + } + fn save(&mut self, cx: &mut Context) { let existing_keybind = self.editing_keybind.clone(); let fs = self.fs.clone(); @@ -1425,6 +1440,14 @@ impl KeybindingEditorModal { return; } + let new_input = match self.validate_action_input(cx) { + Err(input_err) => { + self.set_error(InputError::error(input_err.to_string()), cx); + return; + } + Ok(input) => input, + }; + let action_mapping: ActionMapping = ( ui::text_for_keystrokes(&new_keystrokes, cx).into(), new_context @@ -1481,6 +1504,7 @@ impl KeybindingEditorModal { existing_keybind, &new_keystrokes, new_context.as_deref(), + new_input.as_deref(), &fs, tab_size, ) @@ -1711,6 +1735,7 @@ async fn save_keybinding_update( existing: ProcessedKeybinding, new_keystrokes: &[Keystroke], new_context: Option<&str>, + new_input: Option<&str>, fs: &Arc, tab_size: usize, ) -> anyhow::Result<()> { @@ -1718,41 +1743,36 @@ async fn save_keybinding_update( .await .context("Failed to load keymap file")?; - let existing_keystrokes = existing - .ui_key_binding - .as_ref() - .map(|keybinding| keybinding.keystrokes.as_slice()) - .unwrap_or_default(); - - let existing_context = existing - .context - .as_ref() - .and_then(KeybindContextString::local_str); - - let input = existing - .action_input - .as_ref() - .map(|input| input.text.as_ref()); - 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_input = existing + .action_input + .as_ref() + .map(|input| input.text.as_ref()); + settings::KeybindUpdateOperation::Replace { target: settings::KeybindUpdateTarget { context: existing_context, keystrokes: existing_keystrokes, action_name: &existing.action_name, use_key_equivalents: false, - input, + input: existing_input, }, target_keybind_source: existing .source - .map(|(source, _name)| 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, - input, + input: new_input, }, } } else { @@ -1761,7 +1781,7 @@ async fn save_keybinding_update( keystrokes: new_keystrokes, action_name: &existing.action_name, use_key_equivalents: false, - input, + input: new_input, }) }; let updated_keymap_contents = @@ -1778,7 +1798,7 @@ async fn remove_keybinding( fs: &Arc, tab_size: usize, ) -> anyhow::Result<()> { - let Some(ui_key_binding) = existing.ui_key_binding else { + let Some(keystrokes) = existing.keystrokes() else { anyhow::bail!("Cannot remove a keybinding that does not exist"); }; let keymap_contents = settings::KeymapFile::load_keymap_file(fs) @@ -1791,7 +1811,7 @@ async fn remove_keybinding( .context .as_ref() .and_then(KeybindContextString::local_str), - keystrokes: &ui_key_binding.keystrokes, + keystrokes, action_name: &existing.action_name, use_key_equivalents: false, input: existing @@ -1801,7 +1821,8 @@ async fn remove_keybinding( }, target_keybind_source: existing .source - .map(|(source, _name)| source) + .as_ref() + .map(|(source, _name)| *source) .unwrap_or(KeybindSource::User), };