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

[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 at 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 */