[PATCH 0/3] Fixes for "ui/cocoa: Let the platform toggle fullscreen"

[PATCH 0/3] Fixes for "ui/cocoa: Let the platform toggle fullscreen"

[Akihiko Odaki]

This series contains patches for regressions caused by commit 91aa508d0274
("ui/cocoa: Let the platform toggle fullscreen").

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
Akihiko Odaki (3):
      ui/cocoa: Fix aspect ratio
      ui/cocoa: Resize window after toggling zoom-to-fit
      ui/cocoa: Use NSTrackingInVisibleRect

 ui/cocoa.m | 72 ++++++++++++++++++++++++++++++++------------------------------
 1 file changed, 37 insertions(+), 35 deletions(-)
---
base-commit: ba49d760eb04630e7b15f423ebecf6c871b8f77b
change-id: 20240318-fixes-7b187ec236a0

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>

[PATCH 1/3] ui/cocoa: Fix aspect ratio

[Akihiko Odaki]

[NSWindow setContentAspectRatio:] does not trigger window resize itself,
so the wrong aspect ratio will persist if nothing resizes the window.
Call [NSWindow setContentSize:] in such a case.

Fixes: 91aa508d0274 ("ui/cocoa: Let the platform toggle fullscreen")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 ui/cocoa.m | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index fa879d7dcd4b..d6a5b462f78b 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -508,6 +508,25 @@ - (void) drawRect:(NSRect) rect
     }
 }
 
+- (NSSize)fixAspectRatio:(NSSize)original
+{
+    NSSize scaled;
+    NSSize fixed;
+
+    scaled.width = screen.width * original.height;
+    scaled.height = screen.height * original.width;
+
+    if (scaled.width < scaled.height) {
+        fixed.width = scaled.width / screen.height;
+        fixed.height = original.height;
+    } else {
+        fixed.width = original.width;
+        fixed.height = scaled.height / screen.width;
+    }
+
+    return fixed;
+}
+
 - (NSSize) screenSafeAreaSize
 {
     NSSize size = [[[self window] screen] frame].size;
@@ -525,8 +544,10 @@ - (void) resizeWindow
         [[self window] setContentSize:NSMakeSize(screen.width, screen.height)];
         [[self window] center];
     } else if ([[self window] styleMask] & NSWindowStyleMaskFullScreen) {
-        [[self window] setContentSize:[self screenSafeAreaSize]];
+        [[self window] setContentSize:[self fixAspectRatio:[self screenSafeAreaSize]]];
         [[self window] center];
+    } else {
+        [[self window] setContentSize:[self fixAspectRatio:[self frame].size]];
     }
 }
 

-- 
2.44.0

Re: [PATCH 1/3] ui/cocoa: Fix aspect ratio

[Peter Maydell]

On Mon, 18 Mar 2024 at 07:53, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> [NSWindow setContentAspectRatio:] does not trigger window resize itself,
> so the wrong aspect ratio will persist if nothing resizes the window.
> Call [NSWindow setContentSize:] in such a case.
>
> Fixes: 91aa508d0274 ("ui/cocoa: Let the platform toggle fullscreen")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  ui/cocoa.m | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index fa879d7dcd4b..d6a5b462f78b 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -508,6 +508,25 @@ - (void) drawRect:(NSRect) rect
>      }
>  }
>
> +- (NSSize)fixAspectRatio:(NSSize)original
> +{
> +    NSSize scaled;
> +    NSSize fixed;
> +
> +    scaled.width = screen.width * original.height;
> +    scaled.height = screen.height * original.width;
> +
> +    if (scaled.width < scaled.height) {

Is this a standard algorithm for scaling with a fixed
aspect ratio? It looks rather weird to be comparing
a width against a height here, and to be multiplying a
width by a height.

> +        fixed.width = scaled.width / screen.height;
> +        fixed.height = original.height;
> +    } else {
> +        fixed.width = original.width;
> +        fixed.height = scaled.height / screen.width;
> +    }
> +
> +    return fixed;
> +}
> +

thanks
-- PMM

Re: [PATCH 1/3] ui/cocoa: Fix aspect ratio

[Akihiko Odaki]

