gpui: Simplify `bindings_for_action` API (#34857)

Ben Kunkle and Conrad created 4 months ago

Closes #ISSUE

Simplifies the API to no longer have a variant that returns indices. The
downside is that a few places that used to call
`bindings_for_action_with_indices` now compare `Box<dyn Action>` instead
of indices, however the result is the removal of wrapper code and index
handling that is largely unnecessary

Release Notes:

- N/A *or* Added/Fixed/Improved ...

Co-authored-by: Conrad <[e-mail]>

Change summary

crates/gpui/src/key_dispatch.rs | 39 +++++++++----------------
crates/gpui/src/keymap.rs       | 53 +++++++++++-----------------------
2 files changed, 31 insertions(+), 61 deletions(-)

Detailed changes

crates/gpui/src/key_dispatch.rs

@@ -50,8 +50,8 @@
 ///  KeyBinding::new("cmd-k left", pane::SplitLeft, Some("Pane"))
 ///
 use crate::{
-    Action, ActionRegistry, App, BindingIndex, DispatchPhase, EntityId, FocusId, KeyBinding,
-    KeyContext, Keymap, Keystroke, ModifiersChangedEvent, Window,
+    Action, ActionRegistry, App, DispatchPhase, EntityId, FocusId, KeyBinding, KeyContext, Keymap,
+    Keystroke, ModifiersChangedEvent, Window,
 };
 use collections::FxHashMap;
 use smallvec::SmallVec;
@@ -406,16 +406,11 @@ impl DispatchTree {
         // methods, but this can't be done very cleanly since keymap must be borrowed.
         let keymap = self.keymap.borrow();
         keymap
-            .bindings_for_action_with_indices(action)
-            .filter(|(binding_index, binding)| {
-                Self::binding_matches_predicate_and_not_shadowed(
-                    &keymap,
-                    *binding_index,
-                    &binding.keystrokes,
-                    context_stack,
-                )
+            .bindings_for_action(action)
+            .filter(|binding| {
+                Self::binding_matches_predicate_and_not_shadowed(&keymap, &binding, context_stack)
             })
-            .map(|(_, binding)| binding.clone())
+            .cloned()
             .collect()
     }
 
@@ -428,28 +423,22 @@ impl DispatchTree {
     ) -> Option<KeyBinding> {
         let keymap = self.keymap.borrow();
         keymap
-            .bindings_for_action_with_indices(action)
+            .bindings_for_action(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 }
+            .find(|binding| {
+                Self::binding_matches_predicate_and_not_shadowed(&keymap, &binding, context_stack)
             })
+            .cloned()
     }
 
     fn binding_matches_predicate_and_not_shadowed(
         keymap: &Keymap,
-        binding_index: BindingIndex,
-        keystrokes: &[Keystroke],
+        binding: &KeyBinding,
         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
+        let (bindings, _) = keymap.bindings_for_input(&binding.keystrokes, context_stack);
+        if let Some(found) = bindings.iter().next() {
+            found.action.partial_eq(binding.action.as_ref())
         } else {
             false
         }

crates/gpui/src/keymap.rs

@@ -77,15 +77,6 @@ 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
@@ -118,7 +109,7 @@ impl Keymap {
                 }
             }
 
-            Some((BindingIndex(*ix), binding))
+            Some(binding)
         })
     }
 
@@ -153,22 +144,8 @@ impl Keymap {
         input: &[Keystroke],
         context_stack: &[KeyContext],
     ) -> (SmallVec<[KeyBinding; 1]>, bool) {
-        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)
-    }
-
-    /// 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 mut bindings: SmallVec<[(usize, BindingIndex, &KeyBinding); 1]> = SmallVec::new();
-        let mut pending_bindings: SmallVec<[(BindingIndex, &KeyBinding); 1]> = SmallVec::new();
+        let mut matched_bindings = SmallVec::<[(usize, BindingIndex, &KeyBinding); 1]>::new();
+        let mut pending_bindings = SmallVec::<[(BindingIndex, &KeyBinding); 1]>::new();
 
         for (ix, binding) in self.bindings().enumerate().rev() {
             let Some(depth) = self.binding_enabled(binding, &context_stack) else {
@@ -179,26 +156,30 @@ impl Keymap {
             };
 
             if !pending {
-                bindings.push((depth, BindingIndex(ix), binding))
+                matched_bindings.push((depth, BindingIndex(ix), binding));
             } else {
-                pending_bindings.push((BindingIndex(ix), binding))
+                pending_bindings.push((BindingIndex(ix), binding));
             }
         }
 
-        bindings.sort_by(|(depth_a, ix_a, _), (depth_b, ix_b, _)| {
+        matched_bindings.sort_by(|(depth_a, ix_a, _), (depth_b, ix_b, _)| {
             depth_b.cmp(depth_a).then(ix_b.cmp(ix_a))
         });
 
-        let bindings: SmallVec<[_; 1]> = bindings
-            .into_iter()
-            .take_while(|(_, _, binding)| !is_no_action(&*binding.action))
-            .map(|(_, ix, binding)| (ix, binding.clone()))
-            .collect();
+        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;
+            }
+            bindings.push(binding.clone());
+            first_binding_index.get_or_insert(ix);
+        }
 
         let mut pending = HashSet::default();
         for (ix, binding) in pending_bindings.into_iter().rev() {
-            if let Some((binding_ix, _)) = bindings.first()
-                && *binding_ix > ix
+            if let Some(binding_ix) = first_binding_index
+                && binding_ix > ix
             {
                 continue;
             }