Thanks a lot!

Kai Uwe Broulik wrote on 20190718::07:13:16 re: "D22365: KNotification macOS native support by NSNotificationCenter"

Thanks a lot!

Indeed. I've looked a few times in doing this myself but was never convinced that the amount of work was justified and some aspects seemed just impossible (like the kind of control over notification buttons that is expected in a KNotification, IIRC).
(IOW I'll reserve my opinion whether or not I'll be happy with the native behaviour over what I've gotten used to ;))

Summer holidays are never a good time for me to get involved in coding/review efforts but I'll see if I can test this next week. Provided the NSNotification code still builds on my ageing 10.9 install...

notifybymacosnotificationcenter.mm:162
+
+ CFStringRef cfTitle = notification->title().toCFString(),
+ cfText = notification->text().toCFString();

Why not just toNSString(), given you convert it below anyway? Or is that ObjC awful ownership model quirk?

CFString and NSString are supposed to be "toll-free bridged" so you can cast freely between them. I agree, if you need an NSString just use toNSString(); if anything the extra detour might lead to ARC quirks.

+ close(notification);
+ notify(notification, config);

Is there no way to transparently update a notification?

TBH I'd be surprised if there were.

As I thought this needs some hacking on OS X < 10.10 but a priori all one loses is the user notifications.

I'm missing a test case in this diff, i.e. a description how you can test these new notifications because I wouldn't know off the top of my head which applications post this kind of notifications (and notifications via DBus work on my system because I have the necessary services running).

Another question: Qt officially supports/allows all QPA plugins that work on a given platform, which means that it's not impossible to use the XCB "backend" on Mac. It looks like the new code doesn't consider this possibility, and sadly my KWindowSystem changes for Mac have still not made it into a release (they'd allow a runtime isCocoa check).
Still, I would prefer to see something of the sort

if (d->portalDBusServiceExists) {
  plugin = new NotifyByPortal(this);
} else {
#ifdef Q_OS_MACOS
  plugin = new NotifyByMacOSNotificationCenter(this);
#else
  plugin = new NotifyByPopup(this);
#endif
}

FWIW: you should use the Q_OS_MACOS token to test for the Mac desktop OS.

I haven't been able to give this much attention, sorry.

After backporting the patch to OS X 10.9 it does work so I presume it'll work even better with the full functionality availability.

One thing: it has a hardcoded assumption that the Cocoa notication APIs will always be available and usable - IOW, that the Cocoa QPA plugin will always be the one in use. Of course it's very unlikely to encounter other QPAs in the wild that support full-fledged GUIs but what about the few other cross-platform QPA plugins (minimal and offscreen to name just 2)? Will notifications be disabled upstream of the platform implementation when those are used, for whatever reason?

Because if not, the SDK will throw an exception, or (more likely), something will crash because the integration layer returns nullptr for a required platform function. I found that out when a notification was triggered while I was using the XCB QPA (I use Konsole as my X11 terminal emulator under XQuartz, and also do some remote displaying).

I haven't looked closely at the implementation but I assume it should not be costly to check the platform name before deciding to use the new Mac notification backend, just as a preparation for possible future changes (Qt don't formally outlaw the XCB QPA, for instance).

In D22365#532294, @rjvbb wrote:

I haven't been able to give this much attention, sorry.

After backporting the patch to OS X 10.9 it does work so I presume it'll work even better with the full functionality availability.

One thing: it has a hardcoded assumption that the Cocoa notication APIs will always be available and usable - IOW, that the Cocoa QPA plugin will always be the one in use. Of course it's very unlikely to encounter other QPAs in the wild that support full-fledged GUIs but what about the few other cross-platform QPA plugins (minimal and offscreen to name just 2)? Will notifications be disabled upstream of the platform implementation when those are used, for whatever reason?

Because if not, the SDK will throw an exception, or (more likely), something will crash because the integration layer returns nullptr for a required platform function. I found that out when a notification was triggered while I was using the XCB QPA (I use Konsole as my X11 terminal emulator under XQuartz, and also do some remote displaying).

I haven't looked closely at the implementation but I assume it should not be costly to check the platform name before deciding to use the new Mac notification backend, just as a preparation for possible future changes (Qt don't formally outlaw the XCB QPA, for instance).

Yeah, you're right that we should check system version for back-compatibility.
AFAIU, we can check it before the creation of notification backend instance, if not compatible, use the old implementation, right?

I don't know XCB QPA stuff. Investigating then...

Yeah, you're right that we should check system version for back-compatibility.

Actually, I didn't want to suggest that. I haven't checked, but I think that the Qt requirement already ensures that the Cocoa APIs that you are using are present.

AFAIU, we can check it before the creation of notification backend instance, if not compatible, use the old implementation, right?

Yes - but if the backend compiles (without or with #ifdefs).

I don't know XCB QPA stuff. Investigating then...

Don't take my XCB QPA remarks as a blocker, but do have a look if there's some kind of mechanism in place to check whether an application is running in a full GUI environment. Not necessarily as part of this change request - I'm thinking out loud here, in fact.

There's something for notifications for iOS and macOS here: https://developer.apple.com/documentation/foundation/nsnotification
Would this patch already work on iOS or could a common API for both be used?

(I have no experiences with iOS or macOS, but we (Kaidan) would like to support notifications on iOS in the future.)

In D22365#538245, @lnj wrote:

There's something for notifications for iOS and macOS here: https://developer.apple.com/documentation/foundation/nsnotification
Would this patch already work on iOS or could a common API for both be used?

(I have no experiences with iOS or macOS, but we (Kaidan) would like to support notifications on iOS in the future.)

Yes, I use NSNotification API and in theory, it works on iOS but I have no idea how to test.

One more thing: Apple plans to mark NSNotification as Deprecated and uses UserNotification API in the newer macOS(10.15+) and iOS(13+). It will be nice to use that set of APIs for Apple's platform

Updating with new incoming code.

Will try landing it this week