From e0dc26f958f763e6881f1e4a2dea5c8e31b347d2 Mon Sep 17 00:00:00 2001 From: EricJahn Date: Sat, 25 Jul 2015 16:56:34 +0200 Subject: [PATCH] Fix #1045 Filter all empty tagtypes in MPD protocol handlers. In addition return the tagtype list from the mpd protocol command. --- mopidy/mpd/protocol/reflection.py | 5 ++++- mopidy/mpd/protocol/tagtype_list.py | 22 ++++++++++++++++++ mopidy/mpd/translator.py | 25 +++++++++++++++++++-- tests/mpd/protocol/test_reflection.py | 17 ++++++++++++++ tests/mpd/protocol/test_status.py | 8 +++---- tests/mpd/protocol/test_stored_playlists.py | 6 ++--- tests/mpd/test_translator.py | 22 +++++++++--------- 7 files changed, 84 insertions(+), 21 deletions(-) create mode 100644 mopidy/mpd/protocol/tagtype_list.py diff --git a/mopidy/mpd/protocol/reflection.py b/mopidy/mpd/protocol/reflection.py index a3608a96..2f96be48 100644 --- a/mopidy/mpd/protocol/reflection.py +++ b/mopidy/mpd/protocol/reflection.py @@ -1,6 +1,7 @@ from __future__ import absolute_import, unicode_literals from mopidy.mpd import exceptions, protocol +from mopidy.mpd.protocol import tagtype_list @protocol.commands.add('config', list_command=False) @@ -93,7 +94,9 @@ def tagtypes(context): Shows a list of available song metadata. """ - pass # TODO + return [ + ('tagtype', tagtype) for tagtype in tagtype_list.TAGTYPE_LIST + ] @protocol.commands.add('urlhandlers') diff --git a/mopidy/mpd/protocol/tagtype_list.py b/mopidy/mpd/protocol/tagtype_list.py new file mode 100644 index 00000000..575b1aaf --- /dev/null +++ b/mopidy/mpd/protocol/tagtype_list.py @@ -0,0 +1,22 @@ +from __future__ import unicode_literals + + +TAGTYPE_LIST = [ + 'Artist', + 'ArtistSort', + 'Album', + 'AlbumArtist', + 'AlbumArtistSort', + 'Title', + 'Track', + 'Name', + 'Genre', + 'Date', + 'Composer', + 'Performer', + 'Disc', + 'MUSICBRAINZ_ARTISTID', + 'MUSICBRAINZ_ALBUMID', + 'MUSICBRAINZ_ALBUMARTISTID', + 'MUSICBRAINZ_TRACKID', +] diff --git a/mopidy/mpd/translator.py b/mopidy/mpd/translator.py index d7ecb0f1..2f3efd1d 100644 --- a/mopidy/mpd/translator.py +++ b/mopidy/mpd/translator.py @@ -4,6 +4,7 @@ import datetime import re from mopidy.models import TlTrack +from mopidy.mpd.protocol import tagtype_list # TODO: special handling of local:// uri scheme normalize_path_re = re.compile(r'[^/]+') @@ -35,8 +36,6 @@ def track_to_mpd_format(track, position=None, stream_title=None): result = [ ('file', track.uri or ''), - # 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', concat_multi_values(track.artists, 'name')), ('Album', track.album and track.album.name or ''), @@ -97,9 +96,31 @@ def track_to_mpd_format(track, position=None, stream_title=None): if track.musicbrainz_id is not None: result.append(('MUSICBRAINZ_TRACKID', track.musicbrainz_id)) + + # clean up result + result = [element for element in result if _has_value(*element)] + return result +def _has_value(tagtype, value): + """ + Determine whether to add the tagtype + to the output or not (if they have a default value). + + :param tagtype: the mpd tagtype + :type tagtype: string + :param value: the associated value + :type value: multiple + :rtype: bool + """ + if tagtype in tagtype_list.TAGTYPE_LIST: + if not value: + return False + + return True + + def concat_multi_values(models, attribute): """ Format Mopidy model values for output to MPD client. diff --git a/tests/mpd/protocol/test_reflection.py b/tests/mpd/protocol/test_reflection.py index 4641a8f4..0745479d 100644 --- a/tests/mpd/protocol/test_reflection.py +++ b/tests/mpd/protocol/test_reflection.py @@ -42,6 +42,23 @@ class ReflectionHandlerTest(protocol.BaseTestCase): def test_tagtypes(self): self.send_request('tagtypes') self.assertInResponse('OK') + self.assertInResponse('tagtype: Artist') + self.assertInResponse('tagtype: ArtistSort') + self.assertInResponse('tagtype: Album') + self.assertInResponse('tagtype: AlbumArtist') + self.assertInResponse('tagtype: AlbumArtistSort') + self.assertInResponse('tagtype: Title') + self.assertInResponse('tagtype: Track') + self.assertInResponse('tagtype: Name') + self.assertInResponse('tagtype: Genre') + self.assertInResponse('tagtype: Date') + self.assertInResponse('tagtype: Composer') + self.assertInResponse('tagtype: Performer') + self.assertInResponse('tagtype: Disc') + self.assertInResponse('tagtype: MUSICBRAINZ_ARTISTID') + self.assertInResponse('tagtype: MUSICBRAINZ_ALBUMID') + self.assertInResponse('tagtype: MUSICBRAINZ_ALBUMARTISTID') + self.assertInResponse('tagtype: MUSICBRAINZ_TRACKID') def test_urlhandlers(self): self.send_request('urlhandlers') diff --git a/tests/mpd/protocol/test_status.py b/tests/mpd/protocol/test_status.py index ea4137de..fb448d8d 100644 --- a/tests/mpd/protocol/test_status.py +++ b/tests/mpd/protocol/test_status.py @@ -20,10 +20,10 @@ class StatusHandlerTest(protocol.BaseTestCase): self.send_request('currentsong') self.assertInResponse('file: dummy:/a') self.assertInResponse('Time: 0') - self.assertInResponse('Artist: ') - self.assertInResponse('Title: ') - self.assertInResponse('Album: ') - self.assertInResponse('Track: 0') + self.assertNotInResponse('Artist: ') + self.assertNotInResponse('Title: ') + self.assertNotInResponse('Album: ') + self.assertNotInResponse('Track: 0') self.assertNotInResponse('Date: ') self.assertInResponse('Pos: 0') self.assertInResponse('Id: 0') diff --git a/tests/mpd/protocol/test_stored_playlists.py b/tests/mpd/protocol/test_stored_playlists.py index 90b25a70..2899a126 100644 --- a/tests/mpd/protocol/test_stored_playlists.py +++ b/tests/mpd/protocol/test_stored_playlists.py @@ -45,7 +45,7 @@ class PlaylistsHandlerTest(protocol.BaseTestCase): self.send_request('listplaylistinfo "name"') self.assertInResponse('file: dummy:a') - self.assertInResponse('Track: 0') + self.assertNotInResponse('Track: 0') self.assertNotInResponse('Pos: 0') self.assertInResponse('OK') @@ -56,7 +56,7 @@ class PlaylistsHandlerTest(protocol.BaseTestCase): self.send_request('listplaylistinfo name') self.assertInResponse('file: dummy:a') - self.assertInResponse('Track: 0') + self.assertNotInResponse('Track: 0') self.assertNotInResponse('Pos: 0') self.assertInResponse('OK') @@ -72,7 +72,7 @@ class PlaylistsHandlerTest(protocol.BaseTestCase): self.send_request('listplaylistinfo "a [2]"') self.assertInResponse('file: c') - self.assertInResponse('Track: 0') + self.assertNotInResponse('Track: 0') self.assertNotInResponse('Pos: 0') self.assertInResponse('OK') diff --git a/tests/mpd/test_translator.py b/tests/mpd/test_translator.py index 6a0220a8..57477a51 100644 --- a/tests/mpd/test_translator.py +++ b/tests/mpd/test_translator.py @@ -33,17 +33,17 @@ class TrackMpdFormatTest(unittest.TestCase): path.mtime.undo_fake() def test_track_to_mpd_format_for_empty_track(self): - # TODO: this is likely wrong, see: - # https://github.com/mopidy/mopidy/issues/923#issuecomment-79584110 - result = translator.track_to_mpd_format(Track()) - self.assertIn(('file', ''), result) - self.assertIn(('Time', 0), result) - self.assertIn(('Artist', ''), result) - self.assertIn(('Title', ''), result) - self.assertIn(('Album', ''), result) - self.assertIn(('Track', 0), result) + result = translator.track_to_mpd_format( + Track(uri='a uri', length=137000) + ) + self.assertIn(('file', 'a uri'), result) + self.assertIn(('Time', 137), result) + self.assertNotIn(('Artist', ''), result) + self.assertNotIn(('Title', ''), result) + self.assertNotIn(('Album', ''), result) + self.assertNotIn(('Track', 0), result) self.assertNotIn(('Date', ''), result) - self.assertEqual(len(result), 6) + self.assertEqual(len(result), 2) def test_track_to_mpd_format_with_position(self): result = translator.track_to_mpd_format(Track(), position=1) @@ -137,7 +137,7 @@ class TrackMpdFormatTest(unittest.TestCase): def test_track_to_mpd_format_with_empty_stream_title(self): result = translator.track_to_mpd_format(self.track, stream_title='') self.assertIn(('Name', 'a name'), result) - self.assertIn(('Title', ''), result) + self.assertNotIn(('Title', ''), result) def test_track_to_mpd_format_with_stream_and_no_track_name(self): track = self.track.replace(name=None)