[PATCH v3 0/1] MR9260: winemac: Make macdrv_ime_process_key synchronous.
This is a simplification of https://gitlab.winehq.org/wine/wine/-/merge_requests/9246. -- v3: winemac: Make macdrv_ime_process_key synchronous. 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 | 20 ++++++-------------- dlls/winemac.drv/macdrv.h | 1 - dlls/winemac.drv/macdrv_cocoa.h | 9 +-------- dlls/winemac.drv/macdrv_main.c | 4 ---- 6 files changed, 13 insertions(+), 57 deletions(-) diff --git a/dlls/winemac.drv/cocoa_window.m b/dlls/winemac.drv/cocoa_window.m index abfd3e3919c..69613c857fc 100644 --- a/dlls/winemac.drv/cocoa_window.m +++ b/dlls/winemac.drv/cocoa_window.m @@ -4044,12 +4044,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]]) { @@ -4081,14 +4080,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..7c49be7849f 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,10 @@ 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); + NtUserMsgWaitForMultipleObjectsEx(0, NULL, 0, QS_POSTMESSAGE | QS_SENDMESSAGE, 0); + 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
On Mon Nov 3 13:15:41 2025 +0000, Marc-Aurel Zent wrote:
changed this line in [version 3 of the diff](/wine/wine/-/merge_requests/9260/diffs?diff_id=220591&start_sha=af0c055da08e83a187b318526e429674e40dbe9f#8ad61927ca9cd33508c8ef07654574b3db25a865_1143_1143) Getting rid of IM_SET_TEXT is a bit more involved I believe, so for now it is probably best to keep `NtUserMsgWaitForMultipleObjectsEx`.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_120505
On Mon Nov 3 13:17:23 2025 +0000, Marc-Aurel Zent wrote:
Getting rid of IM_SET_TEXT is a bit more involved I believe, so for now it is probably best to keep `NtUserMsgWaitForMultipleObjectsEx`. Based on v3, I'll summarize my opinion.
As mentioned above, because QUERY_IME_CHAR_RECT(firstRectForCharacterRange) is trapped in OnMainThread, query_ime_char_rect() is not called, resulting in a 0.6s timeout (two firstRectForCharacterRange call) on the Japanese keyboard. QUERY_EVENT_NO_PREEMPT_WAIT(33610da6) was necessary because macdrv_ime_process_key used OnMainThreadAsync. This prevented some order changes in handling QUERY_IME_CHAR_RECT and IM_SET_TEXT events handling. Since it is no longer needed in this MR, QUERY_IME_CHAR_RECT can be handled as a QUERY_EVENT. Again, to prevent order changes in handling QUERY_EVENT::QUERY_IME_CHAR_RECT and IM_SET_TEXT events, IM_SET_TEXT also needs to be handled in OnMainThread. This allows us to remove NtUserMsgWaitForMultipleObjectsEx. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_120529
On Mon Nov 3 15:45:47 2025 +0000, Byeongsik Jeon wrote:
Based on v3, I'll summarize my opinion. As mentioned above, because QUERY_IME_CHAR_RECT(firstRectForCharacterRange) is trapped in OnMainThread, query_ime_char_rect() is not called, resulting in a 0.6s timeout (two firstRectForCharacterRange call) on the Japanese keyboard. QUERY_EVENT_NO_PREEMPT_WAIT(33610da6) was necessary because macdrv_ime_process_key used OnMainThreadAsync. This prevented some order changes in handling QUERY_IME_CHAR_RECT and IM_SET_TEXT events handling. Since it is no longer needed in this MR, QUERY_IME_CHAR_RECT can be handled as a QUERY_EVENT. Again, to prevent order changes in handling QUERY_EVENT::QUERY_IME_CHAR_RECT and IM_SET_TEXT events, IM_SET_TEXT also needs to be handled in OnMainThread. This allows us to remove NtUserMsgWaitForMultipleObjectsEx. I am currently working getting rid of all QUERY_EVENT and QUERY_EVENT_NO_PREEMPT_WAIT logic, and also by extension IM_SET_TEXT.
We unfortunately can't just do IM_SET_TEXT on the main thread, due to it not being the wine thread, and IM_SET_TEXT also needs to be handled when the mouse interacts with the native IME UI, so it is not enough to refactor it to be coupled together with macdrv_ime_process_key. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_120536
On Mon Nov 3 16:21:35 2025 +0000, Marc-Aurel Zent wrote:
I am currently working getting rid of all QUERY_EVENT and QUERY_EVENT_NO_PREEMPT_WAIT logic, and also by extension IM_SET_TEXT. We unfortunately can't just do IM_SET_TEXT on the main thread, due to it not being the wine thread, and IM_SET_TEXT also needs to be handled when the mouse interacts with the native IME UI, so it is not enough to refactor it to be coupled together with macdrv_ime_process_key. [0001-t1.patch](/uploads/58ce40094a04c13e1e6912432df18c61/0001-t1.patch)
How about this? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_120537
and IM_SET_TEXT also needs to be handled when the mouse interacts with the native IME UI
Since this is abstracted away by methods like setMarkedText or insertText, it seems like we don't need to worry about it, right? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_120559
On Mon Nov 3 17:16:13 2025 +0000, Byeongsik Jeon wrote:
and IM_SET_TEXT also needs to be handled when the mouse interacts with the native IME UI Since this is abstracted away by methods like setMarkedText or insertText, it seems like we don't need to worry about it, right? If you're referring to situations like clicking a Wine window when the IME is in composition mode or when the macdrv native IME UI is active, I believe that's a different issue from this MR. This relates to the call to ImmNotifyIME( .. CPS_COMPLETE ) in the WM_LBUTTONDOWN,etc messages.
However, I'll investigate whether it might have some influence. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_120567
We unfortunately can't just do IM_SET_TEXT on the main thread, due to it not being the wine thread, and IM_SET_TEXT also needs to be handled when the mouse interacts with the native IME UI, so it is not enough to refactor it to be coupled together with macdrv_ime_process_key.
Of course this is still not completely race-free (but the previous version also wasn't), since the thread running `- (void) completeText:(NSString*)text` and generating this event can be preempted at any time.
I have reproduced it. Since this occurs independently of macdrv_ime_process_key, we should not remove the IM_SET_TEXT event. Since it's not an ImeProcessKey driven, it call `NtUserPostMessage(hwnd, WM_WINE_IME_NOTIFY, IMN_WINE_SET_COMP_STRING, id)` in win32u/imm.c:post_ime_update. I tried reproducing the same situation on Windows, and it shows a similar pattern. The intermediate steps might differ slightly, but eventually, a WM_IME_COMPOSITION message is sent. In my opinion, this is handled by the next peek_message in the event loop, and there's no need to worry about it in macdrv_ime_process_key. We just need to keep the IM_SET_TEXT event. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_120592
participants (3)
-
Byeongsik Jeon (＠bsjeon) -
Marc-Aurel Zent
-
Marc-Aurel Zent (＠mzent)