Re: [PATCH v7] ui/cocoa: Use NSWindow's ability to resize

[Akihiko Odaki]

On 2023/12/17 20:39, BALATON Zoltan wrote:
> On Sun, 17 Dec 2023, Akihiko Odaki wrote:
> > This change brings two new features:
> > - The window will be resizable if "Zoom To Fit" is eanbled
> > - The window can be made full screen by clicking full screen button
> >  provided by the platform. (The left-top green button.)
> >
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > Tested-by: Rene Engel <ReneEngel80@emailn.de>
> > ---
> > V5 -> V6:
> >  Rebased.
> > ---
> > Changes in v7:
> > - Fixed zoom-to-fit option. (Marek Glogowski)
> > - Link to v6: 
> > 20231211-cocoa-v6-1-49f3be0191f2@daynix.com">https://lore.kernel.org/r/20231211-cocoa-v6-1-49f3be0191f2@daynix.com
> > ---
> > ui/cocoa.m | 542 
> > +++++++++++++++++++++++++++++--------------------------------
> > 1 file changed, 258 insertions(+), 284 deletions(-)
>
> Is ir possible to break this patch up into smaller ones for easier 
> review? E.g. separate patch moving mouse event handling out of 
> handleEventLocked, replacing stretch_video flag with 
> NSWindowStyleMaskResizable and whatever else can be done as independent 
> steps? Not sure if that's possible or needs the whole chnage at once but 
> this patch seems to be too big. Some more comments below.

I split it into three patches with v8, but most changes are still in one 
patch because they depend on the change to unify the full screen window 
and normal window and vice-versa.

