Centralize logic around which keybind to display (#25215)

Ben Kunkle created 10 months ago

Closes #24931

We've flipped back and forth at least once on whether the last or first
added keybinding should be shown in different contexts (See
[this](https://github.com/zed-industries/zed/issues/23621#issuecomment-2614061385)
as well as #23621 and the subsequent #23660)

This PR attempts to pick a side to stick with so that we are at least
consistent until #23660 is resolved and we have a way to determine which
keybinds to display in a manner that is both consistent and not
confusing

Release Notes:

- N/A

Change summary

crates/gpui/src/keymap.rs                | 19 +++++++++++++++++++
crates/gpui/src/platform/mac/platform.rs | 13 ++++---------
crates/ui/src/components/keybinding.rs   | 17 +++++++----------
3 files changed, 30 insertions(+), 19 deletions(-)

Detailed changes

crates/gpui/src/keymap.rs

@@ -188,6 +188,25 @@ 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<'a>(
+        bindings: impl IntoIterator<Item = &'a KeyBinding>,
+    ) -> Option<&'a KeyBinding> {
+        return bindings.into_iter().last();
+    }
 }
 
 #[cfg(test)]

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

@@ -290,15 +290,10 @@ impl MacPlatform {
                 action,
                 os_action,
             } => {
-                // Note that this is not the standard logic for selecting which keybinding to
-                // display. Typically the last binding takes precedence for display. However, in
-                // this case the menus are not updated on context changes. To make these bindings
-                // more likely to be correct, the first binding instead takes precedence (typically
-                // from the base keymap).
-                let keystrokes = keymap
-                    .bindings_for_action(action.as_ref())
-                    .next()
-                    .map(|binding| binding.keystrokes());
+                let keystrokes = crate::Keymap::binding_to_display_from_bindings(
+                    keymap.bindings_for_action(action.as_ref()),
+                )
+                .map(|binding| binding.keystrokes());
 
                 let selector = match os_action {
                     Some(crate::OsAction::Cut) => selector("cut:"),

crates/ui/src/components/keybinding.rs

@@ -30,11 +30,9 @@ impl KeyBinding {
     /// Returns the highest precedence keybinding for an action. This is the last binding added to
     /// the keymap. User bindings are added after built-in bindings so that they take precedence.
     pub fn for_action(action: &dyn Action, window: &mut Window, cx: &App) -> Option<Self> {
-        let key_binding = window
-            .bindings_for_action(action)
-            .into_iter()
-            .rev()
-            .next()?;
+        let key_binding =
+            gpui::Keymap::binding_to_display_from_bindings(&window.bindings_for_action(action))
+                .cloned()?;
         Some(Self::new(key_binding, cx))
     }
 
@@ -45,11 +43,10 @@ impl KeyBinding {
         window: &mut Window,
         cx: &App,
     ) -> Option<Self> {
-        let key_binding = window
-            .bindings_for_action_in(action, focus)
-            .into_iter()
-            .rev()
-            .next()?;
+        let key_binding = gpui::Keymap::binding_to_display_from_bindings(
+            &window.bindings_for_action_in(action, focus),
+        )
+        .cloned()?;
         Some(Self::new(key_binding, cx))
     }