Ideas for breaking changes

[Julusian]

A collection of ideas that could be considered for a breaking change. Many could be done as optional things before then.
Feel free to suggest ideas in comments, or if it needs more description/discussion as an issue that can be linked to

• replace the isVisible functions with the expression syntax. This will make them safer to execute, and hopefully easier for module developers to understand the limitations. This could definitely be added as an option in a minor version, only removing the function support in a major version bump
• static-text fields should use markdown, not html
• remove parseVariablesFromString off the class?
  It has been available for a while through the context parameter, and we want to encourage people to use that as it supports local variables.
  If we do something regarding expressions too, then that will make this even more justified

[MeestorX]

Really? I thought you'd never ask!! ;-)
Not really sure if any of these would be "breaking", they feel like just enhancements. But since they haven't been implemented, I'll assume they are not easy and DO break things...

1. Variable access for more parameters than just button text. Colors, images, etc. on buttons settable by variables. Enable/disable actions/feedbacks by variables. Make as much available to variables as possible. I guess variables also need to be able to handle more than just text values, so maybe they should be more like actions with different types of values - text, numbers, booleans, drop-downs ???
2. Allow internal actions in Presets.
3. Ability to use variables for all types of actions, not just textinput. Number, checkboxes, static-text, etc.
4. Merge feedbacks and triggers. They overlap in their capabilities, so maybe there's a way to make it just one thing.

[Julusian]

@MeestorX

1. Might have some impact here, might not. Expose properties instead of actions companion#1187 might be a part of this. TLDR of that issue is make 'variables' be defined to have types and to be optionally settable. Avoid forcing every module to expose each value as an action, feedback and variable and do the heavy lifting for them.
2. Not breaking, just concerned about changes to the internal actions causing accidental breakages
3. that is semi-breaking. at the very least is opt-in and can break existing assumptions modules have about the contents of the options objects
4. I dont think anything should be done on that. This can already be done with the 'When condition is true' trigger event. The conditions in triggers are feedbacks already, and the condition and event was intentionally split a few releases back to make it possible to add conditions to the other event sources (such as time of day or interval)

[MeestorX]

1. Might have some impact here, might not. Expose properties instead of actions companion#1187 might be a part of this. TLDR of that issue is make 'variables' be defined to have types and to be optionally settable. Avoid forcing every module to expose each value as an action, feedback and variable and do the heavy lifting for them.

As I mentioned in that issue, I don't completely follow, but typically your changes tend to improve things, so I say go for it!

2. Not breaking, just concerned about changes to the internal actions causing accidental breakages

huh? "Not breaking", "accidental breakages"?

3. that is semi-breaking. at the very least is opt-in and can break existing assumptions modules have about the contents of the options objects

Not sure what you mean. Devs can already choose whether or not to support variables, what is a scenario you see to be problematic?

4. I dont think anything should be done on that. This can already be done with the 'When condition is true' trigger event. The conditions in triggers are feedbacks already, and the condition and event was intentionally split a few releases back to make it possible to add conditions to the other event sources (such as time of day or interval)

You're maybe thinking of it backwards? I'm saying that Triggers can do everything a feedbacks can (and there's lots of overlap) (correct me if I'm wrong) and more, so what's the need for feedbacks? Do it all in triggers so there's less confusion and overlap.

[Julusian]

2. not breaking to modules as such to add, but possibly a high risk of breaking any presets using those actions accidentally in the future. But I am warming to that concern being overly paranoid and it will probably be fine

3. In some way we will need to track that the dropdown field should be shown in a variable mode. do we do that by changing the structure of the value inside the options object, or do we add a second property there to describe this? either way, modules will need to be aware of this, because upgrade scripts will need to be aware of the change and handle it correctly. would adding a new property which is only there sometimes break some actions? I wouldn't be surprised if some are passing off the whole blob to some api without validating it. And this is assuming we make it opt-in. If we force it everywhere (which would be nice to do in a way, otherwise it will be only minimally implemented), then all the same concerns and more because modules will have been programmed before this was a thing and so stand no chance of being defensive enough against it.
   The prototype I did a year or two back worked by checking whether the value was a number or a string, which was barely ok for that, and will not work for all control types.
   And now that expressions are a thing and being asked for, we will need to differentiate between being in expression and text+variables mode too

4. oh right. I disagree then. I suppose that technically they can, but they utilise feedbacks for the conditions, so they cant go away entirely. And I think that doing it in triggers will make more complex setups much more sprawling.

For example, to replace this:

I need 3 triggers:

Notice how similar these are in condition, but not enough to share any logic (even if we had 'release actions' in triggers)

Some of this could maybe be simplified if triggers was refocused to be a replacement for feedbacks, but due to the 'action' nature (vs 'state' nature of feedbacks), it is a lot more effort to get the same result.

Even in a simple case of using an expression $(internal:time_s) == 0, wouldn't I need two triggers? one to change the style, and one to set it back to the default?

[MeestorX]

2. not breaking to modules as such to add, but possibly a high risk of breaking any presets using those actions accidentally in the future. But I am warming to that concern being overly paranoid and it will probably be fine

