Yehowshua/ZIm
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Make macOS application menu aware of which key bindings are disabled (#2735)

Browse source 
Follow-up of https://github.com/zed-industries/zed/pull/2678
Deals with https://github.com/zed-industries/community/issues/772

Refreshes macOs menu panel on keymap file change and properly ignore
disabled actions.

Release Notes:

- Fixes a bug when disabled actions from macOs menu were still working
This commit is contained in:
Max Brunsfeld 2023-07-17 11:20:41 -07:00 • committed by GitHub
commit fef73ae921
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
9 changed files with 395 additions and 73 deletions
Download patch file Download diff file Expand all files Collapse all files

12
crates/gpui/src/app.rs
View file

@ -1073,7 +1073,7 @@ impl AppContext {
pub fn is_action_available(&self, action: &dyn Action) -> bool {
let mut available_in_window = false;
let action_type = action.as_any().type_id();
let action_id = action.id();
if let Some(window_id) = self.platform.main_window_id() {
available_in_window = self
.read_window(window_id, |cx| {
@ -1083,7 +1083,7 @@ impl AppContext {
cx.views_metadata.get(&(window_id, view_id))
{
if let Some(actions) = cx.actions.get(&view_metadata.type_id) {
if actions.contains_key(&action_type) {
if actions.contains_key(&action_id) {
return true;
}
}
@ -1094,7 +1094,7 @@ impl AppContext {
})
.unwrap_or(false);
}
available_in_window || self.global_actions.contains_key(&action_type)
available_in_window || self.global_actions.contains_key(&action_id)
}
fn actions_mut(
@ -3399,7 +3399,7 @@ impl<'a, 'b, 'c, V: View> LayoutContext<'a, 'b, 'c, V> {
for (i, view_id) in self.ancestors(view_id).enumerate() {
if let Some(view_metadata) = self.views_metadata.get(&(window_id, view_id)) {
if let Some(actions) = self.actions.get(&view_metadata.type_id) {
if actions.contains_key(&action.as_any().type_id()) {
if actions.contains_key(&action.id()) {
handler_depth = Some(i);
}
}
@ -3407,12 +3407,12 @@ impl<'a, 'b, 'c, V: View> LayoutContext<'a, 'b, 'c, V> {
}
}
if self.global_actions.contains_key(&action.as_any().type_id()) {
if self.global_actions.contains_key(&action.id()) {
handler_depth = Some(contexts.len())
}
self.keystroke_matcher
.bindings_for_action_type(action.as_any().type_id())
.bindings_for_action(action.id())
.find_map(|b| {
let highest_handler = handler_depth?;
if action.eq(b.action())

28
crates/gpui/src/app/window.rs
View file

@ -14,8 +14,8 @@ use crate::{
text_layout::TextLayoutCache,
util::post_inc,
Action, AnyView, AnyViewHandle, AppContext, BorrowAppContext, BorrowWindowContext, Effect,
Element, Entity, Handle, LayoutContext, MouseRegion, MouseRegionId, NoAction, SceneBuilder,
Subscription, View, ViewContext, ViewHandle, WindowInvalidation,
Element, Entity, Handle, LayoutContext, MouseRegion, MouseRegionId, SceneBuilder, Subscription,
View, ViewContext, ViewHandle, WindowInvalidation,
};
use anyhow::{anyhow, bail, Result};
use collections::{HashMap, HashSet};
@ -363,17 +363,13 @@ impl<'a> WindowContext<'a> {
) -> Vec<(&'static str, Box<dyn Action>, SmallVec<[Binding; 1]>)> {
let window_id = self.window_id;
let mut contexts = Vec::new();
let mut handler_depths_by_action_type = HashMap::<TypeId, usize>::default();
let mut handler_depths_by_action_id = HashMap::<TypeId, usize>::default();
for (depth, view_id) in self.ancestors(view_id).enumerate() {
if let Some(view_metadata) = self.views_metadata.get(&(window_id, view_id)) {
contexts.push(view_metadata.keymap_context.clone());
if let Some(actions) = self.actions.get(&view_metadata.type_id) {
handler_depths_by_action_type.extend(
actions
.keys()
.copied()
.map(|action_type| (action_type, depth)),
);
handler_depths_by_action_id
.extend(actions.keys().copied().map(|action_id| (action_id, depth)));
}
} else {
log::error!(
@ -383,21 +379,21 @@ impl<'a> WindowContext<'a> {
}
}
handler_depths_by_action_type.extend(
handler_depths_by_action_id.extend(
self.global_actions
.keys()
.copied()
.map(|action_type| (action_type, contexts.len())),
.map(|action_id| (action_id, contexts.len())),
);
self.action_deserializers
.iter()
.filter_map(move |(name, (type_id, deserialize))| {
if let Some(action_depth) = handler_depths_by_action_type.get(type_id).copied() {
.filter_map(move |(name, (action_id, deserialize))| {
if let Some(action_depth) = handler_depths_by_action_id.get(action_id).copied() {
let action = deserialize(serde_json::Value::Object(Default::default())).ok()?;
let bindings = self
.keystroke_matcher
.bindings_for_action_type(*type_id)
.bindings_for_action(*action_id)
.filter(|b| {
action.eq(b.action())
&& (0..=action_depth)
@ -434,11 +430,7 @@ impl<'a> WindowContext<'a> {
MatchResult::None => false,
MatchResult::Pending => true,
MatchResult::Matches(matches) => {
let no_action_id = (NoAction {}).id();
for (view_id, action) in matches {
if action.id() == no_action_id {
return false;
}
if self.dispatch_action(Some(*view_id), action.as_ref()) {
self.keystroke_matcher.clear_pending();
handled_by = Some(action.boxed_clone());

11
crates/gpui/src/keymap_matcher.rs
View file

@ -8,7 +8,7 @@ use std::{any::TypeId, fmt::Debug};
use collections::HashMap;
use smallvec::SmallVec;
use crate::Action;
use crate::{Action, NoAction};
pub use binding::{Binding, BindingMatchResult};
pub use keymap::Keymap;
@ -47,8 +47,8 @@ impl KeymapMatcher {
self.keymap.clear();
}
pub fn bindings_for_action_type(&self, action_type: TypeId) -> impl Iterator<Item = &Binding> {
self.keymap.bindings_for_action_type(action_type)
pub fn bindings_for_action(&self, action_id: TypeId) -> impl Iterator<Item = &Binding> {
self.keymap.bindings_for_action(action_id)
}
pub fn clear_pending(&mut self) {
@ -81,6 +81,7 @@ impl KeymapMatcher {
// The key is the reverse position of the binding in the bindings list so that later bindings
// match before earlier ones in the user's config
let mut matched_bindings: Vec<(usize, Box<dyn Action>)> = Default::default();
let no_action_id = (NoAction {}).id();
let first_keystroke = self.pending_keystrokes.is_empty();
self.pending_keystrokes.push(keystroke.clone());
@ -108,7 +109,9 @@ impl KeymapMatcher {
match binding.match_keys_and_context(&self.pending_keystrokes, &self.contexts[i..])
{
BindingMatchResult::Complete(action) => {
matched_bindings.push((*view_id, action));
if action.id() != no_action_id {
matched_bindings.push((*view_id, action));
}
}
BindingMatchResult::Partial => {
self.pending_views

4
crates/gpui/src/keymap_matcher/binding.rs
View file

@ -7,8 +7,8 @@ use super::{KeymapContext, KeymapContextPredicate, Keystroke};
pub struct Binding {
action: Box<dyn Action>,
keystrokes: SmallVec<[Keystroke; 2]>,
context_predicate: Option<KeymapContextPredicate>,
pub(super) keystrokes: SmallVec<[Keystroke; 2]>,
pub(super) context_predicate: Option<KeymapContextPredicate>,
}
impl std::fmt::Debug for Binding {

389
crates/gpui/src/keymap_matcher/keymap.rs
View file

@ -1,61 +1,388 @@
use collections::HashSet;
use smallvec::SmallVec;
use std::{
any::{Any, TypeId},
collections::HashMap,
};
use std::{any::TypeId, collections::HashMap};
use super::Binding;
use crate::{Action, NoAction};
use super::{Binding, KeymapContextPredicate, Keystroke};
#[derive(Default)]
pub struct Keymap {
bindings: Vec<Binding>,
binding_indices_by_action_type: HashMap<TypeId, SmallVec<[usize; 3]>>,
binding_indices_by_action_id: HashMap<TypeId, SmallVec<[usize; 3]>>,
disabled_keystrokes: HashMap<SmallVec<[Keystroke; 2]>, HashSet<Option<KeymapContextPredicate>>>,
}
impl Keymap {
pub fn new(bindings: Vec<Binding>) -> Self {
let mut binding_indices_by_action_type = HashMap::new();
for (ix, binding) in bindings.iter().enumerate() {
binding_indices_by_action_type
.entry(binding.action().type_id())
.or_insert_with(SmallVec::new)
.push(ix);
}
Self {
binding_indices_by_action_type,
bindings,
}
#[cfg(test)]
pub(super) fn new(bindings: Vec<Binding>) -> Self {
let mut this = Self::default();
this.add_bindings(bindings);
this
}
pub(crate) fn bindings_for_action_type(
pub(crate) fn bindings_for_action(
&self,
action_type: TypeId,
action_id: TypeId,
) -> impl Iterator<Item = &'_ Binding> {
self.binding_indices_by_action_type
.get(&action_type)
self.binding_indices_by_action_id
.get(&action_id)
.map(SmallVec::as_slice)
.unwrap_or(&[])
.iter()
.map(|ix| &self.bindings[*ix])
.filter(|binding| !self.binding_disabled(binding))
}
pub(crate) fn add_bindings<T: IntoIterator<Item = Binding>>(&mut self, bindings: T) {
let no_action_id = (NoAction {}).id();
let mut new_bindings = Vec::new();
let mut has_new_disabled_keystrokes = false;
for binding in bindings {
self.binding_indices_by_action_type
.entry(binding.action().as_any().type_id())
.or_default()
.push(self.bindings.len());
self.bindings.push(binding);
if binding.action().id() == no_action_id {
has_new_disabled_keystrokes |= self
.disabled_keystrokes
.entry(binding.keystrokes)
.or_default()
.insert(binding.context_predicate);
} else {
new_bindings.push(binding);
}
}
if has_new_disabled_keystrokes {
self.binding_indices_by_action_id.retain(|_, indices| {
indices.retain(|ix| {
let binding = &self.bindings[*ix];
match self.disabled_keystrokes.get(&binding.keystrokes) {
Some(disabled_predicates) => {
!disabled_predicates.contains(&binding.context_predicate)
}
None => true,
}
});
!indices.is_empty()
});
}
for new_binding in new_bindings {
if !self.binding_disabled(&new_binding) {
self.binding_indices_by_action_id
.entry(new_binding.action().id())
.or_default()
.push(self.bindings.len());
self.bindings.push(new_binding);
}
}
}
pub(crate) fn clear(&mut self) {
self.bindings.clear();
self.binding_indices_by_action_type.clear();
self.binding_indices_by_action_id.clear();
self.disabled_keystrokes.clear();
}
pub fn bindings(&self) -> &Vec<Binding> {
&self.bindings
pub fn bindings(&self) -> Vec<&Binding> {
self.bindings
.iter()
.filter(|binding| !self.binding_disabled(binding))
.collect()
}
fn binding_disabled(&self, binding: &Binding) -> bool {
match self.disabled_keystrokes.get(&binding.keystrokes) {
Some(disabled_predicates) => disabled_predicates.contains(&binding.context_predicate),
None => false,
}
}
}
#[cfg(test)]
mod tests {
use crate::actions;
use super::*;
actions!(
keymap_test,
[Present1, Present2, Present3, Duplicate, Missing]
);
#[test]
fn regular_keymap() {
let present_1 = Binding::new("ctrl-q", Present1 {}, None);
let present_2 = Binding::new("ctrl-w", Present2 {}, Some("pane"));
let present_3 = Binding::new("ctrl-e", Present3 {}, Some("editor"));
let keystroke_duplicate_to_1 = Binding::new("ctrl-q", Duplicate {}, None);
let full_duplicate_to_2 = Binding::new("ctrl-w", Present2 {}, Some("pane"));
let missing = Binding::new("ctrl-r", Missing {}, None);
let all_bindings = [
&present_1,
&present_2,
&present_3,
&keystroke_duplicate_to_1,
&full_duplicate_to_2,
&missing,
];
let mut keymap = Keymap::default();
assert_absent(&keymap, &all_bindings);
assert!(keymap.bindings().is_empty());
keymap.add_bindings([present_1.clone(), present_2.clone(), present_3.clone()]);
assert_absent(&keymap, &[&keystroke_duplicate_to_1, &missing]);
assert_present(
&keymap,
&[(&present_1, "q"), (&present_2, "w"), (&present_3, "e")],
);
keymap.add_bindings([
keystroke_duplicate_to_1.clone(),
full_duplicate_to_2.clone(),
]);
assert_absent(&keymap, &[&missing]);
assert!(
!keymap.binding_disabled(&keystroke_duplicate_to_1),
"Duplicate binding 1 was added and should not be disabled"
);
assert!(
!keymap.binding_disabled(&full_duplicate_to_2),
"Duplicate binding 2 was added and should not be disabled"
);
assert_eq!(
keymap
.bindings_for_action(keystroke_duplicate_to_1.action().id())
.map(|binding| &binding.keystrokes)
.flatten()
.collect::<Vec<_>>(),
vec![&Keystroke {
ctrl: true,
alt: false,
shift: false,
cmd: false,
function: false,
key: "q".to_string()
}],
"{keystroke_duplicate_to_1:?} should have the expected keystroke in the keymap"
);
assert_eq!(
keymap
.bindings_for_action(full_duplicate_to_2.action().id())
.map(|binding| &binding.keystrokes)
.flatten()
.collect::<Vec<_>>(),
vec![
&Keystroke {
ctrl: true,
alt: false,
shift: false,
cmd: false,
function: false,
key: "w".to_string()
},
&Keystroke {
ctrl: true,
alt: false,
shift: false,
cmd: false,
function: false,
key: "w".to_string()
}
],
"{full_duplicate_to_2:?} should have a duplicated keystroke in the keymap"
);
let updated_bindings = keymap.bindings();
let expected_updated_bindings = vec![
&present_1,
&present_2,
&present_3,
&keystroke_duplicate_to_1,
&full_duplicate_to_2,
];
assert_eq!(
updated_bindings.len(),
expected_updated_bindings.len(),
"Unexpected updated keymap bindings {updated_bindings:?}"
);
for (i, expected) in expected_updated_bindings.iter().enumerate() {
let keymap_binding = &updated_bindings[i];
assert_eq!(
keymap_binding.context_predicate, expected.context_predicate,
"Unexpected context predicate for keymap {i} element: {keymap_binding:?}"
);
assert_eq!(
keymap_binding.keystrokes, expected.keystrokes,
"Unexpected keystrokes for keymap {i} element: {keymap_binding:?}"
);
}
keymap.clear();
assert_absent(&keymap, &all_bindings);
assert!(keymap.bindings().is_empty());
}
#[test]
fn keymap_with_ignored() {
let present_1 = Binding::new("ctrl-q", Present1 {}, None);
let present_2 = Binding::new("ctrl-w", Present2 {}, Some("pane"));
let present_3 = Binding::new("ctrl-e", Present3 {}, Some("editor"));
let keystroke_duplicate_to_1 = Binding::new("ctrl-q", Duplicate {}, None);
let full_duplicate_to_2 = Binding::new("ctrl-w", Present2 {}, Some("pane"));
let ignored_1 = Binding::new("ctrl-q", NoAction {}, None);
let ignored_2 = Binding::new("ctrl-w", NoAction {}, Some("pane"));
let ignored_3_with_other_context =
Binding::new("ctrl-e", NoAction {}, Some("other_context"));
let mut keymap = Keymap::default();
keymap.add_bindings([
ignored_1.clone(),
ignored_2.clone(),
ignored_3_with_other_context.clone(),
]);
assert_absent(&keymap, &[&present_3]);
assert_disabled(
&keymap,
&[
&present_1,
&present_2,
&ignored_1,
&ignored_2,
&ignored_3_with_other_context,
],
);
assert!(keymap.bindings().is_empty());
keymap.clear();
keymap.add_bindings([
present_1.clone(),
present_2.clone(),
present_3.clone(),
ignored_1.clone(),
ignored_2.clone(),
ignored_3_with_other_context.clone(),
]);
assert_present(&keymap, &[(&present_3, "e")]);
assert_disabled(
&keymap,
&[
&present_1,
&present_2,
&ignored_1,
&ignored_2,
&ignored_3_with_other_context,
],
);
keymap.clear();
keymap.add_bindings([
present_1.clone(),
present_2.clone(),
present_3.clone(),
ignored_1.clone(),
]);
assert_present(&keymap, &[(&present_2, "w"), (&present_3, "e")]);
assert_disabled(&keymap, &[&present_1, &ignored_1]);
assert_absent(&keymap, &[&ignored_2, &ignored_3_with_other_context]);
keymap.clear();
keymap.add_bindings([
present_1.clone(),
present_2.clone(),
present_3.clone(),
keystroke_duplicate_to_1.clone(),
full_duplicate_to_2.clone(),
ignored_1.clone(),
ignored_2.clone(),
ignored_3_with_other_context.clone(),
]);
assert_present(&keymap, &[(&present_3, "e")]);
assert_disabled(
&keymap,
&[
&present_1,
&present_2,
&keystroke_duplicate_to_1,
&full_duplicate_to_2,
&ignored_1,
&ignored_2,
&ignored_3_with_other_context,
],
);
keymap.clear();
}
#[track_caller]
fn assert_present(keymap: &Keymap, expected_bindings: &[(&Binding, &str)]) {
let keymap_bindings = keymap.bindings();
assert_eq!(
expected_bindings.len(),
keymap_bindings.len(),
"Unexpected keymap bindings {keymap_bindings:?}"
);
for (i, (expected, expected_key)) in expected_bindings.iter().enumerate() {
assert!(
!keymap.binding_disabled(expected),
"{expected:?} should not be disabled as it was added into keymap for element {i}"
);
assert_eq!(
keymap
.bindings_for_action(expected.action().id())
.map(|binding| &binding.keystrokes)
.flatten()
.collect::<Vec<_>>(),
vec![&Keystroke {
ctrl: true,
alt: false,
shift: false,
cmd: false,
function: false,
key: expected_key.to_string()
}],
"{expected:?} should have the expected keystroke with key '{expected_key}' in the keymap for element {i}"
);
let keymap_binding = &keymap_bindings[i];
assert_eq!(
keymap_binding.context_predicate, expected.context_predicate,
"Unexpected context predicate for keymap {i} element: {keymap_binding:?}"
);
assert_eq!(
keymap_binding.keystrokes, expected.keystrokes,
"Unexpected keystrokes for keymap {i} element: {keymap_binding:?}"
);
}
}
#[track_caller]
fn assert_absent(keymap: &Keymap, bindings: &[&Binding]) {
for binding in bindings.iter() {
assert!(
!keymap.binding_disabled(binding),
"{binding:?} should not be disabled in the keymap where was not added"
);
assert_eq!(
keymap.bindings_for_action(binding.action().id()).count(),
0,
"{binding:?} should have no actions in the keymap where was not added"
);
}
}
#[track_caller]
fn assert_disabled(keymap: &Keymap, bindings: &[&Binding]) {
for binding in bindings.iter() {
assert!(
keymap.binding_disabled(binding),
"{binding:?} should be disabled in the keymap"
);
assert_eq!(
keymap.bindings_for_action(binding.action().id()).count(),
0,
"{binding:?} should have no actions in the keymap where it was disabled"
);
}
}
}

2
crates/gpui/src/keymap_matcher/keymap_context.rs
View file

@ -44,7 +44,7 @@ impl KeymapContext {
}
}
#[derive(Clone, Debug, Eq, PartialEq)]
#[derive(Clone, Debug, Eq, PartialEq, Hash)]
pub enum KeymapContextPredicate {
Identifier(String),
Equal(String, String),

2
crates/gpui/src/keymap_matcher/keystroke.rs
View file

@ -3,7 +3,7 @@ use std::fmt::Write;
use anyhow::anyhow;
use serde::Deserialize;
#[derive(Clone, Debug, Eq, PartialEq, Default, Deserialize)]
#[derive(Clone, Debug, Eq, PartialEq, Default, Deserialize, Hash)]
pub struct Keystroke {
pub ctrl: bool,
pub alt: bool,

2
crates/gpui/src/platform/mac/platform.rs
View file

@ -231,7 +231,7 @@ impl MacForegroundPlatform {
} => {
// TODO
let keystrokes = keystroke_matcher
.bindings_for_action_type(action.as_any().type_id())
.bindings_for_action(action.id())
.find(|binding| binding.action().eq(action.as_ref()))
.map(|binding| binding.keystrokes());
let selector = match os_action {

18
crates/zed/src/zed.rs
View file

@ -517,11 +517,7 @@ pub fn handle_keymap_file_changes(
let mut settings_subscription = None;
while let Some(user_keymap_content) = user_keymap_file_rx.next().await {
if let Ok(keymap_content) = KeymapFile::parse(&user_keymap_content) {
cx.update(|cx| {
cx.clear_bindings();
load_default_keymap(cx);
keymap_content.clone().add_to_cx(cx).log_err();
});
cx.update(|cx| reload_keymaps(cx, &keymap_content));
let mut old_base_keymap = cx.read(|cx| *settings::get::<BaseKeymap>(cx));
drop(settings_subscription);
@ -530,10 +526,7 @@ pub fn handle_keymap_file_changes(
let new_base_keymap = *settings::get::<BaseKeymap>(cx);
if new_base_keymap != old_base_keymap {
old_base_keymap = new_base_keymap.clone();
cx.clear_bindings();
load_default_keymap(cx);
keymap_content.clone().add_to_cx(cx).log_err();
reload_keymaps(cx, &keymap_content);
}
})
.detach();
@ -544,6 +537,13 @@ pub fn handle_keymap_file_changes(
.detach();
}
fn reload_keymaps(cx: &mut AppContext, keymap_content: &KeymapFile) {
cx.clear_bindings();
load_default_keymap(cx);
keymap_content.clone().add_to_cx(cx).log_err();
cx.set_menus(menus::menus());
}
fn open_local_settings_file(
workspace: &mut Workspace,
_: &OpenLocalSettings,