Fix bindings_for_action handling of shadowed key bindings (#32220)

Fixes two things:

* ~3 months ago [in PR
#26420](https://github.com/zed-industries/zed/pull/26420/files#diff-33b58aa2da03d791c2c4761af6012851b7400e348922d64babe5fd48ac2a8e60)
`bindings_for_action` was changed to return bindings even when they are
shadowed (when the keystrokes would actually do something else).

* For edit prediction keybindings there was some odd behavior where
bindings for `edit_prediction_conflict` were taking precedence over
bindings for `edit_prediction` even when the `edit_prediction_conflict`
predicate didn't match. The workaround for this was #24812. The way it
worked was:

    - List all bindings for the action

- For each binding, get the highest precedence binding with the same
input sequence

- If the highest precedence binding has the same action, include this
binding. This was the bug - this meant that if a binding in the keymap
has the same keystrokes and action it can incorrectly take display
precedence even if its context predicate does not pass.

- Fix is to check that the highest precedence binding is a full match.
To do this efficiently, it's based on an index within the keymap
bindings.

Also adds `highest_precedence_binding_*` variants which avoid the
inefficiency of building lists of bindings just to use the last.

Release Notes:

- Fixed display of keybindings to skip bindings that are shadowed by a
binding that uses the same keystrokes.
- Fixed display of `editor::AcceptEditPrediction` bindings to use the
normal precedence that prioritizes user bindings.

crates/editor/src/editor.rs

@@ -2261,24 +2261,13 @@ impl Editor {
             window.bindings_for_action_in_context(&AcceptEditPrediction, key_context)
         };
 
-        AcceptEditPredictionBinding(
-            bindings
-                .into_iter()
-                .filter(|binding| {
-                    !in_conflict
-                        || binding
-                            .keystrokes()
-                            .first()
-                            .map_or(false, |keystroke| keystroke.modifiers.modified())
-                })
-                .rev()
-                .min_by_key(|binding| {
-                    binding
-                        .keystrokes()
-                        .first()
-                        .map_or(u8::MAX, |k| k.modifiers.number_of_modifiers())
-                }),
-        )
+        AcceptEditPredictionBinding(bindings.into_iter().rev().find(|binding| {
+            !in_conflict
+                || binding
+                    .keystrokes()
+                    .first()
+                    .map_or(false, |keystroke| keystroke.modifiers.modified())
+        }))
     }
 
     pub fn new_file(

crates/gpui/src/key_dispatch.rs

@@ -50,8 +50,8 @@
 ///  KeyBinding::new("cmd-k left", pane::SplitLeft, Some("Pane"))
 ///
 use crate::{
-    Action, ActionRegistry, App, DispatchPhase, EntityId, FocusId, KeyBinding, KeyContext, Keymap,
-    Keystroke, ModifiersChangedEvent, Window,
+    Action, ActionRegistry, App, BindingIndex, DispatchPhase, EntityId, FocusId, KeyBinding,
+    KeyContext, Keymap, Keystroke, ModifiersChangedEvent, Window,
 };
 use collections::FxHashMap;
 use smallvec::SmallVec;
@@ -392,22 +392,67 @@ impl DispatchTree {
 
     /// Returns key bindings that invoke an action on the currently focused element. Bindings are
     /// returned in the order they were added. For display, the last binding should take precedence.
+    ///
+    /// Bindings are only included if they are the highest precedence match for their keystrokes, so
+    /// shadowed bindings are not included.
     pub fn bindings_for_action(
         &self,
         action: &dyn Action,
         context_stack: &[KeyContext],
     ) -> Vec<KeyBinding> {
+        // Ideally this would return a `DoubleEndedIterator` to avoid `highest_precedence_*`
+        // methods, but this can't be done very cleanly since keymap must be borrowed.
         let keymap = self.keymap.borrow();
         keymap
-            .bindings_for_action(action)
-            .filter(|binding| {
-                let (bindings, _) = keymap.bindings_for_input(&binding.keystrokes, context_stack);
-                bindings.iter().any(|b| b.action.partial_eq(action))
+            .bindings_for_action_with_indices(action)
+            .filter(|(binding_index, binding)| {
+                Self::binding_matches_predicate_and_not_shadowed(
+                    &keymap,
+                    *binding_index,
+                    &binding.keystrokes,
+                    context_stack,
+                )
             })
-            .cloned()
+            .map(|(_, binding)| binding.clone())
             .collect()
     }
 
+    /// Returns the highest precedence binding for the given action and context stack. This is the
+    /// same as the last result of `bindings_for_action`, but more efficient than getting all bindings.
+    pub fn highest_precedence_binding_for_action(
+        &self,
+        action: &dyn Action,
+        context_stack: &[KeyContext],
+    ) -> Option<KeyBinding> {
+        let keymap = self.keymap.borrow();
+        keymap
+            .bindings_for_action_with_indices(action)
+            .rev()
+            .find_map(|(binding_index, binding)| {
+                let found = Self::binding_matches_predicate_and_not_shadowed(
+                    &keymap,
+                    binding_index,
+                    &binding.keystrokes,
+                    context_stack,
+                );
+                if found { Some(binding.clone()) } else { None }
+            })
+    }
+
+    fn binding_matches_predicate_and_not_shadowed(
+        keymap: &Keymap,
+        binding_index: BindingIndex,
+        keystrokes: &[Keystroke],
+        context_stack: &[KeyContext],
+    ) -> bool {
+        let (bindings, _) = keymap.bindings_for_input_with_indices(&keystrokes, context_stack);
+        if let Some((highest_precedence_index, _)) = bindings.iter().next() {
+            binding_index == *highest_precedence_index
+        } else {
+            false
+        }
+    }
+
     fn bindings_for_input(
         &self,
         input: &[Keystroke],

crates/gpui/src/keymap.rs

@@ -23,6 +23,10 @@ pub struct Keymap {
     version: KeymapVersion,
 }
 
+/// Index of a binding within a keymap.
+#[derive(Copy, Clone, Debug, Eq, PartialEq)]
+pub struct BindingIndex(usize);
+
 impl Keymap {
     /// Create a new keymap with the given bindings.
     pub fn new(bindings: Vec<KeyBinding>) -> Self {
@@ -63,7 +67,7 @@ impl Keymap {
     }
 
     /// Iterate over all bindings, in the order they were added.
-    pub fn bindings(&self) -> impl DoubleEndedIterator<Item = &KeyBinding> {
+    pub fn bindings(&self) -> impl DoubleEndedIterator<Item = &KeyBinding> + ExactSizeIterator {
         self.bindings.iter()
     }
 
@@ -73,6 +77,15 @@ impl Keymap {
         &'a self,
         action: &'a dyn Action,
     ) -> impl 'a + DoubleEndedIterator<Item = &'a KeyBinding> {
+        self.bindings_for_action_with_indices(action)
+            .map(|(_, binding)| binding)
+    }
+
+    /// Like `bindings_for_action_with_indices`, but also returns the binding indices.
+    pub fn bindings_for_action_with_indices<'a>(
+        &'a self,
+        action: &'a dyn Action,
+    ) -> impl 'a + DoubleEndedIterator<Item = (BindingIndex, &'a KeyBinding)> {
         let action_id = action.type_id();
         let binding_indices = self
             .binding_indices_by_action_id
@@ -105,7 +118,7 @@ impl Keymap {
                 }
             }
 
-            Some(binding)
+            Some((BindingIndex(*ix), binding))
         })
     }
 
@@ -123,7 +136,7 @@ impl Keymap {
 
     /// Returns a list of bindings that match the given input, and a boolean indicating whether or
     /// not more bindings might match if the input was longer. Bindings are returned in precedence
-    /// order.
+    /// order (higher precedence first, reverse of the order they were added to the keymap).
     ///
     /// Precedence is defined by the depth in the tree (matches on the Editor take precedence over
     /// matches on the Pane, then the Workspace, etc.). Bindings with no context are treated as the
@@ -140,18 +153,36 @@ impl Keymap {
         input: &[Keystroke],
         context_stack: &[KeyContext],
     ) -> (SmallVec<[KeyBinding; 1]>, bool) {
-        let possibilities = self.bindings().rev().filter_map(|binding| {
-            binding
-                .match_keystrokes(input)
-                .map(|pending| (binding, pending))
-        });
+        let (bindings, pending) = self.bindings_for_input_with_indices(input, context_stack);
+        let bindings = bindings
+            .into_iter()
+            .map(|(_, binding)| binding)
+            .collect::<SmallVec<[KeyBinding; 1]>>();
+        (bindings, pending)
+    }
 
-        let mut bindings: SmallVec<[(KeyBinding, usize); 1]> = SmallVec::new();
+    /// Like `bindings_for_input`, but also returns the binding indices.
+    pub fn bindings_for_input_with_indices(
+        &self,
+        input: &[Keystroke],
+        context_stack: &[KeyContext],
+    ) -> (SmallVec<[(BindingIndex, KeyBinding); 1]>, bool) {
+        let possibilities = self
+            .bindings()
+            .enumerate()
+            .rev()
+            .filter_map(|(ix, binding)| {
+                binding
+                    .match_keystrokes(input)
+                    .map(|pending| (BindingIndex(ix), binding, pending))
+            });
+
+        let mut bindings: SmallVec<[(BindingIndex, KeyBinding, usize); 1]> = SmallVec::new();
 
         // (pending, is_no_action, depth, keystrokes)
         let mut pending_info_opt: Option<(bool, bool, usize, &[Keystroke])> = None;
 
-        'outer: for (binding, pending) in possibilities {
+        'outer: for (binding_index, binding, pending) in possibilities {
             for depth in (0..=context_stack.len()).rev() {
                 if self.binding_enabled(binding, &context_stack[0..depth]) {
                     let is_no_action = is_no_action(&*binding.action);
@@ -191,20 +222,21 @@ impl Keymap {
                     }
 
                     if !pending {
-                        bindings.push((binding.clone(), depth));
+                        bindings.push((binding_index, binding.clone(), depth));
                         continue 'outer;
                     }
                 }
             }
         }
-        bindings.sort_by(|a, b| a.1.cmp(&b.1).reverse());
+        // sort by descending depth
+        bindings.sort_by(|a, b| a.2.cmp(&b.2).reverse());
         let bindings = bindings
             .into_iter()
-            .map_while(|(binding, _)| {
+            .map_while(|(binding_index, binding, _)| {
                 if is_no_action(&*binding.action) {
                     None
                 } else {
-                    Some(binding)
+                    Some((binding_index, binding))
                 }
             })
             .collect();
@@ -223,34 +255,6 @@ impl Keymap {
 
         true
     }
-
-    /// WARN: Assumes the bindings are in the order they were added to the keymap
-    /// returns the last binding for the given bindings, which
-    /// should be the user's binding in their keymap.json if they've set one,
-    /// otherwise, the last declared binding for this action in the base keymaps
-    /// (with Vim mode bindings being considered as declared later if Vim mode
-    /// is enabled)
-    ///
-    /// If you are considering changing the behavior of this function
-    /// (especially to fix a user reported issue) see issues #23621, #24931,
-    /// and possibly others as evidence that it has swapped back and forth a
-    /// couple times. The decision as of now is to pick a side and leave it
-    /// as is, until we have a better way to decide which binding to display
-    /// that is consistent and not confusing.
-    pub fn binding_to_display_from_bindings(mut bindings: Vec<KeyBinding>) -> Option<KeyBinding> {
-        bindings.pop()
-    }
-
-    /// Returns the first binding present in the iterator, which tends to be the
-    /// default binding without any key context. This is useful for cases where no
-    /// key context is available on binding display. Otherwise, bindings with a
-    /// more specific key context would take precedence and result in a
-    /// potentially invalid keybind being returned.
-    pub fn default_binding_from_bindings_iterator<'a>(
-        mut bindings: impl Iterator<Item = &'a KeyBinding>,
-    ) -> Option<&'a KeyBinding> {
-        bindings.next()
-    }
 }
 
 #[cfg(test)]

crates/gpui/src/platform/mac/platform.rs

@@ -315,6 +315,9 @@ impl MacPlatform {
                     action,
                     os_action,
                 } => {
+                    // Note that this is intentionally using earlier bindings, whereas typically
+                    // later ones take display precedence. See the discussion on
+                    // https://github.com/zed-industries/zed/issues/23621
                     let keystrokes = keymap
                         .bindings_for_action(action.as_ref())
                         .find_or_first(|binding| {

crates/gpui/src/window.rs

@@ -3248,8 +3248,7 @@ impl Window {
     /// Return a key binding string for an action, to display in the UI. Uses the highest precedence
     /// binding for the action (last binding added to the keymap).
     pub fn keystroke_text_for(&self, action: &dyn Action) -> String {
-        self.bindings_for_action(action)
-            .last()
+        self.highest_precedence_binding_for_action(action)
             .map(|binding| {
                 binding
                     .keystrokes()
@@ -3921,6 +3920,38 @@ impl Window {
             .bindings_for_action(action, &self.rendered_frame.dispatch_tree.context_stack)
     }
 
+    /// Returns the highest precedence key binding that invokes an action on the currently focused
+    /// element. This is more efficient than getting the last result of `bindings_for_action`.
+    pub fn highest_precedence_binding_for_action(&self, action: &dyn Action) -> Option<KeyBinding> {
+        self.rendered_frame
+            .dispatch_tree
+            .highest_precedence_binding_for_action(
+                action,
+                &self.rendered_frame.dispatch_tree.context_stack,
+            )
+    }
+
+    /// Returns the key bindings for an action in a context.
+    pub fn bindings_for_action_in_context(
+        &self,
+        action: &dyn Action,
+        context: KeyContext,
+    ) -> Vec<KeyBinding> {
+        let dispatch_tree = &self.rendered_frame.dispatch_tree;
+        dispatch_tree.bindings_for_action(action, &[context])
+    }
+
+    /// Returns the highest precedence key binding for an action in a context. This is more
+    /// efficient than getting the last result of `bindings_for_action_in_context`.
+    pub fn highest_precedence_binding_for_action_in_context(
+        &self,
+        action: &dyn Action,
+        context: KeyContext,
+    ) -> Option<KeyBinding> {
+        let dispatch_tree = &self.rendered_frame.dispatch_tree;
+        dispatch_tree.highest_precedence_binding_for_action(action, &[context])
+    }
+
     /// Returns any bindings that would invoke an action on the given focus handle if it were
     /// focused. Bindings are returned in the order they were added. For display, the last binding
     /// should take precedence.
@@ -3930,26 +3961,37 @@ impl Window {
         focus_handle: &FocusHandle,
     ) -> Vec<KeyBinding> {
         let dispatch_tree = &self.rendered_frame.dispatch_tree;
-
-        let Some(node_id) = dispatch_tree.focusable_node_id(focus_handle.id) else {
+        let Some(context_stack) = self.context_stack_for_focus_handle(focus_handle) else {
             return vec![];
         };
+        dispatch_tree.bindings_for_action(action, &context_stack)
+    }
+
+    /// Returns the highest precedence key binding that would invoke an action on the given focus
+    /// handle if it were focused. This is more efficient than getting the last result of
+    /// `bindings_for_action_in`.
+    pub fn highest_precedence_binding_for_action_in(
+        &self,
+        action: &dyn Action,
+        focus_handle: &FocusHandle,
+    ) -> Option<KeyBinding> {
+        let dispatch_tree = &self.rendered_frame.dispatch_tree;
+        let context_stack = self.context_stack_for_focus_handle(focus_handle)?;
+        dispatch_tree.highest_precedence_binding_for_action(action, &context_stack)
+    }
+
+    fn context_stack_for_focus_handle(
+        &self,
+        focus_handle: &FocusHandle,
+    ) -> Option<Vec<KeyContext>> {
+        let dispatch_tree = &self.rendered_frame.dispatch_tree;
+        let node_id = dispatch_tree.focusable_node_id(focus_handle.id)?;
         let context_stack: Vec<_> = dispatch_tree
             .dispatch_path(node_id)
             .into_iter()
             .filter_map(|node_id| dispatch_tree.node(node_id).context.clone())
             .collect();
-        dispatch_tree.bindings_for_action(action, &context_stack)
-    }
-
-    /// Returns the key bindings for the given action in the given context.
-    pub fn bindings_for_action_in_context(
-        &self,
-        action: &dyn Action,
-        context: KeyContext,
-    ) -> Vec<KeyBinding> {
-        let dispatch_tree = &self.rendered_frame.dispatch_tree;
-        dispatch_tree.bindings_for_action(action, &[context])
+        Some(context_stack)
     }
 
     /// Returns a generic event listener that invokes the given listener with the view and context associated with the given view handle.

crates/ui/src/components/keybinding.rs

@@ -35,8 +35,7 @@ impl KeyBinding {
         if let Some(focused) = window.focused(cx) {
             return Self::for_action_in(action, &focused, window, cx);
         }
-        let key_binding =
-            gpui::Keymap::binding_to_display_from_bindings(window.bindings_for_action(action))?;
+        let key_binding = window.highest_precedence_binding_for_action(action)?;
         Some(Self::new(key_binding, cx))
     }
 
@@ -47,9 +46,7 @@ impl KeyBinding {
         window: &mut Window,
         cx: &App,
     ) -> Option<Self> {
-        let key_binding = gpui::Keymap::binding_to_display_from_bindings(
-            window.bindings_for_action_in(action, focus),
-        )?;
+        let key_binding = window.highest_precedence_binding_for_action_in(action, focus)?;
         Some(Self::new(key_binding, cx))
     }
 
@@ -355,8 +352,7 @@ impl KeyIcon {
 
 /// Returns a textual representation of the key binding for the given [`Action`].
 pub fn text_for_action(action: &dyn Action, window: &Window, cx: &App) -> Option<String> {
-    let bindings = window.bindings_for_action(action);
-    let key_binding = bindings.last()?;
+    let key_binding = window.highest_precedence_binding_for_action(action)?;
     Some(text_for_keystrokes(key_binding.keystrokes(), cx))
 }