D20265: Introduce libnotificationmanager

[David Edmundson]

davidedmundson added a comment.


  I have a bunch of minor comments, but nothing that's a big show-stopper. 
  Then I think we're probably best shipping it soon.

INLINE COMMENTS

> notificationmanagerplugin.cpp:36
> +    qmlRegisterType<Notifications>(uri, 1, 0, "Notifications");
> +    qmlRegisterType<Job>();
> +    qmlRegisterType<Settings>(uri, 1, 0, "Settings");

uncreatabletype?

> job_p.cpp:290
> +        updateField(value, m_descriptionValue2, &Job::descriptionValue2Changed);
> +    }
> +    updateHasDetails();

descriptionUrlChanged

> jobsmodel.cpp:162
> +    if (dirty) {
> +        emit dataChanged(index, index, {role});
> +    }

Why?

job::setDismissed has a signal that should propagate through

> jobsmodel.cpp:201
> +    if (checkIndex(idx)) {
> +        emit d->m_jobViews.at(idx.row())->resume();
> +    }

emit is wrong

> jobsmodel_p.cpp:80
> +                qCDebug(NOTIFICATIONMANAGER) << "By the time we wanted to show JobView" << job->id() << "from" << job->applicationName() << ", it was already stopped";
> +                remove(job);
> +                continue;

If it's pending then it won't be in the model, which means we won't call removeAt

which means we won't actually delete it.

Possible fix

  void JobsModelPrivate::remove(Job *job)
  {
      const int row = m_jobViews.indexOf(job);
      if (row > -1) {
          removeAt(row);
      } else {
       delete job;
  }
  }

> jobsmodel_p.cpp:138
> +
> +    m_serviceWatcher = new QDBusServiceWatcher(this);
> +    m_serviceWatcher->setConnection(sessionBus);

can you move this to the ctor for the same reason as the other code

> jobsmodel_p.cpp:422
> +        }
> +        job->setError(127); // KIO::ERR_SLAVE_DIED
> +        job->setErrorText(i18n("Application closed unexpectedly."));

you  can use the enum, AFAIK it was hardcoded so we could backport this patch into 5.8 where anything non-zero was enough for what we wanted.

> limitedrowcountproxymodel.cpp:25
> +
> +using namespace NotificationManager;
> +

This is a good candidate for KItemModels

> limitedrowcountproxymodel.cpp:44
> +    if (sourceModel) {
> +        connect(sourceModel, &QAbstractItemModel::rowsInserted, this, &LimitedRowCountProxyModel::invalidateFilter);
> +        connect(sourceModel, &QAbstractItemModel::rowsRemoved, this, &LimitedRowCountProxyModel::invalidateFilter);

I assume we need this as a row being inserted earlier in the list doesn't cause us to re-evaluate existing rows that previously passed the filter and are unchanged?

We should have this for rowsMoved

> notification.h:52
> +
> +    // should this be virtual for good measure?
> +    ~Notification();

yes

> server_p.cpp:70
> +
> +    m_inhibitionWatcher = new QDBusServiceWatcher(this);
> +    m_inhibitionWatcher->setConnection(QDBusConnection::sessionBus());

there's a very obscure crash here.

If we fail to register the service, we don't create m_inhibitionWatcher and usages later will crash.

It's possible we'd still get into this code as one could call this the notify method on our existing service name.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D20265

To: broulik, #plasma
Cc: hein, mart, nicolasfella, davidedmundson, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20190508/90d127b0/attachment-0001.html>