diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index a356a09dea..3424a72697 100644 --- a/crates/editor/src/editor.rs +++ b/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( diff --git a/crates/gpui/src/key_dispatch.rs b/crates/gpui/src/key_dispatch.rs index c124e01c50..eb6eceeac0 100644 --- a/crates/gpui/src/key_dispatch.rs +++ b/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 { + // 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 { + 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], diff --git a/crates/gpui/src/keymap.rs b/crates/gpui/src/keymap.rs index d7434185f7..ef088259de 100644 --- a/crates/gpui/src/keymap.rs +++ b/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) -> Self { @@ -63,7 +67,7 @@ impl Keymap { } /// Iterate over all bindings, in the order they were added. - pub fn bindings(&self) -> impl DoubleEndedIterator { + pub fn bindings(&self) -> impl DoubleEndedIterator + ExactSizeIterator { self.bindings.iter() } @@ -73,6 +77,15 @@ impl Keymap { &'a self, action: &'a dyn Action, ) -> impl 'a + DoubleEndedIterator { + 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 { 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::>(); + (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) -> Option { - 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, - ) -> Option<&'a KeyBinding> { - bindings.next() - } } #[cfg(test)] diff --git a/crates/gpui/src/platform/mac/platform.rs b/crates/gpui/src/platform/mac/platform.rs index 6cfd97ad33..35bc99553d 100644 --- a/crates/gpui/src/platform/mac/platform.rs +++ b/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| { diff --git a/crates/gpui/src/window.rs b/crates/gpui/src/window.rs index af94bc3188..32d5501557 100644 --- a/crates/gpui/src/window.rs +++ b/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 { + 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 { + 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 { + 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 { 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 { + 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> { + 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 { - 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. diff --git a/crates/ui/src/components/keybinding.rs b/crates/ui/src/components/keybinding.rs index f41c76356c..b57454d7c1 100644 --- a/crates/ui/src/components/keybinding.rs +++ b/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 { - 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 { - 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)) }