From 2273fc05598d84411782b52e6ede86b2cd7c804c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Gr=C3=A4=C3=9Flin?= Date: Tue, 26 Apr 2016 14:55:47 +0200 Subject: [PATCH] Cancel the EffectLoader query on clear The Scripted and PluginEffectLoader perform locating all effects which are to be loaded in a thread. When the EffectLoader gets cleared so far the query did not get canceled. This resulted in effects maybe getting loaded. This problems shows on build.kde.org if the test is too fast and tears down the Effect system while effects are still being queried. Reviewed-By: David Edmundson --- autotests/test_plugin_effectloader.cpp | 28 ++++++++++++++++++++++++ autotests/test_scripted_effectloader.cpp | 28 ++++++++++++++++++++++++ effectloader.cpp | 18 +++++++++++---- effectloader.h | 2 ++ 4 files changed, 72 insertions(+), 4 deletions(-) diff --git a/autotests/test_plugin_effectloader.cpp b/autotests/test_plugin_effectloader.cpp index 50cb589b84..df6e5f4ebd 100644 --- a/autotests/test_plugin_effectloader.cpp +++ b/autotests/test_plugin_effectloader.cpp @@ -58,6 +58,7 @@ private Q_SLOTS: void testLoadPluginEffect_data(); void testLoadPluginEffect(); void testLoadAllEffects(); + void testCancelLoadAllEffects(); }; void TestPluginEffectLoader::testHasEffect_data() @@ -381,5 +382,32 @@ void TestPluginEffectLoader::testLoadAllEffects() spy.clear(); } +void TestPluginEffectLoader::testCancelLoadAllEffects() +{ + // this test verifies that no test gets loaded when the loader gets cleared + MockEffectsHandler mockHandler(KWin::OpenGL2Compositing); + KWin::PluginEffectLoader loader; + loader.setPluginSubDirectory(QString()); + + // prepare the configuration to hard enable/disable the effects we want to load + KSharedConfig::Ptr config = KSharedConfig::openConfig(QString(), KConfig::SimpleConfig); + KConfigGroup plugins = config->group("Plugins"); + plugins.writeEntry(QStringLiteral("fakeeffectpluginEnabled"), true); + plugins.sync(); + + loader.setConfig(config); + + qRegisterMetaType(); + QSignalSpy spy(&loader, &KWin::PluginEffectLoader::effectLoaded); + QVERIFY(spy.isValid()); + + loader.queryAndLoadAll(); + loader.clear(); + + // Should not load any effect + QVERIFY(!spy.wait(100)); + QVERIFY(spy.isEmpty()); +} + QTEST_MAIN(TestPluginEffectLoader) #include "test_plugin_effectloader.moc" diff --git a/autotests/test_scripted_effectloader.cpp b/autotests/test_scripted_effectloader.cpp index 38d77cb2b6..fee8e014c1 100644 --- a/autotests/test_scripted_effectloader.cpp +++ b/autotests/test_scripted_effectloader.cpp @@ -72,6 +72,7 @@ private Q_SLOTS: void testLoadScriptedEffect_data(); void testLoadScriptedEffect(); void testLoadAllEffects(); + void testCancelLoadAllEffects(); }; void TestScriptedEffectLoader::testHasEffect_data() @@ -387,5 +388,32 @@ void TestScriptedEffectLoader::testLoadAllEffects() QCOMPARE(loadedEffects.at(1), kwin4 + QStringLiteral("scalein")); } +void TestScriptedEffectLoader::testCancelLoadAllEffects() +{ + // this test verifies that no test gets loaded when the loader gets cleared + MockEffectsHandler mockHandler(KWin::XRenderCompositing); + KWin::ScriptedEffectLoader loader; + + // prepare the configuration to hard enable/disable the effects we want to load + KSharedConfig::Ptr config = KSharedConfig::openConfig(QString(), KConfig::SimpleConfig); + const QString kwin4 = QStringLiteral("kwin4_effect_"); + KConfigGroup plugins = config->group("Plugins"); + plugins.writeEntry(kwin4 + QStringLiteral("scaleinEnabled"), true); + plugins.sync(); + + loader.setConfig(config); + + qRegisterMetaType(); + QSignalSpy spy(&loader, &KWin::ScriptedEffectLoader::effectLoaded); + QVERIFY(spy.isValid()); + + loader.queryAndLoadAll(); + loader.clear(); + + // Should not load any effect + QVERIFY(!spy.wait(100)); + QVERIFY(spy.isEmpty()); +} + QTEST_MAIN(TestScriptedEffectLoader) #include "test_scripted_effectloader.moc" diff --git a/effectloader.cpp b/effectloader.cpp index ee85ac43e3..030aa21346 100644 --- a/effectloader.cpp +++ b/effectloader.cpp @@ -258,9 +258,12 @@ bool ScriptedEffectLoader::loadEffect(const KPluginMetaData &effect, LoadEffectF void ScriptedEffectLoader::queryAndLoadAll() { + if (m_queryConnection) { + return; + } // perform querying for the services in a thread QFutureWatcher> *watcher = new QFutureWatcher>(this); - connect(watcher, &QFutureWatcher>::finished, this, + m_queryConnection = connect(watcher, &QFutureWatcher>::finished, this, [this, watcher]() { const auto effects = watcher->result(); for (auto effect : effects) { @@ -270,6 +273,7 @@ void ScriptedEffectLoader::queryAndLoadAll() } } watcher->deleteLater(); + m_queryConnection = QMetaObject::Connection(); }, Qt::QueuedConnection); watcher->setFuture(QtConcurrent::run(this, &ScriptedEffectLoader::findAllEffects)); @@ -296,7 +300,8 @@ KPluginMetaData ScriptedEffectLoader::findEffect(const QString &name) const void ScriptedEffectLoader::clear() { - // TODO: cancel future + disconnect(m_queryConnection); + m_queryConnection = QMetaObject::Connection(); m_queue->clear(); } @@ -432,9 +437,12 @@ bool PluginEffectLoader::loadEffect(const KPluginMetaData &info, LoadEffectFlags void PluginEffectLoader::queryAndLoadAll() { + if (m_queryConnection) { + return; + } // perform querying for the services in a thread QFutureWatcher> *watcher = new QFutureWatcher>(this); - connect(watcher, &QFutureWatcher>::finished, this, + m_queryConnection = connect(watcher, &QFutureWatcher>::finished, this, [this, watcher]() { const auto effects = watcher->result(); for (const auto &effect : effects) { @@ -444,6 +452,7 @@ void PluginEffectLoader::queryAndLoadAll() } } watcher->deleteLater(); + m_queryConnection = QMetaObject::Connection(); }, Qt::QueuedConnection); watcher->setFuture(QtConcurrent::run(this, &PluginEffectLoader::findAllEffects)); @@ -461,7 +470,8 @@ void PluginEffectLoader::setPluginSubDirectory(const QString &directory) void PluginEffectLoader::clear() { - // TODO: cancel future + disconnect(m_queryConnection); + m_queryConnection = QMetaObject::Connection(); m_queue->clear(); } diff --git a/effectloader.h b/effectloader.h index 38ca86a26a..b33876ad71 100644 --- a/effectloader.h +++ b/effectloader.h @@ -323,6 +323,7 @@ private: KPluginMetaData findEffect(const QString &name) const; QStringList m_loadedEffects; EffectLoadQueue< ScriptedEffectLoader, KPluginMetaData > *m_queue; + QMetaObject::Connection m_queryConnection; }; class PluginEffectLoader : public AbstractEffectLoader @@ -350,6 +351,7 @@ private: QStringList m_loadedEffects; EffectLoadQueue< PluginEffectLoader, KPluginMetaData> *m_queue; QString m_pluginSubDirectory; + QMetaObject::Connection m_queryConnection; }; class EffectLoader : public AbstractEffectLoader