Add Fields to Script UI #18250

karwosts · 2023-10-16T04:34:57Z

Proposed change

Add a fields editor to the UI for scripts.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New feature (thank you!)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Example configuration

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue or discussion:
Support fields & variables in script editor #13771
https://community.home-assistant.io/t/wth-are-fields-variables-in-scripts-automations-arent-included-in-the-ui/467210
Link to documentation pull request:

Checklist

The code change is tested and works locally.
There is no commented out code in this PR.
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

matthiasdebaat · 2023-10-16T08:47:50Z

Thanks! Great that you add this to the UI. Some feedback points:

Don't show Fields on default, because it's a pretty advanced option. Let's add an option in the overflow menu, like we do with modes in the automation editor.
Remove example, because it's not used.
Variable name should have an input on its own, because that can be different per user. With the order: Name, Field variable key name, Description.
Move Selector below description.
Default should render the selector.

steverep

Don't show Fields on default, because it's a pretty advanced option. Let's add an option in the overflow menu, like we do with modes in the automation editor.

@matthiasdebaat I disagree about hiding it by default. IMO, it's what makes scripts more useful and versatile than probably most users realize. That said, I agree that they seem advanced, but I think that's more due to the docs and some of the confusing terminology used (e.g. "inputs" vs. "fields", "sequence" vs. "actions", "selector" which should probably just be something like "field type", etc.). Maybe this is an opportunity to make that more user friendly rather than hiding it?

As a very simple example, I have a TTS announcement script with the string to say as input. It has logic to handle volumes, time of day to talk or be quiet, etc. So if any of my automations need to make an announcement, they just call that script rather than repeating that everywhere.

Remove example, because it's not used.

Should be removed from the docs too.

steverep · 2023-10-16T17:56:00Z

src/panels/config/script/ha-script-field-row.ts

+              .label=${this.hass.localize("ui.common.menu")}
+              .path=${mdiDotsVertical}
+            ></ha-icon-button>
+            <mwc-list-item


I think we should add a menu item to edit in Yaml mode here.

steverep · 2023-10-16T17:59:06Z

src/panels/config/script/ha-script-field-row.ts

+  },
+  {
+    name: "default",
+    selector: { object: {} },


Agree with @matthiasdebaat this should be below the selector type, and it should just render that selector.

steverep · 2023-10-16T18:06:54Z

src/panels/config/script/ha-script-field-row.ts

+  },
+  {
+    name: "selector",
+    selector: { object: {} },


I understand why, but I don't like displaying a code editor here. It makes it too complex for many users, and is unnecessary since many selectors have no options.

How about just displaying a select drop down with the different selector types? As a first cut, could leave any non-default options only available in Yaml?

Yeah I struggled a little bit with this too.

I think the problem with just a dropdown is that some selectors (like number) have mandatory options, like min/max. So you can't really just have a "default" number selector without requiring extra input.

It's almost like we want a ha-selector-selector that lets you pick a type of selector from a list, and based on what is chosen, shows an additional form of more options.

At the simplest level we could just mandate that it is a text: selector and not have any option at all, that might cover a lot of usecases. Don't know if that's too limited.

Yeah, you're right. I can try to come up with something for a selector picker as a separate PR to help.

Yeah, we do want a selector selector in the future, but with all the options this can be a lot of work.

Yeah, we do want a selector selector in the future, but with all the options this can be a lot of work.

Yup... I got maybe 100 lines into it and quickly realized that. 😓

Not sure if you were still working on this, but I did play with creating a ha-selector-selector. Styling/css is a bit rough still but it seems functional.

I don't think we need all options of all selectors to get started with it, we can just cover some of the more common ones, and have a manual yaml option for those that fall outside the initial scope.

Dunno if you were farther along than this, so do let me know if you want to drive it, otherwise I can continue fleshing this out.

I don't think the amount of work will be too bad, as we just need to define one form schema per selector type (and we can ignore many of them that don't have any options).

karwosts · 2023-10-16T19:09:09Z

Remove example, because it's not used.

Should be removed from the docs too.

What is meant exactly by "not used"? If you put a value in example, than it shows up in the example in developer tools, and it is filled when you click "Fill example data".

Do we just mean that it's very niche? Or that it is not considered to be useful? Or that it is deprecated? It still has functionality...

steverep · 2023-10-16T19:31:22Z

What is meant exactly by "not used"? If you put a value in example, than it shows up in the example in developer tools, and it is filled when you click "Fill example data".

Do we just mean that it's very niche? Or that it is not considered to be useful? Or that it is deprecated? It still has functionality...

Oh yeah I forgot about that - probably because I don't use it ever. 😏

My vote is to just plan to deprecate it, and just leave it as a Yaml-only setting for now. At the very least it should be moved to the bottom.

karwosts · 2023-10-18T04:44:48Z

Updates:

Render default with selector
Remove example
Add key as a separate field

I will point out one thing that seems slightly dangerous about rendering the selector for default is that if default has a value, and user change the selector, it's very likely to be invalid input and cause the selector to throw exception and fail to render.

I've got around this for a moment by clearing default everytime the selector is modified, though that may not be great for being user friendly. Might think about it some more if there is better way to handle.

