diff mbox series

ui/cocoa: Fix incorrect window clipping on macOS Sonoma

Message ID DDADA9A9-DED4-4510-A532-7600C0233467@daveparsons.net (mailing list archive)
State New, archived
Headers show
Series ui/cocoa: Fix incorrect window clipping on macOS Sonoma | expand

Commit Message

David Parsons Feb. 17, 2024, 3:58 p.m. UTC 
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>

Comments

Michael Tokarev Feb. 22, 2024, 6:08 a.m. UTC | #1 
[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.

Thanks,

/mjt
Peter Maydell Feb. 22, 2024, 5:10 p.m. UTC | #2 
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
Akihiko Odaki Feb. 23, 2024, 11:28 a.m. UTC | #3 
On 2024/02/23 2:10, Peter Maydell wrote:
> 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.

Thanks for posting a patch for this critical problem.

>>>
>>> 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).

Apparently it is that case.

Please check if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_VERSION_14_0
instead of using @available. See commit 5e24600a7c1c ("ui/cocoa.m: Fix 
macOS 10.14 deprecation warnings") for example.

> 
> 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.

It is a chore to convert the coordinates using NSView.bounds. Let's keep 
using clipsToBounds.

> 
> -- PMM
David Parsons Feb. 23, 2024, 5:24 p.m. UTC | #4 
Hi Akihiko

I’ve re-worked the patch to match your suggestion. I have compiled
and tested it on Sonoma and Monterey and both builds worked correctly.

New patch is below. I’m new to sending patches to QEMU so please let 
me know if I need to do anything else to get it incorporated into the
repo.

Dave 

diff --git a/ui/cocoa.m b/ui/cocoa.m
index eb99064bee..bbf9704b8c 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -54,6 +54,10 @@
 #define MAC_OS_X_VERSION_10_13 101300
 #endif
 
+#ifndef MAC_OS_VERSION_14_0
+#define MAC_OS_VERSION_14_0 140000
+#endif
+
 /* 10.14 deprecates NSOnState and NSOffState in favor of
  * NSControlStateValueOn/Off, which were introduced in 10.13.
  * Define for older versions
@@ -365,6 +369,9 @@ - (id)initWithFrame:(NSRect)frameRect
         screen.width = frameRect.size.width;
         screen.height = frameRect.size.height;
         kbd = qkbd_state_init(dcl.con);
+#if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_VERSION_14_0
+        [self setClipsToBounds:YES];
+#endif
 
     }
     return self;


> On 23 Feb 2024, at 11:28, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> 
> On 2024/02/23 2:10, Peter Maydell wrote:
>> 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.
> 
> Thanks for posting a patch for this critical problem.
> 
>>>> 
>>>> 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).
> 
> Apparently it is that case.
> 
> Please check if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_VERSION_14_0
> instead of using @available. See commit 5e24600a7c1c ("ui/cocoa.m: Fix macOS 10.14 deprecation warnings") for example.
> 
>> 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.
> 
> It is a chore to convert the coordinates using NSView.bounds. Let's keep using clipsToBounds.
> 
>> -- PMM
> 
>
BALATON Zoltan Feb. 23, 2024, 6:29 p.m. UTC | #5 
On Fri, 23 Feb 2024, David Parsons wrote:
> Hi Akihiko
>
> I’ve re-worked the patch to match your suggestion. I have compiled
> and tested it on Sonoma and Monterey and both builds worked correctly.
>
> New patch is below. I’m new to sending patches to QEMU so please let
> me know if I need to do anything else to get it incorporated into the
> repo.

See here for detailed docs:
https://www.qemu.org/docs/master/devel/submitting-a-patch.html

In short you'd need to make the patch the same way you did the first 
version with the changed content but as [PATCH v2] and send it again as a 
new patch like you did the first time. (You can use -v2 option to git 
format-patch to add v2 to subject but if you're not using git format-patch 
directly then I'm not sure what option will do that for the way you send 
the patch.)

Regards,
BALATON Zoltan

> Dave
>
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index eb99064bee..bbf9704b8c 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -54,6 +54,10 @@
> #define MAC_OS_X_VERSION_10_13 101300
> #endif
>
> +#ifndef MAC_OS_VERSION_14_0
> +#define MAC_OS_VERSION_14_0 140000
> +#endif
> +
> /* 10.14 deprecates NSOnState and NSOffState in favor of
>  * NSControlStateValueOn/Off, which were introduced in 10.13.
>  * Define for older versions
> @@ -365,6 +369,9 @@ - (id)initWithFrame:(NSRect)frameRect
>         screen.width = frameRect.size.width;
>         screen.height = frameRect.size.height;
>         kbd = qkbd_state_init(dcl.con);
> +#if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_VERSION_14_0
> +        [self setClipsToBounds:YES];
> +#endif
>
>     }
>     return self;
>
>
>> On 23 Feb 2024, at 11:28, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2024/02/23 2:10, Peter Maydell wrote:
>>> 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.
>>
>> Thanks for posting a patch for this critical problem.
>>
>>>>>
>>>>> 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).
>>
>> Apparently it is that case.
>>
>> Please check if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_VERSION_14_0
>> instead of using @available. See commit 5e24600a7c1c ("ui/cocoa.m: Fix macOS 10.14 deprecation warnings") for example.
>>
>>> 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.
>>
>> It is a chore to convert the coordinates using NSView.bounds. Let's keep using clipsToBounds.
>>
>>> -- PMM
>>
>>
>
>
diff mbox series

Patch

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;