Re: [PATCH v5] user32: Don't save maximized position placement for toplevel windows covering the entire work area.

[Gabriel Ivăncescu]

On 26/08/2021 09:47, Huw Davies wrote:
> On Wed, Aug 25, 2021 at 06:16:47PM +0300, Gabriel Ivăncescu wrote:
> > Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51672
> > Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
>
> I meant to mention this earlier but the commit summary message is a
> bit long.  Simply deleting the word "placement" would help.
> > --- a/dlls/user32/winpos.c
> > +++ b/dlls/user32/winpos.c
> > @@ -675,6 +675,33 @@ BOOL WINAPI MoveWindow( HWND hwnd, INT x, INT y, INT cx, INT cy,
> >   }
> >
> > +/*******************************************************************
> > + *           get_maximized_area_rect
> > + *
> > + * Get the work area that a maximized window can cover, depending on style.
> > + */
> > +static BOOL get_work_rect_for_window( HWND hwnd, RECT *rect )
>
> I'd call this get_work_rect (I don't thing the "for_window" adds anything.
> Also, note that the comment above is calling this something else.
> > +{
> > +    HMONITOR monitor = MonitorFromWindow( hwnd, MONITOR_DEFAULTTOPRIMARY );
> > +    MONITORINFO mon_info;
> > +    DWORD style;
> > +
> > +    if (!monitor) return FALSE;
> > +
> > +    mon_info.cbSize = sizeof(mon_info);
> > +    GetMonitorInfoW( monitor, &mon_info );
> > +
> > +    style = GetWindowLongW( hwnd, GWL_STYLE );
> > +    *rect = mon_info.rcMonitor;
> > +    if (style & WS_MAXIMIZEBOX)
> > +    {
> > +        if ((style & WS_CAPTION) == WS_CAPTION || !(style & (WS_CHILD | WS_POPUP)))
> > +            *rect = mon_info.rcWork;
> > +    }
> > +    return TRUE;
> > +}
> > +
> > +
> >   /*******************************************************************
> >    *           WINPOS_GetMinMaxInfo
> >    *
> > @@ -916,6 +929,35 @@ static POINT get_minimized_pos( HWND hwnd, POINT pt )
> >       return pt;
> >   }
> >
> > +static void update_maximized_pos( WND *wnd )
> > +{
> > +    RECT rc_work;
> > +
> > +    /* For top level windows covering the work area, we might have
> > +       to "forget" the maximized position. Windows presumably does
> > +       this to avoid situations where the border style changes,
> > +       which would lead the window to be outside the screen, or the
> > +       window gets reloaded on a different screen, and the "saved"
> > +       position no longer applies to it (despite being maximized).
> > +
> > +       Some applications (e.g. Imperiums: Greek Wars) depend on this.
> > +    */
> > +    if (wnd->parent && wnd->parent != GetDesktopWindow())
> > +        return;
> > +
> > +    if (wnd->dwStyle & WS_MAXIMIZE)
> > +    {
> > +        if (!get_work_rect_for_window( wnd->obj.handle, &rc_work ))
> > +            SetRect( &rc_work, 0, 0, GetSystemMetrics( SM_CXSCREEN ), GetSystemMetrics( SM_CYSCREEN ) );
>
> As you pointed out in another email, you now have the user lock.  It
> doesn't seem very hard to avoid that, simply call get_work_rect()
> before taking the lock and pass the work rect to this function.
> There's a small wrinkle in that GetMonitorWindow() calls
> GetWindowPlacement() if the window is iconic, so you wouldn't
> want to call get_work_rect() in that case.
>
> Huw.

Thanks, I didn't really like the IsIconic thing, it seemed fragile, so I 
went with a slightly different approach, which also makes 
update_maximized_pos more typical like the other helpers, in that it 
takes a HWND rather than a WND, and does all the implementation details 
within. In one call site I just moved the call outside the lock, but for 
the other I had to release/re-acquire the lock, I hope it's not too big 
of a deal. It's still less code than special casing the work rect it in 
both callers.

I sent it so you can see if you like it better, if not I'll revert it to 
your suggestion.

Thanks,
Gabriel