karwosts · 2023-10-18T21:02:54Z

I think I got all the requests addressed, so flipping this out of draft.

The selector field is still a possible sore spot, not sure if its ok as-is, or if we want to see something else. Maybe enhance it in a follow up.

bramkragten · 2023-10-24T21:13:04Z

I will point out one thing that seems slightly dangerous about rendering the selector for default is that if default has a value, and user change the selector, it's very likely to be invalid input and cause the selector to throw exception and fail to render.

I've got around this for a moment by clearing default everytime the selector is modified, though that may not be great for being user friendly. Might think about it some more if there is better way to handle.

I think this needs some attention, maybe we should only clear it when the type of the selector is changed? Or... Something to think about

karwosts · 2023-10-24T21:26:39Z

I think this needs some attention, maybe we should only clear it when the type of the selector is changed? Or... Something to think about

Is there some way the form could catch if one of its children throws an exception, and clear it in that case?

bramkragten · 2023-10-24T21:36:50Z

Is there some way the form could catch if one of its children throws an exception, and clear it in that case?

We could listen for the "error" and "unhandledrejection" event, and use those?

bramkragten · 2023-10-24T21:48:06Z

I think the better option is to make our selectors better, they should handle wrong values instead if throw and not render...

bramkragten · 2023-10-24T21:55:31Z

Did a few, there might be more...

home-assistant bot added cla-signed hacktoberfest WTH Issues & PRs generated from the "Month of What the Heck?" labels Oct 16, 2023

frenck added the Noteworthy Marks a PR as noteworthy and should be in the release notes (in case it normally would not appear) label Oct 16, 2023

frenck marked this pull request as draft October 16, 2023 09:02

steverep reviewed Oct 16, 2023

View reviewed changes

karwosts marked this pull request as ready for review October 18, 2023 21:02

karwosts and others added 7 commits October 24, 2023 23:01

Add Fields to Script UI

1a91222

updates

daded7d

key error handling

e90d565

Hide by default in overflow menu, yaml editor, error handling

5a3be74

remove a translation key

017408c

Open field when adding, calculate key name from name

271284c

translate default key name, fix

c5132f1

bramkragten force-pushed the script-fields branch from f0ac87b to c5132f1 Compare October 24, 2023 21:09

bramkragten previously approved these changes Oct 24, 2023

View reviewed changes

have selectors check value types

3e70c35

bramkragten dismissed their stale review via 3e70c35 October 24, 2023 21:55

bramkragten previously approved these changes Oct 24, 2023

View reviewed changes

Merge branch 'home-assistant:dev' into script-fields

7e1ceef

fix some number selector rendering

92483de

karwosts dismissed bramkragten’s stale review via 92483de October 25, 2023 00:28

bramkragten approved these changes Oct 25, 2023

View reviewed changes

bramkragten merged commit 80112bb into home-assistant:dev Oct 25, 2023
12 checks passed

karwosts deleted the script-fields branch October 26, 2023 12:40

bramkragten mentioned this pull request Oct 27, 2023

Dashboards fail to load in iOS 15 (2023.11.0b0) #18419

Closed

3 tasks

github-actions bot locked and limited conversation to collaborators Oct 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Fields to Script UI #18250

Add Fields to Script UI #18250

karwosts commented Oct 16, 2023

matthiasdebaat commented Oct 16, 2023

steverep left a comment

steverep Oct 16, 2023

steverep Oct 16, 2023

steverep Oct 16, 2023

karwosts Oct 16, 2023

steverep Oct 16, 2023

bramkragten Oct 18, 2023

steverep Oct 18, 2023

karwosts Oct 20, 2023 •

edited

Loading

karwosts commented Oct 16, 2023

steverep commented Oct 16, 2023

karwosts commented Oct 18, 2023

karwosts commented Oct 18, 2023

bramkragten commented Oct 24, 2023

karwosts commented Oct 24, 2023

bramkragten commented Oct 24, 2023

bramkragten commented Oct 24, 2023

bramkragten commented Oct 24, 2023

Add Fields to Script UI #18250

Add Fields to Script UI #18250

Conversation

karwosts commented Oct 16, 2023

Proposed change

Type of change

Example configuration

Additional information

Checklist

matthiasdebaat commented Oct 16, 2023

steverep left a comment

Choose a reason for hiding this comment

steverep Oct 16, 2023

Choose a reason for hiding this comment

steverep Oct 16, 2023

Choose a reason for hiding this comment

steverep Oct 16, 2023

Choose a reason for hiding this comment

karwosts Oct 16, 2023

Choose a reason for hiding this comment

steverep Oct 16, 2023

Choose a reason for hiding this comment

bramkragten Oct 18, 2023

Choose a reason for hiding this comment

steverep Oct 18, 2023

Choose a reason for hiding this comment

karwosts Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

karwosts commented Oct 16, 2023

steverep commented Oct 16, 2023

karwosts commented Oct 18, 2023

karwosts commented Oct 18, 2023

bramkragten commented Oct 24, 2023

karwosts commented Oct 24, 2023

bramkragten commented Oct 24, 2023

bramkragten commented Oct 24, 2023

bramkragten commented Oct 24, 2023

karwosts Oct 20, 2023 •

edited

Loading