From de6718abdf9866c39b1b3ed5794a78800ea61b2e Mon Sep 17 00:00:00 2001 From: Carl Suster Date: Mon, 1 Apr 2019 12:02:15 +1100 Subject: [PATCH 01/23] bpd: separate tests by command category --- test/test_player.py | 75 ++++++++++++++++++++++++++++++--------------- 1 file changed, 51 insertions(+), 24 deletions(-) diff --git a/test/test_player.py b/test/test_player.py index 1c2b0165d8..1e917f0785 100644 --- a/test/test_player.py +++ b/test/test_player.py @@ -241,7 +241,7 @@ def _test(self): return unittest.expectedFailure(_test) if expectedFailure else _test -class BPDTest(unittest.TestCase, TestHelper): +class BPDTestHelper(unittest.TestCase, TestHelper): def setUp(self): self.setup_beets(disk=True) self.load_plugins('bpd') @@ -327,39 +327,47 @@ def _assert_failed(self, response, code, pos=None): if code is not None: self.assertEqual(code, response.err_data[0]) + def _bpd_add(self, client, *items): + """ Add the given item to the BPD playlist + """ + paths = ['/'.join([ + item.artist, item.album, + py3_path(os.path.basename(item.path))]) for item in items] + responses = client.send_commands(*[('add', path) for path in paths]) + self._assert_ok(*responses) + + +class BPDTest(BPDTestHelper): def test_server_hello(self): with self.run_bpd(do_hello=False) as client: self.assertEqual(client.readline(), b'OK MPD 0.13.0\n') + def test_unknown_cmd(self): + with self.run_bpd() as client: + response = client.send_command('notacommand') + self._assert_failed(response, bpd.ERROR_UNKNOWN) + + +class BPDQueryTest(BPDTestHelper): test_implements_query = implements({ 'clearerror', 'currentsong', 'idle', 'status', 'stats', }, expectedFailure=True) + +class BPDPlaybackTest(BPDTestHelper): test_implements_playback = implements({ 'consume', 'crossfade', 'mixrampd', 'mixrampdelay', 'random', 'repeat', 'setvol', 'single', 'replay_gain_mode', 'replay_gain_status', 'volume', }, expectedFailure=True) + +class BPDControlTest(BPDTestHelper): test_implements_control = implements({ - 'next', 'pause', 'play', 'playid', 'previous', 'seek', + 'next', 'pause', 'playid', 'previous', 'seek', 'seekid', 'seekcur', 'stop', }, expectedFailure=True) - def _bpd_add(self, client, *items): - """ Add the given item to the BPD playlist - """ - paths = ['/'.join([ - item.artist, item.album, - py3_path(os.path.basename(item.path))]) for item in items] - responses = client.send_commands(*[('add', path) for path in paths]) - self._assert_ok(*responses) - - def test_unknown_cmd(self): - with self.run_bpd() as client: - response = client.send_command('notacommand') - self._assert_failed(response, bpd.ERROR_UNKNOWN) - def test_cmd_play(self): with self.run_bpd() as client: self._bpd_add(client, self.item1) @@ -371,10 +379,12 @@ def test_cmd_play(self): self.assertEqual('stop', responses[0].data['state']) self.assertEqual('play', responses[2].data['state']) + +class BPDQueueTest(BPDTestHelper): test_implements_queue = implements({ - 'add', 'addid', 'clear', 'delete', 'deleteid', 'move', + 'addid', 'clear', 'delete', 'deleteid', 'move', 'moveid', 'playlist', 'playlistfind', 'playlistid', - 'playlistinfo', 'playlistsearch', 'plchanges', + 'playlistsearch', 'plchanges', 'plchangesposid', 'prio', 'prioid', 'rangeid', 'shuffle', 'swap', 'swapid', 'addtagid', 'cleartagid', }, expectedFailure=True) @@ -390,19 +400,22 @@ def test_cmd_playlistinfo(self): ('playlistinfo',), ('playlistinfo', '0'), ('playlistinfo', '200')) - self._assert_failed(responses, bpd.ERROR_ARG, pos=2) + +class BPDPlaylistsTest(BPDTestHelper): test_implements_playlists = implements({ 'listplaylist', 'listplaylistinfo', 'listplaylists', 'load', 'playlistadd', 'playlistclear', 'playlistdelete', 'playlistmove', 'rename', 'rm', 'save', }, expectedFailure=True) + +class BPDDatabaseTest(BPDTestHelper): test_implements_database = implements({ - 'albumart', 'count', 'find', 'findadd', 'list', 'listall', - 'listallinfo', 'listfiles', 'lsinfo', 'readcomments', - 'search', 'searchadd', 'searchaddpl', 'update', 'rescan', + 'albumart', 'find', 'findadd', 'listall', + 'listallinfo', 'listfiles', 'readcomments', + 'searchadd', 'searchaddpl', 'update', 'rescan', }, expectedFailure=True) def test_cmd_search(self): @@ -411,7 +424,7 @@ def test_cmd_search(self): self._assert_ok(response) self.assertEqual(self.item1.title, response.data['Title']) - def test_cmd_list_simple(self): + def test_cmd_list(self): with self.run_bpd() as client: responses = client.send_commands( ('list', 'album'), @@ -439,16 +452,22 @@ def test_cmd_count(self): self.assertEqual('1', response.data['songs']) self.assertEqual('0', response.data['playtime']) + +class BPDMountsTest(BPDTestHelper): test_implements_mounts = implements({ 'mount', 'unmount', 'listmounts', 'listneighbors', }, expectedFailure=True) + +class BPDStickerTest(BPDTestHelper): test_implements_stickers = implements({ 'sticker', }, expectedFailure=True) + +class BPDConnectionTest(BPDTestHelper): test_implements_connection = implements({ - 'close', 'kill', 'password', 'ping', 'tagtypes', + 'close', 'kill', 'tagtypes', }) def test_cmd_password(self): @@ -489,19 +508,27 @@ def test_tagtypes_mask(self): response = client.send_command('tagtypes', 'clear') self._assert_ok(response) + +class BPDPartitionTest(BPDTestHelper): test_implements_partitions = implements({ 'partition', 'listpartitions', 'newpartition', }, expectedFailure=True) + +class BPDDeviceTest(BPDTestHelper): test_implements_devices = implements({ 'disableoutput', 'enableoutput', 'toggleoutput', 'outputs', }, expectedFailure=True) + +class BPDReflectionTest(BPDTestHelper): test_implements_reflection = implements({ 'config', 'commands', 'notcommands', 'urlhandlers', 'decoders', }, expectedFailure=True) + +class BPDPeersTest(BPDTestHelper): test_implements_peers = implements({ 'subscribe', 'unsubscribe', 'channels', 'readmessages', 'sendmessage', From d94a5393b2929cdfc0c759728ad185b8d40f68dd Mon Sep 17 00:00:00 2001 From: Carl Suster Date: Sat, 30 Mar 2019 19:03:38 +1100 Subject: [PATCH 02/23] bpd: fix crossfade command Although crossfade is not implemented in bpd, we can store the setting and repeat is back to clients. Also log a warning that the operation is not implemented. The real MPD doesn't show the crossfade in status if it's zero since that means no crossfade, so now we don't either. --- beetsplug/bpd/__init__.py | 6 +++++- test/test_player.py | 15 ++++++++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/beetsplug/bpd/__init__.py b/beetsplug/bpd/__init__.py index a29690b03c..0d16e22ca8 100644 --- a/beetsplug/bpd/__init__.py +++ b/beetsplug/bpd/__init__.py @@ -307,9 +307,11 @@ def cmd_status(self, conn): u'random: ' + six.text_type(int(self.random)), u'playlist: ' + six.text_type(self.playlist_version), u'playlistlength: ' + six.text_type(len(self.playlist)), - u'xfade: ' + six.text_type(self.crossfade), ) + if self.crossfade > 0: + yield u'xfade: ' + six.text_type(self.crossfade) + if self.current_index == -1: state = u'stop' elif self.paused: @@ -353,6 +355,8 @@ def cmd_crossfade(self, conn, crossfade): crossfade = cast_arg(int, crossfade) if crossfade < 0: raise BPDError(ERROR_ARG, u'crossfade time must be nonnegative') + self._log.warning(u'crossfade is not implemented in bpd') + self.crossfade = crossfade def cmd_clear(self, conn): """Clear the playlist.""" diff --git a/test/test_player.py b/test/test_player.py index 1e917f0785..d7f65ff3fa 100644 --- a/test/test_player.py +++ b/test/test_player.py @@ -356,11 +356,24 @@ class BPDQueryTest(BPDTestHelper): class BPDPlaybackTest(BPDTestHelper): test_implements_playback = implements({ - 'consume', 'crossfade', 'mixrampd', 'mixrampdelay', 'random', + 'consume', 'mixrampd', 'mixrampdelay', 'random', 'repeat', 'setvol', 'single', 'replay_gain_mode', 'replay_gain_status', 'volume', }, expectedFailure=True) + def test_cmd_crossfade(self): + with self.run_bpd() as client: + responses = client.send_commands( + ('status',), + ('crossfade', '123'), + ('status',), + ('crossfade', '-2')) + response = client.send_command('crossfade', '0.5') + self._assert_failed(responses, bpd.ERROR_ARG, pos=3) + self._assert_failed(response, bpd.ERROR_ARG) + self.assertNotIn('xfade', responses[0].data) + self.assertAlmostEqual(123, int(responses[2].data['xfade'])) + class BPDControlTest(BPDTestHelper): test_implements_control = implements({ From 0f53ae9a87c059ac7d8d7f76adc95d67bed4d096 Mon Sep 17 00:00:00 2001 From: Carl Suster Date: Sat, 30 Mar 2019 19:06:01 +1100 Subject: [PATCH 03/23] bpd: error instead of crashing on extra argument If an MPC client is expecting a command to take an argument that bpd isn't expecting (e.g. because of a difference in protocol versions) then bpd currently crashes completely. Instead, do what the real MPD does and return an error message over the protocol. --- beetsplug/bpd/__init__.py | 10 +++++++++- test/test_player.py | 5 +++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/beetsplug/bpd/__init__.py b/beetsplug/bpd/__init__.py index 0d16e22ca8..4f04d01fca 100644 --- a/beetsplug/bpd/__init__.py +++ b/beetsplug/bpd/__init__.py @@ -674,7 +674,8 @@ def run(self, conn): # Attempt to get correct command function. func_name = 'cmd_' + self.name if not hasattr(conn.server, func_name): - raise BPDError(ERROR_UNKNOWN, u'unknown command', self.name) + raise BPDError(ERROR_UNKNOWN, + u'unknown command "{}"'.format(self.name)) func = getattr(conn.server, func_name) # Ensure we have permission for this command. @@ -690,6 +691,13 @@ def run(self, conn): for data in results: yield conn.send(data) + except TypeError: + # The client provided too many arguments. + raise BPDError(ERROR_ARG, + u'wrong number of arguments for "{}"' + .format(self.name), + self.name) + except BPDError as e: # An exposed error. Set the command name and then let # the Connection handle it. diff --git a/test/test_player.py b/test/test_player.py index d7f65ff3fa..d2ea496c4c 100644 --- a/test/test_player.py +++ b/test/test_player.py @@ -347,6 +347,11 @@ def test_unknown_cmd(self): response = client.send_command('notacommand') self._assert_failed(response, bpd.ERROR_UNKNOWN) + def test_unexpected_argument(self): + with self.run_bpd() as client: + response = client.send_command('clearerror', 'extra argument') + self._assert_failed(response, bpd.ERROR_ARG) + class BPDQueryTest(BPDTestHelper): test_implements_query = implements({ From 1511e313f7958a989e56dfb0bae14f4b4fe8e6f3 Mon Sep 17 00:00:00 2001 From: Carl Suster Date: Sat, 30 Mar 2019 20:18:08 +1100 Subject: [PATCH 04/23] bpd: add mixramp commands These are a more sophisticated version of crossfade so we're free to ignore them, at least for now. We now track the values of the two settings, and show them in the status output. Like MPD, we suppress the mixrampdb value if it's set to nan, which is how it signals that the feature should be turned off. --- beetsplug/bpd/__init__.py | 22 ++++++++++++++++++++++ test/test_player.py | 22 +++++++++++++++++++++- 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/beetsplug/bpd/__init__.py b/beetsplug/bpd/__init__.py index 4f04d01fca..5c394212c5 100644 --- a/beetsplug/bpd/__init__.py +++ b/beetsplug/bpd/__init__.py @@ -25,6 +25,7 @@ import traceback import random import time +import math import beets from beets.plugins import BeetsPlugin @@ -174,6 +175,8 @@ def __init__(self, host, port, password, log): self.repeat = False self.volume = VOLUME_MAX self.crossfade = 0 + self.mixrampdb = 0.0 + self.mixrampdelay = float('nan') self.playlist = [] self.playlist_version = 0 self.current_index = -1 @@ -307,8 +310,11 @@ def cmd_status(self, conn): u'random: ' + six.text_type(int(self.random)), u'playlist: ' + six.text_type(self.playlist_version), u'playlistlength: ' + six.text_type(len(self.playlist)), + u'mixrampdb: ' + six.text_type(self.mixrampdb), ) + if not math.isnan(self.mixrampdelay): + yield u'mixrampdelay: ' + six.text_type(self.mixrampdelay) if self.crossfade > 0: yield u'xfade: ' + six.text_type(self.crossfade) @@ -358,6 +364,22 @@ def cmd_crossfade(self, conn, crossfade): self._log.warning(u'crossfade is not implemented in bpd') self.crossfade = crossfade + def cmd_mixrampdb(self, conn, db): + """Set the mixramp normalised max volume in dB.""" + db = cast_arg(float, db) + if db > 0: + raise BPDError(ERROR_ARG, u'mixrampdb time must be negative') + self._log.warning('mixramp is not implemented in bpd') + self.mixrampdb = db + + def cmd_mixrampdelay(self, conn, delay): + """Set the mixramp delay in seconds.""" + delay = cast_arg(float, delay) + if delay < 0: + raise BPDError(ERROR_ARG, u'mixrampdelay time must be nonnegative') + self._log.warning('mixramp is not implemented in bpd') + self.mixrampdelay = delay + def cmd_clear(self, conn): """Clear the playlist.""" self.playlist = [] diff --git a/test/test_player.py b/test/test_player.py index d2ea496c4c..5589b4d6c6 100644 --- a/test/test_player.py +++ b/test/test_player.py @@ -361,7 +361,7 @@ class BPDQueryTest(BPDTestHelper): class BPDPlaybackTest(BPDTestHelper): test_implements_playback = implements({ - 'consume', 'mixrampd', 'mixrampdelay', 'random', + 'consume', 'random', 'repeat', 'setvol', 'single', 'replay_gain_mode', 'replay_gain_status', 'volume', }, expectedFailure=True) @@ -379,6 +379,26 @@ def test_cmd_crossfade(self): self.assertNotIn('xfade', responses[0].data) self.assertAlmostEqual(123, int(responses[2].data['xfade'])) + def test_cmd_mixrampdb(self): + with self.run_bpd() as client: + responses = client.send_commands( + ('mixrampdb', '-17'), + ('status',)) + self._assert_ok(*responses) + self.assertAlmostEqual(-17, float(responses[1].data['mixrampdb'])) + + def test_cmd_mixrampdelay(self): + with self.run_bpd() as client: + responses = client.send_commands( + ('mixrampdelay', '2'), + ('status',), + ('mixrampdelay', 'nan'), + ('status',), + ('mixrampdelay', '-2')) + self._assert_failed(responses, bpd.ERROR_ARG, pos=4) + self.assertAlmostEqual(2, float(responses[1].data['mixrampdelay'])) + self.assertNotIn('mixrampdelay', responses[3].data) + class BPDControlTest(BPDTestHelper): test_implements_control = implements({ From 67a0b38d200cee2fa4f38a084d3128a132512bc8 Mon Sep 17 00:00:00 2001 From: Carl Suster Date: Sat, 30 Mar 2019 20:25:20 +1100 Subject: [PATCH 05/23] bpd: add dummy command for volume MPD supports a deprecated command 'volume' which was used to change the volume by a relative amount unlike its replacement 'setvol' which uses an absolute amount. As far as I can tell 'volume' always responds with a system error message "No mixer". --- beetsplug/bpd/__init__.py | 4 ++++ test/test_player.py | 23 ++++++++++++++++++++--- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/beetsplug/bpd/__init__.py b/beetsplug/bpd/__init__.py index 5c394212c5..4fb4cb25b9 100644 --- a/beetsplug/bpd/__init__.py +++ b/beetsplug/bpd/__init__.py @@ -356,6 +356,10 @@ def cmd_setvol(self, conn, vol): raise BPDError(ERROR_ARG, u'volume out of range') self.volume = vol + def cmd_volume(self, conn, vol_delta): + """Deprecated command to change the volume by a relative amount.""" + raise BPDError(ERROR_SYSTEM, u'No mixer') + def cmd_crossfade(self, conn, crossfade): """Set the number of seconds of crossfading.""" crossfade = cast_arg(int, crossfade) diff --git a/test/test_player.py b/test/test_player.py index 5589b4d6c6..db70a17bea 100644 --- a/test/test_player.py +++ b/test/test_player.py @@ -46,7 +46,7 @@ def _gstplayer_play(_): # noqa: 42 "seek" ], **{ 'playing': False, - 'volume': 0.0, + 'volume': 0, 'time.return_value': (0, 0), 'play_file.side_effect': _gstplayer_play, }) @@ -362,8 +362,8 @@ class BPDQueryTest(BPDTestHelper): class BPDPlaybackTest(BPDTestHelper): test_implements_playback = implements({ 'consume', 'random', - 'repeat', 'setvol', 'single', 'replay_gain_mode', - 'replay_gain_status', 'volume', + 'repeat', 'single', 'replay_gain_mode', + 'replay_gain_status', }, expectedFailure=True) def test_cmd_crossfade(self): @@ -399,6 +399,23 @@ def test_cmd_mixrampdelay(self): self.assertAlmostEqual(2, float(responses[1].data['mixrampdelay'])) self.assertNotIn('mixrampdelay', responses[3].data) + def test_cmd_setvol(self): + with self.run_bpd() as client: + responses = client.send_commands( + ('setvol', '67'), + ('status',), + ('setvol', '32'), + ('status',), + ('setvol', '101')) + self._assert_failed(responses, bpd.ERROR_ARG, pos=4) + self.assertEqual('67', responses[1].data['volume']) + self.assertEqual('32', responses[3].data['volume']) + + def test_cmd_volume(self): + with self.run_bpd() as client: + response = client.send_command('volume', '10') + self._assert_failed(response, bpd.ERROR_SYSTEM) + class BPDControlTest(BPDTestHelper): test_implements_control = implements({ From e5851866d74e4ac945f63511b2ff48f16233550e Mon Sep 17 00:00:00 2001 From: Carl Suster Date: Sat, 30 Mar 2019 21:30:34 +1100 Subject: [PATCH 06/23] bpd: add replay_gain_* commands There's a special status command for checking the replay gain mode, which can be set to one of a short list of possible values. For now at least we can ignore this feature, but track the setting anyway. --- beetsplug/bpd/__init__.py | 12 ++++++++++++ test/test_player.py | 12 ++++++++++-- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/beetsplug/bpd/__init__.py b/beetsplug/bpd/__init__.py index 4fb4cb25b9..d5e4489f40 100644 --- a/beetsplug/bpd/__init__.py +++ b/beetsplug/bpd/__init__.py @@ -177,6 +177,7 @@ def __init__(self, host, port, password, log): self.crossfade = 0 self.mixrampdb = 0.0 self.mixrampdelay = float('nan') + self.replay_gain_mode = 'off' self.playlist = [] self.playlist_version = 0 self.current_index = -1 @@ -384,6 +385,17 @@ def cmd_mixrampdelay(self, conn, delay): self._log.warning('mixramp is not implemented in bpd') self.mixrampdelay = delay + def cmd_replay_gain_mode(self, conn, mode): + """Set the replay gain mode.""" + if mode not in ['off', 'track', 'album', 'auto']: + raise BPDError(ERROR_ARG, u'Unrecognised replay gain mode') + self._log.warning('replay gain is not implemented in bpd') + self.replay_gain_mode = mode + + def cmd_replay_gain_status(self, conn): + """Get the replaygain mode.""" + yield u'replay_gain_mode: ' + six.text_type(self.replay_gain_mode) + def cmd_clear(self, conn): """Clear the playlist.""" self.playlist = [] diff --git a/test/test_player.py b/test/test_player.py index db70a17bea..44572c3a69 100644 --- a/test/test_player.py +++ b/test/test_player.py @@ -362,8 +362,7 @@ class BPDQueryTest(BPDTestHelper): class BPDPlaybackTest(BPDTestHelper): test_implements_playback = implements({ 'consume', 'random', - 'repeat', 'single', 'replay_gain_mode', - 'replay_gain_status', + 'repeat', 'single', }, expectedFailure=True) def test_cmd_crossfade(self): @@ -416,6 +415,15 @@ def test_cmd_volume(self): response = client.send_command('volume', '10') self._assert_failed(response, bpd.ERROR_SYSTEM) + def test_cmd_replay_gain(self): + with self.run_bpd() as client: + responses = client.send_commands( + ('replay_gain_mode', 'track'), + ('replay_gain_status',), + ('replay_gain_mode', 'notanoption')) + self._assert_failed(responses, bpd.ERROR_ARG, pos=2) + self.assertAlmostEqual('track', responses[1].data['replay_gain_mode']) + class BPDControlTest(BPDTestHelper): test_implements_control = implements({ From 859e16d1e310b6cc8bdafb3f8fd591df8f36b908 Mon Sep 17 00:00:00 2001 From: Carl Suster Date: Sun, 31 Mar 2019 01:01:45 +1100 Subject: [PATCH 07/23] bpd: support consume command --- beetsplug/bpd/__init__.py | 11 +++++++++++ test/test_player.py | 23 ++++++++++++++++++++++- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/beetsplug/bpd/__init__.py b/beetsplug/bpd/__init__.py index d5e4489f40..a0d6d62548 100644 --- a/beetsplug/bpd/__init__.py +++ b/beetsplug/bpd/__init__.py @@ -173,6 +173,7 @@ def __init__(self, host, port, password, log): # Default server values. self.random = False self.repeat = False + self.consume = False self.volume = VOLUME_MAX self.crossfade = 0 self.mixrampdb = 0.0 @@ -309,6 +310,7 @@ def cmd_status(self, conn): u'volume: ' + six.text_type(self.volume), u'repeat: ' + six.text_type(int(self.repeat)), u'random: ' + six.text_type(int(self.random)), + u'consume: ' + six.text_type(int(self.consume)), u'playlist: ' + six.text_type(self.playlist_version), u'playlistlength: ' + six.text_type(len(self.playlist)), u'mixrampdb: ' + six.text_type(self.mixrampdb), @@ -350,6 +352,10 @@ def cmd_repeat(self, conn, state): """Set or unset repeat mode.""" self.repeat = cast_arg('intbool', state) + def cmd_consume(self, conn, state): + """Set or unset consume mode.""" + self.consume = cast_arg('intbool', state) + def cmd_setvol(self, conn, vol): """Set the player's volume level (0-100).""" vol = cast_arg(int, vol) @@ -519,7 +525,12 @@ def cmd_currentsong(self, conn): def cmd_next(self, conn): """Advance to the next song in the playlist.""" + old_index = self.current_index self.current_index = self._succ_idx() + if self.consume: + self.playlist.pop(old_index) + if self.current_index > old_index: + self.current_index -= 1 if self.current_index >= len(self.playlist): # Fallen off the end. Just move to stopped state. return self.cmd_stop(conn) diff --git a/test/test_player.py b/test/test_player.py index 44572c3a69..b9544769c9 100644 --- a/test/test_player.py +++ b/test/test_player.py @@ -361,10 +361,31 @@ class BPDQueryTest(BPDTestHelper): class BPDPlaybackTest(BPDTestHelper): test_implements_playback = implements({ - 'consume', 'random', + 'random', 'repeat', 'single', }, expectedFailure=True) + def test_cmd_consume(self): + with self.run_bpd() as client: + self._bpd_add(client, self.item1, self.item2) + responses = client.send_commands( + ('consume', '0'), + ('playlistinfo',), + ('next',), + ('playlistinfo',), + ('consume', '1'), + ('playlistinfo',), + ('play', '0'), + ('next',), + ('playlistinfo',), + ('status',)) + self._assert_ok(*responses) + self.assertEqual(responses[1].data['Id'], responses[3].data['Id']) + self.assertEqual(['1', '2'], responses[5].data['Id']) + self.assertEqual('2', responses[8].data['Id']) + self.assertEqual('1', responses[9].data['consume']) + self.assertEqual('play', responses[9].data['state']) + def test_cmd_crossfade(self): with self.run_bpd() as client: responses = client.send_commands( From 71e7621642fa996e6c4b0eff8e5ad1afa58a3ca8 Mon Sep 17 00:00:00 2001 From: Carl Suster Date: Mon, 1 Apr 2019 11:28:31 +1100 Subject: [PATCH 08/23] bpd: no-op support for persistent playlists The real MPD offers persistent playlist manipulation, storing the playlists in a directory set in the config file. If that directory is not available then the feature is disabled and the relevant commands all respond with errors. Based on this, the initial support in bpd just returns errors matching the MPD server in the disabled mode. For playlistadd, extend the _bpd_add helper to work with playlists other than the queue in order to support testing the real implementations of these commands in the future. --- beetsplug/bpd/__init__.py | 36 +++++++++++++++++++ test/test_player.py | 74 ++++++++++++++++++++++++++++++++++----- 2 files changed, 102 insertions(+), 8 deletions(-) diff --git a/beetsplug/bpd/__init__.py b/beetsplug/bpd/__init__.py index a0d6d62548..c29b0604d2 100644 --- a/beetsplug/bpd/__init__.py +++ b/beetsplug/bpd/__init__.py @@ -1136,6 +1136,42 @@ def cmd_count(self, conn, tag, value): yield u'songs: ' + six.text_type(songs) yield u'playtime: ' + six.text_type(int(playtime)) + # Persistent playlist manipulation. In MPD this is an optional feature so + # these dummy implementations match MPD's behaviour with the feature off. + + def cmd_listplaylist(self, conn, playlist): + raise BPDError(ERROR_NO_EXIST, u'No such playlist') + + def cmd_listplaylistinfo(self, conn, playlist): + raise BPDError(ERROR_NO_EXIST, u'No such playlist') + + def cmd_listplaylists(self, conn): + raise BPDError(ERROR_UNKNOWN, u'Stored playlists are disabled') + + def cmd_load(self, conn, playlist): + raise BPDError(ERROR_NO_EXIST, u'Stored playlists are disabled') + + def cmd_playlistadd(self, conn, playlist, uri): + raise BPDError(ERROR_UNKNOWN, u'Stored playlists are disabled') + + def cmd_playlistclear(self, conn, playlist): + raise BPDError(ERROR_UNKNOWN, u'Stored playlists are disabled') + + def cmd_playlistdelete(self, conn, playlist, index): + raise BPDError(ERROR_UNKNOWN, u'Stored playlists are disabled') + + def cmd_playlistmove(self, conn, playlist, from_index, to_index): + raise BPDError(ERROR_UNKNOWN, u'Stored playlists are disabled') + + def cmd_rename(self, conn, playlist, new_name): + raise BPDError(ERROR_UNKNOWN, u'Stored playlists are disabled') + + def cmd_rm(self, conn, playlist): + raise BPDError(ERROR_UNKNOWN, u'Stored playlists are disabled') + + def cmd_save(self, conn, playlist): + raise BPDError(ERROR_UNKNOWN, u'Stored playlists are disabled') + # "Outputs." Just a dummy implementation because we don't control # any outputs. diff --git a/test/test_player.py b/test/test_player.py index b9544769c9..80ba595cd6 100644 --- a/test/test_player.py +++ b/test/test_player.py @@ -327,13 +327,18 @@ def _assert_failed(self, response, code, pos=None): if code is not None: self.assertEqual(code, response.err_data[0]) - def _bpd_add(self, client, *items): - """ Add the given item to the BPD playlist + def _bpd_add(self, client, *items, **kwargs): + """ Add the given item to the BPD playlist or queue. """ paths = ['/'.join([ item.artist, item.album, py3_path(os.path.basename(item.path))]) for item in items] - responses = client.send_commands(*[('add', path) for path in paths]) + playlist = kwargs.get('playlist') + if playlist: + commands = [('playlistadd', playlist, path) for path in paths] + else: + commands = [('add', path) for path in paths] + responses = client.send_commands(*commands) self._assert_ok(*responses) @@ -488,11 +493,64 @@ def test_cmd_playlistinfo(self): class BPDPlaylistsTest(BPDTestHelper): - test_implements_playlists = implements({ - 'listplaylist', 'listplaylistinfo', 'listplaylists', 'load', - 'playlistadd', 'playlistclear', 'playlistdelete', - 'playlistmove', 'rename', 'rm', 'save', - }, expectedFailure=True) + test_implements_playlists = implements({'playlistadd'}) + + def test_cmd_listplaylist(self): + with self.run_bpd() as client: + response = client.send_command('listplaylist', 'anything') + self._assert_failed(response, bpd.ERROR_NO_EXIST) + + def test_cmd_listplaylistinfo(self): + with self.run_bpd() as client: + response = client.send_command('listplaylistinfo', 'anything') + self._assert_failed(response, bpd.ERROR_NO_EXIST) + + def test_cmd_listplaylists(self): + with self.run_bpd() as client: + response = client.send_command('listplaylists') + self._assert_failed(response, bpd.ERROR_UNKNOWN) + + def test_cmd_load(self): + with self.run_bpd() as client: + response = client.send_command('load', 'anything') + self._assert_failed(response, bpd.ERROR_NO_EXIST) + + @unittest.skip + def test_cmd_playlistadd(self): + with self.run_bpd() as client: + self._bpd_add(client, self.item1, playlist='anything') + + def test_cmd_playlistclear(self): + with self.run_bpd() as client: + response = client.send_command('playlistclear', 'anything') + self._assert_failed(response, bpd.ERROR_UNKNOWN) + + def test_cmd_playlistdelete(self): + with self.run_bpd() as client: + response = client.send_command('playlistdelete', 'anything', '0') + self._assert_failed(response, bpd.ERROR_UNKNOWN) + + def test_cmd_playlistmove(self): + with self.run_bpd() as client: + response = client.send_command( + 'playlistmove', 'anything', '0', '1') + self._assert_failed(response, bpd.ERROR_UNKNOWN) + + def test_cmd_rename(self): + with self.run_bpd() as client: + response = client.send_command('rename', 'anything', 'newname') + self._assert_failed(response, bpd.ERROR_UNKNOWN) + + def test_cmd_rm(self): + with self.run_bpd() as client: + response = client.send_command('rm', 'anything') + self._assert_failed(response, bpd.ERROR_UNKNOWN) + + def test_cmd_save(self): + with self.run_bpd() as client: + self._bpd_add(client, self.item1) + response = client.send_command('save', 'newplaylist') + self._assert_failed(response, bpd.ERROR_UNKNOWN) class BPDDatabaseTest(BPDTestHelper): From bae9c40600018f1797c50b1d83cd3bfb886d77fc Mon Sep 17 00:00:00 2001 From: Carl Suster Date: Mon, 1 Apr 2019 16:26:00 +1100 Subject: [PATCH 09/23] bpd: support the single command This command instructs bpd to stop playing when the current song finishes. In the MPD 0.20 protocol this flag gains a value 'oneshot' but for now we just support its older version with a boolean value. --- beetsplug/bpd/__init__.py | 9 +++++++++ test/test_player.py | 20 ++++++++++++++++++-- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/beetsplug/bpd/__init__.py b/beetsplug/bpd/__init__.py index c29b0604d2..afb7b6c5e2 100644 --- a/beetsplug/bpd/__init__.py +++ b/beetsplug/bpd/__init__.py @@ -174,6 +174,7 @@ def __init__(self, host, port, password, log): self.random = False self.repeat = False self.consume = False + self.single = False self.volume = VOLUME_MAX self.crossfade = 0 self.mixrampdb = 0.0 @@ -311,6 +312,7 @@ def cmd_status(self, conn): u'repeat: ' + six.text_type(int(self.repeat)), u'random: ' + six.text_type(int(self.random)), u'consume: ' + six.text_type(int(self.consume)), + u'single: ' + six.text_type(int(self.single)), u'playlist: ' + six.text_type(self.playlist_version), u'playlistlength: ' + six.text_type(len(self.playlist)), u'mixrampdb: ' + six.text_type(self.mixrampdb), @@ -356,6 +358,11 @@ def cmd_consume(self, conn, state): """Set or unset consume mode.""" self.consume = cast_arg('intbool', state) + def cmd_single(self, conn, state): + """Set or unset single mode.""" + # TODO support oneshot in addition to 0 and 1 [MPD 0.20] + self.single = cast_arg('intbool', state) + def cmd_setvol(self, conn, vol): """Set the player's volume level (0-100).""" vol = cast_arg(int, vol) @@ -534,6 +541,8 @@ def cmd_next(self, conn): if self.current_index >= len(self.playlist): # Fallen off the end. Just move to stopped state. return self.cmd_stop(conn) + elif self.single: + return self.cmd_stop(conn) else: return self.cmd_play(conn) diff --git a/test/test_player.py b/test/test_player.py index 80ba595cd6..4ecf67c3df 100644 --- a/test/test_player.py +++ b/test/test_player.py @@ -367,8 +367,8 @@ class BPDQueryTest(BPDTestHelper): class BPDPlaybackTest(BPDTestHelper): test_implements_playback = implements({ 'random', - 'repeat', 'single', - }, expectedFailure=True) + 'repeat', + }) def test_cmd_consume(self): with self.run_bpd() as client: @@ -391,6 +391,22 @@ def test_cmd_consume(self): self.assertEqual('1', responses[9].data['consume']) self.assertEqual('play', responses[9].data['state']) + def test_cmd_single(self): + with self.run_bpd() as client: + self._bpd_add(client, self.item1, self.item2) + responses = client.send_commands( + ('status',), + ('single', '1'), + ('play',), + ('status',), + ('next',), + ('status',)) + self._assert_ok(*responses) + self.assertEqual('0', responses[0].data['single']) + self.assertEqual('1', responses[3].data['single']) + self.assertEqual('play', responses[3].data['state']) + self.assertEqual('stop', responses[5].data['state']) + def test_cmd_crossfade(self): with self.run_bpd() as client: responses = client.send_commands( From b245c0e755a5ab87b527bf09d59180c7c2d8c65d Mon Sep 17 00:00:00 2001 From: Carl Suster Date: Mon, 1 Apr 2019 16:30:45 +1100 Subject: [PATCH 10/23] bpd: test fields returned by status command --- beetsplug/bpd/__init__.py | 14 +++++++++++--- test/test_player.py | 21 ++++++++++++++++++++- 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/beetsplug/bpd/__init__.py b/beetsplug/bpd/__init__.py index afb7b6c5e2..425dbab3d5 100644 --- a/beetsplug/bpd/__init__.py +++ b/beetsplug/bpd/__init__.py @@ -1009,11 +1009,19 @@ def cmd_status(self, conn): if self.current_index > -1: item = self.playlist[self.current_index] - yield u'bitrate: ' + six.text_type(item.bitrate / 1000) - # Missing 'audio'. + yield ( + u'bitrate: ' + six.text_type(item.bitrate / 1000), + # TODO provide a real value samplerate:bits:channels 44100:24:2 + u'audio: 0:0:0', + ) (pos, total) = self.player.time() - yield u'time: ' + six.text_type(pos) + u':' + six.text_type(total) + yield ( + u'time: ' + six.text_type(pos) + u':' + six.text_type(total), + # TODO provide elapsed and duration with higher precision + u'elapsed: ' + six.text_type(float(pos)), + u'duration: ' + six.text_type(float(total)), + ) # Also missing 'updating_db'. diff --git a/test/test_player.py b/test/test_player.py index 4ecf67c3df..c11cbaf233 100644 --- a/test/test_player.py +++ b/test/test_player.py @@ -360,9 +360,28 @@ def test_unexpected_argument(self): class BPDQueryTest(BPDTestHelper): test_implements_query = implements({ - 'clearerror', 'currentsong', 'idle', 'status', 'stats', + 'clearerror', 'currentsong', 'idle', 'stats', }, expectedFailure=True) + def test_cmd_status(self): + with self.run_bpd() as client: + self._bpd_add(client, self.item1, self.item2) + responses = client.send_commands( + ('status',), + ('play',), + ('status',)) + self._assert_ok(*responses) + fields_not_playing = { + 'repeat', 'random', 'single', 'consume', 'playlist', + 'playlistlength', 'mixrampdb', 'state', + 'volume' # not (always?) returned by MPD + } + self.assertEqual(fields_not_playing, set(responses[0].data.keys())) + fields_playing = fields_not_playing | { + 'song', 'songid', 'time', 'elapsed', 'bitrate', 'duration', 'audio' + } + self.assertEqual(fields_playing, set(responses[2].data.keys())) + class BPDPlaybackTest(BPDTestHelper): test_implements_playback = implements({ From 0c3a63ef9f527b3c2289f34e85cf44c5f97a0129 Mon Sep 17 00:00:00 2001 From: Carl Suster Date: Mon, 1 Apr 2019 17:39:35 +1100 Subject: [PATCH 11/23] bpd: fix repeat mode behaviour The repeat flag indicates that the entire playlist should be repeated. If both the repeat and single flags are set then this triggers the old behaviour of looping over a single track. --- beetsplug/bpd/__init__.py | 12 ++++++---- test/test_player.py | 50 +++++++++++++++++++++++++++++++++++++-- 2 files changed, 56 insertions(+), 6 deletions(-) diff --git a/beetsplug/bpd/__init__.py b/beetsplug/bpd/__init__.py index 425dbab3d5..c43d24fd58 100644 --- a/beetsplug/bpd/__init__.py +++ b/beetsplug/bpd/__init__.py @@ -233,10 +233,10 @@ def _random_idx(self): def _succ_idx(self): """Returns the index for the next song to play. - It also considers random and repeat flags. + It also considers random, single and repeat flags. No boundaries are checked. """ - if self.repeat: + if self.repeat and self.single: return self.current_index if self.random: return self._random_idx() @@ -535,13 +535,17 @@ def cmd_next(self, conn): old_index = self.current_index self.current_index = self._succ_idx() if self.consume: + # TODO how does consume interact with single+repeat? self.playlist.pop(old_index) if self.current_index > old_index: self.current_index -= 1 if self.current_index >= len(self.playlist): - # Fallen off the end. Just move to stopped state. + # Fallen off the end. Move to stopped state or loop. + if self.repeat: + self.current_index = -1 + return self.cmd_play(conn) return self.cmd_stop(conn) - elif self.single: + elif self.single and not self.repeat: return self.cmd_stop(conn) else: return self.cmd_play(conn) diff --git a/test/test_player.py b/test/test_player.py index c11cbaf233..7707a825c5 100644 --- a/test/test_player.py +++ b/test/test_player.py @@ -386,7 +386,6 @@ def test_cmd_status(self): class BPDPlaybackTest(BPDTestHelper): test_implements_playback = implements({ 'random', - 'repeat', }) def test_cmd_consume(self): @@ -426,6 +425,38 @@ def test_cmd_single(self): self.assertEqual('play', responses[3].data['state']) self.assertEqual('stop', responses[5].data['state']) + def test_cmd_repeat(self): + with self.run_bpd() as client: + self._bpd_add(client, self.item1, self.item2) + responses = client.send_commands( + ('repeat', '1'), + ('play',), + ('currentsong',), + ('next',), + ('currentsong',), + ('next',), + ('currentsong',)) + self._assert_ok(*responses) + self.assertEqual('1', responses[2].data['Id']) + self.assertEqual('2', responses[4].data['Id']) + self.assertEqual('1', responses[6].data['Id']) + + def test_cmd_repeat_with_single(self): + with self.run_bpd() as client: + self._bpd_add(client, self.item1, self.item2) + responses = client.send_commands( + ('repeat', '1'), + ('single', '1'), + ('play',), + ('currentsong',), + ('next',), + ('status',), + ('currentsong',)) + self._assert_ok(*responses) + self.assertEqual('1', responses[3].data['Id']) + self.assertEqual('play', responses[5].data['state']) + self.assertEqual('1', responses[6].data['Id']) + def test_cmd_crossfade(self): with self.run_bpd() as client: responses = client.send_commands( @@ -488,7 +519,7 @@ def test_cmd_replay_gain(self): class BPDControlTest(BPDTestHelper): test_implements_control = implements({ - 'next', 'pause', 'playid', 'previous', 'seek', + 'pause', 'playid', 'previous', 'seek', 'seekid', 'seekcur', 'stop', }, expectedFailure=True) @@ -503,6 +534,21 @@ def test_cmd_play(self): self.assertEqual('stop', responses[0].data['state']) self.assertEqual('play', responses[2].data['state']) + def test_cmd_next(self): + with self.run_bpd() as client: + self._bpd_add(client, self.item1, self.item2) + responses = client.send_commands( + ('play',), + ('currentsong',), + ('next',), + ('currentsong',), + ('next',), + ('status',)) + self._assert_ok(*responses) + self.assertEqual('1', responses[1].data['Id']) + self.assertEqual('2', responses[3].data['Id']) + self.assertEqual('stop', responses[5].data['state']) + class BPDQueueTest(BPDTestHelper): test_implements_queue = implements({ From a4fe6875a1dfc60cbf6c60eb26b697a8c36aeff4 Mon Sep 17 00:00:00 2001 From: Carl Suster Date: Mon, 1 Apr 2019 17:41:25 +1100 Subject: [PATCH 12/23] bpd: fix bug in bounds check of current song index The songs are indexed starting from zero for the play command, however the bound check was off by one. An index matching the length of the playlist would crash the server instead of responding with an error message over the protocol. --- beetsplug/bpd/__init__.py | 2 +- test/test_player.py | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/beetsplug/bpd/__init__.py b/beetsplug/bpd/__init__.py index c43d24fd58..9b21fae9df 100644 --- a/beetsplug/bpd/__init__.py +++ b/beetsplug/bpd/__init__.py @@ -569,7 +569,7 @@ def cmd_play(self, conn, index=-1): """Begin playback, possibly at a specified playlist index.""" index = cast_arg(int, index) - if index < -1 or index > len(self.playlist): + if index < -1 or index >= len(self.playlist): raise ArgumentIndexError() if index == -1: # No index specified: start where we are. diff --git a/test/test_player.py b/test/test_player.py index 7707a825c5..5e196debd8 100644 --- a/test/test_player.py +++ b/test/test_player.py @@ -525,14 +525,17 @@ class BPDControlTest(BPDTestHelper): def test_cmd_play(self): with self.run_bpd() as client: - self._bpd_add(client, self.item1) + self._bpd_add(client, self.item1, self.item2) responses = client.send_commands( ('status',), ('play',), - ('status',)) + ('status',), + ('play', '1'), + ('currentsong',)) self._assert_ok(*responses) self.assertEqual('stop', responses[0].data['state']) self.assertEqual('play', responses[2].data['state']) + self.assertEqual('2', responses[4].data['Id']) def test_cmd_next(self): with self.run_bpd() as client: From 12e49b3c8807febb18da61b87d181ed2933b0768 Mon Sep 17 00:00:00 2001 From: Carl Suster Date: Mon, 1 Apr 2019 17:51:21 +1100 Subject: [PATCH 13/23] bpd: skipping backwards through zero keeps playing Previously issuing the 'previous' command when at position 0 on the playlist would cause bpd to stop playing. MPD instead just restarts the currently playing song instead, so we now match this behaviour. --- beetsplug/bpd/__init__.py | 5 ++--- test/test_player.py | 19 ++++++++++++++++++- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/beetsplug/bpd/__init__.py b/beetsplug/bpd/__init__.py index 9b21fae9df..ad2a3f921a 100644 --- a/beetsplug/bpd/__init__.py +++ b/beetsplug/bpd/__init__.py @@ -554,9 +554,8 @@ def cmd_previous(self, conn): """Step back to the last song.""" self.current_index = self._prev_idx() if self.current_index < 0: - return self.cmd_stop(conn) - else: - return self.cmd_play(conn) + self.current_index = 0 + return self.cmd_play(conn) def cmd_pause(self, conn, state=None): """Set the pause state playback.""" diff --git a/test/test_player.py b/test/test_player.py index 5e196debd8..b4c5c1cd05 100644 --- a/test/test_player.py +++ b/test/test_player.py @@ -519,7 +519,7 @@ def test_cmd_replay_gain(self): class BPDControlTest(BPDTestHelper): test_implements_control = implements({ - 'pause', 'playid', 'previous', 'seek', + 'pause', 'playid', 'seek', 'seekid', 'seekcur', 'stop', }, expectedFailure=True) @@ -552,6 +552,23 @@ def test_cmd_next(self): self.assertEqual('2', responses[3].data['Id']) self.assertEqual('stop', responses[5].data['state']) + def test_cmd_previous(self): + with self.run_bpd() as client: + self._bpd_add(client, self.item1, self.item2) + responses = client.send_commands( + ('play', '1'), + ('currentsong',), + ('previous',), + ('currentsong',), + ('previous',), + ('status',), + ('currentsong',)) + self._assert_ok(*responses) + self.assertEqual('2', responses[1].data['Id']) + self.assertEqual('1', responses[3].data['Id']) + self.assertEqual('play', responses[5].data['state']) + self.assertEqual('1', responses[6].data['Id']) + class BPDQueueTest(BPDTestHelper): test_implements_queue = implements({ From 146c5f5e13f4d1c5fd3bea9642d10613256a0837 Mon Sep 17 00:00:00 2001 From: Carl Suster Date: Mon, 1 Apr 2019 18:05:10 +1100 Subject: [PATCH 14/23] bpd: fix repeat, consume and single in reverse These flags are all relevant to the 'previous' command as well as the 'next' command. --- beetsplug/bpd/__init__.py | 10 +++++++-- test/test_player.py | 44 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/beetsplug/bpd/__init__.py b/beetsplug/bpd/__init__.py index ad2a3f921a..930710e8bf 100644 --- a/beetsplug/bpd/__init__.py +++ b/beetsplug/bpd/__init__.py @@ -247,7 +247,7 @@ def _prev_idx(self): It also considers random and repeat flags. No boundaries are checked. """ - if self.repeat: + if self.repeat and self.single: return self.current_index if self.random: return self._random_idx() @@ -552,9 +552,15 @@ def cmd_next(self, conn): def cmd_previous(self, conn): """Step back to the last song.""" + old_index = self.current_index self.current_index = self._prev_idx() + if self.consume: + self.playlist.pop(old_index) if self.current_index < 0: - self.current_index = 0 + if self.repeat: + self.current_index = len(self.playlist) - 1 + else: + self.current_index = 0 return self.cmd_play(conn) def cmd_pause(self, conn, state=None): diff --git a/test/test_player.py b/test/test_player.py index b4c5c1cd05..ecf08c7b38 100644 --- a/test/test_player.py +++ b/test/test_player.py @@ -409,6 +409,21 @@ def test_cmd_consume(self): self.assertEqual('1', responses[9].data['consume']) self.assertEqual('play', responses[9].data['state']) + def test_cmd_consume_in_reverse(self): + with self.run_bpd() as client: + self._bpd_add(client, self.item1, self.item2) + responses = client.send_commands( + ('consume', '1'), + ('play', '1'), + ('playlistinfo',), + ('previous',), + ('playlistinfo',), + ('status',)) + self._assert_ok(*responses) + self.assertEqual(['1', '2'], responses[2].data['Id']) + self.assertEqual('1', responses[4].data['Id']) + self.assertEqual('play', responses[5].data['state']) + def test_cmd_single(self): with self.run_bpd() as client: self._bpd_add(client, self.item1, self.item2) @@ -457,6 +472,35 @@ def test_cmd_repeat_with_single(self): self.assertEqual('play', responses[5].data['state']) self.assertEqual('1', responses[6].data['Id']) + def test_cmd_repeat_in_reverse(self): + with self.run_bpd() as client: + self._bpd_add(client, self.item1, self.item2) + responses = client.send_commands( + ('repeat', '1'), + ('play',), + ('currentsong',), + ('previous',), + ('currentsong',)) + self._assert_ok(*responses) + self.assertEqual('1', responses[2].data['Id']) + self.assertEqual('2', responses[4].data['Id']) + + def test_cmd_repeat_with_single_in_reverse(self): + with self.run_bpd() as client: + self._bpd_add(client, self.item1, self.item2) + responses = client.send_commands( + ('repeat', '1'), + ('single', '1'), + ('play',), + ('currentsong',), + ('previous',), + ('status',), + ('currentsong',)) + self._assert_ok(*responses) + self.assertEqual('1', responses[3].data['Id']) + self.assertEqual('play', responses[5].data['state']) + self.assertEqual('1', responses[6].data['Id']) + def test_cmd_crossfade(self): with self.run_bpd() as client: responses = client.send_commands( From e839e4ea191a0eef15661d1a164bee6302d8f800 Mon Sep 17 00:00:00 2001 From: Carl Suster Date: Tue, 2 Apr 2019 09:39:07 +1100 Subject: [PATCH 15/23] bpd: improve exception handling Check function signature instead of using TypeError to crudely guess that the wrong number of arguments were provided. Prevent bpd from crashing when trying to log a traceback. The `traceback.format_exc` function takes an optional argument which is supposed to be an integer restricting the length of the backtrace to show. Instead we were passing the exception object to this function and causing a new exception to be raised. --- beetsplug/bpd/__init__.py | 29 +++++++++++++++++++---------- test/test_player.py | 12 +++++++++++- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/beetsplug/bpd/__init__.py b/beetsplug/bpd/__init__.py index 930710e8bf..52619e7e14 100644 --- a/beetsplug/bpd/__init__.py +++ b/beetsplug/bpd/__init__.py @@ -31,7 +31,7 @@ from beets.plugins import BeetsPlugin import beets.ui from beets import vfs -from beets.util import bluelet +from beets.util import bluelet, inspect from beets.library import Item from beets import dbcore from beets.mediafile import MediaFile @@ -613,12 +613,17 @@ def cmd_seekid(self, conn, track_id, pos): index = self._id_to_index(track_id) return self.cmd_seek(conn, index, pos) + # Debugging/testing commands that are not part of the MPD protocol. + def cmd_profile(self, conn): """Memory profiling for debugging.""" from guppy import hpy heap = hpy().heap() print(heap) + def cmd_crash_TypeError(self, conn): + 'a' + 2 + class Connection(object): """A connection between a client and the server. Handles input and @@ -744,6 +749,17 @@ def run(self, conn): raise BPDError(ERROR_UNKNOWN, u'unknown command "{}"'.format(self.name)) func = getattr(conn.server, func_name) + argspec = inspect.getargspec(func) + max_args = len(argspec.args) - 1 + min_args = max_args + if argspec.defaults: + min_args -= len(argspec.defaults) + wrong_num = (len(self.args) > max_args) or (len(self.args) < min_args) + if wrong_num and not argspec.varargs: + raise BPDError(ERROR_ARG, + u'wrong number of arguments for "{}"' + .format(self.name), + self.name) # Ensure we have permission for this command. if conn.server.password and \ @@ -758,13 +774,6 @@ def run(self, conn): for data in results: yield conn.send(data) - except TypeError: - # The client provided too many arguments. - raise BPDError(ERROR_ARG, - u'wrong number of arguments for "{}"' - .format(self.name), - self.name) - except BPDError as e: # An exposed error. Set the command name and then let # the Connection handle it. @@ -776,9 +785,9 @@ def run(self, conn): # it on the Connection. raise - except Exception as e: + except Exception: # An "unintentional" error. Hide it from the client. - conn.server._log.error('{}', traceback.format_exc(e)) + conn.server._log.error('{}', traceback.format_exc()) raise BPDError(ERROR_SYSTEM, u'server error', self.name) diff --git a/test/test_player.py b/test/test_player.py index ecf08c7b38..98fd13f636 100644 --- a/test/test_player.py +++ b/test/test_player.py @@ -354,9 +354,19 @@ def test_unknown_cmd(self): def test_unexpected_argument(self): with self.run_bpd() as client: - response = client.send_command('clearerror', 'extra argument') + response = client.send_command('ping', 'extra argument') self._assert_failed(response, bpd.ERROR_ARG) + def test_missing_argument(self): + with self.run_bpd() as client: + response = client.send_command('add') + self._assert_failed(response, bpd.ERROR_ARG) + + def test_system_error(self): + with self.run_bpd() as client: + response = client.send_command('crash_TypeError') + self._assert_failed(response, bpd.ERROR_SYSTEM) + class BPDQueryTest(BPDTestHelper): test_implements_query = implements({ From 9622e7433beba9336ec3a48e7c03b85f690febf6 Mon Sep 17 00:00:00 2001 From: Carl Suster Date: Tue, 2 Apr 2019 09:44:34 +1100 Subject: [PATCH 16/23] bpd: return real audio data --- beetsplug/bpd/__init__.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/beetsplug/bpd/__init__.py b/beetsplug/bpd/__init__.py index 52619e7e14..1e0d574255 100644 --- a/beetsplug/bpd/__init__.py +++ b/beetsplug/bpd/__init__.py @@ -1029,8 +1029,11 @@ def cmd_status(self, conn): yield ( u'bitrate: ' + six.text_type(item.bitrate / 1000), - # TODO provide a real value samplerate:bits:channels 44100:24:2 - u'audio: 0:0:0', + u'audio: {}:{}:{}'.format( + six.text_type(item.samplerate), + six.text_type(item.bitdepth), + six.text_type(item.channels), + ), ) (pos, total) = self.player.time() From 36c85a8aebe31e721cd4155a3445040ef9f52d50 Mon Sep 17 00:00:00 2001 From: Carl Suster Date: Tue, 2 Apr 2019 10:10:59 +1100 Subject: [PATCH 17/23] Fix beets.util.inspect for Python 3 Under the original compatibility shim we weren't correctly inclusing `self` in the argument list for bound methods. --- beets/util/inspect.py | 26 +------------------------- beetsplug/bpd/__init__.py | 2 +- 2 files changed, 2 insertions(+), 26 deletions(-) diff --git a/beets/util/inspect.py b/beets/util/inspect.py index 9815a561a1..b90c0fe451 100644 --- a/beets/util/inspect.py +++ b/beets/util/inspect.py @@ -16,35 +16,11 @@ from __future__ import division, absolute_import, print_function import inspect -from collections import namedtuple from six import PY2 -ArgSpec = namedtuple('ArgSpec', 'args varargs keywords defaults') - - def getargspec(func): if PY2: return inspect.getargspec(func) - - sig = inspect.signature(func) - args = [ - p.name for p in sig.parameters.values() - if p.kind == inspect.Parameter.POSITIONAL_OR_KEYWORD - ] - varargs = [ - p.name for p in sig.parameters.values() - if p.kind == inspect.Parameter.VAR_POSITIONAL - ] - varargs = varargs[0] if varargs else None - varkw = [ - p.name for p in sig.parameters.values() - if p.kind == inspect.Parameter.VAR_KEYWORD - ] - varkw = varkw[0] if varkw else None - defaults = tuple(p.default for p in sig.parameters.values() - if p.kind == inspect.Parameter.POSITIONAL_OR_KEYWORD - and p.default is not p.empty) or None - - return ArgSpec(args, varargs, varkw, defaults) + return inspect.getfullargspec(func) diff --git a/beetsplug/bpd/__init__.py b/beetsplug/bpd/__init__.py index 1e0d574255..086af21b39 100644 --- a/beetsplug/bpd/__init__.py +++ b/beetsplug/bpd/__init__.py @@ -750,7 +750,7 @@ def run(self, conn): u'unknown command "{}"'.format(self.name)) func = getattr(conn.server, func_name) argspec = inspect.getargspec(func) - max_args = len(argspec.args) - 1 + max_args = len(argspec.args) - 2 min_args = max_args if argspec.defaults: min_args -= len(argspec.defaults) From 4be2e1b5e68d37e806b9766c30436f987113912b Mon Sep 17 00:00:00 2001 From: Carl Suster Date: Tue, 2 Apr 2019 10:22:47 +1100 Subject: [PATCH 18/23] Remove beets.util.inspect wrapper --- beets/plugins.py | 9 ++++++--- beets/util/inspect.py | 26 -------------------------- beetsplug/bpd/__init__.py | 11 +++++++++-- test/test_plugins.py | 2 +- 4 files changed, 16 insertions(+), 32 deletions(-) delete mode 100644 beets/util/inspect.py diff --git a/beets/plugins.py b/beets/plugins.py index 7019b70a02..5ca9ae3bb3 100644 --- a/beets/plugins.py +++ b/beets/plugins.py @@ -19,6 +19,7 @@ import traceback import re +import inspect from collections import defaultdict from functools import wraps @@ -26,7 +27,6 @@ import beets from beets import logging from beets import mediafile -from beets.util import inspect import six PLUGIN_NAMESPACE = 'beetsplug' @@ -127,7 +127,10 @@ def _set_log_level_and_params(self, base_log_level, func): value after the function returns). Also determines which params may not be sent for backwards-compatibility. """ - argspec = inspect.getargspec(func) + if six.PY2: + func_args = inspect.getargspec(func).args + else: + func_args = inspect.getfullargspec(func).args @wraps(func) def wrapper(*args, **kwargs): @@ -142,7 +145,7 @@ def wrapper(*args, **kwargs): if exc.args[0].startswith(func.__name__): # caused by 'func' and not stuff internal to 'func' kwargs = dict((arg, val) for arg, val in kwargs.items() - if arg in argspec.args) + if arg in func_args) return func(*args, **kwargs) else: raise diff --git a/beets/util/inspect.py b/beets/util/inspect.py deleted file mode 100644 index b90c0fe451..0000000000 --- a/beets/util/inspect.py +++ /dev/null @@ -1,26 +0,0 @@ -# -*- coding: utf-8 -*- -# This file is part of beets. -# Copyright 2019, Vladimir Zhelezov. -# -# Permission is hereby granted, free of charge, to any person obtaining -# a copy of this software and associated documentation files (the -# "Software"), to deal in the Software without restriction, including -# without limitation the rights to use, copy, modify, merge, publish, -# distribute, sublicense, and/or sell copies of the Software, and to -# permit persons to whom the Software is furnished to do so, subject to -# the following conditions: -# -# The above copyright notice and this permission notice shall be -# included in all copies or substantial portions of the Software. - -from __future__ import division, absolute_import, print_function - -import inspect - -from six import PY2 - - -def getargspec(func): - if PY2: - return inspect.getargspec(func) - return inspect.getfullargspec(func) diff --git a/beetsplug/bpd/__init__.py b/beetsplug/bpd/__init__.py index 086af21b39..c8fc10509c 100644 --- a/beetsplug/bpd/__init__.py +++ b/beetsplug/bpd/__init__.py @@ -26,12 +26,13 @@ import random import time import math +import inspect import beets from beets.plugins import BeetsPlugin import beets.ui from beets import vfs -from beets.util import bluelet, inspect +from beets.util import bluelet from beets.library import Item from beets import dbcore from beets.mediafile import MediaFile @@ -749,7 +750,13 @@ def run(self, conn): raise BPDError(ERROR_UNKNOWN, u'unknown command "{}"'.format(self.name)) func = getattr(conn.server, func_name) - argspec = inspect.getargspec(func) + + if six.PY2: + # caution: the fields of the namedtuple are slightly different + argspec = inspect.getargspec(func) + else: + argspec = inspect.getfullargspec(func) + max_args = len(argspec.args) - 2 min_args = max_args if argspec.defaults: diff --git a/test/test_plugins.py b/test/test_plugins.py index 7c32e9aca6..b141586995 100644 --- a/test/test_plugins.py +++ b/test/test_plugins.py @@ -322,7 +322,7 @@ def dummy(self): @patch('beets.plugins.find_plugins') @patch('beets.plugins.inspect') def test_events_called(self, mock_inspect, mock_find_plugins): - mock_inspect.getargspec.return_value = None + mock_inspect.getargspec.args.return_value = None class DummyPlugin(plugins.BeetsPlugin): def __init__(self): From 28db7d3d33b8bd989e9dc32fd9c0fc97dbbf42c7 Mon Sep 17 00:00:00 2001 From: Carl Suster Date: Tue, 2 Apr 2019 11:15:00 +1100 Subject: [PATCH 19/23] bpd: provide precision time in status --- beetsplug/bpd/__init__.py | 10 ++++++---- beetsplug/bpd/gstplayer.py | 4 ++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/beetsplug/bpd/__init__.py b/beetsplug/bpd/__init__.py index c8fc10509c..fd714ab633 100644 --- a/beetsplug/bpd/__init__.py +++ b/beetsplug/bpd/__init__.py @@ -1045,10 +1045,12 @@ def cmd_status(self, conn): (pos, total) = self.player.time() yield ( - u'time: ' + six.text_type(pos) + u':' + six.text_type(total), - # TODO provide elapsed and duration with higher precision - u'elapsed: ' + six.text_type(float(pos)), - u'duration: ' + six.text_type(float(total)), + u'time: {}:{}'.format( + six.text_type(int(pos)), + six.text_type(int(total)), + ), + u'elapsed: ' + u'{:.3f}'.format(pos), + u'duration: ' + u'{:.3f}'.format(total), ) # Also missing 'updating_db'. diff --git a/beetsplug/bpd/gstplayer.py b/beetsplug/bpd/gstplayer.py index 705692aa51..fffa8a6eda 100644 --- a/beetsplug/bpd/gstplayer.py +++ b/beetsplug/bpd/gstplayer.py @@ -177,12 +177,12 @@ def time(self): posq = self.player.query_position(fmt) if not posq[0]: raise QueryError("query_position failed") - pos = posq[1] // (10 ** 9) + pos = posq[1] / (10 ** 9) lengthq = self.player.query_duration(fmt) if not lengthq[0]: raise QueryError("query_duration failed") - length = lengthq[1] // (10 ** 9) + length = lengthq[1] / (10 ** 9) self.cached_time = (pos, length) return (pos, length) From d074dac771689fc33a59cdcd115c4bfe80f6afed Mon Sep 17 00:00:00 2001 From: Carl Suster Date: Tue, 2 Apr 2019 13:37:40 +1100 Subject: [PATCH 20/23] bpd: add comments to the error handling code --- beetsplug/bpd/__init__.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/beetsplug/bpd/__init__.py b/beetsplug/bpd/__init__.py index fd714ab633..93237ca874 100644 --- a/beetsplug/bpd/__init__.py +++ b/beetsplug/bpd/__init__.py @@ -623,6 +623,11 @@ def cmd_profile(self, conn): print(heap) def cmd_crash_TypeError(self, conn): + """Deliberately trigger a TypeError for testing purposes. + We want to test that the server properly responds with ERROR_SYSTEM + without crashing, and that this is not treated as ERROR_ARG (since it + is caused by a programming error, not a protocol error). + """ 'a' + 2 @@ -753,15 +758,21 @@ def run(self, conn): if six.PY2: # caution: the fields of the namedtuple are slightly different + # between the results of getargspec and getfullargspec. argspec = inspect.getargspec(func) else: argspec = inspect.getfullargspec(func) + # Check that `func` is able to handle the number of arguments sent + # by the client (so we can raise ERROR_ARG instead of ERROR_SYSTEM). + # Maximum accepted arguments: argspec includes "self" and "conn". max_args = len(argspec.args) - 2 + # Minimum accepted arguments: some arguments might be optional/ min_args = max_args if argspec.defaults: min_args -= len(argspec.defaults) wrong_num = (len(self.args) > max_args) or (len(self.args) < min_args) + # If the command accepts a variable number of arguments skip the check. if wrong_num and not argspec.varargs: raise BPDError(ERROR_ARG, u'wrong number of arguments for "{}"' From 20e2f8beec316b2c6bd535ae110c316b7274a0c2 Mon Sep 17 00:00:00 2001 From: Carl Suster Date: Tue, 2 Apr 2019 13:38:43 +1100 Subject: [PATCH 21/23] bpd: output an info-level message when ready --- beetsplug/bpd/__init__.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/beetsplug/bpd/__init__.py b/beetsplug/bpd/__init__.py index 93237ca874..88faad703e 100644 --- a/beetsplug/bpd/__init__.py +++ b/beetsplug/bpd/__init__.py @@ -858,10 +858,13 @@ def __init__(self, library, host, port, password, log): raise NoGstreamerError() else: raise + log.info(u'Starting server...') super(Server, self).__init__(host, port, password, log) self.lib = library self.player = gstplayer.GstPlayer(self.play_finished) self.cmd_update(None) + log.info(u'Server ready and listening on {}:{}'.format( + host, port)) def run(self): self.player.run() From 140d25df5287c0cf949094b696266113c01d7998 Mon Sep 17 00:00:00 2001 From: Carl Suster Date: Tue, 2 Apr 2019 13:50:16 +1100 Subject: [PATCH 22/23] Changelog for #3200 --- docs/changelog.rst | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index c822af6089..1acfdb9c3d 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -81,6 +81,11 @@ New features: to be displayed. Thanks to :user:`pprkut`. :bug:`3089` +* :doc:`/plugins/bpd`: MPD protocol commands ``consume`` and ``single`` are now + supported along with updated semantics for ``repeat`` and ``previous`` and + new fields for ``status``. The bpd server now understands and ignores some + additional commands. + :bug:`3200` :bug:`800` Changes: @@ -181,6 +186,8 @@ Fixes: :bug:`3192` * Fix compatibility with pre-release versions of Python 3.8. :bug:`3201` :bug:`3202` +* :doc:`/plugins/bpd`: Fix crashes in the bpd server during exception handling. + :bug:`3200` .. _python-itunes: https://github.com/ocelma/python-itunes From 95dd513b2585d0a1a997f25ccfae856027039e69 Mon Sep 17 00:00:00 2001 From: Carl Suster Date: Tue, 2 Apr 2019 14:25:56 +1100 Subject: [PATCH 23/23] bpd: add flake8 exception for test command --- beetsplug/bpd/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beetsplug/bpd/__init__.py b/beetsplug/bpd/__init__.py index 88faad703e..598e2971f3 100644 --- a/beetsplug/bpd/__init__.py +++ b/beetsplug/bpd/__init__.py @@ -622,7 +622,7 @@ def cmd_profile(self, conn): heap = hpy().heap() print(heap) - def cmd_crash_TypeError(self, conn): + def cmd_crash_TypeError(self, conn): # noqa: N802 """Deliberately trigger a TypeError for testing purposes. We want to test that the server properly responds with ERROR_SYSTEM without crashing, and that this is not treated as ERROR_ARG (since it