ControlProxy improvements #4224
Conversation
Pull Request Test Coverage Report for Build 1144431471
💛 - Coveralls |
src/control/controlobjectscript.h
Outdated
| // Called from update(); | ||
| void emitValueChanged() override { | ||
| /// Called from `ControlEngineScriptLegacy::trigger` | ||
| void emitValueChanged() { |
There was a problem hiding this comment.
Rename to emitTrigger() while we are at it?
There was a problem hiding this comment.
Should this become a private method and ControlEngineScriptLegacy a friend class then?
There was a problem hiding this comment.
Should this become a private method and ControlEngineScriptLegacy a friend class then?
Hmm, I don't see a big problem with this method being part of the public interface. It may as well be used by the future controller engine implementation (and this class is only used in controller engines anyway).
There was a problem hiding this comment.
Then we should not mention by whom it is used in a comment, because public methods are supposed to be used by anyone.
| emit trigger(get(), this); | ||
| /// Called from `ControlEngineScriptLegacy::trigger` | ||
| void trigger() { | ||
| emit triggered(get(), this); | ||
| } | ||
|
|
||
| signals: | ||
| // It will connect to the slotValueChanged as well |
There was a problem hiding this comment.
Unrelated, but I don't understand this comment.
| // Called from update(); | ||
| void emitValueChanged() override { | ||
| emit trigger(get(), this); | ||
| /// Called from `ControlEngineScriptLegacy::trigger` |
There was a problem hiding this comment.
Please remove this comment then, because it is confusing. It's a public method that is accessible by anyone.
| @@ -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) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I didn't notice the Qt::UniqueConnection flag. The documentation of this hacky code is insufficient. Unmaintainable.
There was a problem hiding this comment.
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 ...
There was a problem hiding this comment.
All has started with this bug: https://bugs.launchpad.net/mixxx/+bug/1406124
|
The situation has not changed up to the current "dev" version of Qt The uniqueness of the connection is verified by sender and receiver only, without comparing the connection type. There is room fro improvements, after analyzing all use cases of ControlProxy and having the whole picture carved out. While this PR only claims to clean up the code, we have a real issue to solve. This is the unverified use of pain pointers in case of direct connections from inside Qt. My original plan was to fix this and than finally add a feature that swaps out all engine data after the end of the audio call back at once. I would be happy to revive them. |
No description provided.