From e062f30d9ea264662137a96f7d769deb8af8670e Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Wed, 20 Nov 2024 20:29:47 -0700 Subject: [PATCH] Rename ime_key -> key_char and update behavior (#20953) As part of the recent changes to keyboard support, ime_key is no longer populated by the IME; but instead by the keyboard. As part of #20877 I changed some code to assume that falling back to key was ok, but this was not ok; instead we need to populate this more similarly to how it was done before #20336. The alternative fix could be to instead of simulating these events in our own code to push a fake native event back to the platform input handler. Closes #ISSUE Release Notes: - Fixed a bug where tapping `shift` coudl type "shift" if you had a binding on "shift shift" --- crates/gpui/examples/input.rs | 4 +- crates/gpui/src/platform/keystroke.rs | 37 ++++++++++--------- crates/gpui/src/platform/linux/platform.rs | 6 +-- .../gpui/src/platform/linux/wayland/client.rs | 6 +-- .../gpui/src/platform/linux/wayland/window.rs | 4 +- crates/gpui/src/platform/linux/x11/client.rs | 16 ++++---- crates/gpui/src/platform/linux/x11/window.rs | 4 +- crates/gpui/src/platform/mac/events.rs | 26 +++++++------ crates/gpui/src/platform/mac/window.rs | 21 +++++------ crates/gpui/src/platform/windows/events.rs | 14 +++---- crates/gpui/src/window.rs | 12 ++---- .../markdown_preview/src/markdown_renderer.rs | 2 +- crates/terminal/src/mappings/keys.rs | 2 +- crates/vim/src/digraph.rs | 2 +- 14 files changed, 77 insertions(+), 79 deletions(-) diff --git a/crates/gpui/examples/input.rs b/crates/gpui/examples/input.rs index d52697c43f07d..29014946cb478 100644 --- a/crates/gpui/examples/input.rs +++ b/crates/gpui/examples/input.rs @@ -581,8 +581,8 @@ impl Render for InputExample { format!( "{:} {}", ks.unparse(), - if let Some(ime_key) = ks.ime_key.as_ref() { - format!("-> {:?}", ime_key) + if let Some(key_char) = ks.key_char.as_ref() { + format!("-> {:?}", key_char) } else { "".to_owned() } diff --git a/crates/gpui/src/platform/keystroke.rs b/crates/gpui/src/platform/keystroke.rs index 20a12a691b080..af1e5179db7b4 100644 --- a/crates/gpui/src/platform/keystroke.rs +++ b/crates/gpui/src/platform/keystroke.rs @@ -12,14 +12,15 @@ pub struct Keystroke { /// 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, + /// key_char is the character that could have been typed when + /// this binding was pressed. + /// e.g. for s this is "s", for option-s "รŸ", and cmd-s None + pub key_char: Option, } impl Keystroke { /// When matching a key we cannot know whether the user intended to type - /// the ime_key or the key itself. On some non-US keyboards keys we use in our + /// the key_char or the key itself. 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 brazilian keyboard). @@ -27,10 +28,10 @@ impl Keystroke { /// This method assumes that `self` was typed and `target' is in the keymap, and checks /// both possibilities for self against the target. pub(crate) fn should_match(&self, target: &Keystroke) -> bool { - if let Some(ime_key) = self - .ime_key + if let Some(key_char) = self + .key_char .as_ref() - .filter(|ime_key| ime_key != &&self.key) + .filter(|key_char| key_char != &&self.key) { let ime_modifiers = Modifiers { control: self.modifiers.control, @@ -38,7 +39,7 @@ impl Keystroke { ..Default::default() }; - if &target.key == ime_key && target.modifiers == ime_modifiers { + if &target.key == key_char && target.modifiers == ime_modifiers { return true; } } @@ -47,9 +48,9 @@ impl Keystroke { } /// key syntax is: - /// [ctrl-][alt-][shift-][cmd-][fn-]key[->ime_key] - /// ime_key syntax is only used for generating test events, - /// when matching a key with an ime_key set will be matched without it. + /// [ctrl-][alt-][shift-][cmd-][fn-]key[->key_char] + /// key_char syntax is only used for generating test events, + /// when matching a key with an key_char set will be matched without it. pub fn parse(source: &str) -> anyhow::Result { let mut control = false; let mut alt = false; @@ -57,7 +58,7 @@ impl Keystroke { let mut platform = false; let mut function = false; let mut key = None; - let mut ime_key = None; + let mut key_char = None; let mut components = source.split('-').peekable(); while let Some(component) = components.next() { @@ -74,7 +75,7 @@ impl Keystroke { break; } else if next.len() > 1 && next.starts_with('>') { key = Some(String::from(component)); - ime_key = Some(String::from(&next[1..])); + key_char = Some(String::from(&next[1..])); components.next(); } else { return Err(anyhow!("Invalid keystroke `{}`", source)); @@ -118,7 +119,7 @@ impl Keystroke { function, }, key, - ime_key, + key_char: key_char, }) } @@ -154,7 +155,7 @@ impl Keystroke { /// Returns true if this keystroke left /// the ime system in an incomplete state. pub fn is_ime_in_progress(&self) -> bool { - self.ime_key.is_none() + self.key_char.is_none() && (is_printable_key(&self.key) || self.key.is_empty()) && !(self.modifiers.platform || self.modifiers.control @@ -162,17 +163,17 @@ impl Keystroke { || self.modifiers.alt) } - /// Returns a new keystroke with the ime_key filled. + /// Returns a new keystroke with the key_char filled. /// This is used for dispatch_keystroke where we want users to /// be able to simulate typing "space", etc. pub fn with_simulated_ime(mut self) -> Self { - if self.ime_key.is_none() + if self.key_char.is_none() && !self.modifiers.platform && !self.modifiers.control && !self.modifiers.function && !self.modifiers.alt { - self.ime_key = match self.key.as_str() { + self.key_char = match self.key.as_str() { "space" => Some(" ".into()), "tab" => Some("\t".into()), "enter" => Some("\n".into()), diff --git a/crates/gpui/src/platform/linux/platform.rs b/crates/gpui/src/platform/linux/platform.rs index f778ebc074e33..650ed70af8c6d 100644 --- a/crates/gpui/src/platform/linux/platform.rs +++ b/crates/gpui/src/platform/linux/platform.rs @@ -742,14 +742,14 @@ impl Keystroke { } } - // Ignore control characters (and DEL) for the purposes of ime_key - let ime_key = + // Ignore control characters (and DEL) for the purposes of key_char + let key_char = (key_utf32 >= 32 && key_utf32 != 127 && !key_utf8.is_empty()).then_some(key_utf8); Keystroke { modifiers, key, - ime_key, + key_char, } } diff --git a/crates/gpui/src/platform/linux/wayland/client.rs b/crates/gpui/src/platform/linux/wayland/client.rs index ab87bb20242ed..e193201957918 100644 --- a/crates/gpui/src/platform/linux/wayland/client.rs +++ b/crates/gpui/src/platform/linux/wayland/client.rs @@ -1208,7 +1208,7 @@ impl Dispatch for WaylandClientStatePtr { compose.feed(keysym); match compose.status() { xkb::Status::Composing => { - keystroke.ime_key = None; + keystroke.key_char = None; state.pre_edit_text = compose.utf8().or(Keystroke::underlying_dead_key(keysym)); let pre_edit = @@ -1220,7 +1220,7 @@ impl Dispatch for WaylandClientStatePtr { xkb::Status::Composed => { state.pre_edit_text.take(); - keystroke.ime_key = compose.utf8(); + keystroke.key_char = compose.utf8(); if let Some(keysym) = compose.keysym() { keystroke.key = xkb::keysym_get_name(keysym); } @@ -1340,7 +1340,7 @@ impl Dispatch for WaylandClientStatePtr { keystroke: Keystroke { modifiers: Modifiers::default(), key: commit_text.clone(), - ime_key: Some(commit_text), + key_char: Some(commit_text), }, is_held: false, })); diff --git a/crates/gpui/src/platform/linux/wayland/window.rs b/crates/gpui/src/platform/linux/wayland/window.rs index 8d4516b3f37b7..55ba4f6004d39 100644 --- a/crates/gpui/src/platform/linux/wayland/window.rs +++ b/crates/gpui/src/platform/linux/wayland/window.rs @@ -687,11 +687,11 @@ impl WaylandWindowStatePtr { } } if let PlatformInput::KeyDown(event) = input { - if let Some(ime_key) = &event.keystroke.ime_key { + if let Some(key_char) = &event.keystroke.key_char { let mut state = self.state.borrow_mut(); if let Some(mut input_handler) = state.input_handler.take() { drop(state); - input_handler.replace_text_in_range(None, ime_key); + input_handler.replace_text_in_range(None, key_char); self.state.borrow_mut().input_handler = Some(input_handler); } } diff --git a/crates/gpui/src/platform/linux/x11/client.rs b/crates/gpui/src/platform/linux/x11/client.rs index 82ef39fc6b2a7..f6c3af0348709 100644 --- a/crates/gpui/src/platform/linux/x11/client.rs +++ b/crates/gpui/src/platform/linux/x11/client.rs @@ -178,7 +178,7 @@ pub struct X11ClientState { pub(crate) compose_state: Option, pub(crate) pre_edit_text: Option, pub(crate) composing: bool, - pub(crate) pre_ime_key_down: Option, + pub(crate) pre_key_char_down: Option, pub(crate) cursor_handle: cursor::Handle, pub(crate) cursor_styles: HashMap, pub(crate) cursor_cache: HashMap, @@ -446,7 +446,7 @@ impl X11Client { compose_state, pre_edit_text: None, - pre_ime_key_down: None, + pre_key_char_down: None, composing: false, cursor_handle, @@ -858,7 +858,7 @@ impl X11Client { let modifiers = modifiers_from_state(event.state); state.modifiers = modifiers; - state.pre_ime_key_down.take(); + state.pre_key_char_down.take(); let keystroke = { let code = event.detail.into(); let xkb_state = state.previous_xkb_state.clone(); @@ -880,13 +880,13 @@ impl X11Client { match compose_state.status() { xkbc::Status::Composed => { state.pre_edit_text.take(); - keystroke.ime_key = compose_state.utf8(); + keystroke.key_char = compose_state.utf8(); if let Some(keysym) = compose_state.keysym() { keystroke.key = xkbc::keysym_get_name(keysym); } } xkbc::Status::Composing => { - keystroke.ime_key = None; + keystroke.key_char = None; state.pre_edit_text = compose_state .utf8() .or(crate::Keystroke::underlying_dead_key(keysym)); @@ -1156,7 +1156,7 @@ impl X11Client { match event { Event::KeyPress(event) | Event::KeyRelease(event) => { let mut state = self.0.borrow_mut(); - state.pre_ime_key_down = Some(Keystroke::from_xkb( + state.pre_key_char_down = Some(Keystroke::from_xkb( &state.xkb, state.modifiers, event.detail.into(), @@ -1187,11 +1187,11 @@ impl X11Client { fn xim_handle_commit(&self, window: xproto::Window, text: String) -> Option<()> { let window = self.get_window(window).unwrap(); let mut state = self.0.borrow_mut(); - let keystroke = state.pre_ime_key_down.take(); + let keystroke = state.pre_key_char_down.take(); state.composing = false; drop(state); if let Some(mut keystroke) = keystroke { - keystroke.ime_key = Some(text.clone()); + keystroke.key_char = Some(text.clone()); window.handle_input(PlatformInput::KeyDown(crate::KeyDownEvent { keystroke, is_held: false, diff --git a/crates/gpui/src/platform/linux/x11/window.rs b/crates/gpui/src/platform/linux/x11/window.rs index 15712233c2ffe..4df1b50f3fb17 100644 --- a/crates/gpui/src/platform/linux/x11/window.rs +++ b/crates/gpui/src/platform/linux/x11/window.rs @@ -846,9 +846,9 @@ impl X11WindowStatePtr { if let PlatformInput::KeyDown(event) = input { let mut state = self.state.borrow_mut(); if let Some(mut input_handler) = state.input_handler.take() { - if let Some(ime_key) = &event.keystroke.ime_key { + if let Some(key_char) = &event.keystroke.key_char { drop(state); - input_handler.replace_text_in_range(None, ime_key); + input_handler.replace_text_in_range(None, key_char); state = self.state.borrow_mut(); } state.input_handler = Some(input_handler); diff --git a/crates/gpui/src/platform/mac/events.rs b/crates/gpui/src/platform/mac/events.rs index 51716cccb4e2e..f715dba5620d9 100644 --- a/crates/gpui/src/platform/mac/events.rs +++ b/crates/gpui/src/platform/mac/events.rs @@ -245,7 +245,7 @@ unsafe fn parse_keystroke(native_event: id) -> Keystroke { .charactersIgnoringModifiers() .to_str() .to_string(); - let mut ime_key = None; + let mut key_char = None; let first_char = characters.chars().next().map(|ch| ch as u16); let modifiers = native_event.modifierFlags(); @@ -261,13 +261,19 @@ unsafe fn parse_keystroke(native_event: id) -> Keystroke { #[allow(non_upper_case_globals)] let key = match first_char { Some(SPACE_KEY) => { - ime_key = Some(" ".to_string()); + key_char = Some(" ".to_string()); "space".to_string() } + Some(TAB_KEY) => { + key_char = Some("\t".to_string()); + "tab".to_string() + } + Some(ENTER_KEY) | Some(NUMPAD_ENTER_KEY) => { + key_char = Some("\n".to_string()); + "enter".to_string() + } Some(BACKSPACE_KEY) => "backspace".to_string(), - Some(ENTER_KEY) | Some(NUMPAD_ENTER_KEY) => "enter".to_string(), Some(ESCAPE_KEY) => "escape".to_string(), - Some(TAB_KEY) => "tab".to_string(), Some(SHIFT_TAB_KEY) => "tab".to_string(), Some(NSUpArrowFunctionKey) => "up".to_string(), Some(NSDownArrowFunctionKey) => "down".to_string(), @@ -348,7 +354,7 @@ unsafe fn parse_keystroke(native_event: id) -> Keystroke { chars_ignoring_modifiers }; - if always_use_cmd_layout || alt { + if !control && !command && !function { let mut mods = NO_MOD; if shift { mods |= SHIFT_MOD; @@ -356,11 +362,9 @@ unsafe fn parse_keystroke(native_event: id) -> Keystroke { if alt { mods |= OPTION_MOD; } - let alt_key = chars_for_modified_key(native_event.keyCode(), mods); - if alt_key != key { - ime_key = Some(alt_key); - } - }; + + key_char = Some(chars_for_modified_key(native_event.keyCode(), mods)); + } key } @@ -375,7 +379,7 @@ unsafe fn parse_keystroke(native_event: id) -> Keystroke { function, }, key, - ime_key, + key_char, } } diff --git a/crates/gpui/src/platform/mac/window.rs b/crates/gpui/src/platform/mac/window.rs index e5a04191a33a9..abb532980a489 100644 --- a/crates/gpui/src/platform/mac/window.rs +++ b/crates/gpui/src/platform/mac/window.rs @@ -1283,18 +1283,17 @@ extern "C" fn handle_key_event(this: &Object, native_event: id, key_equivalent: } if event.is_held { - let handled = with_input_handler(&this, |input_handler| { - if !input_handler.apple_press_and_hold_enabled() { - input_handler.replace_text_in_range( - None, - &event.keystroke.ime_key.unwrap_or(event.keystroke.key), - ); + if let Some(key_char) = event.keystroke.key_char.as_ref() { + let handled = with_input_handler(&this, |input_handler| { + if !input_handler.apple_press_and_hold_enabled() { + input_handler.replace_text_in_range(None, &key_char); + return YES; + } + NO + }); + if handled == Some(YES) { return YES; } - NO - }); - if handled == Some(YES) { - return YES; } } @@ -1437,7 +1436,7 @@ extern "C" fn cancel_operation(this: &Object, _sel: Sel, _sender: id) { let keystroke = Keystroke { modifiers: Default::default(), key: ".".into(), - ime_key: None, + key_char: None, }; let event = PlatformInput::KeyDown(KeyDownEvent { keystroke: keystroke.clone(), diff --git a/crates/gpui/src/platform/windows/events.rs b/crates/gpui/src/platform/windows/events.rs index 92adf6c7cb24a..5f45d260d94a6 100644 --- a/crates/gpui/src/platform/windows/events.rs +++ b/crates/gpui/src/platform/windows/events.rs @@ -386,7 +386,7 @@ fn handle_char_msg( return Some(1); }; drop(lock); - let ime_key = keystroke.ime_key.clone(); + let key_char = keystroke.key_char.clone(); let event = KeyDownEvent { keystroke, is_held: lparam.0 & (0x1 << 30) > 0, @@ -397,7 +397,7 @@ fn handle_char_msg( if dispatch_event_result.default_prevented || !dispatch_event_result.propagate { return Some(0); } - let Some(ime_char) = ime_key else { + let Some(ime_char) = key_char else { return Some(1); }; with_input_handler(&state_ptr, |input_handler| { @@ -1172,7 +1172,7 @@ fn parse_syskeydown_msg_keystroke(wparam: WPARAM) -> Option { Some(Keystroke { modifiers, key, - ime_key: None, + key_char: None, }) } @@ -1220,7 +1220,7 @@ fn parse_keydown_msg_keystroke(wparam: WPARAM) -> Option { return Some(KeystrokeOrModifier::Keystroke(Keystroke { modifiers, key: format!("f{}", offset + 1), - ime_key: None, + key_char: None, })); }; return None; @@ -1231,7 +1231,7 @@ fn parse_keydown_msg_keystroke(wparam: WPARAM) -> Option { Some(KeystrokeOrModifier::Keystroke(Keystroke { modifiers, key, - ime_key: None, + key_char: None, })) } @@ -1253,7 +1253,7 @@ fn parse_char_msg_keystroke(wparam: WPARAM) -> Option { Some(Keystroke { modifiers, key, - ime_key: Some(first_char.to_string()), + key_char: Some(first_char.to_string()), }) } } @@ -1327,7 +1327,7 @@ fn basic_vkcode_to_string(code: u16, modifiers: Modifiers) -> Option Some(Keystroke { modifiers, key, - ime_key: None, + key_char: None, }) } diff --git a/crates/gpui/src/window.rs b/crates/gpui/src/window.rs index ec1fd601ecb6e..e4fa74f981ed5 100644 --- a/crates/gpui/src/window.rs +++ b/crates/gpui/src/window.rs @@ -3038,7 +3038,7 @@ impl<'a> WindowContext<'a> { return true; } - if let Some(input) = keystroke.with_simulated_ime().ime_key { + if let Some(input) = keystroke.key_char { if let Some(mut input_handler) = self.window.platform_window.take_input_handler() { input_handler.dispatch_input(&input, self); self.window.platform_window.set_input_handler(input_handler); @@ -3267,7 +3267,7 @@ impl<'a> WindowContext<'a> { if let Some(key) = key { keystroke = Some(Keystroke { key: key.to_string(), - ime_key: None, + key_char: None, modifiers: Modifiers::default(), }); } @@ -3482,13 +3482,7 @@ impl<'a> WindowContext<'a> { if !self.propagate_event { continue 'replay; } - if let Some(input) = replay - .keystroke - .with_simulated_ime() - .ime_key - .as_ref() - .cloned() - { + if let Some(input) = replay.keystroke.key_char.as_ref().cloned() { if let Some(mut input_handler) = self.window.platform_window.take_input_handler() { input_handler.dispatch_input(&input, self); self.window.platform_window.set_input_handler(input_handler) diff --git a/crates/markdown_preview/src/markdown_renderer.rs b/crates/markdown_preview/src/markdown_renderer.rs index f38e1c49b5abc..37ca5636a6794 100644 --- a/crates/markdown_preview/src/markdown_renderer.rs +++ b/crates/markdown_preview/src/markdown_renderer.rs @@ -206,7 +206,7 @@ fn render_markdown_list_item( let secondary_modifier = Keystroke { key: "".to_string(), modifiers: Modifiers::secondary_key(), - ime_key: None, + key_char: None, }; Tooltip::text( format!("{}-click to toggle the checkbox", secondary_modifier), diff --git a/crates/terminal/src/mappings/keys.rs b/crates/terminal/src/mappings/keys.rs index 2d4fe4c62e97b..1efc1f17d23cf 100644 --- a/crates/terminal/src/mappings/keys.rs +++ b/crates/terminal/src/mappings/keys.rs @@ -343,7 +343,7 @@ mod test { function: false, }, key: "๐Ÿ––๐Ÿป".to_string(), //2 char string - ime_key: None, + key_char: None, }; assert_eq!(to_esc_str(&ks, &TermMode::NONE, false), None); } diff --git a/crates/vim/src/digraph.rs b/crates/vim/src/digraph.rs index 4c09dd3e33313..dcccc8b5cd316 100644 --- a/crates/vim/src/digraph.rs +++ b/crates/vim/src/digraph.rs @@ -83,7 +83,7 @@ impl Vim { cx: &mut ViewContext, ) { // handled by handle_literal_input - if keystroke_event.keystroke.ime_key.is_some() { + if keystroke_event.keystroke.key_char.is_some() { return; };