diff mbox series

ui/cocoa: Remove the uses of full screen APIs

Message ID 20210212000540.28486-1-akihiko.odaki@gmail.com (mailing list archive)
State New, archived
Headers show
Series ui/cocoa: Remove the uses of full screen APIs | expand

Commit Message

Akihiko Odaki Feb. 12, 2021, 12:05 a.m. UTC
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(-)

Comments

Gerd Hoffmann Feb. 17, 2021, 1:09 p.m. UTC | #1
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
Akihiko Odaki Feb. 19, 2021, 9:38 a.m. UTC | #2
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
BALATON Zoltan Feb. 19, 2021, 10:24 a.m. UTC | #3
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
>
>
Akihiko Odaki Feb. 19, 2021, 11:01 a.m. UTC | #4
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
> >
> >
Gerd Hoffmann Feb. 19, 2021, 1:55 p.m. UTC | #5
> > 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
diff mbox series

Patch

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];
     }
 }