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

@@ -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')

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',
+]

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.

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')

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.assertNotInResponse('Artist: ')
-        self.assertInResponse('Title: ')
+        self.assertNotInResponse('Title: ')
-        self.assertInResponse('Album: ')
+        self.assertNotInResponse('Album: ')
-        self.assertInResponse('Track: 0')
+        self.assertNotInResponse('Track: 0')
         self.assertNotInResponse('Date: ')
         self.assertInResponse('Pos: 0')
         self.assertInResponse('Id: 0')

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')
 

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:
+        result = translator.track_to_mpd_format(
-        # https://github.com/mopidy/mopidy/issues/923#issuecomment-79584110
+            Track(uri='a uri', length=137000)
-        result = translator.track_to_mpd_format(Track())
+        )
-        self.assertIn(('file', ''), result)
+        self.assertIn(('file', 'a uri'), result)
-        self.assertIn(('Time', 0), result)
+        self.assertIn(('Time', 137), result)
-        self.assertIn(('Artist', ''), result)
+        self.assertNotIn(('Artist', ''), result)
-        self.assertIn(('Title', ''), result)
+        self.assertNotIn(('Title', ''), result)
-        self.assertIn(('Album', ''), result)
+        self.assertNotIn(('Album', ''), result)
-        self.assertIn(('Track', 0), 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)