From de9053c7ca276adba5d1244fe9805bf331a3c646 Mon Sep 17 00:00:00 2001 From: Ben Kunkle Date: Mon, 7 Jul 2025 11:44:19 -0500 Subject: [PATCH] keymap_ui: Add ability to edit context (#34019) Closes #ISSUE Adds a context input to the keybind edit modal. Also fixes some bugs in the keymap update function to handle context changes gracefully. The current keybind update strategy implemented in this PR is * when the context doesn't change, just update the binding in place * when the context changes, but the binding is the only binding in the keymap section, update the binding _and_ context in place * when the context changes, and the binding is _not_ the only binding in the keymap section, remove the existing binding and create a new section with the update context and binding so as to avoid impacting other bindings Release Notes: - N/A *or* Added/Fixed/Improved ... --- crates/gpui/src/platform/keystroke.rs | 2 +- crates/settings/src/keymap_file.rs | 187 +++++++++++++++++++++++--- crates/settings_ui/src/keybindings.rs | 163 ++++++++++++++++------ 3 files changed, 288 insertions(+), 64 deletions(-) diff --git a/crates/gpui/src/platform/keystroke.rs b/crates/gpui/src/platform/keystroke.rs index 18adc1af10073001b82e0a72f8e372fafe4395b0..40387a820230cfc0f73f90643c082619ceaa595a 100644 --- a/crates/gpui/src/platform/keystroke.rs +++ b/crates/gpui/src/platform/keystroke.rs @@ -55,7 +55,7 @@ impl Keystroke { /// /// This method assumes that `self` was typed and `target' is in the keymap, and checks /// both possibilities for self against the target. - pub(crate) fn should_match(&self, target: &Keystroke) -> bool { + pub fn should_match(&self, target: &Keystroke) -> bool { #[cfg(not(target_os = "windows"))] if let Some(key_char) = self .key_char diff --git a/crates/settings/src/keymap_file.rs b/crates/settings/src/keymap_file.rs index 4c4ceee49bcd0a90ac43329e6ecd6211a423ae65..ca54b6a877361af15a634ec7ce3c247ffeaff49f 100644 --- a/crates/settings/src/keymap_file.rs +++ b/crates/settings/src/keymap_file.rs @@ -604,7 +604,7 @@ impl KeymapFile { // if trying to replace a keybinding that is not user-defined, treat it as an add operation match operation { KeybindUpdateOperation::Replace { - target_source, + target_keybind_source: target_source, source, .. } if target_source != KeybindSource::User => { @@ -643,7 +643,12 @@ impl KeymapFile { else { continue; }; - if keystrokes != target.keystrokes { + 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 { @@ -655,18 +660,75 @@ impl KeymapFile { } if let Some(index) = found_index { - let (replace_range, replace_value) = replace_top_level_array_value_in_json_text( - &keymap_contents, - &["bindings", &target.keystrokes_unparsed()], - Some(&source_action_value), - Some(&source.keystrokes_unparsed()), - index, - tab_size, - ) - .context("Failed to replace keybinding")?; - keymap_contents.replace_range(replace_range, &replace_value); - - return Ok(keymap_contents); + 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 + + let (replace_range, replace_value) = + replace_top_level_array_value_in_json_text( + &keymap_contents, + &["bindings", &target.keystrokes_unparsed()], + Some(&source_action_value), + Some(&source.keystrokes_unparsed()), + index, + tab_size, + ) + .context("Failed to replace keybinding")?; + keymap_contents.replace_range(replace_range, &replace_value); + + return Ok(keymap_contents); + } else if keymap.0[index] + .bindings + .as_ref() + .map_or(true, |bindings| bindings.len() == 1) + { + // if we are replacing the only binding in the section, + // just update the section in place, updating the context + // and the binding + + let (replace_range, replace_value) = + replace_top_level_array_value_in_json_text( + &keymap_contents, + &["bindings", &target.keystrokes_unparsed()], + Some(&source_action_value), + Some(&source.keystrokes_unparsed()), + index, + tab_size, + ) + .context("Failed to replace keybinding")?; + keymap_contents.replace_range(replace_range, &replace_value); + + let (replace_range, replace_value) = + replace_top_level_array_value_in_json_text( + &keymap_contents, + &["context"], + source.context.map(Into::into).as_ref(), + None, + index, + tab_size, + ) + .context("Failed to replace keybinding")?; + keymap_contents.replace_range(replace_range, &replace_value); + return Ok(keymap_contents); + } else { + // if we are replacing one of multiple bindings in a section + // with a context change, remove the existing binding from the + // section, then treat this operation as an add operation of the + // new binding with the updated context. + + let (replace_range, replace_value) = + replace_top_level_array_value_in_json_text( + &keymap_contents, + &["bindings", &target.keystrokes_unparsed()], + None, + None, + index, + tab_size, + ) + .context("Failed to replace keybinding")?; + keymap_contents.replace_range(replace_range, &replace_value); + operation = KeybindUpdateOperation::Add(source); + } } else { log::warn!( "Failed to find keybinding to update `{:?} -> {}` creating new binding for `{:?} -> {}` instead", @@ -712,7 +774,7 @@ pub enum KeybindUpdateOperation<'a> { source: KeybindUpdateTarget<'a>, /// Describes the keybind to remove target: KeybindUpdateTarget<'a>, - target_source: KeybindSource, + target_keybind_source: KeybindSource, }, Add(KeybindUpdateTarget<'a>), } @@ -1001,7 +1063,7 @@ mod tests { use_key_equivalents: false, input: Some(r#"{"foo": "bar"}"#), }, - target_source: KeybindSource::Base, + target_keybind_source: KeybindSource::Base, }, r#"[ { @@ -1027,14 +1089,14 @@ mod tests { r#"[ { "bindings": { - "ctrl-a": "zed::SomeAction" + "a": "zed::SomeAction" } } ]"# .unindent(), KeybindUpdateOperation::Replace { target: KeybindUpdateTarget { - keystrokes: &parse_keystrokes("ctrl-a"), + keystrokes: &parse_keystrokes("a"), action_name: "zed::SomeAction", context: None, use_key_equivalents: false, @@ -1047,7 +1109,7 @@ mod tests { use_key_equivalents: false, input: Some(r#"{"foo": "bar"}"#), }, - target_source: KeybindSource::User, + target_keybind_source: KeybindSource::User, }, r#"[ { @@ -1088,7 +1150,7 @@ mod tests { use_key_equivalents: false, input: None, }, - target_source: KeybindSource::User, + target_keybind_source: KeybindSource::User, }, r#"[ { @@ -1131,7 +1193,7 @@ mod tests { use_key_equivalents: false, input: Some(r#"{"foo": "bar"}"#), }, - target_source: KeybindSource::User, + target_keybind_source: KeybindSource::User, }, r#"[ { @@ -1149,5 +1211,88 @@ mod tests { ]"# .unindent(), ); + + check_keymap_update( + r#"[ + { + "context": "SomeContext", + "bindings": { + "a": "foo::bar", + "b": "baz::qux", + } + } + ]"# + .unindent(), + KeybindUpdateOperation::Replace { + target: KeybindUpdateTarget { + keystrokes: &parse_keystrokes("a"), + action_name: "foo::bar", + context: Some("SomeContext"), + use_key_equivalents: false, + input: None, + }, + source: KeybindUpdateTarget { + keystrokes: &parse_keystrokes("c"), + action_name: "foo::baz", + context: Some("SomeOtherContext"), + use_key_equivalents: false, + input: None, + }, + target_keybind_source: KeybindSource::User, + }, + r#"[ + { + "context": "SomeContext", + "bindings": { + "b": "baz::qux", + } + }, + { + "context": "SomeOtherContext", + "bindings": { + "c": "foo::baz" + } + } + ]"# + .unindent(), + ); + + check_keymap_update( + r#"[ + { + "context": "SomeContext", + "bindings": { + "a": "foo::bar", + } + } + ]"# + .unindent(), + KeybindUpdateOperation::Replace { + target: KeybindUpdateTarget { + keystrokes: &parse_keystrokes("a"), + action_name: "foo::bar", + context: Some("SomeContext"), + use_key_equivalents: false, + input: None, + }, + source: KeybindUpdateTarget { + keystrokes: &parse_keystrokes("c"), + action_name: "foo::baz", + context: Some("SomeOtherContext"), + use_key_equivalents: false, + input: None, + }, + target_keybind_source: KeybindSource::User, + }, + r#"[ + { + "context": "SomeOtherContext", + "bindings": { + "c": "foo::baz", + } + } + ]"# + .unindent(), + ); } } diff --git a/crates/settings_ui/src/keybindings.rs b/crates/settings_ui/src/keybindings.rs index 1f5f4b1b7e18c7720227d6c04d7f8680e469c94b..34d4b8585256d12b62b725d360679225b7360a82 100644 --- a/crates/settings_ui/src/keybindings.rs +++ b/crates/settings_ui/src/keybindings.rs @@ -1,4 +1,7 @@ -use std::{ops::Range, sync::Arc}; +use std::{ + ops::{Not, Range}, + sync::Arc, +}; use anyhow::{Context as _, anyhow}; use collections::HashSet; @@ -824,6 +827,7 @@ impl RenderOnce for SyntaxHighlightedText { struct KeybindingEditorModal { editing_keybind: ProcessedKeybinding, keybind_editor: Entity, + context_editor: Entity, fs: Arc, error: Option, } @@ -842,17 +846,86 @@ impl KeybindingEditorModal { pub fn new( editing_keybind: ProcessedKeybinding, fs: Arc, - _window: &mut Window, + window: &mut Window, cx: &mut App, ) -> Self { let keybind_editor = cx.new(KeystrokeInput::new); + let context_editor = cx.new(|cx| { + let mut editor = Editor::single_line(window, cx); + if let Some(context) = editing_keybind + .context + .as_ref() + .and_then(KeybindContextString::local) + { + editor.set_text(context.clone(), window, cx); + } else { + editor.set_placeholder_text("Keybinding context", cx); + } + + editor + }); Self { editing_keybind, fs, keybind_editor, + context_editor, error: None, } } + + fn save(&mut self, cx: &mut Context) { + 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() { + self.error = Some("Keystrokes cannot be empty".to_string()); + cx.notify(); + return; + } + let tab_size = cx.global::().json_tab_size(); + let new_context = self + .context_editor + .read_with(cx, |editor, cx| editor.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.error = Some(err.to_string()); + cx.notify(); + return; + } + + cx.spawn(async move |this, cx| { + if let Err(err) = save_keybinding_update( + existing_keybind, + &new_keystrokes, + new_context.as_deref(), + &fs, + tab_size, + ) + .await + { + this.update(cx, |this, cx| { + this.error = Some(err.to_string()); + cx.notify(); + }) + .log_err(); + } else { + this.update(cx, |_this, cx| { + cx.emit(DismissEvent); + }) + .ok(); + } + }) + .detach(); + } } impl Render for KeybindingEditorModal { @@ -868,14 +941,35 @@ impl Render for KeybindingEditorModal { .gap_2() .child( v_flex().child(Label::new("Edit Keystroke")).child( - Label::new( - "Input the desired keystroke for the selected action and hit save.", - ) - .color(Color::Muted), + Label::new("Input the desired keystroke for the selected action.") + .color(Color::Muted), ), ) .child(self.keybind_editor.clone()), ) + .child( + v_flex() + .p_3() + .gap_3() + .child( + v_flex().child(Label::new("Edit Keystroke")).child( + Label::new("Input the desired keystroke for the selected action.") + .color(Color::Muted), + ), + ) + .child( + div() + .w_full() + .border_color(cx.theme().colors().border_variant) + .border_1() + .py_2() + .px_3() + .min_h_8() + .rounded_md() + .bg(theme.editor_background) + .child(self.context_editor.clone()), + ), + ) .child( h_flex() .p_2() @@ -888,38 +982,11 @@ impl Render for KeybindingEditorModal { Button::new("cancel", "Cancel") .on_click(cx.listener(|_, _, _, cx| cx.emit(DismissEvent))), ) - .child(Button::new("save-btn", "Save").on_click(cx.listener( - |this, _event, _window, cx| { - let existing_keybind = this.editing_keybind.clone(); - let fs = this.fs.clone(); - let new_keystrokes = this - .keybind_editor - .read_with(cx, |editor, _| editor.keystrokes.clone()); - if new_keystrokes.is_empty() { - this.error = Some("Keystrokes cannot be empty".to_string()); - cx.notify(); - return; - } - let tab_size = cx.global::().json_tab_size(); - cx.spawn(async move |this, cx| { - if let Err(err) = save_keybinding_update( - existing_keybind, - &new_keystrokes, - &fs, - tab_size, - ) - .await - { - this.update(cx, |this, cx| { - this.error = Some(err.to_string()); - cx.notify(); - }) - .log_err(); - } - }) - .detach(); - }, - ))), + .child( + Button::new("save-btn", "Save").on_click( + cx.listener(|this, _event, _window, cx| Self::save(this, cx)), + ), + ), ) .when_some(self.error.clone(), |this, error| { this.child( @@ -937,6 +1004,7 @@ impl Render for KeybindingEditorModal { async fn save_keybinding_update( existing: ProcessedKeybinding, new_keystrokes: &[Keystroke], + new_context: Option<&str>, fs: &Arc, tab_size: usize, ) -> anyhow::Result<()> { @@ -950,7 +1018,7 @@ async fn save_keybinding_update( .map(|keybinding| keybinding.keystrokes.as_slice()) .unwrap_or_default(); - let context = existing + let existing_context = existing .context .as_ref() .and_then(KeybindContextString::local_str); @@ -963,18 +1031,18 @@ async fn save_keybinding_update( let operation = if existing.ui_key_binding.is_some() { settings::KeybindUpdateOperation::Replace { target: settings::KeybindUpdateTarget { - context, + context: existing_context, keystrokes: existing_keystrokes, action_name: &existing.action, use_key_equivalents: false, input, }, - target_source: existing + target_keybind_source: existing .source .map(|(source, _name)| source) .unwrap_or(KeybindSource::User), source: settings::KeybindUpdateTarget { - context, + context: new_context, keystrokes: new_keystrokes, action_name: &existing.action, use_key_equivalents: false, @@ -1071,6 +1139,17 @@ impl KeystrokeInput { cx.stop_propagation(); cx.notify(); } + + fn keystrokes(&self) -> &[Keystroke] { + if self + .keystrokes + .last() + .map_or(false, |last| last.key.is_empty()) + { + return &self.keystrokes[..self.keystrokes.len() - 1]; + } + return &self.keystrokes; + } } impl Focusable for KeystrokeInput {