keymap: Detect and report errors for uppercase keybindings (#27558)

Ben Kunkle created

Closes #25353

Detect keybindings using upper case instead of lowercase, and report an
error

Release Notes:

- N/A

Change summary

crates/editor/src/test/editor_test_context.rs |   2 
crates/gpui/src/app/test_context.rs           |   8 +
crates/gpui/src/platform/keystroke.rs         | 122 +++++++++++++++-----
crates/terminal/src/mappings/keys.rs          |  41 +++---
crates/vim/src/test.rs                        |   4 
5 files changed, 121 insertions(+), 56 deletions(-)

Detailed changes

crates/editor/src/test/editor_test_context.rs 🔗

@@ -240,7 +240,7 @@ impl EditorTestContext {
     // unlike cx.simulate_keystrokes(), this does not run_until_parked
     // so you can use it to test detailed timing
     pub fn simulate_keystroke(&mut self, keystroke_text: &str) {
-        let keystroke = Keystroke::parse(keystroke_text).unwrap();
+        let keystroke = Keystroke::parse_case_insensitive(keystroke_text).unwrap();
         self.cx.dispatch_keystroke(self.window, keystroke);
     }
 

crates/gpui/src/app/test_context.rs 🔗

@@ -399,7 +399,7 @@ impl TestAppContext {
     pub fn simulate_keystrokes(&mut self, window: AnyWindowHandle, keystrokes: &str) {
         for keystroke in keystrokes
             .split(' ')
-            .map(Keystroke::parse)
+            .map(Keystroke::parse_case_insensitive)
             .map(Result::unwrap)
         {
             self.dispatch_keystroke(window, keystroke);
@@ -413,7 +413,11 @@ impl TestAppContext {
     /// will type abc into your current editor
     /// This will also run the background executor until it's parked.
     pub fn simulate_input(&mut self, window: AnyWindowHandle, input: &str) {
-        for keystroke in input.split("").map(Keystroke::parse).map(Result::unwrap) {
+        for keystroke in input
+            .split("")
+            .map(Keystroke::parse_case_insensitive)
+            .map(Result::unwrap)
+        {
             self.dispatch_keystroke(window, keystroke);
         }
 

crates/gpui/src/platform/keystroke.rs 🔗

@@ -42,9 +42,9 @@ impl Display for InvalidKeystrokeError {
 }
 
 /// Sentence explaining what keystroke parser expects, starting with "Expected ..."
-pub const KEYSTROKE_PARSE_EXPECTED_MESSAGE: &str = "Expected a sequence of modifiers \
+pub const KEYSTROKE_PARSE_EXPECTED_MESSAGE: &str = "Expected a sequence of lowercase modifiers \
     (`ctrl`, `alt`, `shift`, `fn`, `cmd`, `super`, or `win`) \
-    followed by a key, separated by `-`.";
+    followed by a lowercase key, separated by `-`.";
 
 impl Keystroke {
     /// When matching a key we cannot know whether the user intended to type
@@ -81,6 +81,28 @@ impl Keystroke {
     /// secondary means "cmd" on macOS and "ctrl" on other platforms
     /// when matching a key with an key_char set will be matched without it.
     pub fn parse(source: &str) -> std::result::Result<Self, InvalidKeystrokeError> {
+        return Self::parse_impl(source, true);
+    }
+
+    /// Parse a keystroke case-insensitively. This means
+    /// keystrokes like `ctrl-T` will not be rejected.
+    /// Useful in tests to allow more concise keystroke inputs,
+    /// e.g., `simulate_keystrokes("ctrl-T")` instead of `simulate_keystrokes("ctrl-shift-t")`.
+    /// This also allows `simulate_input` style functions to support capital letters,
+    /// e.g., `simulate_input("Title Case")` can work by just parsing each character as a keystroke
+    /// and dispatching it, instead of needing to parse something like
+    /// `simulate_input("shift-title shift-case")`.
+    #[cfg(any(test, feature = "test-support"))]
+    pub fn parse_case_insensitive(
+        source: &str,
+    ) -> std::result::Result<Self, InvalidKeystrokeError> {
+        return Self::parse_impl(source, false);
+    }
+
+    fn parse_impl(
+        source: &str,
+        case_sensitive: bool,
+    ) -> std::result::Result<Self, InvalidKeystrokeError> {
         let mut control = false;
         let mut alt = false;
         let mut shift = false;
@@ -91,38 +113,74 @@ impl Keystroke {
 
         let mut components = source.split('-').peekable();
         while let Some(component) = components.next() {
-            match component {
-                "ctrl" => control = true,
-                "alt" => alt = true,
-                "shift" => shift = true,
-                "fn" => function = true,
-                "secondary" => {
-                    if cfg!(target_os = "macos") {
-                        platform = true
-                    } else {
-                        control = true
-                    };
+            if component.eq_ignore_ascii_case("ctrl") {
+                control = true;
+                continue;
+            }
+            if component.eq_ignore_ascii_case("alt") {
+                alt = true;
+                continue;
+            }
+            if component.eq_ignore_ascii_case("shift") {
+                shift = true;
+                continue;
+            }
+            if component.eq_ignore_ascii_case("fn") {
+                function = true;
+                continue;
+            }
+            if component.eq_ignore_ascii_case("secondary") {
+                if cfg!(target_os = "macos") {
+                    platform = true;
+                } else {
+                    control = true;
+                };
+                continue;
+            }
+
+            let is_platform = component.eq_ignore_ascii_case("cmd")
+                || component.eq_ignore_ascii_case("super")
+                || component.eq_ignore_ascii_case("win");
+
+            if is_platform {
+                platform = true;
+                continue;
+            }
+
+            let mut key_str = component.to_string();
+
+            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(key_str);
+                    key_char = Some(String::from(&next[1..]));
+                    components.next();
+                } else {
+                    return Err(InvalidKeystrokeError {
+                        keystroke: source.to_owned(),
+                    });
                 }
-                "cmd" | "super" | "win" => platform = true,
-                _ => {
-                    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));
-                            key_char = Some(String::from(&next[1..]));
-                            components.next();
-                        } else {
-                            return Err(InvalidKeystrokeError {
-                                keystroke: source.to_owned(),
-                            });
-                        }
-                    } else {
-                        key = Some(String::from(component));
-                    }
+                continue;
+            }
+
+            if component.len() == 1 && component.as_bytes()[0].is_ascii_uppercase() {
+                if case_sensitive {
+                    return Err(InvalidKeystrokeError {
+                        keystroke: source.to_owned(),
+                    });
+                } else {
+                    // Convert to shift + lowercase char if parsing case insensitively
+                    shift = true;
+                    key_str.make_ascii_lowercase();
                 }
+            } else if case_sensitive {
+                // convert ascii chars to lowercase so that named keys like "tab" and "enter"
+                // are accepted case insensitively and stored how we expect so they are matched properly
+                key_str.make_ascii_lowercase()
             }
+            key = Some(key_str);
         }
 
         // Allow for the user to specify a keystroke modifier as the key itself
@@ -159,7 +217,7 @@ impl Keystroke {
                 function,
             },
             key,
-            key_char: key_char,
+            key_char,
         })
     }
 

crates/terminal/src/mappings/keys.rs 🔗

@@ -240,15 +240,18 @@ pub fn to_esc_str(keystroke: &Keystroke, mode: &TermMode, alt_is_meta: bool) ->
         }
     }
 
-    let alt_meta_binding =
-        if alt_is_meta && modifiers == AlacModifiers::Alt && keystroke.key.is_ascii() {
-            Some(format!("\x1b{}", keystroke.key))
-        } else {
-            None
-        };
-
-    if alt_meta_binding.is_some() {
-        return alt_meta_binding;
+    if alt_is_meta {
+        let is_alt_lowercase_ascii = modifiers == AlacModifiers::Alt && keystroke.key.is_ascii();
+        let is_alt_uppercase_ascii =
+            keystroke.modifiers.alt && keystroke.modifiers.shift && keystroke.key.is_ascii();
+        if is_alt_lowercase_ascii || is_alt_uppercase_ascii {
+            let key = if is_alt_uppercase_ascii {
+                &keystroke.key.to_ascii_uppercase()
+            } else {
+                &keystroke.key
+            };
+            return Some(format!("\x1b{}", key));
+        }
     }
 
     None
@@ -390,12 +393,12 @@ mod test {
         for (lower, upper) in letters_lower.zip(letters_upper) {
             assert_eq!(
                 to_esc_str(
-                    &Keystroke::parse(&format!("ctrl-{}", lower)).unwrap(),
+                    &Keystroke::parse(&format!("ctrl-shift-{}", lower)).unwrap(),
                     &mode,
                     false
                 ),
                 to_esc_str(
-                    &Keystroke::parse(&format!("ctrl-shift-{}", upper)).unwrap(),
+                    &Keystroke::parse_case_insensitive(&format!("ctrl-{}", upper)).unwrap(),
                     &mode,
                     false
                 ),
@@ -412,7 +415,7 @@ mod test {
         for character in ascii_printable {
             assert_eq!(
                 to_esc_str(
-                    &Keystroke::parse(&format!("alt-{}", character)).unwrap(),
+                    &Keystroke::parse_case_insensitive(&format!("alt-{}", character)).unwrap(),
                     &TermMode::NONE,
                     true
                 )
@@ -453,15 +456,15 @@ mod test {
         //    8     | Shift + Alt + Control
         // ---------+---------------------------
         // from: https://invisible-island.net/xterm/ctlseqs/ctlseqs.html#h2-PC-Style-Function-Keys
-        assert_eq!(2, modifier_code(&Keystroke::parse("shift-A").unwrap()));
-        assert_eq!(3, modifier_code(&Keystroke::parse("alt-A").unwrap()));
-        assert_eq!(4, modifier_code(&Keystroke::parse("shift-alt-A").unwrap()));
-        assert_eq!(5, modifier_code(&Keystroke::parse("ctrl-A").unwrap()));
-        assert_eq!(6, modifier_code(&Keystroke::parse("shift-ctrl-A").unwrap()));
-        assert_eq!(7, modifier_code(&Keystroke::parse("alt-ctrl-A").unwrap()));
+        assert_eq!(2, modifier_code(&Keystroke::parse("shift-a").unwrap()));
+        assert_eq!(3, modifier_code(&Keystroke::parse("alt-a").unwrap()));
+        assert_eq!(4, modifier_code(&Keystroke::parse("shift-alt-a").unwrap()));
+        assert_eq!(5, modifier_code(&Keystroke::parse("ctrl-a").unwrap()));
+        assert_eq!(6, modifier_code(&Keystroke::parse("shift-ctrl-a").unwrap()));
+        assert_eq!(7, modifier_code(&Keystroke::parse("alt-ctrl-a").unwrap()));
         assert_eq!(
             8,
-            modifier_code(&Keystroke::parse("shift-ctrl-alt-A").unwrap())
+            modifier_code(&Keystroke::parse("shift-ctrl-alt-a").unwrap())
         );
     }
 }

crates/vim/src/test.rs 🔗

@@ -1349,12 +1349,12 @@ async fn test_sneak(cx: &mut gpui::TestAppContext) {
                 Some("vim_mode == normal"),
             ),
             KeyBinding::new(
-                "S",
+                "shift-s",
                 PushSneakBackward { first_char: None },
                 Some("vim_mode == normal"),
             ),
             KeyBinding::new(
-                "S",
+                "shift-s",
                 PushSneakBackward { first_char: None },
                 Some("vim_mode == visual"),
             ),