diff --git a/docs/changelog.rst b/docs/changelog.rst index 0304d881..6584d166 100644 --- a/docs/changelog.rst +++ b/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`) diff --git a/mopidy/mpd/protocol/stored_playlists.py b/mopidy/mpd/protocol/stored_playlists.py index 68ae1e9e..fd4d2974 100644 --- a/mopidy/mpd/protocol/stored_playlists.py +++ b/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 = uri is not None and context.core.playlists.lookup(uri).get() - if not playlist: - raise exceptions.MpdNoExistError('No such playlist') + playlist = _get_playlist(context, name) 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 = uri is not None and context.core.playlists.lookup(uri).get() - if not playlist: - raise exceptions.MpdNoExistError('No such playlist') + playlist = _get_playlist(context, name) + track_uris = [track.uri for track in playlist.tracks] + tracks_map = context.core.library.lookup(uris=track_uris).get() + 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 = uri is not None and context.core.playlists.lookup(uri).get() - if not playlist: - 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() + playlist = _get_playlist(context, name) + track_uris = [track.uri for track in playlist.tracks[playlist_slice]] + context.core.tracklist.add(uris=track_uris).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 = uri is not None and context.core.playlists.lookup(uri).get() + old_playlist = _get_playlist(context, name, must_exist=False) 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 = uri is not None and context.core.playlists.lookup(uri).get() + playlist = _get_playlist(context, name, must_exist=False) 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 = uri is not None and context.core.playlists.lookup(uri).get() - if not playlist: - raise exceptions.MpdNoExistError('No such playlist') + playlist = _get_playlist(context, name) 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 = uri is not None and context.core.playlists.lookup(uri).get() - if not playlist: - raise exceptions.MpdNoExistError('No such playlist') + playlist = _get_playlist(context, name) 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) - if not old_uri: - raise exceptions.MpdNoExistError('No such playlist') + old_playlist = _get_playlist(context, old_name) - old_playlist = context.core.playlists.lookup(old_uri).get() - 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(): + if _get_playlist(context, new_name, must_exist=False): 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 = uri is not None and context.core.playlists.lookup(uri).get() + playlist = _get_playlist(context, name, must_exist=False) 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) diff --git a/tests/dummy_backend.py b/tests/dummy_backend.py index 465aeab6..37702bc7 100644 --- a/tests/dummy_backend.py +++ b/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: diff --git a/tests/mpd/protocol/test_stored_playlists.py b/tests/mpd/protocol/test_stored_playlists.py index fc3e8214..a5f7f70d 100644 --- a/tests/mpd/protocol/test_stored_playlists.py +++ b/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')]) - playlist2 = Playlist(name='a', uri='dummy:a2', tracks=[Track(uri='c')]) + tracks = [ + 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: '