From 0384456e7d05526dc9b81124ae15a62ddaa6c9bd Mon Sep 17 00:00:00 2001 From: Mikayla Maki Date: Thu, 9 Mar 2023 16:18:37 -0800 Subject: [PATCH 1/4] Change context matcher to search the entire stack --- crates/gpui/src/keymap_matcher/keymap_context.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/crates/gpui/src/keymap_matcher/keymap_context.rs b/crates/gpui/src/keymap_matcher/keymap_context.rs index bbf6bfc14b..fbaaa5fbc5 100644 --- a/crates/gpui/src/keymap_matcher/keymap_context.rs +++ b/crates/gpui/src/keymap_matcher/keymap_context.rs @@ -64,7 +64,13 @@ impl KeymapContextPredicate { pub fn eval(&self, contexts: &[KeymapContext]) -> bool { let Some(context) = contexts.first() else { return false }; match self { - Self::Identifier(name) => (&context.set).contains(name.as_str()), + Self::Identifier(name) => { + if (&context.set).contains(name.as_str()) { + true + } else { + self.eval(&contexts[1..]) + } + } Self::Equal(left, right) => context .map .get(left.as_str()) From 9dc608dc4b4dedd11638d7cb472456e77d9272b6 Mon Sep 17 00:00:00 2001 From: Mikayla Maki Date: Thu, 9 Mar 2023 19:32:09 -0800 Subject: [PATCH 2/4] Switch from changing the meaning of the predicate to adding an additional match_dispatch_path_context API for UI elements --- crates/gpui/src/app.rs | 4 ++-- crates/gpui/src/keymap_matcher/binding.rs | 9 +++++++++ crates/gpui/src/keymap_matcher/keymap_context.rs | 8 +------- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/crates/gpui/src/app.rs b/crates/gpui/src/app.rs index 31563010b7..ab88a39a9a 100644 --- a/crates/gpui/src/app.rs +++ b/crates/gpui/src/app.rs @@ -1248,7 +1248,7 @@ impl MutableAppContext { self.keystroke_matcher .bindings_for_action_type(action.as_any().type_id()) .find_map(|b| { - if b.match_context(&contexts) { + if b.match_dispatch_path_context(&contexts) { Some(b.keystrokes().into()) } else { None @@ -1283,7 +1283,7 @@ impl MutableAppContext { deserialize("{}").ok()?, self.keystroke_matcher .bindings_for_action_type(*type_id) - .filter(|b| b.match_context(&contexts)) + .filter(|b| b.match_dispatch_path_context(&contexts)) .collect(), )) } else { diff --git a/crates/gpui/src/keymap_matcher/binding.rs b/crates/gpui/src/keymap_matcher/binding.rs index c1cfd14e82..1b0217b5ff 100644 --- a/crates/gpui/src/keymap_matcher/binding.rs +++ b/crates/gpui/src/keymap_matcher/binding.rs @@ -42,6 +42,15 @@ impl Binding { .unwrap_or(true) } + pub fn match_dispatch_path_context(&self, contexts: &[KeymapContext]) -> bool { + for i in 0..contexts.len() { + if self.match_context(&contexts[i..]) { + return true; + } + } + false + } + pub fn match_keys_and_context( &self, pending_keystrokes: &Vec, diff --git a/crates/gpui/src/keymap_matcher/keymap_context.rs b/crates/gpui/src/keymap_matcher/keymap_context.rs index fbaaa5fbc5..bbf6bfc14b 100644 --- a/crates/gpui/src/keymap_matcher/keymap_context.rs +++ b/crates/gpui/src/keymap_matcher/keymap_context.rs @@ -64,13 +64,7 @@ impl KeymapContextPredicate { pub fn eval(&self, contexts: &[KeymapContext]) -> bool { let Some(context) = contexts.first() else { return false }; match self { - Self::Identifier(name) => { - if (&context.set).contains(name.as_str()) { - true - } else { - self.eval(&contexts[1..]) - } - } + Self::Identifier(name) => (&context.set).contains(name.as_str()), Self::Equal(left, right) => context .map .get(left.as_str()) From 00a38e4c3b1149ba11f0a5dbde0b1be78681f5db Mon Sep 17 00:00:00 2001 From: Mikayla Maki Date: Fri, 10 Mar 2023 12:12:32 -0800 Subject: [PATCH 3/4] Bound the search range for the keybindings by the highest handler path --- crates/gpui/src/app.rs | 58 ++++++++++++++++++----- crates/gpui/src/keymap_matcher/binding.rs | 9 ---- 2 files changed, 46 insertions(+), 21 deletions(-) diff --git a/crates/gpui/src/app.rs b/crates/gpui/src/app.rs index ab88a39a9a..2f29815f03 100644 --- a/crates/gpui/src/app.rs +++ b/crates/gpui/src/app.rs @@ -485,7 +485,9 @@ pub struct MutableAppContext { cx: AppContext, action_deserializers: HashMap<&'static str, (TypeId, DeserializeActionCallback)>, capture_actions: HashMap>>>, + // Entity Types -> { Action Types -> Action Handlers } actions: HashMap>>>, + // Action Types -> Action Handlers global_actions: HashMap>, keystroke_matcher: KeymapMatcher, next_entity_id: usize, @@ -1239,20 +1241,34 @@ impl MutableAppContext { action: &dyn Action, ) -> Option> { let mut contexts = Vec::new(); - for view_id in self.ancestors(window_id, view_id) { + let mut highest_handler = None; + for (i, view_id) in self.ancestors(window_id, view_id).enumerate() { if let Some(view) = self.views.get(&(window_id, view_id)) { + if let Some(actions) = self.actions.get(&view.as_any().type_id()) { + if actions.contains_key(&action.as_any().type_id()) { + highest_handler = Some(i); + } + } contexts.push(view.keymap_context(self)); } } + if self.global_actions.contains_key(&action.as_any().type_id()) { + highest_handler = Some(contexts.len()) + } + self.keystroke_matcher .bindings_for_action_type(action.as_any().type_id()) .find_map(|b| { - if b.match_dispatch_path_context(&contexts) { - Some(b.keystrokes().into()) - } else { - None - } + highest_handler + .map(|highest_handler| { + if (0..=highest_handler).any(|depth| b.match_context(&contexts[depth..])) { + Some(b.keystrokes().into()) + } else { + None + } + }) + .flatten() }) } @@ -1261,29 +1277,47 @@ impl MutableAppContext { window_id: usize, view_id: usize, ) -> impl Iterator, SmallVec<[&Binding; 1]>)> { - let mut action_types: HashSet<_> = self.global_actions.keys().copied().collect(); - let mut contexts = Vec::new(); - for view_id in self.ancestors(window_id, view_id) { + let mut handler_depth_by_action_type = HashMap::>::default(); + for (i, view_id) in self.ancestors(window_id, view_id).enumerate() { if let Some(view) = self.views.get(&(window_id, view_id)) { contexts.push(view.keymap_context(self)); let view_type = view.as_any().type_id(); if let Some(actions) = self.actions.get(&view_type) { - action_types.extend(actions.keys().copied()); + handler_depth_by_action_type.extend( + actions + .keys() + .copied() + .map(|action_type| (action_type, Some(i))), + ); } } } + handler_depth_by_action_type.extend( + self.global_actions + .keys() + .copied() + .map(|action_type| (action_type, None)), + ); + self.action_deserializers .iter() .filter_map(move |(name, (type_id, deserialize))| { - if action_types.contains(type_id) { + if let Some(action_depth) = handler_depth_by_action_type.get(type_id) { Some(( *name, deserialize("{}").ok()?, self.keystroke_matcher .bindings_for_action_type(*type_id) - .filter(|b| b.match_dispatch_path_context(&contexts)) + .filter(|b| { + if let Some(action_depth) = *action_depth { + (0..=action_depth) + .any(|depth| b.match_context(&contexts[depth..])) + } else { + true + } + }) .collect(), )) } else { diff --git a/crates/gpui/src/keymap_matcher/binding.rs b/crates/gpui/src/keymap_matcher/binding.rs index 1b0217b5ff..c1cfd14e82 100644 --- a/crates/gpui/src/keymap_matcher/binding.rs +++ b/crates/gpui/src/keymap_matcher/binding.rs @@ -42,15 +42,6 @@ impl Binding { .unwrap_or(true) } - pub fn match_dispatch_path_context(&self, contexts: &[KeymapContext]) -> bool { - for i in 0..contexts.len() { - if self.match_context(&contexts[i..]) { - return true; - } - } - false - } - pub fn match_keys_and_context( &self, pending_keystrokes: &Vec, From ece2af1e223b299b329d4b2b1762daa8a47798c2 Mon Sep 17 00:00:00 2001 From: Mikayla Maki Date: Fri, 10 Mar 2023 15:36:20 -0800 Subject: [PATCH 4/4] Fix a corner case in available action resolution Add tests for new keybinding resolution behavior co-authored-by: max --- crates/gpui/src/app.rs | 152 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 135 insertions(+), 17 deletions(-) diff --git a/crates/gpui/src/app.rs b/crates/gpui/src/app.rs index 2f29815f03..1dbea615ed 100644 --- a/crates/gpui/src/app.rs +++ b/crates/gpui/src/app.rs @@ -1241,12 +1241,12 @@ impl MutableAppContext { action: &dyn Action, ) -> Option> { let mut contexts = Vec::new(); - let mut highest_handler = None; + let mut handler_depth = None; for (i, view_id) in self.ancestors(window_id, view_id).enumerate() { if let Some(view) = self.views.get(&(window_id, view_id)) { if let Some(actions) = self.actions.get(&view.as_any().type_id()) { if actions.contains_key(&action.as_any().type_id()) { - highest_handler = Some(i); + handler_depth = Some(i); } } contexts.push(view.keymap_context(self)); @@ -1254,13 +1254,13 @@ impl MutableAppContext { } if self.global_actions.contains_key(&action.as_any().type_id()) { - highest_handler = Some(contexts.len()) + handler_depth = Some(contexts.len()) } self.keystroke_matcher .bindings_for_action_type(action.as_any().type_id()) .find_map(|b| { - highest_handler + handler_depth .map(|highest_handler| { if (0..=highest_handler).any(|depth| b.match_context(&contexts[depth..])) { Some(b.keystrokes().into()) @@ -1278,45 +1278,40 @@ impl MutableAppContext { view_id: usize, ) -> impl Iterator, SmallVec<[&Binding; 1]>)> { let mut contexts = Vec::new(); - let mut handler_depth_by_action_type = HashMap::>::default(); - for (i, view_id) in self.ancestors(window_id, view_id).enumerate() { + let mut handler_depths_by_action_type = HashMap::::default(); + for (depth, view_id) in self.ancestors(window_id, view_id).enumerate() { if let Some(view) = self.views.get(&(window_id, view_id)) { contexts.push(view.keymap_context(self)); let view_type = view.as_any().type_id(); if let Some(actions) = self.actions.get(&view_type) { - handler_depth_by_action_type.extend( + handler_depths_by_action_type.extend( actions .keys() .copied() - .map(|action_type| (action_type, Some(i))), + .map(|action_type| (action_type, depth)), ); } } } - handler_depth_by_action_type.extend( + handler_depths_by_action_type.extend( self.global_actions .keys() .copied() - .map(|action_type| (action_type, None)), + .map(|action_type| (action_type, contexts.len())), ); self.action_deserializers .iter() .filter_map(move |(name, (type_id, deserialize))| { - if let Some(action_depth) = handler_depth_by_action_type.get(type_id) { + if let Some(action_depth) = handler_depths_by_action_type.get(type_id).copied() { Some(( *name, deserialize("{}").ok()?, self.keystroke_matcher .bindings_for_action_type(*type_id) .filter(|b| { - if let Some(action_depth) = *action_depth { - (0..=action_depth) - .any(|depth| b.match_context(&contexts[depth..])) - } else { - true - } + (0..=action_depth).any(|depth| b.match_context(&contexts[depth..])) }) .collect(), )) @@ -5321,6 +5316,7 @@ impl Subscription { mod tests { use super::*; use crate::{actions, elements::*, impl_actions, MouseButton, MouseButtonEvent}; + use itertools::Itertools; use postage::{sink::Sink, stream::Stream}; use serde::Deserialize; use smol::future::poll_once; @@ -6751,6 +6747,128 @@ mod tests { actions.borrow_mut().clear(); } + #[crate::test(self)] + fn test_keystrokes_for_action(cx: &mut MutableAppContext) { + actions!(test, [Action1, Action2, GlobalAction]); + + struct View1 {} + struct View2 {} + + impl Entity for View1 { + type Event = (); + } + impl Entity for View2 { + type Event = (); + } + + impl super::View for View1 { + fn render(&mut self, _: &mut RenderContext) -> ElementBox { + Empty::new().boxed() + } + fn ui_name() -> &'static str { + "View1" + } + } + impl super::View for View2 { + fn render(&mut self, _: &mut RenderContext) -> ElementBox { + Empty::new().boxed() + } + fn ui_name() -> &'static str { + "View2" + } + } + + let (window_id, view_1) = cx.add_window(Default::default(), |_| View1 {}); + let view_2 = cx.add_view(&view_1, |cx| { + cx.focus_self(); + View2 {} + }); + + cx.add_action(|_: &mut View1, _: &Action1, _cx| {}); + cx.add_action(|_: &mut View2, _: &Action2, _cx| {}); + cx.add_global_action(|_: &GlobalAction, _| {}); + + cx.add_bindings(vec![ + Binding::new("a", Action1, Some("View1")), + Binding::new("b", Action2, Some("View1 > View2")), + Binding::new("c", GlobalAction, Some("View3")), // View 3 does not exist + ]); + + // Sanity check + assert_eq!( + cx.keystrokes_for_action(window_id, view_1.id(), &Action1) + .unwrap() + .as_slice(), + &[Keystroke::parse("a").unwrap()] + ); + assert_eq!( + cx.keystrokes_for_action(window_id, view_2.id(), &Action2) + .unwrap() + .as_slice(), + &[Keystroke::parse("b").unwrap()] + ); + + // The 'a' keystroke propagates up the view tree from view_2 + // to view_1. The action, Action1, is handled by view_1. + assert_eq!( + cx.keystrokes_for_action(window_id, view_2.id(), &Action1) + .unwrap() + .as_slice(), + &[Keystroke::parse("a").unwrap()] + ); + + // Actions that are handled below the current view don't have bindings + assert_eq!( + cx.keystrokes_for_action(window_id, view_1.id(), &Action2), + None + ); + + // Actions that are handled in other branches of the tree should not have a binding + assert_eq!( + cx.keystrokes_for_action(window_id, view_2.id(), &GlobalAction), + None + ); + + // Produces a list of actions and keybindings + fn available_actions( + window_id: usize, + view_id: usize, + cx: &mut MutableAppContext, + ) -> Vec<(&'static str, Vec)> { + cx.available_actions(window_id, view_id) + .map(|(action_name, _, bindings)| { + ( + action_name, + bindings + .iter() + .map(|binding| binding.keystrokes()[0].clone()) + .collect::>(), + ) + }) + .sorted_by(|(name1, _), (name2, _)| name1.cmp(name2)) + .collect() + } + + // Check that global actions do not have a binding, even if a binding does exist in another view + assert_eq!( + &available_actions(window_id, view_1.id(), cx), + &[ + ("test::Action1", vec![Keystroke::parse("a").unwrap()]), + ("test::GlobalAction", vec![]) + ], + ); + + // Check that view 1 actions and bindings are available even when called from view 2 + assert_eq!( + &available_actions(window_id, view_2.id(), cx), + &[ + ("test::Action1", vec![Keystroke::parse("a").unwrap()]), + ("test::Action2", vec![Keystroke::parse("b").unwrap()]), + ("test::GlobalAction", vec![]), + ], + ); + } + #[crate::test(self)] async fn test_model_condition(cx: &mut TestAppContext) { struct Counter(usize);