After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 735283 - gdkwindow-quartz: Support native fullscreen mode
gdkwindow-quartz: Support native fullscreen mode
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Backend: Quartz
3.13.x
Other Mac OS
: Normal normal
: ---
Assigned To: gtk-quartz maintainers
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2014-08-23 13:43 UTC by jessevdk@gmail.com
Modified: 2014-08-25 10:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gdkwindow-quartz: Support native fullscreen mode (5.57 KB, patch)
2014-08-23 13:43 UTC, jessevdk@gmail.com
needs-work Details | Review
gdkwindow-quartz: Support native fullscreen mode (18.06 KB, patch)
2014-08-23 13:49 UTC, jessevdk@gmail.com
reviewed Details | Review
gdkwindow-quartz: Support native fullscreen mode (5.44 KB, patch)
2014-08-23 14:10 UTC, jessevdk@gmail.com
committed Details | Review

Description jessevdk@gmail.com 2014-08-23 13:43:37 UTC
Created attachment 284290 [details] [review]
gdkwindow-quartz: Support native fullscreen mode

This implements native fullscreen mode on OS X >= 10.7. This patch
adds tracking of the fullscreen mode if externally modified, as well
as toggling the native fullscreen mode of the window as needed.
Comment 1 Ignacio Casal Quinteiro (nacho) 2014-08-23 13:46:08 UTC
Review of attachment 284290 [details] [review]:

First round of nitpicks :)

::: gdk/quartz/GdkQuartzNSWindow.c
@@ +664,3 @@
+  [super setStyleMask:styleMask];
+
+  isfullscreen = (([self styleMask] & NSFullScreenWindowMask) != 0);

same as for gedit I'd say is_fullscreen

@@ +666,3 @@
+  isfullscreen = (([self styleMask] & NSFullScreenWindowMask) != 0);
+
+  if (wasfullscreen != isfullscreen)

was_fullscreen

::: gdk/quartz/gdkwindow-quartz.c
@@ +2661,3 @@
+  GdkWindowImplQuartz *impl = GDK_WINDOW_IMPL_QUARTZ (window->impl);
+  return ([impl->toplevel styleMask] & NSFullScreenWindowMask) != 0;
+}

empty line

@@ +2695,3 @@
+_gdk_quartz_window_update_fullscreen_state (GdkWindow *window)
+{
+  gboolean isfullscreen;

same as in the other part
Comment 2 jessevdk@gmail.com 2014-08-23 13:49:12 UTC
Created attachment 284291 [details] [review]
gdkwindow-quartz: Support native fullscreen mode

This implements native fullscreen mode on OS X >= 10.7. This patch
adds tracking of the fullscreen mode if externally modified, as well
as toggling the native fullscreen mode of the window as needed.
Comment 3 Emmanuele Bassi (:ebassi) 2014-08-23 13:59:14 UTC
Review of attachment 284291 [details] [review]:

there are lots of whitespace changes; care to avoid them? it makes reviewing the patch more complicated.

it looks generally okay to me, but I can't currently test it.

::: gdk/quartz/gdkwindow-quartz.c
@@ +2654,3 @@
 }
 
+#ifdef AVAILABLE_MAC_OS_X_VERSION_10_7_AND_LATER

I don't particularly like redefining whole functions; can't these go into the bodies of each existing function?
Comment 4 jessevdk@gmail.com 2014-08-23 14:01:44 UTC
(In reply to comment #3)
> Review of attachment 284291 [details] [review]:
> 
> there are lots of whitespace changes; care to avoid them? it makes reviewing
> the patch more complicated.

Sorry about that, that was unintentional (the first patch didn't have those I think). Will fix.

> ::: gdk/quartz/gdkwindow-quartz.c
> @@ +2654,3 @@
>  }
> 
> +#ifdef AVAILABLE_MAC_OS_X_VERSION_10_7_AND_LATER
> 
> I don't particularly like redefining whole functions; can't these go into the
> bodies of each existing function?

I did it mostly to avoid having another ifdef for the get_fullscreen_geometry function declaration. But I can split it out.
Comment 5 jessevdk@gmail.com 2014-08-23 14:10:12 UTC
Created attachment 284293 [details] [review]
gdkwindow-quartz: Support native fullscreen mode

This implements native fullscreen mode on OS X >= 10.7. This patch
adds tracking of the fullscreen mode if externally modified, as well
as toggling the native fullscreen mode of the window as needed.

 - Removed whitespace changes
 - Simplify non-native force unfullscreen on hide
Comment 6 Emmanuele Bassi (:ebassi) 2014-08-25 10:20:18 UTC
Review of attachment 284293 [details] [review]:

looks okay, and I assume you tested it.
Comment 7 jessevdk@gmail.com 2014-08-25 10:54:54 UTC
Comment on attachment 284293 [details] [review]
gdkwindow-quartz: Support native fullscreen mode

Thanks for the review!