On 2024/03/22 21:22, Peter Maydell wrote:
> On Mon, 18 Mar 2024 at 07:53, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> [NSWindow setContentAspectRatio:] does not trigger window resize itself,
>> so the wrong aspect ratio will persist if nothing resizes the window.
>> Call [NSWindow setContentSize:] in such a case.
>>
>> Fixes: 91aa508d0274 ("ui/cocoa: Let the platform toggle fullscreen")
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   ui/cocoa.m | 23 ++++++++++++++++++++++-
>>   1 file changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/ui/cocoa.m b/ui/cocoa.m
>> index fa879d7dcd4b..d6a5b462f78b 100644
>> --- a/ui/cocoa.m
>> +++ b/ui/cocoa.m
>> @@ -508,6 +508,25 @@ - (void) drawRect:(NSRect) rect
>>       }
>>   }
>>
>> +- (NSSize)fixAspectRatio:(NSSize)original
>> +{
>> +    NSSize scaled;
>> +    NSSize fixed;
>> +
>> +    scaled.width = screen.width * original.height;
>> +    scaled.height = screen.height * original.width;
>> +
>> +    if (scaled.width < scaled.height) {
> 
> Is this a standard algorithm for scaling with a fixed
> aspect ratio? It looks rather weird to be comparing
> a width against a height here, and to be multiplying a
> width by a height.

Not sure if it's a standard, but it's an algorithm with least error I 
came up with.

Regards,
Akihiko Odaki

> 
>> +        fixed.width = scaled.width / screen.height;
>> +        fixed.height = original.height;
>> +    } else {
>> +        fixed.width = original.width;
>> +        fixed.height = scaled.height / screen.width;
>> +    }
>> +
>> +    return fixed;
>> +}
>> +
> 
> thanks
> -- PMM

Re: [PATCH 1/3] ui/cocoa: Fix aspect ratio

[Peter Maydell]

On Fri, 22 Mar 2024 at 12:25, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2024/03/22 21:22, Peter Maydell wrote:
> > On Mon, 18 Mar 2024 at 07:53, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>
> >> [NSWindow setContentAspectRatio:] does not trigger window resize itself,
> >> so the wrong aspect ratio will persist if nothing resizes the window.
> >> Call [NSWindow setContentSize:] in such a case.
> >>
> >> Fixes: 91aa508d0274 ("ui/cocoa: Let the platform toggle fullscreen")
> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >> ---
> >>   ui/cocoa.m | 23 ++++++++++++++++++++++-
> >>   1 file changed, 22 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/ui/cocoa.m b/ui/cocoa.m
> >> index fa879d7dcd4b..d6a5b462f78b 100644
> >> --- a/ui/cocoa.m
> >> +++ b/ui/cocoa.m
> >> @@ -508,6 +508,25 @@ - (void) drawRect:(NSRect) rect
> >>       }
> >>   }
> >>
> >> +- (NSSize)fixAspectRatio:(NSSize)original
> >> +{
> >> +    NSSize scaled;
> >> +    NSSize fixed;
> >> +
> >> +    scaled.width = screen.width * original.height;
> >> +    scaled.height = screen.height * original.width;
> >> +
> >> +    if (scaled.width < scaled.height) {
> >
> > Is this a standard algorithm for scaling with a fixed
> > aspect ratio? It looks rather weird to be comparing
> > a width against a height here, and to be multiplying a
> > width by a height.
>
> Not sure if it's a standard, but it's an algorithm with least error I
> came up with.

OK. Maybe a comment would help (at least it helps me in thinking
through the code :-))

 /*
  * Here screen is our guest's output size, and original is the
  * size of the largest possible area of the screen we can display on.
  * We want to scale up (screen.width x screen.height) by either:
  *   1) original.height / screen.height
  *   2) original.width / screen.width
  * With the first scale factor the scale will result in an output
  * height of original.height (i.e. we will fill the whole height
  * of the available screen space and have black bars left and right)
  * and with the second scale factor the scaling will result in an
  * output width of original.width (i.e. we fill the whole width of
  * the available screen space and have black bars top and bottom).
  * We need to pick whichever keeps the whole of the guest output
  * on the screen, which is to say the smaller of the two scale factors.
  * To avoid doing more division than strictly necessary, instead
  * of directly comparing scale factors 1 and 2 we instead
  * calculate and compare those two scale factors multiplied by
  * (screen.height * screen.width).
  */

Having written that out, it seems to me that the variable
names here could more clearly reflect what they're doing
(eg "screen" is not the size of the screen we're displaying
on, "original" is not the old displayable area size but the
new one we're trying to fit into, scaled doesn't actually
contain a (width, height) that go together with each other,
and it doesn't contain the actual scale factor we're going to
be using either).

> >> +        fixed.width = scaled.width / screen.height;
> >> +        fixed.height = original.height;
> >> +    } else {
> >> +        fixed.width = original.width;
> >> +        fixed.height = scaled.height / screen.width;
> >> +    }
> >> +
> >> +    return fixed;
> >> +}
> >> +

thanks
-- PMM

Re: [PATCH 1/3] ui/cocoa: Fix aspect ratio

[Akihiko Odaki]

