Merge pull request #1621 from kingosticks/fix/mpd-load-tracklist-metadata

Lookup track metadata for MPD load and listplaylistinfo

docs/changelog.rst

@@ -40,6 +40,9 @@ Feature release.
 - MPD: Added Unix domain sockets for binding MPD to.
   (Fixes: :issue:`1531`, PR: :issue:`1629`)
 
+- MPD: Lookup track metadata for MPD ``load`` and ``listplaylistinfo``.
+  (PR:1621 :issue:`1511`)
+
 - Ensure that decoding of OS errors with unknown encoding never crashes, but
   instead replaces unknown bytes with a replacement marker. (Fixes:
   :issue:`1599`)

mopidy/mpd/protocol/stored_playlists.py

@@ -3,7 +3,6 @@ from __future__ import absolute_import, division, unicode_literals
 import datetime
 import logging
 import re
-import warnings
 
 from mopidy.compat import urllib
 from mopidy.mpd import exceptions, protocol, translator
@@ -16,6 +15,16 @@ def _check_playlist_name(name):
         raise exceptions.MpdInvalidPlaylistName()
 
 
+def _get_playlist(context, name, must_exist=True):
+    playlist = None
+    uri = context.lookup_playlist_uri_from_name(name)
+    if uri:
+        playlist = context.core.playlists.lookup(uri).get()
+    if must_exist and playlist is None:
+        raise exceptions.MpdNoExistError('No such playlist')
+    return playlist
+
+
 @protocol.commands.add('listplaylist')
 def listplaylist(context, name):
     """
@@ -31,10 +40,7 @@ def listplaylist(context, name):
         file: relative/path/to/file2.ogg
         file: relative/path/to/file3.mp3
     """
-    uri = context.lookup_playlist_uri_from_name(name)
+    playlist = _get_playlist(context, name)
-    playlist = uri is not None and context.core.playlists.lookup(uri).get()
-    if not playlist:
-        raise exceptions.MpdNoExistError('No such playlist')
     return ['file: %s' % t.uri for t in playlist.tracks]
 
 
@@ -52,10 +58,13 @@ def listplaylistinfo(context, name):
         Standard track listing, with fields: file, Time, Title, Date,
         Album, Artist, Track
     """
-    uri = context.lookup_playlist_uri_from_name(name)
+    playlist = _get_playlist(context, name)
-    playlist = uri is not None and context.core.playlists.lookup(uri).get()
+    track_uris = [track.uri for track in playlist.tracks]
-    if not playlist:
+    tracks_map = context.core.library.lookup(uris=track_uris).get()
-        raise exceptions.MpdNoExistError('No such playlist')
+    tracks = []
+    for uri in track_uris:
+        tracks.extend(tracks_map[uri])
+    playlist = playlist.replace(tracks=tracks)
     return translator.playlist_to_mpd_format(playlist)
 
 
@@ -134,14 +143,9 @@ def load(context, name, playlist_slice=slice(0, None)):
     - MPD 0.17.1 does not fail if the specified range is outside the playlist,
       in either or both ends.
     """
-    uri = context.lookup_playlist_uri_from_name(name)
+    playlist = _get_playlist(context, name)
-    playlist = uri is not None and context.core.playlists.lookup(uri).get()
+    track_uris = [track.uri for track in playlist.tracks[playlist_slice]]
-    if not playlist:
+    context.core.tracklist.add(uris=track_uris).get()
-        raise exceptions.MpdNoExistError('No such playlist')
-
-    with warnings.catch_warnings():
-        warnings.filterwarnings('ignore', 'tracklist.add.*"tracks".*')
-        context.core.tracklist.add(playlist.tracks[playlist_slice]).get()
 
 
 @protocol.commands.add('playlistadd')
@@ -156,8 +160,7 @@ def playlistadd(context, name, track_uri):
         ``NAME.m3u`` will be created if it does not exist.
     """
     _check_playlist_name(name)
-    uri = context.lookup_playlist_uri_from_name(name)
+    old_playlist = _get_playlist(context, name, must_exist=False)
-    old_playlist = uri is not None and context.core.playlists.lookup(uri).get()
     if not old_playlist:
         # Create new playlist with this single track
         lookup_res = context.core.library.lookup(uris=[track_uri]).get()
@@ -227,8 +230,7 @@ def playlistclear(context, name):
     The playlist will be created if it does not exist.
     """
     _check_playlist_name(name)
-    uri = context.lookup_playlist_uri_from_name(name)
+    playlist = _get_playlist(context, name, must_exist=False)
-    playlist = uri is not None and context.core.playlists.lookup(uri).get()
     if not playlist:
         playlist = context.core.playlists.create(name).get()
 
@@ -236,7 +238,7 @@ def playlistclear(context, name):
     playlist = playlist.replace(tracks=[])
     if context.core.playlists.save(playlist).get() is None:
         raise exceptions.MpdFailedToSavePlaylist(
-            urllib.parse.urlparse(uri).scheme)
+            urllib.parse.urlparse(playlist.uri).scheme)
 
 
 @protocol.commands.add('playlistdelete', songpos=protocol.UINT)
@@ -249,10 +251,7 @@ def playlistdelete(context, name, songpos):
         Deletes ``SONGPOS`` from the playlist ``NAME.m3u``.
     """
     _check_playlist_name(name)
-    uri = context.lookup_playlist_uri_from_name(name)
+    playlist = _get_playlist(context, name)
-    playlist = uri is not None and context.core.playlists.lookup(uri).get()
-    if not playlist:
-        raise exceptions.MpdNoExistError('No such playlist')
 
     try:
         # Convert tracks to list and remove requested
@@ -266,7 +265,7 @@ def playlistdelete(context, name, songpos):
     saved_playlist = context.core.playlists.save(playlist).get()
     if saved_playlist is None:
         raise exceptions.MpdFailedToSavePlaylist(
-            urllib.parse.urlparse(uri).scheme)
+            urllib.parse.urlparse(playlist.uri).scheme)
 
 
 @protocol.commands.add(
@@ -290,10 +289,7 @@ def playlistmove(context, name, from_pos, to_pos):
         return
 
     _check_playlist_name(name)
-    uri = context.lookup_playlist_uri_from_name(name)
+    playlist = _get_playlist(context, name)
-    playlist = uri is not None and context.core.playlists.lookup(uri).get()
-    if not playlist:
-        raise exceptions.MpdNoExistError('No such playlist')
     if from_pos == to_pos:
         return  # Nothing to do
 
@@ -310,7 +306,7 @@ def playlistmove(context, name, from_pos, to_pos):
     saved_playlist = context.core.playlists.save(playlist).get()
     if saved_playlist is None:
         raise exceptions.MpdFailedToSavePlaylist(
-            urllib.parse.urlparse(uri).scheme)
+            urllib.parse.urlparse(playlist.uri).scheme)
 
 
 @protocol.commands.add('rename')
@@ -325,21 +321,14 @@ def rename(context, old_name, new_name):
     _check_playlist_name(old_name)
     _check_playlist_name(new_name)
 
-    old_uri = context.lookup_playlist_uri_from_name(old_name)
+    old_playlist = _get_playlist(context, old_name)
-    if not old_uri:
-        raise exceptions.MpdNoExistError('No such playlist')
 
-    old_playlist = context.core.playlists.lookup(old_uri).get()
+    if _get_playlist(context, new_name, must_exist=False):
-    if not old_playlist:
-        raise exceptions.MpdNoExistError('No such playlist')
-
-    new_uri = context.lookup_playlist_uri_from_name(new_name)
-    if new_uri and context.core.playlists.lookup(new_uri).get():
         raise exceptions.MpdExistError('Playlist already exists')
     # TODO: should we purge the mapping in an else?
 
     # Create copy of the playlist and remove original
-    uri_scheme = urllib.parse.urlparse(old_uri).scheme
+    uri_scheme = urllib.parse.urlparse(old_playlist.uri).scheme
     new_playlist = context.core.playlists.create(new_name, uri_scheme).get()
     new_playlist = new_playlist.replace(tracks=old_playlist.tracks)
     saved_playlist = context.core.playlists.save(new_playlist).get()
@@ -377,8 +366,7 @@ def save(context, name):
     """
     _check_playlist_name(name)
     tracks = context.core.tracklist.get_tracks().get()
-    uri = context.lookup_playlist_uri_from_name(name)
+    playlist = _get_playlist(context, name, must_exist=False)
-    playlist = uri is not None and context.core.playlists.lookup(uri).get()
     if not playlist:
         # Create new playlist
         _create_playlist(context, name, tracks)
@@ -388,4 +376,4 @@ def save(context, name):
         saved_playlist = context.core.playlists.save(new_playlist).get()
         if saved_playlist is None:
             raise exceptions.MpdFailedToSavePlaylist(
-                urllib.parse.urlparse(uri).scheme)
+                urllib.parse.urlparse(playlist.uri).scheme)

tests/dummy_backend.py

@@ -102,11 +102,15 @@ class DummyPlaylistsProvider(backend.PlaylistsProvider):
     def __init__(self, backend):
         super(DummyPlaylistsProvider, self).__init__(backend)
         self._playlists = []
+        self._allow_save = True
 
     def set_dummy_playlists(self, playlists):
         """For tests using the dummy provider through an actor proxy."""
         self._playlists = playlists
 
+    def set_allow_save(self, enabled):
+        self._allow_save = enabled
+
     def as_list(self):
         return [
             Ref.playlist(uri=pl.uri, name=pl.name) for pl in self._playlists]
@@ -137,6 +141,9 @@ class DummyPlaylistsProvider(backend.PlaylistsProvider):
             self._playlists.remove(playlist)
 
     def save(self, playlist):
+        if not self._allow_save:
+            return None
+
         old_playlist = self.lookup(playlist.uri)
 
         if old_playlist is not None:

tests/mpd/protocol/test_stored_playlists.py

@@ -46,6 +46,10 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
         self.assertInResponse('OK')
 
     def test_listplaylistinfo(self):
+        tracks = [
+            Track(uri='dummy:a', name='Track A', length=5000),
+        ]
+        self.backend.library.dummy_library = tracks
         self.backend.playlists.set_dummy_playlists([
             Playlist(
                 name='name', uri='dummy:name', tracks=[Track(uri='dummy:a')])])
@@ -53,14 +57,20 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
         self.send_request('listplaylistinfo "name"')
 
         self.assertInResponse('file: dummy:a')
+        self.assertInResponse('Title: Track A')
+        self.assertInResponse('Time: 5')
         self.assertNotInResponse('Track: 0')
         self.assertNotInResponse('Pos: 0')
         self.assertInResponse('OK')
 
     def test_listplaylistinfo_without_quotes(self):
+        tracks = [
+            Track(uri='dummy:a'),
+        ]
+        self.backend.library.dummy_library = tracks
         self.backend.playlists.set_dummy_playlists([
             Playlist(
-                name='name', uri='dummy:name', tracks=[Track(uri='dummy:a')])])
+                name='name', uri='dummy:name', tracks=tracks)])
 
         self.send_request('listplaylistinfo name')
 
@@ -76,13 +86,18 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
             'ACK [50@0] {listplaylistinfo} No such playlist')
 
     def test_listplaylistinfo_duplicate(self):
-        playlist1 = Playlist(name='a', uri='dummy:a1', tracks=[Track(uri='b')])
+        tracks = [
-        playlist2 = Playlist(name='a', uri='dummy:a2', tracks=[Track(uri='c')])
+            Track(uri='dummy:b'),
+            Track(uri='dummy:c'),
+        ]
+        self.backend.library.dummy_library = tracks
+        playlist1 = Playlist(name='a', uri='dummy:a1', tracks=tracks[:1])
+        playlist2 = Playlist(name='a', uri='dummy:a2', tracks=tracks[1:])
         self.backend.playlists.set_dummy_playlists([playlist1, playlist2])
 
         self.send_request('listplaylistinfo "a [2]"')
 
-        self.assertInResponse('file: c')
+        self.assertInResponse('file: dummy:c')
         self.assertNotInResponse('Track: 0')
         self.assertNotInResponse('Pos: 0')
         self.assertInResponse('OK')
@@ -236,6 +251,25 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
         self.send_request('load "unknown/playlist"')
         self.assertEqualResponse('ACK [50@0] {load} No such playlist')
 
