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

[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 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?
>>>    
>>
>> 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?