[Differential] [Commented On] D4799: Delay notifications until desktop session has loaded

[Martin Klapetek]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170226/8a3304b4/attachment.html>