diff --git a/crates/gpui/src/keymap.rs b/crates/gpui/src/keymap.rs index 03739feb55248884694c3ad1834b80d0ffd4a003..27ca70e32827fdb2da7600bf057f430dc4e19821 100644 --- a/crates/gpui/src/keymap.rs +++ b/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, + ) -> Option<&'a KeyBinding> { + return bindings.into_iter().last(); + } } #[cfg(test)] diff --git a/crates/gpui/src/platform/mac/platform.rs b/crates/gpui/src/platform/mac/platform.rs index 8fa7d4ffa6040272d1d61097289d3ea340c2f0a3..a678c778961817cc41e13e8e7e8c42da15a3f4bd 100644 --- a/crates/gpui/src/platform/mac/platform.rs +++ b/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:"), diff --git a/crates/ui/src/components/keybinding.rs b/crates/ui/src/components/keybinding.rs index 4a122b6e160e013e3fe61c6e23b0015f8479afb0..c3a3e6579d4961e956cd605b711e8f45fe37e8dd 100644 --- a/crates/ui/src/components/keybinding.rs +++ b/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 { - 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 { - 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)) }