Check if media commands are actually applicable#7595
Conversation
|
@Juggels, thanks for your PR! By analyzing the history of the files in this pull request, we identified @pvizeli, @rhooper and @bjarniivarsson to be potential reviewers. |
There was a problem hiding this comment.
I am kind of reluctant to include business logic. What does Sonos do when you call play and there is no playlist size?
There was a problem hiding this comment.
Understandable, as I felt the same way at first.
The Sonos controller just says "No music", and the play/pause button does nothing on the surface. From a UPnP standpoint, it's valid for the SoCo package to throw the 701 error as playing or pausing, etc. on an empty playlist is an invalid state transition (because there is nothing to play or pause). So the options were to try/catch the 701 error everywhere, or control the play/pause support based on playlist size.
There was a problem hiding this comment.
If we start catching 701 errors we wouldn't have to implement any business logic and just make sure we don't log them. That sounds more maintainable. Is there any added value from going down the business logic route?
There was a problem hiding this comment.
There is not necessarily any added value I think. I figured this would be a bit more clean, as this way the logic for when play/pause/etc is supported is explicit. Also I wasn't sure where I would best implement the catch statements. I guess that would be in the self.media_pause() (etc) functions?
There was a problem hiding this comment.
Can you do something similar to the soco_error decorator? https://github.com/Juggels/home-assistant/blob/db8414048ee85cb63ac388b3f8a4b571a2f9074f/homeassistant/components/media_player/sonos.py#L248-L259
There was a problem hiding this comment.
I'll give it a go :)
btw, just noticed that the macOS Sonos Controller app also disables the play/pause button upon an empty playlist (apparently the iOS app doesn't, that's where I checked earlier)
|
Processed requested changes in latest commit |
There was a problem hiding this comment.
Add this to the finally block from try…catch so that it's guaranteed to enable logging after it runs.
There was a problem hiding this comment.
you can just return the value once you have added the finally block 👍
There was a problem hiding this comment.
There is no reason to support an errorcodes being None
- Explicitly allow ‘stop’ and ‘play’ on radio streams - Disallow media commands when the playlist is empty - Check if command is supported when calling `turn_on` and `turn_off`
Clean up soco_filter_upnperror and fix small bug in support_previous_track determination
|
Changes processed 😄 |
ghost
added
5 participants
Description:
stopandplayon radio streamsplay,stoporpausean empty playlist)turn_onandturn_offImplementing these checks prevents the UPnP Error 701 (Transition not available)
Related issue (if applicable): fixes #4816
Checklist:
If the code communicates with devices, web services, or third-party tools:
toxrun successfully. Your PR cannot be merged unless tests pass