From 57798a27572bf2dc26b8278f65966011a5cb4096 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 15 Jan 2014 00:09:57 +0100 Subject: [PATCH 1/9] audio: Ensure tests can be run on their own --- tests/audio/test_scan.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/audio/test_scan.py b/tests/audio/test_scan.py index ed3f8e01..5753ecf3 100644 --- a/tests/audio/test_scan.py +++ b/tests/audio/test_scan.py @@ -3,6 +3,9 @@ from __future__ import unicode_literals import os import unittest +import gobject +gobject.threads_init() + from mopidy import exceptions from mopidy.audio import scan from mopidy.models import Track, Artist, Album From 7209b38aa639371670f95f6632c7df2c821db35e Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 15 Jan 2014 00:26:30 +0100 Subject: [PATCH 2/9] audio: Nest tags in scan data return value --- mopidy/audio/scan.py | 19 ++++----- tests/audio/test_scan.py | 92 +++++++++++++++++++++------------------- 2 files changed, 58 insertions(+), 53 deletions(-) diff --git a/mopidy/audio/scan.py b/mopidy/audio/scan.py index 0c8e3478..46ab6f8f 100644 --- a/mopidy/audio/scan.py +++ b/mopidy/audio/scan.py @@ -52,18 +52,17 @@ class Scanner(object): :return: Dictionary of tags, duration, mtime and uri information. """ try: + data = {'uri': uri} self._setup(uri) - data = self._collect() - # Make sure uri, mtime and duration does not come from tags. - data[b'uri'] = uri - data[b'mtime'] = self._query_mtime(uri) - data[gst.TAG_DURATION] = self._query_duration() + data['tags'] = self._collect() + data['mtime'] = self._query_mtime(uri) + data['duration'] = self._query_duration() finally: self._reset() if self._min_duration_ms is None: return data - elif data[gst.TAG_DURATION] >= self._min_duration_ms * gst.MSECOND: + elif data['duration'] >= self._min_duration_ms * gst.MSECOND: return data raise exceptions.ScannerError('Rejecting file with less than %dms ' @@ -131,8 +130,8 @@ def audio_data_to_track(data): track_kwargs = {} def _retrieve(source_key, target_key, target): - if source_key in data: - target.setdefault(target_key, data[source_key]) + if source_key in data['tags']: + target.setdefault(target_key, data['tags'][source_key]) _retrieve(gst.TAG_ALBUM, 'name', album_kwargs) _retrieve(gst.TAG_TRACK_COUNT, 'num_tracks', album_kwargs) @@ -160,8 +159,8 @@ def audio_data_to_track(data): _retrieve(gst.TAG_LOCATION, 'comment', track_kwargs) _retrieve(gst.TAG_COPYRIGHT, 'comment', track_kwargs) - if gst.TAG_DATE in data and data[gst.TAG_DATE]: - date = data[gst.TAG_DATE] + if gst.TAG_DATE in data['tags'] and data['tags'][gst.TAG_DATE]: + date = data['tags'][gst.TAG_DATE] try: date = datetime.date(date.year, date.month, date.day) except ValueError: diff --git a/tests/audio/test_scan.py b/tests/audio/test_scan.py index 5753ecf3..c3ee568c 100644 --- a/tests/audio/test_scan.py +++ b/tests/audio/test_scan.py @@ -25,26 +25,28 @@ class TranslatorTest(unittest.TestCase): def setUp(self): self.data = { 'uri': 'uri', - 'album': 'albumname', - 'track-number': 1, - 'artist': 'name', - 'composer': 'composer', - 'performer': 'performer', - 'album-artist': 'albumartistname', - 'title': 'trackname', - 'track-count': 2, - 'album-disc-number': 2, - 'album-disc-count': 3, - 'date': FakeGstDate(2006, 1, 1,), - 'container-format': 'ID3 tag', - 'genre': 'genre', 'duration': 4531000000, - 'comment': 'comment', - 'musicbrainz-trackid': 'mbtrackid', - 'musicbrainz-albumid': 'mbalbumid', - 'musicbrainz-artistid': 'mbartistid', - 'musicbrainz-albumartistid': 'mbalbumartistid', 'mtime': 1234, + 'tags': { + 'album': 'albumname', + 'track-number': 1, + 'artist': 'name', + 'composer': 'composer', + 'performer': 'performer', + 'album-artist': 'albumartistname', + 'title': 'trackname', + 'track-count': 2, + 'album-disc-number': 2, + 'album-disc-count': 3, + 'date': FakeGstDate(2006, 1, 1,), + 'container-format': 'ID3 tag', + 'genre': 'genre', + 'comment': 'comment', + 'musicbrainz-trackid': 'mbtrackid', + 'musicbrainz-albumid': 'mbalbumid', + 'musicbrainz-artistid': 'mbartistid', + 'musicbrainz-albumartistid': 'mbalbumartistid', + }, } self.album = { @@ -143,98 +145,98 @@ class TranslatorTest(unittest.TestCase): self.check() def test_missing_track_number(self): - del self.data['track-number'] + del self.data['tags']['track-number'] del self.track['track_no'] self.check() def test_missing_track_count(self): - del self.data['track-count'] + del self.data['tags']['track-count'] del self.album['num_tracks'] self.check() def test_missing_track_name(self): - del self.data['title'] + del self.data['tags']['title'] del self.track['name'] self.check() def test_missing_track_musicbrainz_id(self): - del self.data['musicbrainz-trackid'] + del self.data['tags']['musicbrainz-trackid'] del self.track['musicbrainz_id'] self.check() def test_missing_album_name(self): - del self.data['album'] + del self.data['tags']['album'] del self.album['name'] self.check() def test_missing_album_musicbrainz_id(self): - del self.data['musicbrainz-albumid'] + del self.data['tags']['musicbrainz-albumid'] del self.album['musicbrainz_id'] self.check() def test_missing_artist_name(self): - del self.data['artist'] + del self.data['tags']['artist'] del self.artist['name'] self.check() def test_missing_composer_name(self): - del self.data['composer'] + del self.data['tags']['composer'] del self.composer['name'] self.check() def test_multiple_track_composers(self): - self.data['composer'] = ['composer1', 'composer2'] + self.data['tags']['composer'] = ['composer1', 'composer2'] self.composer = self.composer_multiple self.check() def test_multiple_track_performers(self): - self.data['performer'] = ['performer1', 'performer2'] + self.data['tags']['performer'] = ['performer1', 'performer2'] self.performer = self.performer_multiple self.check() def test_missing_performer_name(self): - del self.data['performer'] + del self.data['tags']['performer'] del self.performer['name'] self.check() def test_missing_artist_musicbrainz_id(self): - del self.data['musicbrainz-artistid'] + del self.data['tags']['musicbrainz-artistid'] del self.artist['musicbrainz_id'] self.check() def test_multiple_track_artists(self): - self.data['artist'] = ['name1', 'name2'] + self.data['tags']['artist'] = ['name1', 'name2'] self.data['musicbrainz-artistid'] = 'mbartistid' self.artist = self.artist_multiple self.check() def test_missing_album_artist(self): - del self.data['album-artist'] + del self.data['tags']['album-artist'] del self.albumartist['name'] self.check() def test_missing_album_artist_musicbrainz_id(self): - del self.data['musicbrainz-albumartistid'] + del self.data['tags']['musicbrainz-albumartistid'] del self.albumartist['musicbrainz_id'] self.check() def test_missing_genre(self): - del self.data['genre'] + del self.data['tags']['genre'] del self.track['genre'] self.check() def test_missing_date(self): - del self.data['date'] + del self.data['tags']['date'] del self.track['date'] self.check() def test_invalid_date(self): - self.data['date'] = FakeGstDate(65535, 1, 1) + self.data['tags']['date'] = FakeGstDate(65535, 1, 1) del self.track['date'] self.check() def test_missing_comment(self): - del self.data['comment'] + del self.data['tags']['comment'] del self.track['comment'] self.check() @@ -263,6 +265,10 @@ class ScannerTest(unittest.TestCase): name = path_to_data_dir(name) self.assertEqual(self.data[name][key], value) + def check_tag(self, name, key, value): + name = path_to_data_dir(name) + self.assertEqual(self.data[name]['tags'][key], value) + def test_data_is_set(self): self.scan(self.find('scanner/simple')) self.assert_(self.data) @@ -287,18 +293,18 @@ class ScannerTest(unittest.TestCase): def test_artist_is_set(self): self.scan(self.find('scanner/simple')) - self.check('scanner/simple/song1.mp3', 'artist', 'name') - self.check('scanner/simple/song1.ogg', 'artist', 'name') + self.check_tag('scanner/simple/song1.mp3', 'artist', 'name') + self.check_tag('scanner/simple/song1.ogg', 'artist', 'name') def test_album_is_set(self): self.scan(self.find('scanner/simple')) - self.check('scanner/simple/song1.mp3', 'album', 'albumname') - self.check('scanner/simple/song1.ogg', 'album', 'albumname') + self.check_tag('scanner/simple/song1.mp3', 'album', 'albumname') + self.check_tag('scanner/simple/song1.ogg', 'album', 'albumname') def test_track_is_set(self): self.scan(self.find('scanner/simple')) - self.check('scanner/simple/song1.mp3', 'title', 'trackname') - self.check('scanner/simple/song1.ogg', 'title', 'trackname') + self.check_tag('scanner/simple/song1.mp3', 'title', 'trackname') + self.check_tag('scanner/simple/song1.ogg', 'title', 'trackname') def test_nonexistant_dir_does_not_fail(self): self.scan(self.find('scanner/does-not-exist')) From 807bedad851b0c79decd14b5b4b9521feb708ddc Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 15 Jan 2014 01:14:08 +0100 Subject: [PATCH 3/9] audio: Change audio data convert to operate on taglists --- mopidy/audio/scan.py | 114 +++++++++++++++++---------------------- tests/audio/test_scan.py | 53 +++++++++--------- 2 files changed, 75 insertions(+), 92 deletions(-) diff --git a/mopidy/audio/scan.py b/mopidy/audio/scan.py index 46ab6f8f..0bee4a9c 100644 --- a/mopidy/audio/scan.py +++ b/mopidy/audio/scan.py @@ -82,7 +82,7 @@ class Scanner(object): """Polls for messages to collect data.""" start = time.time() timeout_s = self._timeout_ms / float(1000) - data = {} + tags = {} while time.time() - start < timeout_s: if not self._bus.have_pending(): @@ -92,14 +92,21 @@ class Scanner(object): if message.type == gst.MESSAGE_ERROR: raise exceptions.ScannerError(message.parse_error()[0]) elif message.type == gst.MESSAGE_EOS: - return data + return tags elif message.type == gst.MESSAGE_ASYNC_DONE: if message.src == self._pipe: - return data + return tags elif message.type == gst.MESSAGE_TAG: + # Taglists are not really dicts, hence the key usage. + # Beyond that, we only keep the last tag for each key, + # as we assume this is the best, and force everything + # to lists. taglist = message.parse_tag() for key in taglist.keys(): - data[key] = taglist[key] + value = taglist[key] + if not isinstance(value, list): + value = [value] + tags[key] = value raise exceptions.ScannerError('Timeout after %dms' % self._timeout_ms) @@ -122,45 +129,45 @@ class Scanner(object): def audio_data_to_track(data): """Convert taglist data + our extras to a track.""" - albumartist_kwargs = {} + tags = data['tags'] album_kwargs = {} - artist_kwargs = {} - composer_kwargs = {} - performer_kwargs = {} track_kwargs = {} - def _retrieve(source_key, target_key, target): - if source_key in data['tags']: - target.setdefault(target_key, data['tags'][source_key]) + def _retrieve(source_key, target_key, target, convert): + if tags.get(source_key, None): + result = convert(tags[source_key]) + target.setdefault(target_key, result) - _retrieve(gst.TAG_ALBUM, 'name', album_kwargs) - _retrieve(gst.TAG_TRACK_COUNT, 'num_tracks', album_kwargs) - _retrieve(gst.TAG_ALBUM_VOLUME_COUNT, 'num_discs', album_kwargs) - _retrieve(gst.TAG_ARTIST, 'name', artist_kwargs) - _retrieve(gst.TAG_COMPOSER, 'name', composer_kwargs) - _retrieve(gst.TAG_PERFORMER, 'name', performer_kwargs) - _retrieve(gst.TAG_ALBUM_ARTIST, 'name', albumartist_kwargs) - _retrieve(gst.TAG_TITLE, 'name', track_kwargs) - _retrieve(gst.TAG_TRACK_NUMBER, 'track_no', track_kwargs) - _retrieve(gst.TAG_ALBUM_VOLUME_NUMBER, 'disc_no', track_kwargs) - _retrieve(gst.TAG_GENRE, 'genre', track_kwargs) - _retrieve(gst.TAG_BITRATE, 'bitrate', track_kwargs) + first = lambda values: values[0] + join = lambda values: ', '.join(values) + artists = lambda values: [Artist(name=v) for v in values] + + _retrieve(gst.TAG_ARTIST, 'artists', track_kwargs, artists) + _retrieve(gst.TAG_COMPOSER, 'composers', track_kwargs, artists) + _retrieve(gst.TAG_PERFORMER, 'performers', track_kwargs, artists) + _retrieve(gst.TAG_TITLE, 'name', track_kwargs, join) + _retrieve(gst.TAG_TRACK_NUMBER, 'track_no', track_kwargs, first) + _retrieve(gst.TAG_ALBUM_VOLUME_NUMBER, 'disc_no', track_kwargs, first) + _retrieve(gst.TAG_GENRE, 'genre', track_kwargs, join) + _retrieve(gst.TAG_BITRATE, 'bitrate', track_kwargs, first) + + _retrieve(gst.TAG_ALBUM, 'name', album_kwargs, join) + _retrieve(gst.TAG_ALBUM_ARTIST, 'artists', album_kwargs, artists) + _retrieve(gst.TAG_TRACK_COUNT, 'num_tracks', album_kwargs, first) + _retrieve(gst.TAG_ALBUM_VOLUME_COUNT, 'num_discs', album_kwargs, first) # Following keys don't seem to have TAG_* constant. - _retrieve('comment', 'comment', track_kwargs) - _retrieve('musicbrainz-trackid', 'musicbrainz_id', track_kwargs) - _retrieve('musicbrainz-artistid', 'musicbrainz_id', artist_kwargs) - _retrieve('musicbrainz-albumid', 'musicbrainz_id', album_kwargs) - _retrieve( - 'musicbrainz-albumartistid', 'musicbrainz_id', albumartist_kwargs) + _retrieve('comment', 'comment', track_kwargs, join) + _retrieve('musicbrainz-trackid', 'musicbrainz_id', track_kwargs, first) + _retrieve('musicbrainz-albumid', 'musicbrainz_id', album_kwargs, first) # For streams, will not override if a better value has already been set. - _retrieve(gst.TAG_ORGANIZATION, 'name', track_kwargs) - _retrieve(gst.TAG_LOCATION, 'comment', track_kwargs) - _retrieve(gst.TAG_COPYRIGHT, 'comment', track_kwargs) + _retrieve(gst.TAG_ORGANIZATION, 'name', track_kwargs, join) + _retrieve(gst.TAG_LOCATION, 'comment', track_kwargs, join) + _retrieve(gst.TAG_COPYRIGHT, 'comment', track_kwargs, join) - if gst.TAG_DATE in data['tags'] and data['tags'][gst.TAG_DATE]: - date = data['tags'][gst.TAG_DATE] + if tags.get(gst.TAG_DATE, None): + date = tags[gst.TAG_DATE][0] try: date = datetime.date(date.year, date.month, date.day) except ValueError: @@ -168,8 +175,13 @@ def audio_data_to_track(data): else: track_kwargs['date'] = date.isoformat() - if albumartist_kwargs: - album_kwargs['artists'] = [Artist(**albumartist_kwargs)] + def _retrive_mb_artistid(source_key, target): + if source_key in tags and len(target.get('artists', [])) == 1: + target['artists'][0] = target['artists'][0].copy( + musicbrainz_id=tags[source_key][0]) + + _retrive_mb_artistid('musicbrainz-artistid', track_kwargs) + _retrive_mb_artistid('musicbrainz-albumartistid', album_kwargs) if data['mtime']: track_kwargs['last_modified'] = int(data['mtime']) @@ -179,34 +191,4 @@ def audio_data_to_track(data): track_kwargs['uri'] = data['uri'] track_kwargs['album'] = Album(**album_kwargs) - - # TODO: this feels like a half assed workaround. we need to be sure that we - # don't suddenly have lists in our models where we expect strings etc - if ('genre' in track_kwargs and - not isinstance(track_kwargs['genre'], basestring)): - track_kwargs['genre'] = ', '.join(track_kwargs['genre']) - - if ('name' in artist_kwargs - and not isinstance(artist_kwargs['name'], basestring)): - track_kwargs['artists'] = [Artist(name=artist) - for artist in artist_kwargs['name']] - else: - track_kwargs['artists'] = [Artist(**artist_kwargs)] - - if ('name' in composer_kwargs - and not isinstance(composer_kwargs['name'], basestring)): - track_kwargs['composers'] = [Artist(name=artist) - for artist in composer_kwargs['name']] - else: - track_kwargs['composers'] = \ - [Artist(**composer_kwargs)] if composer_kwargs else '' - - if ('name' in performer_kwargs - and not isinstance(performer_kwargs['name'], basestring)): - track_kwargs['performers'] = [Artist(name=artist) - for artist in performer_kwargs['name']] - else: - track_kwargs['performers'] = \ - [Artist(**performer_kwargs)] if performer_kwargs else '' - return Track(**track_kwargs) diff --git a/tests/audio/test_scan.py b/tests/audio/test_scan.py index c3ee568c..e3db53ef 100644 --- a/tests/audio/test_scan.py +++ b/tests/audio/test_scan.py @@ -28,24 +28,24 @@ class TranslatorTest(unittest.TestCase): 'duration': 4531000000, 'mtime': 1234, 'tags': { - 'album': 'albumname', - 'track-number': 1, - 'artist': 'name', - 'composer': 'composer', - 'performer': 'performer', - 'album-artist': 'albumartistname', - 'title': 'trackname', - 'track-count': 2, - 'album-disc-number': 2, - 'album-disc-count': 3, - 'date': FakeGstDate(2006, 1, 1,), - 'container-format': 'ID3 tag', - 'genre': 'genre', - 'comment': 'comment', - 'musicbrainz-trackid': 'mbtrackid', - 'musicbrainz-albumid': 'mbalbumid', - 'musicbrainz-artistid': 'mbartistid', - 'musicbrainz-albumartistid': 'mbalbumartistid', + 'album': ['albumname'], + 'track-number': [1], + 'artist': ['name'], + 'composer': ['composer'], + 'performer': ['performer'], + 'album-artist': ['albumartistname'], + 'title': ['trackname'], + 'track-count': [2], + 'album-disc-number': [2], + 'album-disc-count': [3], + 'date': [FakeGstDate(2006, 1, 1,)], + 'container-format': ['ID3 tag'], + 'genre': ['genre'], + 'comment': ['comment'], + 'musicbrainz-trackid': ['mbtrackid'], + 'musicbrainz-albumid': ['mbalbumid'], + 'musicbrainz-artistid': ['mbartistid'], + 'musicbrainz-albumartistid': ['mbalbumartistid'], }, } @@ -115,7 +115,7 @@ class TranslatorTest(unittest.TestCase): and not isinstance(self.artist['name'], basestring)): self.track['artists'] = [Artist(name=artist) for artist in self.artist['name']] - else: + elif 'name' in self.artist: self.track['artists'] = [Artist(**self.artist)] if ('name' in self.composer @@ -213,6 +213,7 @@ class TranslatorTest(unittest.TestCase): def test_missing_album_artist(self): del self.data['tags']['album-artist'] del self.albumartist['name'] + del self.albumartist['musicbrainz_id'] self.check() def test_missing_album_artist_musicbrainz_id(self): @@ -231,7 +232,7 @@ class TranslatorTest(unittest.TestCase): self.check() def test_invalid_date(self): - self.data['tags']['date'] = FakeGstDate(65535, 1, 1) + self.data['tags']['date'] = [FakeGstDate(65535, 1, 1)] del self.track['date'] self.check() @@ -293,18 +294,18 @@ class ScannerTest(unittest.TestCase): def test_artist_is_set(self): self.scan(self.find('scanner/simple')) - self.check_tag('scanner/simple/song1.mp3', 'artist', 'name') - self.check_tag('scanner/simple/song1.ogg', 'artist', 'name') + self.check_tag('scanner/simple/song1.mp3', 'artist', ['name']) + self.check_tag('scanner/simple/song1.ogg', 'artist', ['name']) def test_album_is_set(self): self.scan(self.find('scanner/simple')) - self.check_tag('scanner/simple/song1.mp3', 'album', 'albumname') - self.check_tag('scanner/simple/song1.ogg', 'album', 'albumname') + self.check_tag('scanner/simple/song1.mp3', 'album', ['albumname']) + self.check_tag('scanner/simple/song1.ogg', 'album', ['albumname']) def test_track_is_set(self): self.scan(self.find('scanner/simple')) - self.check_tag('scanner/simple/song1.mp3', 'title', 'trackname') - self.check_tag('scanner/simple/song1.ogg', 'title', 'trackname') + self.check_tag('scanner/simple/song1.mp3', 'title', ['trackname']) + self.check_tag('scanner/simple/song1.ogg', 'title', ['trackname']) def test_nonexistant_dir_does_not_fail(self): self.scan(self.find('scanner/does-not-exist')) From b0b5d4972f756cb2b4128837234f488d371e3910 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 15 Jan 2014 19:39:11 +0100 Subject: [PATCH 4/9] models: Make .copy(foo=None) null out field. --- mopidy/models.py | 8 ++++++-- tests/test_models.py | 4 ++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/mopidy/models.py b/mopidy/models.py index ed371f23..aab69a3f 100644 --- a/mopidy/models.py +++ b/mopidy/models.py @@ -69,10 +69,14 @@ class ImmutableObject(object): data = {} for key in self.__dict__.keys(): public_key = key.lstrip('_') - data[public_key] = values.pop(public_key, self.__dict__[key]) + value = values.pop(public_key, self.__dict__[key]) + if value is not None: + data[public_key] = value for key in values.keys(): if hasattr(self, key): - data[key] = values.pop(key) + value = values.pop(key) + if value is not None: + data[key] = value if values: raise TypeError( 'copy() got an unexpected keyword argument "%s"' % key) diff --git a/tests/test_models.py b/tests/test_models.py index 02cba8f4..5872e0e6 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -53,6 +53,10 @@ class GenericCopyTest(unittest.TestCase): test = lambda: Track().copy(invalid_key=True) self.assertRaises(TypeError, test) + def test_copying_track_to_remove(self): + track = Track(name='foo').copy(name=None) + self.assertEquals(track.__dict__, Track().__dict__) + class RefTest(unittest.TestCase): def test_uri(self): From ac9f106107eaf72a48a6d14cbc97ba5318bef0b3 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 15 Jan 2014 19:39:41 +0100 Subject: [PATCH 5/9] audio: Update how translator test data is built. --- tests/audio/test_scan.py | 189 +++++++++++---------------------------- 1 file changed, 54 insertions(+), 135 deletions(-) diff --git a/tests/audio/test_scan.py b/tests/audio/test_scan.py index e3db53ef..9a225d18 100644 --- a/tests/audio/test_scan.py +++ b/tests/audio/test_scan.py @@ -21,6 +21,7 @@ class FakeGstDate(object): self.day = day +# TODO: keep ids without name? class TranslatorTest(unittest.TestCase): def setUp(self): self.data = { @@ -28,13 +29,13 @@ class TranslatorTest(unittest.TestCase): 'duration': 4531000000, 'mtime': 1234, 'tags': { - 'album': ['albumname'], + 'album': ['album'], 'track-number': [1], - 'artist': ['name'], + 'artist': ['artist'], 'composer': ['composer'], 'performer': ['performer'], - 'album-artist': ['albumartistname'], - 'title': ['trackname'], + 'album-artist': ['albumartist'], + 'title': ['track'], 'track-count': [2], 'album-disc-number': [2], 'album-disc-count': [3], @@ -42,204 +43,122 @@ class TranslatorTest(unittest.TestCase): 'container-format': ['ID3 tag'], 'genre': ['genre'], 'comment': ['comment'], - 'musicbrainz-trackid': ['mbtrackid'], - 'musicbrainz-albumid': ['mbalbumid'], - 'musicbrainz-artistid': ['mbartistid'], - 'musicbrainz-albumartistid': ['mbalbumartistid'], + 'musicbrainz-trackid': ['trackid'], + 'musicbrainz-albumid': ['albumid'], + 'musicbrainz-artistid': ['artistid'], + 'musicbrainz-albumartistid': ['albumartistid'], }, } - self.album = { - 'name': 'albumname', - 'num_tracks': 2, - 'num_discs': 3, - 'musicbrainz_id': 'mbalbumid', - } + artist = Artist(name='artist', musicbrainz_id='artistid') + composer = Artist(name='composer') + performer = Artist(name='performer') + albumartist = Artist(name='albumartist', + musicbrainz_id='albumartistid') - self.artist_single = { - 'name': 'name', - 'musicbrainz_id': 'mbartistid', - } + album = Album(name='album', num_tracks=2, num_discs=3, + musicbrainz_id='albumid', artists=[albumartist]) - self.artist_multiple = { - 'name': ['name1', 'name2'], - 'musicbrainz_id': 'mbartistid', - } + self.track = Track(uri='uri', name='track', date='2006-01-01', + genre='genre', track_no=1, disc_no=2, length=4531, + comment='comment', musicbrainz_id='trackid', + last_modified=1234, album=album, artists=[artist], + composers=[composer], performers=[performer]) - self.artist = self.artist_single - - self.composer_single = { - 'name': 'composer', - } - - self.composer_multiple = { - 'name': ['composer1', 'composer2'], - } - - self.composer = self.composer_single - - self.performer_single = { - 'name': 'performer', - } - - self.performer_multiple = { - 'name': ['performer1', 'performer2'], - } - - self.performer = self.performer_single - - self.albumartist = { - 'name': 'albumartistname', - 'musicbrainz_id': 'mbalbumartistid', - } - - self.track = { - 'uri': 'uri', - 'name': 'trackname', - 'date': '2006-01-01', - 'genre': 'genre', - 'track_no': 1, - 'disc_no': 2, - 'comment': 'comment', - 'length': 4531, - 'musicbrainz_id': 'mbtrackid', - 'last_modified': 1234, - } - - def build_track(self): - if self.albumartist: - self.album['artists'] = [Artist(**self.albumartist)] - self.track['album'] = Album(**self.album) - - if ('name' in self.artist - and not isinstance(self.artist['name'], basestring)): - self.track['artists'] = [Artist(name=artist) - for artist in self.artist['name']] - elif 'name' in self.artist: - self.track['artists'] = [Artist(**self.artist)] - - if ('name' in self.composer - and not isinstance(self.composer['name'], basestring)): - self.track['composers'] = [Artist(name=artist) - for artist in self.composer['name']] - else: - self.track['composers'] = [Artist(**self.composer)] \ - if self.composer else '' - - if ('name' in self.performer - and not isinstance(self.performer['name'], basestring)): - self.track['performers'] = [Artist(name=artist) - for artist in self.performer['name']] - else: - self.track['performers'] = [Artist(**self.performer)] \ - if self.performer else '' - - return Track(**self.track) - - def check(self): - expected = self.build_track() + def check(self, expected): actual = scan.audio_data_to_track(self.data) self.assertEqual(expected, actual) def test_basic_data(self): - self.check() + self.check(self.track) def test_missing_track_number(self): del self.data['tags']['track-number'] - del self.track['track_no'] - self.check() + self.check(self.track.copy(track_no=None)) def test_missing_track_count(self): del self.data['tags']['track-count'] - del self.album['num_tracks'] - self.check() + album = self.track.album.copy(num_tracks=None) + self.check(self.track.copy(album=album)) def test_missing_track_name(self): del self.data['tags']['title'] - del self.track['name'] - self.check() + self.check(self.track.copy(name=None)) def test_missing_track_musicbrainz_id(self): del self.data['tags']['musicbrainz-trackid'] - del self.track['musicbrainz_id'] - self.check() + self.check(self.track.copy(musicbrainz_id=None)) def test_missing_album_name(self): del self.data['tags']['album'] - del self.album['name'] - self.check() + album = self.track.album.copy(name=None) + self.check(self.track.copy(album=album)) def test_missing_album_musicbrainz_id(self): del self.data['tags']['musicbrainz-albumid'] - del self.album['musicbrainz_id'] - self.check() + album = self.track.album.copy(musicbrainz_id=None) + self.check(self.track.copy(album=album)) def test_missing_artist_name(self): del self.data['tags']['artist'] - del self.artist['name'] - self.check() + self.check(self.track.copy(artists=[])) def test_missing_composer_name(self): del self.data['tags']['composer'] - del self.composer['name'] - self.check() + self.check(self.track.copy(composers=[])) def test_multiple_track_composers(self): self.data['tags']['composer'] = ['composer1', 'composer2'] - self.composer = self.composer_multiple - self.check() + composers = [Artist(name='composer1'), Artist(name='composer2')] + self.check(self.track.copy(composers=composers)) def test_multiple_track_performers(self): self.data['tags']['performer'] = ['performer1', 'performer2'] - self.performer = self.performer_multiple - self.check() + performers = [Artist(name='performer1'), Artist(name='performer2')] + self.check(self.track.copy(performers=performers)) def test_missing_performer_name(self): del self.data['tags']['performer'] - del self.performer['name'] - self.check() + self.check(self.track.copy(performers=[])) def test_missing_artist_musicbrainz_id(self): del self.data['tags']['musicbrainz-artistid'] - del self.artist['musicbrainz_id'] - self.check() + artist = list(self.track.artists)[0].copy(musicbrainz_id=None) + self.check(self.track.copy(artists=[artist])) def test_multiple_track_artists(self): self.data['tags']['artist'] = ['name1', 'name2'] - self.data['musicbrainz-artistid'] = 'mbartistid' - self.artist = self.artist_multiple - self.check() + self.data['musicbrainz-artistid'] = 'artistid' + artists = [Artist(name='name1'), Artist(name='name2')] + self.check(self.track.copy(artists=artists)) def test_missing_album_artist(self): del self.data['tags']['album-artist'] - del self.albumartist['name'] - del self.albumartist['musicbrainz_id'] - self.check() + album = self.track.album.copy(artists=[]) + self.check(self.track.copy(album=album)) def test_missing_album_artist_musicbrainz_id(self): del self.data['tags']['musicbrainz-albumartistid'] - del self.albumartist['musicbrainz_id'] - self.check() + albumartist = list(self.track.album.artists)[0] + albumartist = albumartist.copy(musicbrainz_id=None) + album = self.track.album.copy(artists=[albumartist]) + self.check(self.track.copy(album=album)) def test_missing_genre(self): del self.data['tags']['genre'] - del self.track['genre'] - self.check() + self.check(self.track.copy(genre=None)) def test_missing_date(self): del self.data['tags']['date'] - del self.track['date'] - self.check() + self.check(self.track.copy(date=None)) def test_invalid_date(self): self.data['tags']['date'] = [FakeGstDate(65535, 1, 1)] - del self.track['date'] - self.check() + self.check(self.track.copy(date=None)) def test_missing_comment(self): del self.data['tags']['comment'] - del self.track['comment'] - self.check() + self.check(self.track.copy(comment=None)) class ScannerTest(unittest.TestCase): From 3e9cd0c4b78b2b655cee702a8b41a4897f990c67 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 15 Jan 2014 20:03:02 +0100 Subject: [PATCH 6/9] audio: Add tests for all fields that can be converted --- mopidy/audio/scan.py | 4 +- tests/audio/test_scan.py | 211 ++++++++++++++++++++++++++++++--------- 2 files changed, 165 insertions(+), 50 deletions(-) diff --git a/mopidy/audio/scan.py b/mopidy/audio/scan.py index 0bee4a9c..4b83cdc6 100644 --- a/mopidy/audio/scan.py +++ b/mopidy/audio/scan.py @@ -139,7 +139,7 @@ def audio_data_to_track(data): target.setdefault(target_key, result) first = lambda values: values[0] - join = lambda values: ', '.join(values) + join = lambda values: '; '.join(values) artists = lambda values: [Artist(name=v) for v in values] _retrieve(gst.TAG_ARTIST, 'artists', track_kwargs, artists) @@ -151,7 +151,7 @@ def audio_data_to_track(data): _retrieve(gst.TAG_GENRE, 'genre', track_kwargs, join) _retrieve(gst.TAG_BITRATE, 'bitrate', track_kwargs, first) - _retrieve(gst.TAG_ALBUM, 'name', album_kwargs, join) + _retrieve(gst.TAG_ALBUM, 'name', album_kwargs, first) _retrieve(gst.TAG_ALBUM_ARTIST, 'artists', album_kwargs, artists) _retrieve(gst.TAG_TRACK_COUNT, 'num_tracks', album_kwargs, first) _retrieve(gst.TAG_ALBUM_VOLUME_COUNT, 'num_discs', album_kwargs, first) diff --git a/tests/audio/test_scan.py b/tests/audio/test_scan.py index 9a225d18..cc44ecd6 100644 --- a/tests/audio/test_scan.py +++ b/tests/audio/test_scan.py @@ -47,6 +47,7 @@ class TranslatorTest(unittest.TestCase): 'musicbrainz-albumid': ['albumid'], 'musicbrainz-artistid': ['artistid'], 'musicbrainz-albumartistid': ['albumartistid'], + 'bitrate': [1000], }, } @@ -62,81 +63,176 @@ class TranslatorTest(unittest.TestCase): self.track = Track(uri='uri', name='track', date='2006-01-01', genre='genre', track_no=1, disc_no=2, length=4531, comment='comment', musicbrainz_id='trackid', - last_modified=1234, album=album, artists=[artist], - composers=[composer], performers=[performer]) + last_modified=1234, album=album, bitrate=1000, + artists=[artist], composers=[composer], + performers=[performer]) def check(self, expected): actual = scan.audio_data_to_track(self.data) self.assertEqual(expected, actual) - def test_basic_data(self): + def test_track(self): self.check(self.track) - def test_missing_track_number(self): + def test_none_track_length(self): + self.data['duration'] = None + self.check(self.track.copy(length=None)) + + def test_none_track_last_modified(self): + self.data['mtime'] = None + self.check(self.track.copy(last_modified=None)) + + def test_missing_track_no(self): del self.data['tags']['track-number'] self.check(self.track.copy(track_no=None)) - def test_missing_track_count(self): - del self.data['tags']['track-count'] - album = self.track.album.copy(num_tracks=None) - self.check(self.track.copy(album=album)) + def test_multiple_track_no(self): + self.data['tags']['track-number'].append(9) + self.check(self.track) + + def test_missing_track_disc_no(self): + del self.data['tags']['album-disc-number'] + self.check(self.track.copy(disc_no=None)) + + def test_multiple_track_disc_no(self): + self.data['tags']['album-disc-number'].append(9) + self.check(self.track) def test_missing_track_name(self): del self.data['tags']['title'] self.check(self.track.copy(name=None)) + def test_multiple_track_name(self): + self.data['tags']['title'] = ['name1', 'name2'] + self.check(self.track.copy(name='name1; name2')) + def test_missing_track_musicbrainz_id(self): del self.data['tags']['musicbrainz-trackid'] self.check(self.track.copy(musicbrainz_id=None)) + def test_multiple_track_musicbrainz_id(self): + self.data['tags']['musicbrainz-trackid'].append('id') + self.check(self.track) + + def test_missing_track_bitrate(self): + del self.data['tags']['bitrate'] + self.check(self.track.copy(bitrate=None)) + + def test_multiple_track_bitrate(self): + self.data['tags']['bitrate'].append(1234) + self.check(self.track) + + def test_missing_track_genre(self): + del self.data['tags']['genre'] + self.check(self.track.copy(genre=None)) + + def test_multiple_track_genre(self): + self.data['tags']['genre'] = ['genre1', 'genre2'] + self.check(self.track.copy(genre='genre1; genre2')) + + def test_missing_track_date(self): + del self.data['tags']['date'] + self.check(self.track.copy(date=None)) + + def test_multiple_track_date(self): + self.data['tags']['date'].append(FakeGstDate(2030, 1, 1)) + self.check(self.track) + + def test_invalid_track_date(self): + self.data['tags']['date'] = [FakeGstDate(65535, 1, 1)] + self.check(self.track.copy(date=None)) + + def test_missing_track_comment(self): + del self.data['tags']['comment'] + self.check(self.track.copy(comment=None)) + + def test_multiple_track_comment(self): + self.data['tags']['comment'] = ['comment1', 'comment2'] + self.check(self.track.copy(comment='comment1; comment2')) + + def test_missing_track_artist_name(self): + del self.data['tags']['artist'] + self.check(self.track.copy(artists=[])) + + def test_multiple_track_artist_name(self): + self.data['tags']['artist'] = ['name1', 'name2'] + artists = [Artist(name='name1'), Artist(name='name2')] + self.check(self.track.copy(artists=artists)) + + def test_missing_track_artist_musicbrainz_id(self): + del self.data['tags']['musicbrainz-artistid'] + artist = list(self.track.artists)[0].copy(musicbrainz_id=None) + self.check(self.track.copy(artists=[artist])) + + def test_multiple_track_artist_musicbrainz_id(self): + self.data['tags']['musicbrainz-artistid'].append('id') + self.check(self.track) + + def test_missing_track_composer_name(self): + del self.data['tags']['composer'] + self.check(self.track.copy(composers=[])) + + def test_multiple_track_composer_name(self): + self.data['tags']['composer'] = ['composer1', 'composer2'] + composers = [Artist(name='composer1'), Artist(name='composer2')] + self.check(self.track.copy(composers=composers)) + + def test_missing_track_performer_name(self): + del self.data['tags']['performer'] + self.check(self.track.copy(performers=[])) + + def test_multiple_track_performe_name(self): + self.data['tags']['performer'] = ['performer1', 'performer2'] + performers = [Artist(name='performer1'), Artist(name='performer2')] + self.check(self.track.copy(performers=performers)) + def test_missing_album_name(self): del self.data['tags']['album'] album = self.track.album.copy(name=None) self.check(self.track.copy(album=album)) + def test_multiple_album_name(self): + self.data['tags']['album'].append('album2') + self.check(self.track) + def test_missing_album_musicbrainz_id(self): del self.data['tags']['musicbrainz-albumid'] album = self.track.album.copy(musicbrainz_id=None) self.check(self.track.copy(album=album)) - def test_missing_artist_name(self): - del self.data['tags']['artist'] - self.check(self.track.copy(artists=[])) + def test_multiple_album_musicbrainz_id(self): + self.data['tags']['musicbrainz-albumid'].append('id') + self.check(self.track) - def test_missing_composer_name(self): - del self.data['tags']['composer'] - self.check(self.track.copy(composers=[])) + def test_missing_album_num_tracks(self): + del self.data['tags']['track-count'] + album = self.track.album.copy(num_tracks=None) + self.check(self.track.copy(album=album)) - def test_multiple_track_composers(self): - self.data['tags']['composer'] = ['composer1', 'composer2'] - composers = [Artist(name='composer1'), Artist(name='composer2')] - self.check(self.track.copy(composers=composers)) + def test_multiple_album_num_tracks(self): + self.data['tags']['track-count'].append(9) + self.check(self.track) - def test_multiple_track_performers(self): - self.data['tags']['performer'] = ['performer1', 'performer2'] - performers = [Artist(name='performer1'), Artist(name='performer2')] - self.check(self.track.copy(performers=performers)) + def test_missing_album_num_discs(self): + del self.data['tags']['album-disc-count'] + album = self.track.album.copy(num_discs=None) + self.check(self.track.copy(album=album)) - def test_missing_performer_name(self): - del self.data['tags']['performer'] - self.check(self.track.copy(performers=[])) + def test_multiple_album_num_discs(self): + self.data['tags']['album-disc-count'].append(9) + self.check(self.track) - def test_missing_artist_musicbrainz_id(self): - del self.data['tags']['musicbrainz-artistid'] - artist = list(self.track.artists)[0].copy(musicbrainz_id=None) - self.check(self.track.copy(artists=[artist])) - - def test_multiple_track_artists(self): - self.data['tags']['artist'] = ['name1', 'name2'] - self.data['musicbrainz-artistid'] = 'artistid' - artists = [Artist(name='name1'), Artist(name='name2')] - self.check(self.track.copy(artists=artists)) - - def test_missing_album_artist(self): + def test_missing_album_artist_name(self): del self.data['tags']['album-artist'] album = self.track.album.copy(artists=[]) self.check(self.track.copy(album=album)) + def test_multiple_album_artist_name(self): + self.data['tags']['album-artist'] = ['name1', 'name2'] + artists = [Artist(name='name1'), Artist(name='name2')] + album = self.track.album.copy(artists=artists) + self.check(self.track.copy(album=album)) + def test_missing_album_artist_musicbrainz_id(self): del self.data['tags']['musicbrainz-albumartistid'] albumartist = list(self.track.album.artists)[0] @@ -144,21 +240,40 @@ class TranslatorTest(unittest.TestCase): album = self.track.album.copy(artists=[albumartist]) self.check(self.track.copy(album=album)) - def test_missing_genre(self): - del self.data['tags']['genre'] - self.check(self.track.copy(genre=None)) + def test_multiple_album_artist_musicbrainz_id(self): + self.data['tags']['musicbrainz-albumartistid'].append('id') + self.check(self.track) - def test_missing_date(self): - del self.data['tags']['date'] - self.check(self.track.copy(date=None)) + def test_stream_organization_track_name(self): + del self.data['tags']['title'] + self.data['tags']['organization'] = ['organization'] + self.check(self.track.copy(name='organization')) - def test_invalid_date(self): - self.data['tags']['date'] = [FakeGstDate(65535, 1, 1)] - self.check(self.track.copy(date=None)) + def test_multiple_organization_track_name(self): + del self.data['tags']['title'] + self.data['tags']['organization'] = ['organization1', 'organization2'] + self.check(self.track.copy(name='organization1; organization2')) - def test_missing_comment(self): + # TODO: combine all comment types? + def test_stream_location_track_comment(self): del self.data['tags']['comment'] - self.check(self.track.copy(comment=None)) + self.data['tags']['location'] = ['location'] + self.check(self.track.copy(comment='location')) + + def test_multiple_location_track_comment(self): + del self.data['tags']['comment'] + self.data['tags']['location'] = ['location1', 'location2'] + self.check(self.track.copy(comment='location1; location2')) + + def test_stream_copyright_track_comment(self): + del self.data['tags']['comment'] + self.data['tags']['copyright'] = ['copyright'] + self.check(self.track.copy(comment='copyright')) + + def test_multiple_copyright_track_comment(self): + del self.data['tags']['comment'] + self.data['tags']['copyright'] = ['copyright1', 'copyright2'] + self.check(self.track.copy(comment='copyright1; copyright2')) class ScannerTest(unittest.TestCase): From 81b920a9e414daa1499a6b4ed2cfc2bc9f340498 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 15 Jan 2014 20:34:05 +0100 Subject: [PATCH 7/9] model: Update to handle None in sets. Also adds some missing tests for composers and performers. --- mopidy/models.py | 19 ++++++++++--------- tests/test_models.py | 27 +++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 9 deletions(-) diff --git a/mopidy/models.py b/mopidy/models.py index aab69a3f..e1a1270f 100644 --- a/mopidy/models.py +++ b/mopidy/models.py @@ -278,8 +278,8 @@ class Album(ImmutableObject): # actual usage of this field with more than one image. def __init__(self, *args, **kwargs): - self.__dict__['artists'] = frozenset(kwargs.pop('artists', [])) - self.__dict__['images'] = frozenset(kwargs.pop('images', [])) + self.__dict__['artists'] = frozenset(kwargs.pop('artists', None) or []) + self.__dict__['images'] = frozenset(kwargs.pop('images', None) or []) super(Album, self).__init__(*args, **kwargs) @@ -365,9 +365,10 @@ class Track(ImmutableObject): last_modified = 0 def __init__(self, *args, **kwargs): - self.__dict__['artists'] = frozenset(kwargs.pop('artists', [])) - self.__dict__['composers'] = frozenset(kwargs.pop('composers', [])) - self.__dict__['performers'] = frozenset(kwargs.pop('performers', [])) + get = lambda key: frozenset(kwargs.pop(key, None) or []) + self.__dict__['artists'] = get('artists') + self.__dict__['composers'] = get('composers') + self.__dict__['performers'] = get('performers') super(Track, self).__init__(*args, **kwargs) @@ -436,7 +437,7 @@ class Playlist(ImmutableObject): last_modified = None def __init__(self, *args, **kwargs): - self.__dict__['tracks'] = tuple(kwargs.pop('tracks', [])) + self.__dict__['tracks'] = tuple(kwargs.pop('tracks', None) or []) super(Playlist, self).__init__(*args, **kwargs) # TODO: def insert(self, pos, track): ... ? @@ -472,7 +473,7 @@ class SearchResult(ImmutableObject): albums = tuple() def __init__(self, *args, **kwargs): - self.__dict__['tracks'] = tuple(kwargs.pop('tracks', [])) - self.__dict__['artists'] = tuple(kwargs.pop('artists', [])) - self.__dict__['albums'] = tuple(kwargs.pop('albums', [])) + self.__dict__['tracks'] = tuple(kwargs.pop('tracks', None) or []) + self.__dict__['artists'] = tuple(kwargs.pop('artists', None) or []) + self.__dict__['albums'] = tuple(kwargs.pop('albums', None) or []) super(SearchResult, self).__init__(*args, **kwargs) diff --git a/tests/test_models.py b/tests/test_models.py index 5872e0e6..9a4f97b7 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -276,6 +276,9 @@ class AlbumTest(unittest.TestCase): self.assertIn(artist, album.artists) self.assertRaises(AttributeError, setattr, album, 'artists', None) + def test_artists_none(self): + self.assertEqual(set(), Album(artists=None).artists) + def test_num_tracks(self): num_tracks = 11 album = Album(num_tracks=num_tracks) @@ -307,6 +310,9 @@ class AlbumTest(unittest.TestCase): self.assertIn(image, album.images) self.assertRaises(AttributeError, setattr, album, 'images', None) + def test_images_none(self): + self.assertEqual(set(), Album(images=None).images) + def test_invalid_kwarg(self): test = lambda: Album(foo='baz') self.assertRaises(TypeError, test) @@ -476,6 +482,27 @@ class TrackTest(unittest.TestCase): self.assertEqual(set(track.artists), set(artists)) self.assertRaises(AttributeError, setattr, track, 'artists', None) + def test_artists_none(self): + self.assertEqual(set(), Track(artists=None).artists) + + def test_composers(self): + artists = [Artist(name='name1'), Artist(name='name2')] + track = Track(composers=artists) + self.assertEqual(set(track.composers), set(artists)) + self.assertRaises(AttributeError, setattr, track, 'composers', None) + + def test_composers_none(self): + self.assertEqual(set(), Track(composers=None).composers) + + def test_performers(self): + artists = [Artist(name='name1'), Artist(name='name2')] + track = Track(performers=artists) + self.assertEqual(set(track.performers), set(artists)) + self.assertRaises(AttributeError, setattr, track, 'performers', None) + + def test_performers_none(self): + self.assertEqual(set(), Track(performers=None).performers) + def test_album(self): album = Album() track = Track(album=album) From 2dc8282b259ec07acf2d726dee251ac9015e648f Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 15 Jan 2014 20:34:22 +0100 Subject: [PATCH 8/9] audio: Cleanup translator code. This code was trying to be to smart for it's own good. This commit greatly reduces the magic and leaves us with much more straight forward code. --- mopidy/audio/scan.py | 93 +++++++++++++++++++++++++------------------- 1 file changed, 52 insertions(+), 41 deletions(-) diff --git a/mopidy/audio/scan.py b/mopidy/audio/scan.py index 4b83cdc6..b0a18de5 100644 --- a/mopidy/audio/scan.py +++ b/mopidy/audio/scan.py @@ -127,46 +127,63 @@ class Scanner(object): return os.path.getmtime(path.uri_to_path(uri)) +def _artists(tags, artist_name, artist_id=None): + # Name missing, don't set artist + if not tags.get(artist_name): + return None + # One artist name and id, provide artist with id. + if len(tags[artist_name]) == 1 and artist_id in tags: + return [Artist(name=tags[artist_name][0], + musicbrainz_id=tags[artist_id][0])] + # Multiple artist, provide artists without id. + return [Artist(name=name) for name in tags[artist_name]] + + +def _date(tags): + if not tags.get(gst.TAG_DATE): + return None + try: + date = tags[gst.TAG_DATE][0] + return datetime.date(date.year, date.month, date.day).isoformat() + except ValueError: + return None + + def audio_data_to_track(data): """Convert taglist data + our extras to a track.""" tags = data['tags'] album_kwargs = {} track_kwargs = {} - def _retrieve(source_key, target_key, target, convert): - if tags.get(source_key, None): - result = convert(tags[source_key]) - target.setdefault(target_key, result) + track_kwargs['composers'] = _artists(tags, gst.TAG_COMPOSER) + track_kwargs['performers'] = _artists(tags, gst.TAG_PERFORMER) + track_kwargs['artists'] = _artists( + tags, gst.TAG_ARTIST, 'musicbrainz-artistid') + album_kwargs['artists'] = _artists( + tags, gst.TAG_ALBUM_ARTIST, 'musicbrainz-albumartistid') - first = lambda values: values[0] - join = lambda values: '; '.join(values) - artists = lambda values: [Artist(name=v) for v in values] + track_kwargs['genre'] = '; '.join(tags.get(gst.TAG_GENRE, [])) + track_kwargs['name'] = '; '.join(tags.get(gst.TAG_TITLE, [])) + if not track_kwargs['name']: + track_kwargs['name'] = '; '.join(tags.get(gst.TAG_ORGANIZATION, [])) - _retrieve(gst.TAG_ARTIST, 'artists', track_kwargs, artists) - _retrieve(gst.TAG_COMPOSER, 'composers', track_kwargs, artists) - _retrieve(gst.TAG_PERFORMER, 'performers', track_kwargs, artists) - _retrieve(gst.TAG_TITLE, 'name', track_kwargs, join) - _retrieve(gst.TAG_TRACK_NUMBER, 'track_no', track_kwargs, first) - _retrieve(gst.TAG_ALBUM_VOLUME_NUMBER, 'disc_no', track_kwargs, first) - _retrieve(gst.TAG_GENRE, 'genre', track_kwargs, join) - _retrieve(gst.TAG_BITRATE, 'bitrate', track_kwargs, first) + track_kwargs['comment'] = '; '.join(tags.get('comment', [])) + if not track_kwargs['comment']: + track_kwargs['comment'] = '; '.join(tags.get(gst.TAG_LOCATION, [])) + if not track_kwargs['comment']: + track_kwargs['comment'] = '; '.join(tags.get(gst.TAG_COPYRIGHT, [])) - _retrieve(gst.TAG_ALBUM, 'name', album_kwargs, first) - _retrieve(gst.TAG_ALBUM_ARTIST, 'artists', album_kwargs, artists) - _retrieve(gst.TAG_TRACK_COUNT, 'num_tracks', album_kwargs, first) - _retrieve(gst.TAG_ALBUM_VOLUME_COUNT, 'num_discs', album_kwargs, first) + track_kwargs['track_no'] = tags.get(gst.TAG_TRACK_NUMBER, [None])[0] + track_kwargs['disc_no'] = tags.get(gst.TAG_ALBUM_VOLUME_NUMBER, [None])[0] + track_kwargs['bitrate'] = tags.get(gst.TAG_BITRATE, [None])[0] + track_kwargs['musicbrainz_id'] = tags.get('musicbrainz-trackid', [None])[0] - # Following keys don't seem to have TAG_* constant. - _retrieve('comment', 'comment', track_kwargs, join) - _retrieve('musicbrainz-trackid', 'musicbrainz_id', track_kwargs, first) - _retrieve('musicbrainz-albumid', 'musicbrainz_id', album_kwargs, first) + album_kwargs['name'] = tags.get(gst.TAG_ALBUM, [None])[0] + album_kwargs['num_tracks'] = tags.get(gst.TAG_TRACK_COUNT, [None])[0] + album_kwargs['num_discs'] = tags.get(gst.TAG_ALBUM_VOLUME_COUNT, [None])[0] + album_kwargs['musicbrainz_id'] = tags.get('musicbrainz-albumid', [None])[0] - # For streams, will not override if a better value has already been set. - _retrieve(gst.TAG_ORGANIZATION, 'name', track_kwargs, join) - _retrieve(gst.TAG_LOCATION, 'comment', track_kwargs, join) - _retrieve(gst.TAG_COPYRIGHT, 'comment', track_kwargs, join) - - if tags.get(gst.TAG_DATE, None): + if tags.get(gst.TAG_DATE): date = tags[gst.TAG_DATE][0] try: date = datetime.date(date.year, date.month, date.day) @@ -175,19 +192,13 @@ def audio_data_to_track(data): else: track_kwargs['date'] = date.isoformat() - def _retrive_mb_artistid(source_key, target): - if source_key in tags and len(target.get('artists', [])) == 1: - target['artists'][0] = target['artists'][0].copy( - musicbrainz_id=tags[source_key][0]) + track_kwargs['date'] = _date(tags) + track_kwargs['last_modified'] = int(data.get('mtime') or 0) + track_kwargs['length'] = (data.get(gst.TAG_DURATION) or 0) // gst.MSECOND - _retrive_mb_artistid('musicbrainz-artistid', track_kwargs) - _retrive_mb_artistid('musicbrainz-albumartistid', album_kwargs) - - if data['mtime']: - track_kwargs['last_modified'] = int(data['mtime']) - - if data[gst.TAG_DURATION]: - track_kwargs['length'] = data[gst.TAG_DURATION] // gst.MSECOND + # Clear out any empty values we found + track_kwargs = {k: v for k, v in track_kwargs.items() if v} + album_kwargs = {k: v for k, v in album_kwargs.items() if v} track_kwargs['uri'] = data['uri'] track_kwargs['album'] = Album(**album_kwargs) From c5be900ab48c77968b771f14e1cdd141d6984a98 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 15 Jan 2014 23:53:17 +0100 Subject: [PATCH 9/9] audio: Review fixes --- mopidy/audio/scan.py | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/mopidy/audio/scan.py b/mopidy/audio/scan.py index b0a18de5..56f385e3 100644 --- a/mopidy/audio/scan.py +++ b/mopidy/audio/scan.py @@ -52,11 +52,11 @@ class Scanner(object): :return: Dictionary of tags, duration, mtime and uri information. """ try: - data = {'uri': uri} self._setup(uri) - data['tags'] = self._collect() - data['mtime'] = self._query_mtime(uri) - data['duration'] = self._query_duration() + tags = self._collect() # Ensure collect before queries. + data = {'uri': uri, 'tags': tags, + 'mtime': self._query_mtime(uri), + 'duration': self._query_duration()} finally: self._reset() @@ -97,10 +97,10 @@ class Scanner(object): if message.src == self._pipe: return tags elif message.type == gst.MESSAGE_TAG: - # Taglists are not really dicts, hence the key usage. - # Beyond that, we only keep the last tag for each key, - # as we assume this is the best, and force everything - # to lists. + # Taglists are not really dicts, hence the lack of .items() and + # explicit .keys. We only keep the last tag for each key, as we + # assume this is the best, some formats will produce multiple + # taglists. Lastly we force everything to lists for conformity. taglist = message.parse_tag() for key in taglist.keys(): value = taglist[key] @@ -183,15 +183,6 @@ def audio_data_to_track(data): album_kwargs['num_discs'] = tags.get(gst.TAG_ALBUM_VOLUME_COUNT, [None])[0] album_kwargs['musicbrainz_id'] = tags.get('musicbrainz-albumid', [None])[0] - if tags.get(gst.TAG_DATE): - date = tags[gst.TAG_DATE][0] - try: - date = datetime.date(date.year, date.month, date.day) - except ValueError: - pass # Ignore invalid dates - else: - track_kwargs['date'] = date.isoformat() - track_kwargs['date'] = _date(tags) track_kwargs['last_modified'] = int(data.get('mtime') or 0) track_kwargs['length'] = (data.get(gst.TAG_DURATION) or 0) // gst.MSECOND