[PATCH] ui/cocoa: Remove the uses of full screen APIs

[PATCH] ui/cocoa: Remove the uses of full screen APIs

[Akihiko Odaki]

The detections of full screen APIs were wrong. A detection is coded as:
[NSView respondsToSelector:@selector(exitFullScreenModeWithOptions:)]
but it should be:
[NSView instancesRespondToSelector:@selector(exitFullScreenModeWithOptions:)]

The uses of full screen APIs were also incorrect, and if you fix the
detections, the full screen view stretches the video, changing the
aspect ratio, even if zooming is disabled.

Remove the code as it does nothing good.

Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
---
 ui/cocoa.m | 41 +++++++++++++++--------------------------
 1 file changed, 15 insertions(+), 26 deletions(-)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index 13fba8103e1..36e45cd98b4 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -564,37 +564,26 @@ - (void) toggleFullScreen:(id)sender
         isFullscreen = FALSE;
         [self ungrabMouse];
         [self setContentDimensions];
-        if ([NSView respondsToSelector:@selector(exitFullScreenModeWithOptions:)]) { // test if "exitFullScreenModeWithOptions" is supported on host at runtime
-            [self exitFullScreenModeWithOptions:nil];
-        } else {
-            [fullScreenWindow close];
-            [normalWindow setContentView: self];
-            [normalWindow makeKeyAndOrderFront: self];
-            [NSMenu setMenuBarVisible:YES];
-        }
+        [fullScreenWindow close];
+        [normalWindow setContentView: self];
+        [normalWindow makeKeyAndOrderFront: self];
+        [NSMenu setMenuBarVisible:YES];
     } else { // switch from desktop to fullscreen
         isFullscreen = TRUE;
         [normalWindow orderOut: nil]; /* Hide the window */
         [self grabMouse];
         [self setContentDimensions];
-        if ([NSView respondsToSelector:@selector(enterFullScreenMode:withOptions:)]) { // test if "enterFullScreenMode:withOptions" is supported on host at runtime
-            [self enterFullScreenMode:[NSScreen mainScreen] withOptions:[NSDictionary dictionaryWithObjectsAndKeys:
-                [NSNumber numberWithBool:NO], NSFullScreenModeAllScreens,
-                [NSDictionary dictionaryWithObjectsAndKeys:[NSNumber numberWithBool:NO], kCGDisplayModeIsStretched, nil], NSFullScreenModeSetting,
-                 nil]];
-        } else {
-            [NSMenu setMenuBarVisible:NO];
-            fullScreenWindow = [[NSWindow alloc] initWithContentRect:[[NSScreen mainScreen] frame]
-                styleMask:NSWindowStyleMaskBorderless
-                backing:NSBackingStoreBuffered
-                defer:NO];
-            [fullScreenWindow setAcceptsMouseMovedEvents: YES];
-            [fullScreenWindow setHasShadow:NO];
-            [fullScreenWindow setBackgroundColor: [NSColor blackColor]];
-            [self setFrame:NSMakeRect(cx, cy, cw, ch)];
-            [[fullScreenWindow contentView] addSubview: self];
-            [fullScreenWindow makeKeyAndOrderFront:self];
-        }
+        [NSMenu setMenuBarVisible:NO];
+        fullScreenWindow = [[NSWindow alloc] initWithContentRect:[[NSScreen mainScreen] frame]
+            styleMask:NSWindowStyleMaskBorderless
+            backing:NSBackingStoreBuffered
+            defer:NO];
+        [fullScreenWindow setAcceptsMouseMovedEvents: YES];
+        [fullScreenWindow setHasShadow:NO];
+        [fullScreenWindow setBackgroundColor: [NSColor blackColor]];
+        [self setFrame:NSMakeRect(cx, cy, cw, ch)];
+        [[fullScreenWindow contentView] addSubview: self];
+        [fullScreenWindow makeKeyAndOrderFront:self];
     }
 }
 
-- 
2.24.3 (Apple Git-128)

Re: [PATCH] ui/cocoa: Remove the uses of full screen APIs

[Gerd Hoffmann]

On Fri, Feb 12, 2021 at 09:05:40AM +0900, Akihiko Odaki wrote:
> The detections of full screen APIs were wrong. A detection is coded as:
> [NSView respondsToSelector:@selector(exitFullScreenModeWithOptions:)]
> but it should be:
> [NSView instancesRespondToSelector:@selector(exitFullScreenModeWithOptions:)]
> 
> The uses of full screen APIs were also incorrect, and if you fix the
> detections, the full screen view stretches the video, changing the
> aspect ratio, even if zooming is disabled.
> 
> Remove the code as it does nothing good.

So, it's broken right now (and probably for quite a while without anyone
complaining).  And the attempt to fix it didn't work out very well.
Correct?

Just dropping the code makes sense to me then.

Any objections or better suggestions from the macos camp?
If not I'll go queue it for the next UI pull request in a day or two.

thanks,
  Gerd

Re: [PATCH] ui/cocoa: Remove the uses of full screen APIs

[Akihiko Odaki]

2021年2月17日(水) 22:09 Gerd Hoffmann <kraxel@redhat.com>:
>
> On Fri, Feb 12, 2021 at 09:05:40AM +0900, Akihiko Odaki wrote:
> > The detections of full screen APIs were wrong. A detection is coded as:
> > [NSView respondsToSelector:@selector(exitFullScreenModeWithOptions:)]
> > but it should be:
> > [NSView instancesRespondToSelector:@selector(exitFullScreenModeWithOptions:)]
> >
> > The uses of full screen APIs were also incorrect, and if you fix the
> > detections, the full screen view stretches the video, changing the
> > aspect ratio, even if zooming is disabled.
> >
> > Remove the code as it does nothing good.
>
> So, it's broken right now (and probably for quite a while without anyone
> complaining).  And the attempt to fix it didn't work out very well.
> Correct?

Because the detections of APIs are wrong, the code using those APIs
were never executed and nobody realized it was broken.

I did not seriously attempt to fix it because the APIs are no longer
the best ways to implement fullscreen. ([NSWindow -toggleFullScreen:]
is more favorable today.) There is not much to reuse even if
implementing fullscreen with [NSView -enterFullScreenModeWithOptions:]
since the code is so small.

>
> Just dropping the code makes sense to me then.
>
> Any objections or better suggestions from the macos camp?
> If not I'll go queue it for the next UI pull request in a day or two.
>
> thanks,
>   Gerd
>

Thank you for responding to my patches.

Akihiko Odaki

Re: [PATCH] ui/cocoa: Remove the uses of full screen APIs

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 1847 bytes --]

On Fri, 19 Feb 2021, Akihiko Odaki wrote:
> 2021年2月17日(水) 22:09 Gerd Hoffmann <kraxel@redhat.com>:
>>
>> On Fri, Feb 12, 2021 at 09:05:40AM +0900, Akihiko Odaki wrote:
>>> The detections of full screen APIs were wrong. A detection is coded as:
>>> [NSView respondsToSelector:@selector(exitFullScreenModeWithOptions:)]
>>> but it should be:
>>> [NSView instancesRespondToSelector:@selector(exitFullScreenModeWithOptions:)]
>>>
>>> The uses of full screen APIs were also incorrect, and if you fix the
>>> detections, the full screen view stretches the video, changing the
>>> aspect ratio, even if zooming is disabled.
>>>
>>> Remove the code as it does nothing good.
>>
>> So, it's broken right now (and probably for quite a while without anyone
>> complaining).  And the attempt to fix it didn't work out very well.
>> Correct?
>
> Because the detections of APIs are wrong, the code using those APIs
> were never executed and nobody realized it was broken.

Full screen on MacOS X worked when I've last tried but that was 2 years 
ago.

> I did not seriously attempt to fix it because the APIs are no longer
> the best ways to implement fullscreen. ([NSWindow -toggleFullScreen:]
> is more favorable today.) There is not much to reuse even if
> implementing fullscreen with [NSView -enterFullScreenModeWithOptions:]
> since the code is so small.

I think there are people using QEMU to run old MacOS versions on MacOS 
X/macOS and may not follow this mailing list but I'm sure they'll complain 
once you break it.

Regards,
BALATON Zoltan

>> Just dropping the code makes sense to me then.
>>
>> Any objections or better suggestions from the macos camp?
>> If not I'll go queue it for the next UI pull request in a day or two.
>>
>> thanks,
>>   Gerd
>>
>
> Thank you for responding to my patches.
>
> Akihiko Odaki
>
>

Re: [PATCH] ui/cocoa: Remove the uses of full screen APIs

[Akihiko Odaki]

2021年2月19日(金) 19:24 BALATON Zoltan <balaton@eik.bme.hu>:
>
> On Fri, 19 Feb 2021, Akihiko Odaki wrote:
> > 2021年2月17日(水) 22:09 Gerd Hoffmann <kraxel@redhat.com>:
> >>
> >> On Fri, Feb 12, 2021 at 09:05:40AM +0900, Akihiko Odaki wrote:
> >>> The detections of full screen APIs were wrong. A detection is coded as:
> >>> [NSView respondsToSelector:@selector(exitFullScreenModeWithOptions:)]
> >>> but it should be:
> >>> [NSView instancesRespondToSelector:@selector(exitFullScreenModeWithOptions:)]
> >>>
> >>> The uses of full screen APIs were also incorrect, and if you fix the
> >>> detections, the full screen view stretches the video, changing the
> >>> aspect ratio, even if zooming is disabled.
> >>>
> >>> Remove the code as it does nothing good.
> >>
> >> So, it's broken right now (and probably for quite a while without anyone
> >> complaining).  And the attempt to fix it didn't work out very well.
> >> Correct?
> >
> > Because the detections of APIs are wrong, the code using those APIs
> > were never executed and nobody realized it was broken.
>
> Full screen on MacOS X worked when I've last tried but that was 2 years
> ago.
>
> > I did not seriously attempt to fix it because the APIs are no longer
> > the best ways to implement fullscreen. ([NSWindow -toggleFullScreen:]
> > is more favorable today.) There is not much to reuse even if
> > implementing fullscreen with [NSView -enterFullScreenModeWithOptions:]
> > since the code is so small.
>
> I think there are people using QEMU to run old MacOS versions on MacOS
> X/macOS and may not follow this mailing list but I'm sure they'll complain
> once you break it.

It was not clear what "full screen APIs" refer to in my patch. Today
macOS have three different methods to enter fullscreen:
- [NSWindow -toggleFullscreen:] (the latest one)
- [NSView -enterFullScreenModeWithOptions:]
- Make a borderless window whose frame matches with the screen

ui/cocoa checks if [NSView -enterFullScreenModeWithOptions:] exists
and uses it in this case. Otherwise, it chooses the last method.
However, the detection of [NSView -enterFullScreenModeWithOptions:]
was broken and I fixed it to find the use of [NSView
-enterFullScreenModeWithOptions:] was wrong. This patch deletes
references to [NSView -enterFullScreenModeWithOptions:] but the code
implementing the last method is still intact and properly functions.

Akihiko Odaki

>
> Regards,
> BALATON Zoltan
>
> >> Just dropping the code makes sense to me then.
> >>
> >> Any objections or better suggestions from the macos camp?
> >> If not I'll go queue it for the next UI pull request in a day or two.
> >>
> >> thanks,
> >>   Gerd
> >>
> >
> > Thank you for responding to my patches.
> >
> > Akihiko Odaki
> >
> >

Re: [PATCH] ui/cocoa: Remove the uses of full screen APIs

[Gerd Hoffmann]

> > I think there are people using QEMU to run old MacOS versions on MacOS
> > X/macOS and may not follow this mailing list but I'm sure they'll complain
> > once you break it.
> 
> It was not clear what "full screen APIs" refer to in my patch. Today
> macOS have three different methods to enter fullscreen:
> - [NSWindow -toggleFullscreen:] (the latest one)
> - [NSView -enterFullScreenModeWithOptions:]
> - Make a borderless window whose frame matches with the screen
> 
> ui/cocoa checks if [NSView -enterFullScreenModeWithOptions:] exists
> and uses it in this case. Otherwise, it chooses the last method.
> However, the detection of [NSView -enterFullScreenModeWithOptions:]
> was broken and I fixed it to find the use of [NSView
> -enterFullScreenModeWithOptions:] was wrong. This patch deletes
> references to [NSView -enterFullScreenModeWithOptions:] but the code
> implementing the last method is still intact and properly functions.

Ah, that explains why fullscreen worked just fine when I tried it
yesterday in my macos test vm.

Can you update the commit message explaining this please?  The text
above should do it for the most part.

thanks,
  Gerd

[PATCH v2] ui/cocoa: Remove the uses of full screen APIs

[Akihiko Odaki]

The detections of [NSView -enterFullScreen:] and
[NSView -exitFullScreen:] were wrong. A detection is coded as:
[NSView respondsToSelector:@selector(exitFullScreenModeWithOptions:)]
but it should be:
[NSView instancesRespondToSelector:@selector(exitFullScreenModeWithOptions:)]

Because of those APIs were not detected, ui/cocoa always falled
back to a borderless window whose frame matches the screen to
implement fullscreen behavior.

The code using [NSView -enterFullScreen:] and
[NSView -exitFullScreen:] will be used if you fix the detections,
but its behavior is undesirable; the full screen view stretches
the video, changing the aspect ratio, even if zooming is disabled.

This change removes the code as it does nothing good.

Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
---
 ui/cocoa.m | 41 +++++++++++++++--------------------------
 1 file changed, 15 insertions(+), 26 deletions(-)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index 13fba8103e1..36e45cd98b4 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -564,37 +564,26 @@ - (void) toggleFullScreen:(id)sender
         isFullscreen = FALSE;
         [self ungrabMouse];
         [self setContentDimensions];
-        if ([NSView respondsToSelector:@selector(exitFullScreenModeWithOptions:)]) { // test if "exitFullScreenModeWithOptions" is supported on host at runtime
-            [self exitFullScreenModeWithOptions:nil];
-        } else {
-            [fullScreenWindow close];
-            [normalWindow setContentView: self];
-            [normalWindow makeKeyAndOrderFront: self];
-            [NSMenu setMenuBarVisible:YES];
-        }
+        [fullScreenWindow close];
+        [normalWindow setContentView: self];
+        [normalWindow makeKeyAndOrderFront: self];
+        [NSMenu setMenuBarVisible:YES];
     } else { // switch from desktop to fullscreen
         isFullscreen = TRUE;
         [normalWindow orderOut: nil]; /* Hide the window */
         [self grabMouse];
         [self setContentDimensions];
-        if ([NSView respondsToSelector:@selector(enterFullScreenMode:withOptions:)]) { // test if "enterFullScreenMode:withOptions" is supported on host at runtime
-            [self enterFullScreenMode:[NSScreen mainScreen] withOptions:[NSDictionary dictionaryWithObjectsAndKeys:
-                [NSNumber numberWithBool:NO], NSFullScreenModeAllScreens,
-                [NSDictionary dictionaryWithObjectsAndKeys:[NSNumber numberWithBool:NO], kCGDisplayModeIsStretched, nil], NSFullScreenModeSetting,
-                 nil]];
-        } else {
-            [NSMenu setMenuBarVisible:NO];
-            fullScreenWindow = [[NSWindow alloc] initWithContentRect:[[NSScreen mainScreen] frame]
-                styleMask:NSWindowStyleMaskBorderless
-                backing:NSBackingStoreBuffered
-                defer:NO];
-            [fullScreenWindow setAcceptsMouseMovedEvents: YES];
-            [fullScreenWindow setHasShadow:NO];
-            [fullScreenWindow setBackgroundColor: [NSColor blackColor]];
-            [self setFrame:NSMakeRect(cx, cy, cw, ch)];
-            [[fullScreenWindow contentView] addSubview: self];
-            [fullScreenWindow makeKeyAndOrderFront:self];
-        }
+        [NSMenu setMenuBarVisible:NO];
+        fullScreenWindow = [[NSWindow alloc] initWithContentRect:[[NSScreen mainScreen] frame]
+            styleMask:NSWindowStyleMaskBorderless
+            backing:NSBackingStoreBuffered
+            defer:NO];
+        [fullScreenWindow setAcceptsMouseMovedEvents: YES];
+        [fullScreenWindow setHasShadow:NO];
+        [fullScreenWindow setBackgroundColor: [NSColor blackColor]];
+        [self setFrame:NSMakeRect(cx, cy, cw, ch)];
+        [[fullScreenWindow contentView] addSubview: self];
+        [fullScreenWindow makeKeyAndOrderFront:self];
     }
 }
 
-- 
2.24.3 (Apple Git-128)

Re: [PATCH v2] ui/cocoa: Remove the uses of full screen APIs

[Gerd Hoffmann]

  Hi,

> The detections of [NSView -enterFullScreen:] and
> [NSView -exitFullScreen:] were wrong. A detection is coded as:
> [NSView respondsToSelector:@selector(exitFullScreenModeWithOptions:)]
> but it should be:
> [NSView instancesRespondToSelector:@selector(exitFullScreenModeWithOptions:)]
> 
> Because of those APIs were not detected, ui/cocoa always falled
> back to a borderless window whose frame matches the screen to
> implement fullscreen behavior.
> 
> The code using [NSView -enterFullScreen:] and
> [NSView -exitFullScreen:] will be used if you fix the detections,
> but its behavior is undesirable; the full screen view stretches
> the video, changing the aspect ratio, even if zooming is disabled.
> 
> This change removes the code as it does nothing good.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>

Added to UI queue.

thanks,
  Gerd