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

Fix isActive for locks #3862

Merged
merged 2 commits into from
Sep 19, 2023
Merged

Fix isActive for locks #3862

merged 2 commits into from
Sep 19, 2023

Conversation

aronsky
Copy link
Contributor

@aronsky aronsky commented Sep 14, 2023

Summary

Lock entity should be active when locked, inactive otherwise

Screenshots

irrelevant

Link to pull request in Documentation repository

irrelevant

Any other notes

A recent PR (#3816) made changes to how different entities and their states are represented. This change caused the locks to be represented by active (on) tiles when unlocked, and inactive (off) tiles when locked. This new representation is counter-intuitive, and contradicts the lock representation in tiles provided by other home automation software (e.g. Google Home).

This PR reverts this change at its core (the logic of isActive for lock entities), while keeping the rest of the changes in #3816 intact.

Lock entity should be active when locked, inactive otherwise
@dshokouhi
Copy link
Member

If you read the actual PR #3805 we listed it as a breaking change in #3836 because this makes the app more in line with what the frontend considers as active and we should actually match the frontend here.

https://github.com/home-assistant/frontend/blob/dev/src/common/entity/state_active.ts#L40

@aronsky
Copy link
Contributor Author

aronsky commented Sep 14, 2023

I see.

And I also understand the desire to simplify code, and to be in line with what the frontend is doing. But specifically for locks, and the tiles on Android, the resulting behavior is counter-productive. As mentioned, other home automation products display lock tiles as active when they are locked. Maybe the real fix would be to change the frontend logic that you linked to, as well? I would argue that a lock's purpose is to, well, lock things - so it's active when locked, and inactive when unlocked.

@dshokouhi
Copy link
Member

Maybe the real fix would be to change the frontend logic that you linked to, as well?

Yes this would be the correct approach before we should accept this PR because we do base our logic on what HA users expect and not what the expectation is from other apps. That is also the reason why we link to frontend logic in several places so its easier for us to update and keep things in sync. This is also why we highlighted it as a breaking change as we are aware some users may not like it.

Copy link
Member

@dshokouhi dshokouhi left a comment

Choose a reason for hiding this comment

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

Just need to add a comment so we can remember why we explicitly changed the logic from the fronted.

You can also close your frontend PR too

@home-assistant home-assistant bot marked this pull request as draft September 17, 2023 18:13
@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@jpelgrom
Copy link
Member

Linked another issue to this PR, as the action will no longer be inverted if this is merged

Copy link
Member

@dshokouhi dshokouhi left a comment

Choose a reason for hiding this comment

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

Thanks for the adding the comment. As the requested changes are now completed I will put this PR ready for review so we can merge it.

@dshokouhi dshokouhi marked this pull request as ready for review September 18, 2023 13:55
@JBassett JBassett merged commit a31cd2f into home-assistant:master Sep 19, 2023
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.

Can't control locks via device controls since last update
4 participants