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

Support in blueprint schema for input sections #110513

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

karwosts
Copy link
Contributor

@karwosts karwosts commented Feb 14, 2024

Proposed change

Add an optional section to blueprint definition called input_sections, and allow inputs to optionally attach themselves to a particular section. This information can then be used by the UI to visually group certain sets of inputs.

Authors of large blueprints sometimes claim that the current big flat list of all inputs becomes difficult to organize and can overwhelm users visually with number of options, and would like a way to organize them better. With collapsable sections, various related inputs can be visually grouped and collapsed to improve organization.

collapsible-blueprint

Type of change

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

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

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

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@home-assistant
Copy link

Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (blueprint) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of blueprint can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign blueprint Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

Copy link
Contributor

@lellky lellky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@SirGoodenough
Copy link
Contributor

SirGoodenough commented Feb 15, 2024

This would be a welcome change.
I tend to provide all the options with the knowledge this scares some people off, but also if I don't add them, some of the user base will ask.
My attempts at coding sections with fruit emojis and such are marginally effective.

I received a DM from one of my new users yesterday saying Dude, I look at this and my head explodes. I just want to do XY, what do I need to actually fill out.
Screenshot_20240215_125728

This would solve those kind of issues very well from what I see.

I would ask if not already, could the headings allow markdown, assuming that is possible?

https://community.home-assistant.io/t/is-there-a-way-to-add-headlines-separators-sections-in-blueprint-ui/674162
https://community.home-assistant.io/t/collapsible-selector-groups/634545
https://community.home-assistant.io/t/can-the-left-column-of-a-blueprint-be-configured-as-a-pull-down-menu-instead-of-a-list/497509

@karwosts
Copy link
Contributor Author

karwosts commented Feb 15, 2024

I would ask if not already, could the headings allow markdown, assuming that is possible?

This would be a detail of the ultimate frontend implementation, but yes in my example I did use the same markdown support for the headings/descriptions as is already supported for the individual items (you can see some of the headings in the .gif have emojis there). So author can choose an MDI icon, or whatever custom markdown you come up with.

@emontnemery
Copy link
Contributor

This is really awesome, we would really want this also in data entry flows 🙈
@karwosts could you explore if the implementation could be modified in a way where it could be used also for data entry flows, so we don't need to build the same thing multiple times?

@karwosts
Copy link
Contributor Author

karwosts commented Feb 15, 2024

This is really awesome, we would really want this also in data entry flows 🙈 @karwosts could you explore if the implementation could be modified in a way where it could be used also for data entry flows, so we don't need to build the same thing multiple times?

Hmm, I'm not much knowledgeabout about config flows, but I'm not sure I'm really seeing much overlap. Blueprints seem to have their entire yaml passed to the frontend, so we can add this kind of extra metadata to it, and just read those extra keys right in frontend blueprint processing logic.

In config flows, it seems to just pass a schema for a list of selectors (in code, instead of yaml). I'll imagine that something like this could maybe be added to that, but I don't think it would have any real code overlap with how blueprints are handled.

The frontend code for rendering blueprints and config flows is separate as well.

So while I can see that this might also be a nice enhancement for config flows, I'm not seeing any obvious way to get around "building the same thing multiple times".

If you have any ideas though I'm happy to explore other implementations.

@Blackshome
Copy link

Thanks @karwosts I look forward to the new feature. I am sure a lot of the community will love it. Keep up the good work you are doing 10 out of 10

Blacky

@emontnemery
Copy link
Contributor

emontnemery commented Mar 1, 2024

This is really awesome, we would really want this also in data entry flows 🙈 @karwosts could you explore if the implementation could be modified in a way where it could be used also for data entry flows, so we don't need to build the same thing multiple times?

Hmm, I'm not much knowledgeabout about config flows, but I'm not sure I'm really seeing much overlap. Blueprints seem to have their entire yaml passed to the frontend, so we can add this kind of extra metadata to it, and just read those extra keys right in frontend blueprint processing logic.

In config flows, it seems to just pass a schema for a list of selectors (in code, instead of yaml). I'll imagine that something like this could maybe be added to that, but I don't think it would have any real code overlap with how blueprints are handled.

The frontend code for rendering blueprints and config flows is separate as well.

So while I can see that this might also be a nice enhancement for config flows, I'm not seeing any obvious way to get around "building the same thing multiple times".

If you have any ideas though I'm happy to explore other implementations.

OK, I guess you're right that there's not much overlap with data entry flows (config flows, option flows, etc.).
Another case is services.yaml, where at least the yaml format would be very similar to what's proposed in this PR, but the definition of sections would happen in strings.json.

Let's go ahead with the implementation in this PR 👍

Three other comments:

  • A section with required fields should not be possible to collapse as it will make the user interface confusing.
  • Should we add an additional flag to control if a section can be collapsed, or is it enough to make sections with required fields always show?
  • Please change the name of default_collapsed to collapsed

@emontnemery
Copy link
Contributor

@karwosts is there a frontend PR already, if there is, please link it

@@ -8,5 +8,8 @@
CONF_HOMEASSISTANT = "homeassistant"
CONF_MIN_VERSION = "min_version"
CONF_AUTHOR = "author"
CONF_INPUT_SECTIONS = "input_sections"
CONF_SECTION = "section"
CONF_DEFAULT_COLLAPSED = "default_collapsed"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename this to COLLAPSED

@karwosts

This comment was marked as outdated.

@karwosts
Copy link
Contributor Author

karwosts commented Mar 2, 2024

Frontend PR:

home-assistant/frontend#19946

@emontnemery
Copy link
Contributor

This needs to be documented, probably as a new sub-page here https://www.home-assistant.io/docs/blueprint/, and maybe also with a mention in the blueprint tutorial

Copy link
Contributor

@emontnemery emontnemery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved from core point of view, can be merged when documentation PRs have been opened and linked and the frontend changes are approved.

@emontnemery emontnemery self-requested a review March 4, 2024 16:08
@emontnemery
Copy link
Contributor

emontnemery commented Mar 4, 2024

I removed my approving review since there are concerns that the proposed data format is not great for all use cases, and we may want a schema where sections also contains their inputs.

To be concrete, one concern is that if we allow this feature also for services, the section definitions will live in strings.yaml. It will then be difficult for integration authors to visualize how the service fields are presented to the user if the definitions are not in the same file.

@karwosts
Copy link
Contributor Author

karwosts commented Mar 26, 2024

karwosts what bramkragten is suggestion is to change from this:

Is this a solution you would be prepared to accept?

Logically it makes sense, practically it seems a lot more involved on the core side, I'm not that confident in my python that I could fumble my way to something that would be acceptible to review anytime soon. Maybe I'll give it a try eventually (unless there are any volunteers lurking 😁)

To something more similar to this:

Also I'm assuming that you meant to remove all the section: keys in this example, I don't think they make sense in that case.

@emontnemery
Copy link
Contributor

emontnemery commented Mar 28, 2024

Is this a solution you would be prepared to accept?

Yes

Logically it makes sense, practically it seems a lot more involved on the core side

What @bramkragten tried to say is that the resulting automation should not have the nested sections, it should be merged and flattened like this.

use_blueprint:
    path: blah.yaml
    input:
        title: "The title"
        message: "The message"
        othermessage: "Another message"

The links above show that this is already supported by ha-form. Hence, I don't think any complicated changes are needed in core, the changes should still only be in homeassistant/components/blueprint/schemas.py to allow input_section structures.

Also I'm assuming that you meant to remove all the section: keys in this example, I don't think they make sense in that case.

Yes, I've fixed the example now 👍

@karwosts
Copy link
Contributor Author

karwosts commented Mar 29, 2024

To something more similar to this:

blueprint:
  input_sections:
    section_a:
      name: "Options"
      icon: "mdi:icon"
      description: "Some description about this group of options"
      input:
        title:
          selector:
            text: {}
    section_b:
      name: "More Options"
      default_collapsed: true
      input:
        message:
          selector:
            text: {}
        othermessage:
          selector:
            text: {}

Actually I'm not sure if this schema is sufficient. How would you define a BP with a section, followed by a bare input not in a section, followed by another section? I assume that's something valid we would want to support.

Seems all inputs would need to be wrapped in a section in this schema. Or can assume that dictionary keys that contain input: are sections, and keys that do not contain input: are inputs?

E.g. maybe something like this would be a valid minimal schema with 3 inputs, (title in section foo, message is ungrouped, and othermessage in section bar)

blueprint:
  input:
    foo:
      input: 
        title:
    message:
    bar:
      input:
        othermessage:

@karwosts
Copy link
Contributor Author

karwosts commented Mar 29, 2024

Updated with a new schema, what I think is being requested. A new BP with this schema may look like this:

blueprint:
  name: Confirmable Notification with Sections
  description: >-
    A script that sends an actionable notification with a confirmation before
    running the specified action.
  domain: script
  source_url: https://github.com/home-assistant/core/blob/master/homeassistant/components/script/blueprints/confirmable_notification.yaml

  input:
    notify_device:
      name: Device to notify
      description: Device needs to run the official Home Assistant app to receive notifications.
      selector:
        device:
          integration: mobile_app
    body_section:
      input:
        title:
          name: "Title"
          description: "The title of the button shown in the notification."
          default: ""
          selector:
            text:
        message:
          name: "Message"
          description: "The message body"
          selector:
            text:
    confirm_text:
      name: "Confirmation Text"
      description: "Text to show on the confirmation button"
      default: "Confirm"
      selector:
        text:
    confirm_action:
      name: "Confirmation Action"
      description: "Action to run when notification is confirmed"
      default: []
      selector:
        action:
    dismiss_section:
      name: "Dismiss Options"
      icon: mdi:close-circle
      input:        
        dismiss_text:
          name: "Dismiss Text"
          description: "Text to show on the dismiss button"
          default: "Dismiss"
          selector:
            text:
        dismiss_action:
          name: "Dismiss Action"
          description: "Action to run when notification is dismissed"
          default: []
          selector:
            action:

image

@karwosts karwosts marked this pull request as ready for review March 29, 2024 16:59
@karwosts

This comment has been minimized.

@edwardtfn
Copy link

This looks awesome.
I believe you are keeping the collapsed: true/false option, right?
What would happen if one adds a section without any input underneath?

@karwosts
Copy link
Contributor Author

I believe you are keeping the collapsed: true/false option, right?

Yes it's still there.

What would happen if one adds a section without any input underneath?

I guess it would have a collapsible box with just the section description and no selectors. Not sure why one would do this, but it could just be a generic long-form markdown section, maybe could write a long README in the description and make it collapsible?

@edwardtfn
Copy link

Not sure why one would do this

Users always do what they are not expected to do. 😉

@@ -26,24 +26,38 @@ def blueprint_1():
)


@pytest.fixture
def blueprint_2():
@pytest.fixture(params=[False, True])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I'm doing here (I think) is for every test that uses blueprint_2, I'm creating an additional variant that uses the same blueprint, except that the inputs are put into sections.

This shouldn't functionally affect the test, so the test code should still pass equally whether the inputs are in sections or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, but for the final version of the PR, please add a new blueprint_3 or blueprint_2_sectioned instead of branching in the fixture.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I make an entirely new fixture, is it still possible to reuse the existing tests that currently consume blueprint_2? That's why I used the param so I can leverage and don't have to duplicate the tests.

I'm not so familiar with pytest so the concept of how to run a test with multiple different fixtures I haven't grasped yet.

