[PATCH] winemac: Fix macdrv_WindowPosChanged behavior when SWP_HIDEWINDOW flag is used.

[Piotr Caban]

In this case surface points to dummy surface or NULL. Later e.g. set_window_surface assumes that it was created by Mac driver.

Signed-off-by: Piotr Caban 
---
dlls/winemac.drv/window.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

[Ken Thomases]

On Mar 9, 2020, at 3:56 PM, Piotr Caban  wrote:
> In this case surface points to dummy surface or NULL. Later e.g. set_window_surface assumes that it was created by Mac driver.
> diff --git a/dlls/winemac.drv/window.c b/dlls/winemac.drv/window.c
> index 605c0c87f1..e2e8cc3f13 100644
> --- a/dlls/winemac.drv/window.c
> +++ b/dlls/winemac.drv/window.c
> @@ -2119,7 +2119,7 @@ void CDECL macdrv_WindowPosChanged(HWND hwnd, HWND insert_after, UINT swp_flags,
>      data->window_rect = *window_rect;
>      data->whole_rect  = *visible_rect;
>      data->client_rect = *client_rect;
> - if (data->cocoa_window && !data->ulw_layered)
> - if (data->cocoa_window && !data->ulw_layered && !(swp_flags & SWP_HIDEWINDOW))
> {
>     if (surface) window_surface_add_ref(surface);
>     if (new_style & WS_MINIMIZE)

Hmm.  This means that when a window is hidden, we don't release its surface, which can be a large allocation.

I think the right fix would be something like changing get_mac_surface() in surface.c to check surface->funcs against &macdrv_surface_funcs and returning NULL if it's not one of ours.  Then, check the return in the appropriate places (in a number of places where it's called we know for sure that it's one of ours).

Thanks,
Ken

[Piotr Caban]

On 3/11/20 4:16 PM, Ken Thomases wrote:
> Hmm.  This means that when a window is hidden, we don't release its surface, which can be a large allocation.
>
> I think the right fix would be something like changing get_mac_surface() in surface.c to check surface->funcs against &macdrv_surface_funcs and returning NULL if it's not one of ours.  Then, check the return in the appropriate places (in a number of places where it's called we know for sure that it's one of ours).

Isn't it better to never store dummy_surface in data->surface? How about 
doing something like:
if (!(swp_flags & SWP_HIDEWINDOW)) surface = NULL;
in macdrv_WindowPosChanged?

Thanks,
Piotr

[Piotr Caban]

On 3/11/20 4:28 PM, Piotr Caban wrote:
> On 3/11/20 4:16 PM, Ken Thomases wrote:
> > Hmm.  This means that when a window is hidden, we don't release its 
> > surface, which can be a large allocation.
> >
> > I think the right fix would be something like changing 
> > get_mac_surface() in surface.c to check surface->funcs against 
> > &macdrv_surface_funcs and returning NULL if it's not one of ours.  
> > Then, check the return in the appropriate places (in a number of 
> > places where it's called we know for sure that it's one of ours).
>
> Isn't it better to never store dummy_surface in data->surface? How about 
> doing something like:
> if (!(swp_flags & SWP_HIDEWINDOW)) surface = NULL;

if (swp_flags & SWP_HIDEWINDOW) surface = NULL;

[Ken Thomases]

On Mar 11, 2020, at 10:30 AM, Piotr Caban  wrote:
> On 3/11/20 4:28 PM, Piotr Caban wrote:
> > On 3/11/20 4:16 PM, Ken Thomases wrote:
> > > Hmm.  This means that when a window is hidden, we don't release its surface, which can be a large allocation.
> > >
> > > I think the right fix would be something like changing get_mac_surface() in surface.c to check surface->funcs against &macdrv_surface_funcs and returning NULL if it's not one of ours.  Then, check the return in the appropriate places (in a number of places where it's called we know for sure that it's one of ours).
> >
> > Isn't it better to never store dummy_surface in data->surface? How about doing something like:
> > if (!(swp_flags & SWP_HIDEWINDOW)) surface = NULL;
>
> if (swp_flags & SWP_HIDEWINDOW) surface = NULL;

Perhaps, although I'd want to base the decision on the funcs pointer, not the swp_flags.  Of course, you've already sent an implementation of my suggestion and that seems fine, too.

-Ken