simplify ControllerEngine::connectControl #656

Be-ing · 2015-07-20T01:31:14Z

also make debugging and error messages more descriptive

This fixes https://bugs.launchpad.net/mixxx/+bug/1458264:
engine.connectionControl(group, control, function, true) connects control to a function rather than removes connection)

I tried writing a test to demonstrate the above bug but couldn't get the test to fail (on a branch without this commit of course). I did see that the bug was there because the test output showed the connection being made twice, but the logic of the test did not fail when this happened.

…nnectControl also make debugging and error messages more descriptive This fixes https://bugs.launchpad.net/mixxx/+bug/1458264 ('engine.connectionControl(group, control, function, true) connects control to a function rather than removes connection')

Be-ing · 2015-07-20T01:47:55Z

I noticed that the tests use a syntax that is not documented on the wiki (http://mixxx.org/wiki/doku.php/midi_scripting#automatic_reactions_to_changes_in_mixxx ):

        "    connection = engine.connectControl('[Test]', 'potmeter', \n"
        "                            'testConnectDisconnectControlCallback');\n"

and

        "    connection.disconnect();\n"

As far as I can tell by grepping for '.disconnect(' in the source, only one mapping, the one for the Denon MC6000Mk2, users this convention and only on one line. Why is this available if it is not documented? Should the tests be modified to reflect the documentation, that is using engine.connectControl with true as the last argument? Should this feature be removed to simply controllerengine.cpp? Or should the wiki be updated to mention this? I think this feature is confusing because the wiki says connectControl should return true or false.

daschuer · 2015-07-20T05:33:14Z

src/controllers/controllerengine.cpp

-    QScriptValue function;
+    ControllerEngineConnection conn;
+    conn.key = ConfigKey(group, name);
+    conn.ce = this;


can you rename this to something more significant like "conn.parent = this" or similar.

daschuer · 2015-07-20T06:18:17Z

Thank you for the patch.
You are right, the disconnect() function is somehow redundant. It is a bit faster but requires storing the connection inside the script. In any case it is not recommend to use connect and reconnect in time critical loops since connect and reconnect is a locking inside QT.
What is the use case for disconnect a CO?

Be-ing · 2015-07-20T06:21:25Z

What is the use case for disconnect a CO?

Do you mean the disconnect() function specifically or disconnecting a CO from a script function in general? Disconnecting COs is essential for deck toggle buttons and programming different layers.

daschuer · 2015-07-20T06:38:36Z

I have not analyses what bad can happed due to the Qt locking. But in any case our connect and disconnect functions are not light weight. A a common dispatcher callback that calls the active deck callbacks would be a better solution.

We should offer a library that includes a deck toggle layer that does not require connect and disconnect.
But that is an other story.

For now I think we should keep disconnect and update the doku since this will keep the API unchanged.

Be-ing · 2015-07-20T06:52:57Z

In any case it is not recommend to use connect and reconnect in time critical loops since connect and reconnect is a locking inside QT.

I can't think of a use case that would require (dis)connecting controls in a time critical loop, but it would be good to make a note of that on the wiki.

We should offer a library that includes a deck toggle layer that does not require connect and disconnect.

Yes, and I think the whole mapping system could really use an overhaul.

But that is an other story

Agreed.

For now I think we should keep disconnect and update the doku since this will keep the API unchanged.

I would rather remove this almost unused and confusingly redundant functionality than document it.
@uklotzde , what is your opinion on that?

daschuer · 2015-07-20T10:18:56Z

IMHO the original bug should be fixed in 1.12. The current approach from this PR suits to this.
Keeping disconnect hidden is also Ok for 1.12

There should nothing against streamline this issue for master.

Thoughts?

Be-ing · 2015-07-20T15:24:35Z

This bug is not critical, it only affects script writers, and it is trivial to work around. I think the best fix for it is reorganizing the connectControl function, but I am also concerned that this could introduce regressions.

uklotzde · 2015-07-20T15:43:00Z

The invocation of ".disconnect(" you found in my MC6000MK2 script is a
function of a custom type that I've defined for convenience. The function
DenonMC6000MK2.Control.prototype.disconnect simply calls
engine.connectControl(..., true) internally.

On 07/20/2015 08:54 AM, Be-ing wrote:

In any case it is not recommend to use connect and reconnect in time
critical loops since connect and reconnect is a locking inside QT.
I can't think of a use case that would require (dis)connecting controls in
a time critical loop, but it would be good to make a note of that on the wiki.
We should offer a library that includes a deck toggle layer that does
not require connect and disconnect.
Yes, and I think the whole mapping system could really use an overhaul.
But that is an other story
Agreed.

For now I think we should keep disconnect and update the doku since
this will keep the API unchanged.
I would rather remove this almost entirely unused and confusingly
redundant functionality than document it.
@uklotzde https://github.com/uklotzde , what is your opinion on that?

—
Reply to this email directly or view it on GitHub
#656 (comment).

Be-ing · 2015-07-20T15:48:12Z

Well then, I'm definitely in favor of removing the .disconnect() function and changing the tests to test the method that is actually documented on the wiki and used by scripts.

Be-ing · 2015-07-20T16:20:52Z

@pwhelan , care to comment?

Be-ing · 2015-07-20T16:35:27Z

https://github.com/Be-ing/mixxx/blob/script_disconnect_control/src/controllers/controllerengine.cpp#L813

I'm not clear about the purpose of this clause. When would the callback be a QObject? Should this clause be removed?

daschuer · 2015-07-20T20:59:51Z

Will be OK for me. Do we have conclusion about the target branch?

daschuer · 2015-07-20T21:07:41Z

I'm not clear about the purpose of this clause ...

This should be a short cut to re-connect a stored "conn" object.
If you change the return value to boolean, this condition is useless.

simplify ControllerEngine::connectControl improve debug messages in ControllerEngine::connectControl consolidate ControllerEngine::disconnectControl into ControllerEngine::connectControl remove ControllerEngine::disconnect

Be-ing · 2015-07-20T22:34:20Z

Will be OK for me. Do we have conclusion about the target branch?

Now that it isn't overcomplicated and I understand what it's doing, I'd be more comfortable merging it into 1.12.

With that functionality removed, the tests need to be updated before this is merged.

pwhelan · 2015-07-21T14:48:07Z

@Be-ing I don't understand your question at all.

pwhelan · 2015-07-21T14:49:30Z

Well then, I'm definitely in favor of removing the .disconnect()

I'm not really in favor of removing this. I actually use it quite a bit. I would be willing to document it on the wiki.

Be-ing · 2015-07-21T17:09:03Z

@pwhelan , how do you use it? In what script? What is the advantage of using it rather than connectControl(..., true)? Why should this duplicate functionality be supported?

daschuer · 2015-07-26T20:18:42Z

@Be-ing: would you mind to prepare a 1.12 branch, that fixes the issue, but keeps the API?
This way we do not break existing mappings in the wild.

pwhelan · 2015-12-01T16:58:04Z

@Be-ing one huge advantage is that the context of the disconnect is clear, ie: in case there are multiple "callbacks" on the same CO with the same name.

Be-ing · 2016-04-11T16:03:39Z

I've started a new branch to present the undocumented functionality in connectControl() in a cleaner way.

Be-ing · 2017-03-15T04:10:13Z

@pwhelan

@Be-ing one huge advantage is that the context of the disconnect is clear, ie: in case there are multiple "callbacks" on the same CO with the same name.

Actually that was not the case. Calling the disconnect method on these objects would disconnect all the callbacks connected to that ControlObject sharing the same callback function, even if the this object for the connections was different. I fixed this in #1218.

daschuer reviewed Jul 20, 2015
View reviewed changes

Be-ing changed the title ~~organize ControllerEngine::connectControl and ControllerEngine::disconnectControl~~ simplify ControllerEngine::connectControl Jul 20, 2015

Be-ing mentioned this pull request Apr 11, 2016

call MIDI input script functions with appropriate "this" object #919

Merged

Be-ing closed this Apr 11, 2016

Be-ing mentioned this pull request Sep 22, 2016

Add new Mixco based MIDI controller scripts #1016

Merged

Be-ing mentioned this pull request Feb 28, 2017

Script connection object trigger method #1198

Merged

Be-ing mentioned this pull request Apr 6, 2017

better error handling for ControllerEngine::makeConnection & connectControl #1232

Merged

Be-ing deleted the script_disconnect_control branch April 24, 2019 23:42

mixxxbot mentioned this pull request Aug 22, 2022

engine.connectionControl(group, control, function, true) connects control to a function rather than removes connection #8062

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

simplify ControllerEngine::connectControl #656

simplify ControllerEngine::connectControl #656

Be-ing commented Jul 20, 2015

Be-ing commented Jul 20, 2015

daschuer Jul 20, 2015

daschuer commented Jul 20, 2015

Be-ing commented Jul 20, 2015

daschuer commented Jul 20, 2015

Be-ing commented Jul 20, 2015

daschuer commented Jul 20, 2015

Be-ing commented Jul 20, 2015

uklotzde commented Jul 20, 2015

Be-ing commented Jul 20, 2015

Be-ing commented Jul 20, 2015

Be-ing commented Jul 20, 2015

daschuer commented Jul 20, 2015

daschuer commented Jul 20, 2015

Be-ing commented Jul 20, 2015

pwhelan commented Jul 21, 2015

pwhelan commented Jul 21, 2015

Be-ing commented Jul 21, 2015

daschuer commented Jul 26, 2015

pwhelan commented Dec 1, 2015

Be-ing commented Apr 11, 2016

Be-ing commented Mar 15, 2017

simplify ControllerEngine::connectControl #656

simplify ControllerEngine::connectControl #656

Conversation

Be-ing commented Jul 20, 2015

Be-ing commented Jul 20, 2015

daschuer Jul 20, 2015

Choose a reason for hiding this comment

daschuer commented Jul 20, 2015

Be-ing commented Jul 20, 2015

daschuer commented Jul 20, 2015

Be-ing commented Jul 20, 2015

daschuer commented Jul 20, 2015

Be-ing commented Jul 20, 2015

uklotzde commented Jul 20, 2015

Be-ing commented Jul 20, 2015

Be-ing commented Jul 20, 2015

Be-ing commented Jul 20, 2015

daschuer commented Jul 20, 2015

daschuer commented Jul 20, 2015

Be-ing commented Jul 20, 2015

pwhelan commented Jul 21, 2015

pwhelan commented Jul 21, 2015

Be-ing commented Jul 21, 2015

daschuer commented Jul 26, 2015

pwhelan commented Dec 1, 2015

Be-ing commented Apr 11, 2016

Be-ing commented Mar 15, 2017