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

Lookup track metadata for MPD load and listplaylistinfo #1621

Merged
merged 3 commits into from Apr 19, 2018

Conversation

3 participants
@kingosticks
Member

kingosticks commented May 17, 2017

This is the fix for #1511 that I forgot to submit.

Tests currently fail due to #1619.

@@ -38,6 +37,17 @@ def listplaylist(context, name):
return ['file: %s' % t.uri for t in playlist.tracks]
def _lookup_playlist(context, name):
uri = context.lookup_playlist_uri_from_name(name)
playlist = uri is not None and context.core.playlists.lookup(uri).get()

This comment has been minimized.

@adamcik

adamcik Jun 14, 2017

Member

Would it be cleaner to spread this across normal ifs statements instead of statement and ...?

This comment has been minimized.

@kingosticks

kingosticks Jun 15, 2017

Member

I think this is OK and the same line is used throughout the file.

raise exceptions.MpdNoExistError('No such playlist')
track_uris = [track.uri for track in playlist.tracks]
tracks_map = context.core.library.lookup(uris=track_uris).get()
tracks = [track for u in track_uris for track in tracks_map[u]]

This comment has been minimized.

@adamcik

adamcik Jun 14, 2017

Member

Not a huge fan of these nested for loops like this, I tend to get confused about which one is the inner one.

This comment has been minimized.

@kingosticks

@kingosticks kingosticks force-pushed the kingosticks:fix/mpd-load-tracklist-metadata branch from a4a1c4f to e23c2e7 Jun 16, 2017

@kingosticks

This comment has been minimized.

Member

kingosticks commented Jun 16, 2017

I'm really glad you prompted to me to take another look at this. I'd managed to mess up load: it didn't need to lookup the tracks since I was already getting that for free with tracklist.add(uris=track_uris). And as part of refactoring everything to use my new _get_playlist helper I discovered a bug and lack of testing in the cases where core.playlists.save fails. I just hacked in a mechanism to force DummyPlaylistsProvider.save to fail (it's late), maybe some mocking is better? I'm all ears to a cleaner solution.

playlist = None
uri = context.lookup_playlist_uri_from_name(name)
if uri:
playlist = context.core.playlists.lookup(uri).get()

This comment has been minimized.

@adamcik

adamcik Jun 24, 2017

Member

Should we be using mopidy.core.PlaylistsController.get_items to reduce how much we lookup?

Feel free to postpone this for this PR.

This comment has been minimized.

@kingosticks

kingosticks Jun 27, 2017

Member

I'm not doing the full track lookups in here anymore, these are just refs since other than for listplaylistinfo that's all we ever want.

This comment has been minimized.

@jodal

jodal Apr 14, 2018

Member

core.playlist.lookup() returns a Playlist object with full Track objects in its Playlist.tracks field. The core playlist API seems to be missing a way to get a playlist Ref with just the name of the playlist other than using core.playlist.as_list() and filtering the returned playlist Refs by URI yourself. We should consider making an equivalent of core.playlist.lookup() to get a playlist Ref.

I see nothing wrong in the implementation of _get_playlist() in this PR. It makes nothing worse and reduces a bunch of duplication. Improving the core playlists API can come later.

👍

This comment has been minimized.

@kingosticks

kingosticks Apr 15, 2018

Member

I am completely confused regarding my original reply. I have no idea why I thought I was getting Refs from core.PlaylistsController.lookup(). @adamcik your suggestion of core.PlaylistsController.get_items() seems much better and I can only assume that's what I thought I was getting from lookup()! But then that doesn't fit with what I've done here since some of these routines do need the full Track objects and some don't. And this change makes listplaylistinfo lookup the Tracks twice which is a bit weird.

with warnings.catch_warnings():
warnings.filterwarnings('ignore', 'tracklist.add.*"tracks".*')
context.core.tracklist.add(playlist.tracks[playlist_slice]).get()

This comment has been minimized.

@adamcik

adamcik Jun 24, 2017

Member

We might as well keep adding the track ignoring warnings if we are looking up the playlist in a way which gets full tracks. Unless we switch to just getting refs like the comment above mentions.

This comment has been minimized.

@kingosticks

kingosticks Jun 27, 2017

Member

I don't understand. My track_uris here are Track Refs. Wasn't the intent here to catch deprecation warnings? Which are not longer being emitted since I'm using the uris parameter instead of tracks?

This comment has been minimized.

@jodal

jodal Apr 14, 2018

Member

The track variable here is actually full Track objects, but I guess the point of this PR is to do a new lookup of the tracks on load and listplaylistinfo in case the backend doesn't provided complete information in the returned Track objects.

👍

uri = context.lookup_playlist_uri_from_name(name)
if uri:
playlist = context.core.playlists.lookup(uri).get()
if must_exist and not playlist:

This comment has been minimized.

@jodal

jodal Jun 25, 2017

Member

s/not playlist/playlist is None/, so you don't error on empty playlists (in a future where Playlist might implement a container ABC).

@kingosticks

This comment has been minimized.

Member

kingosticks commented Apr 10, 2018

What can I do to finish this off?

@jodal

jodal approved these changes Apr 14, 2018

@@ -340,6 +397,22 @@ def test_playlistmove(self):
"dummy:c",
self.backend.playlists.get_items('dummy:a1').get()[0].uri)
def test_playlistmove_save_falis(self):

This comment has been minimized.

@jodal

jodal Apr 14, 2018

Member

s/falis/fails/

self.assertEqual(1, len(tracks))
self.assertEqual('dummy:a', tracks[0].uri)
self.assertEqual('Track A', tracks[0].name)
self.assertEqual(5000, tracks[0].length)

This comment has been minimized.

@jodal

jodal Apr 14, 2018

Member

I'd flip the arguments on the previous four lines.

playlist = None
uri = context.lookup_playlist_uri_from_name(name)
if uri:
playlist = context.core.playlists.lookup(uri).get()

This comment has been minimized.

@jodal

jodal Apr 14, 2018

Member

core.playlist.lookup() returns a Playlist object with full Track objects in its Playlist.tracks field. The core playlist API seems to be missing a way to get a playlist Ref with just the name of the playlist other than using core.playlist.as_list() and filtering the returned playlist Refs by URI yourself. We should consider making an equivalent of core.playlist.lookup() to get a playlist Ref.

I see nothing wrong in the implementation of _get_playlist() in this PR. It makes nothing worse and reduces a bunch of duplication. Improving the core playlists API can come later.

👍

with warnings.catch_warnings():
warnings.filterwarnings('ignore', 'tracklist.add.*"tracks".*')
context.core.tracklist.add(playlist.tracks[playlist_slice]).get()

This comment has been minimized.

@jodal

jodal Apr 14, 2018

Member

The track variable here is actually full Track objects, but I guess the point of this PR is to do a new lookup of the tracks on load and listplaylistinfo in case the backend doesn't provided complete information in the returned Track objects.

👍

@jodal

This comment has been minimized.

Member

jodal commented Apr 14, 2018

Also, please add a changelog entry.

@kingosticks kingosticks force-pushed the kingosticks:fix/mpd-load-tracklist-metadata branch from 08303bf to e87599a Apr 15, 2018

@kingosticks

This comment has been minimized.

Member

kingosticks commented Apr 15, 2018

Addressed comments, except the warning thing, I'm not sure what makes sense there.

@jodal jodal merged commit 2cb7993 into mopidy:develop Apr 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment