D22365: KNotification macOS native support by NSNotificationCenter

[Kai Uwe Broulik]

broulik added a comment.


  Thanks a lot!

INLINE COMMENTS

> notifybymacosnotificationcenter.h:6
> +
> +class MacOSNotificationCenterPrivate;
> +

Unused

> notifybymacosnotificationcenter.h:14
> +    NotifyByMacOSNotificationCenter(QObject* parent);
> +    ~NotifyByMacOSNotificationCenter();
> +

Add `override`

> notifybymacosnotificationcenter.mm:7
> +#include <QIcon>
> +#include <QLoggingCategory>
> +#include <QDebug>

Unused?

> notifybymacosnotificationcenter.mm:20
> +
> +    void insertKNotification(int internalId, KNotification *notification);
> +    KNotification *getKNotification(int internalId);

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

> notifybymacosnotificationcenter.mm:28
> +private:
> +    id m_delegate;
> +

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

> notifybymacosnotificationcenter.mm:30
> +
> +    int m_internalCounter;
> +    QMap<int, KNotification*> m_notifications;

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

> notifybymacosnotificationcenter.mm:34
> +
> +static MacOSNotificationCenterPrivate macosNotificationCenterPrivate;
> +

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

> notifybymacosnotificationcenter.mm:52
> +{
> +    KNotification *originNotification;
> +    qCDebug(LOG_KNOTIFICATIONS) << "User clicked on notification " 

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

> notifybymacosnotificationcenter.mm:61
> +            break;
> +        case NSUserNotificationActivationTypeContentsClicked:
> +            qCDebug(LOG_KNOTIFICATIONS) << "Contents clicked";

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

> notifybymacosnotificationcenter.mm:70
> +            originNotification = macosNotificationCenterPrivate.getKNotification([notification.userInfo[@"internalId"] intValue]);
> +            if (!originNotification) break;
> +            emit originNotification->activate([notification.additionalActivationAction.identifier intValue] + 1);

Coding style

  if (!originNotification) {
      break;
  }

> notifybymacosnotificationcenter.mm:96
> +    // Try to finish all KNotification
> +    for (KNotification *notification : m_notifications.values()) {
> +        notification->deref();

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

> notifybymacosnotificationcenter.mm:110
> +{
> +    if (!notification) return;
> +

Coding style:

  if (!notification) {
      return;
  }

> notifybymacosnotificationcenter.mm:117
> +{
> +    return m_notifications[internalId];
> +}

Avoid using `operator[]` unless you're assigning something

> notifybymacosnotificationcenter.mm:138
> +{
> +    // Clear notifications
> +    NSArray<NSUserNotification *> *deliveredNotifications = [NSUserNotificationCenter defaultUserNotificationCenter].deliveredNotifications;

Why in the constructor?

> notifybymacosnotificationcenter.mm:145
> +
> +    qCDebug(LOG_KNOTIFICATIONS) << "Knotification macos backend created";
> +}

Are all of these debug messages neccessary?

> notifybymacosnotificationcenter.mm:159
> +    NSUserNotification *osxNotification = [[[NSUserNotification alloc] init] autorelease];
> +    NSString *notificationId = [NSString stringWithFormat: @"%d", notification->id()],
> +             *internalNotificationId = [NSString stringWithFormat: @"%d", internalId];

Coding style:
One variable per line

  NSString *..;
  NSString *...;

same below

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

> notifybymacosnotificationcenter.mm:169
> +    osxNotification.informativeText = [NSString stringWithString: (NSString *)cfText];
> +    osxNotification.contentImage = QtMac::toNSImage(notification->pixmap());
> +

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

> notifybymacosnotificationcenter.mm:221
> +{
> +    close(notification);
> +    notify(notification, config);

Is there no way to transparently update a notification?

REPOSITORY
  R289 KNotifications

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

To: Inoki, rjvbb
Cc: broulik, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190718/a3b52639/attachment-0001.html>