From 50069a215376eb369554eefc0875719949309b38 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Tue, 5 Nov 2024 16:18:41 -0800 Subject: [PATCH] Unbind app menu actions (#20268) Closes #7544 Release Notes: - Fixed an issue that prevented removing key bindings for actions used in the macOS application menu. --- crates/gpui/src/action.rs | 8 ++- crates/gpui/src/keymap.rs | 84 ++++++++++++++++++++++++++----- crates/gpui/src/keymap/context.rs | 47 +++++++++++++++++ 3 files changed, 126 insertions(+), 13 deletions(-) diff --git a/crates/gpui/src/action.rs b/crates/gpui/src/action.rs index 2b5b3b9756e04..b81ba6136f648 100644 --- a/crates/gpui/src/action.rs +++ b/crates/gpui/src/action.rs @@ -1,7 +1,7 @@ use crate::SharedString; use anyhow::{anyhow, Context, Result}; use collections::HashMap; -pub use no_action::NoAction; +pub use no_action::{is_no_action, NoAction}; use serde_json::json; use std::any::{Any, TypeId}; @@ -321,6 +321,12 @@ macro_rules! __impl_action { mod no_action { use crate as gpui; + use std::any::Any as _; actions!(zed, [NoAction]); + + /// Returns whether or not this action represents a removed key binding. + pub fn is_no_action(action: &dyn gpui::Action) -> bool { + action.as_any().type_id() == (NoAction {}).type_id() + } } diff --git a/crates/gpui/src/keymap.rs b/crates/gpui/src/keymap.rs index 426afaa9027c1..34600e91676f7 100644 --- a/crates/gpui/src/keymap.rs +++ b/crates/gpui/src/keymap.rs @@ -4,10 +4,10 @@ mod context; pub use binding::*; pub use context::*; -use crate::{Action, Keystroke, NoAction}; +use crate::{is_no_action, Action, Keystroke}; use collections::HashMap; use smallvec::SmallVec; -use std::any::{Any, TypeId}; +use std::any::TypeId; /// An opaque identifier of which version of the keymap is currently active. /// The keymap's version is changed whenever bindings are added or removed. @@ -19,6 +19,7 @@ pub struct KeymapVersion(usize); pub struct Keymap { bindings: Vec, binding_indices_by_action_id: HashMap>, + no_action_binding_indices: Vec, version: KeymapVersion, } @@ -39,10 +40,14 @@ impl Keymap { pub fn add_bindings>(&mut self, bindings: T) { for binding in bindings { let action_id = binding.action().as_any().type_id(); - self.binding_indices_by_action_id - .entry(action_id) - .or_default() - .push(self.bindings.len()); + if is_no_action(&*binding.action) { + self.no_action_binding_indices.push(self.bindings.len()); + } else { + self.binding_indices_by_action_id + .entry(action_id) + .or_default() + .push(self.bindings.len()); + } self.bindings.push(binding); } @@ -53,6 +58,7 @@ impl Keymap { pub fn clear(&mut self) { self.bindings.clear(); self.binding_indices_by_action_id.clear(); + self.no_action_binding_indices.clear(); self.version.0 += 1; } @@ -67,12 +73,39 @@ impl Keymap { action: &'a dyn Action, ) -> impl 'a + DoubleEndedIterator { let action_id = action.type_id(); - self.binding_indices_by_action_id + let binding_indices = self + .binding_indices_by_action_id .get(&action_id) .map_or(&[] as _, SmallVec::as_slice) - .iter() - .map(|ix| &self.bindings[*ix]) - .filter(move |binding| binding.action().partial_eq(action)) + .iter(); + + binding_indices.filter_map(|ix| { + let binding = &self.bindings[*ix]; + if !binding.action().partial_eq(action) { + return None; + } + + for null_ix in &self.no_action_binding_indices { + if null_ix > ix { + let null_binding = &self.bindings[*null_ix]; + if null_binding.keystrokes == binding.keystrokes { + let null_binding_matches = + match (&null_binding.context_predicate, &binding.context_predicate) { + (None, _) => true, + (Some(_), None) => false, + (Some(null_predicate), Some(predicate)) => { + null_predicate.is_superset(predicate) + } + }; + if null_binding_matches { + return None; + } + } + } + } + + Some(binding) + }) } /// all bindings for input returns all bindings that might match the input @@ -134,7 +167,7 @@ impl Keymap { let bindings = bindings .into_iter() .map_while(|(binding, _)| { - if binding.action.as_any().type_id() == (NoAction {}).type_id() { + if is_no_action(&*binding.action) { None } else { Some(binding) @@ -162,7 +195,7 @@ impl Keymap { mod tests { use super::*; use crate as gpui; - use gpui::actions; + use gpui::{actions, NoAction}; actions!( keymap_test, @@ -241,4 +274,31 @@ mod tests { .0 .is_empty()); } + + #[test] + fn test_bindings_for_action() { + let bindings = [ + KeyBinding::new("ctrl-a", ActionAlpha {}, Some("pane")), + KeyBinding::new("ctrl-b", ActionBeta {}, Some("editor && mode == full")), + KeyBinding::new("ctrl-c", ActionGamma {}, Some("workspace")), + KeyBinding::new("ctrl-a", NoAction {}, Some("pane && active")), + KeyBinding::new("ctrl-b", NoAction {}, Some("editor")), + ]; + + let mut keymap = Keymap::default(); + keymap.add_bindings(bindings.clone()); + + assert_bindings(&keymap, &ActionAlpha {}, &["ctrl-a"]); + assert_bindings(&keymap, &ActionBeta {}, &[]); + assert_bindings(&keymap, &ActionGamma {}, &["ctrl-c"]); + + #[track_caller] + fn assert_bindings(keymap: &Keymap, action: &dyn Action, expected: &[&str]) { + let actual = keymap + .bindings_for_action(action) + .map(|binding| binding.keystrokes[0].unparse()) + .collect::>(); + assert_eq!(actual, expected, "{:?}", action); + } + } } diff --git a/crates/gpui/src/keymap/context.rs b/crates/gpui/src/keymap/context.rs index fccc02886ba41..c7be02f8d38ef 100644 --- a/crates/gpui/src/keymap/context.rs +++ b/crates/gpui/src/keymap/context.rs @@ -269,6 +269,30 @@ impl KeyBindingContextPredicate { } } + /// Returns whether or not this predicate matches all possible contexts matched by + /// the other predicate. + pub fn is_superset(&self, other: &Self) -> bool { + if self == other { + return true; + } + + if let KeyBindingContextPredicate::Or(left, right) = self { + return left.is_superset(other) || right.is_superset(other); + } + + match other { + KeyBindingContextPredicate::Child(_, child) => self.is_superset(child), + KeyBindingContextPredicate::And(left, right) => { + self.is_superset(left) || self.is_superset(right) + } + KeyBindingContextPredicate::Identifier(_) => false, + KeyBindingContextPredicate::Equal(_, _) => false, + KeyBindingContextPredicate::NotEqual(_, _) => false, + KeyBindingContextPredicate::Not(_) => false, + KeyBindingContextPredicate::Or(_, _) => false, + } + } + fn parse_expr(mut source: &str, min_precedence: u32) -> anyhow::Result<(Self, &str)> { type Op = fn( KeyBindingContextPredicate, @@ -559,4 +583,27 @@ mod tests { ) ); } + + #[test] + fn test_is_superset() { + assert_is_superset("editor", "editor", true); + assert_is_superset("editor", "workspace", false); + + assert_is_superset("editor", "editor && vim_mode", true); + assert_is_superset("editor", "mode == full && editor", true); + assert_is_superset("editor && mode == full", "editor", false); + + assert_is_superset("editor", "something > editor", true); + assert_is_superset("editor", "editor > menu", false); + + assert_is_superset("foo || bar || baz", "bar", true); + assert_is_superset("foo || bar || baz", "quux", false); + + #[track_caller] + fn assert_is_superset(a: &str, b: &str, result: bool) { + let a = KeyBindingContextPredicate::parse(a).unwrap(); + let b = KeyBindingContextPredicate::parse(b).unwrap(); + assert_eq!(a.is_superset(&b), result, "({a:?}).is_superset({b:?})"); + } + } }