D22365: KNotification macOS native support by NSNotificationCenter

[Weixuan Xiao]

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, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D22365: KNotification macOS native support by NSNotificationCenter

[Weixuan Xiao]

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

[Weixuan Xiao]

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 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

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

[Weixuan Xiao]

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 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...

REVISION DETAIL
  https://phabricator.kde.org/D22365

To: Inoki, rjvbb, nicolasfella
Cc: nicolasfella, broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D22365: KNotification macOS native support by NSNotificationCenter

[Weixuan Xiao]

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

[Weixuan Xiao]

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
  src/knotificationmanager.cpp
  src/notifybymacosnotificationcenter.h
  src/notifybymacosnotificationcenter.mm

To: Inoki, rjvbb
Cc: nicolasfella, broulik, kde-frameworks-devel, LeGast00n, sbergeron, 
michaelh, ngraham, bruns

D22365: KNotification macOS native support by NSNotificationCenter

[Weixuan Xiao]

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
  src/notifybymacosnotificationcenter.h
  src/notifybymacosnotificationcenter.mm

To: Inoki, rjvbb
Cc: nicolasfella, broulik, kde-frameworks-devel, LeGast00n, sbergeron, 
michaelh, ngraham, bruns

D22365: KNotification macOS native support by NSNotificationCenter

[Weixuan Xiao]

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 itself 
> triggers, is this for that?

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

> broulik wrote in notifybymacosnotificationcenter.mm:138
> Why in the constructor?

To clear previously delivered notifications

> broulik wrote in notifybymacosnotificationcenter.mm:169
> A notification doesn't neccessarily have a `pixmap()` but could just be an 
> `iconName()` where you need `QIcon::fromTheme()`

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

> broulik wrote in notifybymacosnotificationcenter.mm:221
> Is there no way to transparently update a notification?

Could have one in UserNotifications framework but not NotificationCenter 
framework.

UserNotifications framework requires macOS 10.14+
So maybe later

REPOSITORY
  R289 KNotifications

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

[Weixuan Xiao]

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
  https://phabricator.kde.org/D22365

AFFECTED FILES
  src/CMakeLists.txt
  src/knotificationmanager.cpp
  src/notifybymacosnotificationcenter.h
  src/notifybymacosnotificationcenter.mm

To: Inoki
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns