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

Add ability for ClimateControl Card to cycle through entity states #4142

Merged
merged 8 commits into from
Feb 22, 2024

Conversation

StopMotionCuber
Copy link
Contributor

Summary

The ClimateControl right now doesn't cycle through modes (toggle in case it's a normal heating) when clicking on it within "Device controls" on Android which means more hassle when trying to change modes, as I would have to go into the app and in general it requires more clicks.

Screenshots/Screencast

image

Screencast.from.2024-01-17.20-23-47.webm

Link to pull request in Documentation repository

None so far. If that is needed, please come back to me.

Any other notes

Didn't know whether I should create an issue for that just to have the implementation directly around and decided against that. In case I missed some workflow feel free to reach out to me. Would be happy to see this change merged

@jpelgrom
Copy link
Member

Do you actually have an example of this working? I've seen this option in the documentation before but couldn't get it to cycle/do anything with multiple modes, and your video also doesn't show anything except off/heat. If anything, it shows it not working (0:07 tap and nothing happens, needs another tap at 0:10).

@StopMotionCuber
Copy link
Contributor Author

I'm not sure about the additional tap (it was definitely necessary in this case). Probably cause the service didn't return yet or something. Anyway, pushed an additional statement that modifies the internal state directly before sending stuff to home assistant to allow for more consistent cycling.
I'm using BetterThermostat for my heater and it only exposes these 2 modes, so that's why it was only a toggle.

As my main entity has an auto mode and therefore doesn't pass the "entityShouldBePresentedAsThermostat" check, I've just mocked the code to map auto to TemperatureControlTemplate.MODE_HEAT_COOL and TemperatureControlTemplate.FLAG_MODE_HEAT_COOL and could therefore present a cycle:

screen-20240118-215608.2.mp4

I've seen this option in the documentation before but couldn't get it to cycle/do anything with multiple modes

You're probably talking about the ModeAction which is probably intended to be triggered by that > within the control element you see in the video, but I also didn't manage to find out how to interact with that. Therefore I'm using a BooleanAction instead of that ModeAction.

@jpelgrom
Copy link
Member

jpelgrom commented Jan 25, 2024

pushed an additional statement that modifies the internal state directly before sending stuff to home assistant to allow for more consistent cycling

This does result in weird looking updates with 2 transitions where state and temperature update independently. Why do this manually when all other controls depend entirely on the state to change on the server? This way the state will appear updated even if it failed.

The "Toggle climate" string in line 76 should also be translated, or use a more generic "Toggle" like already exists in the strings file.

@StopMotionCuber
Copy link
Contributor Author

pushed an additional statement that modifies the internal state directly before sending stuff to home assistant to allow for more consistent cycling

This does result in weird looking updates with 2 transitions where state and temperature update independently. Why do this manually when all other controls depend entirely on the state to change on the server? This way the state will appear updated even if it failed.

The intention was that a "double-press" would be more responsive on that, as the new state is already internal and another press would then go to the next state before the update has propagated to the server. Not sure if it actually works like that.
Regarding the weird looking transition, I've just tested to remove that instruction and the transition still looks as weird.
When using BetterThermostat this behavior doesn't happen, as it doesn't change the target temperature based on the mode.

The "Toggle climate" string in line 76 should also be translated, or use a more generic "Toggle" like already exists in the strings file.

Thanks for the hint, adapted that.
As far as I see this string is not used within the UI though (the ToggleRangeTemplate is embedded in the TemperatureControlTemplate, so all strings are set based on the latter), so the string value shouldn't matter.

@StopMotionCuber
Copy link
Contributor Author

pr_build is failing, but showing java.lang.OutOfMemoryError. I guess that's a GitHub Actions failure and unrelated to the PR itself. Builds fine on my workstation. Not sure how I can rerun the action though :/

@jpelgrom
Copy link
Member

pr_build is failing, but showing java.lang.OutOfMemoryError. I guess that's a GitHub Actions failure and unrelated to the PR itself. Builds fine on my workstation. Not sure how I can rerun the action though :/

GitHub Actions can be flaky sometimes, we can/will restart it for you in that case :)

@StopMotionCuber
Copy link
Contributor Author

@jpelgrom In general I also wanted to ask whether this PR has a good chance of being merged, once stuff is implemented properly? I feel like it's kind of a hack using a Toggle for an action that's cycling through modes, but one that enhances usability with the way android is implementing their controls right now.
You haven't given a green light on the idea of this enhancement yet, but jumped right into a review.

@jpelgrom
Copy link
Member

jpelgrom commented Feb 8, 2024

@jpelgrom In general I also wanted to ask whether this PR has a good chance of being merged

Sorry for the confusion, that was implied by reviewing the code - as far as I'm concerned this would be accepted when properly implemented and I currently don't see anything that would block it. You've described a good use case, it doesn't conflict with existing functionality and while you may consider this a hack it also doesn't feel unexpected/Android isn't exposing the modes otherwise.

We've updated the linter which might throw failures after merging this branch - if you can, please rebase your PR.

@jpelgrom
Copy link
Member

Tested and left one minor comment, otherwise it's looking good!

Do you also want to submit a documentation PR to reflect that it is not just a temperature slider, but also cycles through modes when tapped now?

@StopMotionCuber
Copy link
Contributor Author

Tested and left one minor comment, otherwise it's looking good!

Do you also want to submit a documentation PR to reflect that it is not just a temperature slider, but also cycles through modes when tapped now?

Surely can do so within the next days. I assume you mean here: https://github.com/home-assistant/companion.home-assistant/blob/master/docs/integrations/android-device-controls.md

Copy link
Member

@jpelgrom jpelgrom left a comment

Choose a reason for hiding this comment

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

Good idea, thanks for implementing it!

@jpelgrom
Copy link
Member

Surely can do so within the next days. I assume you mean here: https://github.com/home-assistant/companion.home-assistant/blob/master/docs/integrations/android-device-controls.md

Yes, that'd be the place to add it. Don't forget to add a beta label (see any recent documentation PR as an example).

@JBassett JBassett merged commit 45e0ca9 into home-assistant:master Feb 22, 2024
4 checks passed
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.

3 participants