From 05b66ba4a360542d5561996fd4427c67f45d5ebc Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 12 Feb 2015 22:02:59 +0100 Subject: [PATCH 1/7] models: Add basic image model --- mopidy/models.py | 17 +++++++++++++++++ tests/test_models.py | 35 +++++++++++++++++++++++++++-------- 2 files changed, 44 insertions(+), 8 deletions(-) diff --git a/mopidy/models.py b/mopidy/models.py index 758b6c6d..daadf7b8 100644 --- a/mopidy/models.py +++ b/mopidy/models.py @@ -212,6 +212,23 @@ class Ref(ImmutableObject): return cls(**kwargs) +class Image(ImmutableObject): + """ + :param string uri: URI of the image + :param int width: Optional width of image or :class:`None` + :param int height: Optional height of image or :class:`None` + """ + + #: The image URI. Read-only. + uri = None + + #: Optional width of the image or :class:`None`. Read-only. + width = None + + #: Optional height of the image or :class:`None`. Read-only. + height = None + + class Artist(ImmutableObject): """ :param uri: artist URI diff --git a/tests/test_models.py b/tests/test_models.py index ed1586da..af8e0f82 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -4,8 +4,8 @@ import json import unittest from mopidy.models import ( - Album, Artist, ModelJSONEncoder, Playlist, Ref, SearchResult, TlTrack, - Track, model_json_decoder) + Album, Artist, Image, ModelJSONEncoder, Playlist, Ref, SearchResult, + TlTrack, Track, model_json_decoder) class GenericCopyTest(unittest.TestCase): @@ -74,7 +74,7 @@ class RefTest(unittest.TestCase): def test_invalid_kwarg(self): with self.assertRaises(TypeError): - SearchResult(foo='baz') + Ref(foo='baz') def test_repr_without_results(self): self.assertEquals( @@ -123,11 +123,30 @@ class RefTest(unittest.TestCase): self.assertEqual(ref.name, 'bar') self.assertEqual(ref.type, Ref.PLAYLIST) - def test_track_constructor(self): - ref = Ref.track(uri='foo', name='bar') - self.assertEqual(ref.uri, 'foo') - self.assertEqual(ref.name, 'bar') - self.assertEqual(ref.type, Ref.TRACK) + +class ImageTest(unittest.TestCase): + def test_uri(self): + uri = 'an_uri' + image = Image(uri=uri) + self.assertEqual(image.uri, uri) + with self.assertRaises(AttributeError): + image.uri = None + + def test_width(self): + image = Image(width=100) + self.assertEqual(image.width, 100) + with self.assertRaises(AttributeError): + image.width = None + + def test_height(self): + image = Image(height=100) + self.assertEqual(image.height, 100) + with self.assertRaises(AttributeError): + image.height = None + + def test_invalid_kwarg(self): + with self.assertRaises(TypeError): + Image(foo='baz') class ArtistTest(unittest.TestCase): From c0b0e3657a24dd9306cc14c61cbe294025b6af91 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 12 Feb 2015 22:38:42 +0100 Subject: [PATCH 2/7] core: Add core.library.get_images --- mopidy/backend/__init__.py | 8 +++++++ mopidy/core/library.py | 16 +++++++++++++ tests/core/test_library.py | 46 +++++++++++++++++++++++++++++++++++++- 3 files changed, 69 insertions(+), 1 deletion(-) diff --git a/mopidy/backend/__init__.py b/mopidy/backend/__init__.py index 45268f9f..3dc3a28c 100644 --- a/mopidy/backend/__init__.py +++ b/mopidy/backend/__init__.py @@ -92,6 +92,14 @@ class LibraryProvider(object): """ return [] + def get_images(self, uris): + """ + See :meth:`mopidy.core.LibraryController.get_images`. + + *MAY be implemented by subclass.* + """ + return {} + # TODO: replace with search(query, exact=True, ...) def find_exact(self, query=None, uris=None): """ diff --git a/mopidy/core/library.py b/mopidy/core/library.py index 2ada23d4..bdff2ccd 100644 --- a/mopidy/core/library.py +++ b/mopidy/core/library.py @@ -72,6 +72,22 @@ class LibraryController(object): return [] return backend.library.browse(uri).get() + def get_images(self, uris): + """Lookup the images for the given URIs + + :param list uris: list of uris to find images for + :rtype: dict mapping uris to :class:`mopidy.models.Image` instances + """ + futures = [ + backend.library.get_images(backend_uris) + for (backend, backend_uris) + in self._get_backends_to_uris(uris).items() if backend_uris] + + images = {} + for result in pykka.get_all(futures): + images.update(result) + return images + def find_exact(self, query=None, uris=None, **kwargs): """ Search the library for tracks where ``field`` is ``values``. diff --git a/tests/core/test_library.py b/tests/core/test_library.py index 9bd3b244..cece44e1 100644 --- a/tests/core/test_library.py +++ b/tests/core/test_library.py @@ -5,7 +5,7 @@ import unittest import mock from mopidy import backend, core -from mopidy.models import Ref, SearchResult, Track +from mopidy.models import Image, Ref, SearchResult, Track class CoreLibraryTest(unittest.TestCase): @@ -14,6 +14,8 @@ class CoreLibraryTest(unittest.TestCase): self.backend1 = mock.Mock() self.backend1.uri_schemes.get.return_value = ['dummy1'] self.library1 = mock.Mock(spec=backend.LibraryProvider) + self.library1.get_images().get.return_value = {} + self.library1.get_images.reset_mock() self.library1.root_directory.get.return_value = dummy1_root self.backend1.library = self.library1 @@ -21,6 +23,8 @@ class CoreLibraryTest(unittest.TestCase): self.backend2 = mock.Mock() self.backend2.uri_schemes.get.return_value = ['dummy2', 'du2'] self.library2 = mock.Mock(spec=backend.LibraryProvider) + self.library2.get_images().get.return_value = {} + self.library2.get_images.reset_mock() self.library2.root_directory.get.return_value = dummy2_root self.backend2.library = self.library2 @@ -33,6 +37,46 @@ class CoreLibraryTest(unittest.TestCase): self.core = core.Core(mixer=None, backends=[ self.backend1, self.backend2, self.backend3]) + def test_get_images_returns_empty_dict_for_no_uris(self): + self.assertEqual({}, self.core.library.get_images([])) + + def test_get_images_returns_empty_dict_for_unknown_uri(self): + self.assertEqual({}, self.core.library.get_images(['dummy4:bar'])) + + def test_get_images_returns_empty_dict_for_library_less_uri(self): + self.assertEqual({}, self.core.library.get_images(['dummy3:foo'])) + + def test_get_images_maps_uri_to_backend(self): + self.core.library.get_images(['dummy1:track']) + self.library1.get_images.assert_called_once_with(['dummy1:track']) + self.library2.get_images.assert_not_called() + + def test_get_images_maps_uri_to_backends(self): + self.core.library.get_images(['dummy1:track', 'dummy2:track']) + self.library1.get_images.assert_called_once_with(['dummy1:track']) + self.library2.get_images.assert_called_once_with(['dummy2:track']) + + def test_get_images_returns_images(self): + self.library1.get_images().get.return_value = { + 'dummy1:track': Image(uri='uri')} + self.library1.get_images.reset_mock() + + result = self.core.library.get_images(['dummy1:track']) + self.assertEqual({'dummy1:track': Image(uri='uri')}, result) + + def test_get_images_merges_results(self): + self.library1.get_images().get.return_value = { + 'dummy1:track': Image(uri='uri1')} + self.library1.get_images.reset_mock() + self.library2.get_images().get.return_value = { + 'dummy2:track': Image(uri='uri2')} + self.library2.get_images.reset_mock() + + result = self.core.library.get_images(['dummy1:track', 'dummy2:track']) + expected = {'dummy1:track': Image(uri='uri1'), + 'dummy2:track': Image(uri='uri2')} + self.assertEqual(expected, result) + def test_browse_root_returns_dir_ref_for_each_lib_with_root_dir_name(self): result = self.core.library.browse(None) From a3133afe6bf8fc21d3930f4124159cf68058da80 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 12 Feb 2015 22:41:21 +0100 Subject: [PATCH 3/7] docs: Add changelog entry for core.library.get_images --- docs/changelog.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index 89090218..b979944e 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -21,6 +21,9 @@ v0.20.0 (UNRELEASED) :meth:`mopidy.core.Playback.stop`. It was a leaky internal abstraction, which was never intended to be used externally. +- Add :meth:`mopidy.core.LibraryController.get_images` for looking up images + for any URI backends know about. (Fixes :issue:`973`) + **Commands** - Make the ``mopidy`` command print a friendly error message if the From b1b6fb78082361725da81945d8eb3de65a1d18d1 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 12 Feb 2015 23:08:27 +0100 Subject: [PATCH 4/7] tests: Re-add incorrectly removed test case --- tests/test_models.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/test_models.py b/tests/test_models.py index af8e0f82..e7aec877 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -123,6 +123,12 @@ class RefTest(unittest.TestCase): self.assertEqual(ref.name, 'bar') self.assertEqual(ref.type, Ref.PLAYLIST) + def test_track_constructor(self): + ref = Ref.track(uri='foo', name='bar') + self.assertEqual(ref.uri, 'foo') + self.assertEqual(ref.name, 'bar') + self.assertEqual(ref.type, Ref.TRACK) + class ImageTest(unittest.TestCase): def test_uri(self): From 533948f8f8d803a0c741d2ede7405b11adeca755 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 12 Feb 2015 23:10:20 +0100 Subject: [PATCH 5/7] core: Make sure we return list of images in get_images tests --- tests/core/test_library.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/core/test_library.py b/tests/core/test_library.py index cece44e1..fb1f9228 100644 --- a/tests/core/test_library.py +++ b/tests/core/test_library.py @@ -58,23 +58,23 @@ class CoreLibraryTest(unittest.TestCase): def test_get_images_returns_images(self): self.library1.get_images().get.return_value = { - 'dummy1:track': Image(uri='uri')} + 'dummy1:track': [Image(uri='uri')]} self.library1.get_images.reset_mock() result = self.core.library.get_images(['dummy1:track']) - self.assertEqual({'dummy1:track': Image(uri='uri')}, result) + self.assertEqual({'dummy1:track': [Image(uri='uri')]}, result) def test_get_images_merges_results(self): self.library1.get_images().get.return_value = { - 'dummy1:track': Image(uri='uri1')} + 'dummy1:track': [Image(uri='uri1')]} self.library1.get_images.reset_mock() self.library2.get_images().get.return_value = { - 'dummy2:track': Image(uri='uri2')} + 'dummy2:track': [Image(uri='uri2')]} self.library2.get_images.reset_mock() result = self.core.library.get_images(['dummy1:track', 'dummy2:track']) - expected = {'dummy1:track': Image(uri='uri1'), - 'dummy2:track': Image(uri='uri2')} + expected = {'dummy1:track': [Image(uri='uri1')], + 'dummy2:track': [Image(uri='uri2')]} self.assertEqual(expected, result) def test_browse_root_returns_dir_ref_for_each_lib_with_root_dir_name(self): From b7c71b84d5968333495c3997304fad53ceb96179 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 12 Feb 2015 23:51:20 +0100 Subject: [PATCH 6/7] core: Update get_images documenatation --- mopidy/core/library.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/mopidy/core/library.py b/mopidy/core/library.py index bdff2ccd..b3832d5f 100644 --- a/mopidy/core/library.py +++ b/mopidy/core/library.py @@ -75,8 +75,15 @@ class LibraryController(object): def get_images(self, uris): """Lookup the images for the given URIs - :param list uris: list of uris to find images for - :rtype: dict mapping uris to :class:`mopidy.models.Image` instances + Backends can use this to return image URIs for any URI they know about + be it tracks, albums, playlists... The lookup result is a dictionary + mapping the provided URIs to lists of images. + + Unknown URIs or URIs the corresponding backend couldn't find anything + for will simply return an empty list for that URI. + + :param list uris: list of URIsto find images for + :rtype: {uri: [:class:`mopidy.models.Image`]} """ futures = [ backend.library.get_images(backend_uris) From ddd872cdeafaa809a71a7323286bfadd5b2550ca Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 13 Feb 2015 00:06:57 +0100 Subject: [PATCH 7/7] core: Always return an answer for all URIs in get_images Also make sure that results are tuples instead of lists so we don't accidentally give out mutable state. --- mopidy/core/library.py | 13 +++++++------ tests/core/test_library.py | 20 ++++++++++++-------- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/mopidy/core/library.py b/mopidy/core/library.py index b3832d5f..822836a6 100644 --- a/mopidy/core/library.py +++ b/mopidy/core/library.py @@ -82,18 +82,19 @@ class LibraryController(object): Unknown URIs or URIs the corresponding backend couldn't find anything for will simply return an empty list for that URI. - :param list uris: list of URIsto find images for - :rtype: {uri: [:class:`mopidy.models.Image`]} + :param list uris: list of URIs to find images for + :rtype: {uri: tuple of :class:`mopidy.models.Image`} """ futures = [ backend.library.get_images(backend_uris) for (backend, backend_uris) in self._get_backends_to_uris(uris).items() if backend_uris] - images = {} - for result in pykka.get_all(futures): - images.update(result) - return images + results = {uri: tuple() for uri in uris} + for r in pykka.get_all(futures): + for uri, images in r.items(): + results[uri] += tuple(images) + return results def find_exact(self, query=None, uris=None, **kwargs): """ diff --git a/tests/core/test_library.py b/tests/core/test_library.py index fb1f9228..ccf1b349 100644 --- a/tests/core/test_library.py +++ b/tests/core/test_library.py @@ -40,11 +40,13 @@ class CoreLibraryTest(unittest.TestCase): def test_get_images_returns_empty_dict_for_no_uris(self): self.assertEqual({}, self.core.library.get_images([])) - def test_get_images_returns_empty_dict_for_unknown_uri(self): - self.assertEqual({}, self.core.library.get_images(['dummy4:bar'])) + def test_get_images_returns_empty_result_for_unknown_uri(self): + result = self.core.library.get_images(['dummy4:track']) + self.assertEqual({'dummy4:track': tuple()}, result) - def test_get_images_returns_empty_dict_for_library_less_uri(self): - self.assertEqual({}, self.core.library.get_images(['dummy3:foo'])) + def test_get_images_returns_empty_result_for_library_less_uri(self): + result = self.core.library.get_images(['dummy3:track']) + self.assertEqual({'dummy3:track': tuple()}, result) def test_get_images_maps_uri_to_backend(self): self.core.library.get_images(['dummy1:track']) @@ -62,7 +64,7 @@ class CoreLibraryTest(unittest.TestCase): self.library1.get_images.reset_mock() result = self.core.library.get_images(['dummy1:track']) - self.assertEqual({'dummy1:track': [Image(uri='uri')]}, result) + self.assertEqual({'dummy1:track': (Image(uri='uri'),)}, result) def test_get_images_merges_results(self): self.library1.get_images().get.return_value = { @@ -72,9 +74,11 @@ class CoreLibraryTest(unittest.TestCase): 'dummy2:track': [Image(uri='uri2')]} self.library2.get_images.reset_mock() - result = self.core.library.get_images(['dummy1:track', 'dummy2:track']) - expected = {'dummy1:track': [Image(uri='uri1')], - 'dummy2:track': [Image(uri='uri2')]} + result = self.core.library.get_images( + ['dummy1:track', 'dummy2:track', 'dummy3:track', 'dummy4:track']) + expected = {'dummy1:track': (Image(uri='uri1'),), + 'dummy2:track': (Image(uri='uri2'),), + 'dummy3:track': tuple(), 'dummy4:track': tuple()} self.assertEqual(expected, result) def test_browse_root_returns_dir_ref_for_each_lib_with_root_dir_name(self):