-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add GDD Type invoke-action #11
Conversation
Co-authored by imaretic <[email protected]> Co-authored by Tuomo Kulomaa <[email protected]>
README.md
Outdated
"type": "null", | ||
"gddType": "invoke-action", | ||
"gddOptions": { | ||
// [mandatory] The function to invoke in the template |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description of the "invoke" field should clarify if this is "the name of the function to invoke" or "an invocation of the function", i.e. - for a function called myCustomHello
should this be myCustomHello
or myCustomHello("someArg0")
? I was confused by what this is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
I've improved the description a tad now.
The invoke
property is supposed to contain the script being executed in the target template, myCustomHello("someArg0")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jstarpl would you say this description is good enough?
https://github.com/SuperFlyTV/GraphicsDataDefinition/pull/11/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R386
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way the AMCP command CG INVOKE is documented, it does not allow to have arguments. Will that be changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the AMCP documentation definitely needs updating because AFAIK it this been supported for years. It's essentially executing any JS that it gets, not necessarily just invoking a function. You can do CG 1-1 INVOKE 1 "var a = 1;a++;console.log(a)"
and it'll work too, which might be a good or a bad thing, depending on where the invoked script comes from
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, but what you explain is just kind of an inline function, that sits in place of the function name, like CG 1-1 INVOKE 1 "function_name". That is no contradiction to the documentation. What @nytamin and @jstarpl are talking about is having a function name and an argument, like we have for the CG UPDATE command. And that is undocumented and should, in my opinion be avoided.
Passing scripts as strings encourages using eval, which is not a good practice. How about making separate properties for the action name and an array of arguments? It would be more language-agnostic, letting the rendered decide how to safely identify the function to call, and call it with given arguments, in a language/technology that the template is written in, without letting any arbitrary code to slip through. |
A second attempt, following the dicussion in #11
I agree. I'm closing this PR and have instead made an alternative (#23) that defines the function name and arguments separately. |
This PR adds a special GDD Type called "invoke-cction".
The intention is to allow the user to click on buttons in the GUI to INVOKE custom functions in the template (this is a fairly common use case for CasparCG templates).
Co-authored by imaretic [email protected]
Co-authored by Tuomo Kulomaa [email protected]