Skip to content
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

Extend expressions to action options #2345

Open
2 tasks done
dnmeid opened this issue Feb 23, 2023 · 12 comments
Open
2 tasks done

Extend expressions to action options #2345

dnmeid opened this issue Feb 23, 2023 · 12 comments
Assignees
Labels
area/backend Something in the core of companion area/gui GUI / Webapp related Enhancement New feature or request
Milestone

Comments

@dnmeid
Copy link
Member

dnmeid commented Feb 23, 2023

Is this a feature relevant to companion itself, and not a module?

  • I believe this to be a feature for companion, not a module

Is there an existing issue for this?

  • I have searched the existing issues

Describe the feature

Button text can be entered either as text (with inline variable support) or as an expression with numerous calculations and functions available. For the modules on the other hand there is the option to allow users to enter variables in textinput options and then parse the variables in the text.
The goal of this feature suggestion is to extend the expressions functionality to all action options.

Aspects:

  • The options get a new optional (mandatory if expression is present) property expressionEnabled: boolean to store if the expression or the original input is used
  • When the option expressionEnabled is set, the input element gets a toggle button to switch between the default input (ordinary textinput, dropdown, checkbox, number slider...) and an expression textinput
  • When the expression is changed or a variable used in the expression changes, the result of the expression is calculated and typecasted to what the option needs
    • textinput -> result as string
    • chekbox -> result is truthy value
    • number -> result parsed as number
    • dropdown -> when result is number then index of options, when text then matching item
    • multidropdown -> when result is array of numbers or strings then multiple items, when number or string then single item
    • variable -> result as string
  • failed expressions should give null and the module can decide what to do then
  • the expression doesn't change what is written in the value of the original option, at execution time either the original value or the expression result is used
  • The options get a new optional property expression: stringto preset the expression itself
  • The option gets a new optional property affectsSubscription. Calls to subsribe/unsubscribe would only be made if the option changes and this property is not set to false

Possibilities tbd:

  • extend expressions also to feedback options, but not sure about the usecases at the moment. It would require to track feedbacks with expressions and then recalculate them if any used variable in the expression changes
  • how/where to store expressions, enable state of expressions and affectsSubscription in the database
  • should there be range checks at core level or should this be left to the module (like with regex also is only a visual hint)?

Usecases

Using variables or custom variables in actions and at the same time retaining the convenience of input elements like dropdown menu for manual selection.

@dnmeid dnmeid added Enhancement New feature or request area/gui GUI / Webapp related area/backend Something in the core of companion labels Feb 23, 2023
@dnmeid dnmeid self-assigned this Feb 23, 2023
@Julusian
Copy link
Member

failed expressions should lead to the options default value, module developers need to make sure that default value makes sense

I think it would make more sense for the action to be skipped. Maybe expressionFallbackValue would be a good idea to opt into the action to still be called.

extend expressions also to feedback options, but not sure about the usecases at the moment. It would require to track feedbacks with expressions and then recalculate them if any used variable in the expression changes

As of #2251 we handle this for feedbacks for basic variable parsing, it should be possible to extend to expressions safely.


I started playing with this almost a year back in #2032, and have a very long note with my thoughts https://github.com/orgs/bitfocus/projects/2?query=is%3Aopen+sort%3Aupdated-desc&pane=issue&itemId=19678623.

The TLDR is that the main question I have is: how do we store that the user has switched an input field to expression mode? The module owns the options property, and anything we change away from the current structure could be problematic with upgrade scripts, and typescript.
I wrote that note in the flow of expressions would be pre-parsed and modules wouldnt be aware that an expression was being used. Of course we could go the other way and make it like parsing variables is today

@dnmeid
Copy link
Member Author

dnmeid commented Feb 23, 2023

think it would make more sense for the action to be skipped.

I would leave it to the module dev to decide what to do. E.g. the expressionFallbackValue can be set to a value which makes sense to use or it can be set to a marker like null to make the callback aware that the expression failed.
Having said that I see that it would be sufficient to just return null for a failed expression and the module dev now can also use a default value in the callback code. Or log an error or just return without doing anything.
That means in the moment an expression is possible the value can also be of type null and the module needs to take care of that.

how do we store that the user has switched an input field to expression mode?

So far I don't see a big problem with storing it in the database as extra props in options. Until now nobody can use expressions for options, so no upgrade script will take care of them if they exist. If a module starts using expressions but at the same time uses outdated update scripts, which may brake the expressions, I would call this a bug in the module.
Additional props like expressionUsed and expression can be safely exposed to the module. I would even consider making them available like any other property, so the module can actively work with them and use them e.g. in presets.
Then we wouldn't need expressionUsed, because the existance of the property enableExpression could be used to decide wether expression is available and the value could be used to decide wether it is turned on or off.

