From 8e12eb0ab1d77df595e2f90b9c737a51916e6039 Mon Sep 17 00:00:00 2001 From: Ben Kunkle Date: Thu, 27 Mar 2025 17:17:43 -0400 Subject: [PATCH] keymap: Detect and report errors for uppercase keybindings (#27558) Closes #25353 Detect keybindings using upper case instead of lowercase, and report an error Release Notes: - N/A --- crates/editor/src/test/editor_test_context.rs | 2 +- crates/gpui/src/app/test_context.rs | 8 +- crates/gpui/src/platform/keystroke.rs | 126 +++++++++++++----- crates/terminal/src/mappings/keys.rs | 41 +++--- crates/vim/src/test.rs | 4 +- 5 files changed, 123 insertions(+), 58 deletions(-) diff --git a/crates/editor/src/test/editor_test_context.rs b/crates/editor/src/test/editor_test_context.rs index 37bc175c64..565bac336f 100644 --- a/crates/editor/src/test/editor_test_context.rs +++ b/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); } diff --git a/crates/gpui/src/app/test_context.rs b/crates/gpui/src/app/test_context.rs index c4cb595252..fb5d5cb602 100644 --- a/crates/gpui/src/app/test_context.rs +++ b/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); } diff --git a/crates/gpui/src/platform/keystroke.rs b/crates/gpui/src/platform/keystroke.rs index bb16398a35..67c8b64c14 100644 --- a/crates/gpui/src/platform/keystroke.rs +++ b/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 { + 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 { + return Self::parse_impl(source, false); + } + + fn parse_impl( + source: &str, + case_sensitive: bool, + ) -> std::result::Result { 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 - }; - } - "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)); - } - } + 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(), + }); + } + 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, }) } diff --git a/crates/terminal/src/mappings/keys.rs b/crates/terminal/src/mappings/keys.rs index 1efc1f17d2..ef1b3464af 100644 --- a/crates/terminal/src/mappings/keys.rs +++ b/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()) ); } } diff --git a/crates/vim/src/test.rs b/crates/vim/src/test.rs index c9664e9346..581f66d97e 100644 --- a/crates/vim/src/test.rs +++ b/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"), ),