From 8d94de8eb2e6b6b48f9a45b4aeb7a92c7176f4b8 Mon Sep 17 00:00:00 2001 From: Mikayla Maki Date: Thu, 27 Oct 2022 11:27:26 -0700 Subject: [PATCH 1/5] WIP: Change to matches to vec --- crates/editor/src/blink_manager.rs | 1 - crates/gpui/src/app.rs | 7 +++++++ crates/gpui/src/keymap.rs | 16 +++++++--------- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/crates/editor/src/blink_manager.rs b/crates/editor/src/blink_manager.rs index df66ec2125a8a7fa73a2bf9cc2943cd04e29fc88..681692f0f5428362c35caca8f4030d8222821cec 100644 --- a/crates/editor/src/blink_manager.rs +++ b/crates/editor/src/blink_manager.rs @@ -71,7 +71,6 @@ impl BlinkManager { if epoch == self.blink_epoch && self.enabled && !self.blinking_paused { self.visible = !self.visible; cx.notify(); - dbg!(cx.handle()); let epoch = self.next_blink_epoch(); let interval = self.blink_interval; diff --git a/crates/gpui/src/app.rs b/crates/gpui/src/app.rs index 925a4542aae4adf77a85dfd1e4ed99487f92a5c8..17aa758bb64a1183b53050560ba511e07b2db144 100644 --- a/crates/gpui/src/app.rs +++ b/crates/gpui/src/app.rs @@ -1524,6 +1524,13 @@ impl MutableAppContext { pub fn dispatch_keystroke(&mut self, window_id: usize, keystroke: &Keystroke) -> bool { if let Some(focused_view_id) = self.focused_view_id(window_id) { + // For each view in self + // - Get it's keymap context + // - Try to execute keybinding + + // For us, get out dispatch path + // - ONLY execute for that terminal path + let dispatch_path = self .ancestors(window_id, focused_view_id) .map(|view_id| { diff --git a/crates/gpui/src/keymap.rs b/crates/gpui/src/keymap.rs index 603190a810315e91e4f844f89c958341b1d80505..9218b849ecec60f744060d2b168a9c13ecd45c51 100644 --- a/crates/gpui/src/keymap.rs +++ b/crates/gpui/src/keymap.rs @@ -77,10 +77,7 @@ where pub enum MatchResult { None, Pending, - Match { - view_id: usize, - action: Box, - }, + Match(Vec<(usize, Box)>), } impl Debug for MatchResult { @@ -157,8 +154,10 @@ impl Matcher { dispatch_path: Vec<(usize, Context)>, ) -> MatchResult { let mut any_pending = false; + let mut matched_bindings = Vec::new(); let first_keystroke = self.pending.is_empty(); + dbg!(&dispatch_path); for (view_id, context) in dispatch_path { if !first_keystroke && !self.pending.contains_key(&view_id) { continue; @@ -185,10 +184,7 @@ impl Matcher { { if binding.keystrokes.len() == pending.keystrokes.len() { self.pending.remove(&view_id); - return MatchResult::Match { - view_id, - action: binding.action.boxed_clone(), - }; + matched_bindings.push((view_id, binding.action.boxed_clone())); } else { retain_pending = true; pending.context = Some(context.clone()); @@ -203,7 +199,9 @@ impl Matcher { } } - if any_pending { + if !matched_bindings.is_empty() { + MatchResult::Match(matched_bindings) + } else if any_pending { MatchResult::Pending } else { MatchResult::None From e02199fa2ae59168974a2ba029ba9da451536d85 Mon Sep 17 00:00:00 2001 From: K Simmons Date: Thu, 27 Oct 2022 12:33:51 -0700 Subject: [PATCH 2/5] fixed binding fallback --- crates/gpui/src/app.rs | 21 +++---- crates/gpui/src/keymap.rs | 116 +++++++++++++++++--------------------- 2 files changed, 64 insertions(+), 73 deletions(-) diff --git a/crates/gpui/src/app.rs b/crates/gpui/src/app.rs index 17aa758bb64a1183b53050560ba511e07b2db144..ea3fee4fdf95b2d7ace0829c3127d4320015b147 100644 --- a/crates/gpui/src/app.rs +++ b/crates/gpui/src/app.rs @@ -1551,17 +1551,18 @@ impl MutableAppContext { { MatchResult::None => false, MatchResult::Pending => true, - MatchResult::Match { view_id, action } => { - if self.handle_dispatch_action_from_effect( - window_id, - Some(view_id), - action.as_ref(), - ) { - self.keystroke_matcher.clear_pending(); - true - } else { - false + MatchResult::Matches(matches) => { + for (view_id, action) in matches { + if self.handle_dispatch_action_from_effect( + window_id, + Some(view_id), + action.as_ref(), + ) { + self.keystroke_matcher.clear_pending(); + return true; + } } + false } } } else { diff --git a/crates/gpui/src/keymap.rs b/crates/gpui/src/keymap.rs index 9218b849ecec60f744060d2b168a9c13ecd45c51..89831f2c7ff90d43590ca4b9e8b14f7784d1acee 100644 --- a/crates/gpui/src/keymap.rs +++ b/crates/gpui/src/keymap.rs @@ -13,16 +13,11 @@ extern "C" { } pub struct Matcher { - pending: HashMap, + pending_views: HashMap, + pending_keystrokes: Vec, keymap: Keymap, } -#[derive(Default)] -struct Pending { - keystrokes: Vec, - context: Option, -} - #[derive(Default)] pub struct Keymap { bindings: Vec, @@ -77,7 +72,7 @@ where pub enum MatchResult { None, Pending, - Match(Vec<(usize, Box)>), + Matches(Vec<(usize, Box)>), } impl Debug for MatchResult { @@ -85,10 +80,13 @@ impl Debug for MatchResult { match self { MatchResult::None => f.debug_struct("MatchResult2::None").finish(), MatchResult::Pending => f.debug_struct("MatchResult2::Pending").finish(), - MatchResult::Match { view_id, action } => f - .debug_struct("MatchResult::Match") - .field("view_id", view_id) - .field("action", &action.name()) + MatchResult::Matches(matches) => f + .debug_list() + .entries( + matches + .iter() + .map(|(view_id, action)| format!("{view_id}, {}", action.name())), + ) .finish(), } } @@ -99,13 +97,14 @@ impl PartialEq for MatchResult { match (self, other) { (MatchResult::None, MatchResult::None) => true, (MatchResult::Pending, MatchResult::Pending) => true, - ( - MatchResult::Match { view_id, action }, - MatchResult::Match { - view_id: other_view_id, - action: other_action, - }, - ) => view_id == other_view_id && action.eq(other_action.as_ref()), + (MatchResult::Matches(matches), MatchResult::Matches(other_matches)) => { + matches.len() == other_matches.len() + && matches.iter().zip(other_matches.iter()).all( + |((view_id, action), (other_view_id, other_action))| { + view_id == other_view_id && action.eq(other_action.as_ref()) + }, + ) + } _ => false, } } @@ -116,23 +115,24 @@ impl Eq for MatchResult {} impl Matcher { pub fn new(keymap: Keymap) -> Self { Self { - pending: HashMap::new(), + pending_views: HashMap::new(), + pending_keystrokes: Vec::new(), keymap, } } pub fn set_keymap(&mut self, keymap: Keymap) { - self.pending.clear(); + self.clear_pending(); self.keymap = keymap; } pub fn add_bindings>(&mut self, bindings: T) { - self.pending.clear(); + self.clear_pending(); self.keymap.add_bindings(bindings); } pub fn clear_bindings(&mut self) { - self.pending.clear(); + self.clear_pending(); self.keymap.clear(); } @@ -141,11 +141,12 @@ impl Matcher { } pub fn clear_pending(&mut self) { - self.pending.clear(); + self.pending_keystrokes.clear(); + self.pending_views.clear(); } pub fn has_pending_keystrokes(&self) -> bool { - !self.pending.is_empty() + !self.pending_keystrokes.is_empty() } pub fn push_keystroke( @@ -156,54 +157,49 @@ impl Matcher { let mut any_pending = false; let mut matched_bindings = Vec::new(); - let first_keystroke = self.pending.is_empty(); - dbg!(&dispatch_path); + let first_keystroke = self.pending_keystrokes.is_empty(); + self.pending_keystrokes.push(keystroke); + for (view_id, context) in dispatch_path { - if !first_keystroke && !self.pending.contains_key(&view_id) { + // Don't require pending view entry if there are no pending keystrokes + if !first_keystroke && !self.pending_views.contains_key(&view_id) { continue; } - let pending = self.pending.entry(view_id).or_default(); - - if let Some(pending_context) = pending.context.as_ref() { - if pending_context != &context { - pending.keystrokes.clear(); + // If there is a previous view context, invalidate that view if it + // has changed + if let Some(previous_view_context) = self.pending_views.remove(&view_id) { + if previous_view_context != context { + continue; } } - pending.keystrokes.push(keystroke.clone()); - - let mut retain_pending = false; for binding in self.keymap.bindings.iter().rev() { - if binding.keystrokes.starts_with(&pending.keystrokes) + if binding.keystrokes.starts_with(&self.pending_keystrokes) && binding .context_predicate .as_ref() .map(|c| c.eval(&context)) .unwrap_or(true) { - if binding.keystrokes.len() == pending.keystrokes.len() { - self.pending.remove(&view_id); + if binding.keystrokes.len() == self.pending_keystrokes.len() { matched_bindings.push((view_id, binding.action.boxed_clone())); } else { - retain_pending = true; - pending.context = Some(context.clone()); + self.pending_views.insert(view_id, context.clone()); + any_pending = true; } } } - - if retain_pending { - any_pending = true; - } else { - self.pending.remove(&view_id); - } } if !matched_bindings.is_empty() { - MatchResult::Match(matched_bindings) + self.pending_views.clear(); + self.pending_keystrokes.clear(); + MatchResult::Matches(matched_bindings) } else if any_pending { MatchResult::Pending } else { + self.pending_keystrokes.clear(); MatchResult::None } } @@ -520,21 +516,15 @@ mod tests { matcher.push_keystroke(Keystroke::parse("a")?, dispatch_path.clone()) ); assert_eq!( - MatchResult::Match { - view_id: 1, - action: Box::new(AB) - }, + MatchResult::Matches(vec![(1, Box::new(AB))]), matcher.push_keystroke(Keystroke::parse("b")?, dispatch_path.clone()) ); - assert!(matcher.pending.is_empty()); + assert!(!matcher.has_pending_keystrokes()); assert_eq!( - MatchResult::Match { - view_id: 2, - action: Box::new(B) - }, + MatchResult::Matches(vec![(2, Box::new(B))]), matcher.push_keystroke(Keystroke::parse("b")?, dispatch_path.clone()) ); - assert!(matcher.pending.is_empty()); + assert!(!matcher.has_pending_keystrokes()); assert_eq!( MatchResult::Pending, matcher.push_keystroke(Keystroke::parse("a")?, dispatch_path.clone()) @@ -543,7 +533,7 @@ mod tests { MatchResult::None, matcher.push_keystroke(Keystroke::parse("c")?, dispatch_path.clone()) ); - assert!(matcher.pending.is_empty()); + assert!(!matcher.has_pending_keystrokes()); Ok(()) } @@ -717,15 +707,15 @@ mod tests { } impl Matcher { - fn test_keystroke( - &mut self, + fn test_keystroke<'a>( + &'a mut self, keystroke: &str, dispatch_path: Vec<(usize, Context)>, ) -> Option> { - if let MatchResult::Match { action, .. } = + if let MatchResult::Matches(matches) = self.push_keystroke(Keystroke::parse(keystroke).unwrap(), dispatch_path) { - Some(action.boxed_clone()) + Some(matches[0].1.boxed_clone()) } else { None } From 672b4456761e9f42ded635b6c6aa325968b01b6b Mon Sep 17 00:00:00 2001 From: K Simmons Date: Thu, 27 Oct 2022 12:36:53 -0700 Subject: [PATCH 3/5] minor tweak to keymap code --- crates/gpui/src/keymap.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/gpui/src/keymap.rs b/crates/gpui/src/keymap.rs index 89831f2c7ff90d43590ca4b9e8b14f7784d1acee..1e98c526341981333c59bd438c0dc1541335ade8 100644 --- a/crates/gpui/src/keymap.rs +++ b/crates/gpui/src/keymap.rs @@ -174,6 +174,7 @@ impl Matcher { } } + // Find the bindings which map the pending keystrokes and current context for binding in self.keymap.bindings.iter().rev() { if binding.keystrokes.starts_with(&self.pending_keystrokes) && binding @@ -182,9 +183,11 @@ impl Matcher { .map(|c| c.eval(&context)) .unwrap_or(true) { + // If the binding is completed, push it onto the matches list if binding.keystrokes.len() == self.pending_keystrokes.len() { matched_bindings.push((view_id, binding.action.boxed_clone())); } else { + // Otherwise, the binding is still pending self.pending_views.insert(view_id, context.clone()); any_pending = true; } @@ -193,13 +196,11 @@ impl Matcher { } if !matched_bindings.is_empty() { - self.pending_views.clear(); - self.pending_keystrokes.clear(); MatchResult::Matches(matched_bindings) } else if any_pending { MatchResult::Pending } else { - self.pending_keystrokes.clear(); + self.clear_pending(); MatchResult::None } } From bf968707dfb1d6d7fc2c7ef937926a3a682bb374 Mon Sep 17 00:00:00 2001 From: K Simmons Date: Thu, 27 Oct 2022 13:28:49 -0700 Subject: [PATCH 4/5] upgrade existing test to match new expected behavior --- crates/gpui/src/keymap.rs | 123 ++++++++++++++++++++++---------------- 1 file changed, 70 insertions(+), 53 deletions(-) diff --git a/crates/gpui/src/keymap.rs b/crates/gpui/src/keymap.rs index 1e98c526341981333c59bd438c0dc1541335ade8..fc97f6962498bc4a502f634a503a8a227b54e9dc 100644 --- a/crates/gpui/src/keymap.rs +++ b/crates/gpui/src/keymap.rs @@ -78,8 +78,8 @@ pub enum MatchResult { impl Debug for MatchResult { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - MatchResult::None => f.debug_struct("MatchResult2::None").finish(), - MatchResult::Pending => f.debug_struct("MatchResult2::Pending").finish(), + MatchResult::None => f.debug_struct("MatchResult::None").finish(), + MatchResult::Pending => f.debug_struct("MatchResult::Pending").finish(), MatchResult::Matches(matches) => f .debug_list() .entries( @@ -195,12 +195,15 @@ impl Matcher { } } + if !any_pending { + self.clear_pending(); + } + if !matched_bindings.is_empty() { MatchResult::Matches(matched_bindings) } else if any_pending { MatchResult::Pending } else { - self.clear_pending(); MatchResult::None } } @@ -494,7 +497,7 @@ mod tests { #[test] fn test_push_keystroke() -> Result<()> { - actions!(test, [B, AB, C]); + actions!(test, [B, AB, C, D, DA]); let mut ctx1 = Context::default(); ctx1.set.insert("1".into()); @@ -508,31 +511,56 @@ mod tests { Binding::new("a b", AB, Some("1")), Binding::new("b", B, Some("2")), Binding::new("c", C, Some("2")), + Binding::new("d", D, Some("1")), + Binding::new("d", D, Some("2")), + Binding::new("d a", DA, Some("2")), ]); let mut matcher = Matcher::new(keymap); + // Binding with pending prefix always takes precedence assert_eq!( + matcher.push_keystroke(Keystroke::parse("a")?, dispatch_path.clone()), MatchResult::Pending, - matcher.push_keystroke(Keystroke::parse("a")?, dispatch_path.clone()) ); + // B alone doesn't match because a was pending, so AB is returned instead assert_eq!( + matcher.push_keystroke(Keystroke::parse("b")?, dispatch_path.clone()), MatchResult::Matches(vec![(1, Box::new(AB))]), - matcher.push_keystroke(Keystroke::parse("b")?, dispatch_path.clone()) ); assert!(!matcher.has_pending_keystrokes()); + + // Without an a prefix, B is dispatched like expected assert_eq!( + matcher.push_keystroke(Keystroke::parse("b")?, dispatch_path.clone()), MatchResult::Matches(vec![(2, Box::new(B))]), - matcher.push_keystroke(Keystroke::parse("b")?, dispatch_path.clone()) ); assert!(!matcher.has_pending_keystrokes()); + + // If a is prefixed, C will not be dispatched because there + // was a pending binding for it assert_eq!( + matcher.push_keystroke(Keystroke::parse("a")?, dispatch_path.clone()), MatchResult::Pending, - matcher.push_keystroke(Keystroke::parse("a")?, dispatch_path.clone()) ); assert_eq!( + matcher.push_keystroke(Keystroke::parse("c")?, dispatch_path.clone()), MatchResult::None, - matcher.push_keystroke(Keystroke::parse("c")?, dispatch_path.clone()) + ); + assert!(!matcher.has_pending_keystrokes()); + + // If a single keystroke matches multiple bindings in the tree + // all of them are returned so that we can fallback if the action + // handler decides to propagate the action + assert_eq!( + matcher.push_keystroke(Keystroke::parse("d")?, dispatch_path.clone()), + MatchResult::Matches(vec![(2, Box::new(D)), (1, Box::new(D))]), + ); + // If none of the d action handlers consume the binding, a pending + // binding may then be used + assert_eq!( + matcher.push_keystroke(Keystroke::parse("a")?, dispatch_path.clone()), + MatchResult::Matches(vec![(2, Box::new(DA))]), ); assert!(!matcher.has_pending_keystrokes()); @@ -655,71 +683,60 @@ mod tests { // Basic match assert_eq!( - downcast(&matcher.test_keystroke("a", vec![(1, ctx_a.clone())])), - Some(&A("x".to_string())) + matcher.push_keystroke(Keystroke::parse("a")?, vec![(1, ctx_a.clone())]), + MatchResult::Matches(vec![(1, Box::new(A("x".to_string())))]) ); + matcher.clear_pending(); // Multi-keystroke match - assert!(matcher - .test_keystroke("a", vec![(1, ctx_b.clone())]) - .is_none()); assert_eq!( - downcast(&matcher.test_keystroke("b", vec![(1, ctx_b.clone())])), - Some(&Ab) + matcher.push_keystroke(Keystroke::parse("a")?, vec![(1, ctx_b.clone())]), + MatchResult::Pending + ); + assert_eq!( + matcher.push_keystroke(Keystroke::parse("b")?, vec![(1, ctx_b.clone())]), + MatchResult::Matches(vec![(1, Box::new(Ab))]) ); + matcher.clear_pending(); // Failed matches don't interfere with matching subsequent keys - assert!(matcher - .test_keystroke("x", vec![(1, ctx_a.clone())]) - .is_none()); assert_eq!( - downcast(&matcher.test_keystroke("a", vec![(1, ctx_a.clone())])), - Some(&A("x".to_string())) + matcher.push_keystroke(Keystroke::parse("x")?, vec![(1, ctx_a.clone())]), + MatchResult::None ); + assert_eq!( + matcher.push_keystroke(Keystroke::parse("a")?, vec![(1, ctx_a.clone())]), + MatchResult::Matches(vec![(1, Box::new(A("x".to_string())))]) + ); + matcher.clear_pending(); // Pending keystrokes are cleared when the context changes - assert!(&matcher - .test_keystroke("a", vec![(1, ctx_b.clone())]) - .is_none()); assert_eq!( - downcast(&matcher.test_keystroke("b", vec![(1, ctx_a.clone())])), - Some(&B) + matcher.push_keystroke(Keystroke::parse("a")?, vec![(1, ctx_b.clone())]), + MatchResult::Pending + ); + assert_eq!( + matcher.push_keystroke(Keystroke::parse("b")?, vec![(1, ctx_a.clone())]), + MatchResult::None ); + matcher.clear_pending(); let mut ctx_c = Context::default(); ctx_c.set.insert("c".into()); // Pending keystrokes are maintained per-view - assert!(matcher - .test_keystroke("a", vec![(1, ctx_b.clone()), (2, ctx_c.clone())]) - .is_none()); assert_eq!( - downcast(&matcher.test_keystroke("b", vec![(1, ctx_b.clone())])), - Some(&Ab) + matcher.push_keystroke( + Keystroke::parse("a")?, + vec![(1, ctx_b.clone()), (2, ctx_c.clone())] + ), + MatchResult::Pending + ); + assert_eq!( + matcher.push_keystroke(Keystroke::parse("b")?, vec![(1, ctx_b.clone())]), + MatchResult::Matches(vec![(1, Box::new(Ab))]) ); Ok(()) } - - fn downcast(action: &Option>) -> Option<&A> { - action - .as_ref() - .and_then(|action| action.as_any().downcast_ref()) - } - - impl Matcher { - fn test_keystroke<'a>( - &'a mut self, - keystroke: &str, - dispatch_path: Vec<(usize, Context)>, - ) -> Option> { - if let MatchResult::Matches(matches) = - self.push_keystroke(Keystroke::parse(keystroke).unwrap(), dispatch_path) - { - Some(matches[0].1.boxed_clone()) - } else { - None - } - } - } } From c5650f9334e33a312f8a84f8c04120ee16e11493 Mon Sep 17 00:00:00 2001 From: K Simmons Date: Thu, 27 Oct 2022 13:29:57 -0700 Subject: [PATCH 5/5] remove unnecessary comment --- crates/gpui/src/app.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/crates/gpui/src/app.rs b/crates/gpui/src/app.rs index ea3fee4fdf95b2d7ace0829c3127d4320015b147..c62305f572302211c7c7b3ec6c6da477aa8d0591 100644 --- a/crates/gpui/src/app.rs +++ b/crates/gpui/src/app.rs @@ -1524,13 +1524,6 @@ impl MutableAppContext { pub fn dispatch_keystroke(&mut self, window_id: usize, keystroke: &Keystroke) -> bool { if let Some(focused_view_id) = self.focused_view_id(window_id) { - // For each view in self - // - Get it's keymap context - // - Try to execute keybinding - - // For us, get out dispatch path - // - ONLY execute for that terminal path - let dispatch_path = self .ancestors(window_id, focused_view_id) .map(|view_id| {