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

Add new playlists API #1075

Merged
merged 12 commits into from Mar 23, 2015

Conversation

3 participants
@jodal
Member

jodal commented Mar 22, 2015

Fixes #1057

@jodal jodal added this to the v1.0 - Audio cleanup 1 milestone Mar 22, 2015

@jodal jodal force-pushed the jodal:feature/new-playlists-api branch from 483ddf5 to d37bd62 Mar 22, 2015

"""
futures = [
b.playlists.as_list()
for b in self.backends.with_playlists.values()]
results = pykka.get_all(futures)

This comment has been minimized.

@adamcik

adamcik Mar 23, 2015

Member

Should we bother with catching the potential not implemented exceptions from this in core?

This comment has been minimized.

@jodal

jodal Mar 23, 2015

Member

Currently, only M3U and Spotify implement a playlists provider, so for 1.0 migration it's not a huge issue.

More long term, making core more robust to halfdone/stupid/evil backends might be worth doing, but comes further down the list than making core more robust to halfdone/stupid/evil clients.

:rtype: list of :class:`mopidy.models.Playlist`
.. deprecated:: 1.0
Use :meth:`as_list` and :meth:`get_items` instead.

This comment has been minimized.

@adamcik

adamcik Mar 23, 2015

Member

Note that modification time is no longer being set?

This comment has been minimized.

@jodal

jodal Mar 23, 2015

Member

Fixed

if playlist is None:
return None
return [Ref.track(uri=t.uri, name=t.name) for t in playlist.tracks]
@property
def playlists(self):

This comment has been minimized.

@adamcik

adamcik Mar 23, 2015

Member

Remove the playlists property?

This comment has been minimized.

@jodal

jodal Mar 23, 2015

Member

Fixed

@adamcik

This comment has been minimized.

Member

adamcik commented Mar 23, 2015

Not a 100% sure about the names, but I think we need to keep going forward and then have @tkem, @kingosticks and potentially other client authors give this a go to verify if we are on the right track or not.

@tkem

This comment has been minimized.

Member

tkem commented Mar 23, 2015

@adamcik: agreed ;-)

jodal added some commits Mar 23, 2015

core: Doc Playlist.last_modified not being set
...if get_playlists() is called with include_tracks=False
Merge branch 'develop' into feature/new-playlists-api
Conflicts:
	docs/changelog.rst

adamcik added a commit that referenced this pull request Mar 23, 2015

Merge pull request #1075 from jodal/feature/new-playlists-api
core/backend: Add new playlists API

@adamcik adamcik merged commit dd0c86f into mopidy:develop Mar 23, 2015

3 checks passed

Scrutinizer 13 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 76.41%
Details

@jodal jodal removed the 2 - Working label Mar 23, 2015

@jodal jodal deleted the jodal:feature/new-playlists-api branch Mar 23, 2015

@tkem

This comment has been minimized.

Member

tkem commented Mar 24, 2015

Doing some initial prototyping for Mopidy-Mobile, I find that I really like playlists.as_list (I was only using the "Ref" part of Playlist anyway, anticipating things to come), but at least for now, I have little use for playlists.get_items. Reasons for the latter are:

a) The mid-term goal is to provide for "real" playlist editing. Now, as long as the "write" part of the playlists API is based on Playlist models, I'll rather get the full Playlists in the first place.

b) Having only Refs makes it somewhat awkward to add playlist items to the tracklist. I can either pass URIs to tracklist.add, or make up Track objects with name and uri.

I guess passing URIs is fine for 99% of all playlist items (and the more sensible thing to do), since metadata will be retrieved using library.lookup or extracted via gstreamer for streams, except for streams or local files (e.g. WAV) that haven't been tagged appropriately. In this case, when converting playlist items to Tracks, you at least have the name property to show up. I also find that for radio streams it takes a while for the tags to be picked up, so the initial/fake name is quite useful.

To handle b) it might also be possible for Mopidy-Mobile to check for stream items based on URI scheme (i.e. "file://", "http://", etc.), and "make up" a Track with URI and name to pass to tracklist.add, and for all other schemes only pass the URI. This also makes me wonder how to handle a future source field for playlist items, at least for M3U...

However, there's no need to delay this any further. Both methods seem to work as expected, I just wanted to share my thoughts on this after some initial "hands-on" experience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment