diff --git a/docs/changelog.rst b/docs/changelog.rst index 40d707a9..09bf743a 100644 --- a/docs/changelog.rst +++ b/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 diff --git a/mopidy/backend.py b/mopidy/backend.py index 0dc656ad..02a624d9 100644 --- a/mopidy/backend.py +++ b/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 - # 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): + def as_list(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 playlists(self, playlists): + def get_items(self, uri): + """ + 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): diff --git a/mopidy/core/playlists.py b/mopidy/core/playlists.py index 15d35aa9..54797abe 100644 --- a/mopidy/core/playlists.py +++ b/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 - """ - Get the available playlists. + def as_list(self): + """ + Get a list of the currently available playlists. - Returns a list of :class:`mopidy.models.Playlist`. - """ - def get_playlists(self, include_tracks=True): - futures = [b.playlists.playlists - for b in self.backends.with_playlists.values()] + 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 + """ + futures = [ + b.playlists.as_list() + for b in self.backends.with_playlists.values()] results = pykka.get_all(futures) - playlists = list(itertools.chain(*results)) - if not include_tracks: - playlists = [p.copy(tracks=[]) for p in playlists] - return playlists + return list(itertools.chain(*results)) + + def get_items(self, uri): + """ + 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 diff --git a/mopidy/m3u/playlists.py b/mopidy/m3u/playlists.py index a753f00a..1fc5b4c3 100644 --- a/mopidy/m3u/playlists.py +++ b/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 playlists(self): - return sorted( - self._playlists.values(), key=operator.attrgetter('name')) + def as_list(self): + refs = [ + Ref.playlist(uri=pl.uri, name=pl.name) + for pl in self._playlists.values()] + return sorted(refs, key=operator.attrgetter('name')) - @playlists.setter - def playlists(self, playlists): - self._playlists = {playlist.uri: playlist for playlist in playlists} + def get_items(self, uri): + playlist = self._playlists.get(uri) + 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)) diff --git a/tests/backend/test_backend.py b/tests/backend/test_backend.py index c72633fb..e6aac76f 100644 --- a/tests/backend/test_backend.py +++ b/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') diff --git a/tests/core/test_playlists.py b/tests/core/test_playlists.py index 55a75767..fecbbdcb 100644 --- a/tests/core/test_playlists.py +++ b/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) diff --git a/tests/dummy_backend.py b/tests/dummy_backend.py index d4441673..9f4a0986 100644 --- a/tests/dummy_backend.py +++ b/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) diff --git a/tests/m3u/test_playlists.py b/tests/m3u/test_playlists.py index fd77348c..07ffc0a3 100644 --- a/tests/m3u/test_playlists.py +++ b/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() - self.backend = actor.M3UBackend.start( - config=self.config, audio=self.audio).proxy() - self.core = core.Core(backends=[self.backend]) + audio = dummy_audio.create_proxy() + backend = actor.M3UBackend.start( + config=self.config, audio=audio).proxy() + 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.assert_(backend.playlists.playlists) - self.assertEqual( - playlist.uri, backend.playlists.playlists[0].uri) - self.assertEqual( - playlist.name, backend.playlists.playlists[0].name) - self.assertEqual( - track.uri, backend.playlists.playlists[0].tracks[0].uri) + self.assertEqual(len(self.core.playlists.as_list()), 1) + result = self.core.playlists.lookup(playlist.uri) + self.assertEqual(playlist.uri, result.uri) + self.assertEqual(playlist.name, result.name) + self.assertEqual(track.uri, result.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): - self.assert_(not self.core.playlists.playlists) + def test_as_list_empty_to_start_with(self): + 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.backend.playlists.playlists = [Playlist(name='a'), playlist] + self.core.playlists.create('a') + 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 = [ - Playlist(name='a'), Playlist(name='b')] + self.core.playlists.create('a') + 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.assertIn(playlist2, self.core.playlists.playlists) + self.assertIsNone(self.core.playlists.lookup(playlist1.uri)) + 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.assertIn(playlist2, self.core.playlists.playlists) + self.assertNotEqual( + 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.assert_(backend.playlists.playlists) - self.assertEqual('m3u:test.m3u', backend.playlists.playlists[0].uri) - self.assertEqual( - playlist.name, backend.playlists.playlists[0].name) - self.assertEqual( - track.uri, backend.playlists.playlists[0].tracks[0].uri) + self.assertEqual(len(self.core.playlists.as_list()), 1) + result = self.core.playlists.lookup('m3u:test.m3u') + self.assertEqual('m3u:test.m3u', result.uri) + self.assertEqual(playlist.name, result.name) + self.assertEqual(track.uri, result.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)