Fix a bug where the IME pre-edit would desync from Zed (#12651)

fixes #11829 

In https://github.com/zed-industries/zed/pull/7494, we introduced IME
event buffering, so that we could preempt the IME with a keystroke event
in some cases. However, this caused a desynchronization bug in long
multi-step IME composition, such as the pre-edit used in the Japanese
Romaji keyboard (and other languages). We found that this was due to the
IME issuing actions, and then immediately querying the editor's state
before we had applied those actions. Therefore, this PR removes IME
action buffering.

We have tested all of the cases in the `handle_key_event` documentation
and added a few of our own.

Release Notes:

- Fixed an issue where the IME pre-edit could desynchronize from the
editor on macOS
([#11829](https://github.com/zed-industries/zed/pull/12651))

---------

Co-authored-by: Jan Solanti <jhs@psonet.com>
This commit is contained in:
Mikayla Maki 2024-06-04 12:17:44 -07:00 committed by GitHub
parent 62e790074c
commit 1a0708f28c
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194

View File

@ -310,6 +310,7 @@ unsafe fn build_window_class(name: &'static str, superclass: &Class) -> *const C
decl.register()
}
#[derive(Debug, Clone)]
#[allow(clippy::enum_variant_names)]
enum ImeInput {
InsertText(String, Option<Range<usize>>),
@ -339,7 +340,7 @@ struct MacWindowState {
traffic_light_position: Option<Point<Pixels>>,
previous_modifiers_changed_event: Option<PlatformInput>,
// State tracking what the IME did after the last request
input_during_keydown: Option<SmallVec<[ImeInput; 1]>>,
last_ime_action: Option<ImeInput>,
previous_keydown_inserted_text: Option<String>,
external_files_dragged: bool,
// Whether the next left-mouse click is also the focusing click.
@ -635,7 +636,7 @@ impl MacWindow {
.as_ref()
.and_then(|titlebar| titlebar.traffic_light_position),
previous_modifiers_changed_event: None,
input_during_keydown: None,
last_ime_action: None,
previous_keydown_inserted_text: None,
external_files_dragged: false,
first_mouse: false,
@ -1190,14 +1191,22 @@ extern "C" fn handle_key_down(this: &Object, _: Sel, native_event: id) {
}
// Things to test if you're modifying this method:
// U.S. layout:
// - The IME consumes characters like 'j' and 'k', which makes paging through `less` in
// the terminal behave incorrectly by default. This behavior should be patched by our
// IME integration
// Brazilian layout:
// - `" space` should type a quote
// - `" space` should create an unmarked 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?)
// - `" up` should insert a quote, unmark it, and move up one line
// - `" cmd-down` should insert a quote, unmark it, and move to the end of the file
// - `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 $)
// Japanese (Romaji) layout:
// - Triggering the IME composer (e.g. via typing 'a i' and then the left key), and then selecting
// results of different length (e.g. kana -> kanji -> emoji -> back to kanji via the up and down keys)
// should maintain the composing state in the editor
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();
@ -1227,12 +1236,12 @@ extern "C" fn handle_key_event(this: &Object, native_event: id, key_equivalent:
} else {
lock.last_fresh_keydown = Some(keydown.clone());
}
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.
// being pressed. This will call back into other functions like `insert_text`, etc.
// Note that the IME expects it's actions to be applied immediately, and buffering them
// can break pre-edit
if !fn_modifier {
unsafe {
let input_context: id = msg_send![this, inputContext];
@ -1243,17 +1252,10 @@ extern "C" fn handle_key_event(this: &Object, native_event: id, key_equivalent:
let mut handled = false;
let mut lock = window_state.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 last_ime = lock.last_ime_action.take();
let mut callback = lock.event_callback.take();
drop(lock);
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()
@ -1265,15 +1267,12 @@ extern "C" fn handle_key_event(this: &Object, native_event: id, key_equivalent:
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)).propagate;
let _ = callback(PlatformInput::KeyDown(event));
}
}
}
if !handled {
handled = true;
send_to_input_handler(this, ime);
}
handled = true;
} else if !is_composing {
let is_held = event.is_held;
@ -1926,10 +1925,8 @@ 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;
}
lock.last_ime_action = Some(ime.clone());
if let Some(mut input_handler) = lock.input_handler.take() {
drop(lock);
match ime {