On 2024/03/22 21:55, Peter Maydell wrote:
> On Fri, 22 Mar 2024 at 12:25, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2024/03/22 21:22, Peter Maydell wrote:
>>> On Mon, 18 Mar 2024 at 07:53, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>
>>>> [NSWindow setContentAspectRatio:] does not trigger window resize itself,
>>>> so the wrong aspect ratio will persist if nothing resizes the window.
>>>> Call [NSWindow setContentSize:] in such a case.
>>>>
>>>> Fixes: 91aa508d0274 ("ui/cocoa: Let the platform toggle fullscreen")
>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>> ---
>>>>    ui/cocoa.m | 23 ++++++++++++++++++++++-
>>>>    1 file changed, 22 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/ui/cocoa.m b/ui/cocoa.m
>>>> index fa879d7dcd4b..d6a5b462f78b 100644
>>>> --- a/ui/cocoa.m
>>>> +++ b/ui/cocoa.m
>>>> @@ -508,6 +508,25 @@ - (void) drawRect:(NSRect) rect
>>>>        }
>>>>    }
>>>>
>>>> +- (NSSize)fixAspectRatio:(NSSize)original
>>>> +{
>>>> +    NSSize scaled;
>>>> +    NSSize fixed;
>>>> +
>>>> +    scaled.width = screen.width * original.height;
>>>> +    scaled.height = screen.height * original.width;
>>>> +
>>>> +    if (scaled.width < scaled.height) {
>>>
>>> Is this a standard algorithm for scaling with a fixed
>>> aspect ratio? It looks rather weird to be comparing
>>> a width against a height here, and to be multiplying a
>>> width by a height.
>>
>> Not sure if it's a standard, but it's an algorithm with least error I
>> came up with.
> 
> OK. Maybe a comment would help (at least it helps me in thinking
> through the code :-))
> 
>   /*
>    * Here screen is our guest's output size, and original is the
>    * size of the largest possible area of the screen we can display on.
>    * We want to scale up (screen.width x screen.height) by either:
>    *   1) original.height / screen.height
>    *   2) original.width / screen.width
>    * With the first scale factor the scale will result in an output
>    * height of original.height (i.e. we will fill the whole height
>    * of the available screen space and have black bars left and right)
>    * and with the second scale factor the scaling will result in an
>    * output width of original.width (i.e. we fill the whole width of
>    * the available screen space and have black bars top and bottom).
>    * We need to pick whichever keeps the whole of the guest output
>    * on the screen, which is to say the smaller of the two scale factors.
>    * To avoid doing more division than strictly necessary, instead
>    * of directly comparing scale factors 1 and 2 we instead
>    * calculate and compare those two scale factors multiplied by
>    * (screen.height * screen.width).
>    */
> 
> Having written that out, it seems to me that the variable
> names here could more clearly reflect what they're doing
> (eg "screen" is not the size of the screen we're displaying
> on, "original" is not the old displayable area size but the
> new one we're trying to fit into, scaled doesn't actually
> contain a (width, height) that go together with each other,
> and it doesn't contain the actual scale factor we're going to
> be using either).

With v2, I added the comment and renamed original to max as it's the 
largest area we can display on as described in the comment.

screen and scaled are not renamed. Renaming screen is a bit out of scope 
of this patch as it's an existing variable. The variable is referenced 
from several places so a patch to rename it will be a bit large and not 
suited to include in a bug fix series. I couldn't just invent a good 
name for scaled.

Regards,
Akihiko Odaki

> 
>>>> +        fixed.width = scaled.width / screen.height;
>>>> +        fixed.height = original.height;
>>>> +    } else {
>>>> +        fixed.width = original.width;
>>>> +        fixed.height = scaled.height / screen.width;
>>>> +    }
>>>> +
>>>> +    return fixed;
>>>> +}
>>>> +
> 
> thanks
> -- PMM

[PATCH 2/3] ui/cocoa: Resize window after toggling zoom-to-fit

[Akihiko Odaki]

Resize the window so that the content will fit without zooming.

Fixes: 91aa508d0274 ("ui/cocoa: Let the platform toggle fullscreen")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 ui/cocoa.m | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index d6a5b462f78b..1324be6d32fe 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -1378,6 +1378,7 @@ - (void)zoomToFit:(id) sender
 
     [[cocoaView window] setStyleMask:styleMask];
     [sender setState:styleMask & NSWindowStyleMaskResizable ? NSControlStateValueOn : NSControlStateValueOff];
+    [cocoaView resizeWindow];
 }
 
 - (void)toggleZoomInterpolation:(id) sender

-- 
2.44.0

Re: [PATCH 2/3] ui/cocoa: Resize window after toggling zoom-to-fit

[Peter Maydell]

On Mon, 18 Mar 2024 at 07:53, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> Resize the window so that the content will fit without zooming.
>
> Fixes: 91aa508d0274 ("ui/cocoa: Let the platform toggle fullscreen")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  ui/cocoa.m | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index d6a5b462f78b..1324be6d32fe 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -1378,6 +1378,7 @@ - (void)zoomToFit:(id) sender
>
>      [[cocoaView window] setStyleMask:styleMask];
>      [sender setState:styleMask & NSWindowStyleMaskResizable ? NSControlStateValueOn : NSControlStateValueOff];
> +    [cocoaView resizeWindow];
>  }
>
>  - (void)toggleZoomInterpolation:(id) sender
>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 3/3] ui/cocoa: Use NSTrackingInVisibleRect

[Akihiko Odaki]

I observed [NSTrackingArea rect] becomes de-synchronized with the view
frame with some unknown condition. Specify NSTrackingInVisibleRect
option to let Cocoa automatically update NSTrackingArea, which also
saves code for synchronization.

Fixes: 91aa508d0274 ("ui/cocoa: Let the platform toggle fullscreen")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 ui/cocoa.m | 48 ++++++++++++++----------------------------------
 1 file changed, 14 insertions(+), 34 deletions(-)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index 1324be6d32fe..1e2b7d931905 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -306,7 +306,6 @@ static void handleAnyDeviceErrors(Error * err)
 */
 @interface QemuCocoaView : NSView
 {
-    NSTrackingArea *trackingArea;
     QEMUScreen screen;
     pixman_image_t *pixman_image;
     QKbdState *kbd;
@@ -359,6 +358,19 @@ - (id)initWithFrame:(NSRect)frameRect
     self = [super initWithFrame:frameRect];
     if (self) {
 
+        NSTrackingAreaOptions options = NSTrackingActiveInKeyWindow |
+                                        NSTrackingMouseEnteredAndExited |
+                                        NSTrackingMouseMoved |
+                                        NSTrackingInVisibleRect;
+
+        NSTrackingArea *trackingArea =
+            [[NSTrackingArea alloc] initWithRect:CGRectZero
+                                         options:options
+                                           owner:self
+                                        userInfo:nil];
+
+        [self addTrackingArea:trackingArea];
+        [trackingArea release];
         screen.width = frameRect.size.width;
         screen.height = frameRect.size.height;
         kbd = qkbd_state_init(dcl.con);
@@ -392,41 +404,9 @@ - (BOOL) isOpaque
     return YES;
 }
 
-- (void) removeTrackingRect
-{
-    if (trackingArea) {
-        [self removeTrackingArea:trackingArea];
-        [trackingArea release];
-        trackingArea = nil;
-    }
-}
-
-- (void) frameUpdated
-{
-    [self removeTrackingRect];
-
-    if ([self window]) {
-        NSTrackingAreaOptions options = NSTrackingActiveInKeyWindow |
-                                        NSTrackingMouseEnteredAndExited |
-                                        NSTrackingMouseMoved;
-        trackingArea = [[NSTrackingArea alloc] initWithRect:[self frame]
-                                                    options:options
-                                                      owner:self
-                                                   userInfo:nil];
-        [self addTrackingArea:trackingArea];
-        [self updateUIInfo];
-    }
-}
-
 - (void) viewDidMoveToWindow
 {
     [self resizeWindow];
-    [self frameUpdated];
-}
-
-- (void) viewWillMoveToWindow:(NSWindow *)newWindow
-{
-    [self removeTrackingRect];
 }
 
 - (void) hideCursor
@@ -1284,7 +1264,7 @@ - (void)windowDidExitFullScreen:(NSNotification *)notification
 - (void)windowDidResize:(NSNotification *)notification
 {
     [cocoaView updateBounds];
-    [cocoaView frameUpdated];
+    [cocoaView updateUIInfo];
 }
 
 /* Called when the user clicks on a window's close button */

-- 
2.44.0

Re: [PATCH 3/3] ui/cocoa: Use NSTrackingInVisibleRect

[Peter Maydell]

On Mon, 18 Mar 2024 at 07:53, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> I observed [NSTrackingArea rect] becomes de-synchronized with the view
> frame with some unknown condition. Specify NSTrackingInVisibleRect
> option to let Cocoa automatically update NSTrackingArea, which also
> saves code for synchronization.
>
> Fixes: 91aa508d0274 ("ui/cocoa: Let the platform toggle fullscreen")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>

It would be nice to specify in the commit message what
the user-visible consequences of this problem are (presumably
that the guest mouse stops following the host mouse correctly?)
but anyway

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM