From 7abd8d0da4472c33d9def3a4b6786417a3195c1e Mon Sep 17 00:00:00 2001
From: Eike Hein <hein@kde.org>
Date: Sat, 3 Feb 2018 04:31:14 +0900
Subject: [PATCH] Fix dupe handling in requestAddLauncherToActivities; improve
 unit test

Summary:
The business logic was using the resolved URL in lookups in
internal data structures, instead of the internal key it
previously established equivalence to. This could lead to
junk in internal maps and unnecessarily emitted model changes.

This also improves the unit test not to require apps to be
installed, fixing the CI failure over missing Dolphin.

Reviewers: #plasma, kossebau

Subscribers: plasma-devel

Tags: #plasma

Differential Revision: https://phabricator.kde.org/D10253
---
 libtaskmanager/autotests/launchertasksmodeltest.cpp | 12 +++++-------
 libtaskmanager/launchertasksmodel.cpp               | 11 ++++++-----
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/libtaskmanager/autotests/launchertasksmodeltest.cpp b/libtaskmanager/autotests/launchertasksmodeltest.cpp
index 3d801b79d..2110bd689 100644
--- a/libtaskmanager/autotests/launchertasksmodeltest.cpp
+++ b/libtaskmanager/autotests/launchertasksmodeltest.cpp
@@ -23,6 +23,7 @@ License along with this library.  If not, see <http://www.gnu.org/licenses/>.
 #include <QTest>
 
 #include "launchertasksmodel.h"
+#include "tasktools.h"
 
 using namespace TaskManager;
 
@@ -42,16 +43,13 @@ class LauncherTasksModelTest : public QObject
 
     private:
         QStringList m_urlStrings;
-        QStringList m_mangledUrlStrings;
 };
 
 void LauncherTasksModelTest::initTestCase()
 {
     qApp->setProperty("org.kde.KActivities.core.disableAutostart", true);
-    m_urlStrings << QLatin1String("file:///usr/share/applications/org.kde.dolphin.desktop");
-    m_mangledUrlStrings << QLatin1String("applications:org.kde.dolphin.desktop");
+    m_urlStrings << QLatin1String("file:///usr/share/applications/org.kde.systemmonitor.desktop");
     m_urlStrings << QLatin1String("file:///usr/share/applications/org.kde.konversation.desktop");
-    m_mangledUrlStrings << QLatin1String("applications:org.kde.konversation.desktop");
 }
 
 void LauncherTasksModelTest::shouldRoundTripLauncherUrlList()
@@ -67,8 +65,8 @@ void LauncherTasksModelTest::shouldRoundTripLauncherUrlList()
 
     QCOMPARE(m.launcherList(), m_urlStrings);
 
-    QCOMPARE(m.data(m.index(0, 0), AbstractTasksModel::LauncherUrl).toString(), m_mangledUrlStrings.at(0));
-    QCOMPARE(m.data(m.index(1, 0), AbstractTasksModel::LauncherUrl).toString(), m_mangledUrlStrings.at(1));
+    QVERIFY(launcherUrlsMatch(m.data(m.index(0, 0), AbstractTasksModel::LauncherUrl).toUrl(), QUrl(m_urlStrings.at(0))));
+    QVERIFY(launcherUrlsMatch(m.data(m.index(1, 0), AbstractTasksModel::LauncherUrl).toUrl(), QUrl(m_urlStrings.at(1))));
 }
 
 void LauncherTasksModelTest::shouldIgnoreInvalidUrls()
@@ -128,7 +126,7 @@ void LauncherTasksModelTest::shouldAddRemoveLauncher()
     QVERIFY(added);
     QCOMPARE(launcherListChangedSpy.count(), 1);
 
-    QCOMPARE(m.launcherList().at(0), m_urlStrings.at(0));
+    QVERIFY(launcherUrlsMatch(QUrl(m.launcherList().at(0)), QUrl(m_urlStrings.at(0))));
 
     bool removed = m.requestRemoveLauncher(QUrl(m_urlStrings.at(0)));
 
diff --git a/libtaskmanager/launchertasksmodel.cpp b/libtaskmanager/launchertasksmodel.cpp
index 7c6b3428f..4691cb1ee 100644
--- a/libtaskmanager/launchertasksmodel.cpp
+++ b/libtaskmanager/launchertasksmodel.cpp
@@ -174,7 +174,8 @@ bool LauncherTasksModel::Private::requestAddLauncherToActivities(const QUrl &_ur
         if (launcherUrlsMatch(url, launcher, IgnoreQueryItems)) {
             ActivitiesSet newActivities;
 
-            if (!activitiesForLauncher.contains(url)) {
+            // Use the key we established equivalence to ('launcher').
+            if (!activitiesForLauncher.contains(launcher)) {
                 // If we don't have the activities assigned to this url
                 // for some reason
                 newActivities = activities;
@@ -185,7 +186,7 @@ bool LauncherTasksModel::Private::requestAddLauncherToActivities(const QUrl &_ur
                     // launcher should be on all activities
                     newActivities = ActivitiesSet { NULL_UUID };
 
-                } else if (isOnAllActivities(activitiesForLauncher[url])) {
+                } else if (isOnAllActivities(activitiesForLauncher[launcher])) {
                     // If we have been on all activities before, and we have
                     // been asked to be on a specific one, lets make an
                     // exception - we will set the activities to exactly
@@ -195,13 +196,13 @@ bool LauncherTasksModel::Private::requestAddLauncherToActivities(const QUrl &_ur
 
                 } else {
                     newActivities += activities;
-                    newActivities += activitiesForLauncher[url];
+                    newActivities += activitiesForLauncher[launcher];
 
                 }
             }
 
-            if (newActivities != activitiesForLauncher[url]) {
-                setActivitiesForLauncher(url, newActivities);
+            if (newActivities != activitiesForLauncher[launcher]) {
+                setActivitiesForLauncher(launcher, newActivities);
 
                 emit q->dataChanged(
                         q->index(row, 0),