diff --git a/crates/keymap_editor/src/keymap_editor.rs b/crates/keymap_editor/src/keymap_editor.rs index 0bb66ab988..aecaea7fcb 100644 --- a/crates/keymap_editor/src/keymap_editor.rs +++ b/crates/keymap_editor/src/keymap_editor.rs @@ -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, 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, 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, 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, Entity, 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 { + 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() { diff --git a/crates/settings/src/keymap_file.rs b/crates/settings/src/keymap_file.rs index 9f734939d7..72d5606af4 100644 --- a/crates/settings/src/keymap_file.rs +++ b/crates/settings/src/keymap_file.rs @@ -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 { // 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> { 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> { 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();