Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow sonos to select playlists as a source #8258

Merged

Conversation

Projects
None yet
@mcolyer
Copy link
Contributor

commented Jun 30, 2017

Description:

Most of this was taken from #5598 (comment) however I made a few small improvements so that it works for other services than Spotify and it should properly switch to playing the queue if you had another song playing previously.

Known Issues

I'm still struggling with how to make sure that the source is properly selected in the UI afterwards. If someone could help me out with that it would be appreciated.

Related issue: fixes #5598

Checklist:

If the code communicates with devices, web services, or third-party tools:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass

/cc @PatBoud since you did the heavy lifting here.

Allow sonos to select playlists as a source
Most of this was taken from
#5598 (comment)
however I made a few small improvements so that it works for other
services than Spotify and it should properly switch to playing the queue
if you had another song playing previously.

/cc @PatBoud
@homeassistant

This comment has been minimized.

Copy link

commented Jun 30, 2017

Hi @mcolyer,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@mention-bot

This comment has been minimized.

Copy link

commented Jun 30, 2017

@mcolyer, 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.

self._player.add_to_queue(didl)
self._player.play_from_queue(0)
else:
self._player.play_uri(src['uri'], src['meta'], src['title'])

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 30, 2017

line too long (80 > 79 characters)

res = [soco.data_structures.DidlResource(uri=src['uri'], protocol_info="DUMMY")]
didl = soco.data_structures.DidlItem(title="DUMMY", # Sonos gets the title from the item_id
parent_id="DUMMY", # Ditto
item_id=src['uri'],

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 30, 2017

continuation line under-indented for visual indent


res = [soco.data_structures.DidlResource(uri=src['uri'], protocol_info="DUMMY")]
didl = soco.data_structures.DidlItem(title="DUMMY", # Sonos gets the title from the item_id
parent_id="DUMMY", # Ditto

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 30, 2017

continuation line under-indented for visual indent

desc = root.find('item:item', namespaces).find('desc:desc', namespaces).text

res = [soco.data_structures.DidlResource(uri=src['uri'], protocol_info="DUMMY")]
didl = soco.data_structures.DidlItem(title="DUMMY", # Sonos gets the title from the item_id

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 30, 2017

at least two spaces before inline comment
line too long (111 > 79 characters)

namespaces = { 'item' : 'urn:schemas-upnp-org:metadata-1-0/DIDL-Lite/', 'desc': 'urn:schemas-upnp-org:metadata-1-0/DIDL-Lite/' }
desc = root.find('item:item', namespaces).find('desc:desc', namespaces).text

res = [soco.data_structures.DidlResource(uri=src['uri'], protocol_info="DUMMY")]

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 30, 2017

line too long (100 > 79 characters)


root = ET.fromstring(src['meta'])
namespaces = { 'item' : 'urn:schemas-upnp-org:metadata-1-0/DIDL-Lite/', 'desc': 'urn:schemas-upnp-org:metadata-1-0/DIDL-Lite/' }
desc = root.find('item:item', namespaces).find('desc:desc', namespaces).text

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 30, 2017

line too long (96 > 79 characters)

import xml.etree.ElementTree as ET

root = ET.fromstring(src['meta'])
namespaces = { 'item' : 'urn:schemas-upnp-org:metadata-1-0/DIDL-Lite/', 'desc': 'urn:schemas-upnp-org:metadata-1-0/DIDL-Lite/' }

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 30, 2017

whitespace after '{'
whitespace before ':'
line too long (148 > 79 characters)
whitespace before '}'

@homeassistant

This comment has been minimized.

Copy link

commented Jun 30, 2017

Hi @mcolyer,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

self._player.play_from_queue(0)
else:
self._player.play_uri(src['uri'], src['meta'],
src['title'])

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 30, 2017

continuation line under-indented for visual indent

namespaces).text

res = [soco.data_structures.DidlResource(uri=src['uri'],
protocol_info="DUMMY")]

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 30, 2017

line too long (84 > 79 characters)

'urn:schemas-upnp-org:metadata-1-0/DIDL-Lite/',
'desc': 'urn:schemas-upnp-org:metadata-1-0/DIDL-Lite/'}
desc = root.find('item:item', namespaces).find('desc:desc',
namespaces).text

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 30, 2017

continuation line under-indented for visual indent

root = ET.fromstring(src['meta'])
namespaces = {'item':
'urn:schemas-upnp-org:metadata-1-0/DIDL-Lite/',
'desc': 'urn:schemas-upnp-org:metadata-1-0/DIDL-Lite/'}

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 30, 2017

continuation line under-indented for visual indent


root = ET.fromstring(src['meta'])
namespaces = {'item':
'urn:schemas-upnp-org:metadata-1-0/DIDL-Lite/',

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 30, 2017

continuation line under-indented for visual indent

@homeassistant homeassistant added cla-signed and removed cla-needed labels Jun 30, 2017

@mcolyer

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2017

It seems you haven't yet signed a CLA. Please do so here.

So I've now signed the CLA but since most of this code was written by @PatBoud, I think he needs to sign as well.

'urn:schemas-upnp-org:metadata-1-0/DIDL-Lite/',
'desc': 'urn:schemas-upnp-org:metadata-1-0/DIDL-Lite/'}
desc = root.find('item:item', namespaces).
find('desc:desc', namespaces).text

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 30, 2017

unexpected indentation

namespaces = {'item':
'urn:schemas-upnp-org:metadata-1-0/DIDL-Lite/',
'desc': 'urn:schemas-upnp-org:metadata-1-0/DIDL-Lite/'}
desc = root.find('item:item', namespaces).

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 30, 2017

SyntaxError: invalid syntax

root = ET.fromstring(src['meta'])
namespaces = {'item':
'urn:schemas-upnp-org:metadata-1-0/DIDL-Lite/',
'desc': 'urn:schemas-upnp-org:metadata-1-0/DIDL-Lite/'}

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 30, 2017

line too long (89 > 79 characters)


root = ET.fromstring(src['meta'])
namespaces = {'item':
'urn:schemas-upnp-org:metadata-1-0/DIDL-Lite/',

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 30, 2017

line too long (81 > 79 characters)

'urn:schemas-upnp-org:metadata-1-0/DIDL-Lite/',
'desc': 'urn:schemas-upnp-org:metadata-1-0/DIDL-Lite/'}
desc = root.find('item:item', namespaces).find('desc:desc',
namespaces).text

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 30, 2017

line too long (83 > 79 characters)

@lwis

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2017

Although this looks wrong, I don't know how it would fit into SoCo without more changes. Their support for favourites and playlists as of today isn't great.

Also, you'll need to fix the lint errors before this can be merged.

@pvizeli

This comment has been minimized.

Copy link
Member

commented Jun 30, 2017

I think also that need to support on SoCo. I see that for next version he have a new support for favorites.

@balloob

This comment has been minimized.

Copy link
Member

commented Jul 24, 2017

There are 4 lint violations that need to be fixed.

Is this PR ok to be merged @pvizeli / @lwis ?

@mcolyer

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2017

Although this looks wrong, I don't know how it would fit into SoCo without more changes. Their support for favourites and playlists as of today isn't great.

Based on this reply I was assuming there wasn't interest in merging this. If that's not the case I can look at cleaning up the lint violations.

@lwis

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2017

Until we get better upstream support, let's move this out into its own function explaining what it's doing.

@mcolyer

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2017

let's move this out into its own function explaining what it's doing.

Done, assuming the other CI checks pass.

src['title'])

@soco_error
@soco_coordinator

This comment has been minimized.

Copy link
@mcolyer

mcolyer Jul 28, 2017

Author Contributor

@pvizeli I'm not totally sure if this decorator is necessary or not, can you 👍 or 👎?

This comment has been minimized.

Copy link
@balloob

balloob Jul 29, 2017

Member

Both decorators can be removed. This function is only called from within other functions that are already decorated.

This comment has been minimized.

Copy link
@mcolyer

mcolyer Jul 30, 2017

Author Contributor

Removed, 👍 ?


@soco_error
@soco_coordinator
def replace_queue_with_playlist(self, src):

This comment has been minimized.

Copy link
@lwis

lwis Jul 28, 2017

Contributor

Make this private (prefix with _), and add a docstring to explain why this is needed.

This comment has been minimized.

Copy link
@mcolyer

mcolyer Jul 28, 2017

Author Contributor

Added.

This comment has been minimized.

Copy link
@lwis

lwis Jul 28, 2017

Contributor

👍

@mcolyer

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2017

@balloob it looks like it should be good to merge now.

@balloob
Copy link
Member

left a comment

1 change and some linting errors left to fix. After that ok to merge.

src['title'])

@soco_error
@soco_coordinator

This comment has been minimized.

Copy link
@balloob

balloob Jul 29, 2017

Member

Both decorators can be removed. This function is only called from within other functions that are already decorated.

Amendment made.

@lwis

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2017

Some lint issues to resolve, then this can be merged.

@mcolyer

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2017

Hound says no violations found, am I missing something?

@lwis

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2017

Travis runs tox, which has found linting issues.

@mcolyer

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2017

Travis runs tox, which has found linting issues.

Ahh, didn't see that before. It should be all green now.

@balloob balloob merged commit 2b59b91 into home-assistant:dev Aug 1, 2017

4 checks passed

cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.008%) to 93.673%
Details
hound No violations found. Woof!
@balloob

This comment has been minimized.

Copy link
Member

commented Aug 1, 2017

First PR merged @mcolyer, congratulations! 🎉 🐬

@philhawthorne

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2017

@mcolyer thank you for this!

@mcolyer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2017

Of course. I'm excited to use this functionality myself in the next release.

@fabaff fabaff referenced this pull request Aug 12, 2017

Merged

0.51 #8919

dethpickle added a commit to dethpickle/home-assistant that referenced this pull request Aug 18, 2017

Allow sonos to select playlists as a source (home-assistant#8258)
* Allow sonos to select playlists as a source

Most of this was taken from
home-assistant#5598 (comment)
however I made a few small improvements so that it works for other
services than Spotify and it should properly switch to playing the queue
if you had another song playing previously.

/cc @PatBoud

* Attempt to fix style issues

* More indent changes

* Fix misplaced period

* Move playlist replacement to function

* Privatize replace_queue_with_playlist and explain

* Remove unneeded decorator

* Fix doc formatting
@ranathan14

This comment has been minimized.

Copy link

commented Aug 24, 2017

Great Work!!! Tnx

@commento

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2017

I noticed that the same problem is present also for spotify albums when the metadata contains
'object.container.album.musicAlbum'

should I create a PR for this?

@lwis

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2017

If there's a bug you have a fix for, please feel free to contribute :)

commento added a commit to commento/home-assistant that referenced this pull request Aug 30, 2017

Allow sonos to select album as a source
Importing the fix in the PR home-assistant#8258 I noticed that the same error is present also for Spotify album so I have extended the code and tested it. It works fine on my setup

@commento commento referenced this pull request Aug 30, 2017

Merged

Allow sonos to select album as a source #9221

0 of 8 tasks complete

pvizeli added a commit that referenced this pull request Aug 30, 2017

Allow sonos to select album as a source (#9221)
Importing the fix in the PR #8258 I noticed that the same error is present also for Spotify album so I have extended the code and tested it. It works fine on my setup

@home-assistant home-assistant locked and limited conversation to collaborators Dec 11, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.