From 04e1641a29d148a945e7d93682ca89ca28d95a60 Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Fri, 9 Feb 2024 12:22:12 +0100 Subject: [PATCH] Properly handle backspace when in dead key state (#7494) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously we wouldn't handle Backspace in dead key state correctly: instead of removing what was typed, we'd insert the text that was in the dead key state. Example: on a US English layout, press `opt-u` to end up in a dead key state with `¨` waiting for the next character to be typed. Type `backspace`. The `¨` should be removed, but it's not. With this change, the `backspace` is interpreted instead of being ignored. Release Notes: - Fixed backspace not working for dead keys (i.e. when typing accents or umlauts) --------- Co-authored-by: Antonio Co-authored-by: Conrad --- crates/gpui/src/platform/mac/window.rs | 219 ++++++++++++------------- 1 file changed, 106 insertions(+), 113 deletions(-) diff --git a/crates/gpui/src/platform/mac/window.rs b/crates/gpui/src/platform/mac/window.rs index 9f9db753ba..aaa5685895 100644 --- a/crates/gpui/src/platform/mac/window.rs +++ b/crates/gpui/src/platform/mac/window.rs @@ -309,21 +309,11 @@ unsafe fn build_window_class(name: &'static str, superclass: &Class) -> *const C decl.register() } -///Used to track what the IME does when we send it a keystroke. -///This is only used to handle the case where the IME mysteriously -///swallows certain keys. -/// -///Basically a direct copy of the approach that WezTerm uses in: -///github.com/wez/wezterm : d5755f3e : window/src/os/macos/window.rs -enum ImeState { - Continue, - Acted, - None, -} - -struct InsertText { - replacement_range: Option>, - text: String, +#[allow(clippy::enum_variant_names)] +enum ImeInput { + InsertText(String, Option>), + SetMarkedText(String, Option>, Option>), + UnmarkText, } struct MacWindowState { @@ -344,16 +334,14 @@ struct MacWindowState { close_callback: Option>, appearance_changed_callback: Option>, input_handler: Option, - pending_key_down: Option<(KeyDownEvent, Option)>, last_key_equivalent: Option, synthetic_drag_counter: usize, last_fresh_keydown: Option, traffic_light_position: Option>, previous_modifiers_changed_event: Option, // State tracking what the IME did after the last request - ime_state: ImeState, - // Retains the last IME Text - ime_text: Option, + input_during_keydown: Option>, + previous_keydown_inserted_text: Option, external_files_dragged: bool, } @@ -565,7 +553,6 @@ impl MacWindow { close_callback: None, appearance_changed_callback: None, input_handler: None, - pending_key_down: None, last_key_equivalent: None, synthetic_drag_counter: 0, last_fresh_keydown: None, @@ -574,8 +561,8 @@ impl MacWindow { .as_ref() .and_then(|titlebar| titlebar.traffic_light_position), previous_modifiers_changed_event: None, - ime_state: ImeState::None, - ime_text: None, + input_during_keydown: None, + previous_keydown_inserted_text: None, external_files_dragged: false, }))); @@ -1116,6 +1103,15 @@ extern "C" fn handle_key_down(this: &Object, _: Sel, native_event: id) { handle_key_event(this, native_event, false); } +// Things to test if you're modifying this method: +// Brazilian layout: +// - `" space` should type a quote +// - `" backspace` should delete the marked quote +// - `" up` should type the quote, unmark it, and move up one line +// - `" cmd-down` should not leave a marked quote behind (it maybe should dispatch the key though?) +// - `cmd-ctrl-space` and clicking on an emoji should type it +// Czech (QWERTY) layout: +// - in vim mode `option-4` should go to end of line (same as $) extern "C" fn handle_key_event(this: &Object, native_event: id, key_equivalent: bool) -> BOOL { let window_state = unsafe { get_window_state(this) }; let mut lock = window_state.as_ref().lock(); @@ -1123,7 +1119,7 @@ extern "C" fn handle_key_event(this: &Object, native_event: id, key_equivalent: let window_height = lock.content_size().height; let event = unsafe { PlatformInput::from_native(native_event, Some(window_height)) }; - if let Some(PlatformInput::KeyDown(event)) = event { + if let Some(PlatformInput::KeyDown(mut event)) = event { // For certain keystrokes, macOS will first dispatch a "key equivalent" event. // If that event isn't handled, it will then dispatch a "key down" event. GPUI // makes no distinction between these two types of events, so we need to ignore @@ -1143,13 +1139,14 @@ extern "C" fn handle_key_event(this: &Object, native_event: id, key_equivalent: return YES; } } else { - lock.last_fresh_keydown = Some(keydown); + lock.last_fresh_keydown = Some(keydown.clone()); } - lock.pending_key_down = Some((event, None)); + lock.input_during_keydown = Some(SmallVec::new()); drop(lock); // Send the event to the input context for IME handling, unless the `fn` modifier is // being pressed. + // this will call back into `insert_text`, etc. if !fn_modifier { unsafe { let input_context: id = msg_send![this, inputContext]; @@ -1159,48 +1156,63 @@ extern "C" fn handle_key_event(this: &Object, native_event: id, key_equivalent: let mut handled = false; let mut lock = window_state.lock(); - let ime_text = lock.ime_text.clone(); - if let Some((event, insert_text)) = lock.pending_key_down.take() { - let is_held = event.is_held; - if let Some(mut callback) = lock.event_callback.take() { - drop(lock); + let previous_keydown_inserted_text = lock.previous_keydown_inserted_text.take(); + let mut input_during_keydown = lock.input_during_keydown.take().unwrap(); + let mut callback = lock.event_callback.take(); + drop(lock); - let is_composing = - with_input_handler(this, |input_handler| input_handler.marked_text_range()) - .flatten() - .is_some(); + let last_ime = input_during_keydown.pop(); + // on a brazilian keyboard typing `"` and then hitting `up` will cause two IME + // events, one to unmark the quote, and one to send the up arrow. + for ime in input_during_keydown { + send_to_input_handler(this, ime); + } + + let is_composing = + with_input_handler(this, |input_handler| input_handler.marked_text_range()) + .flatten() + .is_some(); + + if let Some(ime) = last_ime { + if let ImeInput::InsertText(text, _) = &ime { if !is_composing { - handled = callback(PlatformInput::KeyDown(event)); - } - - if !handled { - if let Some(insert) = insert_text { - handled = true; - with_input_handler(this, |input_handler| { - input_handler - .replace_text_in_range(insert.replacement_range, &insert.text) - }); - } else if !is_composing && is_held { - if let Some(last_insert_text) = ime_text { - //MacOS IME is a bit funky, and even when you've told it there's nothing to - //inter it will still swallow certain keys (e.g. 'f', 'j') and not others - //(e.g. 'n'). This is a problem for certain kinds of views, like the terminal - with_input_handler(this, |input_handler| { - if input_handler.selected_text_range().is_none() { - handled = true; - input_handler.replace_text_in_range(None, &last_insert_text) - } - }); - } + window_state.lock().previous_keydown_inserted_text = Some(text.clone()); + if let Some(callback) = callback.as_mut() { + event.keystroke.ime_key = Some(text.clone()); + handled = callback(PlatformInput::KeyDown(event)); } } - - window_state.lock().event_callback = Some(callback); } - } else { - handled = true; + + if !handled { + handled = true; + send_to_input_handler(this, ime); + } + } else if !is_composing { + let is_held = event.is_held; + + if let Some(callback) = callback.as_mut() { + handled = callback(PlatformInput::KeyDown(event)); + } + + if !handled && is_held { + if let Some(text) = previous_keydown_inserted_text { + // MacOS IME is a bit funky, and even when you've told it there's nothing to + // enter it will still swallow certain keys (e.g. 'f', 'j') and not others + // (e.g. 'n'). This is a problem for certain kinds of views, like the terminal. + with_input_handler(this, |input_handler| { + if input_handler.selected_text_range().is_none() { + handled = true; + input_handler.replace_text_in_range(None, &text) + } + }); + window_state.lock().previous_keydown_inserted_text = Some(text); + } + } } + window_state.lock().event_callback = callback; + handled as BOOL } else { NO @@ -1615,11 +1627,6 @@ extern "C" fn first_rect_for_character_range( extern "C" fn insert_text(this: &Object, _: Sel, text: id, replacement_range: NSRange) { unsafe { - let window_state = get_window_state(this); - let mut lock = window_state.lock(); - let pending_key_down = lock.pending_key_down.take(); - drop(lock); - let is_attributed_string: BOOL = msg_send![text, isKindOfClass: [class!(NSAttributedString)]]; let text: id = if is_attributed_string == YES { @@ -1631,28 +1638,10 @@ extern "C" fn insert_text(this: &Object, _: Sel, text: id, replacement_range: NS .to_str() .unwrap(); let replacement_range = replacement_range.to_range(); - - window_state.lock().ime_text = Some(text.to_string()); - window_state.lock().ime_state = ImeState::Acted; - - let is_composing = - with_input_handler(this, |input_handler| input_handler.marked_text_range()) - .flatten() - .is_some(); - - if is_composing || text.chars().count() > 1 || pending_key_down.is_none() { - with_input_handler(this, |input_handler| { - input_handler.replace_text_in_range(replacement_range, text) - }); - } else { - let mut pending_key_down = pending_key_down.unwrap(); - pending_key_down.1 = Some(InsertText { - replacement_range, - text: text.to_string(), - }); - pending_key_down.0.keystroke.ime_key = Some(text.to_string()); - window_state.lock().pending_key_down = Some(pending_key_down); - } + send_to_input_handler( + this, + ImeInput::InsertText(text.to_string(), replacement_range), + ); } } @@ -1664,9 +1653,6 @@ extern "C" fn set_marked_text( replacement_range: NSRange, ) { unsafe { - let window_state = get_window_state(this); - window_state.lock().pending_key_down.take(); - let is_attributed_string: BOOL = msg_send![text, isKindOfClass: [class!(NSAttributedString)]]; let text: id = if is_attributed_string == YES { @@ -1680,24 +1666,14 @@ extern "C" fn set_marked_text( .to_str() .unwrap(); - window_state.lock().ime_state = ImeState::Acted; - window_state.lock().ime_text = Some(text.to_string()); - - with_input_handler(this, |input_handler| { - input_handler.replace_and_mark_text_in_range(replacement_range, text, selected_range); - }); + send_to_input_handler( + this, + ImeInput::SetMarkedText(text.to_string(), replacement_range, selected_range), + ); } } - extern "C" fn unmark_text(this: &Object, _: Sel) { - unsafe { - let state = get_window_state(this); - let mut borrow = state.lock(); - borrow.ime_state = ImeState::Acted; - borrow.ime_text.take(); - } - - with_input_handler(this, |input_handler| input_handler.unmark_text()); + send_to_input_handler(this, ImeInput::UnmarkText); } extern "C" fn attributed_substring_for_proposed_range( @@ -1723,14 +1699,7 @@ extern "C" fn attributed_substring_for_proposed_range( .unwrap_or(nil) } -extern "C" fn do_command_by_selector(this: &Object, _: Sel, _: Sel) { - unsafe { - let state = get_window_state(this); - let mut borrow = state.lock(); - borrow.ime_state = ImeState::Continue; - borrow.ime_text.take(); - } -} +extern "C" fn do_command_by_selector(_: &Object, _: Sel, _: Sel) {} extern "C" fn view_did_change_effective_appearance(this: &Object, _: Sel) { unsafe { @@ -1881,6 +1850,30 @@ where } } +fn send_to_input_handler(window: &Object, ime: ImeInput) { + unsafe { + let window_state = get_window_state(window); + let mut lock = window_state.lock(); + if let Some(ime_input) = lock.input_during_keydown.as_mut() { + ime_input.push(ime); + return; + } + if let Some(mut input_handler) = lock.input_handler.take() { + drop(lock); + match ime { + ImeInput::InsertText(text, range) => { + input_handler.replace_text_in_range(range, &text) + } + ImeInput::SetMarkedText(text, range, marked_range) => { + input_handler.replace_and_mark_text_in_range(range, &text, marked_range) + } + ImeInput::UnmarkText => input_handler.unmark_text(), + } + window_state.lock().input_handler = Some(input_handler); + } + } +} + unsafe fn display_id_for_screen(screen: id) -> CGDirectDisplayID { let device_description = NSScreen::deviceDescription(screen); let screen_number_key: id = NSString::alloc(nil).init_str("NSScreenNumber");