D28509: RFC: [libnotificationmanager] introduce the notification watcher

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> CMakeLists.txt:25
> +
> +    #notificationwatcher.cpp
> +    #watcher_p.cpp

Remove

> abstractnotificationsmodel.h:41
>  public:
> -    ~NotificationsModel() override;
> -
> -    using Ptr = QSharedPointer<NotificationsModel>;
> -    static Ptr createNotificationsModel();
> +    AbstractNotificationsModel();
> +    ~AbstractNotificationsModel() override;

`protected` constructor?

> notification.h:128
> +    // Little bit of mess here, we want to sometime keep track of processed hints, and not process it.
> +    QVariantMap& hints() const;
> +    void setHints(const QVariantMap &hints);

Maybe add a new `rawHints() const`?

Not a fan of ref return

> notification_p.h:100
>      QList<QUrl> urls;
> +    QVariantMap hints = QVariantMap();
>  

No need to explicitly initialize

> notificationsmodel.h:3
>   * Copyright 2018-2019 Kai Uwe Broulik <kde at privat.broulik.de>
> + * Copyright 2020 Shah Bhushan <bshah at kde.org>
>   *

wrong order?

> server_p.cpp:230
> +    // TODO: come up with proper authentication/user selection
> +    for (QString bus: m_notificationWatchers) {
> +        qDebug() << "Dispatching notification to " << bus;

`const QString &service : qAsConst(m_notificationWatchers)`

> server_p.cpp:231
> +    for (QString bus: m_notificationWatchers) {
> +        qDebug() << "Dispatching notification to " << bus;
> +        QDBusMessage msg = QDBusMessage::createMethodCall(

Remove, or use categorized logging

> server_p.cpp:232
> +        qDebug() << "Dispatching notification to " << bus;
> +        QDBusMessage msg = QDBusMessage::createMethodCall(
> +            bus,

Can you use generated XML interface for this, maybe?

> server_p.cpp:258
>      // spec says "If the notification no longer exists, an empty D-BUS Error message is sent back."
> +    for (QString bus: m_notificationWatchers) {
> +        qDebug() << "Dispatching notification to " << bus;

Same as above

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

Yes, please do.

> server_p.h:25
>  #include <QDBusContext>
> +#include <QList>
>  

Include `QStringList`

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

I think we should have a `valid` property or any means for the user to check whether this succeeds / the model is working

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, 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/20200421/8486dcfe/attachment-0001.html>