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 3a0f5833..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 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 b2438b07..84cf47cb 100644 --- a/mopidy/mpd/__init__.py +++ b/mopidy/mpd/__init__.py @@ -25,6 +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() return schema def validate_environment(self): diff --git a/mopidy/mpd/actor.py b/mopidy/mpd/actor.py index 69d165ca..7439e73f 100644 --- a/mopidy/mpd/actor.py +++ b/mopidy/mpd/actor.py @@ -67,6 +67,12 @@ class MpdFrontend(pykka.ThreadingActor, CoreListener): def tracklist_changed(self): self.send_idle('playlist') + 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 3bd51567..b64a6cf0 100644 --- a/mopidy/mpd/exceptions.py +++ b/mopidy/mpd/exceptions.py @@ -92,6 +92,27 @@ class MpdNotImplemented(MpdAckError): self.message = 'Not implemented' +class MpdInvalidTrackForPlaylist(MpdAckError): + # NOTE: This is a custom error for Mopidy that does not exist in MPD. + error_code = 0 + + def __init__(self, playlist_scheme, track_scheme, *args, **kwargs): + super(MpdInvalidTrackForPlaylist, self).__init__(*args, **kwargs) + 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) + self.message = 'Backend with scheme "%s" failed to save playlist' % ( + backend_scheme) + + class MpdDisabled(MpdAckError): # NOTE: This is a custom error for Mopidy that does not exist in MPD. error_code = 0 diff --git a/mopidy/mpd/ext.conf b/mopidy/mpd/ext.conf index fe9a0494..ee518a86 100644 --- a/mopidy/mpd/ext.conf +++ b/mopidy/mpd/ext.conf @@ -7,3 +7,4 @@ max_connections = 20 connection_timeout = 60 zeroconf = Mopidy MPD server on $hostname command_blacklist = listall,listallinfo +default_playlist_scheme = m3u diff --git a/mopidy/mpd/protocol/stored_playlists.py b/mopidy/mpd/protocol/stored_playlists.py index bf31fa10..c6ca1b45 100644 --- a/mopidy/mpd/protocol/stored_playlists.py +++ b/mopidy/mpd/protocol/stored_playlists.py @@ -1,10 +1,14 @@ from __future__ import absolute_import, division, unicode_literals import datetime +import logging +import urlparse import warnings from mopidy.mpd import exceptions, protocol, translator +logger = logging.getLogger(__name__) + @protocol.commands.add('listplaylist') def listplaylist(context, name): @@ -135,7 +139,7 @@ def load(context, name, playlist_slice=slice(0, None)): @protocol.commands.add('playlistadd') -def playlistadd(context, name, uri): +def playlistadd(context, name, track_uri): """ *musicpd.org, stored playlists section:* @@ -145,7 +149,63 @@ def playlistadd(context, name, uri): ``NAME.m3u`` will be created if it does not exist. """ - raise exceptions.MpdNotImplemented # TODO + uri = context.lookup_playlist_uri_from_name(name) + 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 uri_tracks in lookup_res.values() + for track in uri_tracks] + _create_playlist(context, name, tracks) + else: + # Add track to existing playlist + lookup_res = context.core.library.lookup(uris=[track_uri]).get() + 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) + + +def _create_playlist(context, name, tracks): + """ + Creates new playlist using backend appropriate for the given tracks + """ + uri_schemes = set([urlparse.urlparse(t.uri).scheme for t in tracks]) + for scheme in uri_schemes: + 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'] + 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("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) @protocol.commands.add('playlistclear') @@ -156,8 +216,18 @@ def playlistclear(context, name): ``playlistclear {NAME}`` Clears the playlist ``NAME.m3u``. + + The playlist will be created if it does not exist. """ - raise exceptions.MpdNotImplemented # TODO + uri = context.lookup_playlist_uri_from_name(name) + playlist = uri is not None and context.core.playlists.lookup(uri).get() + if not playlist: + playlist = context.core.playlists.create(name).get() + + # Just replace tracks with empty list and save + playlist = playlist.replace(tracks=[]) + if context.core.playlists.save(playlist).get() is None: + raise exceptions.MpdFailedToSavePlaylist(urlparse.urlparse(uri).scheme) @protocol.commands.add('playlistdelete', songpos=protocol.UINT) @@ -169,7 +239,20 @@ def playlistdelete(context, name, songpos): Deletes ``SONGPOS`` from the playlist ``NAME.m3u``. """ - raise exceptions.MpdNotImplemented # TODO + uri = context.lookup_playlist_uri_from_name(name) + playlist = uri is not None and context.core.playlists.lookup(uri).get() + if not playlist: + raise exceptions.MpdNoExistError('No such playlist') + + # Convert tracks to list and remove requested + tracks = list(playlist.tracks) + tracks.pop(songpos) + + # Replace tracks and save playlist + playlist = playlist.replace(tracks=tracks) + saved_playlist = context.core.playlists.save(playlist).get() + if saved_playlist is None: + raise exceptions.MpdFailedToSavePlaylist(urlparse.urlparse(uri).scheme) @protocol.commands.add( @@ -189,7 +272,23 @@ def playlistmove(context, name, from_pos, to_pos): documentation, but just the ``SONGPOS`` to move *from*, i.e. ``playlistmove {NAME} {FROM_SONGPOS} {TO_SONGPOS}``. """ - raise exceptions.MpdNotImplemented # TODO + uri = context.lookup_playlist_uri_from_name(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 + + # Convert tracks to list and perform move + tracks = list(playlist.tracks) + track = tracks.pop(from_pos) + tracks.insert(to_pos, track) + + # Replace tracks and save playlist + playlist = playlist.replace(tracks=tracks) + saved_playlist = context.core.playlists.save(playlist).get() + if saved_playlist is None: + raise exceptions.MpdFailedToSavePlaylist(urlparse.urlparse(uri).scheme) @protocol.commands.add('rename') @@ -201,7 +300,19 @@ def rename(context, old_name, new_name): Renames the playlist ``NAME.m3u`` to ``NEW_NAME.m3u``. """ - raise exceptions.MpdNotImplemented # TODO + uri = context.lookup_playlist_uri_from_name(old_name) + uri_scheme = urlparse.urlparse(uri).scheme + 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 + 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') @@ -213,7 +324,8 @@ def rm(context, name): Removes the playlist ``NAME.m3u`` from the playlist directory. """ - raise exceptions.MpdNotImplemented # TODO + uri = context.lookup_playlist_uri_from_name(name) + context.core.playlists.delete(uri).get() @protocol.commands.add('save') @@ -226,4 +338,16 @@ def save(context, name): Saves the current playlist to ``NAME.m3u`` in the playlist directory. """ - raise exceptions.MpdNotImplemented # TODO + tracks = context.core.tracklist.get_tracks().get() + uri = context.lookup_playlist_uri_from_name(name) + playlist = uri is not None and context.core.playlists.lookup(uri).get() + if not playlist: + # Create new playlist + _create_playlist(context, name, tracks) + else: + # Overwrite existing playlist + 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/__init__.py b/tests/mpd/protocol/__init__.py index f34ad4f0..0e8157bd 100644 --- a/tests/mpd/protocol/__init__.py +++ b/tests/mpd/protocol/__init__.py @@ -36,6 +36,7 @@ class BaseTestCase(unittest.TestCase): }, 'mpd': { 'password': None, + 'default_playlist_scheme': 'dummy', } } diff --git a/tests/mpd/protocol/test_stored_playlists.py b/tests/mpd/protocol/test_stored_playlists.py index 90c325ff..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,33 +228,110 @@ 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') def test_playlistadd(self): + tracks = [ + Track(uri='dummy:a'), + Track(uri='dummy:b'), + ] + self.backend.library.dummy_library = tracks + self.backend.playlists.set_dummy_playlists([ + Playlist( + name='name', uri='dummy:a1', tracks=[tracks[0]])]) + + self.send_request('playlistadd "name" "dummy:b"') + + self.assertInResponse('OK') + self.assertEqual( + 2, len(self.backend.playlists.get_items('dummy:a1').get())) + + def test_playlistadd_creates_playlist(self): + tracks = [ + Track(uri='dummy:a'), + ] + self.backend.library.dummy_library = tracks + self.send_request('playlistadd "name" "dummy:a"') - self.assertEqualResponse('ACK [0@0] {playlistadd} Not implemented') + + self.assertInResponse('OK') + self.assertIsNotNone(self.backend.playlists.lookup('dummy:name').get()) def test_playlistclear(self): + self.backend.playlists.set_dummy_playlists([ + Playlist( + name='name', uri='dummy:a1', tracks=[Track(uri='b')])]) + self.send_request('playlistclear "name"') - self.assertEqualResponse('ACK [0@0] {playlistclear} Not implemented') + + self.assertInResponse('OK') + self.assertEqual( + 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.assertIsNotNone(self.backend.playlists.lookup('dummy:name').get()) def test_playlistdelete(self): - self.send_request('playlistdelete "name" "5"') - self.assertEqualResponse('ACK [0@0] {playlistdelete} Not implemented') + 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.send_request('playlistdelete "name" "2"') + + self.assertInResponse('OK') + self.assertEqual( + 2, len(self.backend.playlists.get_items('dummy:a1').get())) def test_playlistmove(self): - self.send_request('playlistmove "name" "5" "10"') - self.assertEqualResponse('ACK [0@0] {playlistmove} Not implemented') + 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.send_request('playlistmove "name" "2" "0"') + + self.assertInResponse('OK') + self.assertEqual( + "dummy:c", + 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.assertEqualResponse('ACK [0@0] {rename} Not implemented') + + self.assertInResponse('OK') + self.assertIsNotNone( + 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.assertEqualResponse('ACK [0@0] {rm} Not implemented') + + self.assertInResponse('OK') + self.assertIsNone(self.backend.playlists.lookup('dummy:a1').get()) def test_save(self): self.send_request('save "name"') - self.assertEqualResponse('ACK [0@0] {save} Not implemented') + + self.assertInResponse('OK') + self.assertIsNotNone(self.backend.playlists.lookup('dummy:name').get())