Change keymap precedence to favor user (#37557)

Mitch (a.k.a Voz) created 2 months ago

Closes #35623 

Previously if a base keymap had a `null` set to an action, leading to a
`NoAction` being assigned to the keymap, if a user wanted to take
advantage of that keymap (in this particular case, `cmd-2`), the keymap
binding check would favor the `NoAction` over the user, since
technically the context depth matched better. Instead, we should always
prefer the user's settings over whatever base or default.

Release Notes:

- Fixed keymap precedence by favoring user settings over base keymap /
configs.

Change summary

crates/gpui/src/keymap.rs               | 69 ++++++++++++++++++++++++++
crates/vim/src/test/vim_test_context.rs |  5 +
2 files changed, 72 insertions(+), 2 deletions(-)

Detailed changes

crates/gpui/src/keymap.rs

@@ -168,9 +168,21 @@ impl Keymap {
 
         let mut bindings: SmallVec<[_; 1]> = SmallVec::new();
         let mut first_binding_index = None;
+
         for (_, ix, binding) in matched_bindings {
             if is_no_action(&*binding.action) {
-                break;
+                // Only break if this is a user-defined NoAction binding
+                // This allows user keymaps to override base keymap NoAction bindings
+                if let Some(meta) = binding.meta {
+                    if meta.0 == 0 {
+                        break;
+                    }
+                } else {
+                    // If no meta is set, assume it's a user binding for safety
+                    break;
+                }
+                // For non-user NoAction bindings, continue searching for user overrides
+                continue;
             }
             bindings.push(binding.clone());
             first_binding_index.get_or_insert(ix);
@@ -617,6 +629,33 @@ mod tests {
         assert!(!matched.1);
     }
 
+    #[test]
+    fn test_context_precedence_with_same_source() {
+        // Test case: User has both Workspace and Editor bindings for the same key
+        // Editor binding should take precedence over Workspace binding
+        let bindings = [
+            KeyBinding::new("cmd-r", ActionAlpha {}, Some("Workspace")),
+            KeyBinding::new("cmd-r", ActionBeta {}, Some("Editor")),
+        ];
+
+        let mut keymap = Keymap::default();
+        keymap.add_bindings(bindings);
+
+        // Test with context stack: [Workspace, Editor] (Editor is deeper)
+        let (result, _) = keymap.bindings_for_input(
+            &[Keystroke::parse("cmd-r").unwrap()],
+            &[
+                KeyContext::parse("Workspace").unwrap(),
+                KeyContext::parse("Editor").unwrap(),
+            ],
+        );
+
+        // Both bindings should be returned, but Editor binding should be first (highest precedence)
+        assert_eq!(result.len(), 2);
+        assert!(result[0].action.partial_eq(&ActionBeta {})); // Editor binding first
+        assert!(result[1].action.partial_eq(&ActionAlpha {})); // Workspace binding second
+    }
+
     #[test]
     fn test_bindings_for_action() {
         let bindings = [
@@ -643,4 +682,32 @@ mod tests {
             assert_eq!(actual, expected, "{:?}", action);
         }
     }
+
+    #[test]
+    fn test_source_precedence_sorting() {
+        // KeybindSource precedence: User (0) > Vim (1) > Base (2) > Default (3)
+        // Test that user keymaps take precedence over default keymaps at the same context depth
+        let mut keymap = Keymap::default();
+
+        // Add a default keymap binding first
+        let mut default_binding = KeyBinding::new("cmd-r", ActionAlpha {}, Some("Editor"));
+        default_binding.set_meta(KeyBindingMetaIndex(3)); // Default source
+        keymap.add_bindings([default_binding]);
+
+        // Add a user keymap binding
+        let mut user_binding = KeyBinding::new("cmd-r", ActionBeta {}, Some("Editor"));
+        user_binding.set_meta(KeyBindingMetaIndex(0)); // User source
+        keymap.add_bindings([user_binding]);
+
+        // Test with Editor context stack
+        let (result, _) = keymap.bindings_for_input(
+            &[Keystroke::parse("cmd-r").unwrap()],
+            &[KeyContext::parse("Editor").unwrap()],
+        );
+
+        // User binding should take precedence over default binding
+        assert_eq!(result.len(), 2);
+        assert!(result[0].action.partial_eq(&ActionBeta {}));
+        assert!(result[1].action.partial_eq(&ActionAlpha {}));
+    }
 }

crates/vim/src/test/vim_test_context.rs

@@ -70,11 +70,14 @@ impl VimTestContext {
         SettingsStore::update_global(cx, |store, cx| {
             store.update_user_settings::<VimModeSetting>(cx, |s| s.vim_mode = Some(enabled));
         });
-        let default_key_bindings = settings::KeymapFile::load_asset_allow_partial_failure(
+        let mut default_key_bindings = settings::KeymapFile::load_asset_allow_partial_failure(
             "keymaps/default-macos.json",
             cx,
         )
         .unwrap();
+        for key_binding in &mut default_key_bindings {
+            key_binding.set_meta(settings::KeybindSource::Default.meta());
+        }
         cx.bind_keys(default_key_bindings);
         if enabled {
             let vim_key_bindings = settings::KeymapFile::load_asset(