Fix multi-key shortcuts with modifiers

Conrad Irwin created

To make this work we need to move the handling of multiple possible key
events into the keyboard shortcut system.

This was broken in #2957.

Change summary

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 
crates/vim/src/state.rs                     |  1 
7 files changed, 117 insertions(+), 57 deletions(-)

Detailed changes

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

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

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

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

crates/vim/src/state.rs ๐Ÿ”—

@@ -186,7 +186,6 @@ impl EditorState {
         if self.active_operator().is_none() && self.pre_count.is_some()
             || self.active_operator().is_some() && self.post_count.is_some()
         {
-            dbg!("VimCount");
             context.add_identifier("VimCount");
         }