Google Assistant SDK: support audio response playback #85989
Conversation
|
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
|
| @@ -22,7 +22,10 @@ | |||
| from .const import CONTENT_AUTH_EXPIRY_TIME, MediaClass, MediaType | |||
|
|
|||
| # Paths that we don't need to sign | |||
| PATHS_WITHOUT_AUTH = ("/api/tts_proxy/",) | |||
| PATHS_WITHOUT_AUTH = ( | |||
There was a problem hiding this comment.
While there is not a super high security bar here, uuid1 paths seem below the bar as my impression is those are on the easier side to predict. Is there a technical motivation for opting out of signed paths?
There was a problem hiding this comment.
I just followed what tts_proxy does.
See comment below where this constant is used:
elif parsed.path.startswith(PATHS_WITHOUT_AUTH):
# We don't sign this path if it doesn't need auth. Although signing itself can't
# hurt, some devices are unable to handle long URLs and the auth signature might
# push it over.
passI just did some tests on my setup and I don't have any such issues on any of my media players. So I removed the change in this file and also set requires_auth = True in GoogleAssistantSDKAudioView.
If anyone complains that playback doesn't work because of long URLs we can consider adding this back.
|
|
||
| audio_view = GoogleAssistantSDKAudioView(hass) | ||
| hass.http.register_view(audio_view) | ||
| hass.data[DOMAIN][entry.entry_id][DATA_AUDIO_VIEW] = audio_view |
There was a problem hiding this comment.
A more commonly seen pattern is to use something like an audio storage manager, rather than passing around references to the View directly. Other code can stick stuff in the manger and the view can interact with that.
(You could also consider having that intermediate manager thing be single object that holds the session, though not sure)
| @@ -180,6 +189,78 @@ async def test_send_text_command_expired_token_refresh_failure( | |||
| assert any(entry.async_get_active_flows(hass, {"reauth"})) == requires_reauth | |||
|
|
|||
|
|
|||
| async def test_send_text_command_media_player( | |||
There was a problem hiding this comment.
The expiration also seems like an important thing to test. Typically this is done with async_fire_time_changed . Speculation:... though you may need to use async_call_later or one of the home assistant variants to make this happen, rather than calling call later on the loop directly.
There was a problem hiding this comment.
Done.
I copied this call later on the loop directly from tts, where it's untested. I wanted to test it but I couldn't figure out how. Thanks for pointing me to the right functions!
|
Also, i'm ignoring the version bump part of this for now assuming that will get sorted out in the other PR before we merge this |
|
Thanks for your review! |
|
Since I reverted the change in media_player/browse_media.py could you fix the labels and remove core from reviewers? |
|
Actually the version bump includes a div id change (assistant-card-content instead of show_text_content). Anyway it doesn't matter much and in the other PR once we sort it out we will either remove HTML parsing or do HTML stripping instead. I prefer merging this if possible before the other PR. |
|
Gentle ping. It would be great if this could be approved and merged before the beta release. |
Proposed change
Support Google Assistant's audio response playback by adding an optional
media_playerfield ingoogle_assistant_sdk.send_text_commandservice.Underlyging library changes: tronikos/gassist_text@0.0.7...0.0.8
Type of change
Additional information
Checklist
black --fast 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..coveragerc.To help with the load of incoming pull requests: