Address review comments and other clean up

* Use index(n, m, parent) instead of parent.child(n, m)
* Update unity count when job is closed without stopping it first
* Iterate whole dataChanged range [topLeft, bottomRight] when group is resolved
* Generate roleNames from meta object
* Use checkIndex()
* Increase default timeout for notifications with thumbnail
* Improved keyboard nav, support Home/End, let Return open thumbnail

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

@@ -274,14 +274,37 @@ ColumnLayout{
             }
             Keys.onEnterPressed: Keys.onReturnPressed(event)
             Keys.onReturnPressed: {
+                // Trigger default action, if any
                 var idx = historyModel.index(currentIndex, 0);
                 if (historyModel.data(idx, NotificationManager.Notifications.HasDefaultActionRole)) {
                     historyModel.invokeDefaultAction(idx);
+                    return;
                 }
+
+                // Trigger thumbnail URL if there's one
+                var urls = historyModel.data(idx, NotificationManager.Notifications.UrlsRole);
+                if (urls && urls.length === 1) {
+                    Qt.openUrlExternally(urls[0]);
+                    historyModel.expire(idx);
+                    return;
+                }
+
+                // TODO for finished jobs trigger "Open" or "Open Containing Folder" action
             }
             Keys.onLeftPressed: setGroupExpanded(currentIndex, LayoutMirroring.enabled)
             Keys.onRightPressed: setGroupExpanded(currentIndex, !LayoutMirroring.enabled)
 
+            Keys.onPressed: {
+                switch (event.key) {
+                case Qt.Key_Home:
+                    currentIndex = 0;
+                    break;
+                case Qt.Key_End:
+                    currentIndex = count - 1;
+                    break;
+                }
+            }
+
             function isRowExpanded(row) {
                 var idx = historyModel.index(row, 0);
                 return historyModel.data(idx, NotificationManager.Notifications.IsGroupExpandedRole);
@@ -381,7 +404,12 @@ ColumnLayout{
                         closable: model.closable
                         closeButtonTooltip: i18n("Close Group")
 
-                        onCloseClicked: historyModel.close(historyModel.index(index, 0))
+                        onCloseClicked: {
+                            historyModel.close(historyModel.index(index, 0))
+                            if (list.count === 0) {
+                                plasmoid.expanded = false;
+                            }
+                        }
 
                         onConfigureClicked: historyModel.configure(historyModel.index(index, 0))
                     }
@@ -469,7 +497,12 @@ ColumnLayout{
                                     return labels;
                                 }
 
-                                onCloseClicked: historyModel.close(historyModel.index(index, 0))
+                                onCloseClicked: {
+                                    historyModel.close(historyModel.index(index, 0));
+                                    if (list.count === 0) {
+                                        plasmoid.expanded = false;
+                                    }
+                                }
                                 onDismissClicked: model.dismissed = false
                                 onConfigureClicked: historyModel.configure(historyModel.index(index, 0))
 

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

@@ -344,7 +344,9 @@ QtObject {
             icon: model.image || model.iconName
             hasDefaultAction: model.hasDefaultAction || false
             timeout: model.timeout
-            defaultTimeout: notificationSettings.popupTimeout
+            // Increase default timeout for notifications with a URL so you have enough time
+            // to interact with the thumbnail or bring the window to the front where you want to drag it into
+            defaultTimeout: notificationSettings.popupTimeout + (model.urls && model.urls.length > 0 ? 5000 : 0)
             // When configured to not keep jobs open permanently, we autodismiss them after the standard timeout
             dismissTimeout: !notificationSettings.permanentJobPopups
                             && model.type === NotificationManager.Notifications.JobType

libnotificationmanager/CMakeLists.txt

@@ -84,6 +84,7 @@ install(TARGETS notificationmanager EXPORT notificationmanagerLibraryTargets ${K
 
 install(FILES
     server.h
+    notifications.h
     notification.h
     jobsmodel.h
     job.h

libnotificationmanager/job.cpp

@@ -27,8 +27,6 @@
 
 #include "notifications.h"
 
-#include <QQmlEngine>
-
 using namespace NotificationManager;
 
 Job::Job(uint id, QObject *parent)
@@ -280,19 +278,3 @@ void Job::kill()
 {
     emit d->cancelRequested();
 }
-
-template<typename T> void processField(const QVariantMap/*Plasma::DataEngine::Data*/ &data,
-                                            const QString &field,
-                                            T &target,
-                                            int role,
-                                            QVector<int> &dirtyRoles)
-{
-    auto it = data.find(field);
-    if (it != data.end()) {
-        const T newValue = it->value<T>();
-        if (target != newValue) {
-            target = newValue;
-            dirtyRoles.append(role);
-        }
-    }
-}

libnotificationmanager/jobsmodel.cpp

@@ -90,7 +90,7 @@ bool JobsModel::isValid() const
 
 QVariant JobsModel::data(const QModelIndex &index, int role) const
 {
-    if (!index.isValid() || index.row() >= d->m_jobViews.count()) {
+    if (!checkIndex(index)) {
         return QVariant();
     }
 
@@ -141,7 +141,7 @@ QVariant JobsModel::data(const QModelIndex &index, int role) const
 
 bool JobsModel::setData(const QModelIndex &index, const QVariant &value, int role)
 {
-    if (!index.isValid() || index.row() >= d->m_jobViews.count()) {
+    if (!checkIndex(index)) {
         return false;
     }
 
@@ -176,47 +176,37 @@ int JobsModel::rowCount(const QModelIndex &parent) const
 
 void JobsModel::close(const QModelIndex &idx)
 {
-    if (!idx.isValid() || idx.row() >= d->m_jobViews.count()) {
-        return;
+    if (checkIndex(idx)) {
+        d->removeAt(idx.row());
     }
-
-    d->removeAt(idx.row());
 }
 
 void JobsModel::expire(const QModelIndex &idx)
 {
-    if (!idx.isValid() || idx.row() >= d->m_jobViews.count()) {
-        return;
+    if (checkIndex(idx)) {
+        d->m_jobViews.at(idx.row())->setExpired(true);
     }
-
-    d->m_jobViews.at(idx.row())->setExpired(true);
 }
 
 void JobsModel::suspend(const QModelIndex &idx)
 {
-    if (!idx.isValid() || idx.row() >= d->m_jobViews.count()) {
-        return;
+    if (checkIndex(idx)) {
+        emit d->m_jobViews.at(idx.row())->suspend();
     }
-
-    emit d->m_jobViews.at(idx.row())->suspend();
 }
 
 void JobsModel::resume(const QModelIndex &idx)
 {
-    if (!idx.isValid() || idx.row() >= d->m_jobViews.count()) {
-        return;
+    if (checkIndex(idx)) {
+        emit d->m_jobViews.at(idx.row())->resume();
     }
-
-    emit d->m_jobViews.at(idx.row())->resume();
 }
 
 void JobsModel::kill(const QModelIndex &idx)
 {
-    if (!idx.isValid() || idx.row() >= d->m_jobViews.count()) {
-        return;
+    if (checkIndex(idx)) {
+        emit d->m_jobViews.at(idx.row())->kill();
     }
-
-    emit d->m_jobViews.at(idx.row())->kill();
 }
 
 void JobsModel::clear(Notifications::ClearFlags flags)

libnotificationmanager/jobsmodel_p.cpp

@@ -348,6 +348,8 @@ void JobsModelPrivate::removeAt(int row)
     m_pendingDirtyRoles.remove(job);
     m_pendingJobViews.removeOne(job);
 
+    const QString desktopEntry = job->desktopEntry();
+
     const QString serviceName = m_jobServices.take(job);
 
     // Check if there's any jobs left for this service, otherwise stop watching it
@@ -360,6 +362,8 @@ void JobsModelPrivate::removeAt(int row)
 
     delete job;
     emit jobViewRemoved(row);
+
+    updateApplicationPercentage(desktopEntry);
 }
 
 // This will forward overall application process via Unity API.
@@ -375,10 +379,13 @@ void JobsModelPrivate::updateApplicationPercentage(const QString &desktopEntry)
 
     for (int i = 0; i < m_jobViews.count(); ++i) {
         Job *job = m_jobViews.at(i);
-        if (job->state() != Notifications::JobStateStopped) {
-            jobsPercentages += job->percentage();
-            ++jobsCount;
+        if (job->state() == Notifications::JobStateStopped
+                || job->desktopEntry() != desktopEntry) {
+            continue;
         }
+
+        jobsPercentages += job->percentage();
+        ++jobsCount;
     }
 
     int percentage = 0;

libnotificationmanager/notificationgroupcollapsingproxymodel.cpp

@@ -47,14 +47,15 @@ void NotificationGroupCollapsingProxyModel::setSourceModel(QAbstractItemModel *s
         connect(source, &QAbstractItemModel::rowsRemoved, this, &NotificationGroupCollapsingProxyModel::invalidateFilter);
 
         // When a group is removed, there is no item that's being removed, instead the item morphs back into a single notification
-        connect(source, &QAbstractItemModel::dataChanged, this, [this](const QModelIndex &topLeft, const QModelIndex &bottomRight, const QVector<int> &roles) {
-            Q_UNUSED(bottomRight); // what about it?
-            Q_UNUSED(roles);
-
+        connect(source, &QAbstractItemModel::dataChanged, this, [this, source](const QModelIndex &topLeft, const QModelIndex &bottomRight, const QVector<int> &roles) {
             if (roles.isEmpty() || roles.contains(Notifications::IsGroupRole)) {
-                if (!topLeft.data(Notifications::IsGroupRole).toBool()) {
-                    if (m_expandedGroups.contains(topLeft)) {
-                        setGroupExpanded(topLeft, false);
+                for (int i = topLeft.row(); i <= bottomRight.row(); ++i) {
+                    const QModelIndex sourceIdx = source->index(i, 0);
+
+                    if (!sourceIdx.data(Notifications::IsGroupRole).toBool()) {
+                        if (m_expandedGroups.contains(sourceIdx)) {
+                            setGroupExpanded(topLeft, false);
+                        }
                     }
                 }
             }
@@ -172,7 +173,7 @@ bool NotificationGroupCollapsingProxyModel::setGroupExpanded(const QModelIndex &
     const QVector<int> dirtyRoles = {Notifications::ExpandedGroupChildrenCountRole, Notifications::IsGroupExpandedRole};
 
     emit dataChanged(idx, idx, dirtyRoles);
-    emit dataChanged(idx.child(0, 0), idx.child(rowCount(idx) - 1, 0), dirtyRoles);
+    emit dataChanged(index(0, 0, idx), index(rowCount(idx) - 1, 0, idx), dirtyRoles);
 
     return true;
 }
@@ -185,13 +186,13 @@ void NotificationGroupCollapsingProxyModel::invalidateGroupRoles()
 
     for (int row = 0; row < rowCount(); ++row) {
         const QModelIndex groupIdx = index(row, 0);
-        emit dataChanged(groupIdx.child(0, 0), groupIdx.child(rowCount(groupIdx) - 1, 0), dirtyRoles);
+        emit dataChanged(index(0, 0, groupIdx), index(rowCount(groupIdx) - 1, 0, groupIdx), dirtyRoles);
     }
 }
 
 bool NotificationGroupCollapsingProxyModel::filterAcceptsRow(int source_row, const QModelIndex &source_parent) const
 {
-    if (source_parent.isValid() && m_limit > 0) {
+    if (m_limit > 0 && source_parent.isValid()) {
         if (!m_expandedGroups.isEmpty() && m_expandedGroups.contains(source_parent)) {
             return true;
         }

libnotificationmanager/notificationgroupingproxymodel.cpp

@@ -43,7 +43,8 @@ bool NotificationGroupingProxyModel::appsMatch(const QModelIndex &a, const QMode
     const QString aDesktopEntry = a.data(Notifications::DesktopEntryRole).toString();
     const QString bDesktopEntry = b.data(Notifications::DesktopEntryRole).toString();
 
-    return aName == bName && aDesktopEntry == bDesktopEntry;
+    return !aName.isEmpty() && aName == bName
+            && !aDesktopEntry.isEmpty() && aDesktopEntry == bDesktopEntry;
 }
 
 bool NotificationGroupingProxyModel::isGroup(int row) const
@@ -471,7 +472,7 @@ QVariant NotificationGroupingProxyModel::data(const QModelIndex &proxyIndex, int
 
         case Notifications::DesktopEntryRole:
             for (int i = 0; i < rowCount(proxyIndex); ++i) {
-                const QString desktopEntry = proxyIndex.child(i, 0).data(Notifications::DesktopEntryRole).toString();
+                const QString desktopEntry = index(i, 0, proxyIndex).data(Notifications::DesktopEntryRole).toString();
                 if (!desktopEntry.isEmpty()) {
                     return desktopEntry;
                 }
@@ -479,7 +480,7 @@ QVariant NotificationGroupingProxyModel::data(const QModelIndex &proxyIndex, int
             return QString();
         case Notifications::NotifyRcNameRole:
             for (int i = 0; i < rowCount(proxyIndex); ++i) {
-                const QString notifyRcName = proxyIndex.child(i, 0).data(Notifications::NotifyRcNameRole).toString();
+                const QString notifyRcName = index(i, 0, proxyIndex).data(Notifications::NotifyRcNameRole).toString();
                 if (!notifyRcName.isEmpty()) {
                     return notifyRcName;
                 }
@@ -489,7 +490,7 @@ QVariant NotificationGroupingProxyModel::data(const QModelIndex &proxyIndex, int
 
         case Notifications::ConfigurableRole: // if there is any configurable child item
             for (int i = 0; i < rowCount(proxyIndex); ++i) {
-                if (proxyIndex.child(i, 0).data(Notifications::ConfigurableRole).toBool()) {
+                if (index(i, 0, proxyIndex).data(Notifications::ConfigurableRole).toBool()) {
                     return true;
                 }
             }
@@ -497,7 +498,7 @@ QVariant NotificationGroupingProxyModel::data(const QModelIndex &proxyIndex, int
 
         case Notifications::ClosableRole: // if there is any closable child item
             for (int i = 0; i < rowCount(proxyIndex); ++i) {
-                if (proxyIndex.child(i, 0).data(Notifications::ClosableRole).toBool()) {
+                if (index(i, 0, proxyIndex).data(Notifications::ClosableRole).toBool()) {
                     return true;
                 }
             }

libnotificationmanager/notifications.cpp

@@ -21,6 +21,7 @@
 #include "notifications.h"
 
 #include <QDebug>
+#include <QMetaEnum>
 #include <QSharedPointer>
 
 #include <KConcatenateRowsProxyModel>
@@ -321,7 +322,6 @@ bool Notifications::Private::isGroup(const QModelIndex &idx)
     return idx.data(Notifications::IsGroupRole).toBool();
 }
 
-// FIXME remove once we just map to the source model
 uint Notifications::Private::notificationId(const QModelIndex &idx)
 {
     return idx.data(Notifications::IdRole).toUInt();
@@ -825,49 +825,26 @@ int Notifications::rowCount(const QModelIndex &parent) const
 
 QHash<int, QByteArray> Notifications::roleNames() const
 {
-    return QHash<int, QByteArray> {
-        {IdRole, QByteArrayLiteral("notificationId")}, // id is QML-reserved
-        {IsGroupRole, QByteArrayLiteral("isGroup")},
-        {GroupChildrenCountRole, QByteArrayLiteral("groupChildrenCount")},
-        {ExpandedGroupChildrenCountRole, QByteArrayLiteral("expandedGroupChildrenCount")},
-        {IsGroupExpandedRole, QByteArrayLiteral("isGroupExpanded")},
-        {IsInGroupRole, QByteArrayLiteral("isInGroup")},
-        {TypeRole, QByteArrayLiteral("type")},
-        {CreatedRole, QByteArrayLiteral("created")},
-        {UpdatedRole, QByteArrayLiteral("updated")},
-        {SummaryRole, QByteArrayLiteral("summary")},
-        {BodyRole, QByteArrayLiteral("body")},
-        {IconNameRole, QByteArrayLiteral("iconName")},
-        {ImageRole, QByteArrayLiteral("image")},
-        {DesktopEntryRole, QByteArrayLiteral("desktopEntry")},
-        {NotifyRcNameRole, QByteArrayLiteral("notifyRcName")},
-
-        {ApplicationNameRole, QByteArrayLiteral("applicationName")},
-        {ApplicationIconNameRole, QByteArrayLiteral("applicationIconName")},
-        {DeviceNameRole, QByteArrayLiteral("deviceName")},
-
-        {ActionNamesRole, QByteArrayLiteral("actionNames")},
-        {ActionLabelsRole, QByteArrayLiteral("actionLabels")},
-        {HasDefaultActionRole, QByteArrayLiteral("hasDefaultAction")},
-        {DefaultActionLabelRole, QByteArrayLiteral("defaultActionLabel")},
-
-        {UrlsRole, QByteArrayLiteral("urls")},
-
-        {UrgencyRole, QByteArrayLiteral("urgency")},
-        {TimeoutRole, QByteArrayLiteral("timeout")},
-
-        {ClosableRole, QByteArrayLiteral("closable")},
-        {ConfigurableRole, QByteArrayLiteral("configurable")},
-        {ConfigureActionLabelRole, QByteArrayLiteral("configureActionLabel")},
-
-        {JobStateRole, QByteArrayLiteral("jobState")},
-        {PercentageRole, QByteArrayLiteral("percentage")},
-        {JobErrorRole, QByteArrayLiteral("jobError")},
-        {SuspendableRole, QByteArrayLiteral("suspendable")},
-        {KillableRole, QByteArrayLiteral("killable")},
-        {JobDetailsRole, QByteArrayLiteral("jobDetails")},
-
-        {ExpiredRole, QByteArrayLiteral("expired")},
-        {DismissedRole, QByteArrayLiteral("dismissed")}
-    };
+    static QHash<int, QByteArray> s_roles;
+
+    if (s_roles.isEmpty()) {
+        s_roles = QSortFilterProxyModel::roleNames();
+
+        // This generates role names from the Roles enum in the form of: FooRole -> foo
+        const QMetaEnum e = staticMetaObject.enumerator(staticMetaObject.indexOfEnumerator("Roles"));
+
+        for (int i = 0; i < e.keyCount(); ++i) {
+            const int value = e.value(i);
+
+            QByteArray key(e.key(i));
+            key[0] = key[0] + 32; // lower case first letter
+            key.chop(4); // strip "Role" suffix
+
+            s_roles.insert(value, key);
+        }
+
+        s_roles.insert(IdRole, QByteArrayLiteral("notificationId")); // id is QML-reserved
+    }
+
+    return s_roles;
 }

libnotificationmanager/notificationsmodel.cpp

@@ -256,7 +256,7 @@ void NotificationsModel::setLastRead(const QDateTime &lastRead)
 
 QVariant NotificationsModel::data(const QModelIndex &index, int role) const
 {
-    if (!index.isValid() || index.row() >= d->notifications.count()) {
+    if (!checkIndex(index)) {
         return QVariant();
     }