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

@@ -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 *": [

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,7 +106,9 @@ impl KeymapMatcher {
             }
 
             for binding in self.keymap.bindings().iter().rev() {
-                match binding.match_keys_and_context(&self.pending_keystrokes, &self.contexts[i..])
+                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 {
@@ -114,16 +116,22 @@ impl KeymapMatcher {
                             }
                         }
                         BindingMatchResult::Partial => {
+                            if pending_key == None || pending_key == Some(possibility.clone()) {
                                 self.pending_views
                                     .insert(*view_id, self.contexts[i].clone());
-                        any_pending = true;
+                                pending_key = Some(possibility)
+                            }
                         }
                         _ => {}
                     }
+                    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(())
     }
 }

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}"
             );

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<String>,
 }
 
 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<Self> {
         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 let Some(next) = components.peek() {
-                        if component.is_empty() && source.ends_with('-') {
+                        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,
         })
     }
 

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,
     }
 }
 

crates/gpui/src/platform/mac/window.rs

@@ -285,6 +285,7 @@ enum ImeState {
     None,
 }
 
+#[derive(Debug)]
 struct InsertText {
     replacement_range: Option<Range<usize>>,
     text: String,
@@ -1006,41 +1007,8 @@ 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 <quote><space>
-                    // - ctrl-` on a brazillian layout by typing <ctrl-`>
-                    // - $ on a czech QWERTY layout by typing <alt-4>
-                    // - 4 on a czech QWERTY layout by typing <shift-4>
-                    // - ctrl-4 on a czech QWERTY layout by typing <ctrl-alt-4> (or <ctrl-shift-4>)
-                    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));
                 }
-                    }
-                }
 
                 if !handled {
                     if let Some(insert) = insert_text {
@@ -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);
         }
     }

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);
     }