Merge pull request #1075 from jodal/feature/new-playlists-api

core/backend: Add new playlists API

docs/changelog.rst

@@ -89,13 +89,27 @@ v1.0.0 (UNRELEASED)
 
   - Made ``mopidy.core.PlaybackController.set_current_tl_track`` internal.
 
+- Add :meth:`mopidy.core.PlaylistsController.as_list`. (Fixes: :issue:`1057`,
+  PR: :issue:`1075`)
+
+- Add :meth:`mopidy.core.PlaylistsController.get_items`. (Fixes: :issue:`1057`,
+  PR: :issue:`1075`)
+
+- **Deprecated:** :meth:`mopidy.core.PlaylistsController.get_playlists`. Use
+  :meth:`~mopidy.core.PlaylistsController.as_list` and
+  :meth:`~mopidy.core.PlaylistsController.get_items` instead. (Fixes:
+  :issue:`1057`, PR: :issue:`1075`)
+
+- **Deprecated:** :meth:`mopidy.core.PlaylistsController.filter`. Use
+  :meth:`~mopidy.core.PlaylistsController.as_list` and filter yourself.
+
 **Backend API**
 
 - Remove default implementation of
   :attr:`mopidy.backend.PlaylistsProvider.playlists`. This is potentially
   backwards incompatible. (PR: :issue:`1046`)
 
-- Changed the API for :class:`mopidy.backend.PlaybackProvider`, note that this
+- Changed the API for :class:`mopidy.backend.PlaybackProvider`. Note that this
   change is **not** backwards compatible for certain backends. These changes
   are crucial to adding gapless in one of the upcoming releases.
   (Fixes: :issue:`1052`, PR: :issue:`1064`)
@@ -114,6 +128,16 @@ v1.0.0 (UNRELEASED)
 
   - :meth:`mopidy.backend.PlaybackProvider.prepare_change` has been added.
 
+- Changed the API for :class:`mopidy.backend.PlaylistsProvider`. Note that this
+  change is **not** backwards compatible. These changes are important to reduce
+  the Mopidy startup time. (Fixes: :issue:`1057`, PR: :issue:`1075`)
+
+  - Add :meth:`mopidy.backend.PlaylistsProvider.as_list`.
+
+  - Add :meth:`mopidy.backend.PlaylistsProvider.get_items`.
+
+  - Remove :attr:`mopidy.backend.PlaylistsProvider.playlists` property.
+
 **Commands**
 
 - Make the ``mopidy`` command print a friendly error message if the

mopidy/backend.py

@@ -300,22 +300,34 @@ class PlaylistsProvider(object):
     def __init__(self, backend):
         self.backend = backend
 
-    # TODO Replace playlists property with a get_playlists() method which
+    def as_list(self):
-    # returns playlist Ref's instead of the gigantic data structures we
-    # currently make available. lookup() should be used for getting full
-    # playlists with all details.
-
-    @property
-    def playlists(self):
         """
-        Currently available playlists.
+        Get a list of the currently available playlists.
 
-        Read/write. List of :class:`mopidy.models.Playlist`.
+        Returns a list of :class:`~mopidy.models.Ref` objects referring to the
+        playlists. In other words, no information about the playlists' content
+        is given.
+
+        :rtype: list of :class:`mopidy.models.Ref`
+
+        .. versionadded:: 1.0
         """
-        return []
+        raise NotImplementedError
 
-    @playlists.setter  # noqa
+    def get_items(self, uri):
-    def playlists(self, playlists):
+        """
+        Get the items in a playlist specified by ``uri``.
+
+        Returns a list of :class:`~mopidy.models.Ref` objects referring to the
+        playlist's items.
+
+        If a playlist with the given ``uri`` doesn't exist, it returns
+        :class:`None`.
+
+        :rtype: list of :class:`mopidy.models.Ref`, or :class:`None`
+
+        .. versionadded:: 1.0
+        """
         raise NotImplementedError
 
     def create(self, name):

mopidy/core/playlists.py

@@ -6,6 +6,7 @@ import urlparse
 import pykka
 
 from mopidy.core import listener
+from mopidy.models import Playlist
 from mopidy.utils.deprecation import deprecated_property
 
 
@@ -16,24 +17,70 @@ class PlaylistsController(object):
         self.backends = backends
         self.core = core
 
-    """
+    def as_list(self):
-    Get the available playlists.
+        """
+        Get a list of the currently available playlists.
 
-    Returns a list of :class:`mopidy.models.Playlist`.
+        Returns a list of :class:`~mopidy.models.Ref` objects referring to the
-    """
+        playlists. In other words, no information about the playlists' content
-    def get_playlists(self, include_tracks=True):
+        is given.
-        futures = [b.playlists.playlists
+
-                   for b in self.backends.with_playlists.values()]
+        :rtype: list of :class:`mopidy.models.Ref`
+
+        .. versionadded:: 1.0
+        """
+        futures = [
+            b.playlists.as_list()
+            for b in self.backends.with_playlists.values()]
         results = pykka.get_all(futures)
-        playlists = list(itertools.chain(*results))
+        return list(itertools.chain(*results))
-        if not include_tracks:
+
-            playlists = [p.copy(tracks=[]) for p in playlists]
+    def get_items(self, uri):
-        return playlists
+        """
+        Get the items in a playlist specified by ``uri``.
+
+        Returns a list of :class:`~mopidy.models.Ref` objects referring to the
+        playlist's items.
+
+        If a playlist with the given ``uri`` doesn't exist, it returns
+        :class:`None`.
+
+        :rtype: list of :class:`mopidy.models.Ref`, or :class:`None`
+
+        .. versionadded:: 1.0
+        """
+        uri_scheme = urlparse.urlparse(uri).scheme
+        backend = self.backends.with_playlists.get(uri_scheme, None)
+        if backend:
+            return backend.playlists.get_items(uri).get()
+
+    def get_playlists(self, include_tracks=True):
+        """
+        Get the available playlists.
+
+        :rtype: list of :class:`mopidy.models.Playlist`
+
+        .. versionchanged:: 1.0
+            If you call the method with ``include_tracks=False``, the
+            :attr:`~mopidy.models.Playlist.last_modified` field of the returned
+            playlists is no longer set.
+
+        .. deprecated:: 1.0
+            Use :meth:`as_list` and :meth:`get_items` instead.
+        """
+        playlist_refs = self.as_list()
+
+        if include_tracks:
+            playlists = [self.lookup(r.uri) for r in playlist_refs]
+            return [pl for pl in playlists if pl is not None]
+        else:
+            return [
+                Playlist(uri=r.uri, name=r.name) for r in playlist_refs]
 
     playlists = deprecated_property(get_playlists)
     """
     .. deprecated:: 1.0
-        Use :meth:`get_playlists` instead.
+        Use :meth:`as_list` and :meth:`get_items` instead.
     """
 
     def create(self, name, uri_scheme=None):
@@ -99,6 +146,9 @@ class PlaylistsController(object):
         :param criteria: one or more criteria to match by
         :type criteria: dict
         :rtype: list of :class:`mopidy.models.Playlist`
+
+        .. deprecated:: 1.0
+            Use :meth:`as_list` and filter yourself.
         """
         criteria = criteria or kwargs
         matches = self.playlists

mopidy/m3u/playlists.py

@@ -8,7 +8,7 @@ import sys
 
 from mopidy import backend
 from mopidy.m3u import translator
-from mopidy.models import Playlist
+from mopidy.models import Playlist, Ref
 
 
 logger = logging.getLogger(__name__)
@@ -22,14 +22,17 @@ class M3UPlaylistsProvider(backend.PlaylistsProvider):
         self._playlists = {}
         self.refresh()
 
-    @property
+    def as_list(self):
-    def playlists(self):
+        refs = [
-        return sorted(
+            Ref.playlist(uri=pl.uri, name=pl.name)
-            self._playlists.values(), key=operator.attrgetter('name'))
+            for pl in self._playlists.values()]
+        return sorted(refs, key=operator.attrgetter('name'))
 
-    @playlists.setter
+    def get_items(self, uri):
-    def playlists(self, playlists):
+        playlist = self._playlists.get(uri)
-        self._playlists = {playlist.uri: playlist for playlist in playlists}
+        if playlist is None:
+            return None
+        return [Ref.track(uri=t.uri, name=t.name) for t in playlist.tracks]
 
     def create(self, name):
         playlist = self._save_m3u(Playlist(name=name))

tests/backend/test_backend.py

@@ -8,6 +8,7 @@ from tests import dummy_backend
 
 
 class LibraryTest(unittest.TestCase):
+
     def test_default_get_images_impl_falls_back_to_album_image(self):
         album = models.Album(images=['imageuri'])
         track = models.Track(uri='trackuri', album=album)
@@ -31,10 +32,14 @@ class LibraryTest(unittest.TestCase):
 
 
 class PlaylistsTest(unittest.TestCase):
-    def test_playlists_default_impl(self):
-        playlists = backend.PlaylistsProvider(backend=None)
 
-        self.assertEqual(playlists.playlists, [])
+    def setUp(self):  # noqa: N802
+        self.provider = backend.PlaylistsProvider(backend=None)
 
+    def test_as_list_default_impl(self):
         with self.assertRaises(NotImplementedError):
-            playlists.playlists = []
+            self.provider.as_list()
+
+    def test_get_items_default_impl(self):
+        with self.assertRaises(NotImplementedError):
+            self.provider.get_items('some uri')

tests/core/test_playlists.py

@@ -5,19 +5,37 @@ import unittest
 import mock
 
 from mopidy import backend, core
-from mopidy.models import Playlist, Track
+from mopidy.models import Playlist, Ref, Track
 
 
 class PlaylistsTest(unittest.TestCase):
     def setUp(self):  # noqa: N802
+        self.plr1a = Ref.playlist(name='A', uri='dummy1:pl:a')
+        self.plr1b = Ref.playlist(name='B', uri='dummy1:pl:b')
+        self.plr2a = Ref.playlist(name='A', uri='dummy2:pl:a')
+        self.plr2b = Ref.playlist(name='B', uri='dummy2:pl:b')
+
+        self.pl1a = Playlist(name='A', tracks=[Track(uri='dummy1:t:a')])
+        self.pl1b = Playlist(name='B', tracks=[Track(uri='dummy1:t:b')])
+        self.pl2a = Playlist(name='A', tracks=[Track(uri='dummy2:t:a')])
+        self.pl2b = Playlist(name='B', tracks=[Track(uri='dummy2:t:b')])
+
+        self.sp1 = mock.Mock(spec=backend.PlaylistsProvider)
+        self.sp1.as_list.return_value.get.return_value = [
+            self.plr1a, self.plr1b]
+        self.sp1.lookup.return_value.get.side_effect = [self.pl1a, self.pl1b]
+
+        self.sp2 = mock.Mock(spec=backend.PlaylistsProvider)
+        self.sp2.as_list.return_value.get.return_value = [
+            self.plr2a, self.plr2b]
+        self.sp2.lookup.return_value.get.side_effect = [self.pl2a, self.pl2b]
+
         self.backend1 = mock.Mock()
         self.backend1.uri_schemes.get.return_value = ['dummy1']
-        self.sp1 = mock.Mock(spec=backend.PlaylistsProvider)
         self.backend1.playlists = self.sp1
 
         self.backend2 = mock.Mock()
         self.backend2.uri_schemes.get.return_value = ['dummy2']
-        self.sp2 = mock.Mock(spec=backend.PlaylistsProvider)
         self.backend2.playlists = self.sp2
 
         # A backend without the optional playlists provider
@@ -26,19 +44,36 @@ class PlaylistsTest(unittest.TestCase):
         self.backend3.has_playlists().get.return_value = False
         self.backend3.playlists = None
 
-        self.pl1a = Playlist(name='A', tracks=[Track(uri='dummy1:a')])
-        self.pl1b = Playlist(name='B', tracks=[Track(uri='dummy1:b')])
-        self.sp1.playlists.get.return_value = [self.pl1a, self.pl1b]
-
-        self.pl2a = Playlist(name='A', tracks=[Track(uri='dummy2:a')])
-        self.pl2b = Playlist(name='B', tracks=[Track(uri='dummy2:b')])
-        self.sp2.playlists.get.return_value = [self.pl2a, self.pl2b]
-
         self.core = core.Core(mixer=None, backends=[
             self.backend3, self.backend1, self.backend2])
 
+    def test_as_list_combines_result_from_backends(self):
+        result = self.core.playlists.as_list()
+
+        self.assertIn(self.plr1a, result)
+        self.assertIn(self.plr1b, result)
+        self.assertIn(self.plr2a, result)
+        self.assertIn(self.plr2b, result)
+
+    def test_get_items_selects_the_matching_backend(self):
+        ref = Ref.track()
+        self.sp2.get_items.return_value.get.return_value = [ref]
+
+        result = self.core.playlists.get_items('dummy2:pl:a')
+
+        self.assertEqual([ref], result)
+        self.assertFalse(self.sp1.get_items.called)
+        self.sp2.get_items.assert_called_once_with('dummy2:pl:a')
+
+    def test_get_items_with_unknown_uri_scheme_does_nothing(self):
+        result = self.core.playlists.get_items('unknown:a')
+
+        self.assertIsNone(result)
+        self.assertFalse(self.sp1.delete.called)
+        self.assertFalse(self.sp2.delete.called)
+
     def test_get_playlists_combines_result_from_backends(self):
-        result = self.core.playlists.playlists
+        result = self.core.playlists.get_playlists()
 
         self.assertIn(self.pl1a, result)
         self.assertIn(self.pl1b, result)

tests/dummy_backend.py

@@ -100,6 +100,17 @@ class DummyPlaylistsProvider(backend.PlaylistsProvider):
         super(DummyPlaylistsProvider, self).__init__(backend)
         self._playlists = []
 
+    def as_list(self):
+        return [
+            Ref.playlist(uri=pl.uri, name=pl.name) for pl in self._playlists]
+
+    def get_items(self, uri):
+        playlist = self._playlists.get(uri)
+        if playlist is None:
+            return
+        return [
+            Ref.track(uri=t.uri, name=t.name) for t in playlist.tracks]
+
     @property
     def playlists(self):
         return copy.copy(self._playlists)

tests/m3u/test_playlists.py

@@ -28,10 +28,10 @@ class M3UPlaylistsProviderTest(unittest.TestCase):
         self.config['m3u']['playlists_dir'] = tempfile.mkdtemp()
         self.playlists_dir = self.config['m3u']['playlists_dir']
 
-        self.audio = dummy_audio.create_proxy()
+        audio = dummy_audio.create_proxy()
-        self.backend = actor.M3UBackend.start(
+        backend = actor.M3UBackend.start(
-            config=self.config, audio=self.audio).proxy()
+            config=self.config, audio=audio).proxy()
-        self.core = core.Core(backends=[self.backend])
+        self.core = core.Core(backends=[backend])
 
     def tearDown(self):  # noqa: N802
         pykka.ActorRegistry.stop_all()
@@ -117,15 +117,11 @@ class M3UPlaylistsProviderTest(unittest.TestCase):
         playlist = playlist.copy(tracks=[track])
         playlist = self.core.playlists.save(playlist)
 
-        backend = self.backend_class(config=self.config, audio=self.audio)
+        self.assertEqual(len(self.core.playlists.as_list()), 1)
-
+        result = self.core.playlists.lookup(playlist.uri)
-        self.assert_(backend.playlists.playlists)
+        self.assertEqual(playlist.uri, result.uri)
-        self.assertEqual(
+        self.assertEqual(playlist.name, result.name)
-            playlist.uri, backend.playlists.playlists[0].uri)
+        self.assertEqual(track.uri, result.tracks[0].uri)
-        self.assertEqual(
-            playlist.name, backend.playlists.playlists[0].name)
-        self.assertEqual(
-            track.uri, backend.playlists.playlists[0].tracks[0].uri)
 
     @unittest.SkipTest
     def test_santitising_of_playlist_filenames(self):
@@ -148,23 +144,23 @@ class M3UPlaylistsProviderTest(unittest.TestCase):
         self.assert_(self.core.playlists.playlists)
         self.assertIn(playlist, self.core.playlists.playlists)
 
-    def test_playlists_empty_to_start_with(self):
+    def test_as_list_empty_to_start_with(self):
-        self.assert_(not self.core.playlists.playlists)
+        self.assertEqual(len(self.core.playlists.as_list()), 0)
 
     def test_delete_non_existant_playlist(self):
         self.core.playlists.delete('m3u:unknown')
 
     def test_delete_playlist_removes_it_from_the_collection(self):
         playlist = self.core.playlists.create('test')
-        self.assertIn(playlist, self.core.playlists.playlists)
+        self.assertEqual(playlist, self.core.playlists.lookup(playlist.uri))
 
         self.core.playlists.delete(playlist.uri)
 
-        self.assertNotIn(playlist, self.core.playlists.playlists)
+        self.assertIsNone(self.core.playlists.lookup(playlist.uri))
 
     def test_delete_playlist_without_file(self):
         playlist = self.core.playlists.create('test')
-        self.assertIn(playlist, self.core.playlists.playlists)
+        self.assertEqual(playlist, self.core.playlists.lookup(playlist.uri))
 
         path = playlist_uri_to_path(playlist.uri, self.playlists_dir)
         self.assertTrue(os.path.exists(path))
@@ -173,11 +169,11 @@ class M3UPlaylistsProviderTest(unittest.TestCase):
         self.assertFalse(os.path.exists(path))
 
         self.core.playlists.delete(playlist.uri)
-        self.assertNotIn(playlist, self.core.playlists.playlists)
+        self.assertIsNone(self.core.playlists.lookup(playlist.uri))
 
     def test_filter_without_criteria(self):
         self.assertEqual(
-            self.core.playlists.playlists, self.core.playlists.filter())
+            self.core.playlists.get_playlists(), self.core.playlists.filter())
 
     def test_filter_with_wrong_criteria(self):
         self.assertEqual([], self.core.playlists.filter(name='foo'))
@@ -188,13 +184,15 @@ class M3UPlaylistsProviderTest(unittest.TestCase):
         self.assertEqual([playlist], playlists)
 
     def test_filter_by_name_returns_single_match(self):
-        playlist = Playlist(name='b')
+        self.core.playlists.create('a')
-        self.backend.playlists.playlists = [Playlist(name='a'), playlist]
+        playlist = self.core.playlists.create('b')
+
         self.assertEqual([playlist], self.core.playlists.filter(name='b'))
 
     def test_filter_by_name_returns_no_matches(self):
-        self.backend.playlists.playlists = [
+        self.core.playlists.create('a')
-            Playlist(name='a'), Playlist(name='b')]
+        self.core.playlists.create('b')
+
         self.assertEqual([], self.core.playlists.filter(name='c'))
 
     def test_lookup_finds_playlist_by_uri(self):
@@ -206,31 +204,32 @@ class M3UPlaylistsProviderTest(unittest.TestCase):
 
     def test_refresh(self):
         playlist = self.core.playlists.create('test')
-        self.assertIn(playlist, self.core.playlists.playlists)
+        self.assertEqual(playlist, self.core.playlists.lookup(playlist.uri))
 
         self.core.playlists.refresh()
 
-        self.assertIn(playlist, self.core.playlists.playlists)
+        self.assertEqual(playlist, self.core.playlists.lookup(playlist.uri))
 
     def test_save_replaces_existing_playlist_with_updated_playlist(self):
         playlist1 = self.core.playlists.create('test1')
-        self.assertIn(playlist1, self.core.playlists.playlists)
+        self.assertEqual(playlist1, self.core.playlists.lookup(playlist1.uri))
 
         playlist2 = playlist1.copy(name='test2')
         playlist2 = self.core.playlists.save(playlist2)
-        self.assertNotIn(playlist1, self.core.playlists.playlists)
+        self.assertIsNone(self.core.playlists.lookup(playlist1.uri))
-        self.assertIn(playlist2, self.core.playlists.playlists)
+        self.assertEqual(playlist2, self.core.playlists.lookup(playlist2.uri))
 
     def test_create_replaces_existing_playlist_with_updated_playlist(self):
         track = Track(uri=generate_song(1))
         playlist1 = self.core.playlists.create('test')
         playlist1 = self.core.playlists.save(playlist1.copy(tracks=[track]))
-        self.assertIn(playlist1, self.core.playlists.playlists)
+        self.assertEqual(playlist1, self.core.playlists.lookup(playlist1.uri))
 
         playlist2 = self.core.playlists.create('test')
         self.assertEqual(playlist1.uri, playlist2.uri)
-        self.assertNotIn(playlist1, self.core.playlists.playlists)
+        self.assertNotEqual(
-        self.assertIn(playlist2, self.core.playlists.playlists)
+            playlist1, self.core.playlists.lookup(playlist1.uri))
+        self.assertEqual(playlist2, self.core.playlists.lookup(playlist1.uri))
 
     def test_save_playlist_with_new_uri(self):
         uri = 'm3u:test.m3u'
@@ -247,14 +246,11 @@ class M3UPlaylistsProviderTest(unittest.TestCase):
         playlist = playlist.copy(tracks=[track])
         playlist = self.core.playlists.save(playlist)
 
-        backend = self.backend_class(config=self.config, audio=self.audio)
+        self.assertEqual(len(self.core.playlists.as_list()), 1)
-
+        result = self.core.playlists.lookup('m3u:test.m3u')
-        self.assert_(backend.playlists.playlists)
+        self.assertEqual('m3u:test.m3u', result.uri)
-        self.assertEqual('m3u:test.m3u', backend.playlists.playlists[0].uri)
+        self.assertEqual(playlist.name, result.name)
-        self.assertEqual(
+        self.assertEqual(track.uri, result.tracks[0].uri)
-            playlist.name, backend.playlists.playlists[0].name)
-        self.assertEqual(
-            track.uri, backend.playlists.playlists[0].tracks[0].uri)
 
     def test_playlist_sort_order(self):
         def check_order(playlists, names):
@@ -264,18 +260,35 @@ class M3UPlaylistsProviderTest(unittest.TestCase):
         self.core.playlists.create('a')
         self.core.playlists.create('b')
 
-        check_order(self.core.playlists.playlists, ['a', 'b', 'c'])
+        check_order(self.core.playlists.as_list(), ['a', 'b', 'c'])
 
         self.core.playlists.refresh()
 
-        check_order(self.core.playlists.playlists, ['a', 'b', 'c'])
+        check_order(self.core.playlists.as_list(), ['a', 'b', 'c'])
 
         playlist = self.core.playlists.lookup('m3u:a.m3u')
         playlist = playlist.copy(name='d')
         playlist = self.core.playlists.save(playlist)
 
-        check_order(self.core.playlists.playlists, ['b', 'c', 'd'])
+        check_order(self.core.playlists.as_list(), ['b', 'c', 'd'])
 
         self.core.playlists.delete('m3u:c.m3u')
 
-        check_order(self.core.playlists.playlists, ['b', 'd'])
+        check_order(self.core.playlists.as_list(), ['b', 'd'])
+
+    def test_get_items_returns_item_refs(self):
+        track = Track(uri='dummy:a', name='A', length=60000)
+        playlist = self.core.playlists.create('test')
+        playlist = self.core.playlists.save(playlist.copy(tracks=[track]))
+
+        item_refs = self.core.playlists.get_items(playlist.uri)
+
+        self.assertEqual(len(item_refs), 1)
+        self.assertEqual(item_refs[0].type, 'track')
+        self.assertEqual(item_refs[0].uri, 'dummy:a')
+        self.assertEqual(item_refs[0].name, 'A')
+
+    def test_get_items_of_unknown_playlist_returns_none(self):
+        item_refs = self.core.playlists.get_items('dummy:unknown')
+
+        self.assertIsNone(item_refs)