+    def test_load_full_track_metadata(self):
+        tracks = [
+            Track(uri='dummy:a', name='Track A', length=5000),
+        ]
+        self.backend.library.dummy_library = tracks
+        self.backend.playlists.set_dummy_playlists([
+            Playlist(
+                name='A-list', uri='dummy:a1', tracks=[Track(uri='dummy:a')])])
+
+        self.send_request('load "A-list"')
+
+        tracks = self.core.tracklist.tracks.get()
+        self.assertEqual(len(tracks), 1)
+        self.assertEqual(tracks[0].uri, 'dummy:a')
+        self.assertEqual(tracks[0].name, 'Track A')
+        self.assertEqual(tracks[0].length, 5000)
+
+        self.assertInResponse('OK')
+
     def test_playlistadd(self):
         tracks = [
             Track(uri='dummy:a'),
@@ -286,6 +320,13 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
         self.assertInResponse('OK')
         self.assertIsNotNone(self.backend.playlists.lookup('dummy:name').get())
 
+    def test_playlistclear_creates_playlist_save_fails(self):
+        self.backend.playlists.set_allow_save(False)
+        self.send_request('playlistclear "name"')
+
+        self.assertInResponse('ACK [0@0] {playlistclear} Backend with '
+                              'scheme "dummy" failed to save playlist')
+
     def test_playlistclear_invalid_name_acks(self):
         self.send_request('playlistclear "foo/bar"')
         self.assertInResponse('ACK [2@0] {playlistclear} playlist name is '
@@ -308,6 +349,22 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
         self.assertEqual(
             2, len(self.backend.playlists.get_items('dummy:a1').get()))
 
+    def test_playlistdelete_save_fails(self):
+        tracks = [
+            Track(uri='dummy:a'),
+            Track(uri='dummy:b'),
+            Track(uri='dummy:c'),
+        ]   # len() == 3
+        self.backend.playlists.set_dummy_playlists([
+            Playlist(
+                name='name', uri='dummy:a1', tracks=tracks)])
+        self.backend.playlists.set_allow_save(False)
+
+        self.send_request('playlistdelete "name" "2"')
+
+        self.assertInResponse('ACK [0@0] {playlistdelete} Backend with '
+                              'scheme "dummy" failed to save playlist')
+
     def test_playlistdelete_invalid_name_acks(self):
         self.send_request('playlistdelete "foo/bar" "0"')
         self.assertInResponse('ACK [2@0] {playlistdelete} playlist name is '
@@ -340,6 +397,22 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
             "dummy:c",
             self.backend.playlists.get_items('dummy:a1').get()[0].uri)
 
+    def test_playlistmove_save_fails(self):
+        tracks = [
+            Track(uri='dummy:a'),
+            Track(uri='dummy:b'),
+            Track(uri='dummy:c')    # this one is getting moved to top
+        ]
+        self.backend.playlists.set_dummy_playlists([
+            Playlist(
+                name='name', uri='dummy:a1', tracks=tracks)])
+        self.backend.playlists.set_allow_save(False)
+
+        self.send_request('playlistmove "name" "2" "0"')
+
+        self.assertInResponse('ACK [0@0] {playlistmove} Backend with '
+                              'scheme "dummy" failed to save playlist')
+
     def test_playlistmove_invalid_name_acks(self):
         self.send_request('playlistmove "foo/bar" "0" "1"')
         self.assertInResponse('ACK [2@0] {playlistmove} playlist name is '
@@ -387,6 +460,17 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
         self.assertIsNotNone(
             self.backend.playlists.lookup('dummy:new_name').get())
 
+    def test_rename_save_fails(self):
+        self.backend.playlists.set_dummy_playlists([
+            Playlist(
+                name='old_name', uri='dummy:a1', tracks=[Track(uri='b')])])
+        self.backend.playlists.set_allow_save(False)
+
+        self.send_request('rename "old_name" "new_name"')
+
+        self.assertInResponse('ACK [0@0] {rename} Backend with '
+                              'scheme "dummy" failed to save playlist')
+
     def test_rename_unknown_playlist_acks(self):
         self.send_request('rename "foo" "bar"')
         self.assertInResponse('ACK [50@0] {rename} No such playlist')
@@ -438,6 +522,13 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
         self.assertInResponse('OK')
         self.assertIsNotNone(self.backend.playlists.lookup('dummy:name').get())
 
+    def test_save_fails(self):
+        self.backend.playlists.set_allow_save(False)
+        self.send_request('save "name"')
+
+        self.assertInResponse('ACK [0@0] {save} Backend with '
+                              'scheme "dummy" failed to save playlist')
+
     def test_save_invalid_name_acks(self):
         self.send_request('save "foo/bar"')
         self.assertInResponse('ACK [2@0] {save} playlist name is invalid: '