diff --git a/docs/changes.rst b/docs/changes.rst index 6e9a8f66..f024bc30 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -132,6 +132,15 @@ backends: :meth:`mopidy.core.LibraryController.search` now returns plain lists of tracks instead of playlist objects. +- :meth:`mopidy.core.TracklistController.get` has been replaced by + :meth:`mopidy.core.TracklistController.filter`. + +- :meth:`mopidy.core.PlaylistsController.get` has been replaced by + :meth:`mopidy.core.PlaylistsController.filter`. + +- :meth:`mopidy.core.TracklistController.remove` can now remove multiple + tracks, and returns the tracks it removed. + **Bug fixes** - :issue:`218`: The MPD commands ``listplaylist`` and ``listplaylistinfo`` now diff --git a/mopidy/core/playlists.py b/mopidy/core/playlists.py index 25ae2bdf..dcdc665f 100644 --- a/mopidy/core/playlists.py +++ b/mopidy/core/playlists.py @@ -69,35 +69,25 @@ class PlaylistsController(object): if backend: backend.playlists.delete(uri).get() - def get(self, **criteria): + def filter(self, **criteria): """ - Get playlist by given criterias from the set of playlists. - - Raises :exc:`LookupError` if a unique match is not found. + Filter playlists by the given criterias. Examples:: - get(name='a') # Returns track with name 'a' - get(uri='xyz') # Returns track with URI 'xyz' - get(name='a', uri='xyz') # Returns track with name 'a' and URI - # 'xyz' + filter(name='a') # Returns track with name 'a' + filter(uri='xyz') # Returns track with URI 'xyz' + filter(name='a', uri='xyz') # Returns track with name 'a' and URI + # 'xyz' :param criteria: one or more criteria to match by :type criteria: dict - :rtype: :class:`mopidy.models.Playlist` + :rtype: list of :class:`mopidy.models.Playlist` """ matches = self.playlists for (key, value) in criteria.iteritems(): matches = filter(lambda p: getattr(p, key) == value, matches) - if len(matches) == 1: - return matches[0] - criteria_string = ', '.join( - ['%s=%s' % (k, v) for (k, v) in criteria.iteritems()]) - if len(matches) == 0: - raise LookupError('"%s" match no playlists' % criteria_string) - else: - raise LookupError( - '"%s" match multiple playlists' % criteria_string) + return matches def lookup(self, uri): """ diff --git a/mopidy/core/tracklist.py b/mopidy/core/tracklist.py index b352e06e..e00a42f9 100644 --- a/mopidy/core/tracklist.py +++ b/mopidy/core/tracklist.py @@ -116,22 +116,20 @@ class TracklistController(object): self._tl_tracks = [] self.version += 1 - def get(self, **criteria): + def filter(self, **criteria): """ - Get track by given criterias from tracklist. - - Raises :exc:`LookupError` if a unique match is not found. + Filter the tracklist by the given criterias. Examples:: - get(tlid=7) # Returns track with TLID 7 (tracklist ID) - get(id=1) # Returns track with ID 1 - get(uri='xyz') # Returns track with URI 'xyz' - get(id=1, uri='xyz') # Returns track with ID 1 and URI 'xyz' + filter(tlid=7) # Returns track with TLID 7 (tracklist ID) + filter(id=1) # Returns track with ID 1 + filter(uri='xyz') # Returns track with URI 'xyz' + filter(id=1, uri='xyz') # Returns track with ID 1 and URI 'xyz' :param criteria: on or more criteria to match by :type criteria: dict - :rtype: :class:`mopidy.models.TlTrack` + :rtype: list of :class:`mopidy.models.TlTrack` """ matches = self._tl_tracks for (key, value) in criteria.iteritems(): @@ -140,14 +138,7 @@ class TracklistController(object): else: matches = filter( lambda ct: getattr(ct.track, key) == value, matches) - if len(matches) == 1: - return matches[0] - criteria_string = ', '.join( - ['%s=%s' % (k, v) for (k, v) in criteria.iteritems()]) - if len(matches) == 0: - raise LookupError('"%s" match no tracks' % criteria_string) - else: - raise LookupError('"%s" match multiple tracks' % criteria_string) + return matches def index(self, tl_track): """ @@ -197,20 +188,23 @@ class TracklistController(object): def remove(self, **criteria): """ - Remove the track from the tracklist. + Remove the matching tracks from the tracklist. - Uses :meth:`get()` to lookup the track to remove. + Uses :meth:`filter()` to lookup the tracks to remove. Triggers the :method:`mopidy.core.CoreListener.tracklist_changed` event. :param criteria: on or more criteria to match by :type criteria: dict + :rtype: list of :class:`mopidy.models.TlTrack` that was removed """ - tl_track = self.get(**criteria) - position = self._tl_tracks.index(tl_track) - del self._tl_tracks[position] + tl_tracks = self.filter(**criteria) + for tl_track in tl_tracks: + position = self._tl_tracks.index(tl_track) + del self._tl_tracks[position] self.version += 1 + return tl_tracks def shuffle(self, start=None, end=None): """ diff --git a/mopidy/frontends/mpd/protocol/current_playlist.py b/mopidy/frontends/mpd/protocol/current_playlist.py index 1a3f4f7b..a1af4549 100644 --- a/mopidy/frontends/mpd/protocol/current_playlist.py +++ b/mopidy/frontends/mpd/protocol/current_playlist.py @@ -103,12 +103,11 @@ def deleteid(context, tlid): Deletes the song ``SONGID`` from the playlist """ - try: - tlid = int(tlid) - if context.core.playback.current_tlid.get() == tlid: - context.core.playback.next() - return context.core.tracklist.remove(tlid=tlid).get() - except LookupError: + tlid = int(tlid) + if context.core.playback.current_tlid.get() == tlid: + context.core.playback.next() + tl_tracks = context.core.tracklist.remove(tlid=tlid).get() + if not tl_tracks: raise MpdNoExistError('No such song', command='deleteid') @@ -163,8 +162,10 @@ def moveid(context, tlid, to): """ tlid = int(tlid) to = int(to) - tl_track = context.core.tracklist.get(tlid=tlid).get() - position = context.core.tracklist.index(tl_track).get() + tl_tracks = context.core.tracklist.filter(tlid=tlid).get() + if not tl_tracks: + raise MpdNoExistError('No such song', command='moveid') + position = context.core.tracklist.index(tl_tracks[0]).get() context.core.tracklist.move(position, position + 1, to) @@ -199,12 +200,11 @@ def playlistfind(context, tag, needle): - does not add quotes around the tag. """ if tag == 'filename': - try: - tl_track = context.core.tracklist.get(uri=needle).get() - position = context.core.tracklist.index(tl_track).get() - return translator.track_to_mpd_format(tl_track, position=position) - except LookupError: + tl_tracks = context.core.tracklist.filter(uri=needle).get() + if not tl_tracks: return None + position = context.core.tracklist.index(tl_tracks[0]).get() + return translator.track_to_mpd_format(tl_tracks[0], position=position) raise MpdNotImplemented # TODO @@ -219,13 +219,12 @@ def playlistid(context, tlid=None): and specifies a single song to display info for. """ if tlid is not None: - try: - tlid = int(tlid) - tl_track = context.core.tracklist.get(tlid=tlid).get() - position = context.core.tracklist.index(tl_track).get() - return translator.track_to_mpd_format(tl_track, position=position) - except LookupError: + tlid = int(tlid) + tl_tracks = context.core.tracklist.filter(tlid=tlid).get() + if not tl_tracks: raise MpdNoExistError('No such song', command='playlistid') + position = context.core.tracklist.index(tl_tracks[0]).get() + return translator.track_to_mpd_format(tl_tracks[0], position=position) else: return translator.tracks_to_mpd_format( context.core.tracklist.tl_tracks.get()) @@ -385,8 +384,10 @@ def swapid(context, tlid1, tlid2): """ tlid1 = int(tlid1) tlid2 = int(tlid2) - tl_track1 = context.core.tracklist.get(tlid=tlid1).get() - tl_track2 = context.core.tracklist.get(tlid=tlid2).get() - position1 = context.core.tracklist.index(tl_track1).get() - position2 = context.core.tracklist.index(tl_track2).get() + tl_tracks1 = context.core.tracklist.filter(tlid=tlid1).get() + tl_tracks2 = context.core.tracklist.filter(tlid=tlid2).get() + if not tl_tracks1 or not tl_tracks2: + raise MpdNoExistError('No such song', command='swapid') + position1 = context.core.tracklist.index(tl_tracks1[0]).get() + position2 = context.core.tracklist.index(tl_tracks2[0]).get() swap(context, position1, position2) diff --git a/mopidy/frontends/mpd/protocol/playback.py b/mopidy/frontends/mpd/protocol/playback.py index 74ecfb1c..d166f982 100644 --- a/mopidy/frontends/mpd/protocol/playback.py +++ b/mopidy/frontends/mpd/protocol/playback.py @@ -151,11 +151,10 @@ def playid(context, tlid): tlid = int(tlid) if tlid == -1: return _play_minus_one(context) - try: - tl_track = context.core.tracklist.get(tlid=tlid).get() - return context.core.playback.play(tl_track).get() - except LookupError: + tl_tracks = context.core.tracklist.filter(tlid=tlid).get() + if not tl_tracks: raise MpdNoExistError('No such song', command='playid') + return context.core.playback.play(tl_tracks[0]).get() @handle_request(r'^play (?P-?\d+)$') diff --git a/mopidy/frontends/mpd/protocol/stored_playlists.py b/mopidy/frontends/mpd/protocol/stored_playlists.py index b8ac8c4c..d5d6b2a6 100644 --- a/mopidy/frontends/mpd/protocol/stored_playlists.py +++ b/mopidy/frontends/mpd/protocol/stored_playlists.py @@ -23,11 +23,10 @@ def listplaylist(context, name): file: relative/path/to/file2.ogg file: relative/path/to/file3.mp3 """ - try: - playlist = context.core.playlists.get(name=name).get() - return ['file: %s' % t.uri for t in playlist.tracks] - except LookupError: + playlists = context.core.playlists.filter(name=name).get() + if not playlists: raise MpdNoExistError('No such playlist', command='listplaylist') + return ['file: %s' % t.uri for t in playlists[0].tracks] @handle_request(r'^listplaylistinfo (?P\S+)$') @@ -45,11 +44,10 @@ def listplaylistinfo(context, name): Standard track listing, with fields: file, Time, Title, Date, Album, Artist, Track """ - try: - playlist = context.core.playlists.get(name=name).get() - return playlist_to_mpd_format(playlist) - except LookupError: + playlists = context.core.playlists.filter(name=name).get() + if not playlists: raise MpdNoExistError('No such playlist', command='listplaylistinfo') + return playlist_to_mpd_format(playlists[0]) @handle_request(r'^listplaylists$') @@ -100,11 +98,10 @@ def load(context, name): - ``load`` appends the given playlist to the current playlist. """ - try: - playlist = context.core.playlists.get(name=name).get() - context.core.tracklist.append(playlist.tracks) - except LookupError: + playlists = context.core.playlists.filter(name=name).get() + if not playlists: raise MpdNoExistError('No such playlist', command='load') + context.core.tracklist.append(playlists[0].tracks) @handle_request(r'^playlistadd "(?P[^"]+)" "(?P[^"]+)"$') diff --git a/tests/backends/base/playlists.py b/tests/backends/base/playlists.py index 473caf8c..c162e500 100644 --- a/tests/backends/base/playlists.py +++ b/tests/backends/base/playlists.py @@ -58,43 +58,35 @@ class PlaylistsControllerTest(object): self.assertNotIn(playlist, self.core.playlists.playlists) - def test_get_without_criteria(self): - test = self.core.playlists.get - self.assertRaises(LookupError, test) + def test_filter_without_criteria(self): + self.assertEqual( + self.core.playlists.playlists, self.core.playlists.filter()) - def test_get_with_wrong_cirteria(self): - test = lambda: self.core.playlists.get(name='foo') - self.assertRaises(LookupError, test) + def test_filter_with_wrong_criteria(self): + self.assertEqual([], self.core.playlists.filter(name='foo')) - def test_get_with_right_criteria(self): - playlist1 = self.core.playlists.create('test') - playlist2 = self.core.playlists.get(name='test') - self.assertEqual(playlist1, playlist2) + def test_filter_with_right_criteria(self): + playlist = self.core.playlists.create('test') + playlists = self.core.playlists.filter(name='test') + self.assertEqual([playlist], playlists) - def test_get_by_name_returns_unique_match(self): + def test_filter_by_name_returns_single_match(self): playlist = Playlist(name='b') - self.backend.playlists.playlists = [ - Playlist(name='a'), playlist] - self.assertEqual(playlist, self.core.playlists.get(name='b')) + self.backend.playlists.playlists = [Playlist(name='a'), playlist] + self.assertEqual([playlist], self.core.playlists.filter(name='b')) - def test_get_by_name_returns_first_of_multiple_matches(self): + def test_filter_by_name_returns_multiple_matches(self): playlist = Playlist(name='b') self.backend.playlists.playlists = [ playlist, Playlist(name='a'), Playlist(name='b')] - try: - self.core.playlists.get(name='b') - self.fail('Should raise LookupError if multiple matches') - except LookupError as e: - self.assertEqual('"name=b" match multiple playlists', e[0]) + playlists = self.core.playlists.filter(name='b') + self.assertIn(playlist, playlists) + self.assertEqual(2, len(playlists)) - def test_get_by_name_raises_keyerror_if_no_match(self): + def test_filter_by_name_returns_no_matches(self): self.backend.playlists.playlists = [ Playlist(name='a'), Playlist(name='b')] - try: - self.core.playlists.get(name='c') - self.fail('Should raise LookupError if no match') - except LookupError as e: - self.assertEqual('"name=c" match no playlists', e[0]) + self.assertEqual([], self.core.playlists.filter(name='c')) def test_lookup_finds_playlist_by_uri(self): original_playlist = self.core.playlists.create('test') diff --git a/tests/backends/base/tracklist.py b/tests/backends/base/tracklist.py index 65328f60..a5fbbcb5 100644 --- a/tests/backends/base/tracklist.py +++ b/tests/backends/base/tracklist.py @@ -55,19 +55,56 @@ class TracklistControllerTest(object): self.assertRaises(AssertionError, test) @populate_playlist - def test_get_by_tlid(self): + def test_filter_by_tlid(self): tl_track = self.controller.tl_tracks[1] - self.assertEqual(tl_track, self.controller.get(tlid=tl_track.tlid)) + self.assertEqual( + [tl_track], self.controller.filter(tlid=tl_track.tlid)) @populate_playlist - def test_get_by_uri(self): + def test_filter_by_uri(self): tl_track = self.controller.tl_tracks[1] - self.assertEqual(tl_track, self.controller.get(uri=tl_track.track.uri)) + self.assertEqual( + [tl_track], self.controller.filter(uri=tl_track.track.uri)) @populate_playlist - def test_get_by_uri_raises_error_for_invalid_uri(self): - test = lambda: self.controller.get(uri='foobar') - self.assertRaises(LookupError, test) + def test_filter_by_uri_returns_nothing_for_invalid_uri(self): + self.assertEqual([], self.controller.filter(uri='foobar')) + + def test_filter_by_uri_returns_single_match(self): + track = Track(uri='a') + self.controller.append([Track(uri='z'), track, Track(uri='y')]) + self.assertEqual(track, self.controller.filter(uri='a')[0].track) + + def test_filter_by_uri_returns_multiple_matches(self): + track = Track(uri='a') + self.controller.append([Track(uri='z'), track, track]) + tl_tracks = self.controller.filter(uri='a') + self.assertEqual(track, tl_tracks[0].track) + self.assertEqual(track, tl_tracks[1].track) + + def test_filter_by_uri_returns_nothing_if_no_match(self): + self.controller.playlist = Playlist( + tracks=[Track(uri='z'), Track(uri='y')]) + self.assertEqual([], self.controller.filter(uri='a')) + + def test_filter_by_multiple_criteria_returns_elements_matching_all(self): + track1 = Track(uri='a', name='x') + track2 = Track(uri='b', name='x') + track3 = Track(uri='b', name='y') + self.controller.append([track1, track2, track3]) + self.assertEqual( + track1, self.controller.filter(uri='a', name='x')[0].track) + self.assertEqual( + track2, self.controller.filter(uri='b', name='x')[0].track) + self.assertEqual( + track3, self.controller.filter(uri='b', name='y')[0].track) + + def test_filter_by_criteria_that_is_not_present_in_all_elements(self): + track1 = Track() + track2 = Track(uri='b') + track3 = Track() + self.controller.append([track1, track2, track3]) + self.assertEqual(track2, self.controller.filter(uri='b')[0].track) @populate_playlist def test_clear(self): @@ -85,45 +122,6 @@ class TracklistControllerTest(object): self.controller.clear() self.assertEqual(self.playback.state, PlaybackState.STOPPED) - def test_get_by_uri_returns_unique_match(self): - track = Track(uri='a') - self.controller.append([Track(uri='z'), track, Track(uri='y')]) - self.assertEqual(track, self.controller.get(uri='a').track) - - def test_get_by_uri_raises_error_if_multiple_matches(self): - track = Track(uri='a') - self.controller.append([Track(uri='z'), track, track]) - try: - self.controller.get(uri='a') - self.fail('Should raise LookupError if multiple matches') - except LookupError as e: - self.assertEqual('"uri=a" match multiple tracks', e[0]) - - def test_get_by_uri_raises_error_if_no_match(self): - self.controller.playlist = Playlist( - tracks=[Track(uri='z'), Track(uri='y')]) - try: - self.controller.get(uri='a') - self.fail('Should raise LookupError if no match') - except LookupError as e: - self.assertEqual('"uri=a" match no tracks', e[0]) - - def test_get_by_multiple_criteria_returns_elements_matching_all(self): - track1 = Track(uri='a', name='x') - track2 = Track(uri='b', name='x') - track3 = Track(uri='b', name='y') - self.controller.append([track1, track2, track3]) - self.assertEqual(track1, self.controller.get(uri='a', name='x').track) - self.assertEqual(track2, self.controller.get(uri='b', name='x').track) - self.assertEqual(track3, self.controller.get(uri='b', name='y').track) - - def test_get_by_criteria_that_is_not_present_in_all_elements(self): - track1 = Track() - track2 = Track(uri='b') - track3 = Track() - self.controller.append([track1, track2, track3]) - self.assertEqual(track2, self.controller.get(uri='b').track) - def test_append_appends_to_the_tracklist(self): self.controller.append([Track(uri='a'), Track(uri='b')]) self.assertEqual(len(self.controller.tracks), 2) @@ -222,13 +220,11 @@ class TracklistControllerTest(object): self.assertEqual(track2, self.controller.tracks[1]) @populate_playlist - def test_removing_track_that_does_not_exist(self): - test = lambda: self.controller.remove(uri='/nonexistant') - self.assertRaises(LookupError, test) + def test_removing_track_that_does_not_exist_does_nothing(self): + self.controller.remove(uri='/nonexistant') - def test_removing_from_empty_playlist(self): - test = lambda: self.controller.remove(uri='/nonexistant') - self.assertRaises(LookupError, test) + def test_removing_from_empty_playlist_does_nothing(self): + self.controller.remove(uri='/nonexistant') @populate_playlist def test_shuffle(self): diff --git a/tests/frontends/mpd/protocol/current_playlist_test.py b/tests/frontends/mpd/protocol/current_playlist_test.py index f5f15f81..dd1ba57e 100644 --- a/tests/frontends/mpd/protocol/current_playlist_test.py +++ b/tests/frontends/mpd/protocol/current_playlist_test.py @@ -214,6 +214,11 @@ class CurrentPlaylistHandlerTest(protocol.BaseTestCase): self.assertEqual(tracks[5].name, 'f') self.assertInResponse('OK') + def test_moveid_with_tlid_not_found_in_tracklist_should_ack(self): + self.sendRequest('moveid "9" "0"') + self.assertEqualResponse( + 'ACK [50@0] {moveid} No such song') + def test_playlist_returns_same_as_playlistinfo(self): playlist_response = self.sendRequest('playlist') playlistinfo_response = self.sendRequest('playlistinfo') @@ -505,3 +510,15 @@ class CurrentPlaylistHandlerTest(protocol.BaseTestCase): self.assertEqual(tracks[4].name, 'b') self.assertEqual(tracks[5].name, 'f') self.assertInResponse('OK') + + def test_swapid_with_first_id_unknown_should_ack(self): + self.core.tracklist.append([Track()]) + self.sendRequest('swapid "0" "4"') + self.assertEqualResponse( + 'ACK [50@0] {swapid} No such song') + + def test_swapid_with_second_id_unknown_should_ack(self): + self.core.tracklist.append([Track()]) + self.sendRequest('swapid "4" "0"') + self.assertEqualResponse( + 'ACK [50@0] {swapid} No such song')