At the moment I can not think of how a module should want to manipulate the expression itself in an upgrade script, I guess it should just stay like it is. But you know, crazy guys have crazy ideas.

Taking over from you nice draft (sorry I didn't find that, I was searching issues only)

Complications around the module requesting subscribe to be called?

Yes, if the module wants to do something if its usage is changed, we have to call unsubscribe/subscribe every time a option changes, no matter if it was a user input or a new expression result. But maybe it would help to do a finer differenciation between action added/removed and some option change.
There is another point to consider: isVisible function.
As far as I remember the visibility will be calculated in the frontend. If we calculate the expression only when the action is run it would be fine for the action, but the gui wouldn't update.
That means regardles wether it is in an action or feedback, every expression has to be evaluated immediately when it changes or a used variable changes.

BTW: I'll update the first post along with the discussion

@Julusian
Copy link
Member

Having said that I see that it would be sufficient to just return null for a failed expression

I'm happy with that.

So far I don't see a big problem with storing it in the database as extra props in options.

Are you suggesting the options looks like:

{
  value: 123,
  valueExpression: '100 + 23'
}

or

{
  value: {
    value: 123,
    isExpression: true,
    expression: '100 + 23'
  },
}

I think that doing it as an object would be better, as it gives us scope to add additional info about the expression to it. (eg a debounce time). It does make it more breaking for modules, which is where I started wondering about hiding this from the modules.

I'm not too opposed to either, but its something I want to make sure is defined well as once it is done it will be in the module-api until we want another break potentially every module change.

Then we wouldn't need expressionUsed, because the existance of the property enableExpression could be used to decide wether expression is available and the value could be used to decide wether it is turned on or off.

I thought about trying to make it clever like this, as it is easy to do for number fields. but if a text field has a value of 1 + 2 is that a plain string or an expression?

At the moment I can not think of how a module should want to manipulate the expression itself in an upgrade script, I guess it should just stay like it is. But you know, crazy guys have crazy ideas.

I am thinking that a module shouldnt be able to manipulate an expression. Those would have been defined (most likely, exception being presets) by the user, and it wont be able to make any potentially needed changes to them without understanding what the user has written and how.

But maybe it would help to do a finer differentiation between action added/removed and some option change.

This is where my proposed affectsSubscription property comes in, as a way to only call subscribe on expression change when the module is interested in it.

I don't think we can do anything better because subscribe is intended to let modules only load data when it is needed. For example, in the x32 module it lets it only load the current mute state of a channel when it is needed by an action (for toggle) or a feedback.
Once expressions can be used, it will be asked for to have the channel identifier field be an expression. So the module will either need to know every time that expression changes so that it can make sure it has the current channel loaded, or it will have to load every possible value it may need. In reality, this isnt too bad for x32, but yamaha-rcp has said something about thousands of possible values, so it could be a lot worse there.

But for the value field (which will also be an expression), the module can leave affectsSubscription unset so that we know we dont have to watch that expression.

As far as I remember the visibility will be calculated in the frontend. If we calculate the expression only when the action is run it would be fine for the action, but the gui wouldn't update.

I dont think I follow this, are you saying that the value of these expressions will be shown in the ui and we can avoid doing that?

That means regardles wether it is in an action or feedback, every expression has to be evaluated immediately when it changes or a used variable changes.

I think this is unavoidable for some expressions. But I would hope that the ones used in id fields would hopefully not be updating too frequently and be causing problems. Perhaps later on a debounce could be added, to be able to rate limit them.

@dnmeid
Copy link
Member Author

dnmeid commented Feb 23, 2023

I'd vote for the flat version like

{
  value: 123,
  expression: '100 + 23',
  expressionEnabled: true
}

simply because this wouldn't be a breaking change for update scripts. Is there anything else about the expression which should be stored on a per option basis? Different debounce times per expression only makes sense if the user can also change the debounce time per expression.

if a text field has a value of 1 + 2 is that a plain string or an expression?

Oh maybe I was not clear enough. If an expression should be available to the user in the database there always have to be three properties. Let's look at a textinput option definition:
Without the possibility to use expressions
Definition

{
  id: 'mytext',
  label: 'Text',
  type: 'textinput',
  default: '',
}

Database

  value: '1 + 2'

That is a plain string

With the possibility to use an expression, but at the action creation the expression is not active
Definition

{
  id: 'mytext',
  label: 'Text',
  type: 'textinput',
  default: '',
  expressionEnabled: false,
}

Database

  value: '1 + 2',
  expressionEnabled: true,
  expression: '1 + 2',

The user switched to the expression and made it "1 + 2" evaluated to the number 3 resulting in the string "3"

With the possibility to use an expression and at the action creation the expression is active

{
  id: 'mytext',
  label: 'Text',
  type: 'textinput',
  default: '',
  expressionEnabled: true,
}

Database

  value: '1 + 2',
  expressionEnabled: false,
  expression: '1 + 2',

The user switched to the value input and made it "1 + 2" resulting in the string "1 + 2"

The value or result of the expression doesn't need to be stored, it is calulated at runtime.
I think it couldn't hurt to have the expression field itself available in the definition as well like:

{
  id: 'intensity',
  label: 'Intensity',
  type: 'number',
  min: 0,
  max: 100,
  default: 50,
  expressionEnabled: true,
  expression: '$(internal:time-h) * 2',
}

So the module can optionally preset expressions at action creation exactly like the default value does for the regular input. And then it is the same syntax in the preset definition.

This is where my proposed affectsSubscription property comes in

Yeah, got you now. So this could be helpful to non expression options as well.

I dont think I follow this, are you saying that the value of these expressions will be shown in the ui and we can avoid doing that?

No, the opposite. The UI should show only the expression and maybe the current value it evaluates to when having focus. And we have to calculate at the backend on any updates, not only when the action is run.
But the value generated by the expression has to promote to the gui to update the isVisible status.
I have to admit thet there are some potential pitfalls: a option could make itself invisible and then be unreachable. But as far as I know that is allready possible today, it would just get easier to mess up with expressions.

@Julusian
Copy link
Member

Julusian commented Jul 3, 2023

Something else to consider here for the module side, is that perhaps we could change how the actions and feedbacks options object looks in the callbacks.

So instead of being a plain js object of the values, it could be something like:

class CallbackOptions {
  public getRaw(fieldName: string): any
  public getString(fieldName: string): string // Or should this be `getPlainString`?

  // For expressions:
  public getNumberExpression(fieldName: string): Promise<number> // Or should this be `getNumber`?

  // For variables:
  public getAndParse(fieldName: string): Promise<string>

  // This isn't all the methods, just an example of how some of this could look.
}

Inside these functions we could do some data validation, so if getNumber is used on a field which is not a number, it can throw instead. And other type validation.

To do this, we could add some additional action and feedback types which use this new structure, and these new types could be required when wanting to be able to use expressions.

This class would help simplify things for users (especially typescript users), as then we will provide any wrappers for interpreting the values. And this will help us with tracking what variables are referenced by each action/feedback. Feedbacks today have to use the parseVariablesInString() provided by the second parameter of the callbacks. If they use the one on the class, then the feedback wont be reactive as we can't match up what calls were for what feedback. This is what puts me off having variables scoped to the buttons made generally available, modules will need updating to use the parse method on the context for them to handle it. And how do we show users whether those variables should work?
This simplifies it, as the easiest way of parsing the variables is with options.getAndParse('value').

I don't know how upgrade scripts would work with this, maybe they would similarly have a class, but perhaps a different class that has different operations? Or maybe they would be left with the raw options object?

@MeestorX
Copy link

MeestorX commented Aug 13, 2023

@Julusian it looks like a small side effect is that, like the input toggle that you implemented previously (but only for number fields) this update would work across all option types? BIG upvote to please implement this!
Currently, I have to make ALL options text fields or dropdowns so I can support variables. Doesn't work great for numbers or checkboxes, etc.
Also, hopefully while implementing this, subscribe() can be now be called for both actions and feedbacks when a variable or expression changes value.

@marklneumann
Copy link

Is there a timeline for this functionality to be implemented in a build? It seems there are multiple modules that would benefit from this ability.

@chabad360
Copy link

chabad360 commented Jun 24, 2024

This class would help simplify things for users (especially typescript users), as then we will provide any wrappers for interpreting the values.

If I may, I like the idea of callbacks, but it is possible to use some more complex typing to just provide the same object Companion was before, with full types (see here for a quick experiment). The callbacks should be an escape hatch when the module dev wants the raw values (for some weird reason). Companion should provide a final value to the module, not something they need to process.

Broadly speaking. I would like to suggest that - by default - Companion should provide the final parsed result of the expression to the action, with an escape hatch (i.e. rawValue(fieldName: string)) if the dev wants it. This effectively means, every field can be an expression.
If the module says the field is a checkbox (or any other non-text field), Companion UI will show a checkbox with a little $ next to it, and when the user clicks that $, that field will change to an expression input. There should be some UI to support this, i.e. info popover with what are the valid return values (especially important for dropdown fields), error/result checking of some sort, etc.

If, on the other hand, the field takes text input (i.e. number or text), expressions will only be parsed if they're within {{...}} (or what have you) i.e. {{10/10}} = 1, and {{10}} / {{10}} = 10 / 10. This should preserve backward compatibility, and we could just use an update script to move the internal stuff to be wrapped in {{...}}.
Now I will admit, this bit is not perfect, if only because I just said the other fields would be expressions and that would be a drop confusing, so maybe we could just say any field can become a text field and expressions can be used in that text field (and then it's all the same and every expression needs to be wrapped in {{...}}). The other option is to do the inverse, treat {{...}} as an escape sequence (effectively to legacy/current behavior) and the update script wrap all existing text fields in {{...}}.

My general point is, Expressions (and Variables) support should not be up to the module developer, it creates additional burden for the developer (needless checks and calls), and needless confusion and complexity for the end user. This feature needs to be seamless and pervasive to be effective and truly useful.

P.S. Thanks to @thedist for helping me work out some of the idea here.

@dnmeid
Copy link
Member Author

dnmeid commented Jun 26, 2024

@chabad360 yes, the idea of the implementaion is to make this a non breaking thing for modules with an opt-out. So every module can benefit from expressions without the module dev having to update the code. There will be additional features for the module to have more control.

@dnmeid
Copy link
Member Author

dnmeid commented Feb 18, 2025

With the latest discussion about graphics generation and button styling in mind, I think this feature needs more priority.
Additionally I've been re-thinking the way if the user selected the original value or the expression to be active.
My current proposal is: both.
As soon as there is a valid, non empty expression the expression result should be used. The original value should be made available to the expression in a local variable, e.g. $(this.value).
That means the expression can work like a tween function or completely replace the value depending on what you do with it.

The toggle button besides the input would only switch what you see but would not have an impact on the result. My question is if we should store the state of this button in the database and should sync it between clients or should it be local to to a client? And if it is local what should we present initially?

@Julusian
Copy link
Member

Julusian commented Feb 26, 2025

Whatever we do here should be consistent with how the togglable value/expression fields are in #3299.

As soon as there is a valid, non empty expression the expression result should be used. The original value should be made available to the expression in a local variable, e.g. $(this.value).
That means the expression can work like a tween function or completely replace the value depending on what you do with it.

I'm not sure about this. I think this will be more confusing than helpful. This isn't what we do elsewhere (which could be changed), but I don't see the value in having this split view of only seeing one of the components of the value. Why not just add a = 50 at the top of your expression if you want this behaviour instead of having it elsewhere in the ui?

The toggle button besides the input would only switch what you see but would not have an impact on the result. My question is if we should store the state of this button in the database and should sync it between clients or should it be local to to a client? And if it is local what should we present initially?

I like the approach of having a toggle next to the field to swap the behaviour of the field, and for that I am storing each field as { value: 'value or expression', isExpression: true } in the db.

I will once again advocate for storing each thing as an object, either way upgrade scripts will need to be aware of this, and I think that doing it as an object makes it easier for both us and them.
In particular I am concerned about upgrade scripts which rename fields; if it is a bunch of properties at the root level, then upgrade scripts will need to know what other ones to rename. But as an object, we keep it as a single value to rename, and we make it explicit that this isnt just a value that the script is looking at.


My general point is, Expressions (and Variables) support should not be up to the module developer, it creates additional burden for the developer (needless checks and calls), and needless confusion and complexity for the end user. This feature needs to be seamless and pervasive to be effective and truly useful.

I don't exactly disagree with this; It would be great to make these things seamless for module devs, but I am not sure we can safely tack this on without breaking all kinds of things.

Regarding my lifecycle concerns, the current flow is to at startup/when edited tell the module about each action, module-base will run upgrade scripts if needed, and call subscribe/unsubscribe as needed.
The action is then passed in again when executed.

So it would be trivial to auto-parse expressions when executing, but upgrade and subscribe use the same object, so I am not sure it will be easy to change one without the other (unless this requires an update to module-base)


While this task is explicitly about actions, should we also consider feedbacks?
The approach will likely be very different, but it would be good to have a similar experience for users and structure in the upgrade scripts.

The current approach for feedbacks is companion tells the module about every feedback and its properties, and the module-base code inside the module process handles the rechecking of feedbacks when the module says it needs to, or if a variable parsed in the feedback changes.

If we want companion to be the one parsing expressions like is proposed here for actions, that will work, but the api used by module-base uses the same data input for starting this feedback execution and for running upgrade scripts on.
So as far as I can tell, bringing expressions to feedbacks will at the very least require modules to update their module-base (even if the class api exposed to module code remains unchanged


I feel like I should make a diagram of this action/feedback flow/lifecycle, to make this easier to reason about/visualise..

@Julusian
Copy link
Member

Here are some rough flow diagrams of the interaction between companion and the modules. Hopefully they are clear enough, I have never tried to make a diagram like this before

Image
Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/backend Something in the core of companion area/gui GUI / Webapp related Enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants