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 <k...@privat.broulik.de>
> + * Copyright 2020 Shah Bhushan <bs...@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
