From 1e46e9a94dcdf42b2f93ffd033c9a458795d3c94 Mon Sep 17 00:00:00 2001 From: Mark Greenwood Date: Sun, 5 Jul 2015 13:17:39 +0100 Subject: [PATCH 1/3] mpd:Update protocol version to 0.19 Since mpd 0.19, it has concatenated multiple values using a ';' character. Mopidy has been using ', '. This makes mopidy use a ';' for all artist-related values. In mpd 0.18, multiple values were displayed as multiple lines in the output, hence this change bumps the mpd protocol version to 0.19 to reflect the new behaviour. --- mopidy/mpd/protocol/__init__.py | 4 +-- mopidy/mpd/translator.py | 57 +++++++++++++++++++++------------ 2 files changed, 39 insertions(+), 22 deletions(-) diff --git a/mopidy/mpd/protocol/__init__.py b/mopidy/mpd/protocol/__init__.py index e6b88dbd..b69d5a2a 100644 --- a/mopidy/mpd/protocol/__init__.py +++ b/mopidy/mpd/protocol/__init__.py @@ -22,8 +22,8 @@ ENCODING = 'UTF-8' #: The MPD protocol uses ``\n`` as line terminator. LINE_TERMINATOR = '\n' -#: The MPD protocol version is 0.17.0. -VERSION = '0.17.0' +#: The MPD protocol version is 0.19.0. +VERSION = '0.19.0' def load_protocol_modules(): diff --git a/mopidy/mpd/translator.py b/mopidy/mpd/translator.py index 8359f86b..49af8e5e 100644 --- a/mopidy/mpd/translator.py +++ b/mopidy/mpd/translator.py @@ -37,7 +37,7 @@ def track_to_mpd_format(track, position=None, stream_title=None): # TODO: only show length if not none, see: # https://github.com/mopidy/mopidy/issues/923#issuecomment-79584110 ('Time', track.length and (track.length // 1000) or 0), - ('Artist', artists_to_mpd_format(track.artists)), + ('Artist', concatenate_multiple_values(track.artists, 'name')), ('Title', track.name or ''), ('Album', track.album and track.album.name or ''), ] @@ -58,26 +58,37 @@ def track_to_mpd_format(track, position=None, stream_title=None): result.append(('Id', tlid)) if track.album is not None and track.album.musicbrainz_id is not None: result.append(('MUSICBRAINZ_ALBUMID', track.album.musicbrainz_id)) - # FIXME don't use first and best artist? - # FIXME don't duplicate following code? + if track.album is not None and track.album.artists: - artists = artists_to_mpd_format(track.album.artists) - result.append(('AlbumArtist', artists)) - artists = [ - a for a in track.album.artists if a.musicbrainz_id is not None] - if artists: - result.append( - ('MUSICBRAINZ_ALBUMARTISTID', artists[0].musicbrainz_id)) + result.append( + ('AlbumArtist', + concatenate_multiple_values(track.album.artists, 'name'))) + musicbrainz_ids = concatenate_multiple_values( + track.album.artists, 'musicbrainz_id') + if musicbrainz_ids: + result.append(('MUSICBRAINZ_ALBUMARTISTID', musicbrainz_ids)) + if track.artists: - artists = [a for a in track.artists if a.musicbrainz_id is not None] - if artists: - result.append(('MUSICBRAINZ_ARTISTID', artists[0].musicbrainz_id)) + musicbrainz_ids = concatenate_multiple_values( + track.artists, 'musicbrainz_id') + if musicbrainz_ids: + result.append(('MUSICBRAINZ_ARTISTID', musicbrainz_ids)) if track.composers: - result.append(('Composer', artists_to_mpd_format(track.composers))) + result.append( + ( + 'Composer', + concatenate_multiple_values(track.composers, 'name') + ) + ) if track.performers: - result.append(('Performer', artists_to_mpd_format(track.performers))) + result.append( + ( + 'Performer', + concatenate_multiple_values(track.performers, 'name') + ) + ) if track.genre: result.append(('Genre', track.genre)) @@ -90,17 +101,23 @@ def track_to_mpd_format(track, position=None, stream_title=None): return result -def artists_to_mpd_format(artists): +def concatenate_multiple_values(artists, attribute): """ - Format track artists for output to MPD client. + Format track artist values for output to MPD client. :param artists: the artists :type track: array of :class:`mopidy.models.Artist` + :param attribute: the artist attribute to use + :type string :rtype: string """ - artists = list(artists) - artists.sort(key=lambda a: a.name) - return ', '.join([a.name for a in artists if a.name]) + # Don't sort the values. MPD doesn't appear to (or if it does it's not + # strict alphabetical). If we just use them in the order in which they come + # in then the musicbrainz ids have a higher chance of staying in sync + return ';'.join( + getattr(a, attribute) + for a in artists if getattr(a, attribute, None) is not None + ) def tracks_to_mpd_format(tracks, start=0, end=None): From a2f2d5f1670ff9ea7a0790dff9845a56ec103566 Mon Sep 17 00:00:00 2001 From: Mark Greenwood Date: Sun, 5 Jul 2015 13:40:45 +0100 Subject: [PATCH 2/3] mpd:Update protocol to 0.19 Update tests to reflect new function names --- tests/mpd/test_translator.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/tests/mpd/test_translator.py b/tests/mpd/test_translator.py index 646a22b2..4127eaa2 100644 --- a/tests/mpd/test_translator.py +++ b/tests/mpd/test_translator.py @@ -103,14 +103,20 @@ class TrackMpdFormatTest(unittest.TestCase): result = translator.track_to_mpd_format(track) self.assertIn(('MUSICBRAINZ_ARTISTID', 'foo'), result) - def test_artists_to_mpd_format(self): + def test_concatenate_multiple_values(self): artists = [Artist(name='ABBA'), Artist(name='Beatles')] - translated = translator.artists_to_mpd_format(artists) - self.assertEqual(translated, 'ABBA, Beatles') + translated = translator.concatenate_multiple_values(artists, 'name') + self.assertEqual(translated, 'ABBA;Beatles') - def test_artists_to_mpd_format_artist_with_no_name(self): + def test_concatenate_muultiple_values_artist_with_no_name(self): artists = [Artist(name=None)] - translated = translator.artists_to_mpd_format(artists) + translated = translator.concatenate_multiple_values(artists, 'name') + self.assertEqual(translated, '') + + def test_concatenate_muultiple_values_artist_with_no_musicbrainz_id(self): + artists = [Artist(name="Jah Wobble")] + translated = translator.concatenate_multiple_values( + artists, 'musicbrainz_id') self.assertEqual(translated, '') From f3f140c19f3f15422a6ab6b80a9568f4e38eec00 Mon Sep 17 00:00:00 2001 From: Mark Greenwood Date: Mon, 6 Jul 2015 09:26:52 +0100 Subject: [PATCH 3/3] mpd:Update protocol to 0.19 Address issues raised in review: Fix formatting by shortening function name to concat_multi_values Change comments and variable names to reflect generic nature of function Fix typos in tests Default to single quotes for strings --- mopidy/mpd/translator.py | 39 ++++++++++++++---------------------- tests/mpd/test_translator.py | 15 +++++++------- 2 files changed, 22 insertions(+), 32 deletions(-) diff --git a/mopidy/mpd/translator.py b/mopidy/mpd/translator.py index 49af8e5e..2cd9e3ad 100644 --- a/mopidy/mpd/translator.py +++ b/mopidy/mpd/translator.py @@ -37,7 +37,7 @@ def track_to_mpd_format(track, position=None, stream_title=None): # TODO: only show length if not none, see: # https://github.com/mopidy/mopidy/issues/923#issuecomment-79584110 ('Time', track.length and (track.length // 1000) or 0), - ('Artist', concatenate_multiple_values(track.artists, 'name')), + ('Artist', concat_multi_values(track.artists, 'name')), ('Title', track.name or ''), ('Album', track.album and track.album.name or ''), ] @@ -61,34 +61,24 @@ def track_to_mpd_format(track, position=None, stream_title=None): if track.album is not None and track.album.artists: result.append( - ('AlbumArtist', - concatenate_multiple_values(track.album.artists, 'name'))) - musicbrainz_ids = concatenate_multiple_values( + ('AlbumArtist', concat_multi_values(track.album.artists, 'name'))) + musicbrainz_ids = concat_multi_values( track.album.artists, 'musicbrainz_id') if musicbrainz_ids: result.append(('MUSICBRAINZ_ALBUMARTISTID', musicbrainz_ids)) if track.artists: - musicbrainz_ids = concatenate_multiple_values( - track.artists, 'musicbrainz_id') + musicbrainz_ids = concat_multi_values(track.artists, 'musicbrainz_id') if musicbrainz_ids: result.append(('MUSICBRAINZ_ARTISTID', musicbrainz_ids)) if track.composers: result.append( - ( - 'Composer', - concatenate_multiple_values(track.composers, 'name') - ) - ) + ('Composer', concat_multi_values(track.composers, 'name'))) if track.performers: result.append( - ( - 'Performer', - concatenate_multiple_values(track.performers, 'name') - ) - ) + ('Performer', concat_multi_values(track.performers, 'name'))) if track.genre: result.append(('Genre', track.genre)) @@ -101,22 +91,23 @@ def track_to_mpd_format(track, position=None, stream_title=None): return result -def concatenate_multiple_values(artists, attribute): +def concat_multi_values(models, attribute): """ - Format track artist values for output to MPD client. + Format mopidy model values for output to MPD client. - :param artists: the artists - :type track: array of :class:`mopidy.models.Artist` - :param attribute: the artist attribute to use - :type string + :param models: the models + :type models: array of :class:`mopidy.models.Artist`, + :class:`mopidy.models.Album` or :class:`mopidy.models.Track` + :param attribute: the attribute to use + :type attribute: string :rtype: string """ # Don't sort the values. MPD doesn't appear to (or if it does it's not # strict alphabetical). If we just use them in the order in which they come # in then the musicbrainz ids have a higher chance of staying in sync return ';'.join( - getattr(a, attribute) - for a in artists if getattr(a, attribute, None) is not None + getattr(m, attribute) + for m in models if getattr(m, attribute, None) is not None ) diff --git a/tests/mpd/test_translator.py b/tests/mpd/test_translator.py index 4127eaa2..99c87dad 100644 --- a/tests/mpd/test_translator.py +++ b/tests/mpd/test_translator.py @@ -103,20 +103,19 @@ class TrackMpdFormatTest(unittest.TestCase): result = translator.track_to_mpd_format(track) self.assertIn(('MUSICBRAINZ_ARTISTID', 'foo'), result) - def test_concatenate_multiple_values(self): + def test_concat_multi_values(self): artists = [Artist(name='ABBA'), Artist(name='Beatles')] - translated = translator.concatenate_multiple_values(artists, 'name') + translated = translator.concat_multi_values(artists, 'name') self.assertEqual(translated, 'ABBA;Beatles') - def test_concatenate_muultiple_values_artist_with_no_name(self): + def test_concat_multi_values_artist_with_no_name(self): artists = [Artist(name=None)] - translated = translator.concatenate_multiple_values(artists, 'name') + translated = translator.concat_multi_values(artists, 'name') self.assertEqual(translated, '') - def test_concatenate_muultiple_values_artist_with_no_musicbrainz_id(self): - artists = [Artist(name="Jah Wobble")] - translated = translator.concatenate_multiple_values( - artists, 'musicbrainz_id') + def test_concat_multi_values_artist_with_no_musicbrainz_id(self): + artists = [Artist(name='Jah Wobble')] + translated = translator.concat_multi_values(artists, 'musicbrainz_id') self.assertEqual(translated, '')