Skip to content

Script connection object trigger method#1198

Merged
daschuer merged 2 commits intomixxxdj:masterfrom
Be-ing:script_connection_object_trigger_method
Feb 28, 2017
Merged

Script connection object trigger method#1198
daschuer merged 2 commits intomixxxdj:masterfrom
Be-ing:script_connection_object_trigger_method

Conversation

@Be-ing
Copy link
Copy Markdown
Contributor

@Be-ing Be-ing commented Feb 27, 2017

Previously, engine.trigger(group, key) had to be called by scripts, which would trigger all callbacks connected to that ConfigKey. This new method allows more fine-grained control by scripts.

Previously, engine.trigger(group, key) had to be called by scripts, which would trigger all callbacks connected to that ConfigKey. This new method allows more fine-grained control by scripts.
@daschuer
Copy link
Copy Markdown
Member

LGTM. Though I cannot test this. I sure you did ;-)

@daschuer daschuer merged commit af7faa1 into mixxxdj:master Feb 28, 2017
@Be-ing Be-ing deleted the script_connection_object_trigger_method branch February 28, 2017 20:44
@Pegasus-RPG
Copy link
Copy Markdown
Member

Wait, can you please add a test for this new function?

@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Feb 28, 2017

Yeah, I'll open a new PR with a test. I consider this a quick hack for 2.1. For 2.2 I'd like to rethink this functionality and provide a nicer, more JavaScriptish syntax (something like var conn = new ControlConnection('[Channel1]', 'play_indicator', function () { do stuff; })) along with creating a way to register input callbacks from JS.

@Pegasus-RPG
Copy link
Copy Markdown
Member

Still, anything we're exposing to the users will a) need to be supported and b) unit tested to ensure it works as expected in the future. (If you want to do quick hacks, do them in script, not user-facing APIs.)

@rryan
Copy link
Copy Markdown
Member

rryan commented Feb 28, 2017

I think this still triggers all the callbacks since it does the same thing ControllerEngine::trigger does (causing the ControlObjectScript to emit a valueChanged signal). To just trigger the connection itself I think you would need to call the logic in the loop in ControlObjectScript::slotValueChanged for just the ControllerEngineConnection in question.

@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Feb 28, 2017

I think this still triggers all the callbacks since it does the same thing ControllerEngine::trigger does (causing the ControlObjectScript to emit a valueChanged signal).

No, it does not, because it is up to the script to keep track of the connection objects and specifically trigger the appropriate connections.

engine.connectControl() returning a connection object has never been documented. I discovered it accidentally by reading the C++ source (refer to #656). For the most part, this should be abstracted away by Components. Script authors should only have to use that functionality directly when implementing a custom connect method for a Component that registers multiple connections. Before c317735, a Component would have to also implement its own trigger method in that case because the prototype trigger method relies on engine.trigger with a single outKey. Refer to here for an example.

@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Feb 28, 2017

@rryan
Copy link
Copy Markdown
Member

rryan commented Feb 28, 2017

No, it does not, because it is up to the script to keep track of the connection objects and specifically trigger the appropriate connections.

Hm, are you sure? ControllerEngine::triggerControl looks up the ControlObjectScript using the string group/item and tells it to emitValueChanged. This should be equivalent to ControllerEngine::trigger(connection.key.group, connection.key.item).

engine.connectControl() returning a connection object has never been documented

👍 this isn't a big deal since it was never documented in the first place. It's still nice to have tests though :).

@rryan
Copy link
Copy Markdown
Member

rryan commented Feb 28, 2017

Example use of the fixed functionality

Was this meant for #1196?

@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Feb 28, 2017

Hm, are you sure? ControllerEngine::triggerControl looks up the ControlObjectScript using the string group/item and tells it to emitValueChanged. This should be equivalent to ControllerEngine::trigger(connection.key.group, connection.key.item).

Oh, you're right. I'll try modifying ControllerEngine::triggerControl to specifically call the connection's callback instead of calling the COScript's emitValueChanged method.

@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Feb 28, 2017

Was this meant for #1196?

Yeah, sorry. It can get confusing keeping track of a bunch of PRs and git branches :S Here is where I meant to link.

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.

4 participants