> > diff --git a/ui/cocoa.m b/ui/cocoa.m
> > index cd069da6965b..31a2230f55a4 100644
> > --- a/ui/cocoa.m
> > +++ b/ui/cocoa.m
> > @@ -99,12 +99,10 @@ static void cocoa_switch(DisplayChangeListener *dcl,
> > static DisplayChangeListener dcl = {
> >     .ops = &dcl_ops,
> > };
> > -static int last_buttons;
> > static int cursor_hide = 1;
> > static int left_command_key_enabled = 1;
> > static bool swap_opt_cmd;
> >
> > -static bool stretch_video;
> > static NSTextField *pauseLabel;
> >
> > static bool allow_events;
> > @@ -304,20 +302,17 @@ static void handleAnyDeviceErrors(Error * err)
> > */
> > @interface QemuCocoaView : NSView
> > {
> > +    NSTrackingArea *trackingArea;
> >     QEMUScreen screen;
> > -    NSWindow *fullScreenWindow;
> > -    float cx,cy,cw,ch,cdx,cdy;
> >     pixman_image_t *pixman_image;
> >     QKbdState *kbd;
> >     BOOL isMouseGrabbed;
> > -    BOOL isFullscreen;
> >     BOOL isAbsoluteEnabled;
> >     CFMachPortRef eventsTap;
> > }
> > - (void) switchSurface:(pixman_image_t *)image;
> > - (void) grabMouse;
> > - (void) ungrabMouse;
> > -- (void) toggleFullScreen:(id)sender;
> > - (void) setFullGrab:(id)sender;
> > - (void) handleMonitorInput:(NSEvent *)event;
> > - (bool) handleEvent:(NSEvent *)event;
> > @@ -333,8 +328,6 @@ - (void) setAbsoluteEnabled:(BOOL)tIsAbsoluteEnabled;
> >  */
> > - (BOOL) isMouseGrabbed;
> > - (BOOL) isAbsoluteEnabled;
> > -- (float) cdx;
> > -- (float) cdy;
> > - (QEMUScreen) gscreen;
> > - (void) raiseAllKeys;
> > @end
> > @@ -392,46 +385,43 @@ - (BOOL) isOpaque
> >     return YES;
> > }
> >
> > -- (BOOL) screenContainsPoint:(NSPoint) p
> > +- (void) removeTrackingRect
> > {
> > -    return (p.x > -1 && p.x < screen.width && p.y > -1 && p.y < 
> > screen.height);
> > +    if (trackingArea) {
> > +        [self removeTrackingArea:trackingArea];
> > +        [trackingArea release];
> > +        trackingArea = nil;
> > +    }
> > }
> >
> > -/* Get location of event and convert to virtual screen coordinate */
> > -- (CGPoint) screenLocationOfEvent:(NSEvent *)ev
> > +- (void) frameUpdated
> > {
> > -    NSWindow *eventWindow = [ev window];
> > -    // XXX: Use CGRect and -convertRectFromScreen: to support macOS 
> > 10.10
> > -    CGRect r = CGRectZero;
> > -    r.origin = [ev locationInWindow];
> > -    if (!eventWindow) {
> > -        if (!isFullscreen) {
> > -            return [[self window] convertRectFromScreen:r].origin;
> > -        } else {
> > -            CGPoint locationInSelfWindow = [[self window] 
> > convertRectFromScreen:r].origin;
> > -            CGPoint loc = [self convertPoint:locationInSelfWindow 
> > fromView:nil];
> > -            if (stretch_video) {
> > -                loc.x /= cdx;
> > -                loc.y /= cdy;
> > -            }
> > -            return loc;
> > -        }
> > -    } else if ([[self window] isEqual:eventWindow]) {
> > -        if (!isFullscreen) {
> > -            return r.origin;
> > -        } else {
> > -            CGPoint loc = [self convertPoint:r.origin fromView:nil];
> > -            if (stretch_video) {
> > -                loc.x /= cdx;
> > -                loc.y /= cdy;
> > -            }
> > -            return loc;
> > -        }
> > -    } else {
> > -        return [[self window] convertRectFromScreen:[eventWindow 
> > convertRectToScreen:r]].origin;
> > +    [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
> > {
> >     if (!cursor_hide) {
> > @@ -494,13 +484,14 @@ - (void) drawRect:(NSRect) rect
> >         int i;
> >         CGImageRef clipImageRef;
> >         CGRect clipRect;
> > +        CGFloat d = (CGFloat)h / [self frame].size.height;
> >
> >         [self getRectsBeingDrawn:&rectList count:&rectCount];
> >         for (i = 0; i < rectCount; i++) {
> > -            clipRect.origin.x = rectList[i].origin.x / cdx;
> > -            clipRect.origin.y = (float)h - (rectList[i].origin.y + 
> > rectList[i].size.height) / cdy;
> > -            clipRect.size.width = rectList[i].size.width / cdx;
> > -            clipRect.size.height = rectList[i].size.height / cdy;
> > +            clipRect.origin.x = rectList[i].origin.x * d;
> > +            clipRect.origin.y = (float)h - (rectList[i].origin.y + 
> > rectList[i].size.height) * d;
> > +            clipRect.size.width = rectList[i].size.width * d;
> > +            clipRect.size.height = rectList[i].size.height * d;
> >             clipImageRef = CGImageCreateWithImageInRect(
> >                                                         imageRef,
> >                                                         clipRect
> > @@ -513,36 +504,43 @@ - (void) drawRect:(NSRect) rect
> >     }
> > }
> >
> > -- (void) setContentDimensions
> > +- (NSSize) fixZoomedFullScreenSize:(NSSize)proposedSize
> > {
> > -    COCOA_DEBUG("QemuCocoaView: setContentDimensions\n");
> > +    NSSize size;
> >
> > -    if (isFullscreen) {
> > -        cdx = [[NSScreen mainScreen] frame].size.width / 
> > (float)screen.width;
> > -        cdy = [[NSScreen mainScreen] frame].size.height / 
> > (float)screen.height;
> > +    size.width = (CGFloat)screen.width * proposedSize.height;
> > +    size.height = (CGFloat)screen.height * proposedSize.width;
>
> One of these will be overwritten in the next if below so maybe drop this 
> init and do the calculation in the if legs which is then also clearer to 
> show that this would scale one of these with screen.width/screen.height 
> or the inverse of that.
>
> This also removes stretch_video flag and the calculation to preserve 
> aspect ratio. Is that correct? Would it now distort the image when 
> zooming to full screen if guest resolution is not the same as host 
> screen? Is that how zoom-to-fit should work? At leest with -display sdl 
> going to full screen guest screen is zoomed preserving aspect ratio but 
> maybe sdl does not have zoom-to-fit option. I don't know how it works 
> with other displays such as gtk.

The purpose of this method is to fix the aspect ratio for zoom-to-fit by 
shrinking width or height. It operates in the three steps:
1. Compute the values necessary either for shrinking width or height.
2. Decide which of width or height to shrink.
3. Compute the final values.

> > -        /* stretches video, but keeps same aspect ratio */
> > -        if (stretch_video == true) {
> > -            /* use smallest stretch value - prevents clipping on 
> > sides */
> > -            if (MIN(cdx, cdy) == cdx) {
> > -                cdy = cdx;
> > -            } else {
> > -                cdx = cdy;
> > -            }
> > -        } else {  /* No stretching */
> > -            cdx = cdy = 1;
> > -        }
> > -        cw = screen.width * cdx;
> > -        ch = screen.height * cdy;
> > -        cx = ([[NSScreen mainScreen] frame].size.width - cw) / 2.0;
> > -        cy = ([[NSScreen mainScreen] frame].size.height - ch) / 2.0;
> > +    if (size.width < size.height) {
> > +        size.width /= screen.height;
> > +        size.height = proposedSize.height;
> >     } else {
> > -        cx = 0;
> > -        cy = 0;
> > -        cw = screen.width;
> > -        ch = screen.height;
> > -        cdx = 1.0;
> > -        cdy = 1.0;
> > +        size.width = proposedSize.width;
> > +        size.height /= screen.width;
> > +    }
> > +
> > +    return size;
> > +}
> > +
> > +- (NSSize) screenSafeAreaSize
> > +{
> > +    NSSize size = [[[self window] screen] frame].size;
> > +    NSEdgeInsets insets = [[[self window] screen] safeAreaInsets];
> > +    size.width -= insets.left + insets.right;
> > +    size.height -= insets.top + insets.bottom;
> > +    return size;
> > +}
> > +
> > +- (void) resizeWindow
> > +{
> > +    [[self window] setContentAspectRatio:NSMakeSize(screen.width, 
> > screen.height)];
> > +
> > +    if (([[self window] styleMask] & NSWindowStyleMaskResizable) == 0) {
> > +        [[self window] setContentSize:NSMakeSize(screen.width, 
> > screen.height)];
> > +        [[self window] center];
> > +    } else if (([[self window] styleMask] & 
> > NSWindowStyleMaskFullScreen) != 0) {
> > +        [[self window] setContentSize:[self 
> > fixZoomedFullScreenSize:[self screenSafeAreaSize]]];
> > +        [[self window] center];
> >     }
> > }
> >
> > @@ -563,7 +561,11 @@ - (void) updateUIInfoLocked
> >         CGSize screenPhysicalSize = CGDisplayScreenSize(display);
> >         CVDisplayLinkRef displayLink;
> >
> > -        frameSize = isFullscreen ? screenSize : [self frame].size;
> > +        if (([[self window] styleMask] & NSWindowStyleMaskFullScreen) 
> > == 0) {
> > +            frameSize = [self frame].size;
> > +        } else {
> > +            frameSize = [self screenSafeAreaSize];
> > +        }
>
> In these ifs you seem to test for flag not set with == 0 but maybe 
> swapping the else and then branches and testing for flag set would be 
> clearer than testing for negative condition? I mean:
>
> if (([[self window] styleMask] & NSWindowStyleMaskFullScreen)) {
>      frameSize = [self screenSafeAreaSize];
> } else {
>      frameSize = [self frame].size;
> }

Fixed with v8.

Regards,
Akihiko Odaki