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 read-only playlist API to core and backend #1057

Closed
jodal opened this Issue Mar 19, 2015 · 14 comments

Comments

4 participants
@jodal
Member

jodal commented Mar 19, 2015

Core tasks:

  • Add core.playlists.as_list() -> list of Ref.playlist
  • Add core.playlists.get_items(playlist_uri) -> list of Ref
  • Add backend.playlists.as_list() -> list of Ref.playlist
  • Add backend.playlists.get_items(playlist_uri) -> list of Ref
  • Reimplement core.playlists.get_playlists(include_tracks=True/False) using new backend APIs.
  • Remove backend.playlists.playlists property.
  • Update M3U backend's playlist provider.

Extension tasks:

  • Update Mopidy-Spotify 1 backend's playlist provider.
  • Update Mopidy-Spotify 2 backend's playlist provider.

Open questions:

  • Better name suggestions for as_list() and get_items() are welcome.
  • Should we move the read-only part of the playlist controller/provider into library? You have to implement backend.library.lookup() anyway when adding playlist support to a backend. core.playlists could then be rebranded as "playlist storage".

@jodal jodal added the A-core label Mar 19, 2015

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

@jodal

This comment has been minimized.

Member

jodal commented Mar 19, 2015

Description updated.

@tkem

This comment has been minimized.

Member

tkem commented Mar 20, 2015

ad a) core.playlists.browse(uri=None)? If there's any need for "hierarchical" playlists, for example. If we need a flat list for MPD, it's probably better to have two separate methods, anyway...

ad b) I don't quite get the benefit of moving this to library. If I implement a playlist-only backend (for M3U, PLS, ASX, or something Mopidy-specific sqlite-based), do I really have to implement library.lookup(), too? Also, why would library.browse(SPECIAL_MAGIC_PLAYLISTS_URI), as suggested by @adamcik on IRC be preferable to a dedicated method that does "one thing"? I guess I'm missing something here...

@jodal

This comment has been minimized.

Member

jodal commented Mar 20, 2015

a) In the core-cleanup doc we speced a playlists.as_tree() companion to as_list(), slated for inclusion in 2.0. We opted for a different name than playlists.browse() I think for two reasons:

  • We want it to return the full tree structure in a single call, since we'll have much less playlists than tracks. (Exact return data structure yet to be decided.)
  • Reusing names across controllers makes them harder to talk about since you must qualify the method names with the controller name in addition to often having to qualify if you're talking about the core or backend API.
@jodal

This comment has been minimized.

Member

jodal commented Mar 20, 2015

b) This is an example of using this new API as of 1.0 with just the core.playlist changes:

playlist_refs = core.playlists.as_list()
playlist_ref = playlist_refs[0]
item_refs = core.playlists.get_items(playlist_ref.uri)
track_map = core.library.lookup(uris=[ref.uri for ref in item_refs])
tracks = [track_map[ref.uri] for ref in item_refs]

The actual Playlist model isn't part of this data flow at all.

If we also expect library.lookup() to be able to return Playlist models (like Spotify does, but M3U doesn't), this can be simplified to:

playlist_refs = core.playlists.as_list()
playlist_ref = playlist_refs[0]
playlist = core.library.lookup(playlist_ref.uri)

# The following should now hold true
assert playlist.tracks == tracks
assert playlist.last_modified is not None
@jodal

This comment has been minimized.

Member

jodal commented Mar 20, 2015

b2) The idea of maybe moving:

  • core.playlists.as_list() to core.library.get_playlists()
  • core.playlists.get_items(playlist_uri) to core.library.get_playlist_items(playlist_uri)
  • core.playlists.refresh() to core.library.refresh_playlists()

Was based on the realization that the core controllers can be described by how they are chronologically used:

  1. You get something to listen to from the library
  2. You queue it in the tracklist
  3. You listen to it with playback
  4. You archive it in the history

The playlist controller fits in the same space as the library and we thought that you would need to implement library.lookup() anyway, so maybe it should be part of the library controller?

Additionally, if we leave the playlist controller as the home of just the playlists storage API (see core-cleanup doc for current spec), we can use the presence of a backend.playlists controller to feature-detect if a backend provides playlist storage. Of course, there's other ways to signal that too.

@tkem

This comment has been minimized.

Member

tkem commented Mar 20, 2015

Hmmm... still not convinced yet ;-)

playlist = core.library.lookup(playlist_ref.uri)
assert playlist.tracks == tracks

I guess this is API v1.0 (or whatever that's called now), since library.lookup() as I know it returns a list of Refs, doesn't it?

What I wouldn't like to see is "pure-playlist" extensions having to implement a library just for this. I think that extensions that implement both library and playlist providers will actually be quite rare. There's spotify yes, but AFAIK that never got to implement the storage API, so it could as well have used browse directories for read-only user playlists. I think that seperating library and playlists as started with M3U is "a good thing(TM)", and I hope we'll see more playlist extensions if we keep things simple for backends, too. They also make for good starter projects, too, I guess.

@jodal

This comment has been minimized.

Member

jodal commented Mar 20, 2015

v1.0 is the artist formerly known as v0.20. The thing we're shipping this week :-)

We're trying to talk about "core-cleanup" instead now, to not connect it to a specific release version. The tentative plan is to land most of it in v2.0.

You're right anyway, my second example is wrong. Given our existing API and the changes in the description in the top here, it should become:

playlist_refs = core.playlists.as_list()
playlist_ref = playlist_refs[0]
tracks2 = core.library.lookup(playlist_ref.uri)

# The following should now hold true
assert tracks2 == tracks

For v1.0 we're keeping core.playlists.get_playlists(include_tracks=True/False) around to not break existing clients, but it is reimplemented on top of backend.playlists.as_list(), backend.playlists.get_items(), and backend.library.lookup(). In other words, the two backends (M3U and Spotify) using this gets the new backend playlists API without the performance-killing backend.playlists.playlists property right away.

@tkem

This comment has been minimized.

Member

tkem commented Mar 20, 2015

+1 for kicking backend.playlists.playlists, the new backend.playlists methods (whatever they're called) and keeping things backward-compatible. And moving M3U out of local, of course.

Regarding backend.library.lookup(), I'll form an opinion when I get my client developer hat back on ;-)

@jodal

This comment has been minimized.

Member

jodal commented Mar 20, 2015

I appreciate the discussion! Hoping to end up with the best possible API and migration path :-)

@tkem

This comment has been minimized.

Member

tkem commented Mar 20, 2015

I think there will be fresh insights on this as soon as clients start actually using the playlists API.
Hopefully, that shouldn't be that far from now...

@fatg3erman

This comment has been minimized.

Contributor

fatg3erman commented Mar 30, 2015

Question from a client developer:

I develop Rompr. As you may or may not know, rompr maintains a 'Music Collection' that combines all the tracks available from mopidy's library (anything that returns search results in response to an empty query) and sorts the whole thing by artist and album to provide a backend-agnostic music collection. This is a very different approach from 'file system browsing' - I don't care where the music comes from, I just want to be able to sort everything by artist and album regardless of the backend that provides the track.

In my previous release I added a feature where the collection can be updated on the fly in response to changes in the library without the user needing to do anything. Currently this only works for Spotify, but rompr responds to the playlists_loaded event from mopidy and uses core.playlists.get_playlists to get the track information. This works nicely because it means you can update your Spotify playlists using any other client and rompr's collection will update transparently.

With get_playlists being deprecated however, I no longer see a means of achieving this. The Ref models returned by as_list() and get_items() don't include enough information, so I'm left with the option of using as_list() and then iterating over the list using lookup(), which I've already tried and which is seriously messy and inefficient.

Comments, ideas? Can I have my method back? :)

@tkem

This comment has been minimized.

Member

tkem commented Mar 30, 2015

Empty queries are also deprecated in Mopidy v1.0, so this approach seems to be doomed anyway ;-)

@adamcik

This comment has been minimized.

Member

adamcik commented Mar 30, 2015

"Moving" this to #1094 instead of continuing in this closed bug.

@fatg3erman

This comment has been minimized.

Contributor

fatg3erman commented Mar 30, 2015

Empty queries are also deprecated in Mopidy v1.0, so this approach seems to be doomed anyway ;-)

Really? Well, I'm screwed then. Might as well forget it.

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