I think a good cleanup of the internal actions and feedbacks should be done first, with consideration of the future so that the chances of the internal actions/feedbacks changing in the future its minimal. I haven't looked recently, but last time I did there was what seemed to be a lot of overlap of older and newer versions of what seemed to be the same actions/feedbacks. Would you be able to clean these up if I created a list of suggestions? I presume there'd have to be a "translator" like an upgrade script that internally converts old actions/feedbacks to the new style.

3. In some way we will need to track that the dropdown field should be shown in a variable mode. do we do that by changing the structure of the value inside the options object, or do we add a second property there to describe this? either way, modules will need to be aware of this, because upgrade scripts will need to be aware of the change and handle it correctly. would adding a new property which is only there sometimes break some actions? I wouldn't be surprised if some are passing off the whole blob to some api without validating it. And this is assuming we make it opt-in. If we force it everywhere (which would be nice to do in a way, otherwise it will be only minimally implemented), then all the same concerns and more because modules will have been programmed before this was a thing and so stand no chance of being defensive enough against it.
   The prototype I did a year or two back worked by checking whether the value was a number or a string, which was barely ok for that, and will not work for all control types.
   And now that expressions are a thing and being asked for, we will need to differentiate between being in expression and text+variables mode too

So what you're speaking to is simply how to keep track of which version of the control to show in the gui - i.e. keeping track of whether the user is in "normal" or "variable" mode? If that's what you mean, then why not just a if (options.value == await parseVariablesInString(options.value) then {show the normal version, else show the variables version} If there's no variables or expressions in the value, then those 2 will be the same, and an normal control can be shown.

4. oh right. I disagree then. I suppose that technically they can, but they utilise feedbacks for the conditions, so they cant go away entirely. And I think that doing it in triggers will make more complex setups much more sprawling.

Understood. Minor issue. My OCD can't stand overlap and duplication, but the cleanup of the actions and feedbacks would help that a lot. You're right - no need to do anything about the merging as it would probably confuse users even more.

[MeestorX]

Any more thoughts about allowing internal actions/feedbacks to be available in presets? I would very much like to add presets as I have been asked by new users, but this limitation is a barrier.

[MeestorX]

The Action Recorder is missing the Delay property. When recording actions, the timing can often need to be captured as well to play back the actions with the recorded timing.
This is going to be very important for MIDI message recording, for example.

[MeestorX]

Looks like the structure that includes the delay is already there?
shared-lib/lib/Model/ActionRecorderModel.ts
and
https://github.com/bitfocus/companion/blob/a581c19b264078cbf546f1bd06f7ea36b717f02f/companion/lib/Controls/ActionRecorder.js#L372

[MeestorX]

Can you explain what you mean by the proposed change to isVisible()? Not sure I follow.

[jswalden]

I've been working on converting an existing module to TypeScript, and I've been running up against the sort of concerns/issues that motivated https://github.com/bitfocus/companion-module-bmd-atem/blob/6ecde2befd989e416c4a34dd2c129815d05f83fc/src/common.ts. It would be extremely nice if the various callback APIs, etc. were tightened up so that using TypeScript is more seamless than it is now (and didn't require that extraordinary mess of typing glue). Having every option value be InputValue | undefined means various dealing with edge cases that could be avoided if Companion just did a little more work to expose only option values that are valid, and were restricted to a type indicated by the user. It would also be nice if event.options had a type restricted to containing only the keys corresponding to the action's defined options. (I am still new enough to TypeScript that I'm not certain whether this is possible in all cases if the options are not entirely statically defined.)

Along similar lines, it seems in principle possible (or nearly possible?) to have the type system statically restrict values of default to be within the defined choices, or to do so in at least fairly many cases. (Maybe number-valued options can't restrict default to be within the range of the option?)

Last (and easily least), I don't believe this would be a breaking change, but I think it would be backwards-incompatible -- CompanionPresetAction#options is defined in the TypeScript types as non-optional, but a substantial swath of my module was treating it as optional anyway, in the down/up action sequences in preset steps. And somehow it was working, pre-TypeScript conversion. It's hardly the end of the world adding options: {} a bunch of places, but if the backend is already doing that work, it seems reasonable to support it in TypeScript for actions that don't take any options.

[jswalden]

Also, this is more than a bit trivial/petty (and if you just said "no" I would understand!), but it would be nice if you could specify colors not just by decimal, but by hex. I'm guessing more than a few users would understand color: '#FFFFFF', but will not immediately understand color: '16777215', -- and the problem is likely much worse for colors that aren't literally black or white. (combineRgb isn't quite as legible, IMO, as just specifying colors the way you would in CSS or in many image editing apps.)

[dnmeid]

it would be nice if you could specify colors not just by decimal, but by hex.

@jswalden This thread was meant for discussing breaking changes, but it seems to be parallel universe for any feature request but there is not a single breaking change. It is about ideas of things which can't be done without breaking existing code and will force all or at least some modules to be rewritten. After the adoption rate of the v3 syntax, I hope we won't have a breaking change for years to come.

Anyhow, it is possible to use hex colors ever since. You can just enter a color like color: 0xffaa00. This is the hexadecimal notation of a number and not a string. You have to enter the number with six digits. Since v1.6.0 of base you can also use CSS color strings. That means your example color: '#FFFFFF' is possible today as color: 'rgb(255)' or color: 'white' or color: 'hsl(120, 0%, 100%)'. Color names should only be used, if you do not want to get the color back, i.e. don't use it for setting a color picker.