Final round of review comments

- Fix leak and crash when job goes away before being shown
- Some emit and signal fixes
- Always create services watchers
- Update unity job percentage in destructor, fixes the progress lingering around when disabling in settings
  Also tell the world that we're merely proxying a job progress
- Don't hardcode KIO error codes
- Honor "show critical on top of full screen" setting
- Don't show generic "dialog-information" icon, it's pointlessly generic

applets/notifications/package/contents/ui/NotificationItem.qml

@@ -219,7 +219,19 @@ ColumnLayout {
                 anchors.fill: parent
                 usesPlasmaTheme: false
                 smooth: true
-                source: typeof notificationItem.icon === "string" ? notificationItem.icon : ""
+                source: {
+                    var icon = notificationItem.icon;
+                    if (typeof icon !== "string") { // displayed by QImageItem below
+                        return "";
+                    }
+
+                    // don't show a generic "info" icon since this is a notification already
+                    if (icon === "dialog-information") {
+                        return "";
+                    }
+
+                    return icon;
+                }
                 visible: active
             }
 

applets/notifications/package/contents/ui/NotificationPopup.qml

@@ -94,7 +94,6 @@ PlasmaCore.Dialog {
 
     location: PlasmaCore.Types.Floating
 
-    type: urgency === NotificationManager.Notifications.CriticalUrgency ? PlasmaCore.Dialog.CriticalNotification : PlasmaCore.Dialog.Notification
     flags: Qt.WindowDoesNotAcceptFocus
 
     visible: false

applets/notifications/package/contents/ui/global/Globals.qml

@@ -321,6 +321,8 @@ QtObject {
             readonly property var notificationId: model.notificationId
 
             popupWidth: globals.popupWidth
+            type: model.urgency === NotificationManager.Notifications.CriticalUrgency && notificationSettings.keepCriticalAlwaysOnTop
+                  ? PlasmaCore.Dialog.CriticalNotification : PlasmaCore.Dialog.Notification
 
             notificationType: model.type
 

libnotificationmanager/declarative/notificationmanagerplugin.cpp

@@ -33,6 +33,6 @@ void NotificationManagerPlugin::registerTypes(const char *uri)
     Q_ASSERT(uri == QLatin1String("org.kde.notificationmanager"));
 
     qmlRegisterType<Notifications>(uri, 1, 0, "Notifications");
-    qmlRegisterType<Job>();
+    qmlRegisterUncreatableType<Job>(uri, 1, 0, "Job", QStringLiteral("Can only access Job via JobDetailsRole of JobsModel"));
     qmlRegisterType<Settings>(uri, 1, 0, "Settings");
 }

libnotificationmanager/job_p.cpp

@@ -30,6 +30,8 @@
 #include <KService>
 #include <KLocalizedString>
 
+#include <kio/global.h>
+
 #include "notifications.h"
 
 #include "jobviewv2adaptor.h"
@@ -204,7 +206,7 @@ void JobPrivate::finish()
     QDBusConnection::sessionBus().unregisterObject(m_objectPath.path());
 
     // When user canceled transfer, remove it without notice
-    if (m_error == 1) { // KIO::ERR_USER_CANCELED
+    if (m_error == KIO::ERR_USER_CANCELED) {
         emit closed();
         return;
     }
@@ -281,14 +283,18 @@ void JobPrivate::setInfoMessage(const QString &infoMessage)
 
 bool JobPrivate::setDescriptionField(uint number, const QString &name, const QString &value)
 {
+    bool dirty = false;
     if (number == 0) {
-        updateField(name, m_descriptionLabel1, &Job::descriptionLabel1Changed);
-        updateField(value, m_descriptionValue1, &Job::descriptionValue1Changed);
+        dirty |= updateField(name, m_descriptionLabel1, &Job::descriptionLabel1Changed);
+        dirty |= updateField(value, m_descriptionValue1, &Job::descriptionValue1Changed);
     } else if (number == 1) {
-        updateField(name, m_descriptionLabel2, &Job::descriptionLabel2Changed);
-        updateField(value, m_descriptionValue2, &Job::descriptionValue2Changed);
+        dirty |= updateField(name, m_descriptionLabel2, &Job::descriptionLabel2Changed);
+        dirty |= updateField(value, m_descriptionValue2, &Job::descriptionValue2Changed);
+    }
+    if (dirty) {
+        emit static_cast<Job*>(parent())->descriptionUrlChanged();
+        updateHasDetails();
     }
-    updateHasDetails();
 
     return false;
 }

libnotificationmanager/jobsmodel.cpp

@@ -147,22 +147,16 @@ bool JobsModel::setData(const QModelIndex &index, const QVariant &value, int rol
 
     Job *job = d->m_jobViews.at(index.row());
 
-    bool dirty = false;
-
     switch (role) {
     case Notifications::DismissedRole:
         if (value.toBool() != job->dismissed()) {
             job->setDismissed(value.toBool());
-            dirty = true;
+            return true;
         }
         break;
     }
 
-    if (dirty) {
-        emit dataChanged(index, index, {role});
-    }
-
-    return dirty;
+    return false;
 }
 
 int JobsModel::rowCount(const QModelIndex &parent) const
@@ -191,21 +185,21 @@ void JobsModel::expire(const QModelIndex &idx)
 void JobsModel::suspend(const QModelIndex &idx)
 {
     if (checkIndex(idx)) {
-        emit d->m_jobViews.at(idx.row())->suspend();
+        d->m_jobViews.at(idx.row())->suspend();
     }
 }
 
 void JobsModel::resume(const QModelIndex &idx)
 {
     if (checkIndex(idx)) {
-        emit d->m_jobViews.at(idx.row())->resume();
+        d->m_jobViews.at(idx.row())->resume();
     }
 }
 
 void JobsModel::kill(const QModelIndex &idx)
 {
     if (checkIndex(idx)) {
-        emit d->m_jobViews.at(idx.row())->kill();
+        d->m_jobViews.at(idx.row())->kill();
     }
 }
 

libnotificationmanager/jobsmodel_p.cpp

@@ -38,15 +38,22 @@
 #include <KLocalizedString>
 #include <KService>
 
+#include <kio/global.h>
+
 #include <algorithm>
 
 using namespace NotificationManager;
 
 JobsModelPrivate::JobsModelPrivate(QObject *parent)
     : QObject(parent)
+    , m_serviceWatcher(new QDBusServiceWatcher(this))
     , m_compressUpdatesTimer(new QTimer(this))
     , m_pendingJobViewsTimer(new QTimer(this))
 {
+    m_serviceWatcher->setConnection(QDBusConnection::sessionBus());
+    m_serviceWatcher->setWatchMode(QDBusServiceWatcher::WatchForUnregistration);
+    connect(m_serviceWatcher, &QDBusServiceWatcher::serviceUnregistered, this, &JobsModelPrivate::onServiceUnregistered);
+
     m_compressUpdatesTimer->setInterval(0);
     m_compressUpdatesTimer->setSingleShot(true);
     connect(m_compressUpdatesTimer, &QTimer::timeout, this, [this] {
@@ -94,9 +101,18 @@ JobsModelPrivate::JobsModelPrivate(QObject *parent)
 
 JobsModelPrivate::~JobsModelPrivate()
 {
-    QDBusConnection::sessionBus().unregisterService(QStringLiteral("org.kde.JobViewServer"));
-    QDBusConnection::sessionBus().unregisterService(QStringLiteral("org.kde.kuiserver"));
-    QDBusConnection::sessionBus().unregisterObject(QStringLiteral("/JobViewServer"));
+    QDBusConnection sessionBus = QDBusConnection::sessionBus();
+    sessionBus.unregisterService(QStringLiteral("org.kde.JobViewServer"));
+    sessionBus.unregisterService(QStringLiteral("org.kde.kuiserver"));
+    sessionBus.unregisterObject(QStringLiteral("/JobViewServer"));
+
+    // Remember which services we had running and clear their progress
+    QStringList desktopEntries;
+    for (Job *job : qAsConst(m_jobViews)) {
+        if (!desktopEntries.contains(job->desktopEntry())) {
+            desktopEntries.append(job->desktopEntry());
+        }
+    }
 
     qDeleteAll(m_jobViews);
     m_jobViews.clear();
@@ -104,6 +120,10 @@ JobsModelPrivate::~JobsModelPrivate()
     m_pendingJobViews.clear();
 
     m_pendingDirtyRoles.clear();
+
+    for (const QString &desktopEntry : desktopEntries) {
+        updateApplicationPercentage(desktopEntry);
+    }
 }
 
 bool JobsModelPrivate::init()
@@ -135,11 +155,6 @@ bool JobsModelPrivate::init()
         return false;
     }
 
-    m_serviceWatcher = new QDBusServiceWatcher(this);
-    m_serviceWatcher->setConnection(sessionBus);
-    m_serviceWatcher->setWatchMode(QDBusServiceWatcher::WatchForUnregistration);
-    connect(m_serviceWatcher, &QDBusServiceWatcher::serviceUnregistered, this, &JobsModelPrivate::onServiceUnregistered);
-
     m_valid = true;
     return true;
 }
@@ -333,25 +348,24 @@ QDBusObjectPath JobsModelPrivate::requestView(const QString &desktopEntry,
 
 void JobsModelPrivate::remove(Job *job)
 {
-    const int row = m_jobViews.indexOf(job);
-    if (row > -1) {
-        removeAt(row);
-    }
-}
+    const int activeRow = m_jobViews.indexOf(job);
+    const int pendingRow = m_pendingJobViews.indexOf(job);
 
-void JobsModelPrivate::removeAt(int row)
-{
-    Q_ASSERT(row >= 0 && row < m_jobViews.count());
+    Job *jobToBeRemoved = nullptr;
 
-    emit jobViewAboutToBeRemoved(row);//, job);
-    Job *job = m_jobViews.takeAt(row);
-    m_pendingDirtyRoles.remove(job);
-    m_pendingJobViews.removeOne(job);
+    if (activeRow > -1) {
+        emit jobViewAboutToBeRemoved(activeRow);
+        jobToBeRemoved = m_jobViews.takeAt(activeRow);
+    } else if (pendingRow > -1) {
+        jobToBeRemoved = m_pendingJobViews.takeAt(pendingRow);
+    }
+    Q_ASSERT(jobToBeRemoved);
 
-    const QString desktopEntry = job->desktopEntry();
+    m_pendingDirtyRoles.remove(jobToBeRemoved);
 
-    const QString serviceName = m_jobServices.take(job);
+    const QString desktopEntry = jobToBeRemoved->desktopEntry();
 
+    const QString serviceName = m_jobServices.take(jobToBeRemoved);
     // Check if there's any jobs left for this service, otherwise stop watching it
     auto it = std::find_if(m_jobServices.constBegin(), m_jobServices.constEnd(), [&serviceName](const QString &item) {
         return item == serviceName;
@@ -360,12 +374,20 @@ void JobsModelPrivate::removeAt(int row)
         m_serviceWatcher->removeWatchedService(serviceName);
     }
 
-    delete job;
-    emit jobViewRemoved(row);
+    delete jobToBeRemoved;
+    if (activeRow > -1) {
+        emit jobViewRemoved(activeRow);
+    }
 
     updateApplicationPercentage(desktopEntry);
 }
 
+void JobsModelPrivate::removeAt(int row)
+{
+    Q_ASSERT(row >= 0 && row < m_jobViews.count());
+    remove(m_jobViews.at(row));
+}
+
 // This will forward overall application process via Unity API.
 // This way users of that like Task Manager and Latte Dock still get basic job information.
 void JobsModelPrivate::updateApplicationPercentage(const QString &desktopEntry)
@@ -397,7 +419,9 @@ void JobsModelPrivate::updateApplicationPercentage(const QString &desktopEntry)
         {QStringLiteral("count-visible"), jobsCount > 0},
         {QStringLiteral("count"), jobsCount},
         {QStringLiteral("progress-visible"), jobsCount > 0},
-        {QStringLiteral("progress"), percentage / 100.0}
+        {QStringLiteral("progress"), percentage / 100.0},
+        // so Task Manager knows this is a job progress and can ignore it if disabled in settings
+        {QStringLiteral("proxied-for"), QStringLiteral("kuiserver")}
     };
 
     QDBusMessage message = QDBusMessage::createSignal(QStringLiteral("/org/kde/notificationmanager/jobs"),
@@ -419,7 +443,7 @@ void JobsModelPrivate::onServiceUnregistered(const QString &serviceName)
         if (job->state() == Notifications::JobStateStopped) {
             continue;
         }
-        job->setError(127); // KIO::ERR_SLAVE_DIED
+        job->setError(KIO::ERR_OWNER_DIED);
         job->setErrorText(i18n("Application closed unexpectedly."));
         job->setState(Notifications::JobStateStopped);
     }

libnotificationmanager/limitedrowcountproxymodel.cpp

@@ -42,6 +42,7 @@ void LimitedRowCountProxyModel::setSourceModel(QAbstractItemModel *sourceModel)
 
     if (sourceModel) {
         connect(sourceModel, &QAbstractItemModel::rowsInserted, this, &LimitedRowCountProxyModel::invalidateFilter);
+        connect(sourceModel, &QAbstractItemModel::rowsMoved, this, &LimitedRowCountProxyModel::invalidateFilter);
         connect(sourceModel, &QAbstractItemModel::rowsRemoved, this, &LimitedRowCountProxyModel::invalidateFilter);
     }
 }

libnotificationmanager/notification.h

@@ -49,8 +49,7 @@ public:
     Notification &operator=(const Notification &other);
     Notification &operator=(Notification &&other) Q_DECL_NOEXCEPT;
 
-    // should this be virtual for good measure?
-    ~Notification();
+    virtual ~Notification();
 
     uint id() const;
 

libnotificationmanager/server_p.cpp

@@ -43,8 +43,11 @@ using namespace NotificationManager;
 
 ServerPrivate::ServerPrivate(QObject *parent)
     : QObject(parent)
+    , m_inhibitionWatcher(new QDBusServiceWatcher(this))
 {
-
+    m_inhibitionWatcher->setConnection(QDBusConnection::sessionBus());
+    m_inhibitionWatcher->setWatchMode(QDBusServiceWatcher::WatchForUnregistration);
+    connect(m_inhibitionWatcher, &QDBusServiceWatcher::serviceUnregistered, this, &ServerPrivate::onServiceUnregistered);
 }
 
 ServerPrivate::~ServerPrivate() = default;
@@ -67,11 +70,6 @@ bool ServerPrivate::init()
         return false;
     }
 
-    m_inhibitionWatcher = new QDBusServiceWatcher(this);
-    m_inhibitionWatcher->setConnection(QDBusConnection::sessionBus());
-    m_inhibitionWatcher->setWatchMode(QDBusServiceWatcher::WatchForUnregistration);
-    connect(m_inhibitionWatcher, &QDBusServiceWatcher::serviceUnregistered, this, &ServerPrivate::onServiceUnregistered);
-
     connect(this, &ServerPrivate::inhibitedChanged, this, [this] {
         // emit DBus change signal...
         QDBusMessage signal = QDBusMessage::createSignal(