mpd: Final cleanup of PR #1187, #1308 and #1322

Fixes #1014, fixes #1322

AUTHORS

@@ -72,3 +72,6 @@
 - Cadel Watson <cadel@cadelwatson.com>
 - Loïck Bonniot <git@lesterpig.com>
 - Gustaf Hallberg <ghallberg@gmail.com>
+- kozec <kozec@kozec.com>
+- Jelle van der Waa <jelle@vdwaa.nl>
+- Alex Malone <jalexmalone@gmail.com>

docs/changelog.rst

@@ -25,6 +25,18 @@ Local backend
 MPD frontend
 ------------
 
+- Implemented commands for modifying stored playlists:
+
+  - ``playlistadd``
+  - ``playlistclear``
+  - ``playlistdelete``
+  - ``playlistmove``
+  - ``rename``
+  - ``rm``
+  - ``save``
+
+  (Fixes: :issue:`1014`, PR: :issue:`1187`, :issue:`1308`, :issue:`1322`)
+
 - Start ``songid`` counting at 1 instead of 0 to match the original MPD server.
 
 Zeroconf
@@ -236,9 +248,6 @@ Core API
   :issue:`997` PR: :issue:`1225`)
 
 - Added ``playlist_deleted`` event. (Fixes: :issue:`996`)
-=======
-
-- MPD: Implemented commands for modifying stored playlists (PR: :issue:`1187`)
 
 Models
 ------

docs/ext/mpd.rst

@@ -45,7 +45,6 @@ Items on this list will probably not be supported in the near future.
 The following items are currently not supported, but should be added in the
 near future:
 
-- Modifying stored playlists is not supported
 - ``tagtypes`` is not supported
 - Live update of the music database is not supported
 

mopidy/mpd/__init__.py

@@ -25,7 +25,7 @@ class Extension(ext.Extension):
         schema['connection_timeout'] = config.Integer(minimum=1)
         schema['zeroconf'] = config.String(optional=True)
         schema['command_blacklist'] = config.List(optional=True)
-        schema['default_playlist_scheme'] = config.String(optional=False)
+        schema['default_playlist_scheme'] = config.String()
         return schema
 
     def validate_environment(self):

mopidy/mpd/actor.py

@@ -70,6 +70,9 @@ class MpdFrontend(pykka.ThreadingActor, CoreListener):
     def playlist_changed(self, playlist):
         self.send_idle('stored_playlist')
 
+    def playlist_deleted(self, playlist):
+        self.send_idle('stored_playlist')
+
     def options_changed(self):
         self.send_idle('options')
 

mopidy/mpd/exceptions.py

@@ -93,24 +93,24 @@ class MpdNotImplemented(MpdAckError):
 
 
 class MpdInvalidTrackForPlaylist(MpdAckError):
+    # NOTE: This is a custom error for Mopidy that does not exist in MPD.
     error_code = 0
 
-    def __init__(self, backend_scheme, track_scheme, *args, **kwargs):
+    def __init__(self, playlist_scheme, track_scheme, *args, **kwargs):
         super(MpdInvalidTrackForPlaylist, self).__init__(*args, **kwargs)
-        self.message = 'Playlist backend "%s" can\'t store ' \
+        self.message = (
-            'track scheme "%s"' % (backend_scheme, track_scheme)
+            'Playlist with scheme "%s" can\'t store track scheme "%s"' %
+            (playlist_scheme, track_scheme))
 
 
 class MpdFailedToSavePlaylist(MpdAckError):
+    # NOTE: This is a custom error for Mopidy that does not exist in MPD.
     error_code = 0
 
     def __init__(self, backend_scheme, *args, **kwargs):
         super(MpdFailedToSavePlaylist, self).__init__(*args, **kwargs)
-        if backend_scheme is None:
+        self.message = 'Backend with scheme "%s" failed to save playlist' % (
-            self.message = 'Failed to save playlist'
+            backend_scheme)
-        else:
-            self.message = 'Backend "%s" failed to save playlist' % (
-                backend_scheme, )
 
 
 class MpdDisabled(MpdAckError):

mopidy/mpd/protocol/stored_playlists.py

@@ -150,22 +150,28 @@ def playlistadd(context, name, track_uri):
         ``NAME.m3u`` will be created if it does not exist.
     """
     uri = context.lookup_playlist_uri_from_name(name)
-    playlist = uri is not None and context.core.playlists.lookup(uri).get()
+    old_playlist = uri is not None and context.core.playlists.lookup(uri).get()
-    if not playlist:
+    if not old_playlist:
         # Create new playlist with this single track
         lookup_res = context.core.library.lookup(uris=[track_uri]).get()
-        tracks = [track for selections in lookup_res.values()
+        tracks = [
-                  for track in selections]
+            track
+            for uri_tracks in lookup_res.values()
+            for track in uri_tracks]
         _create_playlist(context, name, tracks)
     else:
         # Add track to existing playlist
-        uri_scheme = urlparse.urlparse(track_uri).scheme
         lookup_res = context.core.library.lookup(uris=[track_uri]).get()
-        to_add = [track for selections in lookup_res.values()
+        new_tracks = [
-                  for track in selections]
+            track
-        playlist = playlist.replace(tracks=list(playlist.tracks) + to_add)
+            for uri_tracks in lookup_res.values()
-        if context.core.playlists.save(playlist).get() is None:
+            for track in uri_tracks]
-            playlist_scheme = urlparse.urlparse(playlist.uri).scheme
+        new_playlist = old_playlist.replace(
+            tracks=list(old_playlist.tracks) + new_tracks)
+        saved_playlist = context.core.playlists.save(new_playlist).get()
+        if saved_playlist is None:
+            playlist_scheme = urlparse.urlparse(old_playlist.uri).scheme
+            uri_scheme = urlparse.urlparse(track_uri).scheme
             raise exceptions.MpdInvalidTrackForPlaylist(
                 playlist_scheme, uri_scheme)
 
@@ -176,28 +182,29 @@ def _create_playlist(context, name, tracks):
     """
     uri_schemes = set([urlparse.urlparse(t.uri).scheme for t in tracks])
     for scheme in uri_schemes:
-        playlist = context.core.playlists.create(name, scheme).get()
+        new_playlist = context.core.playlists.create(name, scheme).get()
-        if not playlist:
+        if new_playlist is None:
-            # Backend can't create playlists at all
+            logger.debug(
-            logger.warning("%s backend can't create playlists", scheme)
+                "Backend for scheme %s can't create playlists", scheme)
-            continue
+            continue  # Backend can't create playlists at all
-        playlist = playlist.replace(tracks=tracks)
+        new_playlist = new_playlist.replace(tracks=tracks)
-        if context.core.playlists.save(playlist).get() is None:
+        saved_playlist = context.core.playlists.save(new_playlist).get()
-            # Failed to save using this backend
+        if saved_playlist is not None:
-            continue
+            return  # Created and saved
-        # Created and saved
+        else:
-        return
+            continue  # Failed to save using this backend
-    # Can't use backend appropriate for passed uri schemes, use default one
+    # Can't use backend appropriate for passed URI schemes, use default one
     default_scheme = context.dispatcher.config[
         'mpd']['default_playlist_scheme']
-    playlist = context.core.playlists.create(name, default_scheme).get()
+    new_playlist = context.core.playlists.create(name, default_scheme).get()
-    if not playlist:
+    if new_playlist is None:
         # If even MPD's default backend can't save playlist, everything is lost
-        logger.warning("Default backend can't create playlists")
+        logger.warning("MPD's default backend can't create playlists")
-        raise exceptions.MpdFailedToSavePlaylist(None)
+        raise exceptions.MpdFailedToSavePlaylist(default_scheme)
-    playlist = playlist.replace(tracks=tracks)
+    new_playlist = new_playlist.replace(tracks=tracks)
-    if context.core.playlists.save(playlist).get() is None:
+    saved_playlist = context.core.playlists.save(new_playlist).get()
-        uri_scheme = urlparse.urlparse(playlist.uri).scheme
+    if saved_playlist is None:
+        uri_scheme = urlparse.urlparse(new_playlist.uri).scheme
         raise exceptions.MpdFailedToSavePlaylist(uri_scheme)
 
 
@@ -210,7 +217,7 @@ def playlistclear(context, name):
 
         Clears the playlist ``NAME.m3u``.
 
-    ``NAME.m3u`` will be created if it does not exist.
+    The playlist will be created if it does not exist.
     """
     uri = context.lookup_playlist_uri_from_name(name)
     playlist = uri is not None and context.core.playlists.lookup(uri).get()
@@ -243,7 +250,8 @@ def playlistdelete(context, name, songpos):
 
     # Replace tracks and save playlist
     playlist = playlist.replace(tracks=tracks)
-    if context.core.playlists.save(playlist).get() is None:
+    saved_playlist = context.core.playlists.save(playlist).get()
+    if saved_playlist is None:
         raise exceptions.MpdFailedToSavePlaylist(urlparse.urlparse(uri).scheme)
 
 
@@ -278,7 +286,8 @@ def playlistmove(context, name, from_pos, to_pos):
 
     # Replace tracks and save playlist
     playlist = playlist.replace(tracks=tracks)
-    if context.core.playlists.save(playlist).get() is None:
+    saved_playlist = context.core.playlists.save(playlist).get()
+    if saved_playlist is None:
         raise exceptions.MpdFailedToSavePlaylist(urlparse.urlparse(uri).scheme)
 
 
@@ -293,15 +302,17 @@ def rename(context, old_name, new_name):
     """
     uri = context.lookup_playlist_uri_from_name(old_name)
     uri_scheme = urlparse.urlparse(uri).scheme
-    playlist = uri is not None and context.core.playlists.lookup(uri).get()
+    old_playlist = uri is not None and context.core.playlists.lookup(uri).get()
-    if not playlist:
+    if not old_playlist:
         raise exceptions.MpdNoExistError('No such playlist')
 
     # Create copy of the playlist and remove original
-    copy = context.core.playlists.create(new_name, uri_scheme).get()
+    new_playlist = context.core.playlists.create(new_name, uri_scheme).get()
-    copy = copy.replace(tracks=playlist.tracks)
+    new_playlist = new_playlist.replace(tracks=old_playlist.tracks)
-    context.core.playlists.save(copy).get()
+    saved_playlist = context.core.playlists.save(new_playlist).get()
-    context.core.playlists.delete(uri).get()
+    if saved_playlist is None:
+        raise exceptions.MpdFailedToSavePlaylist(uri_scheme)
+    context.core.playlists.delete(old_playlist.uri).get()
 
 
 @protocol.commands.add('rm')
@@ -335,7 +346,8 @@ def save(context, name):
         _create_playlist(context, name, tracks)
     else:
         # Overwrite existing playlist
-        playlist = playlist.replace(tracks=tracks)
+        new_playlist = playlist.replace(tracks=tracks)
-        if not context.core.playlists.save(playlist).get():
+        saved_playlist = context.core.playlists.save(new_playlist).get()
+        if saved_playlist is None:
             raise exceptions.MpdFailedToSavePlaylist(
                 urlparse.urlparse(uri).scheme)

tests/mpd/protocol/test_stored_playlists.py

@@ -16,6 +16,7 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
                 name='name', uri='dummy:name', tracks=[Track(uri='dummy:a')])])
 
         self.send_request('listplaylist "name"')
+
         self.assertInResponse('file: dummy:a')
         self.assertInResponse('OK')
 
@@ -25,11 +26,13 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
                 name='name', uri='dummy:name', tracks=[Track(uri='dummy:a')])])
 
         self.send_request('listplaylist name')
+
         self.assertInResponse('file: dummy:a')
         self.assertInResponse('OK')
 
     def test_listplaylist_fails_if_no_playlist_is_found(self):
         self.send_request('listplaylist "name"')
+
         self.assertEqualResponse('ACK [50@0] {listplaylist} No such playlist')
 
     def test_listplaylist_duplicate(self):
@@ -38,6 +41,7 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
         self.backend.playlists.set_dummy_playlists([playlist1, playlist2])
 
         self.send_request('listplaylist "a [2]"')
+
         self.assertInResponse('file: c')
         self.assertInResponse('OK')
 
@@ -47,6 +51,7 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
                 name='name', uri='dummy:name', tracks=[Track(uri='dummy:a')])])
 
         self.send_request('listplaylistinfo "name"')
+
         self.assertInResponse('file: dummy:a')
         self.assertNotInResponse('Track: 0')
         self.assertNotInResponse('Pos: 0')
@@ -58,6 +63,7 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
                 name='name', uri='dummy:name', tracks=[Track(uri='dummy:a')])])
 
         self.send_request('listplaylistinfo name')
+
         self.assertInResponse('file: dummy:a')
         self.assertNotInResponse('Track: 0')
         self.assertNotInResponse('Pos: 0')
@@ -65,6 +71,7 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
 
     def test_listplaylistinfo_fails_if_no_playlist_is_found(self):
         self.send_request('listplaylistinfo "name"')
+
         self.assertEqualResponse(
             'ACK [50@0] {listplaylistinfo} No such playlist')
 
@@ -74,6 +81,7 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
         self.backend.playlists.set_dummy_playlists([playlist1, playlist2])
 
         self.send_request('listplaylistinfo "a [2]"')
+
         self.assertInResponse('file: c')
         self.assertNotInResponse('Track: 0')
         self.assertNotInResponse('Pos: 0')
@@ -86,6 +94,7 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
             Playlist(name='a', uri='dummy:a')])
 
         self.send_request('listplaylists')
+
         self.assertInResponse('playlist: a')
         # Date without milliseconds and with time zone information
         self.assertInResponse('Last-Modified: 2015-08-05T22:51:06Z')
@@ -97,6 +106,7 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
         self.backend.playlists.set_dummy_playlists([playlist1, playlist2])
 
         self.send_request('listplaylists')
+
         self.assertInResponse('playlist: a')
         self.assertInResponse('playlist: a [2]')
         self.assertInResponse('OK')
@@ -107,13 +117,16 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
             Playlist(name='', uri='dummy:', last_modified=last_modified)])
 
         self.send_request('listplaylists')
+
         self.assertNotInResponse('playlist: ')
         self.assertInResponse('OK')
 
     def test_listplaylists_replaces_newline_with_space(self):
         self.backend.playlists.set_dummy_playlists([
             Playlist(name='a\n', uri='dummy:')])
+
         self.send_request('listplaylists')
+
         self.assertInResponse('playlist: a ')
         self.assertNotInResponse('playlist: a\n')
         self.assertInResponse('OK')
@@ -121,7 +134,9 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
     def test_listplaylists_replaces_carriage_return_with_space(self):
         self.backend.playlists.set_dummy_playlists([
             Playlist(name='a\r', uri='dummy:')])
+
         self.send_request('listplaylists')
+
         self.assertInResponse('playlist: a ')
         self.assertNotInResponse('playlist: a\r')
         self.assertInResponse('OK')
@@ -129,7 +144,9 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
     def test_listplaylists_replaces_forward_slash_with_pipe(self):
         self.backend.playlists.set_dummy_playlists([
             Playlist(name='a/b', uri='dummy:')])
+
         self.send_request('listplaylists')
+
         self.assertInResponse('playlist: a|b')
         self.assertNotInResponse('playlist: a/b')
         self.assertInResponse('OK')
@@ -211,6 +228,7 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
 
     def test_load_unknown_playlist_acks(self):
         self.send_request('load "unknown playlist"')
+
         self.assertEqual(0, len(self.core.tracklist.tracks.get()))
         self.assertEqualResponse('ACK [50@0] {load} No such playlist')
 
@@ -235,10 +253,11 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
             Track(uri='dummy:a'),
         ]
         self.backend.library.dummy_library = tracks
+
         self.send_request('playlistadd "name" "dummy:a"')
+
         self.assertInResponse('OK')
-        self.assertNotEqual(
+        self.assertIsNotNone(self.backend.playlists.lookup('dummy:name').get())
-            None, self.backend.playlists.lookup('dummy:name').get())
 
     def test_playlistclear(self):
         self.backend.playlists.set_dummy_playlists([
@@ -246,15 +265,16 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
                 name='name', uri='dummy:a1', tracks=[Track(uri='b')])])
 
         self.send_request('playlistclear "name"')
+
         self.assertInResponse('OK')
         self.assertEqual(
-            0, len(self.backend.playlists.get_items("dummy:a1").get()))
+            0, len(self.backend.playlists.get_items('dummy:a1').get()))
 
     def test_playlistclear_creates_playlist(self):
         self.send_request('playlistclear "name"')
+
         self.assertInResponse('OK')
-        self.assertNotEqual(
+        self.assertIsNotNone(self.backend.playlists.lookup('dummy:name').get())
-            None, self.backend.playlists.lookup("dummy:name").get())
 
     def test_playlistdelete(self):
         tracks = [
@@ -267,9 +287,10 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
                 name='name', uri='dummy:a1', tracks=tracks)])
 
         self.send_request('playlistdelete "name" "2"')
+
         self.assertInResponse('OK')
         self.assertEqual(
-            2, len(self.backend.playlists.get_items("dummy:a1").get()))
+            2, len(self.backend.playlists.get_items('dummy:a1').get()))
 
     def test_playlistmove(self):
         tracks = [
@@ -280,32 +301,37 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
         self.backend.playlists.set_dummy_playlists([
             Playlist(
                 name='name', uri='dummy:a1', tracks=tracks)])
+
         self.send_request('playlistmove "name" "2" "0"')
+
         self.assertInResponse('OK')
         self.assertEqual(
             "dummy:c",
-            self.backend.playlists.get_items("dummy:a1").get()[0].uri)
+            self.backend.playlists.get_items('dummy:a1').get()[0].uri)
 
     def test_rename(self):
         self.backend.playlists.set_dummy_playlists([
             Playlist(
                 name='old_name', uri='dummy:a1', tracks=[Track(uri='b')])])
+
         self.send_request('rename "old_name" "new_name"')
+
         self.assertInResponse('OK')
         self.assertIsNotNone(
-            self.backend.playlists.lookup("dummy:new_name").get())
+            self.backend.playlists.lookup('dummy:new_name').get())
 
     def test_rm(self):
         self.backend.playlists.set_dummy_playlists([
             Playlist(
                 name='name', uri='dummy:a1', tracks=[Track(uri='b')])])
+
         self.send_request('rm "name"')
+
         self.assertInResponse('OK')
-        self.assertEqual(
+        self.assertIsNone(self.backend.playlists.lookup('dummy:a1').get())
-            None, self.backend.playlists.lookup("dummy:a1").get())
 
     def test_save(self):
         self.send_request('save "name"')
+
         self.assertInResponse('OK')
-        self.assertNotEqual(
+        self.assertIsNotNone(self.backend.playlists.lookup('dummy:name').get())
-            None, self.backend.playlists.lookup("dummy:name").get())