Hi Rémi,
Thanks for the review.
On 8/22/19 12:29 PM, Rémi Bernon wrote:
On 8/15/19 3:57 PM, Gabriel Ivăncescu wrote:
@@ -803,7 +820,7 @@ static void focus_out( Display *display , HWND hwnd ) if (ximInComposeMode) return; - x11drv_thread_data()->last_focus = hwnd; + thread_data->last_focus = hwnd; if ((xic = X11DRV_get_ic( hwnd ))) XUnsetICFocus( xic ); if (root_window != DefaultRootWindow(display)) @@ -812,6 +829,12 @@ static void focus_out( Display *display , HWND hwnd ) return; } if (hwnd != GetForegroundWindow()) return;
+ if ((long)(thread_data->focus_serial - serial) > 0) + { + TRACE("ignoring old serial %lu/%lu\n", serial, thread_data->focus_serial); + return; + }
I'm not sure if this is the right thing to do here.
I understand the reason to skip calling SetForegroundWindow on FocusIn events - because SetFocus called by DefWinProc would already have done it before, as a consequence of ShowWindow / WM_ACTIVATE messages - but in the case of focus out events, I believe it is already skipped if the newly focused window is another Wine window.
So I should not add it to focus out then, right?
Also we will not send the WM_CANCELMODE message anymore and I don't know if it is OK.
Sure I can move it after WM_CANCELMODE, but I'm afraid it's impossible to test this as on Windows since it's a X11 quirk (receiving focus events from outside of Wine's control), so we have to go with best judgment here.
My reasoning for not including it is the same as with focus: typically, "older" focus events should be discarded, and they are indeed discarded by the X11 server (which is why they have a time field) although we still process them on the Wine side. In this case I think it's fine if we also don't send messages related to them, but I don't know of any app that relies on WM_CANCELMODE for this purpose.
I suppose, though, that it would be less invasive if we do send the WM_CANCELMODE as before. Perhaps in the future if an app relies on this we can revisit it, so I'll just go with that.
SendMessageW( hwnd, WM_CANCELMODE, 0, 0 ); /* don't reset the foreground window, if the window which is @@ -855,7 +878,7 @@ static BOOL X11DRV_FocusOut( HWND hwnd, XEvent *xev ) return TRUE; } if (!hwnd) return FALSE; - focus_out( event->display, hwnd ); + focus_out( event->display, hwnd, event->serial ); return TRUE; } @@ -1385,6 +1408,7 @@ void wait_for_withdrawn_state( HWND hwnd, BOOL set ) */ void CDECL X11DRV_SetFocus( HWND hwnd ) { + struct x11drv_thread_data *thread_data; struct x11drv_win_data *data; HWND parent; @@ -1400,6 +1424,21 @@ void CDECL X11DRV_SetFocus( HWND hwnd ) } if (!data->managed || data->embedder) set_input_focus( data ); release_win_data( data );
+ hwnd = GetAncestor(hwnd, GA_ROOT); + if (hwnd != (thread_data = x11drv_thread_data())->setfocus_hwnd)
I think it would be more readable to initialize thread_data along with its declaration, like it's done elsewhere, unless there's a good reason not to.
+ {
+ thread_data->setfocus_hwnd = hwnd;
+ /* If we are not processing an event, store the current serial after syncing + with the X server, so that we ignore all older focus-related events. This + prevents focus being reverted later when the app processes its messages. */ + if (!thread_data->current_event) + { + XSync(thread_data->display, False); + thread_data->focus_serial = LastKnownRequestProcessed(thread_data->display) + 1; + } + } } diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h index 0d3695b..63598d7 100644 --- a/dlls/winex11.drv/x11drv.h +++ b/dlls/winex11.drv/x11drv.h @@ -326,10 +326,12 @@ struct x11drv_thread_data XEvent *current_event; /* event currently being processed */ HWND grab_hwnd; /* window that currently grabs the mouse */ HWND last_focus; /* last window that had focus */ + HWND setfocus_hwnd; /* top-level window corresponding to the last SetFocus */ XIM xim; /* input method */ HWND last_xic_hwnd; /* last xic window */ XFontSet font_set; /* international text drawing font set */ Window selection_wnd; /* window used for selection interactions */ + unsigned long focus_serial; /* serial number when last SetFocus without an event happened */ unsigned long warp_serial; /* serial number of last pointer warp request */ Window clip_window; /* window used for cursor clipping */ HWND clip_hwnd; /* message window stored in desktop while clipping is active */
I would rename focus_serial to setfocus_serial and keep it close to setfocus_hwnd as they are meant to work together - or the other way around. This way we also understand that set_focus
Otherwise I would say it looks OK to me with my limited knowledge of the whole thing works.
Noted.
Thanks, Gabriel