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 735054 - gtkapplication-quartz-menu unconditionally uses 10.7 features
gtkapplication-quartz-menu unconditionally uses 10.7 features
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-19 13:29 UTC by jessevdk@gmail.com
Modified: 2014-09-29 19:25 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
quartz: Conditionally check for backingScaleFactor (1.20 KB, patch)
2014-08-19 13:45 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
quartz: Add compile-time checks (1.27 KB, patch)
2014-08-19 14:11 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
quartz: Add availability check for >= 10.7 API (1.61 KB, patch)
2014-08-23 09:04 UTC, jessevdk@gmail.com
committed Details | Review

Description jessevdk@gmail.com 2014-08-19 13:29:45 UTC
At gtkapplication-quartz-menu.c line 267, NSScreen backingScaleFactor is used, which was introduced in 10.7. It would be great if this could be only done conditionally when compiling for >= 10.7.
Comment 1 Emmanuele Bassi (:ebassi) 2014-08-19 13:45:01 UTC
Created attachment 283886 [details] [review]
quartz: Conditionally check for backingScaleFactor

The backingScaleFactor selector was introduced in 10.7, so we need to
check for its existence in order to support older framework versions.
Comment 2 jessevdk@gmail.com 2014-08-19 13:53:00 UTC
This doesn't seem to do the trick yet. You actually get a compile time error (not just runtime):

gtkapplication-quartz-menu.c: In function '-[GNSMenuItem didChangeIcon]':
gtkapplication-quartz-menu.c:271: warning: 'NSScreen' may not respond to '-backingScaleFactor'
gtkapplication-quartz-menu.c:271: warning: (Messages without a matching method signature
gtkapplication-quartz-menu.c:271: warning: will be assumed to return 'id' and accept
gtkapplication-quartz-menu.c:271: warning: '...' as arguments.)
gtkapplication-quartz-menu.c:271: error: incompatible type for argument 1 of 'roundf'
Comment 3 Emmanuele Bassi (:ebassi) 2014-08-19 14:11:37 UTC
Created attachment 283896 [details] [review]
quartz: Add compile-time checks

In case we are compiling on a framework version < 10.7.
Comment 4 jessevdk@gmail.com 2014-08-19 14:24:51 UTC
Review of attachment 283896 [details] [review]:

See comments inline

::: gtk/gtkapplication-quartz-menu.c
@@ +264,3 @@
       theme = gtk_icon_theme_get_default ();
 
+#if defined(MAC_OS_X_VERSION_10_7) && (MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_7)

To use these macros, I think you need to #include <AvailabilityMacros.h>

@@ +270,2 @@
       if (![[NSScreen mainScreen] respondsToSelector:@selector(backingScaleFactor)])
         scale = 1.0f;

scale is actually defined as an integer

@@ +273,3 @@
         scale = roundf ([[NSScreen mainScreen] backingScaleFactor]);
+#else
+      scale = 1.0f;

idem
Comment 5 John Ralls 2014-08-19 14:25:34 UTC
There are a few different ways to avoid the "may not respond" warning. The simplest is to call performSelector: instead of sending the selector message directly. You might need to cast the result to avoid the type error.

Another is to wrap it in #ifdef AVAILABLE_MAC_OS_X_VERSION_10_7_AND_LATER. Since the linker generally won't let you run a Gtk app on an OS version earlier than the one you built it on, that works just as well as the respondsToSelector test.

A particularly clever, perhaps too clever, way is shown in gdk/quartz/gdkwindow-quartz.c:

@protocol CanSetStyleMask
- (void)setStyleMask:(int)mask;
@end

...

      if ([impl->toplevel respondsToSelector:@selector(setStyleMask:)])
        {
          NSString *title = [impl->toplevel title];

          [(id<CanSetStyleMask>)impl->toplevel setStyleMask:new_mask];
Comment 6 jessevdk@gmail.com 2014-08-23 09:04:36 UTC
Created attachment 284280 [details] [review]
quartz: Add availability check for >= 10.7 API

This adds both a compile time check for the SDK version being compiled against, as well as a runtime check in the case the resulting binary is run against an older SDK.

I've tested the above patch, and I think it does everything:

1) No need to include AvailabilityMacros, it's already being included by Cocoa
2) Uses the straightforward AVAILABLE_MAC_OS_X_VERSION_10_7_AND_LATER for compile time check
3) Uses respondsToSelector for runtime check (even though unlikely this would happen, it's still the right thing to do)
4) No clever tricks needed :)
Comment 7 Matthias Clasen 2014-09-29 19:20:46 UTC
*** Bug 737561 has been marked as a duplicate of this bug. ***
Comment 8 Matthias Clasen 2014-09-29 19:25:21 UTC
Attachment 284280 [details] pushed as de96cea - quartz: Add availability check for >= 10.7 API