[PATCH 0/1] MR9260: winemac: Make macdrv_ime_process_key synchronous.
This is a simplification of https://gitlab.winehq.org/wine/wine/-/merge_requests/9246. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- dlls/winemac.drv/cocoa_window.m | 18 ++++++------------ dlls/winemac.drv/event.c | 18 ------------------ dlls/winemac.drv/keyboard.c | 19 +++++-------------- dlls/winemac.drv/macdrv.h | 1 - dlls/winemac.drv/macdrv_cocoa.h | 9 +-------- dlls/winemac.drv/macdrv_main.c | 4 ---- 6 files changed, 12 insertions(+), 57 deletions(-) diff --git a/dlls/winemac.drv/cocoa_window.m b/dlls/winemac.drv/cocoa_window.m index 2dd7d180c24..7728857a479 100644 --- a/dlls/winemac.drv/cocoa_window.m +++ b/dlls/winemac.drv/cocoa_window.m @@ -4040,12 +4040,11 @@ uint32_t macdrv_window_background_color(void) * processed by input sources (AKA IMEs). This is only called when there is an * active non-keyboard input source. */ -void macdrv_ime_process_key(int keyc, unsigned int flags, int repeat, void *himc, - int *done, void *ime_done_event) +int macdrv_ime_process_key(int keyc, unsigned int flags, int repeat, void *himc) { - OnMainThreadAsync(^{ - BOOL ret; - macdrv_event* event; + __block BOOL ret; + + OnMainThread(^{ WineWindow* window = (WineWindow*)[NSApp keyWindow]; if (![window isKindOfClass:[WineWindow class]]) { @@ -4077,14 +4076,9 @@ void macdrv_ime_process_key(int keyc, unsigned int flags, int repeat, void *himc } else ret = FALSE; - - event = macdrv_create_event(SENT_TEXT_INPUT, window); - event->sent_text_input.handled = ret; - event->sent_text_input.done = done; - event->sent_text_input.ime_done_event = ime_done_event; - [[window queue] postEvent:event]; - macdrv_release_event(event); }); + + return (int)ret; } void macdrv_clear_ime_text(void) diff --git a/dlls/winemac.drv/event.c b/dlls/winemac.drv/event.c index 6841fd3603e..05f2eca50e3 100644 --- a/dlls/winemac.drv/event.c +++ b/dlls/winemac.drv/event.c @@ -61,7 +61,6 @@ static const char *dbgstr_event(int type) "QUERY_EVENT_NO_PREEMPT_WAIT", "REASSERT_WINDOW_POSITION", "RELEASE_CAPTURE", - "SENT_TEXT_INPUT", "STATUS_ITEM_MOUSE_BUTTON", "STATUS_ITEM_MOUSE_MOVE", "WINDOW_BROUGHT_FORWARD", @@ -138,7 +137,6 @@ static macdrv_event_mask get_event_mask(DWORD mask) event_mask |= event_mask_for_type(QUERY_EVENT_NO_PREEMPT_WAIT); event_mask |= event_mask_for_type(REASSERT_WINDOW_POSITION); event_mask |= event_mask_for_type(RELEASE_CAPTURE); - event_mask |= event_mask_for_type(SENT_TEXT_INPUT); event_mask |= event_mask_for_type(WINDOW_BROUGHT_FORWARD); event_mask |= event_mask_for_type(WINDOW_CLOSE_REQUESTED); event_mask |= event_mask_for_type(WINDOW_DRAG_BEGIN); @@ -185,19 +183,6 @@ static void macdrv_im_set_text(const macdrv_event *event) free(text); } -/*********************************************************************** - * macdrv_sent_text_input - */ -static void macdrv_sent_text_input(const macdrv_event *event) -{ - HANDLE ime_done_event = event->sent_text_input.ime_done_event; - - TRACE_(imm)("handled: %s\n", event->sent_text_input.handled ? "TRUE" : "FALSE"); - - *event->sent_text_input.done = event->sent_text_input.handled ? 1 : -1; - if (ime_done_event) NtSetEvent(ime_done_event, NULL); -} - /************************************************************************** * drag_operations_to_dropeffects @@ -475,9 +460,6 @@ void macdrv_handle_event(const macdrv_event *event) case RELEASE_CAPTURE: macdrv_release_capture(hwnd, event); break; - case SENT_TEXT_INPUT: - macdrv_sent_text_input(event); - break; case STATUS_ITEM_MOUSE_BUTTON: macdrv_status_item_mouse_button(event); break; diff --git a/dlls/winemac.drv/keyboard.c b/dlls/winemac.drv/keyboard.c index 6c615d10e3d..201a84f2fad 100644 --- a/dlls/winemac.drv/keyboard.c +++ b/dlls/winemac.drv/keyboard.c @@ -1089,7 +1089,8 @@ UINT macdrv_ImeProcessKey(HIMC himc, UINT wparam, UINT lparam, const BYTE *key_s WORD scan = HIWORD(lparam) & 0x1ff, vkey = LOWORD(wparam); BOOL repeat = !!(lparam >> 30), pressed = !(lparam >> 31); unsigned int flags; - int keyc, done = 0; + int keyc; + UINT ret; TRACE("himc %p, scan %#x, vkey %#x, repeat %u, pressed %u\n", himc, scan, vkey, repeat, pressed); @@ -1138,19 +1139,9 @@ UINT macdrv_ImeProcessKey(HIMC himc, UINT wparam, UINT lparam, const BYTE *key_s if (keyc >= ARRAY_SIZE(thread_data->keyc2vkey)) return 0; TRACE("flags 0x%08x keyc 0x%04x\n", flags, keyc); - - if (!thread_data->ime_done_event) - { - NTSTATUS status; - status = NtCreateEvent(&thread_data->ime_done_event, EVENT_ALL_ACCESS, NULL, - SynchronizationEvent, FALSE); - if (status != STATUS_SUCCESS) ERR("NtCreateEvent call failed.\n"); - } - - macdrv_ime_process_key(keyc, flags, repeat, himc, &done, thread_data->ime_done_event); - NtUserMsgWaitForMultipleObjectsEx(1, &thread_data->ime_done_event, INFINITE, 0, 0); - - return done > 0; + ret = (UINT)macdrv_ime_process_key(keyc, flags, repeat, himc); + TRACE("returning %u\n", ret); + return ret; } diff --git a/dlls/winemac.drv/macdrv.h b/dlls/winemac.drv/macdrv.h index 5b9515f086f..22e96fa1129 100644 --- a/dlls/winemac.drv/macdrv.h +++ b/dlls/winemac.drv/macdrv.h @@ -112,7 +112,6 @@ static inline RECT rect_from_cgrect(CGRect cgrect) HKL active_keyboard_layout; WORD keyc2vkey[128]; WORD keyc2scan[128]; - HANDLE ime_done_event; }; extern struct macdrv_thread_data *macdrv_init_thread_data(void); diff --git a/dlls/winemac.drv/macdrv_cocoa.h b/dlls/winemac.drv/macdrv_cocoa.h index bd6af9ed25e..e6999670cb5 100644 --- a/dlls/winemac.drv/macdrv_cocoa.h +++ b/dlls/winemac.drv/macdrv_cocoa.h @@ -291,7 +291,6 @@ extern int macdrv_set_display_mode(const struct macdrv_display* display, QUERY_EVENT_NO_PREEMPT_WAIT, REASSERT_WINDOW_POSITION, RELEASE_CAPTURE, - SENT_TEXT_INPUT, STATUS_ITEM_MOUSE_BUTTON, STATUS_ITEM_MOUSE_MOVE, WINDOW_BROUGHT_FORWARD, @@ -378,11 +377,6 @@ extern int macdrv_set_display_mode(const struct macdrv_display* display, struct { struct macdrv_query *query; } query_event; - struct { - int handled; - int *done; - void *ime_done_event; - } sent_text_input; struct { macdrv_status_item item; int button; @@ -546,8 +540,7 @@ extern void macdrv_order_cocoa_window(macdrv_window w, macdrv_window prev, extern int macdrv_get_view_backing_size(macdrv_view v, int backing_size[2]); extern void macdrv_set_view_backing_size(macdrv_view v, const int backing_size[2]); extern uint32_t macdrv_window_background_color(void); -extern void macdrv_ime_process_key(int keyc, unsigned int flags, int repeat, void *data, - int *done, void *ime_done_event); +extern int macdrv_ime_process_key(int keyc, unsigned int flags, int repeat, void *data); extern int macdrv_is_any_wine_window_visible(void); diff --git a/dlls/winemac.drv/macdrv_main.c b/dlls/winemac.drv/macdrv_main.c index f5e2b5e08d2..20295d1c6ee 100644 --- a/dlls/winemac.drv/macdrv_main.c +++ b/dlls/winemac.drv/macdrv_main.c @@ -464,8 +464,6 @@ void macdrv_ThreadDetach(void) macdrv_destroy_event_queue(data->queue); if (data->keyboard_layout_uchr) CFRelease(data->keyboard_layout_uchr); - if (data->ime_done_event) - NtClose(data->ime_done_event); free(data); /* clear data in case we get re-entered from user32 before the thread is truly dead */ NtUserGetThreadInfo()->driver_data = 0; @@ -525,8 +523,6 @@ struct macdrv_thread_data *macdrv_init_thread_data(void) NtTerminateProcess(0, 1); } - data->ime_done_event = NULL; - macdrv_get_input_source_info(&data->keyboard_layout_uchr, &data->keyboard_type, &data->iso_keyboard, &input_source); data->active_keyboard_layout = macdrv_get_hkl_from_source(input_source); CFRelease(input_source); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9260
Well I don't really know why it was like that in the first place, so I am just going to trust that it works. Also, do you know what the `commandDone` is for? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_119439
On Thu Oct 23 14:20:47 2025 +0000, Rémi Bernon wrote:
Well I don't really know why it was like that in the first place, so I am just going to trust that it works. Also, do you know what the `commandDone` is for? Worked fine for me and I couldn't see any reason why it should matter FWIW.
By the looks of it `commandDone` seems to be from the original IME implementation and `doCommandBySelector:` (which is also deprecated since macOS 10.6, maybe we should implement this differently) eats up the selector for a key binding command in the `NSTextInput`, and sets `commandDone` to `TRUE`, indicating that this event was not handled by the native IME system and should've been passed up the responder chain. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_119456
On Thu Oct 23 14:20:47 2025 +0000, Marc-Aurel Zent wrote:
Worked fine for me and I couldn't see any reason why it should matter FWIW. By the looks of it `commandDone` seems to be from the original IME implementation and `doCommandBySelector:` (which is also deprecated since macOS 10.6, maybe we should implement this differently) eats up the selector for a key binding command in the `NSTextInput`, and sets `commandDone` to `TRUE`, indicating that this event was not handled by the native IME system and should've been passed up the responder chain. Wow! Honestly, I had the same idea, but I didn't know about OnMainThread. Since it makes ime processing responsiveness so much better, I hope it continues.
When commandDone is set to TRUE in doCommandBySelector:, macdrv_ime_process_key returns FALSE even if NSTextInputContext::handleEvent's return value is TRUE. This is necessary so that the key_assumed value is set correctly in win32u's ime_driver_call and the WM_IME_KEYDOWN message is generated. Modifications are needed for completeText:, setMarkedText:. Currently, after macdrv_ImeProcessKey returns, macdrv_im_set_text is called. IM_SET_TEXT is also unnecessary. When I tested it before, completeText:, setMarkedText: were called before NSTextInputContext::handleEvent returned. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_119483
On Thu Oct 23 19:04:18 2025 +0000, Byeongsik Jeon wrote:
Wow! Honestly, I had the same idea, but I didn't know about OnMainThread. Since it makes ime processing responsiveness so much better, I hope it continues. When commandDone is set to TRUE in doCommandBySelector:, macdrv_ime_process_key returns FALSE even if NSTextInputContext::handleEvent's return value is TRUE. This is necessary so that the key_assumed value is set correctly in win32u's ime_driver_call and the WM_IME_KEYDOWN message is generated. Modifications are needed for completeText:, setMarkedText:. Currently, after macdrv_ImeProcessKey returns, macdrv_im_set_text is called. IM_SET_TEXT is also unnecessary. When I tested it before, completeText:, setMarkedText: were called before NSTextInputContext::handleEvent returned. I looked inside the OnMainThread, and perhaps there might be an error in my reply. I'll describe the problematic situation:
1. Add the Korean keyboard ‘Korean 2-Set Keyboard’. 1. Run notepad. 1. Move focus to notepad. 1. Select ‘Korean 2-Set Keyboard’. 1. Currently, since comctl32 edit control does not handle WM_INPUTLANGCHANGE, move focus to another window and then focus back to notepad. 1. Type ‘r’ key. 1. Type ‘RETURN’ key. 1. The order of ‘ㄱ’ and ‘RETURN’ gets reversed. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_119494
On Thu Oct 23 19:57:46 2025 +0000, Byeongsik Jeon wrote:
I looked inside the OnMainThread, and perhaps there might be an error in my reply. I'll describe the problematic situation: 1. Add the Korean keyboard ‘Korean 2-Set Keyboard’. 2. Run notepad. 3. Move focus to notepad. 4. Select ‘Korean 2-Set Keyboard’. 5. Currently, since comctl32 edit control does not handle WM_INPUTLANGCHANGE, move focus to another window and then focus back to notepad. 6. Type ‘r’ key. 7. Type ‘RETURN’ key. 8. The order of ‘ㄱ’ and ‘RETURN’ gets reversed. Update: You might need to change the Notepad font to ‘Apple SD Gothic Neo’. Thanks for testing, I will see if I can reproduce this and see if there is a fix for this (it does sound like a missing lock somewhere though).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_119538
On Fri Oct 24 07:09:00 2025 +0000, Marc-Aurel Zent wrote:
Thanks for testing, I will see if I can reproduce this and see if there is a fix for this (it does sound like a missing lock somewhere though). [poc_drop_IM_SET_TEXT.diff](/uploads/0d81551263091d74a86db489574454fb/poc_drop_IM_SET_TEXT.diff)
In addition to this MR, I tested whether it's possible to drop the IM_SET_TEXT code. It's a proof-of-concept patch and needs cleanup. It seems to work well, including the issue posted above. However, I don't know why CFStringAppend() isn't working. I believe this is due to my lack of programming knowledge and is unrelated to the proof-of-concept. This is not to say it's the only way. Please consider it for reference purposes only. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_119540
On Fri Oct 24 08:14:03 2025 +0000, Byeongsik Jeon wrote:
[poc_drop_IM_SET_TEXT.diff](/uploads/0d81551263091d74a86db489574454fb/poc_drop_IM_SET_TEXT.diff) In addition to this MR, I tested whether it's possible to drop the IM_SET_TEXT code. It's a proof-of-concept patch and needs cleanup. It seems to work well, including the issue posted above. However, I don't know why CFStringAppend() isn't working. I believe this is due to my lack of programming knowledge and is unrelated to the proof-of-concept. This is not to say it's the only way. Please consider it for reference purposes only. Along with the issue posted above, I found another problem in this MR.
On the Japanese Romanji keyboard, the first key press responds slowly. It works fine once the IME enters composition mode. Alternatively, you can observe the slow response by simply holding down the `RETURN` key. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_119541
participants (4)
-
Byeongsik Jeon (＠bsjeon) -
Marc-Aurel Zent
-
Marc-Aurel Zent (＠mzent)
-
Rémi Bernon