From aba77306b61475a92c6ffeaad4e51c286cfebaff Mon Sep 17 00:00:00 2001 From: JoergAtGithub Date: Mon, 20 Dec 2021 19:46:40 +0100 Subject: [PATCH] Made CompressingProxy by default disabled. Now you need to specify the additional boolean argument skipSuperseded as true, to enable it. In case of multiple callback functions connected to the same CO, with contradicting state for skipSuperseded, the CompressingProxy will be disabled and an warning message is printed. --- src/control/controlobjectscript.cpp | 76 ++++++++++++----- src/control/controlobjectscript.h | 1 + .../controllerscriptinterfacelegacy.cpp | 3 +- .../legacy/controllerscriptinterfacelegacy.h | 2 +- .../scripting/legacy/scriptconnection.h | 1 + src/test/controlobjectscripttest.cpp | 82 ++++++++++++++++++- 6 files changed, 142 insertions(+), 23 deletions(-) diff --git a/src/control/controlobjectscript.cpp b/src/control/controlobjectscript.cpp index 8320584fdb29..d83c07481336 100644 --- a/src/control/controlobjectscript.cpp +++ b/src/control/controlobjectscript.cpp @@ -6,28 +6,59 @@ ControlObjectScript::ControlObjectScript( const ConfigKey& key, const RuntimeLoggingCategory& logger, QObject* pParent) : ControlProxy(key, pParent, ControlFlag::AllowMissingOrInvalid), m_logger(logger), - m_proxy(key, logger, this) { + m_proxy(key, logger, this), + m_skipSuperseded(false) { } bool ControlObjectScript::addScriptConnection(const ScriptConnection& conn) { if (m_scriptConnections.isEmpty()) { // Only connect the slots when they are actually needed // by script connections. - connect(m_pControl.data(), - &ControlDoublePrivate::valueChanged, - &m_proxy, - &CompressingProxy::slotValueChanged, - Qt::QueuedConnection); - connect(&m_proxy, - &CompressingProxy::signalValueChanged, - this, - &ControlObjectScript::slotValueChanged, - Qt::DirectConnection); + m_skipSuperseded = conn.skipSuperseded; + if (conn.skipSuperseded) { + connect(m_pControl.data(), + &ControlDoublePrivate::valueChanged, + &m_proxy, + &CompressingProxy::slotValueChanged, + Qt::QueuedConnection); + connect(&m_proxy, + &CompressingProxy::signalValueChanged, + this, + &ControlObjectScript::slotValueChanged, + Qt::DirectConnection); + } else { + connect(m_pControl.data(), + &ControlDoublePrivate::valueChanged, + this, + &ControlObjectScript::slotValueChanged, + Qt::QueuedConnection); + } connect(this, &ControlObjectScript::trigger, this, &ControlObjectScript::slotValueChanged, Qt::QueuedConnection); + } else { + // At least one callback function is already connected to this CO + if (conn.skipSuperseded == false && m_skipSuperseded == true) { + // Disconnect proxy if this is first callback function connected with skipSuperseded false + m_skipSuperseded = false; + qCWarning(m_logger) << conn.key.group + ", " + conn.key.item + + "is connected to different callback functions with differing state of the skipSuperseded. Disable skipping of superseded events for all these callback functions."; + disconnect(m_pControl.data(), + &ControlDoublePrivate::valueChanged, + &m_proxy, + &CompressingProxy::slotValueChanged); + disconnect(&m_proxy, + &CompressingProxy::signalValueChanged, + this, + &ControlObjectScript::slotValueChanged); + connect(m_pControl.data(), + &ControlDoublePrivate::valueChanged, + this, + &ControlObjectScript::slotValueChanged, + Qt::QueuedConnection); + } } for (const auto& priorConnection : qAsConst(m_scriptConnections)) { @@ -60,14 +91,21 @@ bool ControlObjectScript::removeScriptConnection(const ScriptConnection& conn) { } if (m_scriptConnections.isEmpty()) { // no ScriptConnections left, so disconnect signals - disconnect(m_pControl.data(), - &ControlDoublePrivate::valueChanged, - &m_proxy, - &CompressingProxy::slotValueChanged); - disconnect(&m_proxy, - &CompressingProxy::signalValueChanged, - this, - &ControlObjectScript::slotValueChanged); + if (m_skipSuperseded) { + disconnect(m_pControl.data(), + &ControlDoublePrivate::valueChanged, + &m_proxy, + &CompressingProxy::slotValueChanged); + disconnect(&m_proxy, + &CompressingProxy::signalValueChanged, + this, + &ControlObjectScript::slotValueChanged); + } else { + disconnect(m_pControl.data(), + &ControlDoublePrivate::valueChanged, + this, + &ControlObjectScript::slotValueChanged); + } disconnect(this, &ControlObjectScript::trigger, this, diff --git a/src/control/controlobjectscript.h b/src/control/controlobjectscript.h index 4fb24f63cd77..01525ef71b45 100644 --- a/src/control/controlobjectscript.h +++ b/src/control/controlobjectscript.h @@ -44,4 +44,5 @@ class ControlObjectScript : public ControlProxy { QVector m_scriptConnections; const RuntimeLoggingCategory m_logger; CompressingProxy m_proxy; + bool m_skipSuperseded; // This flag is combined for all connections of this Control Object }; diff --git a/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp b/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp index 28aa760d4eeb..733da12049e2 100644 --- a/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp +++ b/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp @@ -212,7 +212,7 @@ double ControllerScriptInterfaceLegacy::getDefaultParameter( } QJSValue ControllerScriptInterfaceLegacy::makeConnection( - const QString& group, const QString& name, const QJSValue& callback) { + const QString& group, const QString& name, const QJSValue& callback, bool skipSuperseded) { auto pJsEngine = m_pScriptEngineLegacy->jsEngine(); VERIFY_OR_DEBUG_ASSERT(pJsEngine) { return QJSValue(); @@ -245,6 +245,7 @@ QJSValue ControllerScriptInterfaceLegacy::makeConnection( connection.controllerEngine = m_pScriptEngineLegacy; connection.callback = callback; connection.id = QUuid::createUuid(); + connection.skipSuperseded = skipSuperseded; if (coScript->addScriptConnection(connection)) { return pJsEngine->newQObject( diff --git a/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.h b/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.h index afeb8914a5bd..da7b988ae048 100644 --- a/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.h +++ b/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.h @@ -32,7 +32,7 @@ class ControllerScriptInterfaceLegacy : public QObject { Q_INVOKABLE double getDefaultValue(const QString& group, const QString& name); Q_INVOKABLE double getDefaultParameter(const QString& group, const QString& name); Q_INVOKABLE QJSValue makeConnection( - const QString& group, const QString& name, const QJSValue& callback); + const QString& group, const QString& name, const QJSValue& callback, bool skipSuperseded = false); // DEPRECATED: Use makeConnection instead. Q_INVOKABLE QJSValue connectControl(const QString& group, const QString& name, diff --git a/src/controllers/scripting/legacy/scriptconnection.h b/src/controllers/scripting/legacy/scriptconnection.h index f2e6558e64bb..a6f607e66a73 100644 --- a/src/controllers/scripting/legacy/scriptconnection.h +++ b/src/controllers/scripting/legacy/scriptconnection.h @@ -18,6 +18,7 @@ class ScriptConnection { QJSValue callback; ControllerScriptInterfaceLegacy* engineJSProxy; ControllerScriptEngineLegacy* controllerEngine; + bool skipSuperseded; void executeCallback(double value) const; diff --git a/src/test/controlobjectscripttest.cpp b/src/test/controlobjectscripttest.cpp index 4d506ecf70c2..8351e5623155 100644 --- a/src/test/controlobjectscripttest.cpp +++ b/src/test/controlobjectscripttest.cpp @@ -35,47 +35,64 @@ class ControlObjectScriptTest : public MixxxTest { void SetUp() override { ck1 = ConfigKey("[Channel1]", "co1"); ck2 = ConfigKey("[Channel1]", "co2"); + ck4 = ConfigKey("[Channel1]", "co4"); co1 = std::make_unique(ck1); co2 = std::make_unique(ck2); + co4 = std::make_unique(ck4); coScript1 = std::make_unique(ck1, k_logger, nullptr); coScript2 = std::make_unique(ck2, k_logger, nullptr); + coScript4 = std::make_unique(ck4, k_logger, nullptr); conn1.key = ck1; conn1.engineJSProxy = nullptr; conn1.controllerEngine = nullptr; conn1.callback = "mock_callback1"; conn1.id = QUuid::createUuid(); + conn1.skipSuperseded = true; conn2.key = ck2; conn2.engineJSProxy = nullptr; conn2.controllerEngine = nullptr; conn2.callback = "mock_callback2"; conn2.id = QUuid::createUuid(); + conn2.skipSuperseded = true; conn3.key = ck2; conn3.engineJSProxy = nullptr; conn3.controllerEngine = nullptr; conn3.callback = "mock_callback3"; conn3.id = QUuid::createUuid(); + conn3.skipSuperseded = true; + + conn4.key = ck1; + conn4.engineJSProxy = nullptr; + conn4.controllerEngine = nullptr; + conn4.callback = "mock_callback1"; + conn4.id = QUuid::createUuid(); + conn4.skipSuperseded = false; coScript1->addScriptConnection(conn1); coScript2->addScriptConnection(conn2); + coScript4->addScriptConnection(conn4); } void TearDown() override { coScript1->removeScriptConnection(conn1); coScript2->removeScriptConnection(conn2); + coScript4->removeScriptConnection(conn4); } - ConfigKey ck1, ck2; + ConfigKey ck1, ck2, ck4; std::unique_ptr co1; std::unique_ptr co2; + std::unique_ptr co4; std::unique_ptr coScript1; std::unique_ptr coScript2; + std::unique_ptr coScript4; - ScriptConnection conn1, conn2, conn3; + ScriptConnection conn1, conn2, conn3, conn4; }; TEST_F(ControlObjectScriptTest, CompressingProxyCompareCount1) { @@ -124,6 +141,32 @@ TEST_F(ControlObjectScriptTest, CompressingProxyCompareValue2) { application()->processEvents(); } +TEST_F(ControlObjectScriptTest, QueuedCompareCount2) { + // Check that slotValueChanged callback is never called for conn4 + EXPECT_CALL(*coScript2, slotValueChanged(_, _)) + .Times(0) + .WillOnce(Return()); + // Check that slotValueChanged callback for conn1 is called only twice (independent of the value), because proxy is disabled + EXPECT_CALL(*coScript4, slotValueChanged(_, _)) + .Times(2) + .WillOnce(Return()); + co4->set(53.0); + co4->set(54.0); + application()->processEvents(); +} + +TEST_F(ControlObjectScriptTest, QueuedCompareValue2) { + // Check that slotValueChanged callback is called for each value, because proxy is disabled + EXPECT_CALL(*coScript4, slotValueChanged(55.0, _)) + .Times(1) + .WillOnce(Return()); + EXPECT_CALL(*coScript4, slotValueChanged(56.0, _)) + .Times(1) + .WillOnce(Return()); + co4->set(55.0); + co4->set(56.0); + application()->processEvents(); +} TEST_F(ControlObjectScriptTest, CompressingProxyCompareCountMulti) { // Check that slotValueChanged callback for conn1 and conn2 is called only once (independent of the value) EXPECT_CALL(*coScript1, slotValueChanged(_, _)).Times(1).WillOnce(Return()); @@ -170,6 +213,41 @@ TEST_F(ControlObjectScriptTest, CompressingProxyMultiConnection) { coScript2->removeScriptConnection(conn3); } +TEST_F(ControlObjectScriptTest, QueuedFallbackMultiConnection) { + // Check that slotValueChanged callback is called 1 time if multiple connections exist forthe same slot + EXPECT_CALL(*coScript1, slotValueChanged(62.0, _)) + .Times(1) + .WillOnce(Return()); + EXPECT_CALL(*coScript2, slotValueChanged(63.0, _)) + .Times(1) + .WillOnce(Return()); + EXPECT_CALL(*coScript1, slotValueChanged(66.0, _)) + .Times(1) + .WillOnce(Return()); + EXPECT_CALL(*coScript2, slotValueChanged(65.0, _)) + .Times(1) + .WillOnce(Return()); + EXPECT_CALL(*coScript2, slotValueChanged(67.0, _)) + .Times(1) + .WillOnce(Return()); + + co1->set(60.0); + co2->set(61.0); + co1->set(62.0); + co2->set(63.0); + application()->processEvents(); + conn3.skipSuperseded = false; + coScript2->addScriptConnection(conn3); + co1->set(64.0); + co2->set(65.0); + co1->set(66.0); + co2->set(67.0); + application()->processEvents(); + + coScript2->removeScriptConnection(conn3); + conn3.skipSuperseded = true; +} + TEST_F(ControlObjectScriptTest, CompressingProxyManyEvents) { // Check maximum number of recursions EXPECT_CALL(*coScript1, slotValueChanged(kMaxNumOfRecursions, _))