Review of attachment 300718 [details] [review]:

See the inline comments.

::: gio/gcocoanotificationbackend.c
@@ +79,3 @@
+  else
+    {
+      g_warning ("This icon type is not handled for Notifications\n");

no need for \n

@@ +92,3 @@
+  if (!g_str_has_prefix (action, "app."))
+    {
+      g_warning ("Notification action does not have \"app.\" prefix.\n");

the same no need for \n

@@ +215,3 @@
+  /* NOTE: There is no priority */
+
+  NSUserNotification *userNotification = [NSUserNotification new];

The declaration should be at the top I guess

@@ +223,3 @@
+  add_actions_to_notification (userNotification, notification);
+
+  NSUserNotificationCenter *center = [NSUserNotificationCenter defaultUserNotificationCenter];

same here

::: gio/gnotificationbackend.c
@@ +56,3 @@
   backend->application = application;
 
+  backend->dbus_connection = g_application_get_dbus_connection (application);

this change sounds like something that should go in a different patch

Created attachment 300768 [details] [review]
Implement GNotification on OSX

(In reply to Ignacio Casal Quinteiro (nacho) from comment #1)
> Review of attachment 300718 [details] [review] [review]:
> ...
> this change sounds like something that should go in a different patch

The NULL check on `backend->dbus_connection` is just to silence a critical warning on systems that don't dbus.. I'm not sure who thought it was a good idea to put that in the backend instead of actual implementations that do require dbus.

Review of attachment 300768 [details] [review]:

Hi, just a couple quick comments for now, I still need to figure out how to test it properly...

Do you know if there a way to specify in configure.ac that the minimum Mac OS version required is 10.8 with this change?

::: gio/gcocoanotificationbackend.c
@@ +53,3 @@
+    return nil;
+
+  return [[NSString alloc] initWithUTF8String:cstr];

It might be better to use [NSString stringWithUTF8String:(const char *)bytes] here (and not call release everywhere else) since the function name doesn't indicate that it's owned by the caller.

@@ +70,3 @@
+        {
+          NSString *str_path = nsstring_from_cstr (path);
+          image = [[NSImage alloc] initByReferencingFile:str_path];

Same with this. The function name doesn't indicate the caller owns the NSImage, so might be better to autorelease it right here.

@@ +217,3 @@
+  /* NOTE: There is no priority */
+
+  userNotification = [NSUserNotification new];

I think the userNotification created here should be released after it's delivered.

@@ +226,3 @@
+
+  center = [NSUserNotificationCenter defaultUserNotificationCenter];
+  center.delegate = [[GNotificationCenterDelegate alloc] init];

It's better to create only one global GNotificationCenterDelegate instead of creating a new one for each notification sent.

Review of attachment 300768 [details] [review]:

Can you tell me how you tested your code? I can't seem to get the gio/tests/gnotification test to deliver any NSUserNotifications.

Also, I run into an abort due to the g_warning() in nsimage_from_gicon function. I had to comment it out to continue testing.

Also, the Mac OS version required I guess is 10.9 because of the usage of the identifier property of NSUserNotification.

::: gio/gcocoanotificationbackend.c
@@ +131,3 @@
+
+static gboolean
+g_cocoa_notification_backend_is_supported (void)

Also, is there a reason we check for a bundle identifier here? Why can't we just return TRUE?

(In reply to William Hua from comment #3)
> Review of attachment 300768 [details] [review] [review]:
> 
> Do you know if there a way to specify in configure.ac that the minimum Mac
> OS version required is 10.8 with this change?
>

It was on my todo list, will look into it.

> ::: gio/gcocoanotificationbackend.c
> @@ +53,3 @@
> +    return nil;
> +
> +  return [[NSString alloc] initWithUTF8String:cstr];
> 
> It might be better to use [NSString stringWithUTF8String:(const char
> *)bytes] here (and not call release everywhere else) since the function name
> doesn't indicate that it's owned by the caller.
> 
> @@ +70,3 @@
> +        {
> +          NSString *str_path = nsstring_from_cstr (path);
> +          image = [[NSImage alloc] initByReferencingFile:str_path];
> 
> Same with this. The function name doesn't indicate the caller owns the
> NSImage, so might be better to autorelease it right here.
> 
> @@ +217,3 @@
> +  /* NOTE: There is no priority */
> +
> +  userNotification = [NSUserNotification new];
> 
> I think the userNotification created here should be released after it's
> delivered.
> 
> @@ +226,3 @@
> +
> +  center = [NSUserNotificationCenter defaultUserNotificationCenter];
> +  center.delegate = [[GNotificationCenterDelegate alloc] init];
> 
> It's better to create only one global GNotificationCenterDelegate instead of
> creating a new one for each notification sent.

Ok.

>Can you tell me how you tested your code? I can't seem to get the gio/tests/gnotification test to deliver any >NSUserNotifications.

Well it has to be in a bundle. Here was my very simple hack of a bundle (it doesn't actually bundle any libs so depends on
running inside of jhbuild, you can even just call ./main directly as long as the bundle layout is intact). https://www.dropbox.com/s/b6jgmwkl3i6jb1i/OSX_BUNDLE.zip?dl=0

>::: gio/gcocoanotificationbackend.c
>@@ +131,3 @@
>+
>+static gboolean
>+g_cocoa_notification_backend_is_supported (void)
>
>Also, is there a reason we check for a bundle identifier here? Why can't we just return TRUE?

Well it won't work without a bundle but there is no harm in it saying its always supported, it just wont do anything.

> >Can you tell me how you tested your code? I can't seem to get the gio/tests/gnotification test to deliver any >NSUserNotifications.
> 
> Well it has to be in a bundle. Here was my very simple hack of a bundle (it
> doesn't actually bundle any libs so depends on
> running inside of jhbuild, you can even just call ./main directly as long as
> the bundle layout is intact).
> https://www.dropbox.com/s/b6jgmwkl3i6jb1i/OSX_BUNDLE.zip?dl=0

Ok, thanks, I didn't know that. I ended up using this swizzle patch to get the notifications working: https://github.com/norio-nomura/usernotification (but the gio/tests/gnotification test is still failing even though it delivers notifications properly now).



> >::: gio/gcocoanotificationbackend.c
> >@@ +131,3 @@
> >+
> >+static gboolean
> >+g_cocoa_notification_backend_is_supported (void)
> >
> >Also, is there a reason we check for a bundle identifier here? Why can't we just return TRUE?
> 
> Well it won't work without a bundle but there is no harm in it saying its
> always supported, it just wont do anything.

Ok, please keep checking for the bundle identifier then, it looks good as is.

> ::: gio/gcocoanotificationbackend.c
> @@ +53,3 @@
> +    return nil;
> +
> +  return [[NSString alloc] initWithUTF8String:cstr];
> 
> It might be better to use [NSString stringWithUTF8String:(const char
> *)bytes] here (and not call release everywhere else) since the function name
> doesn't indicate that it's owned by the caller.
> 
> @@ +70,3 @@
> +        {
> +          NSString *str_path = nsstring_from_cstr (path);
> +          image = [[NSImage alloc] initByReferencingFile:str_path];
> 
> Same with this. The function name doesn't indicate the caller owns the
> NSImage, so might be better to autorelease it right here.

Actually, these two comments I made, please disregard. I don't know if autoreleasing is a good idea any more because I'm not sure if we will get an autorelease pool in general.

(In reply to William Hua from comment #7)
> Actually, these two comments I made, please disregard. I don't know if
> autoreleasing is a good idea any more because I'm not sure if we will get an
> autorelease pool in general.

I admit I don't know a ton about obj-c, but I attempted to use an autorelease pool previously and it did not appear to work, where as I am certain manual releases do work.

Created attachment 301476 [details] [review]
Require OSX 10.9

Created attachment 301539 [details] [review]
Implement GNotification on OSX

>I think the userNotification created here should be released after it's delivered.

Done.

>It's better to create only one global GNotificationCenterDelegate instead of creating a new one for each notification sent.

Done.

> Also, I run into an abort due to the g_warning() in nsimage_from_gicon function. I
> had to comment it out to continue testing.

Can't reproduce, it warns and works as expected here.

(In reply to tingping from comment #10)
> Created attachment 301539 [details] [review] [review]
> Implement GNotification on OSX
> 
> >I think the userNotification created here should be released after it's delivered.
> 
> Done.
> 
> >It's better to create only one global GNotificationCenterDelegate instead of creating a new one for each notification sent.
> 
> Done.

Hi, sorry I don't see those changes in this attachment, can you please try uploading it again?

Also, I was thinking, sorry about this, maybe it's better to leave the required version at 10.7, and conditionally compile the notification backend if the version is at least 10.9? What do you think?

Created attachment 301901 [details] [review]
Implement GNotification on OSX

> Hi, sorry I don't see those changes in this attachment, can you please try uploading
> it again?

Oops.

> Also, I was thinking, sorry about this, maybe it's better to leave the
> required version at 10.7, and conditionally compile the notification backend
> if the version is at least 10.9? What do you think?

My understanding is the current state of OSX support is "Some users are stuck on 10.6 so lets try to support that" but the reality is there are almost no developers on OSX even remotely testing that. I have a handful of commits to improve GLib on OSX and I certainly am not able to test 10.6. 

I think the goal of pushing the required version to 10.8 (which is already unsupported by Apple afaik) or even 10.9 entirely is actually good for this project as it allows for reasonable improvements just as they are dropping support for XP now.

That all said since this is a fairly self contained patch it would not be hard to support everything we currently do if thats what glib wants.

Review of attachment 301476 [details] [review]:

This works.

Review of attachment 301901 [details] [review]:

Sorry I couldn't get around to this till now. But this looks good now, thanks!

I actually think the OSX version check is not sufficient since you can target older releases and we aren't checking that.

What do you mean? The identifier of NSUserNotification is only available since Mac OS 10.9.

(In reply to William Hua from comment #17)
> What do you mean? The identifier of NSUserNotification is only available
> since Mac OS 10.9.

You can build with things like -mmacosx-version-min=10.6 or target a specific SDK directory even if you are building on 10.10. When I get a chance I'll prob just have it try to build a snippet of code as the test. I also think I will make the identifier field optional to keep 10.8.

Just to clarify, you want to keep the minimum version of Mac OS for building as 10.9 and the minimum version for running as 10.8 with a runtime check then? That sounds good, I think your configure.ac patch is still ok as is.

Attachment 301901 [details] pushed as 36e093a - Implement GNotification on OSX

I'm a manager of the MacPorts package management system and the maintainer of our glib2 ports. MacPorts currently runs on Mac OS X 10.4 and later, and glib2 compiles fine on 10.4. Changing the minimum build version to 10.9 will mean that I probably will not want to update glib2 to any newer versions, for several years, until 10.8 and earlier are no longer used. I would ask you to restore support for building on 10.4 and later.

(In reply to Ryan Schmidt from comment #21)

Commenting on a closed bug won't bring this matter to the attention of the maintainers. I strongly encourage you to send an email to gtk-devel-list@gnome.org instead, and at least discuss this on the public mailing list.

> I'm a manager of the MacPorts package management system and the maintainer
> of our glib2 ports. MacPorts currently runs on Mac OS X 10.4 and later, and
> glib2 compiles fine on 10.4.

The fact that GLib compiled fine on a 10 years old, unsupported version of MacOS does not imply that it will continue to do so forever. GLib is not a dead project: new functionality is added in every cycle, and that functionality has minimum requirements. We can implement fall back paths, and try to paper over differences in implementation, but that does not mean that we don't have minimum requirements — especially when it comes to platform support.

> Changing the minimum build version to 10.9 will
> mean that I probably will not want to update glib2 to any newer versions,
> for several years, until 10.8 and earlier are no longer used.

User base of MacOS as of January 2015:

http://www.intego.com/mac-security-blog/os-x-market-share-statistics-1-in-5-macs-still-unsupported/

We could probably lower the build requirement to 10.8, which is used by 8% of the user base (with 10.9 used by 28%, and 10.10 used by 45% of the user base).  That would bring us to cover 80% of the installed user base. Considering that less than 2% is using versions prior to 10.6, I think the requirement to support 10.4 is not really feasible, both in terms of resources spent and results.

On one hand I understand the issue of supporting an unsupported operating system on an unsupported architecture like PPC — after all the people that bought a Mac 10 years ago couldn't possibly know what would happen; on the other hand, though, I don't think GLib is targeting the retro-computing scene, and considering that GLib is still getting new API, new features, and new requirements, it gets more and more infeasible to extend the support backwards.

Sure: GLib may *build* on MacOS 10.4, but how does it run? What portion of the API is even exercised by applications using it?

In this specific case: we could detect at configure-time that the version of MacOS is too old and simply disable the GNotification backend; after all, it's a backend; the points I raised still remain, and I'm sure we'll have this discussion once again down the line, once we add a new feature in GLib that maps to MacOS API. That's why I'd rather have this discussion on gtk-devel-list than on Bugzilla.

The configure check looks at the version of OSX on which it's running, which isn't what's important. What's important is the SDK in use. I routinely build with the 10.5 SDK on a Mac running the latest release. That passes the configure check but fails the build because the 10.5 SDK lacks the NSUserNotification protocol.

(In reply to John Ralls from comment #23)
> The configure check looks at the version of OSX on which it's running, which
> isn't what's important. What's important is the SDK in use.

Yea we discussed that on IRC but a patch was never made. If you had the time to make one that would be great, I don't really have the setup or knowledge for it atm.

Just for reference, the discussion on gtk-devel-list starts here: https://mail.gnome.org/archives/gtk-devel-list/2015-September/msg00025.html

(In reply to Emmanuele Bassi (:ebassi) from comment #25)
> Just for reference, the discussion on gtk-devel-list starts here:
> https://mail.gnome.org/archives/gtk-devel-list/2015-September/msg00025.html

Bit late for that thread but I also implemented a growl backend a while ago: https://github.com/TingPing/glib/tree/tingping/gnotification-gntp

I just don't think that should be maintained in glib, it is very specific to a subset of outdated osx users.

Created attachment 322497 [details] [review]
Fix the configure check so that it looks for what we're actually building.

Created attachment 322498 [details] [review]
Bypass building gcocoanotification if the OS X version is < 10.9.

Either of the two patches I've just attached resolves the problem I identified in comment 23. I prefer the second one because it restores the ability to use GLib on older versions of OS X, albeit without notification support. Please let's decide quickly so that the change can make it into 2.46.

Sorry, s/2.46/2.48/

Created attachment 326811 [details] [review]
Bypass building gcocoanotification if the OS X version is < 10.9.

Replaced the second patch after discovering that I'd neglected to remove references to g_cocoa_notification_backend_get_type().

Created attachment 342185 [details] [review]
Bypass building gcocoanotification if mac_os_x_min_version_required < 10.9

Update the bypass patch to fix an error noted in bug 776219.

Created attachment 342186 [details] [review]
Bypass building gcocoanotification if mac_os_x_min_version_required < 10.9

Sigh. Got the inequality backwards.

*** Bug 752625 has been marked as a duplicate of this bug. ***