From ece2af1e223b299b329d4b2b1762daa8a47798c2 Mon Sep 17 00:00:00 2001 From: Mikayla Maki Date: Fri, 10 Mar 2023 15:36:20 -0800 Subject: [PATCH] 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 2f29815f0328b29331ecdbab2291189d0372299a..1dbea615edccd859c571d6cd7b013867bbb1e509 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);