@@ -1,6 +1,6 @@
# serializer version: 1
# name: test_extract_blueprint_from_community_topic
NodeDictClass({
dict({
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe what is happening here is the inputs used to come from the yaml loader, so they were the special class NodeDictClass, but now the inputs are put together as a dictionary manually in the inputs function, as it has to merge them together from several sections, so that's why the snapshot need to be updated.

@karwosts
Copy link
Contributor Author

I can update docs after geting a final approval on the schema.

Copy link
Contributor

@emontnemery emontnemery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome! I'd like a second pair of eyes on the suggested schema as well as the flattening (which as I understand it) now happens in core.

@@ -26,24 +26,38 @@ def blueprint_1():
)


@pytest.fixture
def blueprint_2():
@pytest.fixture(params=[False, True])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, but for the final version of the PR, please add a new blueprint_3 or blueprint_2_sectioned instead of branching in the fixture.

Comment on lines +99 to +106
inputs = {}
for key, value in self.data[CONF_BLUEPRINT][CONF_INPUT].items():
if value and CONF_INPUT in value:
for key, value in value[CONF_INPUT].items():
inputs[key] = value
else:
inputs[key] = value
return inputs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The requests was to do the flattening in frontend, now the flattening happens in core instead if I understand it correctly. Will this have any side effect on existing blueprinted automations?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reread the comment thread but I never saw a request to "do the flattering in frontend", or at least I missed that if it was implied.

I'm not quite sure what that would really mean anyway, don't I need to have a single list of inputs for core to iterate them for replacement?

I'm also not sure what the concern is here for backward compatibility, for BPs without sections (all of them today), isn't this block of code just basically a no-op?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flattening in frontend is what @bramkragten meant here: #110513 (comment) unless I misunderstand.
However, the solution you've come with seems fine too, I just want another pair of eyes on it.

@@ -83,6 +100,7 @@ def is_blueprint_instance_config(config: Any) -> bool:
str: vol.Any(
None,
BLUEPRINT_INPUT_SCHEMA,
BLUEPRINT_INPUT_SECTION_SCHEMA,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment here, or above, explaining that the correct schema is picked by the presence of an input dictionary.

homeassistant/components/blueprint/schemas.py Show resolved Hide resolved
@emontnemery emontnemery self-requested a review April 10, 2024 07:00
@bramkragten
Copy link
Member

Created a POC for expandable in data flows (config/options/repair) so we can make sure these are somewhat aligned:

https://github.com/home-assistant/core/compare/expandable-support-for-data-flows

@emontnemery
Copy link
Contributor

emontnemery commented Apr 24, 2024

Here's a draft, core-only, PR for services: #116100

homeassistant/components/blueprint/schemas.py Outdated Show resolved Hide resolved
homeassistant/components/blueprint/schemas.py Outdated Show resolved Hide resolved
homeassistant/components/blueprint/schemas.py Outdated Show resolved Hide resolved
@emontnemery
Copy link
Contributor

emontnemery commented Apr 24, 2024

Created a POC for expandable in data flows (config/options/repair) so we can make sure these are somewhat aligned:

I think it would make sense to either call the section "sections" also in the data entry flow, or use the name "expandable" also here and for services.

@karwosts
Copy link
Contributor Author

karwosts commented May 4, 2024

Erik does this have your approval? Currently says Approved & Awaiting Frontend, but I thought maybe you withdrew your approval. If you're waiting for additional reviewers the tags do not indicate that, maybe need to update tags and dismiss review?

Awaiting Frontend doesn't seem accurate as I'm waiting for core approval before going to frontend code.

@emontnemery
Copy link
Contributor

@karwosts I'm happy with your PR 👍

Before moving forward, I'd like to get some feedback from frontend developers

@bramkragten Can you confirm?

@Blackshome
Copy link

Just checking in again. Nice to know this has moved forward. Thanks @karwosts for all your work you have done not only just this feature but everything you do.

I get asked for this feature all the time.

Thanks for everyone who has worked on it....

@emontnemery
Copy link
Contributor

@karwosts There's no documentation PR yet, or it's at least not linked to this PR. Could you open one please?

@karwosts
Copy link
Contributor Author

Documentation: home-assistant/home-assistant.io#32769

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

Successfully merging this pull request may close these issues.

None yet

7 participants