Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ControlProxy improvements #4224

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/control/controlobjectscript.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ bool ControlObjectScript::addScriptConnection(const ScriptConnection& conn) {
&ControlObjectScript::slotValueChanged,
Qt::QueuedConnection);
connect(this,
&ControlObjectScript::trigger,
&ControlObjectScript::triggered,
this,
&ControlObjectScript::slotValueChanged,
Qt::QueuedConnection);
Expand Down Expand Up @@ -60,7 +60,7 @@ bool ControlObjectScript::removeScriptConnection(const ScriptConnection& conn) {
this,
&ControlObjectScript::slotValueChanged);
disconnect(this,
&ControlObjectScript::trigger,
&ControlObjectScript::triggered,
this,
&ControlObjectScript::slotValueChanged);
}
Expand Down
8 changes: 4 additions & 4 deletions src/control/controlobjectscript.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,14 @@ class ControlObjectScript : public ControlProxy {
return m_scriptConnections.first(); };
void disconnectAllConnectionsToFunction(const QJSValue& function);

// Called from update();
void emitValueChanged() override {
emit trigger(get(), this);
/// Called from `ControlEngineScriptLegacy::trigger`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this comment then, because it is confusing. It's a public method that is accessible by anyone.

void trigger() {
emit triggered(get(), this);
}

signals:
// It will connect to the slotValueChanged as well
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated, but I don't understand this comment.

void trigger(double, QObject*);
void triggered(double, QObject*);

protected slots:
// Receives the value from the master control by a unique queued connection
Expand Down
50 changes: 8 additions & 42 deletions src/control/controlproxy.h
Original file line number Diff line number Diff line change
Expand Up @@ -86,33 +86,14 @@ class ControlProxy : public QObject {
Qt::ConnectionType copConnection = static_cast<Qt::ConnectionType>(
requestedConnectionType | Qt::UniqueConnection);

// clazy requires us to to pass a member function to connect() directly
// (i.e. w/o and intermediate variable) when used with
// Qt::UniqueConnection. Otherwise it detects a false positive and
// throws a [-Wclazy-lambda-unique-connection] warning.
switch (requestedConnectionType) {
case Qt::AutoConnection:
connect(m_pControl.data(), &ControlDoublePrivate::valueChanged, this, &ControlProxy::slotValueChangedAuto, copConnection);
break;
case Qt::DirectConnection:
connect(m_pControl.data(), &ControlDoublePrivate::valueChanged, this, &ControlProxy::slotValueChangedDirect, copConnection);
break;
case Qt::QueuedConnection:
connect(m_pControl.data(), &ControlDoublePrivate::valueChanged, this, &ControlProxy::slotValueChangedQueued, copConnection);
break;
default:
// Should be unreachable, but just to make sure ;-)
DEBUG_ASSERT(false);
return false;
}
connect(m_pControl.data(),
&ControlDoublePrivate::valueChanged,
this,
&ControlProxy::slotValueChanged,
copConnection);
return true;
}

/// Called from update();
virtual void emitValueChanged() {
emit valueChanged(get());
}

inline bool valid() const {
return m_pControl != nullptr;
}
Expand Down Expand Up @@ -173,24 +154,9 @@ class ControlProxy : public QObject {
void valueChanged(double);

protected slots:
/// Receives the value from the master control by a unique direct connection
void slotValueChangedDirect(double v, QObject* pSetter) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The different equal functions are there for the unique direct connection.

When one consumer is a in the same thread and one else is in another thread, we need two jump in addresses to maintain the unique connection. This was at least required when writing the code.

Did you check how Qt handles the situation today? If you are not certain we should keep the code as it is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't notice the Qt::UniqueConnection flag. The documentation of this hacky code is insufficient. Unmaintainable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this again, this code has been written having a single Proxy per Control in mind.
Today we have in many cases multiple proxies one for each connection, where this hacky code is not required.

Unfortunately it bypasses the quinine connection Flag. Two proxies in the same thread will lead to two queued up signals and two semaphores locked (if the qt code is kept the same since I have last looked into it)

Options:

  • leave this alone and revert to the single functions
  • dive into the qt code and clarify the situation
  • Introduce a less hacky solution that has another proxy layer one, one per receiving thread.

Maybe we can utilize a QThreadStorage for this ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All has started with this bug: https://bugs.launchpad.net/mixxx/+bug/1406124

if (pSetter != this) {
// This is base implementation of this function without scaling
emit valueChanged(v);
}
}

/// Receives the value from the master control by a unique auto connection
void slotValueChangedAuto(double v, QObject* pSetter) {
if (pSetter != this) {
// This is base implementation of this function without scaling
emit valueChanged(v);
}
}

/// Receives the value from the master control by a unique Queued connection
void slotValueChangedQueued(double v, QObject* pSetter) {
/// Receives the value from the underlying control and emits the
/// `valueChanged` signal
void slotValueChanged(double v, QObject* pSetter) {
if (pSetter != this) {
// This is base implementation of this function without scaling
emit valueChanged(v);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ QJSValue ControllerScriptInterfaceLegacy::connectControl(const QString& group,
void ControllerScriptInterfaceLegacy::trigger(const QString& group, const QString& name) {
ControlObjectScript* coScript = getControlObjectScript(group, name);
if (coScript != nullptr) {
coScript->emitValueChanged();
coScript->trigger();
}
}

Expand Down