[PATCH resend] winex11.drv: Don't set the foreground for focus events that are older than the last SetFocus sent without an event

[Gabriel Ivăncescu]

When creating a visible window, the focus and the respective messages are
immediately sent to the created window to match Windows behavior. However,
the X11 server also sends either FocusIn events or the WM sends ClientMessages
with the WM_TAKE_FOCUS atom (by default) if the windows are managed, since
it received focus. If the application didn't have a message loop up to
process these events, they will remain in the queue.

When creating one window, this is not a problem because the focus set
manually and the focus set when processing the events will be the same and
no message is sent. However, if 2 or more windows are created before the
message loop, when these events are later processed by the app's message
loop, they will change the focus and send the messages again, even though
we've already done so earlier, which confuses some apps at this point. One
example is Heroes of Might and Magic 5.

See comment #11 on https://bugs.winehq.org/show_bug.cgi?id=39742#c11 --
`When "Full screen" active game starts and then shows desktop but it is not
exited and after a few times switching game/desktop it shows game screen
and it is playable`

Related Proton bug: https://github.com/ValveSoftware/Proton/issues/2401

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---

Note that set_focus has to unconditionally (regardless of serial) set
the focus with XSetInputFocus since it's handled in WM_TAKE_FOCUS. The X
server will take care of old focus events and sort them out itself using
the time. We just have to stop sending the respective focus messages
(i.e. SetForegroundWindow) and keep the current foreground window because
it was set by a "later" SetFocus.

 dlls/winex11.drv/event.c  | 65 +++++++++++++++++++++++++++++++--------
 dlls/winex11.drv/x11drv.h |  2 ++
 2 files changed, 54 insertions(+), 13 deletions(-)

diff --git a/dlls/winex11.drv/event.c b/dlls/winex11.drv/event.c
index edeae7c..89a28b1 100644
--- a/dlls/winex11.drv/event.c
+++ b/dlls/winex11.drv/event.c
@@ -591,14 +591,22 @@ static void set_input_focus( struct x11drv_win_data *data )
 /**********************************************************************
  *              set_focus
  */
-static void set_focus( Display *display, HWND hwnd, Time time )
+static void set_focus( Display *display, HWND hwnd, Time time, unsigned long serial )
 {
+    struct x11drv_thread_data *thread_data = x11drv_thread_data();
     HWND focus;
     Window win;
     GUITHREADINFO threadinfo;
 
-    TRACE( "setting foreground window to %p\n", hwnd );
-    SetForegroundWindow( hwnd );
+    if ((long)(thread_data->focus_serial - serial) > 0)
+    {
+        TRACE( "ignoring old serial %lu/%lu\n", serial, thread_data->focus_serial );
+    }
+    else
+    {
+        TRACE( "setting foreground window to %p\n", hwnd );
+        SetForegroundWindow( hwnd );
+    }
 
     threadinfo.cbSize = sizeof(threadinfo);
     GetGUIThreadInfo(0, &threadinfo);
@@ -706,7 +714,7 @@ static void handle_wm_protocols( HWND hwnd, XClientMessageEvent *event )
                                        MAKELONG(HTCAPTION,WM_LBUTTONDOWN) );
             if (ma != MA_NOACTIVATEANDEAT && ma != MA_NOACTIVATE)
             {
-                set_focus( event->display, hwnd, event_time );
+                set_focus( event->display, hwnd, event_time, event->serial );
                 return;
             }
         }
@@ -715,7 +723,7 @@ static void handle_wm_protocols( HWND hwnd, XClientMessageEvent *event )
             hwnd = GetForegroundWindow();
             if (!hwnd) hwnd = last_focus;
             if (!hwnd) hwnd = GetDesktopWindow();
-            set_focus( event->display, hwnd, event_time );
+            set_focus( event->display, hwnd, event_time, event->serial );
             return;
         }
         /* try to find some other window to give the focus to */
