Re: [PATCH] ui/cocoa: Fix incorrect window clipping on macOS Sonoma
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 14
+#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 wrote:
On 2024/02/23 2:10, Peter Maydell wrote:
On Thu, 22 Feb 2024 at 06:08, Michael Tokarev 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
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
Re: [PATCH] ui/cocoa: Fix incorrect window clipping on macOS Sonoma
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 14
+#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 wrote:
>
> On 2024/02/23 2:10, Peter Maydell wrote:
>> On Thu, 22 Feb 2024 at 06:08, Michael Tokarev 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
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
>
>
Re: [PATCH] ui/cocoa: Fix incorrect window clipping on macOS Sonoma
On 2024/02/23 2:10, Peter Maydell wrote:
On Thu, 22 Feb 2024 at 06:08, Michael Tokarev 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
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
Re: [PATCH] ui/cocoa: Fix incorrect window clipping on macOS Sonoma
On Thu, 22 Feb 2024 at 06:08, Michael Tokarev 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
> >
> > 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
Re: [PATCH] ui/cocoa: Fix incorrect window clipping on macOS Sonoma
[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
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
[PATCH] ui/cocoa: Fix incorrect window clipping on macOS Sonoma
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
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;
