Skip to content

introduce ControllerEngine::makeConnection and deprecate ControllerEngine::connectControl#1218

Merged
daschuer merged 9 commits intomixxxdj:masterfrom
Be-ing:connection_object_disconnection_fix
Apr 5, 2017
Merged

introduce ControllerEngine::makeConnection and deprecate ControllerEngine::connectControl#1218
daschuer merged 9 commits intomixxxdj:masterfrom
Be-ing:connection_object_disconnection_fix

Conversation

@Be-ing
Copy link
Copy Markdown
Contributor

@Be-ing Be-ing commented Mar 15, 2017

This fixes a bug in which calling the disconnect() method of script callback connection objects would disconnect all callbacks connected to its ControlObject that shared a callback function. This created a very confusing situation when I accidentally created two Components that both connected the same components.Button.prototype.output function to the same ControlObject -- even though those callback functions would execute in different contexts and have different effects. I have also added tests for the quirky, inconsistent behavior of ControllerEngine::connectControl() and useful debugging messages.

…ests

This fixes a bug in which calling the disconnect() method of
script callback connection objects would disconnect all callbacks connected
to its ControlObject.

Also add debugging messages for connecting and disconnecting callbacks

// Clear the m_connectedControls hash so we stop responding
// to signals.
m_connectedControls.clear();
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This did not have any practical effect because the connections are stored in ControlObjectScripts, so I removed this unnecessary member variable.

and ControllerEngineConnectionScriptValue -> ScriptConnectionInvokableWrapper
@Be-ing Be-ing force-pushed the connection_object_disconnection_fix branch from 6ef0389 to 51d183b Compare March 15, 2017 03:50
@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Mar 15, 2017

I tried to make a wrapper for ControllerEngine::connectControl that could be invoked with new from scripts... it's either:

  • not possible
  • requires some convoluted magic C++ incantations to get QScriptEngine::newFunction to accept a pointer to a member function of ControllerEngine. I tried various incantations of std::bind, std::mem_fn, and std::function to no avail.
  • requires refactoring so ControlObjectScripts and ScriptConnections are kept track of by something outside of ControllerEngine so a function that isn't a member of ControllerEngine can be passed to QScriptEngine::newFunction. This seems to be complicated, dangerous, and way too much work just for a prettier JavaScript syntax.

So I guess the best way forward is to document on the wiki how to use the connection objects returned by ControllerEngine::connectControl.

@Be-ing Be-ing force-pushed the connection_object_disconnection_fix branch from e6424a3 to 1b5365d Compare March 15, 2017 20:12
… a block

and add more tests, comments, and debugging messages
@Be-ing Be-ing force-pushed the connection_object_disconnection_fix branch from 1b5365d to 482cafb Compare March 15, 2017 20:15
@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Mar 15, 2017

I refactored ControllerEngine::connectControl to quarantine the confusing behavior inside its own block. If you use code folding in your editor to collapse the block, the only weird thing about the function is that it checks whether the callback is a function twice (which it also did before this refactoring).

and deprecate the confusing old ControllerEngine::connectControl function
@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Mar 15, 2017

Instead of leaving the confusing code as an isolated block with ControllerEngine::connectControl, I have moved the part of ControllerEngine::connectControl that actually makes the connections into a new script invokable function with a name that hints it returns an object: ControllerEngine::makeConnection. This new function only accepts JavaScript functions as callbacks (not strings of JavaScript source code, which the C++ side should only ever have to handle when loading a script file). It does not double as a way to remove connections; instead, scripts must store the returned connection object and call its disconnect method. I think it will be clearer to explain to script authors that ControllerEngine::connectControl is now deprecated and ControllerEngine::makeConnection should be used instead than to explain that ControllerEngine::connectControl does not actually do what the wiki has said it does. I think this will also be more maintainable with the quirky legacy code confined to ControllerEngine::connectControl.

@Be-ing Be-ing changed the title cleanup ControlObject to script callback connection code introduce ControllerEngine::makeConnection and deprecate ControllerEngine::connectControl Mar 15, 2017
@Be-ing Be-ing mentioned this pull request Mar 21, 2017
2 tasks
@Be-ing Be-ing mentioned this pull request Mar 28, 2017
4 tasks
@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Apr 4, 2017

Can someone review this?

@daschuer
Copy link
Copy Markdown
Member

daschuer commented Apr 5, 2017

LGTM Thank you.

@daschuer daschuer merged commit c74670c into mixxxdj:master Apr 5, 2017
@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Apr 5, 2017

I have updated the wiki to document engine.makeConnection. I also reorganized the wiki page.

I'll update Components to use engine.makeConnection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants