Skip to content

Fix cover stop feature is not always available in google_assistant#106362

Closed
jbouwh wants to merge 1 commit into
devfrom
jbouwh-fix-google_assistant-cover-stop
Closed

Fix cover stop feature is not always available in google_assistant#106362
jbouwh wants to merge 1 commit into
devfrom
jbouwh-fix-google_assistant-cover-stop

Conversation

@jbouwh
Copy link
Copy Markdown
Contributor

@jbouwh jbouwh commented Dec 24, 2023

Proposed change

When a cover entity that supports the stop was exposed to google_assistant, it only worked in an assumed state, opening or closing state. In other states it would show a start button, but that does not make sense.
To determine if stop is supported is up to the integration. This PR changes the behavior of the StartStop control to always show Stop by reporting True on the isRunning state too google_assistant.

The same pattern was used for the valve implementation, so now the tests have been combined.

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

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

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
Copy Markdown
Contributor

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

Code owner commands

Code owners of google_assistant 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 google_assistant 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
Copy Markdown
Contributor

@elupus elupus left a comment

Choose a reason for hiding this comment

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

I don't like this. We should only show stop if its running or unknown. Stop make no sense otherwise. I dont understand what you are trying to fix.

@jbouwh
Copy link
Copy Markdown
Contributor Author

jbouwh commented Dec 25, 2023

I don't like this. We should only show stop if its running or unknown. Stop make no sense otherwise. I don't understand what you are trying to fix.

There are several reasons for this fix:

  • First it is to the integration to decide what to do with a stop request. The documentation says it is to stop a close, open or set_position operation. We cannot depend on closing, or opening states. If we should, then in that case the entity cover component should block calls in case the entity is not in the correct state.
  • Secondly the used StartStop treat is exposing a Start button which does not make sense:

So this PR solves showing a Start button and allows to stop an operation an all states. For the valve implementation we have followed the same pattern. Hope this clarifies what I am trying to fix here.

@elupus
Copy link
Copy Markdown
Contributor

elupus commented Dec 25, 2023

Yes, but you can still ask verbally to stop. We make use of these "is playing" and others for media players to decide what buttons to show in normal UI. It make perfectly sense for google UI to respect such things too.

If a cover is moving it should indicate running, with button showing. If its stand still, stop does not make sense so button should not be visible.

If cover uses stop for something else, that is a bug in the cover integration.

Start make a slot of sense, start opening if closed, start closing when open...

@jbouwh
Copy link
Copy Markdown
Contributor Author

jbouwh commented Dec 25, 2023

Yes, but you can still ask verbally to stop. We make use of these "is playing" and others for media players to decide what buttons to show in normal UI. It make perfectly sense for google UI to respect such things too.

If a cover is moving it should indicate running, with button showing. If its stand still, stop does not make sense so button should not be visible.

If cover uses stop for something else, that is a bug in the cover integration.

Start make a slot of sense, start opening if closed, start closing when open...

I am sorry, but I cannot follow you. It is not possible to hide buttons unfortunately, but showing start assumes that a a user can start it, which is not the case. You assumptions on what cover should supports sounds reasonable, but in fact not all devices support will that behavior in real live. This causes that a supported stop feature, is not exposed.

@elupus
Copy link
Copy Markdown
Contributor

elupus commented Dec 25, 2023

Again why is stop not exposed? You cant stop a non running cover...

Start a non running cover is just an implementation detail in the integration...

@elupus
Copy link
Copy Markdown
Contributor

elupus commented Dec 25, 2023

But maybe we are missing start feature now that i think about it in the google wrapper. It should open/close a cover based on is closed.

@jbouwh
Copy link
Copy Markdown
Contributor Author

jbouwh commented Dec 25, 2023

But maybe we are missing start feature now that i think about it in the google wrapper. It should open/close a cover based on is closed.

We could in fact call cover.toggle with start (in case the state is not opening or closing). This could also be used for valve.

@jbouwh jbouwh marked this pull request as draft December 25, 2023 12:30
@jbouwh
Copy link
Copy Markdown
Contributor Author

jbouwh commented Dec 25, 2023

Yes, but you can still ask verbally to stop. We make use of these "is playing" and others for media players to decide what buttons to show in normal UI. It make perfectly sense for google UI to respect such things too.
If a cover is moving it should indicate running, with button showing. If its stand still, stop does not make sense so button should not be visible.
If cover uses stop for something else, that is a bug in the cover integration.
Start make a slot of sense, start opening if closed, start closing when open...

I am sorry, but I cannot follow you. It is not possible to hide buttons unfortunately, but showing start assumes that a a user can start it, which is not the case. You assumptions on what cover should supports sounds reasonable, but in fact not all devices support will that behavior in real live. This causes that a supported stop feature, is not exposed.

#106378

@jbouwh
Copy link
Copy Markdown
Contributor Author

jbouwh commented Dec 25, 2023

Superseded by #106378

@jbouwh jbouwh closed this Dec 25, 2023
@jbouwh jbouwh deleted the jbouwh-fix-google_assistant-cover-stop branch December 25, 2023 14:42
@github-actions github-actions Bot locked and limited conversation to collaborators Dec 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants