Coalesce signals from QDeclarativeOrganizerModel

This commit ensures that signal handling and emission within the
declarative organizer item model is coalesced, to avoid unnecessary
processing, and to avoid accidentally duplicated signal emission.
It then fixes the unit tests to enforce this behavior.

Change-Id: I011221a832f932360ad965a1b97fb8e309fb0e63
Reviewed-by: Matthew Vogt <matthew.vogt@qinetic.com.au>
Reviewed-by: Robin Burchell <robin.burchell@viroteck.net>
Reviewed-by: Renato Araujo Oliveira Filho <renato.filho@canonical.com>

5 files changed, 53 insertions, 25 deletions

diff --git a/src/imports/organizer/qdeclarativeorganizermodel.cpp b/src/imports/organizer/qdeclarativeorganizermodel.cpp
index d77b786d4..e270ecf66 100644
--- a/src/imports/organizer/qdeclarativeorganizermodel.cpp
+++ b/src/imports/organizer/qdeclarativeorganizermodel.cpp
@@ -37,6 +37,7 @@
 #include <QtCore/qmath.h>
 #include <QtCore/qurl.h>
 #include <QtCore/qpointer.h>
+#include <QtCore/qtimer.h>
 
 #include <QtQml/qqmlinfo.h>
 
@@ -79,6 +80,7 @@ static QString urlToLocalFileName(const QUrl& url)
 }
 
 static const char ITEM_TO_SAVE_PROPERTY[] = {"ITEM_TO_SAVE_PROPERTY"};
+static const char MANUALLY_TRIGGERED_PROPERTY[] = {"MANUALLY_TRIGGERED"};
 
 class QDeclarativeOrganizerModelPrivate
 {
@@ -127,6 +129,11 @@ public:
     QDateTime m_endPeriod;
     QList<QDeclarativeOrganizerCollection*> m_collections;
 
+    QTimer m_updateTimer;
+    QTimer m_updateItemsTimer;
+    QTimer m_fetchCollectionsTimer;
+    QTimer m_modelChangedTimer;
+
     QOrganizerManager::Error m_error;
 
     bool m_autoUpdate;
@@ -190,12 +197,24 @@ QDeclarativeOrganizerModel::QDeclarativeOrganizerModel(QObject *parent) :
     roleNames.insert(OrganizerItemRole, "item");
     setRoleNames(roleNames);
 
-    connect(this, SIGNAL(managerChanged()), SLOT(doUpdate()));
-    connect(this, SIGNAL(filterChanged()), SLOT(doUpdateItems()));
-    connect(this, SIGNAL(fetchHintChanged()), SLOT(doUpdateItems()));
-    connect(this, SIGNAL(sortOrdersChanged()), SLOT(doUpdateItems()));
-    connect(this, SIGNAL(startPeriodChanged()), SLOT(doUpdateItems()));
-    connect(this, SIGNAL(endPeriodChanged()), SLOT(doUpdateItems()));
+    d_ptr->m_updateTimer.setSingleShot(true);
+    d_ptr->m_updateItemsTimer.setSingleShot(true);
+    d_ptr->m_fetchCollectionsTimer.setSingleShot(true);
+    d_ptr->m_modelChangedTimer.setSingleShot(true);
+    d_ptr->m_updateTimer.setInterval(1);
+    d_ptr->m_updateItemsTimer.setInterval(1);
+    d_ptr->m_fetchCollectionsTimer.setInterval(1);
+    d_ptr->m_modelChangedTimer.setInterval(1);
+    connect(&d_ptr->m_updateTimer, &QTimer::timeout, this, &QDeclarativeOrganizerModel::doUpdate);
+    connect(&d_ptr->m_updateItemsTimer, &QTimer::timeout, this, &QDeclarativeOrganizerModel::doUpdateItems);
+    connect(&d_ptr->m_fetchCollectionsTimer, &QTimer::timeout, this, &QDeclarativeOrganizerModel::fetchCollections);
+    connect(&d_ptr->m_modelChangedTimer, &QTimer::timeout, this, &QDeclarativeOrganizerModel::modelChanged);
+
+    connect(this, &QDeclarativeOrganizerModel::filterChanged, &d_ptr->m_updateItemsTimer, static_cast<void (QTimer::*)(void)>(&QTimer::start));
+    connect(this, &QDeclarativeOrganizerModel::fetchHintChanged, &d_ptr->m_updateItemsTimer, static_cast<void (QTimer::*)(void)>(&QTimer::start));
+    connect(this, &QDeclarativeOrganizerModel::sortOrdersChanged, &d_ptr->m_updateItemsTimer, static_cast<void (QTimer::*)(void)>(&QTimer::start));
+    connect(this, &QDeclarativeOrganizerModel::startPeriodChanged, &d_ptr->m_updateItemsTimer, static_cast<void (QTimer::*)(void)>(&QTimer::start));
+    connect(this, &QDeclarativeOrganizerModel::endPeriodChanged, &d_ptr->m_updateItemsTimer, static_cast<void (QTimer::*)(void)>(&QTimer::start));
 }
 
 QDeclarativeOrganizerModel::~QDeclarativeOrganizerModel()
@@ -297,9 +316,11 @@ void QDeclarativeOrganizerModel::update()
     Q_D(QDeclarativeOrganizerModel);
     if (!d->m_componentCompleted || d->m_updatePendingFlag)
         return;
+
     // Disallow possible duplicate request triggering
     d->m_updatePendingFlag = (QDeclarativeOrganizerModelPrivate::UpdatingItemsPending | QDeclarativeOrganizerModelPrivate::UpdatingCollectionsPending);
-    QMetaObject::invokeMethod(this, "fetchCollections", Qt::QueuedConnection);
+    d->m_fetchCollectionsTimer.setProperty(MANUALLY_TRIGGERED_PROPERTY, QVariant::fromValue<bool>(true));
+    d->m_fetchCollectionsTimer.start();
 }
 
 void QDeclarativeOrganizerModel::doUpdate()
@@ -349,7 +370,8 @@ void QDeclarativeOrganizerModel::updateCollections()
     if (!d->m_componentCompleted || d->m_updatePendingFlag)
         return;
     d->m_updatePendingFlag = QDeclarativeOrganizerModelPrivate::UpdatingCollectionsPending;// Disallow possible duplicate request triggering
-    QMetaObject::invokeMethod(this, "fetchCollections", Qt::QueuedConnection);
+    d->m_fetchCollectionsTimer.setProperty(MANUALLY_TRIGGERED_PROPERTY, QVariant::fromValue<bool>(true));
+    d->m_fetchCollectionsTimer.start();
 }
 
 /*!
@@ -561,11 +583,11 @@ void QDeclarativeOrganizerModel::setManager(const QString& managerName)
         d->m_manager = new QOrganizerManager(managerName, QMap<QString, QString>(), this);
     }
 
-    connect(d->m_manager, SIGNAL(dataChanged()), this, SLOT(doUpdate()));
+    connect(d->m_manager, &QOrganizerManager::collectionsAdded, &d->m_fetchCollectionsTimer, static_cast<void (QTimer::*)(void)>(&QTimer::start));
+    connect(d->m_manager, &QOrganizerManager::collectionsChanged, &d->m_fetchCollectionsTimer, static_cast<void (QTimer::*)(void)>(&QTimer::start));
+    connect(d->m_manager, &QOrganizerManager::collectionsRemoved, &d->m_fetchCollectionsTimer, static_cast<void (QTimer::*)(void)>(&QTimer::start));
+    connect(d->m_manager, &QOrganizerManager::dataChanged, &d->m_updateTimer, static_cast<void (QTimer::*)(void)>(&QTimer::start));
     connect(d->m_manager, SIGNAL(itemsModified(QList<QPair<QOrganizerItemId,QOrganizerManager::Operation> >)), this, SLOT(onItemsModified(QList<QPair<QOrganizerItemId,QOrganizerManager::Operation> >)));
-    connect(d->m_manager, SIGNAL(collectionsAdded(QList<QOrganizerCollectionId>)), this, SLOT(fetchCollections()));
-    connect(d->m_manager, SIGNAL(collectionsChanged(QList<QOrganizerCollectionId>)), this, SLOT(fetchCollections()));
-    connect(d->m_manager, SIGNAL(collectionsRemoved(QList<QOrganizerCollectionId>)), this, SLOT(fetchCollections()));
 
     const QOrganizerManager::Error managerError = d->m_manager->error();
     if (QOrganizerManager::NoError != managerError && d->m_error != managerError) {
@@ -1231,7 +1253,7 @@ void QDeclarativeOrganizerModel::requestUpdated()
         endResetModel();
 
         d->m_itemIdHash = newItemIdHash;
-        emit modelChanged();
+        d->m_modelChangedTimer.start();
     }
 }
 
@@ -1399,7 +1421,7 @@ void QDeclarativeOrganizerModel::removeItemsFromModel(const QList<QString> &item
         }
     }
     if (emitSignal)
-        emit modelChanged();
+        d->m_modelChangedTimer.start();
 }
 
 
@@ -1413,6 +1435,7 @@ void QDeclarativeOrganizerModel::onItemsModified(const QList<QPair<QOrganizerIte
     Q_D(QDeclarativeOrganizerModel);
     if (!d->m_autoUpdate)
         return;
+
     QSet<QOrganizerItemId> addedAndChangedItems;
     QList<QString> removedItems;
     for (int i = itemIds.size() - 1; i >= 0; i--) {
@@ -1597,7 +1620,7 @@ void QDeclarativeOrganizerModel::onItemsModifiedFetchRequestStateChanged(QOrgani
         }
 
         if (emitSignal)
-            emit modelChanged();
+            d->m_modelChangedTimer.start();
     }
     d->m_notifiedItems.remove(request);
     request->deleteLater();
@@ -1613,8 +1636,11 @@ void QDeclarativeOrganizerModel::fetchCollections()
     // fetchCollections() is used for both direct calls and
     // signals from model. For signal from model, check also the
     // autoupdate-flag.
-    if (sender() == d->m_manager && !d->m_autoUpdate) {
-        return;
+    if (qobject_cast<QTimer*>(sender()) == &d->m_fetchCollectionsTimer) {
+        if (!d->m_fetchCollectionsTimer.property(MANUALLY_TRIGGERED_PROPERTY).toBool() && !d->m_autoUpdate)
+            return; // it was automatically triggered, but autoUpdate is false, so don't update the model data.
+        // reset the state of the manually triggered properly, for next time.
+        d->m_fetchCollectionsTimer.setProperty(MANUALLY_TRIGGERED_PROPERTY, QVariant::fromValue<bool>(false));
     }
 
     QOrganizerCollectionFetchRequest* req = new QOrganizerCollectionFetchRequest(this);
diff --git a/tests/auto/organizer/qmlorganizer/testcases/tst_collection.qml b/tests/auto/organizer/qmlorganizer/testcases/tst_collection.qml
index 0faa90835..8ae8aa5db 100644
--- a/tests/auto/organizer/qmlorganizer/testcases/tst_collection.qml
+++ b/tests/auto/organizer/qmlorganizer/testcases/tst_collection.qml
@@ -297,15 +297,17 @@ TestCase {
         var savedEvent = organizerModel.items[organizerModel.items.length - 1];
         compare(savedEvent.collectionId, organizerModel.defaultCollectionId());//savedEvent sometimes undefined!?!?!?
         spySettingCollectionId.target = savedEvent;
+        spySettingCollectionId.wait(spyWaitDelay);
+        compare(spySettingCollectionId.count, 1);
 
         // set different collection
         verify(savedCollection.collectionId != organizerModel.defaultCollectionId())
         savedEvent.collectionId = savedCollection.collectionId;
         spySettingCollectionId.wait(spyWaitDelay);
-        compare(spySettingCollectionId.count, 1);
-        // set same collection again
+        compare(spySettingCollectionId.count, 2);
+        // set same collection again, shouldn't cause signal emission
         savedEvent.collectionId = savedCollection.collectionId;
-        compare(spySettingCollectionId.count, 1);
+        compare(spySettingCollectionId.count, 2);
 
         // check the changed collection is saved
         var errorsChangedSpy = create_testobject("import QtTest 1.0 \nSignalSpy {}");
diff --git a/tests/auto/organizer/qmlorganizer/testcases/tst_organizer_versit_export_import_e2e.qml b/tests/auto/organizer/qmlorganizer/testcases/tst_organizer_versit_export_import_e2e.qml
index 222bf7e39..aed1841e9 100644
--- a/tests/auto/organizer/qmlorganizer/testcases/tst_organizer_versit_export_import_e2e.qml
+++ b/tests/auto/organizer/qmlorganizer/testcases/tst_organizer_versit_export_import_e2e.qml
@@ -140,7 +140,6 @@ TestCase {
     function test_organizerImportExportSignaling(data) {
 
         organizerModel.manager = data.managerToBeTested
-        modelChangedSpy.wait()
 
         // Save test Event.
         saveTestEvent()
diff --git a/tests/auto/organizer/qmlorganizer/testcases/tst_organizeritems.qml b/tests/auto/organizer/qmlorganizer/testcases/tst_organizeritems.qml
index f7760d6da..9da7bb265 100644
--- a/tests/auto/organizer/qmlorganizer/testcases/tst_organizeritems.qml
+++ b/tests/auto/organizer/qmlorganizer/testcases/tst_organizeritems.qml
@@ -117,7 +117,7 @@ Rectangle {
                 model.saveItem(newEvent2);
                 utility.waitModelChange(1);
                 model.saveItem(newEvent1);
-                utility.waitModelChange(2);
+                utility.waitModelChange(1);
                 var deletList = model.itemIds();
                 model.removeItems(deletList);
                 utility.waitModelChange(0);
diff --git a/tests/auto/organizer/qmlorganizer/testcases/tst_organizermodel.qml b/tests/auto/organizer/qmlorganizer/testcases/tst_organizermodel.qml
index 69bdd97da..fb9471f06 100644
--- a/tests/auto/organizer/qmlorganizer/testcases/tst_organizermodel.qml
+++ b/tests/auto/organizer/qmlorganizer/testcases/tst_organizermodel.qml
@@ -952,13 +952,12 @@ TestCase {
             collectionsChangedSpy.target = organizerModel
             collectionsChangedSpy.signalName = "collectionsChanged"
 
-            organizerModel.autoUpdate = false;
-
             // starting point
             compare(organizerModel.items.length, 0);
             compare(organizerModel.collections.length, 1);
 
-            // updateItems() - should update only items
+            // autoUpdate is false, so these operations should not cause signal emission
+            organizerModel.autoUpdate = false;
             organizerModel.saveItem(event1);
             organizerModel.saveCollection(collection1);
             wait(signalWaitTime)
@@ -966,6 +965,8 @@ TestCase {
             compare(collectionsChangedSpy.count, 0)
             compare(organizerModel.items.length, 0)
             compare(organizerModel.collections.length, 1)
+
+            // updateItems() - should update only items count
             organizerModel.updateItems();
             modelChangedSpy.wait(signalWaitTime)
             compare(modelChangedSpy.count, 1)