D22365: KNotification macOS native support by NSNotificationCenter

Inoki closed this revision. Inoki added a comment. Landed: https://cgit.kde.org/knotifications.git/commit/?id=58caf6ad5b48aa573a6d3b8d3e23e07afce8a03f REVISION DETAIL https://phabricator.kde.org/D22365 To: Inoki, rjvbb, nicolasfella Cc: lnj, nicolasfella, broulik, kde-frameworks-devel,

D22365: KNotification macOS native support by NSNotificationCenter

Inoki added a comment. Updating with new incoming code. Will try landing it this week REVISION DETAIL https://phabricator.kde.org/D22365 To: Inoki, rjvbb, nicolasfella Cc: lnj, nicolasfella, broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D22365: KNotification macOS native support by NSNotificationCenter

Inoki added a comment. 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

D22365: KNotification macOS native support by NSNotificationCenter

lnj added a comment. 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)

D22365: KNotification macOS native support by NSNotificationCenter

rjvbb added a comment. > 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,

D22365: KNotification macOS native support by NSNotificationCenter

Inoki added a comment. 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

D22365: KNotification macOS native support by NSNotificationCenter

rjvbb added a comment. 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

D22365: KNotification macOS native support by NSNotificationCenter

nicolasfella accepted this revision. nicolasfella added a comment. This revision is now accepted and ready to land. Let's get this in REVISION DETAIL https://phabricator.kde.org/D22365 To: Inoki, rjvbb, nicolasfella Cc: nicolasfella, broulik, kde-frameworks-devel, LeGast00n, GB_2,

D22365: KNotification macOS native support by NSNotificationCenter

rjvbb added a comment. 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

D22365: KNotification macOS native support by NSNotificationCenter

Inoki marked 5 inline comments as done. REVISION DETAIL https://phabricator.kde.org/D22365 To: Inoki, rjvbb Cc: nicolasfella, broulik, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22365: KNotification macOS native support by NSNotificationCenter

Inoki updated this revision to Diff 62797. Inoki added a comment. Add icon from theme as an alternative while pixmap not set CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22365?vs=62414=62797 REVISION DETAIL https://phabricator.kde.org/D22365 AFFECTED FILES src/CMakeLists.txt

D22365: KNotification macOS native support by NSNotificationCenter

broulik added inline comments. INLINE COMMENTS > Inoki wrote in notifybymacosnotificationcenter.mm:169 > KDE Connect does set icon by pixmap, any idea how should we treat these? Check for `pixmap().isNull()` and use `iconName()` then REVISION DETAIL https://phabricator.kde.org/D22365 To:

D22365: KNotification macOS native support by NSNotificationCenter

Inoki updated this revision to Diff 62414. Inoki marked 2 inline comments as done. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22365?vs=61487=62414 REVISION DETAIL https://phabricator.kde.org/D22365 AFFECTED FILES src/CMakeLists.txt src/knotificationmanager.cpp

D22365: KNotification macOS native support by NSNotificationCenter

Inoki marked 16 inline comments as done. Inoki added inline comments. INLINE COMMENTS > broulik wrote in notifybymacosnotificationcenter.h:14 > Add `override` Which one? > broulik wrote in notifybymacosnotificationcenter.mm:61 > We have a "default action" concept now where clicking the popup

D22365: KNotification macOS native support by NSNotificationCenter

rjvbb added a comment. Kai Uwe Broulik wrote on 20190718::07:13:16 re: "D22365 <https://phabricator.kde.org/D22365>: KNotification macOS native support by NSNotificationCenter" > Thanks a lot! Indeed. I've looked a few times in doing this myself but

D22365: KNotification macOS native support by NSNotificationCenter

broulik added a comment. Thanks a lot! INLINE COMMENTS > notifybymacosnotificationcenter.h:6 > + > +class MacOSNotificationCenterPrivate; > + Unused > notifybymacosnotificationcenter.h:14 > +NotifyByMacOSNotificationCenter(QObject* parent); > +~NotifyByMacOSNotificationCenter(); >

D22365: KNotification macOS native support by NSNotificationCenter

broulik added a reviewer: rjvbb. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D22365 To: Inoki, rjvbb Cc: kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22365: KNotification macOS native support by NSNotificationCenter

Inoki created this revision. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. Inoki requested review of this revision. REVISION SUMMARY Add macOS native support to KNotification REPOSITORY R289 KNotifications REVISION DETAIL