From 62e07bb162972c210bff1d58c3428982cba8af5b Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Fri, 15 Sep 2023 10:15:01 -0600 Subject: [PATCH] Fix multi-character shortcuts with modifiers (#2968) This moves the IME shortcut handling over to the keystroke matcher so that it can not clear pending matches when trying out the IME equivalent. Release Notes: - vim: add `g s` / `g S` to show symbols in the current buffer / workspace - Fix multi-key shortcuts with modifiers (preview-only) --- assets/keymaps/vim.json | 2 + crates/gpui/src/keymap_matcher.rs | 71 ++++++++++++++++----- crates/gpui/src/keymap_matcher/keymap.rs | 12 ++-- crates/gpui/src/keymap_matcher/keystroke.rs | 48 +++++++++++++- crates/gpui/src/platform/mac/event.rs | 1 + crates/gpui/src/platform/mac/window.rs | 40 ++---------- crates/terminal/src/mappings/keys.rs | 1 + 7 files changed, 119 insertions(+), 56 deletions(-) diff --git a/assets/keymaps/vim.json b/assets/keymaps/vim.json index 1a7b81ee8f5cb7e16e1290b1e7141d834ee6a887..4dffb07dfc5fd7d7457c3f2f8a4bcf5415413a6f 100644 --- a/assets/keymaps/vim.json +++ b/assets/keymaps/vim.json @@ -125,6 +125,8 @@ "g shift-t": "pane::ActivatePrevItem", "g d": "editor::GoToDefinition", "g shift-d": "editor::GoToTypeDefinition", + "g s": "outline::Toggle", + "g shift-s": "project_symbols::Toggle", "g .": "editor::ToggleCodeActions", // zed specific "g shift-a": "editor::FindAllReferences", // zed specific "g *": [ diff --git a/crates/gpui/src/keymap_matcher.rs b/crates/gpui/src/keymap_matcher.rs index 8cb7d30dfe4a78012023ee724becc09ced6d3212..89dd7bfc7d580103138e1edab9c9560697a632ef 100644 --- a/crates/gpui/src/keymap_matcher.rs +++ b/crates/gpui/src/keymap_matcher.rs @@ -75,7 +75,6 @@ impl KeymapMatcher { keystroke: Keystroke, mut dispatch_path: Vec<(usize, KeymapContext)>, ) -> MatchResult { - let mut any_pending = false; // Collect matched bindings into an ordered list using the position in the matching binding first, // and then the order the binding matched in the view tree second. // The key is the reverse position of the binding in the bindings list so that later bindings @@ -84,7 +83,8 @@ impl KeymapMatcher { let no_action_id = (NoAction {}).id(); let first_keystroke = self.pending_keystrokes.is_empty(); - self.pending_keystrokes.push(keystroke.clone()); + let mut pending_key = None; + let mut previous_keystrokes = self.pending_keystrokes.clone(); self.contexts.clear(); self.contexts @@ -106,24 +106,32 @@ impl KeymapMatcher { } for binding in self.keymap.bindings().iter().rev() { - match binding.match_keys_and_context(&self.pending_keystrokes, &self.contexts[i..]) - { - BindingMatchResult::Complete(action) => { - if action.id() != no_action_id { - matched_bindings.push((*view_id, action)); + for possibility in keystroke.match_possibilities() { + previous_keystrokes.push(possibility.clone()); + match binding.match_keys_and_context(&previous_keystrokes, &self.contexts[i..]) + { + BindingMatchResult::Complete(action) => { + if action.id() != no_action_id { + matched_bindings.push((*view_id, action)); + } } + BindingMatchResult::Partial => { + if pending_key == None || pending_key == Some(possibility.clone()) { + self.pending_views + .insert(*view_id, self.contexts[i].clone()); + pending_key = Some(possibility) + } + } + _ => {} } - BindingMatchResult::Partial => { - self.pending_views - .insert(*view_id, self.contexts[i].clone()); - any_pending = true; - } - _ => {} + previous_keystrokes.pop(); } } } - if !any_pending { + if pending_key.is_some() { + self.pending_keystrokes.push(pending_key.unwrap()); + } else { self.clear_pending(); } @@ -131,7 +139,7 @@ impl KeymapMatcher { // Collect the sorted matched bindings into the final vec for ease of use // Matched bindings are in order by precedence MatchResult::Matches(matched_bindings) - } else if any_pending { + } else if !self.pending_keystrokes.is_empty() { MatchResult::Pending } else { MatchResult::None @@ -340,6 +348,7 @@ mod tests { shift: false, cmd: false, function: false, + ime_key: None, } ); @@ -352,6 +361,7 @@ mod tests { shift: true, cmd: false, function: false, + ime_key: None, } ); @@ -364,6 +374,7 @@ mod tests { shift: true, cmd: true, function: false, + ime_key: None, } ); @@ -466,7 +477,7 @@ mod tests { #[derive(Clone, Deserialize, PartialEq, Eq, Debug)] pub struct A(pub String); impl_actions!(test, [A]); - actions!(test, [B, Ab]); + actions!(test, [B, Ab, Dollar, Quote, Ess, Backtick]); #[derive(Clone, Debug, Eq, PartialEq)] struct ActionArg { @@ -477,6 +488,10 @@ mod tests { Binding::new("a", A("x".to_string()), Some("a")), Binding::new("b", B, Some("a")), Binding::new("a b", Ab, Some("a || b")), + Binding::new("$", Dollar, Some("a")), + Binding::new("\"", Quote, Some("a")), + Binding::new("alt-s", Ess, Some("a")), + Binding::new("ctrl-`", Backtick, Some("a")), ]); let mut context_a = KeymapContext::default(); @@ -543,6 +558,30 @@ mod tests { MatchResult::Matches(vec![(1, Box::new(Ab))]) ); + // handle Czech $ (option + 4 key) + assert_eq!( + matcher.push_keystroke(Keystroke::parse("alt-ç->$")?, vec![(1, context_a.clone())]), + MatchResult::Matches(vec![(1, Box::new(Dollar))]) + ); + + // handle Brazillian quote (quote key then space key) + assert_eq!( + matcher.push_keystroke(Keystroke::parse("space->\"")?, vec![(1, context_a.clone())]), + MatchResult::Matches(vec![(1, Box::new(Quote))]) + ); + + // handle ctrl+` on a brazillian keyboard + assert_eq!( + matcher.push_keystroke(Keystroke::parse("ctrl-->`")?, vec![(1, context_a.clone())]), + MatchResult::Matches(vec![(1, Box::new(Backtick))]) + ); + + // handle alt-s on a US keyboard + assert_eq!( + matcher.push_keystroke(Keystroke::parse("alt-s->ß")?, vec![(1, context_a.clone())]), + MatchResult::Matches(vec![(1, Box::new(Ess))]) + ); + Ok(()) } } diff --git a/crates/gpui/src/keymap_matcher/keymap.rs b/crates/gpui/src/keymap_matcher/keymap.rs index 7cb95cab3a28aac3a41f7515e1551d22b1a776f5..6abbf1016f43552f1f8aa9d1069dfa8d00f36d44 100644 --- a/crates/gpui/src/keymap_matcher/keymap.rs +++ b/crates/gpui/src/keymap_matcher/keymap.rs @@ -162,7 +162,8 @@ mod tests { shift: false, cmd: false, function: false, - key: "q".to_string() + key: "q".to_string(), + ime_key: None, }], "{keystroke_duplicate_to_1:?} should have the expected keystroke in the keymap" ); @@ -179,7 +180,8 @@ mod tests { shift: false, cmd: false, function: false, - key: "w".to_string() + key: "w".to_string(), + ime_key: None, }, &Keystroke { ctrl: true, @@ -187,7 +189,8 @@ mod tests { shift: false, cmd: false, function: false, - key: "w".to_string() + key: "w".to_string(), + ime_key: None, } ], "{full_duplicate_to_2:?} should have a duplicated keystroke in the keymap" @@ -339,7 +342,8 @@ mod tests { shift: false, cmd: false, function: false, - key: expected_key.to_string() + key: expected_key.to_string(), + ime_key: None, }], "{expected:?} should have the expected keystroke with key '{expected_key}' in the keymap for element {i}" ); diff --git a/crates/gpui/src/keymap_matcher/keystroke.rs b/crates/gpui/src/keymap_matcher/keystroke.rs index 164dea8aba145aec263dd1c95afbbb79824bb83c..bc0caddac9b2dd43d37016af49d66b66d684ef76 100644 --- a/crates/gpui/src/keymap_matcher/keystroke.rs +++ b/crates/gpui/src/keymap_matcher/keystroke.rs @@ -2,6 +2,7 @@ use std::fmt::Write; use anyhow::anyhow; use serde::Deserialize; +use smallvec::SmallVec; #[derive(Clone, Debug, Eq, PartialEq, Default, Deserialize, Hash)] pub struct Keystroke { @@ -10,10 +11,47 @@ pub struct Keystroke { pub shift: bool, pub cmd: bool, pub function: bool, + /// key is the character printed on the key that was pressed + /// e.g. for option-s, key is "s" pub key: String, + /// ime_key is the character inserted by the IME engine when that key was pressed. + /// e.g. for option-s, ime_key is "ß" + pub ime_key: Option, } impl Keystroke { + // When matching a key we cannot know whether the user intended to type + // the ime_key or the key. On some non-US keyboards keys we use in our + // bindings are behind option (for example `$` is typed `alt-ç` on a Czech keyboard), + // and on some keyboards the IME handler converts a sequence of keys into a + // specific character (for example `"` is typed as `" space` on a brazillian keyboard). + pub fn match_possibilities(&self) -> SmallVec<[Keystroke; 2]> { + let mut possibilities = SmallVec::new(); + match self.ime_key.as_ref() { + None => possibilities.push(self.clone()), + Some(ime_key) => { + possibilities.push(Keystroke { + ctrl: self.ctrl, + alt: false, + shift: false, + cmd: false, + function: false, + key: ime_key.to_string(), + ime_key: None, + }); + possibilities.push(Keystroke { + ime_key: None, + ..self.clone() + }); + } + } + possibilities + } + + /// key syntax is: + /// [ctrl-][alt-][shift-][cmd-][fn-]key[->ime_key] + /// ime_key is only used for generating test events, + /// when matching a key with an ime_key set will be matched without it. pub fn parse(source: &str) -> anyhow::Result { let mut ctrl = false; let mut alt = false; @@ -21,6 +59,7 @@ impl Keystroke { let mut cmd = false; let mut function = false; let mut key = None; + let mut ime_key = None; let mut components = source.split('-').peekable(); while let Some(component) = components.next() { @@ -31,10 +70,14 @@ impl Keystroke { "cmd" => cmd = true, "fn" => function = true, _ => { - if let Some(component) = components.peek() { - if component.is_empty() && source.ends_with('-') { + if let Some(next) = components.peek() { + if next.is_empty() && source.ends_with('-') { key = Some(String::from("-")); break; + } else if next.len() > 1 && next.starts_with('>') { + key = Some(String::from(component)); + ime_key = Some(String::from(&next[1..])); + components.next(); } else { return Err(anyhow!("Invalid keystroke `{}`", source)); } @@ -54,6 +97,7 @@ impl Keystroke { cmd, function, key, + ime_key, }) } diff --git a/crates/gpui/src/platform/mac/event.rs b/crates/gpui/src/platform/mac/event.rs index 7710834f53433d0efaa685682b720f1ae42dcdf2..c19d7abe4ff9619c486036a0d64a3e4ee9b6020b 100644 --- a/crates/gpui/src/platform/mac/event.rs +++ b/crates/gpui/src/platform/mac/event.rs @@ -327,6 +327,7 @@ unsafe fn parse_keystroke(native_event: id) -> Keystroke { cmd, function, key, + ime_key: None, } } diff --git a/crates/gpui/src/platform/mac/window.rs b/crates/gpui/src/platform/mac/window.rs index ba8c7a69633057c732cbd0b03cdfbd6e34f1ff57..ad8275f0ac9de36732e04999cd182e756e14b7a8 100644 --- a/crates/gpui/src/platform/mac/window.rs +++ b/crates/gpui/src/platform/mac/window.rs @@ -285,6 +285,7 @@ enum ImeState { None, } +#[derive(Debug)] struct InsertText { replacement_range: Option>, text: String, @@ -1006,40 +1007,7 @@ extern "C" fn handle_key_event(this: &Object, native_event: id, key_equivalent: .flatten() .is_some(); if !is_composing { - // if the IME has changed the key, we'll first emit an event with the character - // generated by the IME system; then fallback to the keystroke if that is not - // handled. - // cases that we have working: - // - " on a brazillian layout by typing - // - ctrl-` on a brazillian layout by typing - // - $ on a czech QWERTY layout by typing - // - 4 on a czech QWERTY layout by typing - // - ctrl-4 on a czech QWERTY layout by typing (or ) - if ime_text.is_some() && ime_text.as_ref() != Some(&event.keystroke.key) { - let event_with_ime_text = KeyDownEvent { - is_held: false, - keystroke: Keystroke { - // we match ctrl because some use-cases need it. - // we don't match alt because it's often used to generate the optional character - // we don't match shift because we're not here with letters (usually) - // we don't match cmd/fn because they don't seem to use IME - ctrl: event.keystroke.ctrl, - alt: false, - shift: false, - cmd: false, - function: false, - key: ime_text.clone().unwrap(), - }, - }; - handled = callback(Event::KeyDown(event_with_ime_text)); - } - if !handled { - // empty key happens when you type a deadkey in input composition. - // (e.g. on a brazillian keyboard typing quote is a deadkey) - if !event.keystroke.key.is_empty() { - handled = callback(Event::KeyDown(event)); - } - } + handled = callback(Event::KeyDown(event)); } if !handled { @@ -1197,6 +1165,7 @@ extern "C" fn cancel_operation(this: &Object, _sel: Sel, _sender: id) { shift: false, function: false, key: ".".into(), + ime_key: None, }; let event = Event::KeyDown(KeyDownEvent { keystroke: keystroke.clone(), @@ -1479,6 +1448,9 @@ extern "C" fn insert_text(this: &Object, _: Sel, text: id, replacement_range: NS replacement_range, text: text.to_string(), }); + if text.to_string().to_ascii_lowercase() != pending_key_down.0.keystroke.key { + pending_key_down.0.keystroke.ime_key = Some(text.to_string()); + } window_state.borrow_mut().pending_key_down = Some(pending_key_down); } } diff --git a/crates/terminal/src/mappings/keys.rs b/crates/terminal/src/mappings/keys.rs index 9d1962597164ce7aecf33af61f57aa4a39b52eeb..b933fd48e9671b8e49edd3a8b34df1b353bc20f7 100644 --- a/crates/terminal/src/mappings/keys.rs +++ b/crates/terminal/src/mappings/keys.rs @@ -333,6 +333,7 @@ mod test { cmd: false, function: false, key: "🖖🏻".to_string(), //2 char string + ime_key: None, }; assert_eq!(to_esc_str(&ks, &TermMode::NONE, false), None); }