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

Handle sequences under parallel actions #18470

Closed
wants to merge 15 commits into from

Conversation

surfingbytes
Copy link

@surfingbytes surfingbytes commented Oct 30, 2023

Proposed change

Currently, parallel action can run only single actions in parallel. However, you may need to run sequences of actions in parallel, for example sequence of actions "Robo vac go to bathroom", "wait for mop to be replaced", "clean house" in parallel with another sequence "morning announcements".
Currently, when parallel action contains sequence, it is not possible to edit in UI. This change allows to display it. Most logic was copied from "choose action"- without conditions of course. Right before the parallel action is rendered, it converts all standalone actions into sequence, so everything is displayed the same way.

This is replacement of stale PR 14166 which is impossible to update to latest codebase.

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

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:

@karwosts
Copy link
Contributor

karwosts commented Nov 7, 2023

I think it would be helpful for review purposes if you include a screenshot of what your proposed UI looks like.

Personally I do feel like this is an important missing component in the UI editor, I was actually looking for something like this recently and didn't even realize that parallel actions supported this sequential mode. However IMO I'm not sure that making every action into a sequence is the best UI, feels a bit cluttery to me, and possibly a bit confusing, as there are now multiple "add action" buttons on the screen, and it may not be explicitly clear that these groups are executed sequentially, I didn't see any indication of "sequential" in the displayed text.

Maybe one possibility could be that when you click on "add action" in a parallel block, one of the choices in the dropdown could say "Run in sequence", and then selecting that would add the sequential action block, and leave the other items displayed as usual? Just a thought.

@surfingbytes
Copy link
Author

I'd like to add screenshots, but my docker updated and now it's not possible to run HA in dev container - see https://community.home-assistant.io/t/failing-to-build-a-dev-container/529524/2
However, it's looks very much like "Choose" action..

@surfingbytes
Copy link
Author

@karwosts , I managed to make my prod home assistant to use development frontend, and gt the screenshot from it. As you see, it's very similar to "choose" action. The main button, adding new "parallel branch", is called "Add parallel actions", while inside branch, the button is "Add action", same as everywhere else:
image

@surfingbytes
Copy link
Author

@matthiasdebaat , is there anything else needed to merge this?

@Californian

This comment was marked as off-topic.

@jmpeter

This comment was marked as off-topic.

@matthiasdebaat
Copy link
Collaborator

@matthiasdebaat , is there anything else needed to merge this?

Sorry, I was a couple of weeks offline. Thanks for making a UI for this! To fully understand it, can you share the YAML of your example?

In conditions, every option is also collapsible. Is there a reason why you didn't use that same pattern here?

CleanShot 2024-01-08 at 11 40 54@2x

@surfingbytes
Copy link
Author

surfingbytes commented Jan 8, 2024

Good idea about makingit collapsible, will look into it.

parallel:
  - sequence:
      - wait_for_trigger:
          - platform: state
            entity_id:
              - switch.hall_switch_main_door_l1
            from: "off"
            to: "on"
            for:
              hours: 0
              minutes: 0
              seconds: 0
          - platform: state
            entity_id:
              - binary_sensor.hall_entrance_door_contact
            from: "on"
            to: "off"
            for:
              hours: 0
              minutes: 0
              seconds: 0
      - if:
          - condition: state
            entity_id: switch.hall_switch_main_door_l1
            state: "on"
        then:
          - service: switch.turn_off
            data: {}
            target:
              entity_id: switch.hall_switch_main_door_l1
          - service: script.stop_automation
            data:
              automation: automation.hall_apartment_enter_leave
  - sequence:
      - delay: "00:00:01"
      - service: pyscript.check_device
        data:
          entity_id: device_tracker.tomas_phone_ip
          ip: ****
  - sequence:
      - delay: "00:00:01"
      - service: pyscript.check_device
        data:
          entity_id: device_tracker.zuzik_phone_ip
          ip: ****
  - sequence:
      - wait_for_trigger:
          - platform: state
            entity_id:
              - device_tracker.tomas_phone_ip
        timeout: "00:05:00"
        continue_on_timeout: false
      - if:
          - condition: state
            entity_id: group.apartment_members
            state: home
        then:
          - delay:
              hours: 0
              minutes: 0
              seconds: 10
              milliseconds: 0
          - service: automation.trigger
            data:
              skip_condition: true
            target:
              entity_id: automation.bathroom_washing_machine_notifications
        else:
          - if:
              - condition: state
                entity_id: binary_sensor.dishwasher_common_status_doorstate
                state: "off"
              - condition: state
                entity_id: sensor.dishwasher_operation_state
                state: BSH.Common.EnumType.OperationState.Inactive
            then:
              - service: button.press
                data: {}
                target:
                  entity_id: button.dishwasher_start_pause
                continue_on_error: true
          - delay:
              hours: 0
              minutes: 15
              seconds: 0
              milliseconds: 0
          - if:
              - condition: template
                value_template: >-
                  {{ state_attr('script.roborock_clean_apartment_once_a_day',
                  'last_triggered').date() <= now().date() - timedelta(days=1)
                  }}
            then:
              - service: script.turn_on_for_time
                data:
                  switch: switch.water_pump_strelitzia
                  time:
                    hours: 0
                    minutes: 0
                    seconds: 10
              - service: script.roborock_clean_apartment_once_a_day
                data: {}
    enabled: true

@surfingbytes
Copy link
Author

Not sure why it messes up the formatting when I put it in as "code" though...

@matthiasdebaat
Copy link
Collaborator

If you create it with a collapsible like conditions, I think it's good UI-wise.

@surfingbytes
Copy link
Author

It's now collapsible and supports other actions (re-order, rename..) same as "Choose" action. It also hs better description so now it's clear the sequences are run in parallel to each other, not the actions inside them:

image

@surfingbytes
Copy link
Author

Is there anything else needed @bramkragten @matthiasdebaat ?

@matthiasdebaat
Copy link
Collaborator

UI looks great, thanks!

@surfingbytes
Copy link
Author

Cool, can it be merged then, please?

@simonszu
Copy link

Yes please, i am very much looking forward to this feature, since i am currently migrating my automations from AppDaemon to HA native, and i would need this UI element to complete the last automation which i would need to create in YAML otherwise.

@bramkragten
Copy link
Member

I updated the PR to work with the latest updates of the automation reorder modes.

About the PR itself:

I think it is OK to add sequences to parallel actions, but I don't think this should be the default.

Parallel actions are already pretty complicated and adding another layer in the form of sequences makes it even more complicated.

I would propose to keep the default as single actions, and add a new button Add sequence next to the Add action button.

Sequences will then be supported in the UI, but the current experience is not made more complicated.

@r0bb10
Copy link

r0bb10 commented Feb 24, 2024

Sequence actions also added via UI is very welcome, i handle mine via yaml in nested parallels but having them clean via the new building block would be very nice.

@tieskuh
Copy link

tieskuh commented Mar 13, 2024

I updated the PR to work with the latest updates of the automation reorder modes.

About the PR itself:

I think it is OK to add sequences to parallel actions, but I don't think this should be the default.

Parallel actions are already pretty complicated and adding another layer in the form of sequences makes it even more complicated.

I would propose to keep the default as single actions, and add a new button Add sequence next to the Add action button.

Sequences will then be supported in the UI, but the current experience is not made more complicated.

Yes, that seems like a good solution. @surfingbytes is there any chance you are willing to implement the proposed solution?

@surfingbytes
Copy link
Author

I would propose to keep the default as single actions, and add a new button Add sequence next to the Add action button.

Sequences will then be supported in the UI, but the current experience is not made more complicated.

That is not possible to do in ha-automation-action-parallel. However, it will be possble to do by creating new class ha-automation-action-sequence, which could then be added anywhere via either "Add action" or "Add building block" -> there would be new action type "sequence". To me, it looks like a clean solution, but @bramkragten please confirm, before I start working on it. Thank you.

@surfingbytes
Copy link
Author

Thank you. I'll create this new panel type for sequence, and instead of adding it to standard action types, I'll just add new button to parallel action.

@surfingbytes
Copy link
Author

Can you please check it again @karwosts ? Parallel component is reverted to the same state as in "dev", just new "Add sequence of actions" button is added. New Sequence component is created.
Sequence

@Misiu
Copy link
Contributor

Misiu commented Apr 2, 2024

@surfingbytes can those three buttons be aligned in one line?
Something like this:
image

@surfingbytes
Copy link
Author

I tried, but as it's 2 components, it is not possible. But it's kinda like Choose action, where it has another row for "add default actions":
image

@karwosts
Copy link
Contributor

karwosts commented Apr 7, 2024

Can you please check it again @karwosts ?

You're not waiting for me, you need to wait for one of the maintainers to bless this.

I tried, but as it's 2 components, it is not possible.

I won't opine on if this is better as one row or two, but I think this could be done by adding a <slot> for a third button in ha-automation-action.ts, and then filling that slot in ha-automation-action-parallel.ts

@surfingbytes
Copy link
Author

I think this could be done by adding a <slot> for a third button in ha-automation-action.ts, and then filling that slot in ha-automation-action-parallel.ts

Good idea, thanks. Unfortunately it didn't work when I tried. Either I'm doing something wrong, or it has to do something with the fact that in this case, it would be parent adding elements to slot of it's child. The usage of slot I found in source code is always child adding elements to it's parent's slot.

Anyway, is having it in one line necessary? There are other components (Choose, If-Then) that has commands in 2 lines. If it can be left in 2 lines, is there anything else that needs to be done to get this approved?

@karwosts
Copy link
Contributor

I don't know if it's necessary, I just replied out of curiousity and a learning exercise. Anyway I think it does work, I tried like this:

in ha-automation-action:

          <div class="buttons">
            <ha-button
              outlined
              .disabled=${this.disabled}
              .label=${this.hass.localize(
                "ui.panel.config.automation.editor.actions.add"
              )}
              @click=${this._addActionDialog}
            >
              <ha-svg-icon .path=${mdiPlus} slot="icon"></ha-svg-icon>
            </ha-button>
            <ha-button
              .disabled=${this.disabled}
              .label=${this.hass.localize(
                "ui.panel.config.automation.editor.actions.add_building_block"
              )}
              @click=${this._addActionBuildingBlockDialog}
            >
              <ha-svg-icon .path=${mdiPlus} slot="icon"></ha-svg-icon>
            </ha-button>
            <slot name="extrabutton"></slot>
          </div>

In ha-automation-action-parallel

  protected render() {
    const action = this.action;

    return html`
      <ha-automation-action
        .path=${[...(this.path ?? []), "parallel"]}
        .actions=${action.parallel}
        .disabled=${this.disabled}
        @value-changed=${this._actionsChanged}
        .hass=${this.hass}
      >
            <ha-button
              slot="extrabutton"
              label="Add Sequence"
              @click=${this._testbutton}
            >
              <ha-svg-icon .path=${mdiPlus} slot="icon"></ha-svg-icon>
            </ha-button>
      </ha-automation-action>
    `;
  }

  private _testbutton(ev: CustomEvent) {
    console.log("Pressed the button");
  }

Renders like:
image

You don't have to do it this way, I just mentioned as you had said you were trying to put them on one line. Sorry review is taking a while, thanks for your patience and your contribution!

@surfingbytes
Copy link
Author

@karwosts , thank you for the example with slot. However, that i exactly what I did (and you can see it in the last commit), but not a single thing changed in UI - it's still in 2 rows. And when I put "test" string into the slot in ha-automation-action, the "test" is shown on the first line. But button remains on the second one. Really strange, but I don't know what is wrong with it.

@Misiu is this requirement to get it approved? If not, can it be approved please?

@@ -35,9 +37,33 @@ export class HaParallelAction extends LitElement implements ActionElement {
@value-changed=${this._actionsChanged}
.hass=${this.hass}
></ha-automation-action>
<ha-button
Copy link
Contributor

Choose a reason for hiding this comment

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

You have the button after the automation action, it should be inside it:

<ha-automation-action
    ...
>
   <ha-button ...></ha-button>
</ha-automation-action>

Copy link
Author

Choose a reason for hiding this comment

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

That did it! Thank you

image

Copy link

@ChrisCarini ChrisCarini left a comment

Choose a reason for hiding this comment

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

LGTM - just one small nit for grammar.

🎉 👏

Very excited for this change to land and be released! 🙏

@Misiu
Copy link
Contributor

Misiu commented Apr 24, 2024

@Misiu is this requirement to get it approved? If not, can it be approved please?

Sorry for the late reply.
I'm not the person making the approval decision, I'm just an occasional contributor.
I suggested putting everything in a single row as it looks nicer and saves space.
This was a suggestion, not an approval requirement, but I'm glad you addressed it. Nice job 👏🚀

@surfingbytes
Copy link
Author

@matthiasdebaat , if this UI is ok, could you please approve it?

image

export interface ParallelAction extends BaseAction {
parallel: ManualScriptConfig | Action | (ManualScriptConfig | Action)[];
parallel: SequenceAction[];
Copy link
Member

Choose a reason for hiding this comment

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

It should still accept config that is not in a list right?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, this was forgotten there and is now fixed.

Copy link
Author

Choose a reason for hiding this comment

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

Does this look good now @bramkragten , or do I need to fix anything more?

@frenck
Copy link
Member

frenck commented May 18, 2024

We've discussed this PR with the team, and came to the conclusion that there is a simpler way; that also address other issues.

Instead of trying to handle this for the parallel actions, the idea is to add a new building block: sequence, see:

home-assistant/core#117690

This solves a bunch of other use cases and removes almost all design decisions/exceptions we need to make. Building blocks are already generally available for the parallel action. Adding sequence to that, removes the need for extra buttons and what not.

../Frenck

@surfingbytes
Copy link
Author

Thanks @frenck . When do you think we could expect that new functionality? Also, even if it's not going forward with this PR, I think some of these changes could come handy - for example ha-automation-action-sequence.ts and script_i18n.ts - as the GUI will still be needed.

@frenck
Copy link
Member

frenck commented May 21, 2024

When do you think we could expect that new functionality?

Well, it needs frontend support; recommended would be to build this frontend feature against that branch of that PR.

lso, even if it's not going forward with this PR, I think some of these changes could come handy - for example ha-automation-action-sequence.ts and script_i18n.ts - as the GUI will still be needed.

I think we can adjust/adopt this PR instead? Would you be able to do that?

../Frenck

@frenck frenck mentioned this pull request May 26, 2024
9 tasks
@frenck
Copy link
Member

frenck commented May 26, 2024

I've opened up an alternative PR on the building block approach instead.
See #20874

@surfingbytes
Copy link
Author

I think we can adjust/adopt this PR instead? Would you be able to do that?

Oh, before I was able to get to this, I see it's already done. Big thanks!

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.

Sequence inside a parallel automation shown as unknown in visual editor