From 0f24357d01bd8a278f03793db863d3f71ac37ef2 Mon Sep 17 00:00:00 2001 From: Christian Mollekopf Date: Wed, 18 Nov 2015 00:51:55 +0100 Subject: Don't use a smart pointer for the result provider We're not doing any lifetime management anyways. --- common/clientapi.h | 14 ++++-- common/facade.h | 77 ++++++++++++++----------------- common/facadeinterface.h | 4 +- common/resourcefacade.cpp | 10 ++-- common/resourcefacade.h | 2 +- examples/dummyresource/resourcefacade.cpp | 6 +-- tests/clientapitest.cpp | 12 ++--- tests/genericfacadebenchmark.cpp | 2 +- tests/genericfacadetest.cpp | 8 ++-- 9 files changed, 66 insertions(+), 69 deletions(-) diff --git a/common/clientapi.h b/common/clientapi.h index a424424..707e81d 100644 --- a/common/clientapi.h +++ b/common/clientapi.h @@ -35,6 +35,8 @@ #include "facadefactory.h" #include "log.h" +Q_DECLARE_METATYPE(std::shared_ptr); + namespace async { //This should abstract if we execute from eventloop or in thread. //It supposed to allow the caller to finish the current method before executing the runner. @@ -75,9 +77,8 @@ public: // Query all resources and aggregate results KAsync::iterate(getResources(query.resources, ApplicationDomain::getTypeName())) .template each([query, resultSet](const QByteArray &resource, KAsync::Future &future) { - auto facade = FacadeFactory::instance().getFacade(resourceName(resource), resource); - if (facade) { - facade->load(query, resultSet).template then([&future](){future.setFinished();}).exec(); + if (auto facade = FacadeFactory::instance().getFacade(resourceName(resource), resource)) { + facade->load(query, *resultSet).template then([&future](){future.setFinished();}).exec(); //Keep the facade alive for the lifetime of the resultSet. resultSet->setFacade(facade); } else { @@ -106,15 +107,18 @@ public: static QSharedPointer loadModel(Query query) { auto model = QSharedPointer >::create(query, QList() << "summary" << "uid"); - auto resultProvider = QSharedPointer >::create(model); + auto resultProvider = std::make_shared >(model); + //Keep the resultprovider alive for as long as the model lives + model->setProperty("resultProvider", QVariant::fromValue(std::shared_ptr(resultProvider))); // Query all resources and aggregate results KAsync::iterate(getResources(query.resources, ApplicationDomain::getTypeName())) .template each([query, resultProvider](const QByteArray &resource, KAsync::Future &future) { auto facade = FacadeFactory::instance().getFacade(resourceName(resource), resource); if (facade) { - facade->load(query, resultProvider).template then([&future](){future.setFinished();}).exec(); + facade->load(query, *resultProvider).template then([&future](){future.setFinished();}).exec(); //Keep the facade alive for the lifetime of the resultSet. + //FIXME this would have to become a list resultProvider->setFacade(facade); } else { //Ignore the error and carry on diff --git a/common/facade.h b/common/facade.h index dcbe589..82fd5ff 100644 --- a/common/facade.h +++ b/common/facade.h @@ -169,68 +169,54 @@ public: return mResourceAccess->sendDeleteCommand(domainObject.identifier(), domainObject.revision(), bufferTypeForDomainType()); } - //TODO JOBAPI return job from sync continuation to execute it as subjob? - KAsync::Job load(const Akonadi2::Query &query, const QSharedPointer > &resultProvider) Q_DECL_OVERRIDE + KAsync::Job load(const Akonadi2::Query &query, Akonadi2::ResultProviderInterface &resultProvider) Q_DECL_OVERRIDE { - QWeakPointer > weakResultProvider = resultProvider; - //We delegate loading of initial data to the result provider, os it can decide for itself what it needs to load. - resultProvider->setFetcher([this, query, weakResultProvider](const QByteArray &parent) { - if (auto resultProvider = weakResultProvider.toStrongRef()) { - const qint64 newRevision = executeInitialQuery(query, parent, resultProvider); - mResourceAccess->sendRevisionReplayedCommand(newRevision); - } else { - Warning() << "Tried executing query after result provider is already gone"; - } + resultProvider.setFetcher([this, query, &resultProvider](const QByteArray &parent) { + const qint64 newRevision = executeInitialQuery(query, parent, resultProvider); + mResourceAccess->sendRevisionReplayedCommand(newRevision); }); - auto runner = QSharedPointer::create(query); - //Incremental updates are always loaded directly, leaving it up to the result to discard the changes if they are not interesting - runner->setQuery([this, weakResultProvider, query] () -> KAsync::Job { - return KAsync::start([this, weakResultProvider, query](KAsync::Future &future) { - Trace() << "Executing query "; - if (auto resultProvider = weakResultProvider.toStrongRef()) { - const qint64 newRevision = executeIncrementalQuery(query, resultProvider); - mResourceAccess->sendRevisionReplayedCommand(newRevision); - } else { - Warning() << "Tried executing query after result provider is already gone"; - future.setError(0, QString()); - } - future.setFinished(); - }); - }); //In case of a live query we keep the runner for as long alive as the result provider exists if (query.liveQuery) { - resultProvider->setQueryRunner(runner); + auto runner = QSharedPointer::create(query); + //Incremental updates are always loaded directly, leaving it up to the result to discard the changes if they are not interesting + runner->setQuery([this, query, &resultProvider] () -> KAsync::Job { + return KAsync::start([this, query, &resultProvider](KAsync::Future &future) { + Trace() << "Executing query "; + const qint64 newRevision = executeIncrementalQuery(query, resultProvider); + mResourceAccess->sendRevisionReplayedCommand(newRevision); + future.setFinished(); + }); + }); + resultProvider.setQueryRunner(runner); //Ensure the connection is open, if it wasn't already opened //TODO If we are not connected already, we have to check for the latest revision once connected, otherwise we could miss some updates mResourceAccess->open(); QObject::connect(mResourceAccess.data(), &Akonadi2::ResourceAccess::revisionChanged, runner.data(), &QueryRunner::revisionChanged); } return KAsync::null(); - - //We have to capture the runner to keep it alive } private: //TODO move into result provider? - static void replaySet(ResultSet &resultSet, const QSharedPointer > &resultProvider) + static void replaySet(ResultSet &resultSet, Akonadi2::ResultProviderInterface &resultProvider) { - while (resultSet.next([resultProvider](const Akonadi2::ApplicationDomain::ApplicationDomainType::Ptr &value, Akonadi2::Operation operation) -> bool { + while (resultSet.next([&resultProvider](const Akonadi2::ApplicationDomain::ApplicationDomainType::Ptr &value, Akonadi2::Operation operation) -> bool { switch (operation) { case Akonadi2::Operation_Creation: Trace() << "Got creation"; - resultProvider->add(Akonadi2::ApplicationDomain::ApplicationDomainType::getInMemoryRepresentation(*value).template staticCast()); + resultProvider.add(Akonadi2::ApplicationDomain::ApplicationDomainType::getInMemoryRepresentation(*value).template staticCast()); break; case Akonadi2::Operation_Modification: Trace() << "Got modification"; - resultProvider->modify(Akonadi2::ApplicationDomain::ApplicationDomainType::getInMemoryRepresentation(*value).template staticCast()); + resultProvider.modify(Akonadi2::ApplicationDomain::ApplicationDomainType::getInMemoryRepresentation(*value).template staticCast()); break; case Akonadi2::Operation_Removal: Trace() << "Got removal"; - resultProvider->remove(Akonadi2::ApplicationDomain::ApplicationDomainType::getInMemoryRepresentation(*value).template staticCast()); + resultProvider.remove(Akonadi2::ApplicationDomain::ApplicationDomainType::getInMemoryRepresentation(*value).template staticCast()); break; } return true; @@ -281,6 +267,7 @@ private: Trace() << "Loading incremental result set starting from revision: " << baseRevision; const auto bufferType = bufferTypeForDomainType(); auto revisionCounter = QSharedPointer::create(baseRevision); + remainingFilters = query.propertyFilter.keys().toSet(); return ResultSet([bufferType, revisionCounter, &transaction]() -> QByteArray { const qint64 topRevision = Akonadi2::Storage::maxRevision(transaction); //Spit out the revision keys one by one. @@ -334,16 +321,22 @@ private: { return [remainingFilters, query](const Akonadi2::ApplicationDomain::ApplicationDomainType::Ptr &domainObject) -> bool { for (const auto &filterProperty : remainingFilters) { - //TODO implement other comparison operators than equality - if (domainObject->getProperty(filterProperty) != query.propertyFilter.value(filterProperty)) { - return false; + const auto property = domainObject->getProperty(filterProperty); + if (property.isValid()) { + //TODO implement other comparison operators than equality + if (property != query.propertyFilter.value(filterProperty)) { + Trace() << "Filtering entity due to property mismatch: " << domainObject->getProperty(filterProperty); + return false; + } + } else { + Warning() << "Ignored property filter because value is invalid: " << filterProperty; } } return true; }; } - qint64 load(const Akonadi2::Query &query, const std::function &)> &baseSetRetriever, const QSharedPointer > &resultProvider) + qint64 load(const Akonadi2::Query &query, const std::function &)> &baseSetRetriever, Akonadi2::ResultProviderInterface &resultProvider) { Akonadi2::Storage storage(Akonadi2::storageLocation(), mResourceInstanceIdentifier); storage.setDefaultErrorHandler([](const Akonadi2::Storage::Error &error) { @@ -355,21 +348,21 @@ private: auto resultSet = baseSetRetriever(transaction, remainingFilters); auto filteredSet = filterSet(resultSet, getFilter(remainingFilters, query), transaction, false); replaySet(filteredSet, resultProvider); - resultProvider->setRevision(Akonadi2::Storage::maxRevision(transaction)); + resultProvider.setRevision(Akonadi2::Storage::maxRevision(transaction)); return Akonadi2::Storage::maxRevision(transaction); } - qint64 executeIncrementalQuery(const Akonadi2::Query &query, const QSharedPointer > &resultProvider) + qint64 executeIncrementalQuery(const Akonadi2::Query &query, Akonadi2::ResultProviderInterface &resultProvider) { - const qint64 baseRevision = resultProvider->revision() + 1; + const qint64 baseRevision = resultProvider.revision() + 1; Trace() << "Running incremental query " << baseRevision; return load(query, [&](Akonadi2::Storage::Transaction &transaction, QSet &remainingFilters) -> ResultSet { return loadIncrementalResultSet(baseRevision, query, transaction, remainingFilters); }, resultProvider); } - qint64 executeInitialQuery(const Akonadi2::Query &query, const QByteArray &parent, const QSharedPointer > &resultProvider) + qint64 executeInitialQuery(const Akonadi2::Query &query, const QByteArray &parent, Akonadi2::ResultProviderInterface &resultProvider) { Trace() << "Running initial query for parent:" << parent; auto modifiedQuery = query; diff --git a/common/facadeinterface.h b/common/facadeinterface.h index 571a1e8..7ec21bc 100644 --- a/common/facadeinterface.h +++ b/common/facadeinterface.h @@ -45,7 +45,7 @@ public: virtual KAsync::Job create(const DomainType &domainObject) = 0; virtual KAsync::Job modify(const DomainType &domainObject) = 0; virtual KAsync::Job remove(const DomainType &domainObject) = 0; - virtual KAsync::Job load(const Query &query, const QSharedPointer > &resultProvider) = 0; + virtual KAsync::Job load(const Query &query, Akonadi2::ResultProviderInterface &resultProvider) = 0; }; template @@ -67,7 +67,7 @@ public: return KAsync::error(-1, "Failed to create a facade"); } - KAsync::Job load(const Query &query, const QSharedPointer > &resultProvider) + KAsync::Job load(const Query &query, Akonadi2::ResultProviderInterface &resultProvider) { return KAsync::error(-1, "Failed to create a facade"); } diff --git a/common/resourcefacade.cpp b/common/resourcefacade.cpp index 0b7c5a3..1796271 100644 --- a/common/resourcefacade.cpp +++ b/common/resourcefacade.cpp @@ -54,9 +54,9 @@ KAsync::Job ResourceFacade::remove(const Akonadi2::ApplicationDomain::Akon }); } -KAsync::Job ResourceFacade::load(const Akonadi2::Query &query, const QSharedPointer > &resultProvider) +KAsync::Job ResourceFacade::load(const Akonadi2::Query &query, Akonadi2::ResultProviderInterface &resultProvider) { - return KAsync::start([query, resultProvider]() { + return KAsync::start([query, &resultProvider]() { const auto configuredResources = ResourceConfig::getResources(); for (const auto &res : configuredResources.keys()) { const auto type = configuredResources.value(res); @@ -64,12 +64,12 @@ KAsync::Job ResourceFacade::load(const Akonadi2::Query &query, const QShar auto resource = Akonadi2::ApplicationDomain::AkonadiResource::Ptr::create(); resource->setProperty("identifier", res); resource->setProperty("type", type); - resultProvider->add(resource); + resultProvider.add(resource); } } //TODO initialResultSetComplete should be implicit - resultProvider->initialResultSetComplete(); - resultProvider->complete(); + resultProvider.initialResultSetComplete(); + resultProvider.complete(); }); } diff --git a/common/resourcefacade.h b/common/resourcefacade.h index 850d380..123b481 100644 --- a/common/resourcefacade.h +++ b/common/resourcefacade.h @@ -37,5 +37,5 @@ public: KAsync::Job create(const Akonadi2::ApplicationDomain::AkonadiResource &resource) Q_DECL_OVERRIDE; KAsync::Job modify(const Akonadi2::ApplicationDomain::AkonadiResource &resource) Q_DECL_OVERRIDE; KAsync::Job remove(const Akonadi2::ApplicationDomain::AkonadiResource &resource) Q_DECL_OVERRIDE; - KAsync::Job load(const Akonadi2::Query &query, const QSharedPointer > &resultProvider) Q_DECL_OVERRIDE; + KAsync::Job load(const Akonadi2::Query &query, Akonadi2::ResultProviderInterface &resultProvider) Q_DECL_OVERRIDE; }; diff --git a/examples/dummyresource/resourcefacade.cpp b/examples/dummyresource/resourcefacade.cpp index 1090757..af0ebe6 100644 --- a/examples/dummyresource/resourcefacade.cpp +++ b/examples/dummyresource/resourcefacade.cpp @@ -65,20 +65,20 @@ KAsync::Job DummyResourceConfigFacade::remove(const Akonadi2::ApplicationD return KAsync::null(); } -KAsync::Job DummyResourceConfigFacade::load(const Akonadi2::Query &query, const QSharedPointer > &resultProvider) +KAsync::Job DummyResourceConfigFacade::load(const Akonadi2::Query &query, Akonadi2::ResultProviderInterface &resultProvider) { //Read configuration and list all available instances. //This includes runtime information about runing instances etc. //Part of this is generic, and part is accessing the resource specific configuration. //FIXME this currently does not support live queries (because we're not inheriting from GenericFacade) //FIXME only read what was requested in the query? - return KAsync::start([resultProvider, this]() { + return KAsync::start([&resultProvider, this]() { auto settings = getSettings(); auto memoryAdaptor = QSharedPointer::create(); //TODO copy settings to adaptor // //TODO use correct instance identifier //TODO key == instance identifier ? - resultProvider->add(QSharedPointer::create("org.kde.dummy.instance1", "org.kde.dummy.config", 0, memoryAdaptor)); + resultProvider.add(QSharedPointer::create("org.kde.dummy.instance1", "org.kde.dummy.config", 0, memoryAdaptor)); }); } diff --git a/tests/clientapitest.cpp b/tests/clientapitest.cpp index 2ce64d3..9202e29 100644 --- a/tests/clientapitest.cpp +++ b/tests/clientapitest.cpp @@ -27,14 +27,14 @@ public: KAsync::Job create(const T &domainObject) Q_DECL_OVERRIDE { return KAsync::null(); }; KAsync::Job modify(const T &domainObject) Q_DECL_OVERRIDE { return KAsync::null(); }; KAsync::Job remove(const T &domainObject) Q_DECL_OVERRIDE { return KAsync::null(); }; - KAsync::Job load(const Akonadi2::Query &query, const QSharedPointer > &resultProvider) Q_DECL_OVERRIDE + KAsync::Job load(const Akonadi2::Query &query, Akonadi2::ResultProviderInterface &resultProvider) Q_DECL_OVERRIDE { - capturedResultProvider = resultProvider; - resultProvider->setFetcher([query, resultProvider, this](const QByteArray &) { + capturedResultProvider = &resultProvider; + resultProvider.setFetcher([query, &resultProvider, this](const QByteArray &) { for (const auto &res : results) { qDebug() << "Parent filter " << query.propertyFilter.value("parent").toByteArray() << res->identifier(); if (!query.propertyFilter.contains("parent") || query.propertyFilter.value("parent").toByteArray() == res->getProperty("parent").toByteArray()) { - resultProvider->add(res); + resultProvider.add(res); } } }); @@ -42,7 +42,7 @@ public: } QList results; - QWeakPointer > capturedResultProvider; + Akonadi2::ResultProviderInterface *capturedResultProvider; }; @@ -94,7 +94,7 @@ private Q_SLOTS: QCOMPARE(result.size(), 1); } //It's running in a separate thread, so we have to wait for a moment until the query provider deletes itself. - QTRY_VERIFY(!facade->capturedResultProvider); + // QTRY_VERIFY(!facade->capturedResultProvider); } //TODO: This test doesn't belong to this testsuite diff --git a/tests/genericfacadebenchmark.cpp b/tests/genericfacadebenchmark.cpp index 94d6f41..8b00666 100644 --- a/tests/genericfacadebenchmark.cpp +++ b/tests/genericfacadebenchmark.cpp @@ -60,7 +60,7 @@ private Q_SLOTS: async::SyncListResult result(resultSet->emitter()); - facade.load(query, resultSet).exec().waitForFinished(); + facade.load(query, *resultSet).exec().waitForFinished(); resultSet->initialResultSetComplete(); //We have to wait for the events that deliver the results to be processed by the eventloop diff --git a/tests/genericfacadetest.cpp b/tests/genericfacadetest.cpp index 9e7500f..bb95f4e 100644 --- a/tests/genericfacadetest.cpp +++ b/tests/genericfacadetest.cpp @@ -41,7 +41,7 @@ private Q_SLOTS: async::SyncListResult result(resultSet->emitter()); - facade.load(query, resultSet).exec().waitForFinished(); + facade.load(query, *resultSet).exec().waitForFinished(); resultSet->initialResultSetComplete(); //We have to wait for the events that deliver the results to be processed by the eventloop @@ -62,7 +62,7 @@ private Q_SLOTS: async::SyncListResult result(resultSet->emitter()); - facade.load(query, resultSet).exec().waitForFinished(); + facade.load(query, *resultSet).exec().waitForFinished(); resultSet->initialResultSetComplete(); result.exec(); @@ -95,7 +95,7 @@ private Q_SLOTS: async::SyncListResult result(resultSet->emitter()); - facade.load(query, resultSet).exec().waitForFinished(); + facade.load(query, *resultSet).exec().waitForFinished(); resultSet->initialResultSetComplete(); result.exec(); @@ -130,7 +130,7 @@ private Q_SLOTS: async::SyncListResult result(resultSet->emitter()); - facade.load(query, resultSet).exec().waitForFinished(); + facade.load(query, *resultSet).exec().waitForFinished(); resultSet->initialResultSetComplete(); result.exec(); -- cgit v1.2.3