[PATCH v3 0/5] MR2698: winex11: Avoid breaking XIM XFilterEvent / XmbLookupString sequence.
-- v3: winex11: Make sure HIMC is opened before sending IME updates. winex11: Avoid breaking XIM XFilterEvent / XmbLookupString sequence. winex11: Call (get|free)_event_data within call_event_handler. winex11: Stop merging MotionNotify events. winex11: Stop merging RawMotion events.
From: Rémi Bernon rbernon@codeweavers.com
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55027 --- dlls/winex11.drv/event.c | 52 ---------------------------------------- 1 file changed, 52 deletions(-)
diff --git a/dlls/winex11.drv/event.c b/dlls/winex11.drv/event.c index 55709a0bc5f..ce638fa5637 100644 --- a/dlls/winex11.drv/event.c +++ b/dlls/winex11.drv/event.c @@ -264,46 +264,6 @@ enum event_merge_action MERGE_IGNORE /* ignore the new event, keep the old one */ };
-/*********************************************************************** - * merge_raw_motion_events - */ -#ifdef HAVE_X11_EXTENSIONS_XINPUT2_H -static enum event_merge_action merge_raw_motion_events( XIRawEvent *prev, XIRawEvent *next ) -{ - int i, j, k; - unsigned char mask; - - if (!prev->valuators.mask_len) return MERGE_HANDLE; - if (!next->valuators.mask_len) return MERGE_HANDLE; - - mask = prev->valuators.mask[0] | next->valuators.mask[0]; - if (mask == next->valuators.mask[0]) /* keep next */ - { - for (i = j = k = 0; i < 8; i++) - { - if (XIMaskIsSet( prev->valuators.mask, i )) - next->valuators.values[j] += prev->valuators.values[k++]; - if (XIMaskIsSet( next->valuators.mask, i )) j++; - } - TRACE( "merging duplicate GenericEvent\n" ); - return MERGE_DISCARD; - } - if (mask == prev->valuators.mask[0]) /* keep prev */ - { - for (i = j = k = 0; i < 8; i++) - { - if (XIMaskIsSet( next->valuators.mask, i )) - prev->valuators.values[j] += next->valuators.values[k++]; - if (XIMaskIsSet( prev->valuators.mask, i )) j++; - } - TRACE( "merging duplicate GenericEvent\n" ); - return MERGE_IGNORE; - } - /* can't merge events with disjoint masks */ - return MERGE_HANDLE; -} -#endif - /*********************************************************************** * merge_events * @@ -344,18 +304,6 @@ static enum event_merge_action merge_events( XEvent *prev, XEvent *next ) if (next->xcookie.evtype != XI_RawMotion) break; if (x11drv_thread_data()->warp_serial) break; return MERGE_KEEP; - } - break; - case GenericEvent: - if (prev->xcookie.extension != xinput2_opcode) break; - if (prev->xcookie.evtype != XI_RawMotion) break; - switch (next->type) - { - case GenericEvent: - if (next->xcookie.extension != xinput2_opcode) break; - if (next->xcookie.evtype != XI_RawMotion) break; - if (x11drv_thread_data()->warp_serial) break; - return merge_raw_motion_events( prev->xcookie.data, next->xcookie.data ); #endif } break;
From: Rémi Bernon rbernon@codeweavers.com
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55027 --- dlls/winex11.drv/event.c | 19 ------------------- 1 file changed, 19 deletions(-)
diff --git a/dlls/winex11.drv/event.c b/dlls/winex11.drv/event.c index ce638fa5637..b5fbfcfc7fe 100644 --- a/dlls/winex11.drv/event.c +++ b/dlls/winex11.drv/event.c @@ -288,25 +288,6 @@ static enum event_merge_action merge_events( XEvent *prev, XEvent *next ) return MERGE_KEEP; } break; - case MotionNotify: - switch (next->type) - { - case MotionNotify: - if (prev->xany.window == next->xany.window) - { - TRACE( "discarding duplicate MotionNotify for window %lx\n", prev->xany.window ); - return MERGE_DISCARD; - } - break; -#ifdef HAVE_X11_EXTENSIONS_XINPUT2_H - case GenericEvent: - if (next->xcookie.extension != xinput2_opcode) break; - if (next->xcookie.evtype != XI_RawMotion) break; - if (x11drv_thread_data()->warp_serial) break; - return MERGE_KEEP; -#endif - } - break; } return MERGE_HANDLE; }
From: Rémi Bernon rbernon@codeweavers.com
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55027 --- dlls/winex11.drv/event.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/dlls/winex11.drv/event.c b/dlls/winex11.drv/event.c index b5fbfcfc7fe..f364cdbe715 100644 --- a/dlls/winex11.drv/event.c +++ b/dlls/winex11.drv/event.c @@ -309,6 +309,8 @@ static inline BOOL call_event_handler( Display *display, XEvent *event ) return FALSE; /* no handler, ignore it */ }
+ get_event_data( event ); + #ifdef GenericEvent if (event->type == GenericEvent) hwnd = 0; else #endif @@ -323,6 +325,8 @@ static inline BOOL call_event_handler( Display *display, XEvent *event ) thread_data->current_event = event; ret = handlers[event->type]( hwnd, event ); thread_data->current_event = prev; + + free_event_data( event ); return ret; }
@@ -371,7 +375,6 @@ static BOOL process_events( Display *display, Bool (*filter)(Display*, XEvent*,X else continue; /* filtered, ignore it */ } - get_event_data( &event ); if (prev_event.type) action = merge_events( &prev_event, &event ); switch( action ) { @@ -379,19 +382,16 @@ static BOOL process_events( Display *display, Bool (*filter)(Display*, XEvent*,X queued |= call_event_handler( display, &prev_event ); /* fall through */ case MERGE_DISCARD: /* discard prev, keep new */ - free_event_data( &prev_event ); prev_event = event; break; case MERGE_KEEP: /* handle new, keep prev for future merging */ queued |= call_event_handler( display, &event ); /* fall through */ case MERGE_IGNORE: /* ignore new, keep prev for future merging */ - free_event_data( &event ); break; } } if (prev_event.type) queued |= call_event_handler( display, &prev_event ); - free_event_data( &prev_event ); XFlush( gdi_display ); if (count) TRACE( "processed %d events, returning %d\n", count, queued ); return queued;
From: Rémi Bernon rbernon@codeweavers.com
By only delaying ConfigureNotify events for merging, and report empty composition string when result string is committed.
Based on a patch from Byeong-Sik Jeon bsjeon@hanmail.net.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55027 --- dlls/winex11.drv/event.c | 70 ++++++++++++---------------------------- dlls/winex11.drv/xim.c | 2 +- 2 files changed, 21 insertions(+), 51 deletions(-)
diff --git a/dlls/winex11.drv/event.c b/dlls/winex11.drv/event.c index f364cdbe715..3ba6589d2cf 100644 --- a/dlls/winex11.drv/event.c +++ b/dlls/winex11.drv/event.c @@ -255,44 +255,6 @@ static Bool filter_event( Display *display, XEvent *event, char *arg ) } }
- -enum event_merge_action -{ - MERGE_DISCARD, /* discard the old event */ - MERGE_HANDLE, /* handle the old event */ - MERGE_KEEP, /* keep the old event for future merging */ - MERGE_IGNORE /* ignore the new event, keep the old one */ -}; - -/*********************************************************************** - * merge_events - * - * Try to merge 2 consecutive events. - */ -static enum event_merge_action merge_events( XEvent *prev, XEvent *next ) -{ - switch (prev->type) - { - case ConfigureNotify: - switch (next->type) - { - case ConfigureNotify: - if (prev->xany.window == next->xany.window) - { - TRACE( "discarding duplicate ConfigureNotify for window %lx\n", prev->xany.window ); - return MERGE_DISCARD; - } - break; - case Expose: - case PropertyNotify: - return MERGE_KEEP; - } - break; - } - return MERGE_HANDLE; -} - - /*********************************************************************** * call_event_handler */ @@ -339,7 +301,6 @@ static BOOL process_events( Display *display, Bool (*filter)(Display*, XEvent*,X XEvent event, prev_event; int count = 0; BOOL queued = FALSE; - enum event_merge_action action = MERGE_DISCARD;
prev_event.type = 0; while (XCheckIfEvent( display, &event, filter, (char *)arg )) @@ -375,20 +336,29 @@ static BOOL process_events( Display *display, Bool (*filter)(Display*, XEvent*,X else continue; /* filtered, ignore it */ } - if (prev_event.type) action = merge_events( &prev_event, &event ); - switch( action ) + + /* delay and merge consecutive ConfigureNotify events */ + if (event.type == ConfigureNotify && prev_event.type == ConfigureNotify) { - case MERGE_HANDLE: /* handle prev, keep new */ - queued |= call_event_handler( display, &prev_event ); - /* fall through */ - case MERGE_DISCARD: /* discard prev, keep new */ + if (prev_event.xany.window != event.xany.window) queued |= call_event_handler( display, &prev_event ); + else TRACE( "discarding duplicate ConfigureNotify for window %lx\n", prev_event.xany.window ); prev_event = event; - break; - case MERGE_KEEP: /* handle new, keep prev for future merging */ + } + else if (event.type == ConfigureNotify) + { + TRACE( "delaying ConfigureNotify event for future merging\n" ); + prev_event = event; + } + else + { + /* allow merging over PropertyNotify and Expose events */ + if (event.type != PropertyNotify && event.type != Expose) + { + if (prev_event.type) call_event_handler( display, &prev_event ); + prev_event.type = 0; + } + queued |= call_event_handler( display, &event ); - /* fall through */ - case MERGE_IGNORE: /* ignore new, keep prev for future merging */ - break; } } if (prev_event.type) queued |= call_event_handler( display, &prev_event ); diff --git a/dlls/winex11.drv/xim.c b/dlls/winex11.drv/xim.c index 1c3d2dd9875..1b063cfe886 100644 --- a/dlls/winex11.drv/xim.c +++ b/dlls/winex11.drv/xim.c @@ -140,7 +140,7 @@ void xim_set_result_string( HWND hwnd, const char *str, UINT count ) len = ntdll_umbstowcs( str, count, output, count ); output[len] = 0;
- post_ime_update( hwnd, 0, ime_comp_buf, output ); + post_ime_update( hwnd, 0, NULL, output );
free( output ); }
From: Rémi Bernon rbernon@codeweavers.com
--- dlls/winex11.drv/xim.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/dlls/winex11.drv/xim.c b/dlls/winex11.drv/xim.c index 1b063cfe886..209d63f0402 100644 --- a/dlls/winex11.drv/xim.c +++ b/dlls/winex11.drv/xim.c @@ -175,6 +175,7 @@ static int xic_preedit_start( XIC xic, XPointer user, XPointer arg ) if ((ime_comp_buf = realloc( ime_comp_buf, sizeof(WCHAR) ))) *ime_comp_buf = 0; else ERR( "Failed to allocate preedit buffer\n" );
+ NtUserPostMessage( hwnd, WM_IME_NOTIFY, IMN_WINE_SET_OPEN_STATUS, TRUE ); post_ime_update( hwnd, 0, ime_comp_buf, NULL );
return -1; @@ -190,6 +191,7 @@ static int xic_preedit_done( XIC xic, XPointer user, XPointer arg ) ime_comp_buf = NULL;
post_ime_update( hwnd, 0, NULL, NULL ); + NtUserPostMessage( hwnd, WM_IME_NOTIFY, IMN_WINE_SET_OPEN_STATUS, FALSE );
return 0; }
I simplified the winex11 event merging instead of complicating it and kept only the merging of ConfigureNotify events.
I dropped the merging of RawMotion events, because even though it would reduce wineserver calls when a lot of events are received, it is not how rawinput is supposed to work.
I also don't think merging MotionNotify events is very useful, to reduce the number of redundant messages sent to wineserver as well, so I dropped it too.
If we want to reduce the wineserver calls, I think a better approach would be to send input in batch.
You've done a great job. !2698 worked beautifully, no doubled result texts at all. (Except for the VBA editor in which it happens 100%.) Maybe the VBA editor handles IME inputs in some different way. At least one issue now seems resolved.
On Tue Jun 13 08:13:19 2023 +0000, Masahito Suzuki wrote:
You've done a great job. !2698 worked beautifully, no doubled result texts at all. (Except for the VBA editor in which it happens 100%.) Maybe the VBA editor handles IME inputs in some different way. Anyways, at least one issue now seems resolved.
Thank you for confirming.
Would you mind attaching a log with WINEDEBUG=+xim,+imm environment variable with the VBA editor?
On Tue Jun 13 08:13:19 2023 +0000, Rémi Bernon wrote:
Thank you for confirming. Would you mind attaching a log with WINEDEBUG=+xim,+imm environment variable with the VBA editor?
Yes, sure thing. Here's the log. [wine.log](/uploads/dbc55542c92a065f20f11787c20f23ef/wine.log)
Starting from 36533.296, I typed AIUEO and then Enter. I hope it helps.
I also don't think merging MotionNotify events is very useful, it's probably there to reduce the number of redundant messages sent to wineserver as well, so I dropped it too.
It is definitely useful, otherwise we wouldn't be doing it.
On Tue Jun 13 08:35:16 2023 +0000, Alexandre Julliard wrote:
I also don't think merging MotionNotify events is very useful, it's
probably there to reduce the number of redundant messages sent to wineserver as well, so I dropped it too. It is definitely useful, otherwise we wouldn't be doing it.
Is there a specific reason except to avoid sending unnecessary input? We already merge messages on the `wineserver` side no?
Is there a specific reason except to avoid sending unnecessary input? We already merge messages on the wineserver side no?
Mouse events can come in very quickly, and if the server is spammed with mouse messages it can't handle other requests in a timely manner, so merging on the server side is not sufficient.
Alright. Fwiw I don't think this is the right place to fix this issue, and I'm also not convinced that the merging succeeds very often or that it helps that much, when wineserver cannot already keep up.
@julliard and @rbernon In my opinion, this !2698 is worth a merge anyway, even if VBA remains problematic. To CJK users, not being able to use IME in MSOffice is a critical show-stopper to reason to use WINE as Windows alternative. IME in VBA will be relatively unnecessary, since not many people would use multi-byte characters in variable names. The main case we need those in VBA is in the comments.
On Wed Jun 14 08:04:17 2023 +0000, Masahito Suzuki wrote:
@julliard and @rbernon In my opinion, this !2698 is worth a merge anyway, even if VBA remains problematic. To CJK users, not being able to use IME in MSOffice is a critical show-stopper to reason to use WINE as Windows alternative. IME in VBA will be relatively unnecessary, since not many people would use multi-byte characters in variable names. The main case we need those in VBA is in the comments.
Sure, I will update this shortly with some changes to keep mouse event merging for now.
I didn't find anywhere more appropriate to post this, so please let me know if it's not welcome here. If you're going to include the following two patches: * [PATCH 1/6] winex11: Use a bit mask for event merge actions. * [PATCH 2/6] winex11: Avoid breaking XIM XFilterEvent / XmbLookupString sequence.
then how about also adding some changes like this on top of it. [0001-winex11-In-process_events-strictly-fix-condition-gli.patch](/uploads/f484bc36c40084e0538ea8be14110674/0001-winex11-In-process_events-strictly-fix-condition-gli.patch)
This will more simply and accurately correspond to the old logic, in faster and more simple way, while retaining the extendability and flexibility you've added for setting the handling/freeing flags on old/new events separately.
### Fixing the branch logic
I saw that you extended the event merging logic so that the developer can now set different policies (keep it, process it, or discard it) for each of the old and new events independently. This I interpreted as for the purpose to make it capable for future possible cases when you need to process both old and new events at once in various patterns like "process both old and new events", "process the old event and discard the new event" and so on.
I analyzed the logic for when to perform `prev_event = event;` and found a little glitch.
The original WINE 8.10 logic would branch to either "(process and) free the old event" `HANDLE/DISCARD` flow or "(process and) free the new event" `KEEP/IGNORE`. And when choosing the "(process and) free the old event" branch by setting the event_merge_action value to `MERGE_HANDLE` or `MERGE_DISCARD`, it would do the copy.
In your new code it would do the copy when `MERGE_FREE_NEW` flag is not set. But instead, the copy should be done when both conditions are met: * The old event is set to be freed * The new event is not set to be freed
So I fixed the code to better reflect the original logic.
### Omitting the FREE flag
Now, by going through the code, setting the HANDLE flag on while not setting the FREE flag on for the same event seemed not going to happen at all in design. Otherwise the same event would be processed again later. This means if an event is set to be handled, then it can always be considered set to be freed too. So I made it when the HANDLE flag is on, the logic also implies to free the event as if the FREE flag was on, without needing to set the FREE flag explicitly on. As you can see, `MERGE_HANDLE_OR_FREE_OLD` and `MERGE_HANDLE_OR_FREE_NEW` translates to logical add of those flags. Testing `action | MERGE_HANDLE_OR_FREE_*` means run the following code (free the event) if either `HANDLE | FREE` flag for the event is set. Now those FREE flags became redundant, thus could be made optional, or further, omitted if HANDLE is on, to simplify the logic. I rewrote the code where those flags `HANDLE | FREE` to only have `HANDLE` instead. (Where only `FREE` flag is set remains as is.) You can still explicitly set both HANDLE and FREE flags on just in case, and the result will be the same.
I hope you'll like it.
On Thu Jun 15 13:03:02 2023 +0000, Masahito Suzuki wrote:
I didn't find anywhere more appropriate to post this, so please let me know if it's not welcome here. If you're going to include the following two patches:
- [PATCH 1/6] winex11: Use a bit mask for event merge actions.
- [PATCH 2/6] winex11: Avoid breaking XIM XFilterEvent / XmbLookupString sequence.
then how about also adding some changes like this on top of it. [0001-winex11-In-process_events-strictly-fix-condition-gli.patch](/uploads/f484bc36c40084e0538ea8be14110674/0001-winex11-In-process_events-strictly-fix-condition-gli.patch) This will more simply and accurately correspond to the old logic, in faster and more simple way, while retaining the extendability and flexibility you've added for setting the handling/freeing flags on old/new events separately. ### Fixing the branch logic I saw that you extended the event merging logic so that the developer can now set different policies (keep it, process it, or discard it) for each of the old and new events independently. This I interpreted as for the purpose to make it capable for future possible cases when you need to process both old and new events at once in various patterns like "process both old and new events", "process the old event and discard the new event" and so on. I analyzed the logic for when to perform `prev_event = event;` and found a little glitch. The original WINE 8.10 logic would branch to either "(process and) free the old event" `HANDLE/DISCARD` flow or "(process and) free the new event" `KEEP/IGNORE`. And when choosing the "(process and) free the old event" branch by setting the event_merge_action value to `MERGE_HANDLE` or `MERGE_DISCARD`, it would do the copy. In your new code it would do the copy when `MERGE_FREE_NEW` flag is not set. But instead, the copy should be done when both conditions are met:
- The old event is set to be freed
- The new event is not set to be freed
So I fixed the code to better reflect the original logic. ### Omitting the FREE flag Now, by going through the code, setting the HANDLE flag on while not setting the FREE flag on for the same event seemed not going to happen at all in design. Otherwise the same event would be processed again later. This means if an event is set to be handled, then it can always be considered set to be freed too. So I made it when the HANDLE flag is on, the logic also implies to free the event as if the FREE flag was on, without needing to set the FREE flag explicitly on. As you can see, `MERGE_HANDLE_OR_FREE_OLD` and `MERGE_HANDLE_OR_FREE_NEW` translates to logical add of those flags. Testing `action | MERGE_HANDLE_OR_FREE_*` means run the following code (free the event) if either `HANDLE | FREE` flag for the event is set. Now those FREE flags became redundant, thus could be made optional, or further, omitted if HANDLE is on, to simplify the logic. I rewrote the code where those flags `HANDLE | FREE` to only have `HANDLE` instead. (Where only `FREE` flag is set remains as is.) You can still explicitly set both HANDLE and FREE flags on just in case, and the result will be the same. I hope you'll like it.
Thanks for the suggestion. Then I would rather not make it easier to merge events. I think it is not a great idea, and as I see it it is only there to workaround some limitations, and ideally there would be no need to merge anything.
-
Alexandre Julliard (＠julliard) -
Masahito Suzuki (＠firelzrd)
-
Rémi Bernon
-
Rémi Bernon (＠rbernon)