diff --git a/docs/changelog.rst b/docs/changelog.rst index 727c7885..a157e417 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -13,6 +13,17 @@ v0.17.0 (UNRELEASED) - The search field ``track`` has been renamed to ``track_name`` to avoid confusion with ``track_no``. (Fixes: :issue:`535`) +- The signature of the tracklist's + :meth:`~mopidy.core.TracklistController.filter` and + :meth:`~mopidy.core.TracklistController.remove` methods have changed. + Previously, they expected e.g. ``tracklist.filter(tlid=17)``. Now, the value + must always be a list, e.g. ``tracklist.filter(tlid=[17])``. This change + allows you to get or remove multiple tracks with a single call, e.g. + ``tracklist.remove(tlid=[1, 2, 7])``. This is especially useful for web + clients, as requests can be batched. This also brings the interface closer to + the library's :meth:`~mopidy.core.LibraryController.find_exact` and + :meth:`~mopidy.core.LibraryController.search` methods. + **Local backend** - Library scanning has been switched back to custom code due to various issues diff --git a/mopidy/core/tracklist.py b/mopidy/core/tracklist.py index dbc81945..d3cc0d75 100644 --- a/mopidy/core/tracklist.py +++ b/mopidy/core/tracklist.py @@ -1,5 +1,6 @@ from __future__ import unicode_literals +import collections import logging import random @@ -292,36 +293,51 @@ class TracklistController(object): """ Filter the tracklist by the given criterias. + A criteria consists of a model field to check and a list of values to + compare it against. If the model field matches one of the values, it + may be returned. + + Only tracks that matches all the given criterias are returned. + Examples:: - # Returns track with TLID 7 (tracklist ID) - filter({'tlid': 7}) - filter(tlid=7) + # Returns tracks with TLIDs 1, 2, 3, or 4 (tracklist ID) + filter({'tlid': [1, 2, 3, 4]}) + filter(tlid=[1, 2, 3, 4]) - # Returns track with ID 1 - filter({'id': 1}) - filter(id=1) + # Returns track with IDs 1, 5, or 7 + filter({'id': [1, 5, 7]}) + filter(id=[1, 5, 7]) - # Returns track with URI 'xyz' - filter({'uri': 'xyz'}) - filter(uri='xyz') + # Returns track with URIs 'xyz' or 'abc' + filter({'uri': ['xyz', 'abc']}) + filter(uri=['xyz', 'abc']) - # Returns track with ID 1 and URI 'xyz' - filter({'id': 1, 'uri': 'xyz'}) - filter(id=1, uri='xyz') + # Returns tracks with ID 1 and URI 'xyz' + filter({'id': [1], 'uri': ['xyz']}) + filter(id=[1], uri=['xyz']) + + # Returns track with a matching ID (1, 3 or 6) and a matching URI + # ('xyz' or 'abc') + filter({'id': [1, 3, 6], 'uri': ['xyz', 'abc']}) + filter(id=[1, 3, 6], uri=['xyz', 'abc']) :param criteria: on or more criteria to match by - :type criteria: dict + :type criteria: dict, of (string, list) pairs :rtype: list of :class:`mopidy.models.TlTrack` """ criteria = criteria or kwargs matches = self._tl_tracks - for (key, value) in criteria.iteritems(): + for (key, values) in criteria.iteritems(): + if (not isinstance(values, collections.Iterable) + or isinstance(values, basestring)): + # Fail hard if anyone is using the <0.17 calling style + raise ValueError('Filter values must be iterable: %r' % values) if key == 'tlid': - matches = filter(lambda ct: ct.tlid == value, matches) + matches = filter(lambda ct: ct.tlid in values, matches) else: matches = filter( - lambda ct: getattr(ct.track, key) == value, matches) + lambda ct: getattr(ct.track, key) in values, matches) return matches def move(self, start, end, to_position): @@ -435,7 +451,7 @@ class TracklistController(object): """Private method used by :class:`mopidy.core.PlaybackController`.""" if not self.consume: return False - self.remove(tlid=tl_track.tlid) + self.remove(tlid=[tl_track.tlid]) return True def _trigger_tracklist_changed(self): diff --git a/mopidy/frontends/mpd/protocol/current_playlist.py b/mopidy/frontends/mpd/protocol/current_playlist.py index 20452203..bc040067 100644 --- a/mopidy/frontends/mpd/protocol/current_playlist.py +++ b/mopidy/frontends/mpd/protocol/current_playlist.py @@ -76,7 +76,7 @@ def delete_range(context, start, end=None): if not tl_tracks: raise MpdArgError('Bad song index', command='delete') for (tlid, _) in tl_tracks: - context.core.tracklist.remove(tlid=tlid) + context.core.tracklist.remove(tlid=[tlid]) @handle_request(r'^delete "(?P\d+)"$') @@ -86,7 +86,7 @@ def delete_songpos(context, songpos): songpos = int(songpos) (tlid, _) = context.core.tracklist.slice( songpos, songpos + 1).get()[0] - context.core.tracklist.remove(tlid=tlid) + context.core.tracklist.remove(tlid=[tlid]) except IndexError: raise MpdArgError('Bad song index', command='delete') @@ -101,7 +101,7 @@ def deleteid(context, tlid): Deletes the song ``SONGID`` from the playlist """ tlid = int(tlid) - tl_tracks = context.core.tracklist.remove(tlid=tlid).get() + tl_tracks = context.core.tracklist.remove(tlid=[tlid]).get() if not tl_tracks: raise MpdNoExistError('No such song', command='deleteid') @@ -157,7 +157,7 @@ def moveid(context, tlid, to): """ tlid = int(tlid) to = int(to) - tl_tracks = context.core.tracklist.filter(tlid=tlid).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() @@ -195,7 +195,7 @@ def playlistfind(context, tag, needle): - does not add quotes around the tag. """ if tag == 'filename': - tl_tracks = context.core.tracklist.filter(uri=needle).get() + tl_tracks = context.core.tracklist.filter(uri=[needle]).get() if not tl_tracks: return None position = context.core.tracklist.index(tl_tracks[0]).get() @@ -215,7 +215,7 @@ def playlistid(context, tlid=None): """ if tlid is not None: tlid = int(tlid) - tl_tracks = context.core.tracklist.filter(tlid=tlid).get() + 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() @@ -380,8 +380,8 @@ def swapid(context, tlid1, tlid2): """ tlid1 = int(tlid1) tlid2 = int(tlid2) - tl_tracks1 = context.core.tracklist.filter(tlid=tlid1).get() - tl_tracks2 = context.core.tracklist.filter(tlid=tlid2).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() diff --git a/mopidy/frontends/mpd/protocol/playback.py b/mopidy/frontends/mpd/protocol/playback.py index b9289d8a..0d6bfe75 100644 --- a/mopidy/frontends/mpd/protocol/playback.py +++ b/mopidy/frontends/mpd/protocol/playback.py @@ -151,7 +151,7 @@ def playid(context, tlid): tlid = int(tlid) if tlid == -1: return _play_minus_one(context) - tl_tracks = context.core.tracklist.filter(tlid=tlid).get() + 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() diff --git a/tests/backends/local/tracklist_test.py b/tests/backends/local/tracklist_test.py index 8310ce1a..ac135a25 100644 --- a/tests/backends/local/tracklist_test.py +++ b/tests/backends/local/tracklist_test.py @@ -71,34 +71,34 @@ class LocalTracklistProviderTest(unittest.TestCase): def test_filter_by_tlid(self): tl_track = self.controller.tl_tracks[1] self.assertEqual( - [tl_track], self.controller.filter(tlid=tl_track.tlid)) + [tl_track], self.controller.filter(tlid=[tl_track.tlid])) @populate_tracklist def test_filter_by_uri(self): tl_track = self.controller.tl_tracks[1] self.assertEqual( - [tl_track], self.controller.filter(uri=tl_track.track.uri)) + [tl_track], self.controller.filter(uri=[tl_track.track.uri])) @populate_tracklist def test_filter_by_uri_returns_nothing_for_invalid_uri(self): - self.assertEqual([], self.controller.filter(uri='foobar')) + self.assertEqual([], self.controller.filter(uri=['foobar'])) def test_filter_by_uri_returns_single_match(self): track = Track(uri='a') self.controller.add([Track(uri='z'), track, Track(uri='y')]) - self.assertEqual(track, self.controller.filter(uri='a')[0].track) + 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.add([Track(uri='z'), track, track]) - tl_tracks = self.controller.filter(uri='a') + 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')) + 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') @@ -106,18 +106,18 @@ class LocalTracklistProviderTest(unittest.TestCase): track3 = Track(uri='b', name='y') self.controller.add([track1, track2, track3]) self.assertEqual( - track1, self.controller.filter(uri='a', name='x')[0].track) + track1, self.controller.filter(uri=['a'], name=['x'])[0].track) self.assertEqual( - track2, self.controller.filter(uri='b', name='x')[0].track) + track2, self.controller.filter(uri=['b'], name=['x'])[0].track) self.assertEqual( - track3, self.controller.filter(uri='b', name='y')[0].track) + 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.add([track1, track2, track3]) - self.assertEqual(track2, self.controller.filter(uri='b')[0].track) + self.assertEqual(track2, self.controller.filter(uri=['b'])[0].track) @populate_tracklist def test_clear(self): @@ -227,17 +227,29 @@ class LocalTracklistProviderTest(unittest.TestCase): track1 = self.controller.tracks[1] track2 = self.controller.tracks[2] version = self.controller.version - self.controller.remove(uri=track1.uri) + self.controller.remove(uri=[track1.uri]) self.assertLess(version, self.controller.version) self.assertNotIn(track1, self.controller.tracks) self.assertEqual(track2, self.controller.tracks[1]) @populate_tracklist def test_removing_track_that_does_not_exist_does_nothing(self): - self.controller.remove(uri='/nonexistant') + self.controller.remove(uri=['/nonexistant']) def test_removing_from_empty_playlist_does_nothing(self): - self.controller.remove(uri='/nonexistant') + self.controller.remove(uri=['/nonexistant']) + + @populate_tracklist + def test_remove_lists(self): + track0 = self.controller.tracks[0] + track1 = self.controller.tracks[1] + track2 = self.controller.tracks[2] + version = self.controller.version + self.controller.remove(uri=[track0.uri, track2.uri]) + self.assertLess(version, self.controller.version) + self.assertNotIn(track0, self.controller.tracks) + self.assertNotIn(track2, self.controller.tracks) + self.assertEqual(track1, self.controller.tracks[0]) @populate_tracklist def test_shuffle(self): diff --git a/tests/core/events_test.py b/tests/core/events_test.py index 6d192b87..5d646840 100644 --- a/tests/core/events_test.py +++ b/tests/core/events_test.py @@ -105,7 +105,7 @@ class BackendEventsTest(unittest.TestCase): self.core.tracklist.add([Track(uri='dummy:a')]).get() send.reset_mock() - self.core.tracklist.remove(uri='dummy:a').get() + self.core.tracklist.remove(uri=['dummy:a']).get() self.assertEqual(send.call_args[0][0], 'tracklist_changed') diff --git a/tests/core/tracklist_test.py b/tests/core/tracklist_test.py index 9f17f6de..596a20a6 100644 --- a/tests/core/tracklist_test.py +++ b/tests/core/tracklist_test.py @@ -37,7 +37,7 @@ class TracklistTest(unittest.TestCase): self.assertEqual(tl_tracks, self.core.tracklist.tl_tracks[-1:]) def test_remove_removes_tl_tracks_matching_query(self): - tl_tracks = self.core.tracklist.remove(name='foo') + tl_tracks = self.core.tracklist.remove(name=['foo']) self.assertEqual(2, len(tl_tracks)) self.assertListEqual(self.tl_tracks[:2], tl_tracks) @@ -46,7 +46,7 @@ class TracklistTest(unittest.TestCase): self.assertListEqual(self.tl_tracks[2:], self.core.tracklist.tl_tracks) def test_remove_works_with_dict_instead_of_kwargs(self): - tl_tracks = self.core.tracklist.remove({'name': 'foo'}) + tl_tracks = self.core.tracklist.remove({'name': ['foo']}) self.assertEqual(2, len(tl_tracks)) self.assertListEqual(self.tl_tracks[:2], tl_tracks) @@ -55,15 +55,21 @@ class TracklistTest(unittest.TestCase): self.assertListEqual(self.tl_tracks[2:], self.core.tracklist.tl_tracks) def test_filter_returns_tl_tracks_matching_query(self): - tl_tracks = self.core.tracklist.filter(name='foo') + tl_tracks = self.core.tracklist.filter(name=['foo']) self.assertEqual(2, len(tl_tracks)) self.assertListEqual(self.tl_tracks[:2], tl_tracks) def test_filter_works_with_dict_instead_of_kwargs(self): - tl_tracks = self.core.tracklist.filter({'name': 'foo'}) + tl_tracks = self.core.tracklist.filter({'name': ['foo']}) self.assertEqual(2, len(tl_tracks)) self.assertListEqual(self.tl_tracks[:2], tl_tracks) + def test_filter_fails_if_values_isnt_iterable(self): + self.assertRaises(ValueError, self.core.tracklist.filter, tlid=3) + + def test_filter_fails_if_values_is_a_string(self): + self.assertRaises(ValueError, self.core.tracklist.filter, uri='a') + # TODO Extract tracklist tests from the base backend tests