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

Modify/Update UI-widget parameters from external #833

Open
5 of 11 tasks
xX-Nexus-Xx opened this issue May 7, 2024 · 10 comments
Open
5 of 11 tasks

Modify/Update UI-widget parameters from external #833

xX-Nexus-Xx opened this issue May 7, 2024 · 10 comments
Assignees
Labels
epic A significant feature or piece of work that doesn't easily fit into a single release feature-request New feature or request that needs to be turned into Epic/Story details size:L - 5 Sizing estimation point

Comments

@xX-Nexus-Xx
Copy link

xX-Nexus-Xx commented May 7, 2024

Description

Hi,

in DB1 it was possible to modify/update UI-widget parameters via input (e.g msg.Range.min,msg.Range.max,msg,label for UI-Slider range).

Would it be possible to have the same flexibility with the new UI-widgets in DB2?

Below UI-Slider is just an example (and the one I need most) but the feature request wuld apply to all the UI-widgets with its parameters

image

thanks for considering

Have you provided an initial effort estimate for this issue?

I have provided an initial effort estimate

Tasks

  1. area:docs size:S - 2 task
    joepavitt
  2. 4 of 4
    size:S - 2 task
    joepavitt
  3. 4 of 4
    size:S - 2 task
    joepavitt
  4. 3 of 3
    size:S - 2 task
    joepavitt
  5. feature-request size:S - 2
    joepavitt
  6. Dashboard 1.0 feature-request
    joepavitt
@xX-Nexus-Xx xX-Nexus-Xx added feature-request New feature or request that needs to be turned into Epic/Story details needs-triage Needs looking at to decide what to do labels May 7, 2024
@bartbutenaers
Copy link
Contributor

Hi @joepavitt,
If you want I can have a look at this one.
Unless you need it fast, because that won't fit into my limited free time...

  1. Are there any best practices I need to take into account?
  2. I assume the dropdown node is again the best reference example for this?
  3. And I assume this is not limited to the slider node? If so, it might be better to create a PR per ui node?

@joepavitt
Copy link
Collaborator

And I assume this is not limited to the slider node? If so, it might be better to create a PR per ui node?

We have a considerable backlog of dynamic properties that are needed - I may try and dedicate a day this week you just this.

I should be able to cover it, thanks for the offer @bartbutenaers

@joepavitt joepavitt self-assigned this May 14, 2024
@joepavitt joepavitt added epic A significant feature or piece of work that doesn't easily fit into a single release size:L - 5 Sizing estimation point and removed needs-triage Needs looking at to decide what to do labels May 15, 2024
@joepavitt
Copy link
Collaborator

joepavitt commented May 16, 2024

Had already started work on dynamic props, with several PRs open following Pattern 1 from the below list. However, @Steve-Mcl has raised very valid concerns on this pattern, and he I are are currently talking through the options we have on architecture here:

Pattern 1 - <msg.property>

  • Any dynamic properties can be overridden with msg.<property>
  • This occurs even if a value was defined in the node configuration
    • Worth noting this is against the NR standard, which will only override, if a field is empty

Pros:

  • Easy to follow and understand
  • Easy for the user to define
  • Easy to implement from a development standpoint

Cons:

  • msg objects passed through nodes could then have a knock on effect, e.g. msg.label could end up running through many ui- nodes and override their label property
  • msg.<property> is ambiguous in that it could easily overlap accidentally with other, valid, uses of that same property
  • Invisible "magic" property, not in the configuration explicitly

Pattern 2 - <msg.ui_control.property>

  • Dashboard 1.0's method
  • Similar to pattern 1, but nests the overrides underneath a .ui (or equivalent) to minimise accidental overlap with valid uses of the existing msg properties

Pros:

  • Fixes 1 x on from Pattern 1

Cons:

  • Nested properties are likely to be more complex for low-code users to understand
  • Knock-on effect problem still persists
  • Invisible magic still persists

Pattern 3 - Typed Inputs

  • Utilise Node-RED's built-in "TypedInput", which permits a user to define where the property is defined (e.g. static string or dynamic msg property)

Pros:

  • Clear visibility on the definition of the property
  • Less likely to have knock-on effect to other nodes

Cons:

  • Developing with TypedInputs is not a fun DX
  • Can't set "default" value easily, if you want something driven by msg later, it also needs to be driven by msg at the start

Pattern 4 - Payload/Topic Pairing

  • msg.topic defines the property, msg.payload defines the value
  • Send this to a ui-control node, rather than the node itself

Pros:

  • No risk of knock-on effect

Cons:

  • User needs to find widget/node id in order to define the specific node in question which isn't low-code friendly
  • End up with a lot of wires into a single ui-control
  • More difficult to track back changes to a node given that the event updating it wires to a completely different node

@joepavitt
Copy link
Collaborator

I am swaying towards Pattern 3 - but I'm not sure I have the energy to implement 50+ TypedInputs

@knolleary
Copy link
Member

Given not all properties are suitable for a TypedInput style widget, I'd suggest starting with Pattern 2 - as it will be familiar to existing Dashboard users. It won't preclude Pattern 3 in the future if that's deemed a better solution later.

The knock-on effect could be mitigated by having a standard of clearing that property before passing the message on - they are single-use properties.

@joepavitt
Copy link
Collaborator

The knock-on effect could be mitigated by having a standard of clearing that property before passing the message on - they are single-use properties.

I was in favour of this also

@dceejay
Copy link

dceejay commented May 16, 2024

Obviously I'm biased - but yes... the reasons we (I) went for pattern 2 (msg.ui_control.***) in dashboard 1 was precisely so it was obvious that it was to do with ui and not just picking up generic properties (like msg.label).

And also ... as you pointed out it is an anti pattern to the default Node-RED behaviour where if the author sets a config value then that is honoured first - and can't be overridden. So by using a sub-property I could justify to myself that this was safe enough to avoid users doing it accidentally.

The reason I wanted to allow this override "all over the UI" was that for the config I thought that in general you would want to be able to set a default value using the config field - but be able to override it later if necessary - rather then having to leave something blank and then having to send a valid property value with every message - which could make every single data point into a much larger object. IE not have to send min, max, label etc with every point...

(The technical debt of this is - as someone else has pointed out recently - that in dashboard 1 we only ever retained the payload value over a refresh - so labels, max, min etc would all reset to default on a refresh (or even on a navigate away to another tab and back) - which meant they have to resend those extra control properties when they spot the fresh happening... which is currently less than ideal)

@joepavitt
Copy link
Collaborator

(The technical debt of this is - as someone else has pointed out recently - that in dashboard 1 we only ever retained the payload value over a refresh - so labels, max, min etc would all reset to default on a refresh (or even on a navigate away to another tab and back) - which meant they have to resend those extra control properties when they spot the fresh happening... which is currently less than ideal)

For what it's worth, I do already have a workaround for this as I have a server-side state store which retains the dynamic values, and populates them on a client refresh, etc.

It does however get lost on a Node-RED restart

@dceejay
Copy link

dceejay commented May 16, 2024

Probably fair enough to lose it on a restart - as the rest of the back end may have missed other inputs, and not be in a real/valid state at that point. If you can retain things over a browser refresh then you are already in a better place than dashboard 1.

@joepavitt
Copy link
Collaborator

Taking into account the above, going to move forward with a msg.ui_update.<property> approach. Chosen update instead of control to differentiate from the control node, which this doesn't use at all.

The next question is whether I apply this at core such that it auto-applies to every node and for every property (which some exceptions like id, width, height that I'll throw in). The disadvantage of this is that it will also automatically apply to all third-party nodes, and possibly for properties we don't want to be dynamic. The other problem is that there is still front-end work required to handle the properties, and so, by auto-implementing the server-side, but no the front-end, it will likely result in some broken dashboards.

My thinking is, stick with msg.ui_update, and then move forward with the existing approaches/PRs that update this on a widget-by-widget and property-by-property basis

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
epic A significant feature or piece of work that doesn't easily fit into a single release feature-request New feature or request that needs to be turned into Epic/Story details size:L - 5 Sizing estimation point
Projects
Status: In Progress
Development

No branches or pull requests

5 participants