From 1df28e5c81923272123fd8ddccb8a2cd28d199b3 Mon Sep 17 00:00:00 2001 From: Chris Adams Date: Wed, 20 Apr 2016 13:02:16 +1000 Subject: 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 Reviewed-by: Robin Burchell Reviewed-by: Renato Araujo Oliveira Filho --- .../organizer/qdeclarativeorganizermodel.cpp | 60 ++++++++++++++++------ .../qmlorganizer/testcases/tst_collection.qml | 8 +-- .../tst_organizer_versit_export_import_e2e.qml | 1 - .../qmlorganizer/testcases/tst_organizeritems.qml | 2 +- .../qmlorganizer/testcases/tst_organizermodel.qml | 7 +-- 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 #include #include +#include #include @@ -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 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(&QTimer::start)); + connect(this, &QDeclarativeOrganizerModel::fetchHintChanged, &d_ptr->m_updateItemsTimer, static_cast(&QTimer::start)); + connect(this, &QDeclarativeOrganizerModel::sortOrdersChanged, &d_ptr->m_updateItemsTimer, static_cast(&QTimer::start)); + connect(this, &QDeclarativeOrganizerModel::startPeriodChanged, &d_ptr->m_updateItemsTimer, static_cast(&QTimer::start)); + connect(this, &QDeclarativeOrganizerModel::endPeriodChanged, &d_ptr->m_updateItemsTimer, static_cast(&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(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(true)); + d->m_fetchCollectionsTimer.start(); } /*! @@ -561,11 +583,11 @@ void QDeclarativeOrganizerModel::setManager(const QString& managerName) d->m_manager = new QOrganizerManager(managerName, QMap(), this); } - connect(d->m_manager, SIGNAL(dataChanged()), this, SLOT(doUpdate())); + connect(d->m_manager, &QOrganizerManager::collectionsAdded, &d->m_fetchCollectionsTimer, static_cast(&QTimer::start)); + connect(d->m_manager, &QOrganizerManager::collectionsChanged, &d->m_fetchCollectionsTimer, static_cast(&QTimer::start)); + connect(d->m_manager, &QOrganizerManager::collectionsRemoved, &d->m_fetchCollectionsTimer, static_cast(&QTimer::start)); + connect(d->m_manager, &QOrganizerManager::dataChanged, &d->m_updateTimer, static_cast(&QTimer::start)); connect(d->m_manager, SIGNAL(itemsModified(QList >)), this, SLOT(onItemsModified(QList >))); - connect(d->m_manager, SIGNAL(collectionsAdded(QList)), this, SLOT(fetchCollections())); - connect(d->m_manager, SIGNAL(collectionsChanged(QList)), this, SLOT(fetchCollections())); - connect(d->m_manager, SIGNAL(collectionsRemoved(QList)), 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 &item } } if (emitSignal) - emit modelChanged(); + d->m_modelChangedTimer.start(); } @@ -1413,6 +1435,7 @@ void QDeclarativeOrganizerModel::onItemsModified(const QListm_autoUpdate) return; + QSet addedAndChangedItems; QList 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(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(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) -- cgit v1.2.3