Merge lp:~stolowski/unity-scopes-shell/favoriting-fixes into lp:unity-scopes-shell

[Paweł Stołowski]

Optimize scope favoriting. Make sure click scope is favorited back if reinstalled.

Optimize and refactor scope favoriting. Make sure click scope is favorited back if reinstalled.

The optimization is mostly achieved by temporarily disabling gesttings signals when storing an updated list of favorites.

[Marcus Tomlinson (marcustomlinson)]

Outstanding work Pawel. +1

review: Approve

=== modified file 'src/Unity/CMakeLists.txt'
--- src/Unity/CMakeLists.txt	2016-06-02 08:49:55 +0000
+++ src/Unity/CMakeLists.txt	2016-08-10 07:21:23 +0000
@@ -20,6 +20,7 @@
     collectors.cpp
     department.cpp
     departmentnode.cpp
+    favorites.cpp
     filters.cpp
     filtergroupwidget.cpp
     optionselectorfilter.cpp

=== added file 'src/Unity/favorites.cpp'
--- src/Unity/favorites.cpp	1970-01-01 00:00:00 +0000
+++ src/Unity/favorites.cpp	2016-08-10 07:21:23 +0000
@@ -0,0 +1,160 @@
+/*
+ * Copyright (C) 2016 Canonical, Ltd.
+ *
+ * Authors:
+ *  Pawel Stolowski <[e-mail]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 3.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <QGSettings>
+#include <QVariant>
+#include <QDebug>
+#include <unity/scopes/CannedQuery.h>
+#include <unity/UnityExceptions.h>
+#include <algorithm>
+#include "favorites.h"
+
+namespace scopes_ng
+{
+
+Favorites::Favorites(QObject *parent, QGSettings *dashSettings)
+    : QObject(parent),
+      m_dashSettings(dashSettings)
+{
+    if (m_dashSettings) {
+        readFavoritesFromGSettings();
+        QObject::connect(m_dashSettings, &QGSettings::changed, this, &Favorites::dashSettingsChanged);
+    }
+}
+
+void Favorites::readFavoritesFromGSettings()
+{
+    m_favoriteScopes.clear();
+    m_positionLookup.clear();
+
+    int pos = 0 ;
+    auto const favs = m_dashSettings->get(QStringLiteral("favoriteScopes")).toList();
+    for (auto const fv: favs) {
+        try
+        {
+            auto const query = unity::scopes::CannedQuery::from_uri(fv.toString().toStdString());
+            auto scopeId = QString::fromStdString(query.scope_id());
+            m_favoriteScopes.append(scopeId);
+            m_positionLookup[scopeId] = pos++;
+        }
+        catch (const unity::InvalidArgumentException &e)
+        {
+            qWarning() << "Invalid canned query '" << fv.toString() << "'" << QString::fromStdString(e.what());
+        }
+    }
+}
+
+Favorites::~Favorites()
+{
+}
+
+int Favorites::setFavorite(QString const& scopeId, bool value)
+{
+    if (!value) {
+        int pos = position(scopeId);
+        if (pos >= 0) {
+            m_favoriteScopes.removeAt(pos);
+            m_positionLookup.remove(scopeId);
+            for (int i = pos; i<m_favoriteScopes.size(); i++) {
+                m_positionLookup[m_favoriteScopes[i]] = i;
+            }
+            Q_ASSERT(m_favoriteScopes.size() == m_positionLookup.size());
+            storeFavorites();
+            return pos;
+        }
+    } else {
+        int pos = position(scopeId);
+        if (pos < 0) {
+            m_favoriteScopes.push_back(scopeId);
+            pos = m_favoriteScopes.size() - 1;
+            m_positionLookup[scopeId] = pos;
+        }
+        Q_ASSERT(m_favoriteScopes.size() == m_positionLookup.size());
+        storeFavorites();
+        return pos;
+    }
+
+    return -1;
+}
+
+void Favorites::moveFavoriteTo(QString const& scopeId, int pos)
+{
+    int oldPos = position(scopeId);
+    if (oldPos >= 0) {
+        m_favoriteScopes.move(oldPos, pos);
+        auto const range = std::minmax(oldPos, pos);
+        for (int i = range.first; i<=range.second; i++) {
+            m_positionLookup[m_favoriteScopes[i]] = i;
+        }
+    } else {
+        qWarning() << "Favorites::moveFavoriteTo: no such scope" << scopeId;
+    }
+
+    storeFavorites();
+
+    Q_ASSERT(m_favoriteScopes.size() == m_positionLookup.size());
+}
+
+QStringList Favorites::getFavorites()
+{
+    return m_favoriteScopes;
+}
+
+bool Favorites::hasScope(QString const& scopeId) const
+{
+    return m_positionLookup.find(scopeId) != m_positionLookup.end();
+}
+
+int Favorites::position(QString const& scopeId) const
+{
+    auto it = m_positionLookup.find(scopeId);
+    if (it != m_positionLookup.end()) {
+        return it.value();
+    }
+    return -1;
+}
+
+void Favorites::dashSettingsChanged(QString const &key)
+{
+    if (key != QLatin1String("favoriteScopes")) {
+        return;
+    }
+    readFavoritesFromGSettings();
+    Q_EMIT favoritesChanged();
+}
+
+void Favorites::storeFavorites()
+{
+    if (m_dashSettings) {
+        QStringList cannedQueries;
+        for (auto const& fav: m_favoriteScopes)
+        {
+            const QString query = "scope://" + fav;
+            cannedQueries.push_back(query);
+        }
+
+        QObject::disconnect(m_dashSettings, &QGSettings::changed, this, &Favorites::dashSettingsChanged);
+        m_dashSettings->set(QStringLiteral("favoriteScopes"), QVariant(cannedQueries));
+        QObject::connect(m_dashSettings, &QGSettings::changed, this, &Favorites::dashSettingsChanged);
+    }
+}
+
+} // namespace scopes_ng
+
+#include <favorites.moc>

=== added file 'src/Unity/favorites.h'
--- src/Unity/favorites.h	1970-01-01 00:00:00 +0000
+++ src/Unity/favorites.h	2016-08-10 07:21:23 +0000
@@ -0,0 +1,63 @@
+/*
+ * Copyright (C) 2016 Canonical, Ltd.
+ *
+ * Authors:
+ *  Pawel Stolowski <[e-mail]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 3.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef NG_FAVORITES_H
+#define NG_FAVORITES_H
+
+#include <QStringList>
+#include <QObject>
+#include <QPointer>
+#include <QMap>
+
+class QGSettings;
+
+namespace scopes_ng
+{
+
+class Q_DECL_EXPORT Favorites : public QObject
+{
+    Q_OBJECT
+public:
+    Favorites(QObject *parent, QGSettings *dashSettings);
+    ~Favorites();
+
+    int setFavorite(QString const& scopeId, bool value);
+    void moveFavoriteTo(QString const& scopeId, int pos);
+    bool hasScope(QString const& scopeId) const;
+    int position(QString const& scopeId) const;
+    QStringList getFavorites();
+    void storeFavorites();
+
+Q_SIGNALS:
+    void favoritesChanged();
+
+private Q_SLOTS:
+    void dashSettingsChanged(QString const &key);
+
+private:
+    void readFavoritesFromGSettings();
+
+    QPointer<QGSettings> m_dashSettings;
+    QStringList m_favoriteScopes;
+    QMap<QString, int> m_positionLookup;
+};
+
+} // namespace scopes_ng
+
+#endif // NG_SCOPES_H

=== modified file 'src/Unity/scope.cpp'
--- src/Unity/scope.cpp	2016-06-24 14:03:59 +0000
+++ src/Unity/scope.cpp	2016-08-10 07:21:23 +0000
@@ -75,13 +75,13 @@
 const int RESULTS_TTL_LARGE = 3600000; // 1 hour
 const int SEARCH_CARDINALITY = 300; // maximum number of results accepted from a single scope
 
-Scope::Ptr Scope::newInstance(scopes_ng::Scopes* parent)
+Scope::Ptr Scope::newInstance(scopes_ng::Scopes* parent, bool favorite)
 {
-    auto scope = Scope::Ptr(new Scope(parent), &QObject::deleteLater);
+    auto scope = Scope::Ptr(new Scope(parent, favorite), &QObject::deleteLater);
     return scope;
 }
 
-Scope::Scope(scopes_ng::Scopes* parent) :
+Scope::Scope(scopes_ng::Scopes* parent, bool favorite) :
       m_query_id(0)
     , m_formFactor(QStringLiteral("phone"))
     , m_activeFiltersCount(0)
@@ -91,7 +91,7 @@
     , m_resultsDirty(false)
     , m_delayedSearchProcessing(false)
     , m_hasNavigation(false)
-    , m_favorite(false)
+    , m_favorite(favorite)
     , m_initialQueryDone(false)
     , m_childScopesDirty(true)
     , m_searchController(new CollectionController)

=== modified file 'src/Unity/scope.h'
--- src/Unity/scope.h	2016-06-23 09:27:45 +0000
+++ src/Unity/scope.h	2016-08-10 07:21:23 +0000
@@ -110,7 +110,7 @@
 public:
     typedef QSharedPointer<Scope> Ptr;
 
-    static Scope::Ptr newInstance(scopes_ng::Scopes* parent);
+    static Scope::Ptr newInstance(scopes_ng::Scopes* parent, bool favorite = false);
 
     virtual ~Scope();
 
@@ -205,7 +205,7 @@
     void previewModelDestroyed(QObject *obj);
 
 protected:
-    explicit Scope(scopes_ng::Scopes* parent);
+    explicit Scope(scopes_ng::Scopes* parent, bool favorite = false);
 
     void setStatus(unity::shell::scopes::ScopeInterface::Status status);
     void invalidateLastSearch();

=== modified file 'src/Unity/scopes.cpp'
--- src/Unity/scopes.cpp	2016-06-23 12:53:21 +0000
+++ src/Unity/scopes.cpp	2016-08-10 07:21:23 +0000
@@ -24,6 +24,7 @@
 #include "scope.h"
 #include "overviewscope.h"
 #include "ubuntulocationservice.h"
+#include "favorites.h"
 
 // Qt
 #include <QDebug>
@@ -126,10 +127,8 @@
     QDBusConnection::sessionBus().connect(QString(), QStringLiteral("/com/canonical/unity/scopes"), QStringLiteral("com.canonical.unity.scopes"), QStringLiteral("InvalidateResults"), this, SLOT(invalidateScopeResults(QString)));
 
     m_dashSettings = QGSettings::isSchemaInstalled("com.canonical.Unity.Dash") ? new QGSettings("com.canonical.Unity.Dash", QByteArray(), this) : nullptr;
-    if (m_dashSettings)
-    {
-        QObject::connect(m_dashSettings, &QGSettings::changed, this, &Scopes::dashSettingsChanged);
-    }
+    m_favoriteScopes = new Favorites(this, m_dashSettings);
+    QObject::connect(m_favoriteScopes, &Favorites::favoritesChanged, this, &Scopes::favoritesChanged);
 
     m_overviewScope = OverviewScope::newInstance(this);
 
@@ -427,57 +426,41 @@
 
 void Scopes::processFavoriteScopes()
 {
+    qDebug() << "Scopes::processFavoriteScopes()";
+
     if (m_noFavorites) {
         return;
     }
 
     //
-    // read the favoriteScopes array value from gsettings.
-    // process it and turn its values into scope ids.
     // create new Scope objects or remove existing according to the list of favorities.
     // notify about scopes model changes accordingly.
     if (m_dashSettings) {
-        QStringList newFavorites;
-        QMap<QString, int> favScopesLut;
-        for (auto const& fv: m_dashSettings->get(QStringLiteral("favoriteScopes")).toList())
+        for (auto const& fv: m_favoriteScopes->getFavorites())
         {
-            int pos = 0;
-            try
-            {
-                auto const query = unity::scopes::CannedQuery::from_uri(fv.toString().toStdString());
-                const QString id = QString::fromStdString(query.scope_id());
-
-                if (m_cachedMetadata.find(id) != m_cachedMetadata.end())
-                {
-                    newFavorites.push_back(id);
-                    pos = newFavorites.size() - 1;
-                    favScopesLut[id] = pos;
-                }
-                else
-                {
-                    // If a scope that was favorited no longer exists, unfavorite it in m_dashSettings
-                    setFavorite(id, false);
-                }
-            }
-            catch (const InvalidArgumentException &e)
-            {
-                qWarning() << "Invalid canned query '" << fv.toString() << "'" << QString::fromStdString(e.what());
-            }
-        }
-
-        // this prevents further processing if we get called back when calling scope->setFavorite() below
-        if (m_favoriteScopes == newFavorites)
-            return;
-
-        m_favoriteScopes = newFavorites;
-
-        QSet<QString> oldScopes;
+            // favorited scope not installed?
+            if (m_cachedMetadata.find(fv) == m_cachedMetadata.end())
+            {
+                qDebug() << "Favorited scope" << fv << "is no longer available, un-favoriting";
+                m_favoriteScopes->setFavorite(fv, false);
+            }
+        }
+
+        // special-case clickscope; append it to favorites if it was uninstalled (and in consequence removed from favorites) - see LP: #1603186
+        if (m_cachedMetadata.contains(CLICK_SCOPE_ID) && !m_favoriteScopes->hasScope(CLICK_SCOPE_ID)) {
+            qDebug() << "Favoriting" << CLICK_SCOPE_ID;
+            m_favoriteScopes->setFavorite(CLICK_SCOPE_ID, true);
+        }
+
+        QSet<QString> scopesInTheModel;
         int row = 0;
         // remove un-favorited scopes
         for (auto it = m_scopes.begin(); it != m_scopes.end();)
         {
-            if (!favScopesLut.contains((*it)->id()))
+            if (!m_favoriteScopes->hasScope((*it)->id()))
             {
+                qDebug() << "Scope" << (*it)->id() << "is no longer favorited, removing";
+
                 beginRemoveRows(QModelIndex(), row, row);
                 Scope::Ptr toDelete = *it;
                 toDelete->setFavorite(false);
@@ -490,7 +473,7 @@
             }
             else
             {
-                oldScopes.insert((*it)->id());
+                scopesInTheModel.insert((*it)->id());
                 ++it;
                 ++row;
             }
@@ -498,41 +481,35 @@
 
         // add new favorites
         row = 0;
-        for (auto favIt = m_favoriteScopes.begin(); favIt != m_favoriteScopes.end(); )
+        for (auto const& fv: m_favoriteScopes->getFavorites())
         {
-            auto const fav = *favIt;
-            if (!oldScopes.contains(fav))
+            if (!scopesInTheModel.contains(fv))
             {
-                auto it = m_cachedMetadata.find(fav);
+                auto it = m_cachedMetadata.find(fv);
                 if (it != m_cachedMetadata.end())
                 {
-                    Scope::Ptr scope = Scope::newInstance(this);
+                    qDebug() << "Scope" << fv << "is favorited, adding to scopes model";
+
+                    Scope::Ptr scope = Scope::newInstance(this, true);
                     connect(scope.data(), SIGNAL(isActiveChanged()), this, SLOT(prepopulateNextScopes()));
                     scope->setScopeData(*(it.value()));
-                    scope->setFavorite(true);
                     beginInsertRows(QModelIndex(), row, row);
                     m_scopes.insert(row, scope);
                     endInsertRows();
                 }
-                else
-                {
-                    qWarning() << "No such scope:" << fav;
-                    favIt = m_favoriteScopes.erase(favIt);
-                    continue;
-                }
             }
             ++row;
-            ++favIt;
         }
 
-        // iterate over results, move rows if positions changes
+        // iterate over results, move rows if positions changed
         for (int i = 0; i<m_scopes.size(); )
         {
             auto scope = m_scopes.at(i);
             const QString id = scope->id();
-            if (favScopesLut.contains(id)) {
-                int pos = favScopesLut[id];
+            int pos = m_favoriteScopes->position(id);
+            if (pos >= 0) {
                 if (pos != i) {
+                    qDebug() << "Moving scope" << id << "to row" << pos << "to match position in favorites";
                     beginMoveRows(QModelIndex(), i, i, QModelIndex(), pos + (pos > i ? 1 : 0));
                     m_scopes.move(i, pos);
                     endMoveRows();
@@ -544,17 +521,13 @@
     }
 }
 
-void Scopes::dashSettingsChanged(QString const& key)
+void Scopes::favoritesChanged()
 {
-    if (key != QLatin1String("favoriteScopes")) {
-        return;
-    }
-
     processFavoriteScopes();
 
     if (m_overviewScope)
     {
-        m_overviewScope->updateFavorites(m_favoriteScopes);
+        m_overviewScope->updateFavorites(m_favoriteScopes->getFavorites());
     }
 }
 
@@ -675,7 +648,7 @@
 
 QStringList Scopes::getFavoriteIds() const
 {
-    return m_favoriteScopes;
+    return m_favoriteScopes->getFavorites();
 }
 
 void Scopes::setFavorite(QString const& scopeId, bool value)
@@ -685,32 +658,39 @@
         qWarning() << "Cannot unfavorite" << scopeId;
         return;
     }
-    if (m_dashSettings)
+
+    int row = m_favoriteScopes->setFavorite(scopeId, value);
+    if (row >= 0)
     {
-        QStringList cannedQueries;
-        bool changed = false;
-
-        for (auto const& fav: m_favoriteScopes)
-        {
-            if (value == false && fav == scopeId) {
-                changed = true;
-                continue; // skip it
-            }
-            // TODO: use CannedQuery::to_uri() when we really support them
-            const QString query = "scope://" + fav;
-            cannedQueries.push_back(query);
-        }
-
-        if (value && !m_favoriteScopes.contains(scopeId)) {
-            const QString query = "scope://" + scopeId;
-            cannedQueries.push_back(query);
-            changed = true;
-        }
-
-        if (changed) {
-            // update gsettings entry
-            // note: this will trigger notification, so that new favorites are processed by processFavoriteScopes
-            m_dashSettings->set(QStringLiteral("favoriteScopes"), QVariant(cannedQueries));
+        if (value) {
+            auto it = m_cachedMetadata.find(scopeId);
+            if (it != m_cachedMetadata.end())
+            {
+                Scope::Ptr scope = Scope::newInstance(this, true);
+                connect(scope.data(), SIGNAL(isActiveChanged()), this, SLOT(prepopulateNextScopes()));
+                scope->setScopeData(*(it.value()));
+                beginInsertRows(QModelIndex(), row, row);
+                m_scopes.insert(row, scope);
+                endInsertRows();
+            } else {
+                qWarning() << "setFavorite: unknown scope" << scopeId;
+            }
+        } else {
+            for (auto it = m_scopes.begin(); it != m_scopes.end(); it++)
+            {
+                if ((*it)->id() == scopeId) {
+                    beginRemoveRows(QModelIndex(), row, row);
+                    Scope::Ptr toDelete = *it;
+                    toDelete->setFavorite(false);
+                    // we need to delay actual deletion of Scope object so that shell can animate it
+                    m_scopesToDelete.push_back(toDelete);
+                    // if the timer is already active, we just wait a bit longer, which is no problem
+                    m_scopesToDeleteTimer.start();
+                    it = m_scopes.erase(it);
+                    endRemoveRows();
+                    break;
+                }
+            }
         }
     }
 }
@@ -737,34 +717,12 @@
 
 void Scopes::moveFavoriteTo(QString const& scopeId, int index)
 {
-    if (m_dashSettings)
-    {
-        QStringList cannedQueries;
-        bool found = false;
-
-        int i = 0;
-        for (auto const& fav: m_favoriteScopes)
-        {
-            if (fav == scopeId) {
-                if (index == i)
-                    return; // same position
-                found = true;
-            } else {
-                const QString query = "scope://" + fav;
-                cannedQueries.push_back(query);
-            }
-
-            ++i;
-        }
-
-        if (found) {
-            // insert scopeId at new position
-            const QString query = "scope://" + scopeId;
-            cannedQueries.insert(index, query);
-            // update gsettings entry
-            // note: this will trigger notification, so that new favorites are processed by processFavoriteScopes
-            m_dashSettings->set(QStringLiteral("favoriteScopes"), QVariant(cannedQueries));
-        }
+    int oldPos = m_favoriteScopes->position(scopeId);
+    if (oldPos != index) {
+        m_favoriteScopes->moveFavoriteTo(scopeId, index);
+        beginMoveRows(QModelIndex(), oldPos, oldPos, QModelIndex(), index + (index > oldPos ? 1 : 0));
+        m_scopes.move(oldPos, index);
+        endMoveRows();
     }
 }
 

=== modified file 'src/Unity/scopes.h'
--- src/Unity/scopes.h	2016-06-10 13:41:37 +0000
+++ src/Unity/scopes.h	2016-08-10 07:21:23 +0000
@@ -47,6 +47,7 @@
 class UbuntuLocationService;
 class LocationAccessHelper;
 class Scope;
+class Favorites;
 class OverviewScope;
 
 class Q_DECL_EXPORT Scopes : public unity::shell::scopes::ScopesInterface
@@ -92,7 +93,7 @@
     virtual QString readPartnerId();
 
 private Q_SLOTS:
-    void dashSettingsChanged(QString const &key);
+    void favoritesChanged();
     void processFavoriteScopes();
     void populateScopes();
     void discoveryFinished();
@@ -117,7 +118,7 @@
     QList<QSharedPointer<Scope>> m_scopes;
     QList<QSharedPointer<Scope>> m_scopesToDelete;
     bool m_noFavorites;
-    QStringList m_favoriteScopes;
+    Favorites* m_favoriteScopes;
     QGSettings* m_dashSettings;
     QMap<QString, unity::scopes::ScopeMetadata::SPtr> m_cachedMetadata;
     QSharedPointer<OverviewScope> m_overviewScope;