Conversation
probot-home-assistant
Bot
added
integration: sonos
small-pr
|
I guess this is a breaking change if automations rely on the state. |
|
Tagged this for 107 because it fixes Sonos with the new media control card. |
frenck
left a comment
frenck
left a comment
There was a problem hiding this comment.
Looks good to me. Comments are indeed on existing code, highlighted now because of the indent change.
I think right now, having a bug fixed/improved user experience is more important.
amelchio
left a comment
amelchio
left a comment
There was a problem hiding this comment.
Had some time to read the patch now. I am still not strongly opinionated but to me, this looks a bit like a fix in the wrong place.
| self._media_image_url = self._radio_artwork(media_info["CurrentURI"]) | ||
|
|
||
| self._media_artist = track_info.get("artist") | ||
| self._media_artist = track_info.get("artist") or None |
There was a problem hiding this comment.
Maybe it would be better to add these or None to the property getters for less duplication. We already do that for media_image_url.
There was a problem hiding this comment.
It seems like this change made the PR miss its target because the state property reads from self._media_title which is no longer set to None.
I have included a proposal to fix that in #34311 (by using self.media_title).
| def state(self): | ||
| """Return the state of the entity.""" | ||
| if self._status in ("PAUSED_PLAYBACK", "STOPPED"): | ||
| if self._media_title is not None and self._status in ( |
There was a problem hiding this comment.
I am not sure that it can fail but honestly, this makes me feel odd. It seems to conflate concerns. Maybe we paused some untagged media? Even if that cannot happen with Sonos (I am unsure), it seems like a situation the frontend should be able to handle?
There was a problem hiding this comment.
I consider it a bug in Home Assistant if a media player is claiming to have media loaded (being in a paused/playing state) and it cannot describe that media and it normally can.
Especially since Sonos does have the transitioning state to indicate that it's between songs, which we will still consider being playing.
Even if you play MP3s there is a title (the filename)
There was a problem hiding this comment.
Okay, in that case we have a bug in update_media_linein() :)
Maybe it would look less out of place as a guard.
There was a problem hiding this comment.
Oh sorry, with MP3 I meant one from a local server. You're right linein is different. Do we know the transport info if it's linein / TV ?
There was a problem hiding this comment.
With a guard, do you mean that you want to see the if be inside the other if ?
There was a problem hiding this comment.
Line-in currently has
self._media_artist = source
that we can just change to
self._media_title = source
and it is hard-coded as "TV" or "Line-in".
There was a problem hiding this comment.
I meant
if self._media_title is None:
return STATE_IDLE
but don't change it just for me, I'm fine with whatever :)
* Sonos idle * F-string * Add to properties * Fixes
6 participants
Breaking change
Sonos devices will now report
idleinstead ofpausedif they do not have any current artist metadata available. This can happen when you were playing Spotify on your Sonos and use the Spotify app to play on another device.Proposed change
If there are no music title/artist, don't store an empty string but store
Noneso the attributes are filtered out.Also make sure that if there is no media title, we don't consider ourselves paused but instead mark ourselves as idle, which is more appropriate.
For background, see #32701
Fixes #32701
Type of change
Example entry for
configuration.yaml:# Example configuration.yamlAdditional 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.The integration reached or maintains the following Integration Quality Scale: