[PATCH v2 0/2] MR2698: winex11: Avoid breaking XIM XFilterEvent / XmbLookupString sequence.
-- v2: winex11: Avoid breaking XIM XFilterEvent / XmbLookupString sequence. winex11: Use a bit mask for event merge actions. https://gitlab.winehq.org/wine/wine/-/merge_requests/2698
From: Rémi Bernon <rbernon(a)codeweavers.com> Based on a patch from Byeong-Sik Jeon <bsjeon(a)hanmail.net>. --- dlls/winex11.drv/event.c | 62 ++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 34 deletions(-) diff --git a/dlls/winex11.drv/event.c b/dlls/winex11.drv/event.c index 1ae39eb9edf..67edfa68b60 100644 --- a/dlls/winex11.drv/event.c +++ b/dlls/winex11.drv/event.c @@ -170,9 +170,10 @@ static inline void get_event_data( XEvent *event ) static inline void free_event_data( XEvent *event ) { #if defined(GenericEvent) && defined(HAVE_XEVENT_XCOOKIE) - if (event->xany.type != GenericEvent) return; - if (event->xcookie.data) pXFreeEventData( event->xany.display, event ); + if (event->xany.type == GenericEvent && event->xcookie.data) + pXFreeEventData( event->xany.display, event ); #endif + event->xany.type = 0; } /*********************************************************************** @@ -260,10 +261,10 @@ 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_HANDLE_OLD = 0x1, + MERGE_FREE_OLD = 0x2, + MERGE_HANDLE_NEW = 0x4, + MERGE_FREE_NEW = 0x8, }; /*********************************************************************** @@ -275,8 +276,8 @@ static enum event_merge_action merge_raw_motion_events( XIRawEvent *prev, XIRawE int i, j, k; unsigned char mask; - if (!prev->valuators.mask_len) return MERGE_HANDLE; - if (!next->valuators.mask_len) return MERGE_HANDLE; + if (!prev->valuators.mask_len) return MERGE_HANDLE_OLD | MERGE_FREE_OLD; + if (!next->valuators.mask_len) return MERGE_HANDLE_OLD | MERGE_FREE_OLD; mask = prev->valuators.mask[0] | next->valuators.mask[0]; if (mask == next->valuators.mask[0]) /* keep next */ @@ -288,7 +289,7 @@ static enum event_merge_action merge_raw_motion_events( XIRawEvent *prev, XIRawE if (XIMaskIsSet( next->valuators.mask, i )) j++; } TRACE( "merging duplicate GenericEvent\n" ); - return MERGE_DISCARD; + return MERGE_FREE_OLD; } if (mask == prev->valuators.mask[0]) /* keep prev */ { @@ -299,10 +300,10 @@ static enum event_merge_action merge_raw_motion_events( XIRawEvent *prev, XIRawE if (XIMaskIsSet( prev->valuators.mask, i )) j++; } TRACE( "merging duplicate GenericEvent\n" ); - return MERGE_IGNORE; + return MERGE_FREE_NEW; } /* can't merge events with disjoint masks */ - return MERGE_HANDLE; + return MERGE_HANDLE_OLD | MERGE_FREE_OLD; } #endif @@ -313,6 +314,9 @@ static enum event_merge_action merge_raw_motion_events( XIRawEvent *prev, XIRawE */ static enum event_merge_action merge_events( XEvent *prev, XEvent *next ) { + enum event_merge_action default_action = MERGE_FREE_OLD; + if (prev->type) default_action |= MERGE_HANDLE_OLD; + switch (prev->type) { case ConfigureNotify: @@ -322,12 +326,12 @@ static enum event_merge_action merge_events( XEvent *prev, XEvent *next ) if (prev->xany.window == next->xany.window) { TRACE( "discarding duplicate ConfigureNotify for window %lx\n", prev->xany.window ); - return MERGE_DISCARD; + return MERGE_FREE_OLD; } break; case Expose: case PropertyNotify: - return MERGE_KEEP; + return MERGE_HANDLE_NEW | MERGE_FREE_NEW; } break; case MotionNotify: @@ -337,7 +341,7 @@ static enum event_merge_action merge_events( XEvent *prev, XEvent *next ) if (prev->xany.window == next->xany.window) { TRACE( "discarding duplicate MotionNotify for window %lx\n", prev->xany.window ); - return MERGE_DISCARD; + return MERGE_FREE_OLD; } break; #ifdef HAVE_X11_EXTENSIONS_XINPUT2_H @@ -345,7 +349,7 @@ static enum event_merge_action merge_events( XEvent *prev, XEvent *next ) if (next->xcookie.extension != xinput2_opcode) break; if (next->xcookie.evtype != XI_RawMotion) break; if (x11drv_thread_data()->warp_serial) break; - return MERGE_KEEP; + return MERGE_HANDLE_NEW | MERGE_FREE_NEW; } break; case GenericEvent: @@ -362,7 +366,8 @@ static enum event_merge_action merge_events( XEvent *prev, XEvent *next ) } break; } - return MERGE_HANDLE; + + return default_action; } @@ -408,7 +413,7 @@ 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; + enum event_merge_action action; prev_event.type = 0; while (XCheckIfEvent( display, &event, filter, (char *)arg )) @@ -445,23 +450,12 @@ static BOOL process_events( Display *display, Bool (*filter)(Display*, XEvent*,X continue; /* filtered, ignore it */ } get_event_data( &event ); - if (prev_event.type) action = merge_events( &prev_event, &event ); - switch( action ) - { - case MERGE_HANDLE: /* handle prev, keep new */ - 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; - } + action = merge_events( &prev_event, &event ); + if (action & MERGE_HANDLE_OLD) queued |= call_event_handler( display, &prev_event ); + if (action & MERGE_FREE_OLD) free_event_data( &prev_event ); + if (action & MERGE_HANDLE_NEW) queued |= call_event_handler( display, &event ); + if (action & MERGE_FREE_NEW) free_event_data( &event ); + else prev_event = event; } if (prev_event.type) queued |= call_event_handler( display, &prev_event ); free_event_data( &prev_event ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2698
From: Rémi Bernon <rbernon(a)codeweavers.com> XFilterEvent generates new ClientMessage events, used for XIM callbacks, as well as passes some KeyPress events through meant for XmbLookupString. Delaying the KeyPress events processing breaks the sequence on the next XFilterEvent call. Based on a patch from Byeong-Sik Jeon <bsjeon(a)hanmail.net>. --- dlls/winex11.drv/event.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/dlls/winex11.drv/event.c b/dlls/winex11.drv/event.c index 67edfa68b60..ef3adb2a053 100644 --- a/dlls/winex11.drv/event.c +++ b/dlls/winex11.drv/event.c @@ -367,6 +367,13 @@ static enum event_merge_action merge_events( XEvent *prev, XEvent *next ) break; } + switch (next->type) + { + case KeyPress: + case KeyRelease: + return default_action | MERGE_HANDLE_NEW | MERGE_FREE_NEW; + } + return default_action; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2698
Thank you. It works fine. There are other issues with the xim Korean input, but only when this is fixed we can approach it in the right way. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2698#note_31008
I've modified the proposed patch to not touch merge_events(). I don't think any reason to keep it in prev_event for non-mergeable events, but maybe there's an intent I'm not aware of. ``` diff --git a/dlls/winex11.drv/event.c b/dlls/winex11.drv/event.c index 4ec37185751..8b25ed0e14e 100644 --- a/dlls/winex11.drv/event.c +++ b/dlls/winex11.drv/event.c @@ -364,6 +364,19 @@ static enum event_merge_action merge_events( XEvent *prev, XEvent *next ) } +static BOOL is_mergeable( XEvent *prev, XEvent *next ) +{ + if (next->type == ConfigureNotify || + next->type == MotionNotify || + next->type == GenericEvent) return TRUE; + + if (prev->type == ConfigureNotify && + (next->type == Expose || next->type == PropertyNotify)) return TRUE; + + return FALSE; +} + + /*********************************************************************** * call_event_handler */ @@ -442,6 +455,19 @@ static BOOL process_events( Display *display, Bool (*filter)(Display*, XEvent*,X else continue; /* filtered, ignore it */ } + + if (!is_mergeable( &prev_event, &event )) + { + if (prev_event.type) + { + queued |= call_event_handler( display, &prev_event ); + free_event_data( &prev_event ); + prev_event.type = 0; + } + queued |= call_event_handler( display, &event ); + continue; + } + get_event_data( &event ); if (prev_event.type) action = merge_events( &prev_event, &event ); switch( action ) ``` The patch with just the KeyPress, KeyRelease check instead of !is_mergeable() also works fine, so that's good too. It might be better to focus on the issue only. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2698#note_31159
participants (2)
-
Byeongsik Jeon (＠bsjeon) -
Rémi Bernon