@@ -723,7 +731,7 @@ static void handle_wm_protocols( HWND hwnd, XClientMessageEvent *event )
         if (hwnd) hwnd = GetAncestor( hwnd, GA_ROOT );
         if (!hwnd) hwnd = GetActiveWindow();
         if (!hwnd) hwnd = last_focus;
-        if (hwnd && can_activate_window(hwnd)) set_focus( event->display, hwnd, event_time );
+        if (hwnd && can_activate_window(hwnd)) set_focus( event->display, hwnd, event_time, event->serial );
     }
     else if (protocol == x11drv_atom(_NET_WM_PING))
     {
@@ -785,17 +793,26 @@ static BOOL X11DRV_FocusIn( HWND hwnd, XEvent *xev )
         if (hwnd) hwnd = GetAncestor( hwnd, GA_ROOT );
         if (!hwnd) hwnd = GetActiveWindow();
         if (!hwnd) hwnd = x11drv_thread_data()->last_focus;
-        if (hwnd && can_activate_window(hwnd)) set_focus( event->display, hwnd, CurrentTime );
+        if (hwnd && can_activate_window(hwnd)) set_focus( event->display, hwnd, CurrentTime, event->serial );
+    }
+    else
+    {
+        struct x11drv_thread_data *thread_data = x11drv_thread_data();
+
+        if ((long)(thread_data->focus_serial - event->serial) > 0)
+            TRACE("ignoring old serial %lu/%lu\n", event->serial, thread_data->focus_serial);
+        else
+            SetForegroundWindow( hwnd );
     }
-    else SetForegroundWindow( hwnd );
     return TRUE;
 }
 
 /**********************************************************************
  *              focus_out
  */
-static void focus_out( Display *display , HWND hwnd )
+static void focus_out( Display *display, HWND hwnd, unsigned long serial )
  {
+    struct x11drv_thread_data *thread_data = x11drv_thread_data();
     HWND hwnd_tmp;
     Window focus_win;
     int revert;
@@ -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;
+    }
     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)
+    {
+        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;
+        }
+    }
 }
 
 
@@ -1667,12 +1706,12 @@ static void handle_xembed_protocol( HWND hwnd, XClientMessageEvent *event )
 
     case XEMBED_WINDOW_DEACTIVATE:
         TRACE( "win %p/%lx XEMBED_WINDOW_DEACTIVATE message\n", hwnd, event->window );
-        focus_out( event->display, GetAncestor( hwnd, GA_ROOT ) );
+        focus_out( event->display, GetAncestor( hwnd, GA_ROOT ), event->serial );
         break;
 
     case XEMBED_FOCUS_OUT:
         TRACE( "win %p/%lx XEMBED_FOCUS_OUT message\n", hwnd, event->window );
-        focus_out( event->display, GetAncestor( hwnd, GA_ROOT ) );
+        focus_out( event->display, GetAncestor( hwnd, GA_ROOT ), event->serial );
         break;
 
     case XEMBED_MODALITY_ON:
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 */
-- 
2.21.0

[Rémi Bernon]

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.

Also we will not send the WM_CANCELMODE message anymore and I don't know 
if it is OK.
> 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.
-- 
Rémi Bernon <rbernon(a)codeweavers.com>

[Rémi Bernon]

On 8/22/19 11:29 AM, Rémi Bernon wrote:
> 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

... s/set_focus/SetFocus/ is involved in setting both.
-- 
Rémi Bernon <rbernon(a)codeweavers.com>

[Gabriel Ivăncescu]

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

[Rémi Bernon]

On 8/22/19 2:26 PM, Gabriel Ivăncescu wrote:
> 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?

Yes I think focus_out is fine as it is. Except for the WM_CANCELMODE 
message that is a bit unclear, all it does right now is setting back the 
focus to desktop window if there is no more Wine window with input focus.

I think it will not happen in your scenario. And even if it does, if the 
X server says that Wine windows do not have input focus then we should 
trust it.
-- 
Rémi Bernon <rbernon(a)codeweavers.com>