mck182 added a comment.
Thanks for the patch! I wanted to do exactly this a long
time ago. However this solution brings a burden to all
apps using KNotification in form of a blocking dbus call
which is further only relevant when used in Plasma.
That's a no-no I'm afraid, we'd have to find a better solution.
I was thinking that maybe KSplash should have the notifications
dbus interface instead and handle it somehow on its own,
either just collecting the notifications and then reemitting them
once it's done splashing, or just displaying them directly, I dunno.
INLINE COMMENTS
> knotificationmanager.cpp:140
>
> + d->blockNotifications =
> sessionBus.interface()->isServiceRegistered("org.kde.KSplash");
> + if (d->blockNotifications) {
The problem with this is that `isServiceRegistered` is
a blocking call. Blocking dbus calls are generally a nogo
because it can happen that it will get stuck in dbus
timeout, which is 25 secs by default. And that's bad.
Especially because this would be blocking the hosting
app.
Generally any dbus blocking calls are bad.
> knotificationmanager.cpp:143
> + qCDebug(LOG_KNOTIFICATIONS) << "Splash is in progress, delaying
> notifications";
> + QDBusServiceWatcher* watcher = new QDBusServiceWatcher(this);
> + watcher->setConnection(sessionBus);
The coding style of frameworks is ` *watcher`
> knotificationmanager.cpp:147
> + watcher->setWatchMode(QDBusServiceWatcher::WatchForUnregistration);
> + connect(watcher, SIGNAL(serviceUnregistered(const QString&)), this,
> SLOT(renotify(const QString&)));
> + }
We use the new signals/slots style in frameworks
> knotificationmanager.cpp:268
>
> +void KNotificationManager::renotify(const QString& serv)
> +{
`renotify` is not entirely ideal name; something like
`processQueue` would be much more fitting.
Also, coding style is ` &serv`
> knotificationmanager.cpp:275
> +
> + Q_FOREACH (KNotification* n, d->notifications.values()) {
> + notify(n);
Coding style is ` *n`
> knotificationmanager_p.h:71
> void reparseConfiguration(const QString &app);
> + void renotify(const QString&);
>
Coding style is `&` with the var name, also always
put the variable name in the header and use Q_UNUSED
in the implementation.
REPOSITORY
R289 KNotifications
REVISION DETAIL
https://phabricator.kde.org/D4799
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: vpilo
Cc: mck182, #frameworks
