Generic open/close handler (former cover) and HomeAssistant GUI loader

MycroftAI / skill-homeassistant

Mycroft Skill/Integration for Homeassistant

GNU Lesser General Public License v3.0

115 stars 62 forks source link

Generic open/close handler (former cover) and HomeAssistant GUI loader #91

Closed pfefferle closed 2 years ago

pfefferle commented 2 years ago

Description

I reworked the cover implementation similar to the on/off handler, to have a generic open/close handler
rewrote the cover implementation
added HomeAssistant GUI loader

Type of PR

[ ] Bugfix
[X] Feature implementation
[X] Refactor of code (without functional changes)
[ ] Documentation improvements
[ ] Test improvements

Testing

You can now say:

"open home assistant" to open the web frontend of HomeAssistant
"open {Cover Entity}" to open cover

pfefferle commented 2 years ago

@stratus-ss can you re-check the cover implementation and try to load home assistant on your screen?

pfefferle commented 2 years ago

The new structure makes it easier to also handle locks and other on/off devices.

stratus-ss commented 2 years ago

@stratus-ss can you re-check the cover implementation and try to load home assistant on your screen?

I'll have to dust off my Mark II, it got put away when I moved houses. I scanned the code and nothing jumps out at me.

I'm going to say LGTM but pending testing

pfefferle commented 2 years ago

@stratus-ss I added an acknowledge sound to open/close actions for Home Assistant GUI and an error message, if no screen is connected.

stratus-ss commented 2 years ago

My garage doors do open and close as expected

pfefferle commented 2 years ago

Error messages need to be cleaned up to match expected results. It would be better if the error message kicks back "cannot open {entity} it is not a cover/blind" or something similar

@stratus-ss I understand your point and I do agree, that it is not ideal, but this is a problem by the design of this skill. the skill queries only entities that are matching the requested usecase. For open/close it only queries covers for example: https://github.com/MycroftAI/skill-homeassistant/pull/91/files#diff-8e2c1f727276d30ced72fa67cb848772a9802162bec39b42a8df5359fd063a4cR409.

So the error message is correct, because it can't find lights or wall-sockets. I can change the error to "the device has no open functionality" but then it would alway say that, even if there is no device with that name and that would be inconsistant with the other entity error messages.

Perhaps we should file an issue for that, to try to fix that for the whole skill?

pfefferle commented 2 years ago

I checked the code and the problem is here: https://github.com/MycroftAI/skill-homeassistant/blob/20.08/__init__.py#L113

To fix that, we have to re-check the entity with an empty domain, to see if there is no such entity, or if the entity simply does not support the requested action.

@stratus-ss I would prefer to do that in a different PR, because it affects all actions.

stratus-ss commented 2 years ago

I checked the code and the problem is here: https://github.com/MycroftAI/skill-homeassistant/blob/20.08/__init__.py#L113

To fix that, we have to re-check the entity with an empty domain, to see if there is no such entity, or if the entity simply does not support the requested action.

@stratus-ss I would prefer to do that in a different PR, because it affects all actions.

After reviewing this I am inclined to agree with you. This should be a different PR because of the scope change. Thanks for pointing that out