broulik added inline comments.
INLINE COMMENTS
> notifybypopup.cpp:115
>
> - bool connected = QDBusConnection::sessionBus().connect(QString(), //
> from any service
> -
> QString::fromLatin1(dbusPath),
Previously it would accept the signal from any service which I find odd,
though. Could you maybe check git logs to see if there was a reason for this?
It should survive restarts anyway and the service name is defined, so I really
don't see why it used to be a blind connect.
> notifybypopup.cpp:364
> + QDBusPendingReply<uint> reply = *watcher;
> + notifications.insert(reply.argumentAt<0>(), notification);
> + });
You have `watcher` (which is parented to `notification`) and `notification` as
contexts, but in the lambda you also access `this`. This looks dangerous. How
about using `this` instead of `notification` as context object?
> notifybypopup.cpp:376
> +
> + QDBusPendingCall call = dbusInterface.GetCapabilities();
> +
This is `QDBusPendingReply<QStringList>` (or just `auto`...)
> notifybypopup.cpp:385
> + popupServerCapabilities = capabilities;
> + dbusServiceCapCacheDirty = false;
> +
Unrelated: I was wondering, do we actually monitor the notification service
changing and make them dirty again?
> notifybypopup.cpp:388
> + // re-run notify() on all enqueued notifications
> + for (int i = 0, total = notificationQueue.size(); i < total; ++i) {
> + q->notify(notificationQueue.at(i).first,
> notificationQueue.at(i).second);
range for?
REPOSITORY
R289 KNotifications
REVISION DETAIL
https://phabricator.kde.org/D29420
To: nicolasfella, #frameworks, broulik
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
