From 94400d856e0e0ad263a457c2c412897fe883f3d7 Mon Sep 17 00:00:00 2001 From: iam-liam <117163129+iam-liam@users.noreply.github.com> Date: Tue, 17 Mar 2026 04:56:48 +0000 Subject: [PATCH] keymap_editor: Fix default binding deletion and override (#50699) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Fixes #48725 When using the keymap editor to delete or re-bind default keybindings, two issues caused stale entries to persist: - **Deleting a default binding** created a `NoAction` suppression entry in `keymap.json` (correct for GPUI-level suppression), but the keymap editor displayed it as a phantom `` row alongside the greyed-out default. Searching by keystroke showed both entries, and clash detection counted the phantom as a real binding. - **Changing a default binding's keystroke** (e.g. `⌥⌘T` → `⇧⌃⌘T`) added the new binding but never suppressed the original default under the old keystroke. The old default continued appearing in keystroke searches and clash warnings. ## Fix 1. **Filter `NoAction` suppression bindings from the keymap editor display.** These remain in the internal binding list for conflict detection (so overridden defaults are still correctly greyed out), but are excluded from the visible match results. 2. **Append a `null` suppression for the old keystroke when replacing a non-user binding with a different keystroke.** When `update_keybinding` converts a `Replace` to `Add` for a non-user binding and the keystroke changes, it now also writes `{"old-keystroke": null}` (with the original context) to suppress the stale default. ## Reproduction steps (verified fixed) 1. Open the keymap editor 2. Click Search by Keystroke and press `⌥⌘T` 3. You should see "agent: new thread" and "pane: close other items" (both Default) 4. Right-click "agent: new thread" and choose Delete 5. Double-click "pane: close other items", change keystroke to `⇧⌃⌘T`, click Save 6. Verify no `` phantom row appears, and no stale defaults remain under `⌥⌘T` 7. Close and reopen the keymap editor, search `⌥⌘T` — no results 8. Search for "editor: wrap selections in tag" and assign `⌥⌘T` — no clash warnings ## Test plan - [x] Existing `keymap_update` and `test_keymap_remove` tests pass - [x] Added test: replacing a non-user binding **without** changing the keystroke produces no suppression - [x] Added test: replacing a non-user binding **with** context and keystroke change produces a suppression that preserves the context - [x] Manual verification of all reproduction steps above Release Notes: - Fixed an issue with the keymap editor where where the defaults would not be completely removed when deleting or overriding default keybindings --------- Co-authored-by: Ben Kunkle --- crates/keymap_editor/src/keymap_editor.rs | 347 ++++++++++++++-------- crates/settings/src/keymap_file.rs | 128 ++++++++ 2 files changed, 354 insertions(+), 121 deletions(-) diff --git a/crates/keymap_editor/src/keymap_editor.rs b/crates/keymap_editor/src/keymap_editor.rs index c8df5c1d8cf60ed07d6013cfb088bf8d362cf330..e63c07d9975950afbb57b243114950f77c7240cb 100644 --- a/crates/keymap_editor/src/keymap_editor.rs +++ b/crates/keymap_editor/src/keymap_editor.rs @@ -30,8 +30,8 @@ use settings::{ BaseKeymap, KeybindSource, KeymapFile, Settings as _, SettingsAssets, infer_json_indent_size, }; use ui::{ - ActiveTheme as _, App, Banner, BorrowAppContext, ContextMenu, IconButtonShape, Indicator, - Modal, ModalFooter, ModalHeader, ParentElement as _, PopoverMenu, Render, Section, + ActiveTheme as _, App, Banner, BorrowAppContext, ContextMenu, IconButtonShape, IconPosition, + Indicator, Modal, ModalFooter, ModalHeader, ParentElement as _, PopoverMenu, Render, Section, SharedString, Styled as _, Table, TableColumnWidths, TableInteractionState, TableResizeBehavior, Tooltip, Window, prelude::*, }; @@ -73,6 +73,8 @@ actions!( CopyContext, /// Toggles Conflict Filtering ToggleConflictFilter, + /// Toggles whether NoAction bindings are shown + ToggleNoActionBindings, /// Toggle Keystroke search ToggleKeystrokeSearch, /// Toggles exact matching for keystroke search @@ -183,7 +185,7 @@ impl KeymapEventChannel { } } -#[derive(Default, PartialEq)] +#[derive(Default, PartialEq, Copy, Clone)] enum SearchMode { #[default] Normal, @@ -224,6 +226,25 @@ impl FilterState { } } +#[derive(Default, PartialEq, Eq, Copy, Clone)] +struct SourceFilters { + user: bool, + zed_defaults: bool, + vim_defaults: bool, +} + +impl SourceFilters { + fn allows(&self, source: Option) -> bool { + match source { + Some(KeybindSource::User) => self.user, + Some(KeybindSource::Vim) => self.vim_defaults, + Some(KeybindSource::Base | KeybindSource::Default | KeybindSource::Unknown) | None => { + self.zed_defaults + } + } + } +} + #[derive(Debug, Default, PartialEq, Eq, Clone, Hash)] struct ActionMapping { keystrokes: Rc<[KeybindingKeystroke]>, @@ -412,6 +433,8 @@ struct KeymapEditor { keybindings: Vec, keybinding_conflict_state: ConflictState, filter_state: FilterState, + source_filters: SourceFilters, + show_no_action_bindings: bool, search_mode: SearchMode, search_query_debounce: Option>, // corresponds 1 to 1 with keybindings @@ -539,6 +562,12 @@ impl KeymapEditor { keybindings: vec![], keybinding_conflict_state: ConflictState::default(), filter_state: FilterState::default(), + source_filters: SourceFilters { + user: true, + zed_defaults: true, + vim_defaults: true, + }, + show_no_action_bindings: true, search_mode: SearchMode::default(), string_match_candidates: Arc::new(vec![]), matches: vec![], @@ -637,6 +666,11 @@ impl KeymapEditor { ) .await; this.update(cx, |this, cx| { + matches.retain(|candidate| { + this.source_filters + .allows(this.keybindings[candidate.candidate_id].keybind_source()) + }); + match this.filter_state { FilterState::Conflicts => { matches.retain(|candidate| { @@ -695,6 +729,15 @@ impl KeymapEditor { SearchMode::Normal => {} } + // Filter out NoAction suppression bindings by default. These are internal + // markers created when a user deletes a default binding (to suppress the + // default at the GPUI level), not real bindings the user should usually see. + if !this.show_no_action_bindings { + matches.retain(|item| { + this.keybindings[item.candidate_id].action().name != gpui::NoAction.name() + }); + } + if action_query.is_empty() { matches.sort_by(|item1, item2| { let binding1 = &this.keybindings[item1.candidate_id]; @@ -1367,6 +1410,31 @@ impl KeymapEditor { self.set_filter_state(self.filter_state.invert(), cx); } + fn toggle_no_action_bindings( + &mut self, + _: &ToggleNoActionBindings, + _: &mut Window, + cx: &mut Context, + ) { + self.show_no_action_bindings = !self.show_no_action_bindings; + self.on_query_changed(cx); + } + + fn toggle_user_bindings_filter(&mut self, cx: &mut Context) { + self.source_filters.user = !self.source_filters.user; + self.on_query_changed(cx); + } + + fn toggle_zed_defaults_filter(&mut self, cx: &mut Context) { + self.source_filters.zed_defaults = !self.source_filters.zed_defaults; + self.on_query_changed(cx); + } + + fn toggle_vim_defaults_filter(&mut self, cx: &mut Context) { + self.source_filters.vim_defaults = !self.source_filters.vim_defaults; + self.on_query_changed(cx); + } + fn set_filter_state(&mut self, filter_state: FilterState, cx: &mut Context) { if self.filter_state != filter_state { self.filter_state = filter_state; @@ -1442,6 +1510,127 @@ impl KeymapEditor { .filter(|kb| kb.keystrokes().is_some()) .any(|kb| kb.action().name == action_name) } + + fn render_filter_dropdown( + &self, + focus_handle: &FocusHandle, + cx: &mut Context, + ) -> impl IntoElement { + let focus_handle = focus_handle.clone(); + let keymap_editor = cx.entity(); + return PopoverMenu::new("keymap-editor-filter-menu") + .menu(move |window, cx| { + Some(ContextMenu::build_persistent(window, cx, { + let focus_handle = focus_handle.clone(); + let keymap_editor = keymap_editor.clone(); + move |mut menu, _window, cx| { + let (filter_state, source_filters, show_no_action_bindings) = keymap_editor + .read_with(cx, |editor, _| { + ( + editor.filter_state, + editor.source_filters, + editor.show_no_action_bindings, + ) + }); + + menu = menu + .context(focus_handle.clone()) + .header("Filters") + .map(add_filter( + "Conflicts", + matches!(filter_state, FilterState::Conflicts), + Some(ToggleConflictFilter.boxed_clone()), + &focus_handle, + &keymap_editor, + None, + )) + .map(add_filter( + "No Action", + show_no_action_bindings, + Some(ToggleNoActionBindings.boxed_clone()), + &focus_handle, + &keymap_editor, + None, + )) + .separator() + .header("Categories") + .map(add_filter( + "User", + source_filters.user, + None, + &focus_handle, + &keymap_editor, + Some(|editor, cx| { + editor.toggle_user_bindings_filter(cx); + }), + )) + .map(add_filter( + "Default", + source_filters.zed_defaults, + None, + &focus_handle, + &keymap_editor, + Some(|editor, cx| { + editor.toggle_zed_defaults_filter(cx); + }), + )) + .map(add_filter( + "Vim", + source_filters.vim_defaults, + None, + &focus_handle, + &keymap_editor, + Some(|editor, cx| { + editor.toggle_vim_defaults_filter(cx); + }), + )); + menu + } + })) + }) + .anchor(gpui::Corner::TopRight) + .offset(gpui::Point { + x: px(0.0), + y: px(2.0), + }) + .trigger_with_tooltip( + IconButton::new("KeymapEditorFilterMenuButton", IconName::Sliders) + .icon_size(IconSize::Small) + .when( + self.keybinding_conflict_state.any_user_binding_conflicts(), + |this| this.indicator(Indicator::dot().color(Color::Warning)), + ), + Tooltip::text("Filters"), + ); + + fn add_filter( + name: &'static str, + toggled: bool, + action: Option>, + focus_handle: &FocusHandle, + keymap_editor: &Entity, + cb: Option)>, + ) -> impl FnOnce(ContextMenu) -> ContextMenu { + let focus_handle = focus_handle.clone(); + let keymap_editor = keymap_editor.clone(); + return move |menu: ContextMenu| { + menu.toggleable_entry( + name, + toggled, + IconPosition::End, + action.as_ref().map(|a| a.boxed_clone()), + move |window, cx| { + window.focus(&focus_handle, cx); + if let Some(action) = &action { + window.dispatch_action(action.boxed_clone(), cx); + } else if let Some(cb) = cb { + keymap_editor.update(cx, cb); + } + }, + ) + }; + } + } } struct HumanizedActionNameCache { @@ -1694,6 +1883,7 @@ impl Render for KeymapEditor { let row_count = self.matches.len(); let focus_handle = &self.focus_handle; let theme = cx.theme(); + let search_mode = self.search_mode; v_flex() .id("keymap-editor") @@ -1711,6 +1901,7 @@ impl Render for KeymapEditor { .on_action(cx.listener(Self::copy_action_to_clipboard)) .on_action(cx.listener(Self::copy_context_to_clipboard)) .on_action(cx.listener(Self::toggle_conflict_filter)) + .on_action(cx.listener(Self::toggle_no_action_bindings)) .on_action(cx.listener(Self::toggle_keystroke_search)) .on_action(cx.listener(Self::toggle_exact_keystroke_matching)) .on_action(cx.listener(Self::show_matching_keystrokes)) @@ -1727,6 +1918,7 @@ impl Render for KeymapEditor { .child( h_flex() .gap_2() + .items_center() .child( h_flex() .key_context({ @@ -1748,152 +1940,65 @@ impl Render for KeymapEditor { h_flex() .gap_1() .min_w_96() + .items_center() .child( IconButton::new( - "KeymapEditorToggleFiltersIcon", + "KeymapEditorKeystrokeSearchButton", IconName::Keyboard, ) .icon_size(IconSize::Small) + .toggle_state(matches!( + search_mode, + SearchMode::KeyStroke { .. } + )) .tooltip({ let focus_handle = focus_handle.clone(); - move |_window, cx| { Tooltip::for_action_in( - "Search by Keystroke", + "Search by Keystrokes", &ToggleKeystrokeSearch, - &focus_handle.clone(), + &focus_handle, cx, ) } }) - .toggle_state(matches!( - self.search_mode, - SearchMode::KeyStroke { .. } - )) - .on_click(|_, window, cx| { + .on_click(cx.listener(|_, _, window, cx| { window.dispatch_action( ToggleKeystrokeSearch.boxed_clone(), cx, ); - }), + })), ) .child( - IconButton::new("KeymapEditorConflictIcon", IconName::Warning) - .icon_size(IconSize::Small) - .when( - self.keybinding_conflict_state - .any_user_binding_conflicts(), - |this| { - this.indicator( - Indicator::dot().color(Color::Warning), - ) - }, + self.render_filter_dropdown(focus_handle, cx) + ) + .child( + Button::new("edit-in-json", "Edit in JSON") + .style(ButtonStyle::Subtle) + .key_binding( + ui::KeyBinding::for_action_in(&zed_actions::OpenKeymapFile, &focus_handle, cx) + .map(|kb| kb.size(rems_from_px(10.))), ) - .tooltip({ - let filter_state = self.filter_state; - let focus_handle = focus_handle.clone(); - - move |_window, cx| { - Tooltip::for_action_in( - match filter_state { - FilterState::All => "Show Conflicts", - FilterState::Conflicts => { - "Hide Conflicts" - } - }, - &ToggleConflictFilter, - &focus_handle.clone(), - cx, - ) - } - }) - .selected_icon_color(Color::Warning) - .toggle_state(matches!( - self.filter_state, - FilterState::Conflicts - )) .on_click(|_, window, cx| { window.dispatch_action( - ToggleConflictFilter.boxed_clone(), + zed_actions::OpenKeymapFile.boxed_clone(), cx, ); - }), + }) ) .child( - h_flex() - .w_full() - .px_1p5() - .gap_1() - .justify_end() - .child( - PopoverMenu::new("open-keymap-menu") - .menu(move |window, cx| { - Some(ContextMenu::build(window, cx, |menu, _, _| { - menu.header("View Default...") - .action( - "Zed Key Bindings", - zed_actions::OpenDefaultKeymap - .boxed_clone(), - ) - .action( - "Vim Bindings", - zed_actions::vim::OpenDefaultKeymap.boxed_clone(), - ) - })) - }) - .anchor(gpui::Corner::TopRight) - .offset(gpui::Point { - x: px(0.0), - y: px(2.0), - }) - .trigger_with_tooltip( - IconButton::new( - "OpenKeymapJsonButton", - IconName::Ellipsis, - ) - .icon_size(IconSize::Small), - { - let focus_handle = focus_handle.clone(); - move |_window, cx| { - Tooltip::for_action_in( - "View Default...", - &zed_actions::OpenKeymapFile, - &focus_handle, - cx, - ) - } - }, - ), - ) - .child( - Button::new("edit-in-json", "Edit in JSON") - .style(ButtonStyle::Subtle) - .key_binding( - ui::KeyBinding::for_action_in(&zed_actions::OpenKeymapFile, &focus_handle, cx) - .map(|kb| kb.size(rems_from_px(10.))), - ) - .on_click(|_, window, cx| { - window.dispatch_action( - zed_actions::OpenKeymapFile.boxed_clone(), - cx, - ); - }) + Button::new("create", "Create Keybinding") + .style(ButtonStyle::Outlined) + .key_binding( + ui::KeyBinding::for_action_in(&OpenCreateKeybindingModal, &focus_handle, cx) + .map(|kb| kb.size(rems_from_px(10.))), ) - .child( - Button::new("create", "Create Keybinding") - .style(ButtonStyle::Outlined) - .key_binding( - ui::KeyBinding::for_action_in(&OpenCreateKeybindingModal, &focus_handle, cx) - .map(|kb| kb.size(rems_from_px(10.))), - ) - .on_click(|_, window, cx| { - window.dispatch_action( - OpenCreateKeybindingModal.boxed_clone(), - cx, - ); - }) - ) - + .on_click(|_, window, cx| { + window.dispatch_action( + OpenCreateKeybindingModal.boxed_clone(), + cx, + ); + }) ) ), ) diff --git a/crates/settings/src/keymap_file.rs b/crates/settings/src/keymap_file.rs index 67f41b14521480e7082f4070db0ed50d4dfdc1fe..0bc7c45afb6870c772c5963aebcf9807988ac359 100644 --- a/crates/settings/src/keymap_file.rs +++ b/crates/settings/src/keymap_file.rs @@ -701,6 +701,10 @@ impl KeymapFile { tab_size: usize, keyboard_mapper: &dyn gpui::PlatformKeyboardMapper, ) -> Result { + // When replacing a non-user binding's keystroke, we need to also suppress the old + // default so it doesn't continue showing under the old keystroke. + let mut old_keystroke_suppression: Option<(Option, String)> = None; + match operation { // if trying to replace a keybinding that is not user-defined, treat it as an add operation KeybindUpdateOperation::Replace { @@ -708,6 +712,12 @@ impl KeymapFile { source, target, } if target_source != KeybindSource::User => { + if target.keystrokes_unparsed() != source.keystrokes_unparsed() { + old_keystroke_suppression = Some(( + target.context.map(String::from), + target.keystrokes_unparsed(), + )); + } operation = KeybindUpdateOperation::Add { source, from: Some(target), @@ -886,6 +896,28 @@ impl KeymapFile { ); keymap_contents.replace_range(replace_range, &replace_value); } + + // If we converted a Replace to Add because the target was a non-user binding, + // and the keystroke changed, suppress the old default keystroke with a NoAction + // binding so it doesn't continue appearing under the old keystroke. + if let Some((context, old_keystrokes)) = old_keystroke_suppression { + let mut value = serde_json::Map::with_capacity(2); + if let Some(context) = context { + value.insert("context".to_string(), context.into()); + } + value.insert("bindings".to_string(), { + let mut bindings = serde_json::Map::new(); + bindings.insert(old_keystrokes, Value::Null); + bindings.into() + }); + let (replace_range, replace_value) = append_top_level_array_value_in_json_text( + &keymap_contents, + &value.into(), + tab_size, + ); + keymap_contents.replace_range(replace_range, &replace_value); + } + return Ok(keymap_contents); fn find_binding<'a, 'b>( @@ -1479,6 +1511,102 @@ mod tests { } ] } + }, + { + "bindings": { + "ctrl-a": null + } + } + ]"# + .unindent(), + ); + + // Replacing a non-user binding without changing the keystroke should + // not produce a NoAction suppression entry. + check_keymap_update( + r#"[ + { + "bindings": { + "ctrl-a": "zed::SomeAction" + } + } + ]"# + .unindent(), + KeybindUpdateOperation::Replace { + target: KeybindUpdateTarget { + keystrokes: &parse_keystrokes("ctrl-a"), + action_name: "zed::SomeAction", + context: None, + action_arguments: None, + }, + source: KeybindUpdateTarget { + keystrokes: &parse_keystrokes("ctrl-a"), + action_name: "zed::SomeOtherAction", + context: None, + action_arguments: None, + }, + target_keybind_source: KeybindSource::Base, + }, + r#"[ + { + "bindings": { + "ctrl-a": "zed::SomeAction" + } + }, + { + "bindings": { + "ctrl-a": "zed::SomeOtherAction" + } + } + ]"# + .unindent(), + ); + + // Replacing a non-user binding with a context and a keystroke change + // should produce a suppression entry that preserves the context. + check_keymap_update( + r#"[ + { + "context": "SomeContext", + "bindings": { + "ctrl-a": "zed::SomeAction" + } + } + ]"# + .unindent(), + KeybindUpdateOperation::Replace { + target: KeybindUpdateTarget { + keystrokes: &parse_keystrokes("ctrl-a"), + action_name: "zed::SomeAction", + context: Some("SomeContext"), + action_arguments: None, + }, + source: KeybindUpdateTarget { + keystrokes: &parse_keystrokes("ctrl-b"), + action_name: "zed::SomeOtherAction", + context: Some("SomeContext"), + action_arguments: None, + }, + target_keybind_source: KeybindSource::Default, + }, + r#"[ + { + "context": "SomeContext", + "bindings": { + "ctrl-a": "zed::SomeAction" + } + }, + { + "context": "SomeContext", + "bindings": { + "ctrl-b": "zed::SomeOtherAction" + } + }, + { + "context": "SomeContext", + "bindings": { + "ctrl-a": null + } } ]"# .unindent(),