Fix IME delete misalignment

[amtoaer]

I noticed that when deleting the last letter in the IME, an additional character in the editor is also deleted.

I looked into it and figured out that the only difference between deleting the last letter and others is that the text was empty. After that, I noticed a special logic for text.is_empty(). Everything worked fine after I removed it.

I'm not sure what this.unmark_text(cx) is for, so this change might break other things. I would appreciate it if someone could provide some guidance.

Before(from the linked issue):

Screen.Recording.2024-06-11.at.02.40.18.mov

After:

Closes #12862.

Release Notes:

• Fixed IME delete misalignment (#12862).

[JunkuiZhang]

This implementation might be wrong, the bug was introduced in #12702 , and this PR modified some other codes, thoughts?

[amtoaer]

Get it. I'll revert that PR to see what's going on. I guess the difference is how empty text is handled. 🤔

[d1y]

Everything worked fine after I removed it.

实测 shift 切换 中/英 还是会错位~

Screen.Recording.2024-06-21.at.15.44.09.mov

[mrnugget]

FWIW here are some tests to run through with all the IME changes:

zed/crates/gpui/src/platform/mac/window.rs

Lines 1175 to 1199 in 4ef7ba2

	// 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
	// - `alt-t` should open the tasks menu
	// - In vim mode, this keybinding should work:
	// ```
	// {
	// "context": "Editor && vim_mode == insert",
	// "bindings": {"j j": "vim::NormalBefore"}
	// }
	// ```
	// and typing 'j k' in insert mode with this keybinding should insert the two characters
	// Brazilian layout:
	// - `" space` should create an unmarked quote
	// - `" backspace` should delete the marked quote
	// - `" 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
	// - NOTE: The current implementation does not move the selection 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:
	// - type `a i left down up enter enter` should create an unmarked text "愛"

These things all worked with your PR from a few hours ago. So if there's something else that it broke, it would be great to add that to this list.

[amtoaer]

Thank you for your reply. As Junkui says, this bug was introduced by #12702. I'm trying to figure out which part of that PR causes this issue. Once I find and fix it, I'll add some related tests.

[amtoaer]

Applying this simple patch to 29d29f5a9 (the last commit before that pull request) leads to this issue.

diff --git a/crates/gpui/src/platform/mac/window.rs b/crates/gpui/src/platform/mac/window.rs
index 758af61cd..d23910384 100644
--- a/crates/gpui/src/platform/mac/window.rs
+++ b/crates/gpui/src/platform/mac/window.rs
@@ -311,6 +311,7 @@ unsafe fn build_window_class(name: &'static str, superclass: &Class) -> *const C
 }
 
 #[allow(clippy::enum_variant_names)]
+#[derive(Clone)]
 enum ImeInput {
     InsertText(String, Option<Range<usize>>),
     SetMarkedText(String, Option<Range<usize>>, Option<Range<usize>>),
@@ -1926,20 +1927,25 @@ 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 {
+            match ime.clone() {
                 ImeInput::InsertText(text, range) => {
+                    if let Some(ime_input) = lock.input_during_keydown.as_mut() {
+                        ime_input.push(ime);
+                        lock.input_handler = Some(input_handler);
+                        return;
+                    }
+                    drop(lock);
                     input_handler.replace_text_in_range(range, &text)
                 }
                 ImeInput::SetMarkedText(text, range, marked_range) => {
+                    drop(lock);
                     input_handler.replace_and_mark_text_in_range(range, &text, marked_range)
                 }
-                ImeInput::UnmarkText => input_handler.unmark_text(),
+                ImeInput::UnmarkText => {
+                    drop(lock);
+                    input_handler.unmark_text()
+                }
             }
             window_state.lock().input_handler = Some(input_handler);
         }

I'm not too familiar with how IME works and how keys are handled, so this code is somewhat difficult for me to understand. I can see that the only change this patch makes is to let buffering work only for InsertText.

According to my understanding, before the change, send_to_input_handler would only be executed if lock.input_during_keydown was None. That is, after let mut input_during_keydown = lock.input_during_keydown.take().unwrap();. This limitation is removed for ImeInput::SetMarkedText and ImeInput::UnmarkText after the patch. This might change the order of code execution and affect the IME's behavior, but I'm not quite sure.

These are what I've found for now. I'll continue to try to figure it out tomorrow if I have time.

[amtoaer]

I will close this PR as #13385 is more correct and reasonable.