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

feat: button local variables #3314

Draft
wants to merge 22 commits into
base: develop
Choose a base branch
from
Draft

Conversation

Julusian
Copy link
Member

@Julusian Julusian commented Feb 23, 2025

WIP

Trying to build this as a new type of entity, in the hope that keeps things a bit simpler as these will need to support containing other entities (feedbacks)

TODO:

  • implement usage of the variables
  • add 'constant' variable type
  • add 'feedback' variable type
  • add actions to be able to change a constant variable
  • finish any TODO-localvariable
  • fixup 'add entity' component, to work better for this entity type
  • confirm that circular feedbacks don't cause infinite loop
  • confirm that circular expression loops don't cause infinite loop

image

@dnmeid
Copy link
Member

dnmeid commented Feb 23, 2025

Is this also only a first draft of the UI?
I wonder what the expression is doing at a local variable.
According to the discussions in #1909 and #2827 I thought it would be and look like a global variable with the only difference that it is created in the scope of a control.

@Julusian
Copy link
Member Author

Julusian commented Feb 23, 2025

Well, as I see it we want 3 types of these local variables.

  1. A constant value (which can be updated through an action, behaves like custom variables do)
  2. One which is an expression (which I think should not be settable through actions)
  3. One which the value is from a feedback

Once I started thinking about the data piping and backend structure to be able to do this (primarily needing to be able to host feedbacks), I realised that it would avoid a lot of complexity and risk of future bugs to make them part of the 'entity' system.
So after defining that entity type, this ui became basically 'free'; it is the same react components as used for actions and feedbacks.

Really the driving factor for this approach is 3; 1 & 2 are bonus side effects of this architecture. I am not set on the ui being this way, but it keeps the implementation simple.
I haven't considered the differences between these and 'global' custom variables, but I wouldn't be opposed to bringing the same treatment to that as part of/after this section.

2 isnt explicitly necessary, but I think it will be a nice thing to have to avoid having to repeat expressions in style elements. I don't know why I started with this one instead of the 'constant' value one

@dnmeid
Copy link
Member

dnmeid commented Feb 23, 2025

OK, I'm fine if the UI looking different between the global and the local variables although I'd prefer some uniformity. If this proves to be superior maybe we carry it over to the global variables then.

How will the expression type local variable behave if you set up two of them with cross references? E.g. local:var1 with the expression $(local:var2) * 2 and local:var2 with the expression $(local:var1) * 2. Or one with a circular reference, e.g. local:var1 with the expression $(local:var1) / 2.

So, the constant type will have the options value, initial value, persist, right?

How about the feedback driven type? What is your UI suggestion for making the connection between the feedback and the variable?
It would be possible to add a dropdown to every feedback where you can chose a variable but then you'd at least double the effort of setting it up and also if we think of the data as a stream it would mean we set the receiver at the sender leading to a 1:1 restriction. When I want to reconfigure it, I can kick out existing connections. You get the point.
If we add a dropdown at the variable to point to a feedback, there is the same effort of setting it up but at least it is a less brittle 1:n connection. But then we need to have IDs for the feedbacks.
If the feedback is added to the variable in a container way (like an action to the group), it would be technically fine, but then the only purpose of that variable entity is to provide a name. I guess in that case the feedbacks that we have now in a button would completely go away and the tab will be removed.

I'm not strongly against that implementation, but im not convinced yet.
Let me give an alternative proposal, that of course can also be discussed.

Local variables could be just like global variables with the same or similar UI and the only difference that they have is the scope of a control.
Just like with global variables you could create custom local variables and also there are some that are provided by other elements. E.g. we have some provided by the control like page, row, column.
In the same way a feedback can provide a local variable. The only new thing then would be how to name a local variable created by a feedback. I propose a name property for the feedback. When you add the feedback, the name will automatically be filled with connection_feedback_index. That would then also be the name of the variable, e.g. $(this:myatem_RecordStatus_2)
The user is free to go with that name or to replace it by a custom name.

A local variable that always reflects the result of an expression would not be possible that way, but tbh I fear more problems with that than the value is. We still have the unresolved challenge of circular references.
For the functionality of a reusable expression, it would be much easier to just set a new local variable with an expression from an action. E.g. instead of having a expression type local variable with then name "processed", I could have a regular local variable with the name "processed" and at the beginning of my action stack I add an action to set that variable by an expression.

The whole system of expressions that automatically subscribe to the needed values and automatically reevaluate in case of change has some appeal but also a lot of challenges and I think we will be better off when we try to avoid possible circular references and make the workflows more procedural. I think that will be much easier to understand for the users.

Of course now we have established self reevaluating expressions and they are quite useful and we should keep them but maybe we can try to use them only for "terminal points". I have not a final solution for that problem and am very open for suggestions.

When we solve the problem of circular references we also can introduce the functionality of the entity based expression type local variable with a internal feedback. Today we have internal: Variable: Check boolean expression for boolean values. That can be generic and then you just add that feedback, enter your expression and the feedback will always fill its local variable with the result of the expression. Technically it is the same as the expression type variable but we could avoid introducing many new and different entities.

@Julusian
Copy link
Member Author

Julusian commented Feb 23, 2025

To explain some other decisions I made, I put these under the local name instead of this as I'm worried about name collisions, particularly when we add more to this in the future. local has been a reserved name for as long as this has, just in case we wanted to do this.

How will the expression type local variable behave if you set up two of them with cross references? E.g. local:var1 with the expression $(local:var2) * 2 and local:var2 with the expression $(local:var1) * 2. Or one with a circular reference, e.g. local:var1 with the expression $(local:var1) / 2.

