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