Merge lp:~stolowski/unity-scopes-shell/fix-current-nav-id into lp:unity-scopes-shell

[Paweł Stołowski]

Don't conclude too early that current department id should be reset.

Don't conclude too early that current department id should be reset.

The flushUpdates() method gets called when search query finishes, but may also get called earlier (if scope is slow - plugin has a timer for that) to push whatever has been received so far. This method also resets the id of current deparment if departments have not been received and current department id is not empty.

This fix changes flushUpdates to only consider resetting current department id if the query has finished or departments have been received.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:179
http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-ci/268/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-vivid-amd64-ci/10
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-vivid-armhf-ci/10
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-vivid-armhf-ci/10/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-vivid-i386-ci/10

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-shell-ci/268/rebuild

review: Approve (continuous-integration)

[Marcus Tomlinson (marcustomlinson)]

8 - QObject::connect(&m_aggregatorTimer, &QTimer::timeout, this, &Scope::flushUpdates);
9 + QObject::connect(&m_aggregatorTimer, SIGNAL(timeout()), this, SLOT(flushUpdates()));

11 - QObject::connect(&m_clearTimer, &QTimer::timeout, this, &Scope::flushUpdates);
12 + QObject::connect(&m_clearTimer, SIGNAL(timeout()), this, SLOT(flushUpdates()));

Why are we replacing the safer C++11 connect calls with the older connect variant here?

review: Needs Information

[Pete Woods (pete-woods)]

review: Approve

[Pete Woods (pete-woods)]

Marcus. I guess that lets us use the Qt mechanism for discarding arguments from signals when you connect to a zero argument slot.

[Paweł Stołowski (stolowski)]

> Marcus. I guess that lets us use the Qt mechanism for discarding arguments
> from signals when you connect to a zero argument slot.

Yes, this is the reason.

=== modified file 'src/Unity/scope.cpp'
--- src/Unity/scope.cpp	2014-11-19 15:12:52 +0000
+++ src/Unity/scope.cpp	2014-12-04 14:53:30 +0000
@@ -103,9 +103,9 @@
     }
     QObject::connect(&m_typingTimer, &QTimer::timeout, this, &Scope::typingFinished);
     m_aggregatorTimer.setSingleShot(true);
-    QObject::connect(&m_aggregatorTimer, &QTimer::timeout, this, &Scope::flushUpdates);
+    QObject::connect(&m_aggregatorTimer, SIGNAL(timeout()), this, SLOT(flushUpdates()));
     m_clearTimer.setSingleShot(true);
-    QObject::connect(&m_clearTimer, &QTimer::timeout, this, &Scope::flushUpdates);
+    QObject::connect(&m_clearTimer, SIGNAL(timeout()), this, SLOT(flushUpdates()));
     m_invalidateTimer.setSingleShot(true);
     m_invalidateTimer.setTimerType(Qt::CoarseTimer);
     QObject::connect(&m_invalidateTimer, &QTimer::timeout, this, &Scope::invalidateResults);
@@ -148,7 +148,7 @@
     } else { // status in [FINISHED, ERROR]
         m_aggregatorTimer.stop();
 
-        flushUpdates();
+        flushUpdates(true);
 
         setSearchInProgress(false);
 
@@ -303,7 +303,7 @@
     Q_EMIT searchQueryChanged();
 }
 
-void Scope::flushUpdates()
+void Scope::flushUpdates(bool finalize)
 {
     if (m_delayedClear) {
         // TODO: here we could do resultset diffs
@@ -356,18 +356,26 @@
 
     m_lastRootDepartment = m_rootDepartment;
 
-    bool containsDepartments = m_rootDepartment.get() != nullptr;
-    // design decision - no navigation when doing searches
-    containsDepartments &= m_searchQuery.isEmpty();
-
-    if (containsDepartments != m_hasNavigation) {
-        m_hasNavigation = containsDepartments;
-        Q_EMIT hasNavigationChanged();
-    }
-
-    if (!containsDepartments && !m_currentNavigationId.isEmpty()) {
-        m_currentNavigationId = "";
-        Q_EMIT currentNavigationIdChanged();
+    //
+    // only consider resetting current department id if we are in final flushUpdates
+    // or received departments already. We don't know if we should reset it
+    // until query finishes because departments may still arrive.
+    if (finalize || m_rootDepartment.get() != nullptr)
+    {
+        bool containsDepartments = m_rootDepartment.get() != nullptr;
+        // design decision - no navigation when doing searches
+        containsDepartments &= m_searchQuery.isEmpty();
+        
+        if (containsDepartments != m_hasNavigation) {
+            m_hasNavigation = containsDepartments;
+            Q_EMIT hasNavigationChanged();
+        }
+
+        if (!containsDepartments && !m_currentNavigationId.isEmpty()) {
+            qDebug() << "Resetting current nav id";
+            m_currentNavigationId = "";
+            Q_EMIT currentNavigationIdChanged();
+        }
     }
 
     // process the alt navigation (sort order filter)
@@ -389,26 +397,34 @@
 
     m_lastSortOrderFilter = m_sortOrderFilter;
 
-    bool containsAltNav = m_sortOrderFilter.get() != nullptr;
-    // design decision - no navigation when doing searches
-    containsAltNav &= m_searchQuery.isEmpty();
-
-    if (containsAltNav != m_hasAltNavigation) {
-        m_hasAltNavigation = containsAltNav;
-        Q_EMIT hasAltNavigationChanged();
-    }
-
-    if (!containsAltNav && !m_currentAltNavigationId.isEmpty()) {
-        m_currentAltNavigationId = "";
-        Q_EMIT currentAltNavigationIdChanged();
-    }
-
-    if (containsAltNav && currentAltNav != m_currentAltNavigationId) {
-        m_currentAltNavigationId = currentAltNav;
-        Q_EMIT currentAltNavigationIdChanged();
-
-        // update the alt navigation models
-        updateNavigationModels(m_altNavTree.data(), m_altNavModels, m_currentAltNavigationId);
+    //
+    // only consider resetting alt nav id if we are in final flushUpdates
+    // or received alt nav filter already. We don't know if we should reset it
+    // until query finishes because filter may still arrive.
+    if (finalize || m_sortOrderFilter.get() != nullptr)
+    {
+        bool containsAltNav = m_sortOrderFilter.get() != nullptr;
+        // design decision - no navigation when doing searches
+        containsAltNav &= m_searchQuery.isEmpty();
+
+        if (containsAltNav != m_hasAltNavigation) {
+            m_hasAltNavigation = containsAltNav;
+            Q_EMIT hasAltNavigationChanged();
+        }
+
+        if (!containsAltNav && !m_currentAltNavigationId.isEmpty()) {
+            qDebug() << "Resetting alt nav id";
+            m_currentAltNavigationId = "";
+            Q_EMIT currentAltNavigationIdChanged();
+        }
+
+        if (containsAltNav && currentAltNav != m_currentAltNavigationId) {
+            m_currentAltNavigationId = currentAltNav;
+            Q_EMIT currentAltNavigationIdChanged();
+
+            // update the alt navigation models
+            updateNavigationModels(m_altNavTree.data(), m_altNavModels, m_currentAltNavigationId);
+        }
     }
 }
 
@@ -624,6 +640,7 @@
 void Scope::setCurrentNavigationId(QString const& id)
 {
     if (m_currentNavigationId != id) {
+        qDebug() << "Setting current nav id:" <<  this->id() << id;
         m_currentNavigationId = id;
         Q_EMIT currentNavigationIdChanged();
     }
@@ -702,6 +719,7 @@
         scopes::SearchListenerBase::SPtr listener(new SearchResultReceiver(this));
         m_searchController->setListener(listener);
         try {
+            qDebug() << "Dispatching search:" << id() << m_searchQuery << m_currentNavigationId;
             scopes::QueryCtrlProxy controller = m_proxy->search(m_searchQuery.toStdString(), m_currentNavigationId.toStdString(), m_filterState, meta, listener);
             m_searchController->setController(controller);
         } catch (std::exception& e) {

=== modified file 'src/Unity/scope.h'
--- src/Unity/scope.h	2014-10-15 09:59:13 +0000
+++ src/Unity/scope.h	2014-12-04 14:53:30 +0000
@@ -178,7 +178,7 @@
 
 private Q_SLOTS:
     void typingFinished();
-    void flushUpdates();
+    void flushUpdates(bool finalize = false);
     void metadataRefreshed();
     void internetFlagChanged(QString const& key);
     void departmentModelDestroyed(QObject* obj);