From 8aeb9841c569dda38fabd5598ca0bc6a2279ee7c Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 29 Oct 2015 15:01:05 +0100 Subject: [PATCH] mpd: Final cleanup of PR #1187, #1308 and #1322 Fixes #1014, fixes #1322 --- AUTHORS | 3 + docs/changelog.rst | 15 +++- docs/ext/mpd.rst | 1 - mopidy/mpd/__init__.py | 2 +- mopidy/mpd/actor.py | 3 + mopidy/mpd/exceptions.py | 16 ++-- mopidy/mpd/protocol/stored_playlists.py | 92 ++++++++++++--------- tests/mpd/protocol/test_stored_playlists.py | 50 ++++++++--- 8 files changed, 117 insertions(+), 65 deletions(-) diff --git a/AUTHORS b/AUTHORS index e23cd41e..439b8ed7 100644 --- a/AUTHORS +++ b/AUTHORS @@ -72,3 +72,6 @@ - Cadel Watson - Loïck Bonniot - Gustaf Hallberg +- kozec +- Jelle van der Waa +- Alex Malone diff --git a/docs/changelog.rst b/docs/changelog.rst index 3479bcd7..4f49b8e5 100644 --- a/docs/changelog.rst +++ b/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 ------ diff --git a/docs/ext/mpd.rst b/docs/ext/mpd.rst index b02226a2..7f02facc 100644 --- a/docs/ext/mpd.rst +++ b/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 diff --git a/mopidy/mpd/__init__.py b/mopidy/mpd/__init__.py index 5d7d3972..84cf47cb 100644 --- a/mopidy/mpd/__init__.py +++ b/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): diff --git a/mopidy/mpd/actor.py b/mopidy/mpd/actor.py index 8259f01d..7439e73f 100644 --- a/mopidy/mpd/actor.py +++ b/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') diff --git a/mopidy/mpd/exceptions.py b/mopidy/mpd/exceptions.py index 0e7e4a01..b64a6cf0 100644 --- a/mopidy/mpd/exceptions.py +++ b/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 ' \ - 'track scheme "%s"' % (backend_scheme, track_scheme) + self.message = ( + '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 = 'Failed to save playlist' - else: - self.message = 'Backend "%s" failed to save playlist' % ( - backend_scheme, ) + self.message = 'Backend with scheme "%s" failed to save playlist' % ( + backend_scheme) class MpdDisabled(MpdAckError): diff --git a/mopidy/mpd/protocol/stored_playlists.py b/mopidy/mpd/protocol/stored_playlists.py index e97ce371..c6ca1b45 100644 --- a/mopidy/mpd/protocol/stored_playlists.py +++ b/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() - if not playlist: + 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() - tracks = [track for selections in lookup_res.values() - for track in selections] + tracks = [ + 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() - for track in selections] - playlist = playlist.replace(tracks=list(playlist.tracks) + to_add) - if context.core.playlists.save(playlist).get() is None: - playlist_scheme = urlparse.urlparse(playlist.uri).scheme + new_tracks = [ + track + for uri_tracks in lookup_res.values() + for track in uri_tracks] + 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() - if not playlist: - # Backend can't create playlists at all - logger.warning("%s backend can't create playlists", scheme) - continue - playlist = playlist.replace(tracks=tracks) - if context.core.playlists.save(playlist).get() is None: - # Failed to save using this backend - continue - # Created and saved - return - # Can't use backend appropriate for passed uri schemes, use default one + new_playlist = context.core.playlists.create(name, scheme).get() + if new_playlist is None: + logger.debug( + "Backend for scheme %s can't create playlists", scheme) + continue # Backend can't create playlists at all + new_playlist = new_playlist.replace(tracks=tracks) + saved_playlist = context.core.playlists.save(new_playlist).get() + if saved_playlist is not None: + return # Created and saved + else: + continue # Failed to save using this backend + # 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() - if not playlist: + new_playlist = context.core.playlists.create(name, default_scheme).get() + 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") - raise exceptions.MpdFailedToSavePlaylist(None) - playlist = playlist.replace(tracks=tracks) - if context.core.playlists.save(playlist).get() is None: - uri_scheme = urlparse.urlparse(playlist.uri).scheme + logger.warning("MPD's default backend can't create playlists") + raise exceptions.MpdFailedToSavePlaylist(default_scheme) + new_playlist = new_playlist.replace(tracks=tracks) + saved_playlist = context.core.playlists.save(new_playlist).get() + 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() - if not playlist: + old_playlist = uri is not None and context.core.playlists.lookup(uri).get() + 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() - copy = copy.replace(tracks=playlist.tracks) - context.core.playlists.save(copy).get() - context.core.playlists.delete(uri).get() + 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() + 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) - if not context.core.playlists.save(playlist).get(): + new_playlist = playlist.replace(tracks=tracks) + saved_playlist = context.core.playlists.save(new_playlist).get() + if saved_playlist is None: raise exceptions.MpdFailedToSavePlaylist( urlparse.urlparse(uri).scheme) diff --git a/tests/mpd/protocol/test_stored_playlists.py b/tests/mpd/protocol/test_stored_playlists.py index f7a846aa..e212af09 100644 --- a/tests/mpd/protocol/test_stored_playlists.py +++ b/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( - None, self.backend.playlists.lookup('dummy:name').get()) + self.assertIsNotNone(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( - None, self.backend.playlists.lookup("dummy:name").get()) + self.assertIsNotNone(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( - None, self.backend.playlists.lookup("dummy:a1").get()) + self.assertIsNone(self.backend.playlists.lookup('dummy:a1').get()) def test_save(self): self.send_request('save "name"') + self.assertInResponse('OK') - self.assertNotEqual( - None, self.backend.playlists.lookup("dummy:name").get()) + self.assertIsNotNone(self.backend.playlists.lookup('dummy:name').get())