Merge pull request #574 from jodal/feature/filter_improvement

tracklist.{filter,remove} with multiple criteria values

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

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):

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<songpos>\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()

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()

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):

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')
 

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