yenru0/zed
1
0 Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

keymap_editor: Fix deleting and editing bindings that use deprecated action names (#60300)

Browse Source 
# Objective

In the keymap editor, bindings whose `keymap.json` entries use a
deprecated action alias (e.g. `"editor::CopyRelativePath"` for
`workspace::CopyRelativePath`) could not be deleted or edited. Aliases
resolve to the canonical action on load, so the row displays normally,
but file updates searched for the canonical name and never matched the
alias entry. Deleting failed ("Failed to find keybinding to remove"),
and editing silently fell back to appending a new binding — leaving the
user with duplicate identical-looking rows they couldn't remove.

## Solution

- Thread gpui's deprecated-alias map
(`App::deprecated_actions_to_preferred_actions`) into
`KeymapFile::update_keybinding`, and resolve aliases in file entries
when matching bindings.
- As a side effect, editing an alias-based binding now rewrites the
entry with the canonical action name instead of duplicating it.

## Testing

- Unit tests in `settings` for removing and replacing alias-based
entries.
- End-to-end keymap editor tests (fake fs → keymap load → `KeymapEditor`
→ delete) covering both the alias case and plain duplicate bindings.

## Self-Review Checklist:

- [x] I've reviewed my own diff for quality, security, and reliability
- [x] Unsafe blocks (if any) have justifying comments
- [x] The content adheres to Zed's UI standards
([UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist)
and
[icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md)
guidelines)
- [x] Tests cover the new/changed behavior
- [x] Performance impact has been considered and is acceptable

---

Release Notes:

- Fixed the keymap editor failing to delete or edit key bindings whose
keymap file entries use deprecated action names
This commit is contained in:
Ben Kunkle authored and GitHub committed 2026-07-02 19:24:34 +00:00
1 parent bf39dab4e1
commit d216695d86
2 files changed
+413 -9
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

crates/keymap_editor/src/keymap_editor.rs
+212 -1
View File
@@ -1436,8 +1436,15 @@ impl KeymapEditor {
self.table_interaction_state.read(cx).scroll_offset(),
));
let keyboard_mapper = cx.keyboard_mapper().clone();
let deprecated_aliases = cx.deprecated_actions_to_preferred_actions().clone();
cx.spawn(async move |_, _| {
remove_keybinding(to_remove, &fs, keyboard_mapper.as_ref()).await
remove_keybinding(
to_remove,
&fs,
keyboard_mapper.as_ref(),
&deprecated_aliases,
)
.await
})
.detach_and_notify_err(self.workspace.clone(), window, cx);
}
@@ -2839,6 +2846,7 @@ impl KeybindingEditorModal {
let create = self.creating;
let keyboard_mapper = cx.keyboard_mapper().clone();
let deprecated_aliases = cx.deprecated_actions_to_preferred_actions().clone();
let action_name = self
.get_selected_action_name(cx)
@@ -2869,6 +2877,7 @@ impl KeybindingEditorModal {
new_action_args.as_deref(),
&fs,
keyboard_mapper.as_ref(),
&deprecated_aliases,
)
.await
{
@@ -3607,6 +3616,7 @@ async fn save_keybinding_update(
new_args: Option<&str>,
fs: &Arc<dyn Fs>,
keyboard_mapper: &dyn PlatformKeyboardMapper,
deprecated_aliases: &HashMap<&'static str, &'static str>,
) -> anyhow::Result<()> {
let keymap_contents = settings::KeymapFile::load_keymap_file(fs)
.await
@@ -3656,6 +3666,7 @@ async fn save_keybinding_update(
keymap_contents,
tab_size,
keyboard_mapper,
deprecated_aliases,
)
.map_err(|err| anyhow::anyhow!("Could not save updated keybinding: {}", err))?;
fs.write(
@@ -3678,6 +3689,7 @@ async fn remove_keybinding(
existing: ProcessedBinding,
fs: &Arc<dyn Fs>,
keyboard_mapper: &dyn PlatformKeyboardMapper,
deprecated_aliases: &HashMap<&'static str, &'static str>,
) -> anyhow::Result<()> {
let Some(keystrokes) = existing.keystrokes() else {
anyhow::bail!("Cannot remove a keybinding that does not exist");
@@ -3707,6 +3719,7 @@ async fn remove_keybinding(
keymap_contents,
tab_size,
keyboard_mapper,
deprecated_aliases,
)
.context("Failed to update keybinding")?;
fs.write(
@@ -4015,6 +4028,204 @@ mod persistence {
#[cfg(test)]
mod tests {
use super::*;
use fs::FakeFs;
use gpui::{TestAppContext, VisualTestContext};
use project::Project;
use serde_json::json;
use settings::KeymapFileLoadResult;
use workspace::{AppState, MultiWorkspace};
async fn reload_keymap_from_file(fs: &Arc<FakeFs>, cx: &mut TestAppContext) {
let content = fs.load(paths::keymap_file().as_path()).await.unwrap();
cx.update(|cx| {
let mut key_bindings = match KeymapFile::load(&content, cx) {
KeymapFileLoadResult::Success { key_bindings } => key_bindings,
KeymapFileLoadResult::SomeFailedToLoad { error_message, .. } => {
panic!("keymap failed to load: {error_message:?}")
}
KeymapFileLoadResult::JsonParseFailure { error } => {
panic!("keymap json parse failure: {error}")
}
};
cx.clear_key_bindings();
for key_binding in &mut key_bindings {
key_binding.set_meta(KeybindSource::User.meta());
}
cx.bind_keys(key_bindings);
KeymapEventChannel::trigger_keymap_changed(cx);
});
}
async fn setup_keymap_editor(
cx: &mut TestAppContext,
keymap_content: &str,
) -> (Arc<FakeFs>, Entity<KeymapEditor>, VisualTestContext) {
cx.update(|cx| {
let _state = AppState::test(cx);
editor::init(cx);
cx.set_global(KeymapEventChannel::new());
});
let fs = FakeFs::new(cx.executor());
fs.insert_tree(
paths::config_dir(),
json!({ "keymap.json": keymap_content }),
)
.await;
reload_keymap_from_file(&fs, cx).await;
let project = Project::test(fs.clone(), [], cx).await;
let window_handle =
cx.add_window(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx));
let workspace = window_handle
.read_with(cx, |multi_workspace, _| multi_workspace.workspace().clone())
.unwrap();
let mut cx = VisualTestContext::from_window(window_handle.into(), cx);
let keymap_editor = cx
.update(|window, cx| cx.new(|cx| KeymapEditor::new(workspace.downgrade(), window, cx)));
cx.run_until_parked();
(fs, keymap_editor, cx)
}
fn visible_rows_for_action(editor: &KeymapEditor, action_name: &str) -> Vec<usize> {
editor
.matches
.iter()
.enumerate()
.filter(|(_, string_match)| {
let binding = &editor.keybindings[string_match.candidate_id];
binding.action().name == action_name && binding.keystrokes().is_some()
})
.map(|(index, _)| index)
.collect()
}
#[gpui::test]
async fn test_delete_one_of_two_identical_user_bindings(cx: &mut TestAppContext) {
let keymap_content = r#"[
{
"bindings": {
"alt-cmd-shift-c": "zed::OpenKeymap"
}
},
{
"bindings": {
"alt-cmd-shift-c": "zed::OpenKeymap"
}
}
]"#;
let (fs, keymap_editor, mut cx) = setup_keymap_editor(cx, keymap_content).await;
let cx = &mut cx;
let rows = keymap_editor.read_with(cx, |editor, _| {
visible_rows_for_action(editor, "zed::OpenKeymap")
});
assert_eq!(
rows.len(),
2,
"expected the two duplicate bindings to show as two rows"
);
keymap_editor.update_in(cx, |editor, window, cx| {
editor.selected_index = Some(rows[1]);
editor.delete_binding(&DeleteBinding, window, cx);
});
cx.run_until_parked();
let content = fs.load(paths::keymap_file().as_path()).await.unwrap();
assert_eq!(
content.matches("alt-cmd-shift-c").count(),
1,
"expected exactly one binding remaining in the keymap file, got:\n{content}"
);
// Simulate the keymap file watcher reacting to the change.
reload_keymap_from_file(&fs, cx).await;
cx.run_until_parked();
let rows = keymap_editor.read_with(cx, |editor, _| {
visible_rows_for_action(editor, "zed::OpenKeymap")
});
assert_eq!(rows.len(), 1, "expected one row remaining after deletion");
}
// Regression test: one of the two entries in the keymap file uses a
// deprecated alias of the action (`editor::CopyRelativePath` instead of
// `workspace::CopyRelativePath`). Both rows display identically in the
// keymap editor (aliases resolve to the canonical action on load), but
// deletion targets the canonical action name, so `KeymapFile::update_keybinding`
// used to never find the alias entry, making it impossible to delete.
#[gpui::test]
async fn test_delete_binding_with_deprecated_action_alias(cx: &mut TestAppContext) {
let keymap_content = r#"[
{
"bindings": {
"alt-cmd-shift-c": "editor::CopyRelativePath"
}
},
{
"bindings": {
"alt-cmd-shift-c": "workspace::CopyRelativePath"
}
}
]"#;
let (fs, keymap_editor, mut cx) = setup_keymap_editor(cx, keymap_content).await;
let cx = &mut cx;
let rows = keymap_editor.read_with(cx, |editor, _| {
visible_rows_for_action(editor, "workspace::CopyRelativePath")
});
assert_eq!(
rows.len(),
2,
"both the alias and the canonical entry should show as (identical) rows"
);
// Delete the first row. Both rows report the canonical action name, so
// `find_binding` matches the canonical file entry and removes it.
keymap_editor.update_in(cx, |editor, window, cx| {
editor.selected_index = Some(rows[0]);
editor.delete_binding(&DeleteBinding, window, cx);
});
cx.run_until_parked();
let content = fs.load(paths::keymap_file().as_path()).await.unwrap();
assert_eq!(
content.matches("alt-cmd-shift-c").count(),
1,
"first deletion should remove one of the two entries, got:\n{content}"
);
// Simulate the keymap file watcher reacting to the change.
reload_keymap_from_file(&fs, cx).await;
cx.run_until_parked();
let rows = keymap_editor.read_with(cx, |editor, _| {
visible_rows_for_action(editor, "workspace::CopyRelativePath")
});
assert_eq!(
rows.len(),
1,
"one row should remain after the first deletion"
);
// Delete the remaining row (the alias entry). `find_binding` must
// resolve the deprecated alias in the file to the canonical action
// name to find and remove it.
keymap_editor.update_in(cx, |editor, window, cx| {
editor.selected_index = Some(rows[0]);
editor.delete_binding(&DeleteBinding, window, cx);
});
cx.run_until_parked();
let content = fs.load(paths::keymap_file().as_path()).await.unwrap();
assert_eq!(
content.matches("alt-cmd-shift-c").count(),
0,
"second deletion should remove the remaining (alias) entry, got:\n{content}"
);
}
#[test]
fn normalized_ctx_cmp() {
crates/settings/src/keymap_file.rs
+201 -8
View File
@@ -893,6 +893,7 @@ impl KeymapFile {
mut keymap_contents: String,
tab_size: usize,
keyboard_mapper: &dyn gpui::PlatformKeyboardMapper,
deprecated_aliases: &HashMap<&'static str, &'static str>,
) -> Result<String> {
// When replacing or removing a non-user binding, we may need to write an unbind entry
// to suppress the original default binding.
@@ -937,9 +938,13 @@ impl KeymapFile {
let target_action_value = target
.action_value()
.context("Failed to generate target action JSON value")?;
let Some(binding_location) =
find_binding(&keymap, target, &target_action_value, keyboard_mapper)
else {
let Some(binding_location) = find_binding(
&keymap,
target,
&target_action_value,
keyboard_mapper,
deprecated_aliases,
) else {
anyhow::bail!("Failed to find keybinding to remove");
};
let is_only_binding = binding_location.is_only_entry_in_section(&keymap);
@@ -973,9 +978,13 @@ impl KeymapFile {
.action_value()
.context("Failed to generate source action JSON value")?;
if let Some(binding_location) =
find_binding(&keymap, &target, &target_action_value, keyboard_mapper)
{
if let Some(binding_location) = find_binding(
&keymap,
&target,
&target_action_value,
keyboard_mapper,
deprecated_aliases,
) {
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
@@ -1072,7 +1081,7 @@ impl KeymapFile {
let use_key_equivalents = from.and_then(|from| {
let action_value = from.action_value().context("Failed to serialize action value. `use_key_equivalents` on new keybinding may be incorrect.").log_err()?;
let binding_location =
find_binding(&keymap, &from, &action_value, keyboard_mapper)?;
find_binding(&keymap, &from, &action_value, keyboard_mapper, deprecated_aliases)?;
Some(keymap.0[binding_location.index].use_key_equivalents)
}).unwrap_or(false);
if use_key_equivalents {
@@ -1122,6 +1131,7 @@ impl KeymapFile {
target: &KeybindUpdateTarget<'a>,
target_action_value: &Value,
keyboard_mapper: &dyn gpui::PlatformKeyboardMapper,
deprecated_aliases: &HashMap<&'static str, &'static str>,
) -> Option<BindingLocation<'b>> {
let target_context_parsed =
KeyBindingContextPredicate::parse(target.context.unwrap_or("")).ok();
@@ -1139,6 +1149,7 @@ impl KeymapFile {
target,
target_action_value,
keyboard_mapper,
deprecated_aliases,
|action| &action.0,
) {
return Some(binding_location);
@@ -1151,6 +1162,7 @@ impl KeymapFile {
target,
target_action_value,
keyboard_mapper,
deprecated_aliases,
|action| &action.0,
) {
return Some(binding_location);
@@ -1166,6 +1178,7 @@ impl KeymapFile {
target: &KeybindUpdateTarget<'a>,
target_action_value: &Value,
keyboard_mapper: &dyn gpui::PlatformKeyboardMapper,
deprecated_aliases: &HashMap<&'static str, &'static str>,
action_value: impl Fn(&T) -> &Value,
) -> Option<BindingLocation<'b>> {
let entries = entries?;
@@ -1192,7 +1205,11 @@ impl KeymapFile {
{
continue;
}
if action_value(action) != target_action_value {
if !action_value_matches_target(
action_value(action),
target_action_value,
deprecated_aliases,
) {
continue;
}
return Some(BindingLocation {
@@ -1204,6 +1221,40 @@ impl KeymapFile {
None
}
/// Compares a keymap file entry's action value against the target action
/// value. The target is built from a loaded binding's canonical action
/// name, while the file entry may still use a deprecated alias.
fn action_value_matches_target(
action_value: &Value,
target_action_value: &Value,
deprecated_aliases: &HashMap<&'static str, &'static str>,
) -> bool {
if action_value == target_action_value {
return true;
}
let (name, arguments) = match action_value {
Value::String(name) => (name.as_str(), None),
Value::Array(items) => match items.as_slice() {
[Value::String(name), arguments] => (name.as_str(), Some(arguments)),
_ => return false,
},
_ => return false,
};
let Some(&preferred_name) = deprecated_aliases.get(name) else {
return false;
};
match target_action_value {
Value::String(target_name) => target_name == preferred_name && arguments.is_none(),
Value::Array(items) => match items.as_slice() {
[Value::String(target_name), target_arguments] => {
target_name == preferred_name && arguments == Some(target_arguments)
}
_ => false,
},
_ => false,
}
}
#[derive(Copy, Clone)]
enum BindingKind {
Binding,
@@ -1525,6 +1576,7 @@ impl Action for ActionSequence {
#[cfg(test)]
mod tests {
use collections::HashMap;
use gpui::{Action, App, DummyKeyboardMapper, KeybindingKeystroke, Keystroke, Unbind};
use serde_json::Value;
use unindent::Unindent;
@@ -1742,11 +1794,23 @@ mod tests {
operation: KeybindUpdateOperation,
expected: impl ToString,
) {
check_keymap_update_with_deprecated_aliases(input, operation, expected, &[]);
}
#[track_caller]
fn check_keymap_update_with_deprecated_aliases(
input: impl ToString,
operation: KeybindUpdateOperation,
expected: impl ToString,
deprecated_aliases: &[(&'static str, &'static str)],
) {
let deprecated_aliases = HashMap::from_iter(deprecated_aliases.iter().copied());
let result = KeymapFile::update_keybinding(
operation,
input.to_string(),
4,
&gpui::DummyKeyboardMapper,
&deprecated_aliases,
)
.expect("Update succeeded");
pretty_assertions::assert_eq!(expected.to_string(), result);
@@ -2567,6 +2631,135 @@ mod tests {
);
}
#[test]
fn test_keymap_remove_duplicate_binding() {
zlog::init_test();
// Repro: user created two identical bindings via the keymap editor UI,
// resulting in two sections with the same keystrokes and action.
// Deleting one of them should remove exactly one section.
check_keymap_update(
r#"
[
{
"bindings": {
"alt-cmd-shift-c": "workspace::CopyRelativePath"
}
},
{
"bindings": {
"alt-cmd-shift-c": "workspace::CopyRelativePath"
}
}
]
"#
.unindent(),
KeybindUpdateOperation::Remove {
target: KeybindUpdateTarget {
context: None,
keystrokes: &parse_keystrokes("alt-cmd-shift-c"),
action_name: "workspace::CopyRelativePath",
action_arguments: None,
},
target_keybind_source: KeybindSource::User,
},
r#"
[
{
"bindings": {
"alt-cmd-shift-c": "workspace::CopyRelativePath"
}
}
]
"#
.unindent(),
);
}
#[test]
fn test_keymap_update_deprecated_alias_binding() {
zlog::init_test();
// The keymap file entry uses a deprecated alias of the action, while the
// update target uses the canonical name (as reported by the loaded binding).
let deprecated_aliases = &[("editor::CopyRelativePath", "workspace::CopyRelativePath")];
check_keymap_update_with_deprecated_aliases(
r#"
[
{
"bindings": {
"alt-cmd-shift-c": "editor::CopyRelativePath",
"a": "foo::Bar"
}
}
]
"#
.unindent(),
KeybindUpdateOperation::Remove {
target: KeybindUpdateTarget {
context: None,
keystrokes: &parse_keystrokes("alt-cmd-shift-c"),
action_name: "workspace::CopyRelativePath",
action_arguments: None,
},
target_keybind_source: KeybindSource::User,
},
r#"
[
{
"bindings": {
"a": "foo::Bar"
}
}
]
"#
.unindent(),
deprecated_aliases,
);
// Editing an alias entry should update it in place (rewriting it with the
// canonical name) instead of falling back to appending a new binding.
check_keymap_update_with_deprecated_aliases(
r#"
[
{
"bindings": {
"alt-cmd-shift-c": "editor::CopyRelativePath"
}
}
]
"#
.unindent(),
KeybindUpdateOperation::Replace {
target: KeybindUpdateTarget {
context: None,
keystrokes: &parse_keystrokes("alt-cmd-shift-c"),
action_name: "workspace::CopyRelativePath",
action_arguments: None,
},
source: KeybindUpdateTarget {
context: None,
keystrokes: &parse_keystrokes("ctrl-alt-c"),
action_name: "workspace::CopyRelativePath",
action_arguments: None,
},
target_keybind_source: KeybindSource::User,
},
r#"
[
{
"bindings": {
"ctrl-alt-c": "workspace::CopyRelativePath"
}
}
]
"#
.unindent(),
deprecated_aliases,
);
}
#[test]
fn test_keymap_remove() {
zlog::init_test();