[PATCH 1/2] winex11.drv: Keep the window's own restored state when unminimized, if it was fullscreen before.

[Gabriel Ivăncescu]

Fixes a regression introduced by commit
82c6ec3a32f44e8b3e0cc88b7f10e0c0d7fa1b89, which caused the WM_ACTIVATEAPP
to be sent while the window is minimized, if it has been clicked on in the
taskbar to be restored.

According to the Extended Window Manager Hints spec, WMs remove the
NET_WM_STATE_FULLSCREEN state when minimized and then, when restored, use
the previous size of the window (not the fullscreen) to restore to. This
caused the WM_SYSCOMMAND SC_RESTORE message's ShowWindow to revert the window
back to its original (non-fullscreen) size, instead of being restored to
fullscreen, breaking some apps like Heroes of Might and Magic V.

We have to override the X server's window state here to match Windows
behavior and restore it back to fullscreen.

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

Supersedes patches 193752-193754.

update_net_wm_states is when Wine does a change and needs to inform the X
server about it (i.e. Wine->X11 state change). So, that's good as it is.

read_net_wm_states is when X11 changes some state from outside Wine and
sends it a notification that it did; then Wine has to update its own wm
states to match the X server (so it's properly integrated).

However, in this case, since the WM clears the fullscreen state on
minimization, we must not do that, because Windows doesn't—and more,
when the window is restored from minimized, we must ignore the X11 state
changes and force it back to what Wine had (usually fullscreen unless it
gets hooked by the app, then we keep what the app changed). This is to
match Windows behavior, which differs from X11, and apps rely on.

 dlls/winex11.drv/event.c  | 13 +++++++++++++
 dlls/winex11.drv/window.c |  9 ++++++++-
 dlls/winex11.drv/x11drv.h |  1 +
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/dlls/winex11.drv/event.c b/dlls/winex11.drv/event.c
index 07f7a1a..350d533 100644
--- a/dlls/winex11.drv/event.c
+++ b/dlls/winex11.drv/event.c
@@ -1296,6 +1296,8 @@ static void handle_wm_state_notify( HWND hwnd, XPropertyEvent *event, BOOL updat
 
     if (data->iconic && data->wm_state == NormalState)  /* restore window */
     {
+        DWORD old_state = data->net_wm_state;
+
         data->iconic = FALSE;
         read_net_wm_states( event->display, data );
         if ((style & WS_CAPTION) == WS_CAPTION && (data->net_wm_state & (1 << NET_WM_STATE_MAXIMIZED)))
@@ -1313,11 +1315,22 @@ static void handle_wm_state_notify( HWND hwnd, XPropertyEvent *event, BOOL updat
         {
             if (style & (WS_MINIMIZE | WS_MAXIMIZE))
             {
+                /* if the window was fullscreen before minimized, we have to keep its
+                   restored state and restore it to its fullscreen state, because WMs
+                   remove the fullscreen state when minimized, so don't use their state. */
+                if (old_state & (1 << NET_WM_STATE_FULLSCREEN))
+                    data->keep_restored_state = TRUE;
+
                 TRACE( "restoring win %p/%lx\n", data->hwnd, data->whole_window );
                 release_win_data( data );
                 if ((style & (WS_MINIMIZE | WS_VISIBLE)) == (WS_MINIMIZE | WS_VISIBLE))
                     SetActiveWindow( hwnd );
                 SendMessageW( hwnd, WM_SYSCOMMAND, SC_RESTORE, 0 );
+                if ((data = get_win_data( hwnd )))
+                {
+                    data->keep_restored_state = FALSE;
+                    release_win_data( data );
+                }
                 return;
             }
             TRACE( "not restoring win %p/%lx style %08x\n", data->hwnd, data->whole_window, style );
diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c
index 4571739..d9d207c 100644
--- a/dlls/winex11.drv/window.c
+++ b/dlls/winex11.drv/window.c
@@ -1076,6 +1076,8 @@ void read_net_wm_states( Display* display, struct x11drv_win_data *data )
     if (!maximized_horz)
         new_state &= ~(1 << NET_WM_STATE_MAXIMIZED);
 
+    if ((data->net_wm_state & (1 << NET_WM_STATE_FULLSCREEN)) && data->iconic)
+        new_state |= 1 << NET_WM_STATE_FULLSCREEN;
     data->net_wm_state = new_state;
 }
 
@@ -2450,6 +2452,11 @@ void CDECL X11DRV_WindowPosChanged( HWND hwnd, HWND insert_after, UINT swp_flags
         if (event_type != ConfigureNotify && event_type != PropertyNotify &&
             event_type != GravityNotify && event_type != ReparentNotify)
             event_type = 0;  /* ignore other events */
+
+        /* if we keep the Wine window's restored state, we pretend it's
+           not an event, so that it's synced properly with the X server. */
+        if (data->keep_restored_state && !(new_style & WS_MINIMIZE))
+            event_type = 0;
     }
 
     if (data->mapped && event_type != ReparentNotify)
@@ -2545,7 +2552,7 @@ UINT CDECL X11DRV_ShowWindow( HWND hwnd, INT cmd, RECT *rect, UINT swp )
         }
         goto done;
     }
-    if (!data->managed || !data->mapped || data->iconic) goto done;
+    if (!data->managed || !data->mapped || data->iconic || data->keep_restored_state) goto done;
 
     /* only fetch the new rectangle if the ShowWindow was a result of a window manager event */
 
diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h
index 173d94b..de725d9 100644
--- a/dlls/winex11.drv/x11drv.h
+++ b/dlls/winex11.drv/x11drv.h
@@ -574,6 +574,7 @@ struct x11drv_win_data
     BOOL        shaped : 1;     /* is window using a custom region shape? */
     BOOL        layered : 1;    /* is window layered and with valid attributes? */
     BOOL        use_alpha : 1;  /* does window use an alpha channel? */
+    BOOL        keep_restored_state : 1; /* don't override the window state when restoring from an event */
     int         wm_state;       /* current value of the WM_STATE property */
     DWORD       net_wm_state;   /* bit mask of active x11drv_net_wm_state values */
     Window      embedder;       /* window id of embedder */
-- 
2.21.0

[PATCH 2/2] d3d9/tests: Add interactive test for window pos messages received when restoring a window after device lost.

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---
 dlls/d3d9/tests/device.c | 55 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/dlls/d3d9/tests/device.c b/dlls/d3d9/tests/device.c
index 53a83ad..7c2fcfa 100644
--- a/dlls/d3d9/tests/device.c
+++ b/dlls/d3d9/tests/device.c
@@ -11404,6 +11404,35 @@ static void test_writeonly_resource(void)
     DestroyWindow(window);
 }
 
+static BOOL test_lost_device_focus_set, test_lost_device_pos_changed_ok;
+static WNDPROC test_lost_device_orig_proc;
+
+static LRESULT CALLBACK test_lost_device_proc(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam)
+{
+    switch (msg)
+    {
+    case WM_SETFOCUS:
+        test_lost_device_focus_set = TRUE;
+        break;
+    case WM_WINDOWPOSCHANGED:
+        if (!IsIconic(hwnd))
+        {
+            WINDOWPOS *p = (WINDOWPOS*)lParam;
+            MONITORINFO mi;
+
+            mi.cbSize = sizeof(mi);
+            GetMonitorInfoA(MonitorFromWindow(hwnd, MONITOR_DEFAULTTONEAREST), &mi);
+
+            if (p->x == mi.rcMonitor.left && p->y == mi.rcMonitor.top &&
+                p->cx == registry_mode.dmPelsWidth && p->cy == registry_mode.dmPelsHeight &&
+                !(p->flags & SWP_NOSIZE))
+                test_lost_device_pos_changed_ok = TRUE;
+        }
+        break;
+    }
+    return CallWindowProcA(test_lost_device_orig_proc, hwnd, msg, wParam, lParam);
+}
+
 static void test_lost_device(void)
 {
     struct device_desc device_desc;
@@ -11440,6 +11469,32 @@ static void test_lost_device(void)
     hr = IDirect3DDevice9_Present(device, NULL, NULL, NULL, NULL);
     ok(hr == D3DERR_DEVICELOST, "Got unexpected hr %#x.\n", hr);
 
+    if (winetest_interactive)
+    {
+        MSG msg;
+
+        test_lost_device_focus_set = FALSE;
+        test_lost_device_orig_proc = (WNDPROC)SetWindowLongPtrA(window, GWLP_WNDPROC, (LONG_PTR)test_lost_device_proc);
+
+        trace("Please restore the minimized d3d9_test by clicking on it.\n");
+        while (GetMessageA(&msg, NULL, 0, 0) > 0 && !test_lost_device_focus_set)
+        {
+            TranslateMessage(&msg);
+            DispatchMessageA(&msg);
+        }
+        while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE))
+        {
+            TranslateMessage(&msg);
+            DispatchMessageA(&msg);
+        }
+        SetWindowLongPtrA(window, GWLP_WNDPROC, (LONG_PTR)test_lost_device_orig_proc);
+
+        ok(test_lost_device_pos_changed_ok,
+            "WM_WINDOWPOSCHANGED was never sent with fullscreen size while window was not minimized.\n");
+        ret = SetForegroundWindow(GetDesktopWindow());
+        ok(ret, "Failed to set foreground window.\n");
+    }
+
     ret = ShowWindow(window, SW_RESTORE);
     ok(ret, "Failed to restore window.\n");
     ret = SetForegroundWindow(window);
