Add androidtv_remote.send_text action #139033
Conversation
|
Hey there @Drafteed, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
tronikos
changed the title
| platform = entity_platform.async_get_current_platform() | ||
| platform.async_register_entity_service( | ||
| SERVICE_SEND_TEXT, | ||
| {vol.Required(ATTR_TEXT): cv.string}, | ||
| "send_text", | ||
| ) |
There was a problem hiding this comment.
I am wondering if this service isn't better suited for just a normal integration service, rather than an entity service. Since in that case you would pick either the config entry or the device, WDYT?
There was a problem hiding this comment.
I think it would be more confusing to users since they won't know they can either specify config entry or entity without looking at the documentation and they will end up specifying both. The code will also be more complicated to handle the different selections. Searching for ConfigEntrySelector I only found 12 usages. None of them seem to support config entry or entity/device as target. Anyway it seems we could do what you suggested at any time in the future without being a breaking change so I'd rather stick to the simpler approach for now.
There was a problem hiding this comment.
I dont mean to pick both, i just mean to pick a device, rather than an entity. Changing this will be a breaking change as users have to change their automations
There was a problem hiding this comment.
Maybe instead of an action, it would be worth considering introducing a text entity that could also display the current entered text on TV, like the official app does.
There was a problem hiding this comment.
@joostlek I guess I misunderstood what you are proposing. I personally don't like using a config entry or device in my automations since they are IDs that can change if you remove and re-add the device. I always like using entity IDs. What would the benefit be of your proposal?
@Drafteed the library doesn't support yet getting the current entered text. Somebody needs to reverse engineer that and implement it. I don't have such plans
There was a problem hiding this comment.
That sounds good to me, although input text and Lit_ are features of the underlying libraries used rather than Home Assistant design decisions. Mimicking that in the Android TV Remote component wouldn't be a bad idea.
You could also consider using the non-target device field, which seems to be used for IR remotes but not for media platform integrations that use the remote.send_command service.
action: remote.send_command
data:
command: "{{ text }}"
device: keyboard
target:
entity_id: remote.living_room_tvAs someone who hasn't done much work on Home Assistant core, what is the difference between a service that runs on the integration vs entity vs config entry vs device? Is it the target? Service name? I've seen integration/entity/device/config entry service thrown around in this comment thread but don't fully understand the differences between them.
There was a problem hiding this comment.
I switched to an integration service in this PR. So this now has to be called as:
action: androidtv_remote.send_text
data:
config_entry_id: 0adab1e29ef310f1d2a97849c911dddd
text: hiAs an alternative I have #142438. With that you'll call it with:
action: remote.send_command
data:
device: keyboard
command: hi
target:
entity_id: remote.living_room_tvI prefer the second option.
There was a problem hiding this comment.
I also STRONGLY prefer the second option. I can't even think of any services that act on config entry IDs like the first one does. Definitely nothing related to Android TV or any other media platform.
There was a problem hiding this comment.
There are a few services that act on config entries but all the ones I'm aware of are for integrations that either don't have entities or the services aren't meant to be acted on their entities.
@joostlek you said:
Entity actions should only be used in specific cases where it fits the integration architecture, mostly because they are hard to validate when the device is offline, which leaves a subpar user experience
I think using the existing remote.send_command and device: keyboard better fits the remote platform. As I said earlier you will use this either from a custom card (such as Nerwyn's card) or from an automation that opens an app, sends buttons to navigate to search, sends back to close the keyboard (if your device pops it up), sends text as keyboard input, sends enter, sends buttons to navigate to the results. All these actions are performed on the remote entity and not the media player entity.
Also, as you can see in #142438 we do know when the device is offline as it returns ConnectionClosed.
There was a problem hiding this comment.
FYI, all the 4 options implemented (at the latest or a previous commit) in the 2 PRs are in https://gist.github.com/tronikos/e5ea2c95c0300cfa1aeb2d4f5c9a2e90
I'd prefer 1 since it makes automations much easier but I'd be fine with either 2 or 3. I really don't want the 4, the one with the unreadable config_entry_id.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
…kos/home-assistant-core into androidtv_remote_send_text
Breaking change
Proposed change
Add
androidtv_remote.send_textaction for sending text to an Android TV device.Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: