D28509: RFC: [libnotificationmanager] introduce the notification watcher

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> org.freedesktop.Notifications.xml:55
>  
> +    <method name="RegisterWatcher"/>
> +    <method name="UnRegisterWatcher"/>

Please don't put that on the fdo service

> notificationwatcher.cpp:45
> +    );
> +    QDBusConnection::sessionBus().call(msg, QDBus::NoBlock);
> +}

Do we not want to check if that succeeded? I think having a `valid` property on the watcher could be nice

> notificationwatcher.h:41
> +public Q_SLOTS:
> +    Q_SCRIPTABLE void Notify(uint id, const QString &app_name, uint replaces_id, const QString &app_icon,
> +                        const QString &summary, const QString &body, const QStringList &actions,

Don't leak this implementation detail into the public API. Instead, put all of the dbus handling into a private class, like is done with `Server` vs `ServerPrivate`

> notificationwatcher.h:47
> +private:
> +    QDBusServiceWatcher *m_serviceWatcher;
> +};

Use d-pointer for exported class

> server_p.cpp:247
> +        });
> +        QDBusConnection::sessionBus().call(msg);
> +    }

Don't block

> server_p.cpp:261
> +            QStringLiteral("/NotificationWatcher"),
> +            QStringLiteral(""),
> +            QStringLiteral("CloseNotification")

Don't we want to adhere to an interface?

> server_p.cpp:527
> +    m_notificationWatchers << message().service();
> +    //TODO: add a dbus service watcher which automatically unregisters watcher
> +    //      when service disappears

Then you can probably also use the watcher's "watched services" list rather than storing it in a member

REPOSITORY
  R120 Plasma Workspace

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

To: bshah, #plasma, broulik, davidedmundson
Cc: nicolasfella, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200402/280e6771/attachment-0001.html>