[PATCH v4 0/2] MR9260: winemac: Make macdrv_ime_process_key synchronous.
This is a simplification of https://gitlab.winehq.org/wine/wine/-/merge_requests/9246. -- v4: winemac: Remove QUERY_IME_CHAR_RECT and directly get ime_composition_rect. 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
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- dlls/winemac.drv/cocoa_window.m | 20 ++++-------------- dlls/winemac.drv/event.c | 37 +++++---------------------------- dlls/winemac.drv/macdrv_cocoa.h | 9 +++----- 3 files changed, 12 insertions(+), 54 deletions(-) diff --git a/dlls/winemac.drv/cocoa_window.m b/dlls/winemac.drv/cocoa_window.m index 69613c857fc..2ecf5183e94 100644 --- a/dlls/winemac.drv/cocoa_window.m +++ b/dlls/winemac.drv/cocoa_window.m @@ -920,28 +920,16 @@ - (NSArray*) validAttributesForMarkedText - (NSRect) firstRectForCharacterRange:(NSRange)aRange actualRange:(NSRangePointer)actualRange { - macdrv_query* query; WineWindow* window = (WineWindow*)[self window]; NSRect ret; aRange = NSIntersectionRange(aRange, NSMakeRange(0, [markedText length])); - query = macdrv_create_query(); - query->type = QUERY_IME_CHAR_RECT; - query->window = (macdrv_window)[window retain]; - query->ime_char_rect.himc = [window himc]; - query->ime_char_rect.range = CFRangeMake(aRange.location, aRange.length); + pthread_mutex_lock(&ime_composition_rect_mutex); + ret = NSRectFromCGRect(cgrect_mac_from_win(ime_composition_rect)); + pthread_mutex_unlock(&ime_composition_rect_mutex); - if ([window.queue query:query timeout:0.3 flags:WineQueryNoPreemptWait]) - { - aRange = NSMakeRange(query->ime_char_rect.range.location, query->ime_char_rect.range.length); - ret = NSRectFromCGRect(cgrect_mac_from_win(query->ime_char_rect.rect)); - [[WineApplicationController sharedController] flipRect:&ret]; - } - else - ret = NSMakeRect(100, 100, aRange.length ? 1 : 0, 12); - - macdrv_release_query(query); + [[WineApplicationController sharedController] flipRect:&ret]; if (actualRange) *actualRange = aRange; diff --git a/dlls/winemac.drv/event.c b/dlls/winemac.drv/event.c index 05f2eca50e3..ed288ae29c0 100644 --- a/dlls/winemac.drv/event.c +++ b/dlls/winemac.drv/event.c @@ -36,8 +36,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(event); WINE_DECLARE_DEBUG_CHANNEL(imm); -static pthread_mutex_t ime_mutex = PTHREAD_MUTEX_INITIALIZER; -static RECT ime_composition_rect; +pthread_mutex_t ime_composition_rect_mutex = PTHREAD_MUTEX_INITIALIZER; +CGRect ime_composition_rect; /* return the name of an Mac event */ static const char *dbgstr_event(int type) @@ -290,38 +290,15 @@ static BOOL query_drag_drop_drag(macdrv_query *query) } -/************************************************************************** - * query_ime_char_rect - */ -BOOL query_ime_char_rect(macdrv_query* query) -{ - HWND hwnd = macdrv_get_window_hwnd(query->window); - void *himc = query->ime_char_rect.himc; - CFRange *range = &query->ime_char_rect.range; - - TRACE_(imm)("win %p/%p himc %p range %ld-%ld\n", hwnd, query->window, himc, range->location, - range->length); - - pthread_mutex_lock(&ime_mutex); - query->ime_char_rect.rect = cgrect_from_rect(ime_composition_rect); - pthread_mutex_unlock(&ime_mutex); - - TRACE_(imm)(" -> range %ld-%ld rect %s\n", range->location, - range->length, wine_dbgstr_cgrect(query->ime_char_rect.rect)); - - return TRUE; -} - - /*********************************************************************** * SetIMECompositionRect (MACDRV.@) */ BOOL macdrv_SetIMECompositionRect(HWND hwnd, RECT rect) { TRACE("hwnd %p, rect %s\n", hwnd, wine_dbgstr_rect(&rect)); - pthread_mutex_lock(&ime_mutex); - ime_composition_rect = rect; - pthread_mutex_unlock(&ime_mutex); + pthread_mutex_lock(&ime_composition_rect_mutex); + ime_composition_rect = cgrect_from_rect(rect); + pthread_mutex_unlock(&ime_composition_rect_mutex); return TRUE; } @@ -364,10 +341,6 @@ static void macdrv_query_event(HWND hwnd, const macdrv_event *event) TRACE("QUERY_DRAG_DROP_DROP\n"); success = query_drag_drop_drop(query); break; - case QUERY_IME_CHAR_RECT: - TRACE("QUERY_IME_CHAR_RECT\n"); - success = query_ime_char_rect(query); - break; case QUERY_PASTEBOARD_DATA: TRACE("QUERY_PASTEBOARD_DATA\n"); success = query_pasteboard_data(hwnd, query->pasteboard_data.type); diff --git a/dlls/winemac.drv/macdrv_cocoa.h b/dlls/winemac.drv/macdrv_cocoa.h index e6999670cb5..d4d1dde6bda 100644 --- a/dlls/winemac.drv/macdrv_cocoa.h +++ b/dlls/winemac.drv/macdrv_cocoa.h @@ -415,7 +415,6 @@ extern int macdrv_set_display_mode(const struct macdrv_display* display, QUERY_DRAG_DROP_LEAVE, QUERY_DRAG_DROP_DRAG, QUERY_DRAG_DROP_DROP, - QUERY_IME_CHAR_RECT, QUERY_PASTEBOARD_DATA, QUERY_RESIZE_SIZE, QUERY_RESIZE_START, @@ -436,11 +435,6 @@ extern int macdrv_set_display_mode(const struct macdrv_display* display, uint32_t ops; CFTypeRef pasteboard; } drag_drop; - struct { - void *himc; - CFRange range; - CGRect rect; - } ime_char_rect; struct { CFStringRef type; } pasteboard_data; @@ -578,6 +572,9 @@ extern void macdrv_get_input_source_info(CFDataRef* uchr,CGEventSourceKeyboardTy extern void macdrv_set_status_item_image(macdrv_status_item s, CGImageRef cgimage); extern void macdrv_set_status_item_tooltip(macdrv_status_item s, CFStringRef cftip); +/* ime */ +extern pthread_mutex_t ime_composition_rect_mutex; +extern CGRect ime_composition_rect; extern void macdrv_clear_ime_text(void); #endif /* __WINE_MACDRV_COCOA_H */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9260
On Mon Nov 3 21:58:28 2025 +0000, Byeongsik Jeon wrote:
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. How about the current version? Does that work for you?
We don't really have a need for a ime query event anyways, since that does not call into any Wine functions (and I assume this used to be the case before one of the many refactors). We can drop `QUERY_EVENT_NO_PREEMPT_WAIT` for sure now (since it is not used by any event anymore) and all the other queries as well soon with what I am working on currently. This is actually worse than I originally thought, since we are quite often blocking the UI main thread for extended periods of time (up to 0.3s), depending on where and how fast we can drain these events. And as @rbernon mentioned above, we should also not be doing so outside of `NtUserMsgWaitForMultipleObjectsEx`, which macdriver currently does with that mechanism when dispatching to the main thread from any thread. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_120733
On Tue Nov 4 16:56:12 2025 +0000, Marc-Aurel Zent wrote:
How about the current version? Does that work for you? We don't really have a need for a ime query event anyways, since that does not call into any Wine functions (and I assume this used to be the case before one of the many refactors). We can drop `QUERY_EVENT_NO_PREEMPT_WAIT` for sure now (since it is not used by any event anymore) and all the other queries as well soon with what I am working on currently. This is actually worse than I originally thought, since we are quite often blocking the UI main thread for extended periods of time (up to 0.3s), depending on where and how fast we can drain these events. And as @rbernon mentioned above, we should also not be doing so outside of `NtUserMsgWaitForMultipleObjectsEx`, which macdriver currently does with that mechanism when dispatching to the main thread from any thread. It looks good to me. I could also feel improved responsiveness under certain conditions.
In the old code, query_ime_char_rect called ImmRequestMessage. Some programs require this, but implementing it is tricky due to the current PE-UNIX separation, and it could potentially reduce IME responsiveness. Since it can be compensated for by calling SetIMECompositionRect in functions like NtUserSetCaretPos, it should be fine. Regarding SetIMECompositionRect, Wine currently needs some minor adjustments, so I'll proceed with a separate MR. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_120813
Are there still any concerns with the MR in its current form? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_120912
This merge request was approved by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260
This merge request was approved by Byeongsik Jeon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260
participants (4)
-
Byeongsik Jeon (＠bsjeon) -
Marc-Aurel Zent
-
Marc-Aurel Zent (＠mzent)
-
Rémi Bernon