-- 
2.21.0

[PATCH 2/2] d3d9/tests: Add interactive test for window pos messages received when restoring a window after device lost.

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=80468

Your paranoid android.


=== w8adm (32 bit report) ===

d3d9:
device.c:2293: Test failed: TestCooperativeLevel failed, hr 0x88760869.

=== w1064v1809 (32 bit report) ===

d3d9:
device.c:5042: Test failed: Failed to set foreground window.
device.c:5066: Test failed: Failed to set foreground window.
device.c:5069: Test failed: Failed to reset device, hr 0x88760868.
device.c:5042: Test failed: Failed to set foreground window.
device.c:5066: Test failed: Failed to set foreground window.
device.c:11466: Test failed: Failed to set foreground window.
device.c:11468: Test failed: Got unexpected hr 0.
device.c:11470: Test failed: Got unexpected hr 0.
device.c:11501: Test failed: Failed to set foreground window.
device.c:11503: Test failed: Got unexpected hr 0.
device.c:11505: Test failed: Got unexpected hr 0.
device.c:11523: Test failed: Failed to set foreground window.
device.c:11532: Test failed: Failed to set foreground window.
device.c:11547: Test failed: Failed to set foreground window.
device.c:11549: Test failed: Got unexpected hr 0.
device.c:11553: Test failed: Failed to set foreground window.
device.c:14141: Test failed: Adapter 0: SetForegroundWindow failed, error 0.
device.c:14147: Test failed: Adapter 0: SetForegroundWindow failed, error 0.

=== w1064v1809 (64 bit report) ===

d3d9:
device.c:5042: Test failed: Failed to set foreground window.
device.c:5046: Test failed: Expected device window style 0x34cf0000, got 0x14000000, i=1.
device.c:5050: Test failed: Expected device window extended style 0x108, got 0, i=1.
device.c:5066: Test failed: Failed to set foreground window.
device.c:5042: Test failed: Failed to set foreground window.
device.c:5066: Test failed: Failed to set foreground window.
device.c:5069: Test failed: Failed to reset device, hr 0x88760868.
device.c:5042: Test failed: Failed to set foreground window.
device.c:5066: Test failed: Failed to set foreground window.
device.c:11466: Test failed: Failed to set foreground window.
device.c:11468: Test failed: Got unexpected hr 0.
device.c:11470: Test failed: Got unexpected hr 0.
device.c:11501: Test failed: Failed to set foreground window.
device.c:11503: Test failed: Got unexpected hr 0.
device.c:11505: Test failed: Got unexpected hr 0.
device.c:11523: Test failed: Failed to set foreground window.
device.c:11532: Test failed: Failed to set foreground window.
device.c:11547: Test failed: Failed to set foreground window.
device.c:11549: Test failed: Got unexpected hr 0.
device.c:11553: Test failed: Failed to set foreground window.
device.c:14141: Test failed: Adapter 0: SetForegroundWindow failed, error 0.
device.c:14147: Test failed: Adapter 0: SetForegroundWindow failed, error 0.

=== debiant (32 bit report) ===

d3d9:
device.c:14151: Test failed: Adapter 0: Expect window rect (0,0)-(1024,768), got (0,2)-(1024,770).

=== debiant (32 bit French report) ===

d3d9:
device.c:14151: Test failed: Adapter 0: Expect window rect (0,0)-(1024,768), got (0,2)-(1024,770).

=== debiant (32 bit Japanese:Japan report) ===

d3d9:
device.c:14151: Test failed: Adapter 0: Expect window rect (0,0)-(1024,768), got (0,2)-(1024,770).

=== debiant (32 bit Chinese:China report) ===

d3d9:
device.c:14151: Test failed: Adapter 0: Expect window rect (0,0)-(1024,768), got (0,2)-(1024,770).

=== debiant (64 bit WoW report) ===

d3d9:
device.c:14151: Test failed: Adapter 0: Expect window rect (0,0)-(1024,768), got (0,2)-(1024,770).

[Zhiyi Zhang]

Hi Gabriel,

Now that the root cause is in winex11.drv, you should add tests in user32.
For example, minimizing a fullscreen window and then restore it and check its size.
The d3d9 test doesn't seem to be necessary.

On 10/15/20 10:39 PM, Gabriel Ivăncescu wrote:
> Fixes a regression introduced by commit
> 82c6ec3a32f44e8b3e0cc88b7f10e0c0d7fa1b89, which caused the WM_ACTIVATEAPP
> to be sent while the window is minimized, if it has been clicked on in the
> taskbar to be restored.
>
> According to the Extended Window Manager Hints spec, WMs remove the
> NET_WM_STATE_FULLSCREEN state when minimized and then, when restored, use
> the previous size of the window (not the fullscreen) to restore to. This
> caused the WM_SYSCOMMAND SC_RESTORE message's ShowWindow to revert the window
> back to its original (non-fullscreen) size, instead of being restored to
> fullscreen, breaking some apps like Heroes of Might and Magic V.
>
> We have to override the X server's window state here to match Windows
> behavior and restore it back to fullscreen.
>
> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
> ---
>
> Supersedes patches 193752-193754.
>
> update_net_wm_states is when Wine does a change and needs to inform the X
> server about it (i.e. Wine->X11 state change). So, that's good as it is.
>
> read_net_wm_states is when X11 changes some state from outside Wine and
> sends it a notification that it did; then Wine has to update its own wm
> states to match the X server (so it's properly integrated).
>
> However, in this case, since the WM clears the fullscreen state on
> minimization, we must not do that, because Windows doesn't—and more,
> when the window is restored from minimized, we must ignore the X11 state
> changes and force it back to what Wine had (usually fullscreen unless it
> gets hooked by the app, then we keep what the app changed). This is to
> match Windows behavior, which differs from X11, and apps rely on.
>
> dlls/winex11.drv/event.c  | 13 +++++++++++++
>  dlls/winex11.drv/window.c |  9 ++++++++-
>  dlls/winex11.drv/x11drv.h |  1 +
>  3 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/dlls/winex11.drv/event.c b/dlls/winex11.drv/event.c
> index 07f7a1a..350d533 100644
> --- a/dlls/winex11.drv/event.c
> +++ b/dlls/winex11.drv/event.c
> @@ -1296,6 +1296,8 @@ static void handle_wm_state_notify( HWND hwnd, XPropertyEvent *event, BOOL updat
>
> if (data->iconic && data->wm_state == NormalState)  /* restore window */
>      {
> +        DWORD old_state = data->net_wm_state;
> +
>          data->iconic = FALSE;
>          read_net_wm_states( event->display, data );
>          if ((style & WS_CAPTION) == WS_CAPTION && (data->net_wm_state & (1 << NET_WM_STATE_MAXIMIZED)))
> @@ -1313,11 +1315,22 @@ static void handle_wm_state_notify( HWND hwnd, XPropertyEvent *event, BOOL updat
>          {
>              if (style & (WS_MINIMIZE | WS_MAXIMIZE))
>              {
> +                /* if the window was fullscreen before minimized, we have to keep its
> +                   restored state and restore it to its fullscreen state, because WMs
> +                   remove the fullscreen state when minimized, so don't use their state. */
> +                if (old_state & (1 << NET_WM_STATE_FULLSCREEN))
> +                    data->keep_restored_state = TRUE;
> Shouldn't you restrict this path to restore from minimized state only and not from maximized state?

Do you think 'keep_fullscreen_state' is a more appropriate name?
 
> +
>                  TRACE( "restoring win %p/%lx\n", data->hwnd, data->whole_window );
>                  release_win_data( data );
>                  if ((style & (WS_MINIMIZE | WS_VISIBLE)) == (WS_MINIMIZE | WS_VISIBLE))
>                      SetActiveWindow( hwnd );
>                  SendMessageW( hwnd, WM_SYSCOMMAND, SC_RESTORE, 0 );
> +                if ((data = get_win_data( hwnd )))
> +                {
> +                    data->keep_restored_state = FALSE;
> +                    release_win_data( data );
> +                }
> Also restrict it to only when restoring from minimized state. You can move release_win_data( data )
> to avoid a get_win_data() call.
> return;
>              }
>              TRACE( "not restoring win %p/%lx style %08x\n", data->hwnd, data->whole_window, style );
> diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c
> index 4571739..d9d207c 100644
> --- a/dlls/winex11.drv/window.c
> +++ b/dlls/winex11.drv/window.c
> @@ -1076,6 +1076,8 @@ void read_net_wm_states( Display* display, struct x11drv_win_data *data )
>      if (!maximized_horz)
>          new_state &= ~(1 << NET_WM_STATE_MAXIMIZED);
>
> +    if ((data->net_wm_state & (1 << NET_WM_STATE_FULLSCREEN)) && data->iconic)
> +        new_state |= 1 << NET_WM_STATE_FULLSCREEN;

Please add some comments here.
> data->net_wm_state = new_state;
>  }
>
> @@ -2450,6 +2452,11 @@ void CDECL X11DRV_WindowPosChanged( HWND hwnd, HWND insert_after, UINT swp_flags
>          if (event_type != ConfigureNotify && event_type != PropertyNotify &&
>              event_type != GravityNotify && event_type != ReparentNotify)
>              event_type = 0;  /* ignore other events */
> +
> +        /* if we keep the Wine window's restored state, we pretend it's
> +           not an event, so that it's synced properly with the X server. */
> +        if (data->keep_restored_state && !(new_style & WS_MINIMIZE))
> +            event_type = 0;

Is this really needed? Can this happen when handling ConfigureNotify?
> }
>
> if (data->mapped && event_type != ReparentNotify)
> @@ -2545,7 +2552,7 @@ UINT CDECL X11DRV_ShowWindow( HWND hwnd, INT cmd, RECT *rect, UINT swp )
>          }
>          goto done;
>      }
> -    if (!data->managed || !data->mapped || data->iconic) goto done;
> +    if (!data->managed || !data->mapped || data->iconic || data->keep_restored_state) goto done;

Same here.

Thanks,
Zhiyi
> /* only fetch the new rectangle if the ShowWindow was a result of a window manager event */
>
> diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h
> index 173d94b..de725d9 100644
> --- a/dlls/winex11.drv/x11drv.h
> +++ b/dlls/winex11.drv/x11drv.h
> @@ -574,6 +574,7 @@ struct x11drv_win_data
>      BOOL        shaped : 1;     /* is window using a custom region shape? */
>      BOOL        layered : 1;    /* is window layered and with valid attributes? */
>      BOOL        use_alpha : 1;  /* does window use an alpha channel? */
> +    BOOL        keep_restored_state : 1; /* don't override the window state when restoring from an event */
>      int         wm_state;       /* current value of the WM_STATE property */
>      DWORD       net_wm_state;   /* bit mask of active x11drv_net_wm_state values */
>      Window      embedder;       /* window id of embedder */

[Gabriel Ivăncescu]

Hi Zhiyi,

Thanks for the review.

On 21/10/2020 13:10, Zhiyi Zhang wrote:
> Hi Gabriel,
>
> Now that the root cause is in winex11.drv, you should add tests in user32.
> For example, minimizing a fullscreen window and then restore it and check its size.
> The d3d9 test doesn't seem to be necessary.

It would still have to be interactive, though, because it has to happen 
within an X11 event (such as clicking on the taskbar). Otherwise the bug 
doesn't manifest. But yeah, I'll try to move it.
> On 10/15/20 10:39 PM, Gabriel Ivăncescu wrote:
> > Fixes a regression introduced by commit
> > 82c6ec3a32f44e8b3e0cc88b7f10e0c0d7fa1b89, which caused the WM_ACTIVATEAPP
> > to be sent while the window is minimized, if it has been clicked on in the
> > taskbar to be restored.
> >
> > According to the Extended Window Manager Hints spec, WMs remove the
> > NET_WM_STATE_FULLSCREEN state when minimized and then, when restored, use
> > the previous size of the window (not the fullscreen) to restore to. This
> > caused the WM_SYSCOMMAND SC_RESTORE message's ShowWindow to revert the window
> > back to its original (non-fullscreen) size, instead of being restored to
> > fullscreen, breaking some apps like Heroes of Might and Magic V.
> >
> > We have to override the X server's window state here to match Windows
> > behavior and restore it back to fullscreen.
> >
> > Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
> > ---
> >
> > Supersedes patches 193752-193754.
> >
> > update_net_wm_states is when Wine does a change and needs to inform the X
> > server about it (i.e. Wine->X11 state change). So, that's good as it is.
> >
> > read_net_wm_states is when X11 changes some state from outside Wine and
> > sends it a notification that it did; then Wine has to update its own wm
> > states to match the X server (so it's properly integrated).
> >
> > However, in this case, since the WM clears the fullscreen state on
> > minimization, we must not do that, because Windows doesn't—and more,
> > when the window is restored from minimized, we must ignore the X11 state
> > changes and force it back to what Wine had (usually fullscreen unless it
> > gets hooked by the app, then we keep what the app changed). This is to
> > match Windows behavior, which differs from X11, and apps rely on.
> >
> > dlls/winex11.drv/event.c  | 13 +++++++++++++
> >   dlls/winex11.drv/window.c |  9 ++++++++-
> >   dlls/winex11.drv/x11drv.h |  1 +
> >   3 files changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/dlls/winex11.drv/event.c b/dlls/winex11.drv/event.c
> > index 07f7a1a..350d533 100644
> > --- a/dlls/winex11.drv/event.c
> > +++ b/dlls/winex11.drv/event.c
> > @@ -1296,6 +1296,8 @@ static void handle_wm_state_notify( HWND hwnd, XPropertyEvent *event, BOOL updat
> >
> > if (data->iconic && data->wm_state == NormalState)  /* restore window */
> >       {
> > +        DWORD old_state = data->net_wm_state;
> > +
> >           data->iconic = FALSE;
> >           read_net_wm_states( event->display, data );
> >           if ((style & WS_CAPTION) == WS_CAPTION && (data->net_wm_state & (1 << NET_WM_STATE_MAXIMIZED)))
> > @@ -1313,11 +1315,22 @@ static void handle_wm_state_notify( HWND hwnd, XPropertyEvent *event, BOOL updat
> >           {
> >               if (style & (WS_MINIMIZE | WS_MAXIMIZE))
> >               {
> > +                /* if the window was fullscreen before minimized, we have to keep its
> > +                   restored state and restore it to its fullscreen state, because WMs
> > +                   remove the fullscreen state when minimized, so don't use their state. */
> > +                if (old_state & (1 << NET_WM_STATE_FULLSCREEN))
> > +                    data->keep_restored_state = TRUE;
> > Shouldn't you restrict this path to restore from minimized state only and not from maximized state?
>
> Do you think 'keep_fullscreen_state' is a more appropriate name?

Might be, I tried to keep the name on the shorter side, and also it's 
possible that the app may hook and change the restored state (and we 
keep that). However, I also prefer "keep_fullscreen_state" so I maybe go 
with that.
> > +
> >                   TRACE( "restoring win %p/%lx\n", data->hwnd, data->whole_window );
> >                   release_win_data( data );
> >                   if ((style & (WS_MINIMIZE | WS_VISIBLE)) == (WS_MINIMIZE | WS_VISIBLE))
> >                       SetActiveWindow( hwnd );
> >                   SendMessageW( hwnd, WM_SYSCOMMAND, SC_RESTORE, 0 );
> > +                if ((data = get_win_data( hwnd )))
> > +                {
> > +                    data->keep_restored_state = FALSE;
> > +                    release_win_data( data );
> > +                }
> > Also restrict it to only when restoring from minimized state. You can move release_win_data( data )
> > to avoid a get_win_data() call.

Sorry I don't get the part about restoring from minimized state. You 
mean checking for WS_MINIMIZE? We already check data->iconic which must 
be TRUE. And after all, that's the issue here (the X11 "minimized" 
state, not the Windows side). I don't think it's correct checking for 
WS_MINIMIZE at all.

It's true that SetActiveWindow causes the regression, but that's because 
d3d9's WM_ACTIVATEAPP resizes the window to fullscreen, which "saved" it 
by luck before. This bug, however, can manifest outside of d3d9 too, so 
it's not correct to only handle that case, even if that was the original 
motivation.

It's not a good idea to move release_win_data either, because those 
functions send messages and the app can potentially do crazy stuff in 
them. We don't want to stay in the critical section in that case.

However, to avoid entering the critical section when it's not needed, 
I'll add a check for NET_WM_STATE_FULLSCREEN the second time also.
> > return;
> >               }
> >               TRACE( "not restoring win %p/%lx style %08x\n", data->hwnd, data->whole_window, style );
> > diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c
> > index 4571739..d9d207c 100644
> > --- a/dlls/winex11.drv/window.c
> > +++ b/dlls/winex11.drv/window.c
> > @@ -1076,6 +1076,8 @@ void read_net_wm_states( Display* display, struct x11drv_win_data *data )
> >       if (!maximized_horz)
> >           new_state &= ~(1 << NET_WM_STATE_MAXIMIZED);
> >
> > +    if ((data->net_wm_state & (1 << NET_WM_STATE_FULLSCREEN)) && data->iconic)
> > +        new_state |= 1 << NET_WM_STATE_FULLSCREEN;
>
> Please add some comments here.

Noted.
> > data->net_wm_state = new_state;
> >   }
> >
> > @@ -2450,6 +2452,11 @@ void CDECL X11DRV_WindowPosChanged( HWND hwnd, HWND insert_after, UINT swp_flags
> >           if (event_type != ConfigureNotify && event_type != PropertyNotify &&
> >               event_type != GravityNotify && event_type != ReparentNotify)
> >               event_type = 0;  /* ignore other events */
> > +
> > +        /* if we keep the Wine window's restored state, we pretend it's
> > +           not an event, so that it's synced properly with the X server. */
> > +        if (data->keep_restored_state && !(new_style & WS_MINIMIZE))
> > +            event_type = 0;
>
> Is this really needed? Can this happen when handling ConfigureNotify?

Yes. Consider a d3d9 application that hooks WM_ACTIVATEAPP and sends 
SetWindowPos from it (this is correct wrt to Windows, too). In this 
case, the window is WS_MINIMIZE while that happens, so we shouldn't 
reconfigure (sync) the X window, because on the Win32 side, SetWindowPos 
on a minimized window does nothing.

So even though 'keep_restored_state' is true, we still can't treat it as 
not an event, unless it's not minimized on the Windows side.

Because not being an event just reconfigures the X window to match Wine.
> > }
> >
> > if (data->mapped && event_type != ReparentNotify)
> > @@ -2545,7 +2552,7 @@ UINT CDECL X11DRV_ShowWindow( HWND hwnd, INT cmd, RECT *rect, UINT swp )
> >           }
> >           goto done;
> >       }
> > -    if (!data->managed || !data->mapped || data->iconic) goto done;
> > +    if (!data->managed || !data->mapped || data->iconic || data->keep_restored_state) goto done;
>
> Same here.

You mean adding a comment? Or whether it can happen? If this is the 
latter, this is obviously the most important part of the patch, because 
it *is* the root cause of the issue.

Without this check, ShowWindow overrides the state to the X server one, 
which is the non-fullscreen state (based on the spec) and that's 
obviously wrong and mismatches Windows.

The reason for the *previous* hook treating it as a non-event is 
because, if we *don't* override to the X state, then the Wine state 
(which is fullscreen for example) mismatches the X state. X server 
believes the window is restored to non-fullscreen since that's what it 
did, but Wine thinks it is fullscreen still. We don't sync it to be the 
X state, so we instead have to sync the X server to match Wine (i.e. we 
have to tell it to be fullscreen, instead).

And this sync (from Wine to X11) only happens when there's no event, 
since usually it's when Wine changes the window state via some API (not 
an event from outside Wine).

Thanks,
Gabriel

[Zhiyi Zhang]

On 10/21/20 8:14 PM, Gabriel Ivăncescu wrote:
> Hi Zhiyi,
>
> Thanks for the review.
>
> On 21/10/2020 13:10, Zhiyi Zhang wrote:
> > Hi Gabriel,
> >
> > Now that the root cause is in winex11.drv, you should add tests in user32.
> > For example, minimizing a fullscreen window and then restore it and check its size.
> > The d3d9 test doesn't seem to be necessary.
>
> It would still have to be interactive, though, because it has to happen within an X11 event (such as clicking on the taskbar). Otherwise the bug doesn't manifest. But yeah, I'll try to move it.
> You mentioned that Alt+Tab have the same effect. Maybe you can use that to implement a non-interactive test.
> For example, send Alt+Tab message and then Alt+Tab back, with a timeout waiting for the result.
> > On 10/15/20 10:39 PM, Gabriel Ivăncescu wrote:
> > > Fixes a regression introduced by commit
> > > 82c6ec3a32f44e8b3e0cc88b7f10e0c0d7fa1b89, which caused the WM_ACTIVATEAPP
> > > to be sent while the window is minimized, if it has been clicked on in the
> > > taskbar to be restored.
> > >
> > > According to the Extended Window Manager Hints spec, WMs remove the
> > > NET_WM_STATE_FULLSCREEN state when minimized and then, when restored, use
> > > the previous size of the window (not the fullscreen) to restore to. This
> > > caused the WM_SYSCOMMAND SC_RESTORE message's ShowWindow to revert the window
> > > back to its original (non-fullscreen) size, instead of being restored to
> > > fullscreen, breaking some apps like Heroes of Might and Magic V.
> > >
> > > We have to override the X server's window state here to match Windows
> > > behavior and restore it back to fullscreen.
> > >
> > > Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
> > > ---
> > >
> > > Supersedes patches 193752-193754.
> > >
> > > update_net_wm_states is when Wine does a change and needs to inform the X
> > > server about it (i.e. Wine->X11 state change). So, that's good as it is.
> > >
> > > read_net_wm_states is when X11 changes some state from outside Wine and
> > > sends it a notification that it did; then Wine has to update its own wm
> > > states to match the X server (so it's properly integrated).
> > >
> > > However, in this case, since the WM clears the fullscreen state on
> > > minimization, we must not do that, because Windows doesn't—and more,
> > > when the window is restored from minimized, we must ignore the X11 state
> > > changes and force it back to what Wine had (usually fullscreen unless it
> > > gets hooked by the app, then we keep what the app changed). This is to
> > > match Windows behavior, which differs from X11, and apps rely on.
> > >
> > >   dlls/winex11.drv/event.c  | 13 +++++++++++++
> > >   dlls/winex11.drv/window.c |  9 ++++++++-
> > >   dlls/winex11.drv/x11drv.h |  1 +
> > >   3 files changed, 22 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/dlls/winex11.drv/event.c b/dlls/winex11.drv/event.c
> > > index 07f7a1a..350d533 100644
> > > --- a/dlls/winex11.drv/event.c
> > > +++ b/dlls/winex11.drv/event.c
> > > @@ -1296,6 +1296,8 @@ static void handle_wm_state_notify( HWND hwnd, XPropertyEvent *event, BOOL updat
> > >         if (data->iconic && data->wm_state == NormalState)  /* restore window */
> > >       {
> > > +        DWORD old_state = data->net_wm_state;
> > > +
> > >           data->iconic = FALSE;
> > >           read_net_wm_states( event->display, data );
> > >           if ((style & WS_CAPTION) == WS_CAPTION && (data->net_wm_state & (1 << NET_WM_STATE_MAXIMIZED)))
> > > @@ -1313,11 +1315,22 @@ static void handle_wm_state_notify( HWND hwnd, XPropertyEvent *event, BOOL updat
> > >           {
> > >               if (style & (WS_MINIMIZE | WS_MAXIMIZE))
> > >               {
> > > +                /* if the window was fullscreen before minimized, we have to keep its
> > > +                   restored state and restore it to its fullscreen state, because WMs
> > > +                   remove the fullscreen state when minimized, so don't use their state. */
> > > +                if (old_state & (1 << NET_WM_STATE_FULLSCREEN))
> > > +                    data->keep_restored_state = TRUE;
> > > Shouldn't you restrict this path to restore from minimized state only and not from maximized state?
> >
> > Do you think 'keep_fullscreen_state' is a more appropriate name?
> >  
>
> Might be, I tried to keep the name on the shorter side, and also it's possible that the app may hook and change the restored state (and we keep that). However, I also prefer "keep_fullscreen_state" so I maybe go with that.
> > > +
> > >                   TRACE( "restoring win %p/%lx\n", data->hwnd, data->whole_window );
> > >                   release_win_data( data );
> > >                   if ((style & (WS_MINIMIZE | WS_VISIBLE)) == (WS_MINIMIZE | WS_VISIBLE))
> > >                       SetActiveWindow( hwnd );
> > >                   SendMessageW( hwnd, WM_SYSCOMMAND, SC_RESTORE, 0 );
> > > +                if ((data = get_win_data( hwnd )))
> > > +                {
> > > +                    data->keep_restored_state = FALSE;
> > > +                    release_win_data( data );
> > > +                }
> > > Also restrict it to only when restoring from minimized state. You can move release_win_data( data )
> > > to avoid a get_win_data() call.
>
> Sorry I don't get the part about restoring from minimized state. You mean checking for WS_MINIMIZE? We already check data->iconic which must be TRUE. And after all, that's the issue here (the X11 "minimized" state, not the Windows side). I don't think it's correct checking for WS_MINIMIZE at all.

I mean it can restore from maximized state, i.e., restoring a maximized window to a normal sized window.
> It's true that SetActiveWindow causes the regression, but that's because d3d9's WM_ACTIVATEAPP resizes the window to fullscreen, which "saved" it by luck before. This bug, however, can manifest outside of d3d9 too, so it's not correct to only handle that case, even if that was the original motivation.
>
> It's not a good idea to move release_win_data either, because those functions send messages and the app can potentially do crazy stuff in them. We don't want to stay in the critical section in that case.
>
> However, to avoid entering the critical section when it's not needed, I'll add a check for NET_WM_STATE_FULLSCREEN the second time also.
> > >                   return;
> > >               }
> > >               TRACE( "not restoring win %p/%lx style %08x\n", data->hwnd, data->whole_window, style );
> > > diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c
> > > index 4571739..d9d207c 100644
> > > --- a/dlls/winex11.drv/window.c
> > > +++ b/dlls/winex11.drv/window.c
> > > @@ -1076,6 +1076,8 @@ void read_net_wm_states( Display* display, struct x11drv_win_data *data )
> > >       if (!maximized_horz)
> > >           new_state &= ~(1 << NET_WM_STATE_MAXIMIZED);
> > >   +    if ((data->net_wm_state & (1 << NET_WM_STATE_FULLSCREEN)) && data->iconic)
> > > +        new_state |= 1 << NET_WM_STATE_FULLSCREEN;
> > > Please add some comments here.
>
> Noted.
> > >       data->net_wm_state = new_state;
> > >   }
> > >   @@ -2450,6 +2452,11 @@ void CDECL X11DRV_WindowPosChanged( HWND hwnd, HWND insert_after, UINT swp_flags
> > >           if (event_type != ConfigureNotify && event_type != PropertyNotify &&
> > >               event_type != GravityNotify && event_type != ReparentNotify)
> > >               event_type = 0;  /* ignore other events */
> > > +
> > > +        /* if we keep the Wine window's restored state, we pretend it's
> > > +           not an event, so that it's synced properly with the X server. */
> > > +        if (data->keep_restored_state && !(new_style & WS_MINIMIZE))
> > > +            event_type = 0;
> > > Is this really needed? Can this happen when handling ConfigureNotify?
>
> Yes. Consider a d3d9 application that hooks WM_ACTIVATEAPP and sends SetWindowPos from it (this is correct wrt to Windows, too). In this case, the window is WS_MINIMIZE while that happens, so we shouldn't reconfigure (sync) the X window, because on the Win32 side, SetWindowPos on a minimized window does nothing.
>
> So even though 'keep_restored_state' is true, we still can't treat it as not an event, unless it's not minimized on the Windows side.
>
> Because not being an event just reconfigures the X window to match Wine.
> > >       }
> > >         if (data->mapped && event_type != ReparentNotify)
> > > @@ -2545,7 +2552,7 @@ UINT CDECL X11DRV_ShowWindow( HWND hwnd, INT cmd, RECT *rect, UINT swp )
> > >           }
> > >           goto done;
> > >       }
> > > -    if (!data->managed || !data->mapped || data->iconic) goto done;
> > > +    if (!data->managed || !data->mapped || data->iconic || data->keep_restored_state) goto done;
> > > Same here.
>
> You mean adding a comment? Or whether it can happen? If this is the latter, this is obviously the most important part of the patch, because it *is* the root cause of the issue.
>
> Without this check, ShowWindow overrides the state to the X server one, which is the non-fullscreen state (based on the spec) and that's obviously wrong and mismatches Windows.
>
> The reason for the *previous* hook treating it as a non-event is because, if we *don't* override to the X state, then the Wine state (which is fullscreen for example) mismatches the X state. X server believes the window is restored to non-fullscreen since that's what it did, but Wine thinks it is fullscreen still. We don't sync it to be the X state, so we instead have to sync the X server to match Wine (i.e. we have to tell it to be fullscreen, instead).
>
> And this sync (from Wine to X11) only happens when there's no event, since usually it's when Wine changes the window state via some API (not an event from outside Wine).
>
> Thanks,
> Gabriel

[Gabriel Ivăncescu]

On 21/10/2020 16:59, Zhiyi Zhang wrote:
> On 10/21/20 8:14 PM, Gabriel Ivăncescu wrote:
> > Hi Zhiyi,
> >
> > Thanks for the review.
> >
> > On 21/10/2020 13:10, Zhiyi Zhang wrote:
> > > Hi Gabriel,
> > >
> > > Now that the root cause is in winex11.drv, you should add tests in user32.
> > > For example, minimizing a fullscreen window and then restore it and check its size.
> > > The d3d9 test doesn't seem to be necessary.
> >
> > It would still have to be interactive, though, because it has to happen within an X11 event (such as clicking on the taskbar). Otherwise the bug doesn't manifest. But yeah, I'll try to move it.
> > You mentioned that Alt+Tab have the same effect. Maybe you can use that to implement a non-interactive test.
> > For example, send Alt+Tab message and then Alt+Tab back, with a timeout waiting for the result.

Yeah, but the Alt-Tab I was talking about was the one integrated into 
the WM or DE. Not an alt-tab from Wine, which didn't even receive it. 
Does a synthetic Alt-Tab from Wine generate X state notify events?

I somehow doubt it will work, unfortunately.
> > > On 10/15/20 10:39 PM, Gabriel Ivăncescu wrote:
> > > > Fixes a regression introduced by commit
> > > > 82c6ec3a32f44e8b3e0cc88b7f10e0c0d7fa1b89, which caused the WM_ACTIVATEAPP
> > > > to be sent while the window is minimized, if it has been clicked on in the
> > > > taskbar to be restored.
> > > >
> > > > According to the Extended Window Manager Hints spec, WMs remove the
> > > > NET_WM_STATE_FULLSCREEN state when minimized and then, when restored, use
> > > > the previous size of the window (not the fullscreen) to restore to. This
> > > > caused the WM_SYSCOMMAND SC_RESTORE message's ShowWindow to revert the window
> > > > back to its original (non-fullscreen) size, instead of being restored to
> > > > fullscreen, breaking some apps like Heroes of Might and Magic V.
> > > >
> > > > We have to override the X server's window state here to match Windows
> > > > behavior and restore it back to fullscreen.
> > > >
> > > > Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
> > > > ---
> > > >
> > > > Supersedes patches 193752-193754.
> > > >
> > > > update_net_wm_states is when Wine does a change and needs to inform the X
> > > > server about it (i.e. Wine->X11 state change). So, that's good as it is.
> > > >
> > > > read_net_wm_states is when X11 changes some state from outside Wine and
> > > > sends it a notification that it did; then Wine has to update its own wm
> > > > states to match the X server (so it's properly integrated).
> > > >
> > > > However, in this case, since the WM clears the fullscreen state on
> > > > minimization, we must not do that, because Windows doesn't—and more,
> > > > when the window is restored from minimized, we must ignore the X11 state
> > > > changes and force it back to what Wine had (usually fullscreen unless it
> > > > gets hooked by the app, then we keep what the app changed). This is to
> > > > match Windows behavior, which differs from X11, and apps rely on.
> > > >
> > > >   dlls/winex11.drv/event.c  | 13 +++++++++++++
> > > >    dlls/winex11.drv/window.c |  9 ++++++++-
> > > >    dlls/winex11.drv/x11drv.h |  1 +
> > > >    3 files changed, 22 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/dlls/winex11.drv/event.c b/dlls/winex11.drv/event.c
> > > > index 07f7a1a..350d533 100644
> > > > --- a/dlls/winex11.drv/event.c
> > > > +++ b/dlls/winex11.drv/event.c
> > > > @@ -1296,6 +1296,8 @@ static void handle_wm_state_notify( HWND hwnd, XPropertyEvent *event, BOOL updat
> > > >          if (data->iconic && data->wm_state == NormalState)  /* restore window */
> > > >        {
> > > > +        DWORD old_state = data->net_wm_state;
> > > > +
> > > >            data->iconic = FALSE;
> > > >            read_net_wm_states( event->display, data );
> > > >            if ((style & WS_CAPTION) == WS_CAPTION && (data->net_wm_state & (1 << NET_WM_STATE_MAXIMIZED)))
> > > > @@ -1313,11 +1315,22 @@ static void handle_wm_state_notify( HWND hwnd, XPropertyEvent *event, BOOL updat
> > > >            {
> > > >                if (style & (WS_MINIMIZE | WS_MAXIMIZE))
> > > >                {
> > > > +                /* if the window was fullscreen before minimized, we have to keep its
> > > > +                   restored state and restore it to its fullscreen state, because WMs
> > > > +                   remove the fullscreen state when minimized, so don't use their state. */
> > > > +                if (old_state & (1 << NET_WM_STATE_FULLSCREEN))
> > > > +                    data->keep_restored_state = TRUE;
> > > > Shouldn't you restrict this path to restore from minimized state only and not from maximized state?
> > >
> > > Do you think 'keep_fullscreen_state' is a more appropriate name?
> >
> > Might be, I tried to keep the name on the shorter side, and also it's possible that the app may hook and change the restored state (and we keep that). However, I also prefer "keep_fullscreen_state" so I maybe go with that.
> > > > +
> > > >                    TRACE( "restoring win %p/%lx\n", data->hwnd, data->whole_window );
> > > >                    release_win_data( data );
> > > >                    if ((style & (WS_MINIMIZE | WS_VISIBLE)) == (WS_MINIMIZE | WS_VISIBLE))
> > > >                        SetActiveWindow( hwnd );
> > > >                    SendMessageW( hwnd, WM_SYSCOMMAND, SC_RESTORE, 0 );
> > > > +                if ((data = get_win_data( hwnd )))
> > > > +                {
> > > > +                    data->keep_restored_state = FALSE;
> > > > +                    release_win_data( data );
> > > > +                }
> > > > Also restrict it to only when restoring from minimized state. You can move release_win_data( data )
> > > > to avoid a get_win_data() call.
> >
> > Sorry I don't get the part about restoring from minimized state. You mean checking for WS_MINIMIZE? We already check data->iconic which must be TRUE. And after all, that's the issue here (the X11 "minimized" state, not the Windows side). I don't think it's correct checking for WS_MINIMIZE at all.
>
> I mean it can restore from maximized state, i.e., restoring a maximized window to a normal sized window.

Oh, I thought data->iconic meant the window was minimized. So, I should 
check for (style & WS_MINIMIZE), right?

[Zhiyi Zhang]

On 10/21/20 11:37 PM, Gabriel Ivăncescu wrote:
> On 21/10/2020 16:59, Zhiyi Zhang wrote:
> > On 10/21/20 8:14 PM, Gabriel Ivăncescu wrote:
> > > Hi Zhiyi,
> > >
> > > Thanks for the review.
> > >
> > > On 21/10/2020 13:10, Zhiyi Zhang wrote:
> > > > Hi Gabriel,
> > > >
> > > > Now that the root cause is in winex11.drv, you should add tests in user32.
> > > > For example, minimizing a fullscreen window and then restore it and check its size.
> > > > The d3d9 test doesn't seem to be necessary.
> > >
> > > It would still have to be interactive, though, because it has to happen within an X11 event (such as clicking on the taskbar). Otherwise the bug doesn't manifest. But yeah, I'll try to move it.
> > > You mentioned that Alt+Tab have the same effect. Maybe you can use that to implement a non-interactive test.
> > > For example, send Alt+Tab message and then Alt+Tab back, with a timeout waiting for the result.
>
> Yeah, but the Alt-Tab I was talking about was the one integrated into the WM or DE. Not an alt-tab from Wine, which didn't even receive it. Does a synthetic Alt-Tab from Wine generate X state notify events?
> Oh, right, I forgot wine's explorer.exe doesn't support Alt+Tab yet.
> I somehow doubt it will work, unfortunately.
> > > > On 10/15/20 10:39 PM, Gabriel Ivăncescu wrote:
> > > > > Fixes a regression introduced by commit
> > > > > 82c6ec3a32f44e8b3e0cc88b7f10e0c0d7fa1b89, which caused the WM_ACTIVATEAPP
> > > > > to be sent while the window is minimized, if it has been clicked on in the
> > > > > taskbar to be restored.
> > > > >
> > > > > According to the Extended Window Manager Hints spec, WMs remove the
> > > > > NET_WM_STATE_FULLSCREEN state when minimized and then, when restored, use
> > > > > the previous size of the window (not the fullscreen) to restore to. This
> > > > > caused the WM_SYSCOMMAND SC_RESTORE message's ShowWindow to revert the window
> > > > > back to its original (non-fullscreen) size, instead of being restored to
> > > > > fullscreen, breaking some apps like Heroes of Might and Magic V.
> > > > >
> > > > > We have to override the X server's window state here to match Windows
> > > > > behavior and restore it back to fullscreen.
> > > > >
> > > > > Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
> > > > > ---
> > > > >
> > > > > Supersedes patches 193752-193754.
> > > > >
> > > > > update_net_wm_states is when Wine does a change and needs to inform the X
> > > > > server about it (i.e. Wine->X11 state change). So, that's good as it is.
> > > > >
> > > > > read_net_wm_states is when X11 changes some state from outside Wine and
> > > > > sends it a notification that it did; then Wine has to update its own wm
> > > > > states to match the X server (so it's properly integrated).
> > > > >
> > > > > However, in this case, since the WM clears the fullscreen state on
> > > > > minimization, we must not do that, because Windows doesn't—and more,
> > > > > when the window is restored from minimized, we must ignore the X11 state
> > > > > changes and force it back to what Wine had (usually fullscreen unless it
> > > > > gets hooked by the app, then we keep what the app changed). This is to
> > > > > match Windows behavior, which differs from X11, and apps rely on.
> > > > >
> > > > >    dlls/winex11.drv/event.c  | 13 +++++++++++++
> > > > >    dlls/winex11.drv/window.c |  9 ++++++++-
> > > > >    dlls/winex11.drv/x11drv.h |  1 +
> > > > >    3 files changed, 22 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/dlls/winex11.drv/event.c b/dlls/winex11.drv/event.c
> > > > > index 07f7a1a..350d533 100644
> > > > > --- a/dlls/winex11.drv/event.c
> > > > > +++ b/dlls/winex11.drv/event.c
> > > > > @@ -1296,6 +1296,8 @@ static void handle_wm_state_notify( HWND hwnd, XPropertyEvent *event, BOOL updat
> > > > >          if (data->iconic && data->wm_state == NormalState)  /* restore window */
> > > > >        {
> > > > > +        DWORD old_state = data->net_wm_state;
> > > > > +
> > > > >            data->iconic = FALSE;
> > > > >            read_net_wm_states( event->display, data );
> > > > >            if ((style & WS_CAPTION) == WS_CAPTION && (data->net_wm_state & (1 << NET_WM_STATE_MAXIMIZED)))
> > > > > @@ -1313,11 +1315,22 @@ static void handle_wm_state_notify( HWND hwnd, XPropertyEvent *event, BOOL updat
> > > > >            {
> > > > >                if (style & (WS_MINIMIZE | WS_MAXIMIZE))
> > > > >                {
> > > > > +                /* if the window was fullscreen before minimized, we have to keep its
> > > > > +                   restored state and restore it to its fullscreen state, because WMs
> > > > > +                   remove the fullscreen state when minimized, so don't use their state. */
> > > > > +                if (old_state & (1 << NET_WM_STATE_FULLSCREEN))
> > > > > +                    data->keep_restored_state = TRUE;
> > > > > Shouldn't you restrict this path to restore from minimized state only and not from maximized state?
> > > >
> > > > Do you think 'keep_fullscreen_state' is a more appropriate name?
> > > >   
> > >
> > > Might be, I tried to keep the name on the shorter side, and also it's possible that the app may hook and change the restored state (and we keep that). However, I also prefer "keep_fullscreen_state" so I maybe go with that.
> > > > > +
> > > > >                    TRACE( "restoring win %p/%lx\n", data->hwnd, data->whole_window );
> > > > >                    release_win_data( data );
> > > > >                    if ((style & (WS_MINIMIZE | WS_VISIBLE)) == (WS_MINIMIZE | WS_VISIBLE))
> > > > >                        SetActiveWindow( hwnd );
> > > > >                    SendMessageW( hwnd, WM_SYSCOMMAND, SC_RESTORE, 0 );
> > > > > +                if ((data = get_win_data( hwnd )))
> > > > > +                {
> > > > > +                    data->keep_restored_state = FALSE;
> > > > > +                    release_win_data( data );
> > > > > +                }
> > > > > Also restrict it to only when restoring from minimized state. You can move release_win_data( data )
> > > > > to avoid a get_win_data() call.
> > >
> > > Sorry I don't get the part about restoring from minimized state. You mean checking for WS_MINIMIZE? We already check data->iconic which must be TRUE. And after all, that's the issue here (the X11 "minimized" state, not the Windows side). I don't think it's correct checking for WS_MINIMIZE at all.
> >
> > I mean it can restore from maximized state, i.e., restoring a maximized window to a normal sized window.
>
> Oh, I thought data->iconic meant the window was minimized. So, I should check for (style & WS_MINIMIZE), right?

Yes