KNotification macOS native support by NSNotificationCenter
Needs ReviewPublic

Authored by Inoki on Jul 10 2019, 8:25 AM.

Details

Reviewers
rjvbb
Summary

Add macOS native support to KNotification

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
Inoki created this revision.Jul 10 2019, 8:25 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptJul 10 2019, 8:25 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
Inoki requested review of this revision.Jul 10 2019, 8:25 AM

Thanks a lot!

src/notifybymacosnotificationcenter.h
7

Unused

15

Add override

src/notifybymacosnotificationcenter.mm
8

Unused?

21

I don't really see the point in those methods, since it's "private" you can just have KNotification manipulate m_notifications directly.

29

If possible use Q_FORWARD_DECLARE_OBJC_CLASS to forward-declare the type instead of using generic id

31

Initialize right here = 0; or in the constructor initializer list, not the constructor body

35

Don't do global statics in a library, e.g. use Q_GLOBAL_STATIC or on-demand initialization / refcounting

53

Just declare this down in the switch below, you will have to wrap it in {} to avoid the jump to case label error

62

We have a "default action" concept now where clicking the popup itself triggers, is this for that?

71

Coding style

if (!originNotification) {
    break;
}
97

Don't call values() just to iterate it, use iterators, for (auto it = m_notifications.constBegin(), ...)

111

Coding style:

if (!notification) {
    return;
}
118

Avoid using operator[] unless you're assigning something

139

Why in the constructor?

146

Are all of these debug messages neccessary?

160

Coding style:
One variable per line

NSString *..;
NSString *...;

same below

163

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

170

A notification doesn't neccessarily have a pixmap() but could just be an iconName() where you need QIcon::fromTheme()

222

Is there no way to transparently update a notification?

rjvbb added a comment.Jul 18 2019, 8:48 AM

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.

Inoki marked 16 inline comments as done.Tue, Jul 23, 3:40 PM
Inoki added inline comments.
src/notifybymacosnotificationcenter.h
15

Which one?

src/notifybymacosnotificationcenter.mm
62

It could be. But seems that not all app provides defaultAction, so I do a check first

139

To clear previously delivered notifications

170

KDE Connect does set icon by pixmap, any idea how should we treat these?

222

Could have one in UserNotifications framework but not NotificationCenter framework.

UserNotifications framework requires macOS 10.14+
So maybe later

Inoki updated this revision to Diff 62414.Tue, Jul 23, 3:41 PM
Inoki marked 2 inline comments as done.
broulik added inline comments.Wed, Jul 24, 8:17 AM
src/notifybymacosnotificationcenter.mm
170

Check for pixmap().isNull() and use iconName() then

Inoki updated this revision to Diff 62797.Tue, Jul 30, 3:01 PM

Add icon from theme as an alternative while pixmap not set

Inoki marked 5 inline comments as done.Tue, Jul 30, 3:05 PM
rjvbb added a comment.Thu, Aug 1, 4:16 PM

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.