Re: [PATCH] ui/cocoa: Fix incorrect window clipping on macOS Sonoma

[Peter Maydell]

On Thu, 22 Feb 2024 at 06:08, Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> [Adding a few more Ccs]
>
> 17.02.2024 18:58, David Parsons :
> > macOS Sonoma changes the NSView.clipsToBounds to false by default where it 
> > was true in
> > earlier version of macOS. This causes the window contents to be obscured by 
> > the window
> > frame. This fixes the issue by conditionally setting the clipping on Sonoma 
> > to true.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1994
> > Signed-off-by: David Parsons <dave@daveparsons.net>
> >
> > diff --git a/ui/cocoa.m b/ui/cocoa.m
> > index eb99064bee..c9e3b96004 100644
> > --- a/ui/cocoa.m
> > +++ b/ui/cocoa.m
> > @@ -365,6 +365,9 @@ - (id)initWithFrame:(NSRect)frameRect
> >           screen.width = frameRect.size.width;
> >           screen.height = frameRect.size.height;
> >           kbd = qkbd_state_init(dcl.con);
> > +        if (@available(macOS 14, *)) {
> > +            [self setClipsToBounds:YES];
> > +        }
> >
> >       }
> >       return self;
> >
>
> Hi David!
>
> While the code change is tiny, I for one know nothing about MacOS and
> its cocoa thing, so to me (with my trivial-patches hat on) this is a
> no-go.  I'd love to have a review from someone more knowlegeable in
> this area.

Mmm. Akihiko is the expert here, but I do notice that we don't
seem to be handling the "macos-version-specific" stuff in a
way we've done it before (we don't use @available elsewhere).

I did wonder if we could call the setClipsToBounds method unconditionally;
The release notes say
https://developer.apple.com/documentation/macos-release-notes/appkit-release-notes-for-macos-14#NSView
"This property is available back to macOS 10.9. This availability is
intended to allow code targeting older OSes to set this property to
true without guarding the setter in an availability check."

but I think that might only mean "you can do this building on a new
SDK that's targeting an old version", not "you can do this
when building on an older SDK" (at least judging from the
comments in the gitlab issue).

The other option would be to fix whatever it is that we're
presumably not getting right that means this default change
caused the bug. My guess is that we are in the case
"Confusing a view’s bounds and its dirty rect. The dirty rect
 passed to .drawRect() should be used to determine what to draw,
 not where to draw it. Use NSView.bounds when determining the
 layout of what your view draws."
But unless the fix for that is really obvious and easy I guess
that flipping the default back to its old value is the better
approach.

-- PMM