search

postlund / pyatv

A client library for Apple TV and AirPlay devices
https://pyatv.dev
MIT License
880 stars 95 forks source link

Refactor command handling for reusability #2238

Open michalmo opened 11 months ago

michalmo commented 11 months ago

Short feature/function description

I would like to make some of the features I added earlier this year available in Home Assistant. Some like media links have already been added, others like media player grouping are in progress, but others still don't match any Home Assistant APIs. These include account switching and text input.

It seems natural to me to extend the send_command service, which currently only supports commands from the remote control API, and has recently picked up support for returning results.

In investigating this I noticed that command handling is not done consistently in pyatv. Home Assistant's send_command service and pyatv's atvscript support only the remote control API, while atvremote includes custom parsing to support all available commands.

It seems like a good idea to refactor this command parsing and execution so that it could be reused in atvscript and in Home Assistant.

What needs to be done?

Refactor the command handling from atvremote into it's own helper, that can be re-used in atvscript and in a future PR by Home Assistant.

Is this a breaking change?

No, atvremote should work without changes, and atvscript would support additional commands.

Anything else worth knowing?

Your recent comment at https://github.com/postlund/pyatv/issues/2229#issuecomment-1750668100 suggests a similar goal.

This refactoring is useful for me as a goal to support these commands in remote.send_command in Home Assistant, using the same DSL for commands that atvremote uses. This assumes that this approach is the best way forward. I'm not sure if this has been previously considered. Should the approach be discussed first, either in this issue or in another issue?

postlund commented 11 months ago

I totally agree with refactoring command handling to share code between atvremote and atvscript. Current state is only a product of me being lazy, not doing that refactoring when adding atvscript in the first place. However... I don't believe this parsing should leak outside of pyatv. Applications built on top of pyatv should use proper APIs and implement them in a way that makes sense the that application. Doing account switching with remote_control.send_command doesn't really make sense. That should be handled through a new service, e.g. apple_tv.switch_account. Ideally there should be core concepts for doing that in Home Assistant, but I don't believe there are (and maybe never will be). Text input is a bit trickier as there's no component that matches the behavior we are after AFAIK. It's like a combination of keyboard and keyboard_remote, but not quite. It can be implemented via remote and emit events when text changes, but would ideally be handled by a more appropriate component (if it existed).

I do understand the idea and the simplicity it yields, but it only hides what is actually going on. It's like people wanting toturn off their devices using remote.send_command instead of just using media_player.turn_off (or remote.turn_off).