From 0ccb5dec23e32747eb4e59fb8742e930a36e2601 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sun, 26 Jun 2011 17:37:22 +0200 Subject: [PATCH 01/21] Add debug tool that inspects MPD's idle behaviour --- tools/idle.py | 201 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 201 insertions(+) create mode 100644 tools/idle.py diff --git a/tools/idle.py b/tools/idle.py new file mode 100644 index 00000000..aa56dce2 --- /dev/null +++ b/tools/idle.py @@ -0,0 +1,201 @@ +#! /usr/bin/env python + +# This script is helper to systematicly test the behaviour of MPD's idle +# command. It is simply provided as a quick hack, expect nothing more. + +import logging +import pprint +import socket + +host = '' +port = 6601 + +url = "13 - a-ha - White Canvas.mp3" +artist = "a-ha" + +data = {'id': None, 'id2': None, 'url': url, 'artist': artist} + +# Commands to run before test requests to coerce MPD into right state +setup_requests = [ + 'clear', + 'add "%(url)s"', + 'add "%(url)s"', + 'add "%(url)s"', + 'play', +# 'pause', # Uncomment to test paused idle behaviour +# 'stop', # Uncomment to test stopped idle behaviour +] + +# List of commands to test for idle behaviour. Ordering of list is important in +# order to keep MPD state as intended. Commands that are obviously +# informational only or "harmfull" have been excluded. +test_requests = [ + 'add "%(url)s"', + 'addid "%(url)s" "1"', + 'clear', +# 'clearerror', +# 'close', +# 'commands', + 'consume "1"', + 'consume "0"', +# 'count', + 'crossfade "1"', + 'crossfade "0"', +# 'currentsong', +# 'delete "1:2"', + 'delete "0"', + 'deleteid "%(id)s"', + 'disableoutput "0"', + 'enableoutput "0"', +# 'find', +# 'findadd "artist" "%(artist)s"', +# 'idle', +# 'kill', +# 'list', +# 'listall', +# 'listallinfo', +# 'listplaylist', +# 'listplaylistinfo', +# 'listplaylists', +# 'lsinfo', + 'move "0:1" "2"', + 'move "0" "1"', + 'moveid "%(id)s" "1"', + 'next', +# 'notcommands', +# 'outputs', +# 'password', + 'pause', +# 'ping', + 'play', + 'playid "%(id)s"', +# 'playlist', + 'playlistadd "foo" "%(url)s"', + 'playlistclear "foo"', + 'playlistadd "foo" "%(url)s"', + 'playlistdelete "foo" "0"', +# 'playlistfind', +# 'playlistid', +# 'playlistinfo', + 'playlistadd "foo" "%(url)s"', + 'playlistadd "foo" "%(url)s"', + 'playlistmove "foo" "0" "1"', +# 'playlistsearch', +# 'plchanges', +# 'plchangesposid', + 'previous', + 'random "1"', + 'random "0"', + 'rm "bar"', + 'rename "foo" "bar"', + 'repeat "0"', + 'rm "bar"', + 'save "bar"', + 'load "bar"', +# 'search', + 'seek "1" "10"', + 'seekid "%(id)s" "10"', +# 'setvol "10"', + 'shuffle', + 'shuffle "0:1"', + 'single "1"', + 'single "0"', +# 'stats', +# 'status', + 'stop', + 'swap "1" "2"', + 'swapid "%(id)s" "%(id2)s"', +# 'tagtypes', +# 'update', +# 'urlhandlers', +# 'volume', +] + + +def create_socketfile(): + sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + sock.connect((host, port)) + sock.settimeout(0.5) + fd = sock.makefile('rw', 1) # 1 = line buffered + fd.readline() # Read banner + return fd + + +def wait(fd, prefix=None, collect=None): + while True: + line = fd.readline().rstrip() + if prefix: + logging.debug('%s: %s', prefix, repr(line)) + if line.split()[0] in ('OK', 'ACK'): + break + + +def collect_ids(fd): + fd.write('playlistinfo\n') + + ids = [] + while True: + line = fd.readline() + if line.split()[0] == 'OK': + break + if line.split()[0] == 'Id:': + ids.append(line.split()[1]) + return ids + + +def main(): + subsystems = {} + + command = create_socketfile() + + for test in test_requests: + # Remove any old ids + del data['id'] + del data['id2'] + + # Run setup code to force MPD into known state + for setup in setup_requests: + command.write(setup % data + '\n') + wait(command) + + data['id'], data['id2'] = collect_ids(command)[:2] + + # This connection needs to be make after setup commands are done or + # else they will cause idle events. + idle = create_socketfile() + + # Wait for new idle events + idle.write('idle\n') + + test = test % data + + logging.debug('idle: %s', repr('idle')) + logging.debug('command: %s', repr(test)) + + command.write(test + '\n') + wait(command, prefix='command') + + while True: + try: + line = idle.readline().rstrip() + except socket.timeout: + # Abort try if we time out. + idle.write('noidle\n') + break + + logging.debug('idle: %s', repr(line)) + + if line == 'OK': + break + + request_type = test.split()[0] + subsystem = line.split()[1] + subsystems.setdefault(request_type, set()).add(subsystem) + + logging.debug('---') + + pprint.pprint(subsystems) + + +if __name__ == '__main__': + main() From e919dcf627de3f3eb8951cafc410ec2d347f2dd1 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Tue, 19 Jul 2011 00:49:43 +0200 Subject: [PATCH 02/21] Create helper for sending events to BackendListeners --- mopidy/backends/base/playback.py | 20 +++++--------------- mopidy/frontends/mpd/__init__.py | 2 +- mopidy/listeners.py | 12 ++++++++++++ 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/mopidy/backends/base/playback.py b/mopidy/backends/base/playback.py index 088a5ad4..78e5057d 100644 --- a/mopidy/backends/base/playback.py +++ b/mopidy/backends/base/playback.py @@ -464,27 +464,17 @@ class PlaybackController(object): logger.debug(u'Triggering started playing event') if self.current_track is None: return - ActorRegistry.broadcast({ - 'command': 'pykka_call', - 'attr_path': ('started_playing',), - 'args': [], - 'kwargs': {'track': self.current_track}, - }, target_class=BackendListener) + BackendListener.send('started_playing', + track=self.current_track) def _trigger_stopped_playing_event(self): # TODO Test that this is called on next/prev/end-of-track logger.debug(u'Triggering stopped playing event') if self.current_track is None: return - ActorRegistry.broadcast({ - 'command': 'pykka_call', - 'attr_path': ('stopped_playing',), - 'args': [], - 'kwargs': { - 'track': self.current_track, - 'time_position': self.time_position, - }, - }, target_class=BackendListener) + BackendListener.send('stopped_playing', + track=self.current_track, + time_position=self.time_position) class BasePlaybackProvider(object): diff --git a/mopidy/frontends/mpd/__init__.py b/mopidy/frontends/mpd/__init__.py index 4deb7b89..561f9295 100644 --- a/mopidy/frontends/mpd/__init__.py +++ b/mopidy/frontends/mpd/__init__.py @@ -67,7 +67,7 @@ class MpdSession(network.LineProtocol): logger.debug(u'Response to [%s]:%s from %s: %s', self.host, self.port, self.actor_urn, log.indent(self.terminator.join(response))) - + self.send_lines(response) def close(self): diff --git a/mopidy/listeners.py b/mopidy/listeners.py index dfc5c60b..9977d5da 100644 --- a/mopidy/listeners.py +++ b/mopidy/listeners.py @@ -1,3 +1,5 @@ +from pykka import registry + class BackendListener(object): """ Marker interface for recipients of events sent by the backend. @@ -9,6 +11,16 @@ class BackendListener(object): interested in all events. """ + @staticmethod + def send(event, **kwargs): + """Helper to allow calling of backend listener events""" + registry.ActorRegistry.broadcast({ + 'command': 'pykka_call', + 'attr_path': (event,), + 'args': [], + 'kwargs': kwargs + }, target_class=BackendListener) + def started_playing(self, track): """ Called whenever a new track starts playing. From 215ed61b0b9b52078b15a93d35d374012ce355a0 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Tue, 19 Jul 2011 02:29:40 +0200 Subject: [PATCH 03/21] Update existing listener events to reflect that they only notify about track changes --- mopidy/backends/base/playback.py | 23 +++++++++++------------ mopidy/frontends/lastfm.py | 4 ++-- mopidy/listeners.py | 6 +++--- tests/backends/events_test.py | 24 ++++++++++++------------ tests/listeners_test.py | 8 ++++---- 5 files changed, 32 insertions(+), 33 deletions(-) diff --git a/mopidy/backends/base/playback.py b/mopidy/backends/base/playback.py index 78e5057d..7b697781 100644 --- a/mopidy/backends/base/playback.py +++ b/mopidy/backends/base/playback.py @@ -326,7 +326,7 @@ class PlaybackController(object): original_cp_track = self.current_cp_track if self.cp_track_at_eot: - self._trigger_stopped_playing_event() + self._trigger_track_playback_ended() self.play(self.cp_track_at_eot) else: self.stop(clear_current_track=True) @@ -354,7 +354,7 @@ class PlaybackController(object): return if self.cp_track_at_next: - self._trigger_stopped_playing_event() + self._trigger_track_playback_ended() self.play(self.cp_track_at_next) else: self.stop(clear_current_track=True) @@ -402,7 +402,7 @@ class PlaybackController(object): if self.random and self.current_cp_track in self._shuffled: self._shuffled.remove(self.current_cp_track) - self._trigger_started_playing_event() + self._trigger_track_playback_started() def previous(self): """Play the previous track.""" @@ -410,7 +410,7 @@ class PlaybackController(object): return if self.state == self.STOPPED: return - self._trigger_stopped_playing_event() + self._trigger_track_playback_ended() self.play(self.cp_track_at_previous, on_error_step=-1) def resume(self): @@ -454,25 +454,24 @@ class PlaybackController(object): :type clear_current_track: boolean """ if self.state != self.STOPPED: - self._trigger_stopped_playing_event() if self.provider.stop(): + self._trigger_track_playback_ended() self.state = self.STOPPED if clear_current_track: self.current_cp_track = None - def _trigger_started_playing_event(self): - logger.debug(u'Triggering started playing event') + def _trigger_track_playback_started(self): + logger.debug(u'Triggering track playback started event') if self.current_track is None: return - BackendListener.send('started_playing', + BackendListener.send('track_playback_started', track=self.current_track) - def _trigger_stopped_playing_event(self): - # TODO Test that this is called on next/prev/end-of-track - logger.debug(u'Triggering stopped playing event') + def _trigger_track_playback_ended(self): + logger.debug(u'Triggering track playback ended event') if self.current_track is None: return - BackendListener.send('stopped_playing', + BackendListener.send('track_playback_ended', track=self.current_track, time_position=self.time_position) diff --git a/mopidy/frontends/lastfm.py b/mopidy/frontends/lastfm.py index d50f8dd8..125457cd 100644 --- a/mopidy/frontends/lastfm.py +++ b/mopidy/frontends/lastfm.py @@ -57,7 +57,7 @@ class LastfmFrontend(ThreadingActor, BackendListener): logger.error(u'Error during Last.fm setup: %s', e) self.stop() - def started_playing(self, track): + def track_playback_started(self, track): artists = ', '.join([a.name for a in track.artists]) duration = track.length and track.length // 1000 or 0 self.last_start_time = int(time.time()) @@ -74,7 +74,7 @@ class LastfmFrontend(ThreadingActor, BackendListener): pylast.MalformedResponseError, pylast.WSError) as e: logger.warning(u'Error submitting playing track to Last.fm: %s', e) - def stopped_playing(self, track, time_position): + def track_playback_ended(self, track, time_position): artists = ', '.join([a.name for a in track.artists]) duration = track.length and track.length // 1000 or 0 time_position = time_position // 1000 diff --git a/mopidy/listeners.py b/mopidy/listeners.py index 9977d5da..bcce0c40 100644 --- a/mopidy/listeners.py +++ b/mopidy/listeners.py @@ -21,7 +21,7 @@ class BackendListener(object): 'kwargs': kwargs }, target_class=BackendListener) - def started_playing(self, track): + def track_playback_started(self, track): """ Called whenever a new track starts playing. @@ -32,9 +32,9 @@ class BackendListener(object): """ pass - def stopped_playing(self, track, time_position): + def track_playback_ended(self, track, time_position): """ - Called whenever playback is stopped. + Called whenever playback of a track ends. *MAY* be implemented by actor. diff --git a/tests/backends/events_test.py b/tests/backends/events_test.py index 44529e90..bc39ac00 100644 --- a/tests/backends/events_test.py +++ b/tests/backends/events_test.py @@ -11,8 +11,8 @@ from mopidy.models import Track class BackendEventsTest(unittest.TestCase): def setUp(self): self.events = { - 'started_playing': threading.Event(), - 'stopped_playing': threading.Event(), + 'track_playback_started': threading.Event(), + 'track_playback_ended': threading.Event(), } self.backend = DummyBackend.start().proxy() self.listener = DummyBackendListener.start(self.events).proxy() @@ -20,26 +20,26 @@ class BackendEventsTest(unittest.TestCase): def tearDown(self): ActorRegistry.stop_all() - def test_play_sends_started_playing_event(self): + def test_play_sends_track_playback_started_event(self): self.backend.current_playlist.add([Track(uri='a')]) self.backend.playback.play() - self.events['started_playing'].wait(timeout=1) - self.assertTrue(self.events['started_playing'].is_set()) + self.events['track_playback_started'].wait(timeout=1) + self.assertTrue(self.events['track_playback_started'].is_set()) - def test_stop_sends_stopped_playing_event(self): + def test_stop_sends_track_playback_ended_event(self): self.backend.current_playlist.add([Track(uri='a')]) self.backend.playback.play() self.backend.playback.stop() - self.events['stopped_playing'].wait(timeout=1) - self.assertTrue(self.events['stopped_playing'].is_set()) + self.events['track_playback_ended'].wait(timeout=1) + self.assertTrue(self.events['track_playback_ended'].is_set()) class DummyBackendListener(ThreadingActor, BackendListener): def __init__(self, events): self.events = events - def started_playing(self, track): - self.events['started_playing'].set() + def track_playback_started(self, track): + self.events['track_playback_started'].set() - def stopped_playing(self, track, time_position): - self.events['stopped_playing'].set() + def track_playback_ended(self, track, time_position): + self.events['track_playback_ended'].set() diff --git a/tests/listeners_test.py b/tests/listeners_test.py index 761aff4f..2c31efdb 100644 --- a/tests/listeners_test.py +++ b/tests/listeners_test.py @@ -7,8 +7,8 @@ class BackendListenerTest(unittest.TestCase): def setUp(self): self.listener = BackendListener() - def test_listener_has_default_impl_for_the_started_playing_event(self): - self.listener.started_playing(Track()) + def test_listener_has_default_impl_for_the_track_playback_started(self): + self.listener.track_playback_started(Track()) - def test_listener_has_default_impl_for_the_stopped_playing_event(self): - self.listener.stopped_playing(Track(), 0) + def test_listener_has_default_impl_for_the_track_playback_ended(self): + self.listener.track_playback_ended(Track(), 0) From 1e5a5fb7d0843c79d32fdc0029728dbd2f25f5bf Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Tue, 19 Jul 2011 02:43:48 +0200 Subject: [PATCH 04/21] Add playback state changed event --- mopidy/backends/base/playback.py | 8 +++++++- mopidy/listeners.py | 8 ++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/mopidy/backends/base/playback.py b/mopidy/backends/base/playback.py index 7b697781..1b5e1c9f 100644 --- a/mopidy/backends/base/playback.py +++ b/mopidy/backends/base/playback.py @@ -276,6 +276,9 @@ class PlaybackController(object): def state(self, new_state): (old_state, self._state) = (self.state, new_state) logger.debug(u'Changing state: %s -> %s', old_state, new_state) + + self._trigger_playback_state_changed() + # FIXME play_time stuff assumes backend does not have a better way of # handeling this stuff :/ if (old_state in (self.PLAYING, self.STOPPED) @@ -387,7 +390,6 @@ class PlaybackController(object): self.resume() if cp_track is not None: - self.state = self.STOPPED self.current_cp_track = cp_track self.state = self.PLAYING if not self.provider.play(cp_track.track): @@ -475,6 +477,10 @@ class PlaybackController(object): track=self.current_track, time_position=self.time_position) + def _trigger_playback_state_changed(self): + logger.debug(u'Triggering playback state change event') + BackendListener.send('playback_state_changed') + class BasePlaybackProvider(object): """ diff --git a/mopidy/listeners.py b/mopidy/listeners.py index bcce0c40..f6973eaf 100644 --- a/mopidy/listeners.py +++ b/mopidy/listeners.py @@ -44,3 +44,11 @@ class BackendListener(object): :type time_position: int """ pass + + def playback_state_changed(self): + """ + Called whenever playback state is changed. + + *MAY* be implemented by actor. + """ + pass From ce1a0118d80500a31b55de2874e258deb69c5dd1 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Tue, 19 Jul 2011 02:54:27 +0200 Subject: [PATCH 05/21] Add event that indicates playlist whenever version is changed --- mopidy/backends/base/current_playlist.py | 20 ++++++++++++++++---- mopidy/listeners.py | 8 ++++++++ 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/mopidy/backends/base/current_playlist.py b/mopidy/backends/base/current_playlist.py index 2633f166..e89c23d5 100644 --- a/mopidy/backends/base/current_playlist.py +++ b/mopidy/backends/base/current_playlist.py @@ -2,6 +2,7 @@ from copy import copy import logging import random +from mopidy.listeners import BackendListener from mopidy.models import CpTrack logger = logging.getLogger('mopidy.backends.base') @@ -16,6 +17,7 @@ class CurrentPlaylistController(object): def __init__(self, backend): self.backend = backend + self.cp_id = 0 self._cp_tracks = [] self._version = 0 @@ -53,8 +55,9 @@ class CurrentPlaylistController(object): def version(self, version): self._version = version self.backend.playback.on_current_playlist_change() + self._trigger_playlist_changed() - def add(self, track, at_position=None): + def add(self, track, at_position=None, increase_version=True): """ Add the track to the end of, or at the given position in the current playlist. @@ -68,12 +71,14 @@ class CurrentPlaylistController(object): """ assert at_position <= len(self._cp_tracks), \ u'at_position can not be greater than playlist length' - cp_track = CpTrack(self.version, track) + cp_track = CpTrack(self.cp_id, track) if at_position is not None: self._cp_tracks.insert(at_position, cp_track) else: self._cp_tracks.append(cp_track) - self.version += 1 + if increase_version: + self.version += 1 + self.cp_id += 1 return cp_track def append(self, tracks): @@ -84,7 +89,10 @@ class CurrentPlaylistController(object): :type tracks: list of :class:`mopidy.models.Track` """ for track in tracks: - self.add(track) + self.add(track, increase_version=False) + + if tracks: + self.version += 1 def clear(self): """Clear the current playlist.""" @@ -199,3 +207,7 @@ class CurrentPlaylistController(object): random.shuffle(shuffled) self._cp_tracks = before + shuffled + after self.version += 1 + + def _trigger_playlist_changed(self): + logger.debug(u'Triggering playlist changed event') + BackendListener.send('playlist_changed') diff --git a/mopidy/listeners.py b/mopidy/listeners.py index f6973eaf..397e08ea 100644 --- a/mopidy/listeners.py +++ b/mopidy/listeners.py @@ -52,3 +52,11 @@ class BackendListener(object): *MAY* be implemented by actor. """ pass + + def playlist_changed(self): + """ + Called whenever a playlist is changed. + + *MAY* be implemented by actor. + """ + pass From 4f124480c3998b1d0911f72fd44e535c62b03ffe Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Tue, 19 Jul 2011 03:08:29 +0200 Subject: [PATCH 06/21] Add an options wrapper to magically pick up changes --- mopidy/backends/base/playback.py | 23 +++++++++++++++++++---- mopidy/listeners.py | 8 ++++++++ 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/mopidy/backends/base/playback.py b/mopidy/backends/base/playback.py index 1b5e1c9f..5cab8229 100644 --- a/mopidy/backends/base/playback.py +++ b/mopidy/backends/base/playback.py @@ -8,6 +8,17 @@ from mopidy.listeners import BackendListener logger = logging.getLogger('mopidy.backends.base') + +def option_wrapper(name, default): + def get_option(self): + return getattr(self, name, default) + def set_option(self, value): + if getattr(self, name, default) != value: + self._trigger_options_changed() + return setattr(self, name, value) + return property(get_option, set_option) + + class PlaybackController(object): """ :param backend: the backend @@ -34,7 +45,7 @@ class PlaybackController(object): #: Tracks are removed from the playlist when they have been played. #: :class:`False` #: Tracks are not removed from the playlist. - consume = False + consume = option_wrapper('_consume', False) #: The currently playing or selected track. #: @@ -46,21 +57,21 @@ class PlaybackController(object): #: Tracks are selected at random from the playlist. #: :class:`False` #: Tracks are played in the order of the playlist. - random = False + random = option_wrapper('_random', False) #: :class:`True` #: The current playlist is played repeatedly. To repeat a single track, #: select both :attr:`repeat` and :attr:`single`. #: :class:`False` #: The current playlist is played once. - repeat = False + repeat = option_wrapper('_repeat', False) #: :class:`True` #: Playback is stopped after current song, unless in :attr:`repeat` #: mode. #: :class:`False` #: Playback continues after current song. - single = False + single = option_wrapper('_single', False) def __init__(self, backend, provider): self.backend = backend @@ -481,6 +492,10 @@ class PlaybackController(object): logger.debug(u'Triggering playback state change event') BackendListener.send('playback_state_changed') + def _trigger_options_changed(self): + logger.debug(u'Triggering options changed event') + BackendListener.send('options_changed') + class BasePlaybackProvider(object): """ diff --git a/mopidy/listeners.py b/mopidy/listeners.py index 397e08ea..c0453a2b 100644 --- a/mopidy/listeners.py +++ b/mopidy/listeners.py @@ -60,3 +60,11 @@ class BackendListener(object): *MAY* be implemented by actor. """ pass + + def options_changed(self): + """ + Called whenever an option is changed. + + *MAY* be implemented by actor. + """ + pass From 8ae0381cd8d54b81e31fea994397d62347a44a18 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Tue, 19 Jul 2011 03:44:34 +0200 Subject: [PATCH 07/21] Start adding idle to frontend, mpd-session and dispatcher --- mopidy/frontends/mpd/__init__.py | 28 +++++++++++++++++++++++++--- mopidy/frontends/mpd/dispatcher.py | 3 +++ mopidy/listeners.py | 4 +++- 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/mopidy/frontends/mpd/__init__.py b/mopidy/frontends/mpd/__init__.py index 561f9295..6f6e3bfc 100644 --- a/mopidy/frontends/mpd/__init__.py +++ b/mopidy/frontends/mpd/__init__.py @@ -1,15 +1,15 @@ import logging import sys -from pykka.actor import ThreadingActor +from pykka import registry, actor -from mopidy import settings +from mopidy import listeners, settings from mopidy.frontends.mpd import dispatcher, protocol from mopidy.utils import network, process, log logger = logging.getLogger('mopidy.frontends.mpd') -class MpdFrontend(ThreadingActor): +class MpdFrontend(actor.ThreadingActor, listeners.BackendListener): """ The MPD frontend. @@ -39,6 +39,25 @@ class MpdFrontend(ThreadingActor): def on_stop(self): process.stop_actors_by_class(MpdSession) + def send_idle(self, subsystem): + # FIXME this should be updated once pykka supports non-blocking calls + # on proxies or some similar solution + registry.ActorRegistry.broadcast({ + 'command': 'pykka_call', + 'attr_path': ('on_idle',), + 'args': [subsystem], + 'kwargs': {}, + }, target_class=MpdSession) + + def playback_state_changed(self): + self.send_idle('player') + + def playlist_changed(self): + self.send_idle('playlist') + + def options_changed(self): + self.send_idle('options') + class MpdSession(network.LineProtocol): """ @@ -70,5 +89,8 @@ class MpdSession(network.LineProtocol): self.send_lines(response) + def on_idle(self, subsystem): + self.dispatcher.handle_idle(subsystem) + def close(self): self.stop() diff --git a/mopidy/frontends/mpd/dispatcher.py b/mopidy/frontends/mpd/dispatcher.py index 0f0f0299..6cc05bec 100644 --- a/mopidy/frontends/mpd/dispatcher.py +++ b/mopidy/frontends/mpd/dispatcher.py @@ -47,6 +47,9 @@ class MpdDispatcher(object): ] return self._call_next_filter(request, response, filter_chain) + def handle_idle(self, subsystem): + logger.debug(u'Got idle event for %s', subsystem) + def _call_next_filter(self, request, response, filter_chain): if filter_chain: next_filter = filter_chain.pop(0) diff --git a/mopidy/listeners.py b/mopidy/listeners.py index c0453a2b..5fbccff5 100644 --- a/mopidy/listeners.py +++ b/mopidy/listeners.py @@ -14,11 +14,13 @@ class BackendListener(object): @staticmethod def send(event, **kwargs): """Helper to allow calling of backend listener events""" + # FIXME this should be updated once pykka supports non-blocking calls + # on proxies or some similar solution registry.ActorRegistry.broadcast({ 'command': 'pykka_call', 'attr_path': (event,), 'args': [], - 'kwargs': kwargs + 'kwargs': kwargs, }, target_class=BackendListener) def track_playback_started(self, track): From da3b4c4b93fbd51b58a8ccd7974306253d1ec8d8 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Tue, 19 Jul 2011 11:07:36 +0200 Subject: [PATCH 08/21] Add changed event for volume and updated MpdSession to regcognise it. --- mopidy/frontends/mpd/__init__.py | 3 +++ mopidy/listeners.py | 8 ++++++++ mopidy/mixers/base.py | 11 ++++++++++- 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/mopidy/frontends/mpd/__init__.py b/mopidy/frontends/mpd/__init__.py index 6f6e3bfc..ff239458 100644 --- a/mopidy/frontends/mpd/__init__.py +++ b/mopidy/frontends/mpd/__init__.py @@ -58,6 +58,9 @@ class MpdFrontend(actor.ThreadingActor, listeners.BackendListener): def options_changed(self): self.send_idle('options') + def volume_changed(self): + self.send_idle('mixer') + class MpdSession(network.LineProtocol): """ diff --git a/mopidy/listeners.py b/mopidy/listeners.py index 5fbccff5..590f0ad0 100644 --- a/mopidy/listeners.py +++ b/mopidy/listeners.py @@ -70,3 +70,11 @@ class BackendListener(object): *MAY* be implemented by actor. """ pass + + def volume_changed(self): + """ + Called whenever the volume is changed. + + *MAY* be implemented by actor. + """ + pass diff --git a/mopidy/mixers/base.py b/mopidy/mixers/base.py index ec3d8ae5..8798076a 100644 --- a/mopidy/mixers/base.py +++ b/mopidy/mixers/base.py @@ -1,4 +1,8 @@ -from mopidy import settings +import logging + +from mopidy import listeners, settings + +logger = logging.getLogger('mopdy.mixers') class BaseMixer(object): """ @@ -30,6 +34,7 @@ class BaseMixer(object): elif volume > 100: volume = 100 self.set_volume(volume) + self._trigger_volume_changed() def get_volume(self): """ @@ -46,3 +51,7 @@ class BaseMixer(object): *MUST be implemented by subclass.* """ raise NotImplementedError + + def _trigger_volume_changed(self): + logger.debug(u'Triggering volume changed event') + listeners.BackendListener.send('volume_changed') From 08d486785da5a3a90782278c6cabd74bae28ff0b Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 22 Jul 2011 22:50:36 +0200 Subject: [PATCH 09/21] Basic working version of idle command --- mopidy/frontends/mpd/dispatcher.py | 66 +++++- mopidy/frontends/mpd/protocol/status.py | 6 +- tests/frontends/mpd/protocol/__init__.py | 4 + tests/frontends/mpd/protocol/idle_test.py | 210 ++++++++++++++++++ .../frontends/mpd/protocol/reflection_test.py | 4 +- tests/frontends/mpd/protocol/status_test.py | 15 -- 6 files changed, 283 insertions(+), 22 deletions(-) create mode 100644 tests/frontends/mpd/protocol/idle_test.py diff --git a/mopidy/frontends/mpd/dispatcher.py b/mopidy/frontends/mpd/dispatcher.py index 6cc05bec..a36d38e3 100644 --- a/mopidy/frontends/mpd/dispatcher.py +++ b/mopidy/frontends/mpd/dispatcher.py @@ -20,6 +20,10 @@ from mopidy.utils import flatten logger = logging.getLogger('mopidy.frontends.mpd.dispatcher') +#: Subsystems that can be registered with idle command. +SUBSYSTEMS = ['database', 'mixer', 'options', 'output', + 'player', 'playlist', 'stored_playlist', 'update', ] + class MpdDispatcher(object): """ The MPD session feeds the MPD dispatcher with requests. The dispatcher @@ -32,6 +36,8 @@ class MpdDispatcher(object): self.command_list = False self.command_list_ok = False self.command_list_index = None + self.subscriptions = set() + self.events = set() self.context = MpdContext(self, session=session) def handle_request(self, request, current_command_list_index=None): @@ -42,13 +48,26 @@ class MpdDispatcher(object): self._catch_mpd_ack_errors_filter, self._authenticate_filter, self._command_list_filter, + self._idle_filter, self._add_ok_filter, self._call_handler_filter, ] return self._call_next_filter(request, response, filter_chain) def handle_idle(self, subsystem): - logger.debug(u'Got idle event for %s', subsystem) + self.events.add(subsystem) + + subsystems = self.subscriptions.intersection(self.events) + if not subsystems: + return + + response = [] + for subsystem in subsystems: + response.append(u'changed: %s' % subsystem) + response.append(u'OK') + self.subscriptions = set() + self.events = set() + self.context.session.send_lines(response) def _call_next_filter(self, request, response, filter_chain): if filter_chain: @@ -111,17 +130,60 @@ class MpdDispatcher(object): and request != u'command_list_end') + ### Filter: idle + + def _idle_filter(self, request, response, filter_chain): + if re.match(r'^noidle$', request): + if not self.subscriptions: + return [] + self.subscriptions = set() + self.events = set() + self.context.session.connection.enable_timeout() + return [u'OK'] + + if self.subscriptions: + self.context.session.close() + return [] + + if re.match(r'^idle( .+)?$', request): + for subsystem in self._extract_subsystems(request): + self.subscriptions.add(subsystem) + + subsystems = self.subscriptions.intersection(self.events) + if subsystems: + for subsystem in subsystems: + response.append(u'changed: %s' % subsystem) + self.events = set() + self.subscriptions = set() + response.append(u'OK') + return response + else: + self.context.session.connection.disable_timeout() + return [] + + return self._call_next_filter(request, response, filter_chain) + + def _extract_subsystems(self, request): + match = re.match(r'^idle (?P.+)$', request) + if not match: + return SUBSYSTEMS + return match.groupdict()['subsystems'].split(' ') + + ### Filter: add OK def _add_ok_filter(self, request, response, filter_chain): response = self._call_next_filter(request, response, filter_chain) - if not self._has_error(response): + if not self._has_error(response) and not self._is_idle(request): response.append(u'OK') return response def _has_error(self, response): return response and response[-1].startswith(u'ACK') + def _is_idle(self, request): + return request.startswith('idle') + ### Filter: call handler diff --git a/mopidy/frontends/mpd/protocol/status.py b/mopidy/frontends/mpd/protocol/status.py index abbb8d7f..4a961e76 100644 --- a/mopidy/frontends/mpd/protocol/status.py +++ b/mopidy/frontends/mpd/protocol/status.py @@ -32,8 +32,8 @@ def currentsong(context): position=context.backend.playback.current_playlist_position.get(), cpid=current_cp_track.cpid) -@handle_request(r'^idle$') -@handle_request(r'^idle (?P.+)$') +#@handle_request(r'^idle$') +#@handle_request(r'^idle (?P.+)$') def idle(context, subsystems=None): """ *musicpd.org, status section:* @@ -69,7 +69,7 @@ def idle(context, subsystems=None): """ pass # TODO -@handle_request(r'^noidle$') +#@handle_request(r'^noidle$') def noidle(context): """See :meth:`_status_idle`.""" pass # TODO diff --git a/tests/frontends/mpd/protocol/__init__.py b/tests/frontends/mpd/protocol/__init__.py index 77825a6e..68668bc2 100644 --- a/tests/frontends/mpd/protocol/__init__.py +++ b/tests/frontends/mpd/protocol/__init__.py @@ -27,6 +27,7 @@ class BaseTestCase(unittest.TestCase): self.connection = MockConnetion() self.session = mpd.MpdSession(self.connection) self.dispatcher = self.session.dispatcher + self.context = self.dispatcher.context def tearDown(self): self.backend.stop().get() @@ -38,6 +39,9 @@ class BaseTestCase(unittest.TestCase): self.session.on_line_received(request) return self.connection.response + def assertNoResponse(self): + self.assertEqual([], self.connection.response) + def assertInResponse(self, value): self.assert_(value in self.connection.response, u'Did not find %s ' 'in %s' % (repr(value), repr(self.connection.response))) diff --git a/tests/frontends/mpd/protocol/idle_test.py b/tests/frontends/mpd/protocol/idle_test.py new file mode 100644 index 00000000..319789bf --- /dev/null +++ b/tests/frontends/mpd/protocol/idle_test.py @@ -0,0 +1,210 @@ +from mock import patch + +from mopidy.frontends.mpd.dispatcher import SUBSYSTEMS +from mopidy.models import Track + +from tests.frontends.mpd import protocol + +class IdleHandlerTest(protocol.BaseTestCase): + def idleEvent(self, subsystem): + self.session.on_idle(subsystem) + + def assertEqualEvents(self, events): + self.assertEqual(set(events), self.dispatcher.events) + + def assertEqualSubscriptions(self, events): + self.assertEqual(set(events), self.dispatcher.subscriptions) + + def assertNoEvents(self): + self.assertEqualEvents([]) + + def assertNoSubscriptions(self): + self.assertEqualSubscriptions([]) + + def test_base_state(self): + self.assertNoSubscriptions() + self.assertNoEvents() + self.assertNoResponse() + + def test_idle(self): + self.sendRequest(u'idle') + self.assertEqualSubscriptions(SUBSYSTEMS) + self.assertNoEvents() + self.assertNoResponse() + + def test_idle_disables_timeout(self): + self.sendRequest(u'idle') + self.connection.disable_timeout.assert_called_once_with() + + def test_noidle(self): + self.sendRequest(u'noidle') + self.assertNoSubscriptions() + self.assertNoEvents() + self.assertNoResponse() + + def test_noidle_does_not_call_enable_timeout(self): + self.sendRequest(u'noidle') + self.assertEqual(0, self.connection.enable_timeout.call_count) + + def test_idle_player(self): + self.sendRequest(u'idle player') + self.assertEqualSubscriptions(['player']) + self.assertNoEvents() + self.assertNoResponse() + + def test_idle_player_playlist(self): + self.sendRequest(u'idle player playlist') + self.assertEqualSubscriptions(['player', 'playlist']) + self.assertNoEvents() + self.assertNoResponse() + + def test_idle_then_noidle(self): + self.sendRequest(u'idle') + self.sendRequest(u'noidle') + self.assertNoSubscriptions() + self.assertNoEvents() + self.assertInResponse(u'OK') + + def test_idle_then_noidle_enables_timeout(self): + self.sendRequest(u'idle') + self.sendRequest(u'noidle') + self.connection.enable_timeout.assert_called_once_with() + + def test_idle_then_play(self): + with patch.object(self.session, 'stop') as stop_mock: + self.sendRequest(u'idle') + self.sendRequest(u'play') + stop_mock.assert_called_once_with() + + def test_idle_then_idle(self): + with patch.object(self.session, 'stop') as stop_mock: + self.sendRequest(u'idle') + self.sendRequest(u'idle') + stop_mock.assert_called_once_with() + + def test_idle_player_then_play(self): + with patch.object(self.session, 'stop') as stop_mock: + self.sendRequest(u'idle player') + self.sendRequest(u'play') + stop_mock.assert_called_once_with() + + def test_idle_then_player(self): + self.sendRequest(u'idle') + self.idleEvent(u'player') + self.assertNoSubscriptions() + self.assertNoEvents() + self.assertInResponse(u'changed: player') + self.assertInResponse(u'OK') + + def test_idle_player_then_event_player(self): + self.sendRequest(u'idle player') + self.idleEvent(u'player') + self.assertNoSubscriptions() + self.assertNoEvents() + self.assertInResponse(u'changed: player') + self.assertInResponse(u'OK') + + def test_idle_player_then_noidle(self): + self.sendRequest(u'idle player') + self.sendRequest(u'noidle') + self.assertNoSubscriptions() + self.assertNoEvents() + self.assertInResponse(u'OK') + + def test_idle_player_playlist_then_noidle(self): + self.sendRequest(u'idle player playlist') + self.sendRequest(u'noidle') + self.assertNoEvents() + self.assertNoSubscriptions() + self.assertInResponse(u'OK') + + def test_idle_player_playlist_then_player(self): + self.sendRequest(u'idle player playlist') + self.idleEvent(u'player') + self.assertNoEvents() + self.assertNoSubscriptions() + self.assertInResponse(u'changed: player') + self.assertNotInResponse(u'changed: playlist') + self.assertInResponse(u'OK') + + def test_idle_playlist_then_player(self): + self.sendRequest(u'idle playlist') + self.idleEvent(u'player') + self.assertEqualEvents(['player']) + self.assertEqualSubscriptions(['playlist']) + self.assertNoResponse() + + def test_idle_playlist_then_player_then_playlist(self): + self.sendRequest(u'idle playlist') + self.idleEvent(u'player') + self.idleEvent(u'playlist') + self.assertNoEvents() + self.assertNoSubscriptions() + self.assertNotInResponse(u'changed: player') + self.assertInResponse(u'changed: playlist') + self.assertInResponse(u'OK') + + def test_player(self): + self.idleEvent(u'player') + self.assertEqualEvents(['player']) + self.assertNoSubscriptions() + self.assertNoResponse() + + def test_player_then_idle_player(self): + self.idleEvent(u'player') + self.sendRequest(u'idle player') + self.assertNoEvents() + self.assertNoSubscriptions() + self.assertInResponse(u'changed: player') + self.assertNotInResponse(u'changed: playlist') + self.assertInResponse(u'OK') + + def test_player_then_playlist(self): + self.idleEvent(u'player') + self.idleEvent(u'playlist') + self.assertEqualEvents(['player', 'playlist']) + self.assertNoSubscriptions() + self.assertNoResponse() + + def test_player_then_idle(self): + self.idleEvent(u'player') + self.sendRequest(u'idle') + self.assertNoEvents() + self.assertNoSubscriptions() + self.assertInResponse(u'changed: player') + self.assertInResponse(u'OK') + + def test_player_then_playlist_then_idle(self): + self.idleEvent(u'player') + self.idleEvent(u'playlist') + self.sendRequest(u'idle') + self.assertNoEvents() + self.assertNoSubscriptions() + self.assertInResponse(u'changed: player') + self.assertInResponse(u'changed: playlist') + self.assertInResponse(u'OK') + + def test_player_then_idle_playlist(self): + self.idleEvent(u'player') + self.sendRequest(u'idle playlist') + self.assertEqualEvents(['player']) + self.assertEqualSubscriptions(['playlist']) + self.assertNoResponse() + + def test_player_then_idle_playlist_then_noidle(self): + self.idleEvent(u'player') + self.sendRequest(u'idle playlist') + self.sendRequest(u'noidle') + self.assertNoEvents() + self.assertNoSubscriptions() + self.assertInResponse(u'OK') + + def test_player_then_playlist_then_idle_playlist(self): + self.idleEvent(u'player') + self.idleEvent(u'playlist') + self.sendRequest(u'idle playlist') + self.assertNoEvents() + self.assertNoSubscriptions() + self.assertNotInResponse(u'changed: player') + self.assertInResponse(u'changed: playlist') + self.assertInResponse(u'OK') diff --git a/tests/frontends/mpd/protocol/reflection_test.py b/tests/frontends/mpd/protocol/reflection_test.py index 315e3051..9cf1ef97 100644 --- a/tests/frontends/mpd/protocol/reflection_test.py +++ b/tests/frontends/mpd/protocol/reflection_test.py @@ -9,14 +9,14 @@ class ReflectionHandlerTest(protocol.BaseTestCase): self.assertInResponse(u'command: commands') self.assertInResponse(u'command: play') self.assertInResponse(u'command: status') + self.assertInResponse(u'command: idle') + self.assertInResponse(u'command: noidle') # Check if commands you do not have access to are not present self.assertNotInResponse(u'command: kill') # Check if the blacklisted commands are not present self.assertNotInResponse(u'command: command_list_begin') self.assertNotInResponse(u'command: command_list_ok_begin') self.assertNotInResponse(u'command: command_list_end') - self.assertNotInResponse(u'command: idle') - self.assertNotInResponse(u'command: noidle') self.assertNotInResponse(u'command: sticker') self.assertInResponse(u'OK') diff --git a/tests/frontends/mpd/protocol/status_test.py b/tests/frontends/mpd/protocol/status_test.py index 6762a4fb..f50ecd24 100644 --- a/tests/frontends/mpd/protocol/status_test.py +++ b/tests/frontends/mpd/protocol/status_test.py @@ -27,21 +27,6 @@ class StatusHandlerTest(protocol.BaseTestCase): self.sendRequest(u'currentsong') self.assertInResponse(u'OK') - def test_idle_without_subsystems(self): - # FIXME this is not the correct behaviour for idle... - self.sendRequest(u'idle') - self.assertInResponse(u'OK') - - def test_idle_with_subsystems(self): - # FIXME this is not the correct behaviour for idle... - self.sendRequest(u'idle database playlist') - self.assertInResponse(u'OK') - - def test_noidle(self): - # FIXME this is not the correct behaviour for idle... - self.sendRequest(u'noidle') - self.assertInResponse(u'OK') - def test_stats_command(self): self.sendRequest(u'stats') self.assertInResponse(u'OK') From 171137504f3e53de5dc54ae4dfc7e29111bb8285 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 23 Jul 2011 02:30:13 +0200 Subject: [PATCH 10/21] Move subscriptions and events into context object --- mopidy/frontends/mpd/dispatcher.py | 36 ++++++++++++++--------- tests/frontends/mpd/protocol/idle_test.py | 4 +-- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/mopidy/frontends/mpd/dispatcher.py b/mopidy/frontends/mpd/dispatcher.py index a36d38e3..ea2fc1e8 100644 --- a/mopidy/frontends/mpd/dispatcher.py +++ b/mopidy/frontends/mpd/dispatcher.py @@ -36,8 +36,6 @@ class MpdDispatcher(object): self.command_list = False self.command_list_ok = False self.command_list_index = None - self.subscriptions = set() - self.events = set() self.context = MpdContext(self, session=session) def handle_request(self, request, current_command_list_index=None): @@ -55,9 +53,10 @@ class MpdDispatcher(object): return self._call_next_filter(request, response, filter_chain) def handle_idle(self, subsystem): - self.events.add(subsystem) + self.context.events.add(subsystem) - subsystems = self.subscriptions.intersection(self.events) + subsystems = self.context.subscriptions.intersection( + self.context.events) if not subsystems: return @@ -65,8 +64,8 @@ class MpdDispatcher(object): for subsystem in subsystems: response.append(u'changed: %s' % subsystem) response.append(u'OK') - self.subscriptions = set() - self.events = set() + self.context.subscriptions = set() + self.context.events = set() self.context.session.send_lines(response) def _call_next_filter(self, request, response, filter_chain): @@ -134,27 +133,28 @@ class MpdDispatcher(object): def _idle_filter(self, request, response, filter_chain): if re.match(r'^noidle$', request): - if not self.subscriptions: + if not self.context.subscriptions: return [] - self.subscriptions = set() - self.events = set() + self.context.subscriptions = set() + self.context.events = set() self.context.session.connection.enable_timeout() return [u'OK'] - if self.subscriptions: + if self.context.subscriptions: self.context.session.close() return [] if re.match(r'^idle( .+)?$', request): for subsystem in self._extract_subsystems(request): - self.subscriptions.add(subsystem) + self.context.subscriptions.add(subsystem) - subsystems = self.subscriptions.intersection(self.events) + subsystems = self.context.subscriptions.intersection( + self.context.events) if subsystems: for subsystem in subsystems: response.append(u'changed: %s' % subsystem) - self.events = set() - self.subscriptions = set() + self.context.events = set() + self.context.subscriptions = set() response.append(u'OK') return response else: @@ -246,9 +246,17 @@ class MpdContext(object): #: The current :class:`mopidy.frontends.mpd.MpdSession`. session = None + #: The active subsystems that have pending events. + events = None + + #: The subsytems that we want to be notified about in idle mode. + subscriptions = None + def __init__(self, dispatcher, session=None): self.dispatcher = dispatcher self.session = session + self.events = set() + self.subscriptions = set() self._backend = None self._mixer = None diff --git a/tests/frontends/mpd/protocol/idle_test.py b/tests/frontends/mpd/protocol/idle_test.py index 319789bf..3efb0cb1 100644 --- a/tests/frontends/mpd/protocol/idle_test.py +++ b/tests/frontends/mpd/protocol/idle_test.py @@ -10,10 +10,10 @@ class IdleHandlerTest(protocol.BaseTestCase): self.session.on_idle(subsystem) def assertEqualEvents(self, events): - self.assertEqual(set(events), self.dispatcher.events) + self.assertEqual(set(events), self.context.events) def assertEqualSubscriptions(self, events): - self.assertEqual(set(events), self.dispatcher.subscriptions) + self.assertEqual(set(events), self.context.subscriptions) def assertNoEvents(self): self.assertEqualEvents([]) From e4ce31a438d5adbb13752c09fd8be5bd38776b37 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 23 Jul 2011 03:28:29 +0200 Subject: [PATCH 11/21] Ensure that empty command does not get added to command list --- mopidy/frontends/mpd/protocol/empty.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mopidy/frontends/mpd/protocol/empty.py b/mopidy/frontends/mpd/protocol/empty.py index 33b3bd9f..4cdafd87 100644 --- a/mopidy/frontends/mpd/protocol/empty.py +++ b/mopidy/frontends/mpd/protocol/empty.py @@ -1,6 +1,6 @@ from mopidy.frontends.mpd.protocol import handle_request -@handle_request(r'^\s*$') +@handle_request(r'^[ ]*$') def empty(context): """The original MPD server returns ``OK`` on an empty request.""" pass From 63dba5553fbc74a26420fa3c5c5557469672c3fa Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 23 Jul 2011 03:32:45 +0200 Subject: [PATCH 12/21] Move idle code from dispatcher to protocol.status module --- mopidy/frontends/mpd/dispatcher.py | 55 ++++++--------------- mopidy/frontends/mpd/protocol/reflection.py | 4 -- mopidy/frontends/mpd/protocol/status.py | 40 +++++++++++++-- tests/frontends/mpd/protocol/idle_test.py | 2 +- 4 files changed, 52 insertions(+), 49 deletions(-) diff --git a/mopidy/frontends/mpd/dispatcher.py b/mopidy/frontends/mpd/dispatcher.py index ea2fc1e8..660f82ec 100644 --- a/mopidy/frontends/mpd/dispatcher.py +++ b/mopidy/frontends/mpd/dispatcher.py @@ -20,10 +20,6 @@ from mopidy.utils import flatten logger = logging.getLogger('mopidy.frontends.mpd.dispatcher') -#: Subsystems that can be registered with idle command. -SUBSYSTEMS = ['database', 'mixer', 'options', 'output', - 'player', 'playlist', 'stored_playlist', 'update', ] - class MpdDispatcher(object): """ The MPD session feeds the MPD dispatcher with requests. The dispatcher @@ -132,59 +128,40 @@ class MpdDispatcher(object): ### Filter: idle def _idle_filter(self, request, response, filter_chain): - if re.match(r'^noidle$', request): - if not self.context.subscriptions: - return [] - self.context.subscriptions = set() - self.context.events = set() - self.context.session.connection.enable_timeout() - return [u'OK'] - - if self.context.subscriptions: + if self._is_currently_idle() and not self._is_noidle(request): + logger.debug(u'Client send us %s, only %s is allowed while in ' + 'the idle state', repr(request), repr('noidle')) self.context.session.close() return [] - if re.match(r'^idle( .+)?$', request): - for subsystem in self._extract_subsystems(request): - self.context.subscriptions.add(subsystem) + if not self._is_currently_idle() and self._is_noidle(request): + return [] - subsystems = self.context.subscriptions.intersection( - self.context.events) - if subsystems: - for subsystem in subsystems: - response.append(u'changed: %s' % subsystem) - self.context.events = set() - self.context.subscriptions = set() - response.append(u'OK') - return response - else: - self.context.session.connection.disable_timeout() - return [] + response = self._call_next_filter(request, response, filter_chain) - return self._call_next_filter(request, response, filter_chain) + if self._is_currently_idle(): + return [] + else: + return response - def _extract_subsystems(self, request): - match = re.match(r'^idle (?P.+)$', request) - if not match: - return SUBSYSTEMS - return match.groupdict()['subsystems'].split(' ') + def _is_currently_idle(self): + return bool(self.context.subscriptions) + + def _is_noidle(self, request): + return re.match(r'^noidle$', request) ### Filter: add OK def _add_ok_filter(self, request, response, filter_chain): response = self._call_next_filter(request, response, filter_chain) - if not self._has_error(response) and not self._is_idle(request): + if not self._has_error(response): response.append(u'OK') return response def _has_error(self, response): return response and response[-1].startswith(u'ACK') - def _is_idle(self, request): - return request.startswith('idle') - - ### Filter: call handler def _call_handler_filter(self, request, response, filter_chain): diff --git a/mopidy/frontends/mpd/protocol/reflection.py b/mopidy/frontends/mpd/protocol/reflection.py index 3618f5e1..dbd76034 100644 --- a/mopidy/frontends/mpd/protocol/reflection.py +++ b/mopidy/frontends/mpd/protocol/reflection.py @@ -27,10 +27,6 @@ def commands(context): command_names.remove('command_list_ok_begin') if 'command_list_end' in command_names: command_names.remove('command_list_end') - if 'idle' in command_names: - command_names.remove('idle') - if 'noidle' in command_names: - command_names.remove('noidle') if 'sticker' in command_names: command_names.remove('sticker') diff --git a/mopidy/frontends/mpd/protocol/status.py b/mopidy/frontends/mpd/protocol/status.py index 4a961e76..444ec0c2 100644 --- a/mopidy/frontends/mpd/protocol/status.py +++ b/mopidy/frontends/mpd/protocol/status.py @@ -4,6 +4,10 @@ from mopidy.backends.base import PlaybackController from mopidy.frontends.mpd.protocol import handle_request from mopidy.frontends.mpd.exceptions import MpdNotImplemented +#: Subsystems that can be registered with idle command. +SUBSYSTEMS = ['database', 'mixer', 'options', 'output', + 'player', 'playlist', 'stored_playlist', 'update', ] + @handle_request(r'^clearerror$') def clearerror(context): """ @@ -32,8 +36,8 @@ def currentsong(context): position=context.backend.playback.current_playlist_position.get(), cpid=current_cp_track.cpid) -#@handle_request(r'^idle$') -#@handle_request(r'^idle (?P.+)$') +@handle_request(r'^idle$') +@handle_request(r'^idle (?P.+)$') def idle(context, subsystems=None): """ *musicpd.org, status section:* @@ -67,12 +71,38 @@ def idle(context, subsystems=None): notifications when something changed in one of the specified subsystems. """ - pass # TODO -#@handle_request(r'^noidle$') + if subsystems: + subsystems = subsystems.split() + else: + subsystems = SUBSYSTEMS + + for subsystem in subsystems: + context.subscriptions.add(subsystem) + + active = context.subscriptions.intersection(context.events) + if not active: + context.session.connection.disable_timeout() + return + + response = [] + context.events = set() + context.subscriptions = set() + + for subsystem in active: + response.append(u'changed: %s' % subsystem) + response.append(u'OK') + + return response + +@handle_request(r'^noidle$') def noidle(context): """See :meth:`_status_idle`.""" - pass # TODO + if not context.subscriptions: + return + context.subscriptions = set() + context.events = set() + context.session.connection.enable_timeout() @handle_request(r'^stats$') def stats(context): diff --git a/tests/frontends/mpd/protocol/idle_test.py b/tests/frontends/mpd/protocol/idle_test.py index 3efb0cb1..0f56cd61 100644 --- a/tests/frontends/mpd/protocol/idle_test.py +++ b/tests/frontends/mpd/protocol/idle_test.py @@ -1,6 +1,6 @@ from mock import patch -from mopidy.frontends.mpd.dispatcher import SUBSYSTEMS +from mopidy.frontends.mpd.protocol.status import SUBSYSTEMS from mopidy.models import Track from tests.frontends.mpd import protocol From 0e58d771cdf332edef1f4a93fb131cd5f4ba434b Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 23 Jul 2011 03:58:26 +0200 Subject: [PATCH 13/21] Make tests check that response only has values once, fixes double OK bug --- mopidy/frontends/mpd/protocol/status.py | 2 -- tests/frontends/mpd/protocol/__init__.py | 5 +++ tests/frontends/mpd/protocol/idle_test.py | 42 +++++++++++------------ 3 files changed, 26 insertions(+), 23 deletions(-) diff --git a/mopidy/frontends/mpd/protocol/status.py b/mopidy/frontends/mpd/protocol/status.py index 444ec0c2..5a319a5d 100644 --- a/mopidy/frontends/mpd/protocol/status.py +++ b/mopidy/frontends/mpd/protocol/status.py @@ -91,8 +91,6 @@ def idle(context, subsystems=None): for subsystem in active: response.append(u'changed: %s' % subsystem) - response.append(u'OK') - return response @handle_request(r'^noidle$') diff --git a/tests/frontends/mpd/protocol/__init__.py b/tests/frontends/mpd/protocol/__init__.py index 68668bc2..00314946 100644 --- a/tests/frontends/mpd/protocol/__init__.py +++ b/tests/frontends/mpd/protocol/__init__.py @@ -46,6 +46,11 @@ class BaseTestCase(unittest.TestCase): self.assert_(value in self.connection.response, u'Did not find %s ' 'in %s' % (repr(value), repr(self.connection.response))) + def assertOnceInResponse(self, value): + matched = len([r for r in self.connection.response if r == value]) + self.assertEqual(1, matched, 'Expected to find %s once in %s' % + (repr(value), repr(self.connection.response))) + def assertNotInResponse(self, value): self.assert_(value not in self.connection.response, u'Found %s in %s' % (repr(value), repr(self.connection.response))) diff --git a/tests/frontends/mpd/protocol/idle_test.py b/tests/frontends/mpd/protocol/idle_test.py index 0f56cd61..0f5d4b06 100644 --- a/tests/frontends/mpd/protocol/idle_test.py +++ b/tests/frontends/mpd/protocol/idle_test.py @@ -63,7 +63,7 @@ class IdleHandlerTest(protocol.BaseTestCase): self.sendRequest(u'noidle') self.assertNoSubscriptions() self.assertNoEvents() - self.assertInResponse(u'OK') + self.assertOnceInResponse(u'OK') def test_idle_then_noidle_enables_timeout(self): self.sendRequest(u'idle') @@ -93,39 +93,39 @@ class IdleHandlerTest(protocol.BaseTestCase): self.idleEvent(u'player') self.assertNoSubscriptions() self.assertNoEvents() - self.assertInResponse(u'changed: player') - self.assertInResponse(u'OK') + self.assertOnceInResponse(u'changed: player') + self.assertOnceInResponse(u'OK') def test_idle_player_then_event_player(self): self.sendRequest(u'idle player') self.idleEvent(u'player') self.assertNoSubscriptions() self.assertNoEvents() - self.assertInResponse(u'changed: player') - self.assertInResponse(u'OK') + self.assertOnceInResponse(u'changed: player') + self.assertOnceInResponse(u'OK') def test_idle_player_then_noidle(self): self.sendRequest(u'idle player') self.sendRequest(u'noidle') self.assertNoSubscriptions() self.assertNoEvents() - self.assertInResponse(u'OK') + self.assertOnceInResponse(u'OK') def test_idle_player_playlist_then_noidle(self): self.sendRequest(u'idle player playlist') self.sendRequest(u'noidle') self.assertNoEvents() self.assertNoSubscriptions() - self.assertInResponse(u'OK') + self.assertOnceInResponse(u'OK') def test_idle_player_playlist_then_player(self): self.sendRequest(u'idle player playlist') self.idleEvent(u'player') self.assertNoEvents() self.assertNoSubscriptions() - self.assertInResponse(u'changed: player') + self.assertOnceInResponse(u'changed: player') self.assertNotInResponse(u'changed: playlist') - self.assertInResponse(u'OK') + self.assertOnceInResponse(u'OK') def test_idle_playlist_then_player(self): self.sendRequest(u'idle playlist') @@ -141,8 +141,8 @@ class IdleHandlerTest(protocol.BaseTestCase): self.assertNoEvents() self.assertNoSubscriptions() self.assertNotInResponse(u'changed: player') - self.assertInResponse(u'changed: playlist') - self.assertInResponse(u'OK') + self.assertOnceInResponse(u'changed: playlist') + self.assertOnceInResponse(u'OK') def test_player(self): self.idleEvent(u'player') @@ -155,9 +155,9 @@ class IdleHandlerTest(protocol.BaseTestCase): self.sendRequest(u'idle player') self.assertNoEvents() self.assertNoSubscriptions() - self.assertInResponse(u'changed: player') + self.assertOnceInResponse(u'changed: player') self.assertNotInResponse(u'changed: playlist') - self.assertInResponse(u'OK') + self.assertOnceInResponse(u'OK') def test_player_then_playlist(self): self.idleEvent(u'player') @@ -171,8 +171,8 @@ class IdleHandlerTest(protocol.BaseTestCase): self.sendRequest(u'idle') self.assertNoEvents() self.assertNoSubscriptions() - self.assertInResponse(u'changed: player') - self.assertInResponse(u'OK') + self.assertOnceInResponse(u'changed: player') + self.assertOnceInResponse(u'OK') def test_player_then_playlist_then_idle(self): self.idleEvent(u'player') @@ -180,9 +180,9 @@ class IdleHandlerTest(protocol.BaseTestCase): self.sendRequest(u'idle') self.assertNoEvents() self.assertNoSubscriptions() - self.assertInResponse(u'changed: player') - self.assertInResponse(u'changed: playlist') - self.assertInResponse(u'OK') + self.assertOnceInResponse(u'changed: player') + self.assertOnceInResponse(u'changed: playlist') + self.assertOnceInResponse(u'OK') def test_player_then_idle_playlist(self): self.idleEvent(u'player') @@ -197,7 +197,7 @@ class IdleHandlerTest(protocol.BaseTestCase): self.sendRequest(u'noidle') self.assertNoEvents() self.assertNoSubscriptions() - self.assertInResponse(u'OK') + self.assertOnceInResponse(u'OK') def test_player_then_playlist_then_idle_playlist(self): self.idleEvent(u'player') @@ -206,5 +206,5 @@ class IdleHandlerTest(protocol.BaseTestCase): self.assertNoEvents() self.assertNoSubscriptions() self.assertNotInResponse(u'changed: player') - self.assertInResponse(u'changed: playlist') - self.assertInResponse(u'OK') + self.assertOnceInResponse(u'changed: playlist') + self.assertOnceInResponse(u'OK') From b33e66b0c807e532a20638415c05aaa322fd66c6 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 23 Jul 2011 14:47:35 +0200 Subject: [PATCH 14/21] Add comment to idle filter --- mopidy/frontends/mpd/dispatcher.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mopidy/frontends/mpd/dispatcher.py b/mopidy/frontends/mpd/dispatcher.py index 660f82ec..a638d6e9 100644 --- a/mopidy/frontends/mpd/dispatcher.py +++ b/mopidy/frontends/mpd/dispatcher.py @@ -135,7 +135,7 @@ class MpdDispatcher(object): return [] if not self._is_currently_idle() and self._is_noidle(request): - return [] + return [] # noidle was called before idle response = self._call_next_filter(request, response, filter_chain) From 9895f5197cfb1d910a130a2a4d692ea2a379919a Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 23 Jul 2011 14:48:52 +0200 Subject: [PATCH 15/21] Test via on_receive instead of on_line_received to ensure timeout code is also tested --- tests/frontends/mpd/protocol/__init__.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/frontends/mpd/protocol/__init__.py b/tests/frontends/mpd/protocol/__init__.py index 00314946..8cd91d60 100644 --- a/tests/frontends/mpd/protocol/__init__.py +++ b/tests/frontends/mpd/protocol/__init__.py @@ -34,9 +34,10 @@ class BaseTestCase(unittest.TestCase): self.mixer.stop().get() settings.runtime.clear() - def sendRequest(self, request, clear=False): + def sendRequest(self, request): self.connection.response = [] - self.session.on_line_received(request) + request = '%s\n' % request.encode('utf-8') + self.session.on_receive({'received': request}) return self.connection.response def assertNoResponse(self): From 451b52fde543e2edd0a05b3dd569e67aab66be99 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sun, 24 Jul 2011 01:59:32 +0200 Subject: [PATCH 16/21] Make sure we prevent timeouts when in idle mode --- mopidy/frontends/mpd/protocol/status.py | 4 ++-- mopidy/utils/network.py | 4 +++- tests/frontends/mpd/protocol/idle_test.py | 4 ---- tests/utils/network/lineprotocol_test.py | 12 ++++++++++++ 4 files changed, 17 insertions(+), 7 deletions(-) diff --git a/mopidy/frontends/mpd/protocol/status.py b/mopidy/frontends/mpd/protocol/status.py index 5a319a5d..5ac99dfe 100644 --- a/mopidy/frontends/mpd/protocol/status.py +++ b/mopidy/frontends/mpd/protocol/status.py @@ -82,7 +82,7 @@ def idle(context, subsystems=None): active = context.subscriptions.intersection(context.events) if not active: - context.session.connection.disable_timeout() + context.session.prevent_timeout = True return response = [] @@ -100,7 +100,7 @@ def noidle(context): return context.subscriptions = set() context.events = set() - context.session.connection.enable_timeout() + context.session.prevent_timeout = False @handle_request(r'^stats$') def stats(context): diff --git a/mopidy/utils/network.py b/mopidy/utils/network.py index b7cc144d..719def69 100644 --- a/mopidy/utils/network.py +++ b/mopidy/utils/network.py @@ -286,6 +286,7 @@ class LineProtocol(ThreadingActor): def __init__(self, connection): self.connection = connection + self.prevent_timeout = False self.recv_buffer = '' @property @@ -316,7 +317,8 @@ class LineProtocol(ThreadingActor): line = self.decode(line) self.on_line_received(line) - self.connection.enable_timeout() + if not self.prevent_timeout: + self.connection.enable_timeout() def on_stop(self): """Ensure that cleanup when actor stops.""" diff --git a/tests/frontends/mpd/protocol/idle_test.py b/tests/frontends/mpd/protocol/idle_test.py index 0f5d4b06..da16bf33 100644 --- a/tests/frontends/mpd/protocol/idle_test.py +++ b/tests/frontends/mpd/protocol/idle_test.py @@ -42,10 +42,6 @@ class IdleHandlerTest(protocol.BaseTestCase): self.assertNoEvents() self.assertNoResponse() - def test_noidle_does_not_call_enable_timeout(self): - self.sendRequest(u'noidle') - self.assertEqual(0, self.connection.enable_timeout.call_count) - def test_idle_player(self): self.sendRequest(u'idle player') self.assertEqualSubscriptions(['player']) diff --git a/tests/utils/network/lineprotocol_test.py b/tests/utils/network/lineprotocol_test.py index a87f461c..41d3fbf2 100644 --- a/tests/utils/network/lineprotocol_test.py +++ b/tests/utils/network/lineprotocol_test.py @@ -11,11 +11,13 @@ class LineProtocolTest(unittest.TestCase): self.mock = Mock(spec=network.LineProtocol) self.mock.terminator = network.LineProtocol.terminator self.mock.encoding = network.LineProtocol.encoding + self.mock.prevent_timeout = False def test_init_stores_values_in_attributes(self): network.LineProtocol.__init__(self.mock, sentinel.connection) self.assertEqual(sentinel.connection, self.mock.connection) self.assertEqual('', self.mock.recv_buffer) + self.assertFalse(self.mock.prevent_timeout) def test_on_receive_no_new_lines_adds_to_recv_buffer(self): self.mock.connection = Mock(spec=network.Connection) @@ -36,6 +38,16 @@ class LineProtocolTest(unittest.TestCase): self.mock.connection.disable_timeout.assert_called_once_with() self.mock.connection.enable_timeout.assert_called_once_with() + def test_on_receive_toggles_unless_prevent_timeout_is_set(self): + self.mock.connection = Mock(spec=network.Connection) + self.mock.recv_buffer = '' + self.mock.parse_lines.return_value = [] + self.mock.prevent_timeout = True + + network.LineProtocol.on_receive(self.mock, {'received': 'data'}) + self.mock.connection.disable_timeout.assert_called_once_with() + self.assertEqual(0, self.mock.connection.enable_timeout.call_count) + def test_on_receive_no_new_lines_calls_parse_lines(self): self.mock.connection = Mock(spec=network.Connection) self.mock.recv_buffer = '' From 6bd39187061432250a9fc78b11492e0fee1c9ef4 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sun, 24 Jul 2011 12:03:30 +0200 Subject: [PATCH 17/21] Add idle addition to changelog --- docs/changes.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/changes.rst b/docs/changes.rst index 0a9ab925..19c65ee9 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -25,6 +25,9 @@ v0.6.0 (in development) - The local client now tries to lookup where your music is via XDG, it will fall-back to ``~/music`` or use whatever setting you set manually. +- The idle command is now supported by mopidy for the following subsystems: + player, playlist, options and mixer (Fixes: :issue:`32`). + **Changes** - Replace :attr:`mopidy.backends.base.Backend.uri_handlers` with From 2c0f2ab82cea30434b3793a918d96ce340c02b89 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sun, 24 Jul 2011 19:48:41 +0200 Subject: [PATCH 18/21] Typo fix --- mopidy/frontends/mpd/dispatcher.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mopidy/frontends/mpd/dispatcher.py b/mopidy/frontends/mpd/dispatcher.py index a638d6e9..b502ee5c 100644 --- a/mopidy/frontends/mpd/dispatcher.py +++ b/mopidy/frontends/mpd/dispatcher.py @@ -129,8 +129,8 @@ class MpdDispatcher(object): def _idle_filter(self, request, response, filter_chain): if self._is_currently_idle() and not self._is_noidle(request): - logger.debug(u'Client send us %s, only %s is allowed while in ' - 'the idle state', repr(request), repr('noidle')) + logger.debug(u'Client sent us %s, only %s is allowed while in ' + 'the idle state', repr(request), repr(u'noidle')) self.context.session.close() return [] From 5d025a5721216c4f9ffb7c99665374e4a332b258 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Mon, 25 Jul 2011 00:30:21 +0200 Subject: [PATCH 19/21] Compile noidle regexp used for is_noidle check --- mopidy/frontends/mpd/dispatcher.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/mopidy/frontends/mpd/dispatcher.py b/mopidy/frontends/mpd/dispatcher.py index b502ee5c..cab014a8 100644 --- a/mopidy/frontends/mpd/dispatcher.py +++ b/mopidy/frontends/mpd/dispatcher.py @@ -27,6 +27,8 @@ class MpdDispatcher(object): back to the MPD session. """ + _noidle = re.compile(r'^noidle$') + def __init__(self, session=None): self.authenticated = False self.command_list = False @@ -128,13 +130,13 @@ class MpdDispatcher(object): ### Filter: idle def _idle_filter(self, request, response, filter_chain): - if self._is_currently_idle() and not self._is_noidle(request): + if self._is_currently_idle() and not self._noidle.match(request): logger.debug(u'Client sent us %s, only %s is allowed while in ' 'the idle state', repr(request), repr(u'noidle')) self.context.session.close() return [] - if not self._is_currently_idle() and self._is_noidle(request): + if not self._is_currently_idle() and self._noidle.match(request): return [] # noidle was called before idle response = self._call_next_filter(request, response, filter_chain) @@ -147,9 +149,6 @@ class MpdDispatcher(object): def _is_currently_idle(self): return bool(self.context.subscriptions) - def _is_noidle(self, request): - return re.match(r'^noidle$', request) - ### Filter: add OK From c724fcd7c9e12192e3c3f5afccadab6aede88f02 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Mon, 25 Jul 2011 01:00:18 +0200 Subject: [PATCH 20/21] Turns out idle and noidle are not commands that should be listed --- mopidy/frontends/mpd/protocol/reflection.py | 4 ++++ tests/frontends/mpd/protocol/reflection_test.py | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/mopidy/frontends/mpd/protocol/reflection.py b/mopidy/frontends/mpd/protocol/reflection.py index dbd76034..3618f5e1 100644 --- a/mopidy/frontends/mpd/protocol/reflection.py +++ b/mopidy/frontends/mpd/protocol/reflection.py @@ -27,6 +27,10 @@ def commands(context): command_names.remove('command_list_ok_begin') if 'command_list_end' in command_names: command_names.remove('command_list_end') + if 'idle' in command_names: + command_names.remove('idle') + if 'noidle' in command_names: + command_names.remove('noidle') if 'sticker' in command_names: command_names.remove('sticker') diff --git a/tests/frontends/mpd/protocol/reflection_test.py b/tests/frontends/mpd/protocol/reflection_test.py index 9cf1ef97..315e3051 100644 --- a/tests/frontends/mpd/protocol/reflection_test.py +++ b/tests/frontends/mpd/protocol/reflection_test.py @@ -9,14 +9,14 @@ class ReflectionHandlerTest(protocol.BaseTestCase): self.assertInResponse(u'command: commands') self.assertInResponse(u'command: play') self.assertInResponse(u'command: status') - self.assertInResponse(u'command: idle') - self.assertInResponse(u'command: noidle') # Check if commands you do not have access to are not present self.assertNotInResponse(u'command: kill') # Check if the blacklisted commands are not present self.assertNotInResponse(u'command: command_list_begin') self.assertNotInResponse(u'command: command_list_ok_begin') self.assertNotInResponse(u'command: command_list_end') + self.assertNotInResponse(u'command: idle') + self.assertNotInResponse(u'command: noidle') self.assertNotInResponse(u'command: sticker') self.assertInResponse(u'OK') From 01cfbead30b57803dd8dc58c9ec96bec13ba741c Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Mon, 25 Jul 2011 01:03:58 +0200 Subject: [PATCH 21/21] Use sets to compute output --- mopidy/frontends/mpd/protocol/reflection.py | 28 ++++++--------------- 1 file changed, 8 insertions(+), 20 deletions(-) diff --git a/mopidy/frontends/mpd/protocol/reflection.py b/mopidy/frontends/mpd/protocol/reflection.py index 3618f5e1..df13b4b4 100644 --- a/mopidy/frontends/mpd/protocol/reflection.py +++ b/mopidy/frontends/mpd/protocol/reflection.py @@ -11,28 +11,16 @@ def commands(context): Shows which commands the current user has access to. """ if context.dispatcher.authenticated: - command_names = [command.name for command in mpd_commands] + command_names = set([command.name for command in mpd_commands]) else: - command_names = [command.name for command in mpd_commands - if not command.auth_required] + command_names = set([command.name for command in mpd_commands + if not command.auth_required]) - # No permission to use - if 'kill' in command_names: - command_names.remove('kill') - - # Not shown by MPD in its command list - if 'command_list_begin' in command_names: - command_names.remove('command_list_begin') - if 'command_list_ok_begin' in command_names: - command_names.remove('command_list_ok_begin') - if 'command_list_end' in command_names: - command_names.remove('command_list_end') - if 'idle' in command_names: - command_names.remove('idle') - if 'noidle' in command_names: - command_names.remove('noidle') - if 'sticker' in command_names: - command_names.remove('sticker') + # No one is permited to use kill, rest of commands are not listed by MPD, + # so we shouldn't either. + command_names = command_names - set(['kill', 'command_list_begin', + 'command_list_ok_begin', 'command_list_ok_begin', 'command_list_end', + 'idle', 'noidle', 'sticker']) return [('command', command_name) for command_name in sorted(command_names)]