To mitigate the risk of this, I have gone with an approach of lazily evaluating the expressions, so they will be evaluated when they are used rather than how we are doing feedbacks where we precalculate and cache the values.
As this makes the interaction recursive, rather than events firing, we can easily determine that we are in a loop and abort. I took inspiration from the variables parsing logic here, including returning $RE when we start to loop.

Of course the feedback based ones will need a different strategy, and will need some guard to prevent excessive recalculation which I havent begun thinking about

How about the feedback driven type? What is your UI suggestion for making the connection between the feedback and the variable?
If the feedback is added to the variable in a container way (like an action to the group), it would be technically fine, but then the only purpose of that variable entity is to provide a name. I guess in that case the feedbacks that we have now in a button would completely go away and the tab will be removed.

Yes, this would look the same as an action-group or the other composite action/feedbacks we have (logic and, logic or, logic if). In fact, unless we put the effort in to limit it, it will support all the recently added feedback logic operators

A local variable that always reflects the result of an expression would not be possible that way, but tbh I fear more problems with that than the value is

The reason I want this expression type is to avoid having to repeat myself in the style elements.
Perhaps I want to use the same text colour for two blocks of text that will be defined by the expression $(local:var0) > 0 ? 0xff0000 : ($(local:var0) < 0 ? 0x00ff00 : 0x0000ff). Or perhaps I want to split the button into two halves, and have the midpoint be calculated based on the $(this:draw_width) variable.

I guess in that case the feedbacks that we have now in a button would completely go away and the tab will be removed.

This branch is based off develop without any of the drawing work, to try and keep PRs small. But I am writing this for #3299 and what I think will be wanted for the new drawing, so some bits may be a bit weird and confusing when combined with the old buttons.


I will answer the rest tomorrow, when I have some time to think about it more.

@Julusian
Copy link
Member Author

Quick demo of how a simple self referencing variable behaves:
image

@Julusian
Copy link
Member Author

Today we have internal: Variable: Check boolean expression for boolean values. That can be generic and then you just add that feedback, enter your expression and the feedback will always fill its local variable with the result of the expression. Technically it is the same as the expression type variable but we could avoid introducing many new and different entities.

If your expression produces a boolean then yes it is the same, but for any other value it isn't the same. So it would solve some use cases (with less technical complexity, and more user clicks), but not all of the use cases.


It seems like the rest is based around the concern of circular references and trying to mitigate that, so with the lazy execution allowing circular references to be safe, maybe this is fine?

@Julusian Julusian force-pushed the feat/button-local-variables branch from 2c1dc67 to f157a05 Compare February 24, 2025 19:36
@Julusian Julusian added this to the v4.develop milestone Feb 25, 2025
@dnmeid
Copy link
Member

dnmeid commented Feb 25, 2025

I'm still not convinced.
OK, the circular references are solved.
I think this entity based solution is quite elegant from a developers perspective but for me as a user it feels wrong, it feels too complicated, too bloated, too complex. The actual feedback is hidden very deep.
Again as a developer I can see the advantages but as a user I feel that this will dramatically reduce my working efficiency. I know that it is not "dramatically" but ease of use has always been a major asset of companion. This ease was only possible by reducing functionality and a lot of users are fine with the functionality we have. If we now introduce more functionality, we need to make sure that we don't introduce more complex workflows for everybody. This will be challenging.

For the entities so far:
The "constant" type should not be called constant. Actually it is not constant, it is a variable. It should be settable by an action. It should have the functionality of global variables with the choice of being persistent or having a initial value. I think we don't need a real constant type besides that. Many programming languages have matured over ages without a constant datatype, I think the Companion user won't miss constants.

The feedback type should be generic, not restricted to boolean feedbacks. The variable should give you whatever the feedback returnes. In case of boolean feedbacks, it will be a boolean. In case of adavanced feedbacks it will be a JSON with the combined style definition. The only thing we should make sure is that you can't mix feedback types in one entity. So the first added feedback determins the type and then you can add only feedbacks of the same type later.
That is the equivalent of what I'm saying here:

Today we have internal: Variable: Check boolean expression for boolean values. That can be generic and then you just add that feedback, enter your expression and the feedback will always fill its local variable with the result of the expression. Technically it is the same as the expression type variable but we could avoid introducing many new and different entities.

If your expression produces a boolean then yes it is the same, but for any other value it isn't the same. So it would solve some use cases (with less technical complexity, and more user clicks), but not all of the use cases.

The point is making it generic and not restricted to boolean any more. In my proposal the "expression feedback" would have exactly the same functionality like your expression entity. It just holds an expression and outputs the result to a local variable.

I have to admit that my proposal also has a big downside: I have no elegant solution of how to transfer the current workflow of stacking the feedbacks.

So after thinking about it for a day now, I have a new proposal:
Let's go on with the entities based approach. Look at this mockup:
image

In this proposal we avoid the term local variables. The users would still be find the feedbacks at the known location and they would look very similar. The key changes are:

  • Every feedback now has a name
  • The connection between a feedback and an element is controlled by referencing the name
  • If you want to stack feedbacks so that their outputs are combined like before, you have to put them in a stack

In this proposal the user's mindset would not have to adjust a lot. It is mainly the way of how a feedback relates to a visual element that changes.
The local variables are just one type of feedback then. It is a feedback that gives a value.
The "evaluate expression" feedback is the genericform of the expression type local variable.
Actually technically we turn all feedbacks into local variables and from the user perspective we add the variables to the feedbacks.
Feedbacks can live inside a stack or outside, you can always access any feedback by ist name directly or if you want to have the resulting value use the name of the stack.

The benefits are: less clicks and less overhead because every feedback automatically has a name and thus outputs to the corresponding variable. Easier transition from the current workflow. Names can be automatically generated when a feedback is added with some meaningful content.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants