Merge pull request #1347 from adamcik/feature/mpd-idle-cleanup

MPD idle cleanup

docs/changelog.rst

@@ -56,6 +56,18 @@ MPD frontend
 
 - Start ``songid`` counting at 1 instead of 0 to match the original MPD server.
 
+- Idle events are now emitted on ``seekeded`` events. This fix means that
+  clients relying on ``idle`` events now get notified about seeks.
+  (Fixes: :issue:`1331` :issue:`1347`)
+
+- Idle events are now emitted on ``playlists_loaded`` events. This fix means
+  that clients relying on ``idle`` events now get notified about playlist loads.
+  (Fixes: :issue:`1331` PR: :issue:`1347`)
+
+- Event handler for ``playlist_deleted`` has been unbroken. This unreported bug
+  would cause the MPD Frontend to crash preventing any further communication
+  via the MPD protocol. (PR: :issue:`1347`)
+
 Zeroconf
 --------
 
@@ -75,6 +87,9 @@ Cleanups
 - Removed warning if :file:`~/.config/mopidy/settings.py` exists. We stopped
   using this settings file in 0.14, released in April 2013.
 
+- The ``on_event`` handler in our listener helper now catches exceptions. This
+  means that any errors in event handling won't crash the actor in question.
+
 Gapless
 -------
 

mopidy/core/listener.py

@@ -31,7 +31,8 @@ class CoreListener(listener.Listener):
         :type event: string
         :param kwargs: any other arguments to the specific event handlers
         """
-        getattr(self, event)(**kwargs)
+        # Just delegate to parent, entry mostly for docs.
+        super(CoreListener, self).on_event(event, **kwargs)
 
     def track_playback_paused(self, tl_track, time_position):
         """

mopidy/listener.py

@@ -41,4 +41,9 @@ class Listener(object):
         :type event: string
         :param kwargs: any other arguments to the specific event handlers
         """
-        getattr(self, event)(**kwargs)
+        try:
+            getattr(self, event)(**kwargs)
+        except Exception:
+            # Ensure we don't crash the actor due to "bad" events.
+            logger.exception(
+                'Triggering event failed: %s(%s)', event, ', '.join(kwargs))

mopidy/mpd/actor.py

@@ -4,13 +4,30 @@ import logging
 
 import pykka
 
-from mopidy import exceptions, zeroconf
+from mopidy import exceptions, listener, zeroconf
 from mopidy.core import CoreListener
 from mopidy.internal import encoding, network, process
 from mopidy.mpd import session, uri_mapper
 
 logger = logging.getLogger(__name__)
 
+_CORE_EVENTS_TO_IDLE_SUBSYSTEMS = {
+    'track_playback_paused': None,
+    'track_playback_resumed': None,
+    'track_playback_started': None,
+    'track_playback_ended': None,
+    'playback_state_changed': 'player',
+    'tracklist_changed': 'playlist',
+    'playlists_loaded': 'stored_playlist',
+    'playlist_changed': 'stored_playlist',
+    'playlist_deleted': 'stored_playlist',
+    'options_changed': 'options',
+    'volume_changed': 'mixer',
+    'mute_changed': 'output',
+    'seeked': 'player',
+    'stream_title_changed': 'playlist',
+}
+
 
 class MpdFrontend(pykka.ThreadingActor, CoreListener):
 
@@ -24,6 +41,9 @@ class MpdFrontend(pykka.ThreadingActor, CoreListener):
         self.zeroconf_name = config['mpd']['zeroconf']
         self.zeroconf_service = None
 
+        self._setup_server(config, core)
+
+    def _setup_server(self, config, core):
         try:
             network.Server(
                 self.hostname, self.port,
@@ -56,31 +76,13 @@ class MpdFrontend(pykka.ThreadingActor, CoreListener):
 
         process.stop_actors_by_class(session.MpdSession)
 
+    def on_event(self, event, **kwargs):
+        if event not in _CORE_EVENTS_TO_IDLE_SUBSYSTEMS:
+            logger.warning(
+                'Got unexpected event: %s(%s)', event, ', '.join(kwargs))
+        else:
+            self.send_idle(_CORE_EVENTS_TO_IDLE_SUBSYSTEMS[event])
+
     def send_idle(self, subsystem):
-        listeners = pykka.ActorRegistry.get_by_class(session.MpdSession)
-        for listener in listeners:
-            getattr(listener.proxy(), 'on_idle')(subsystem)
-
-    def playback_state_changed(self, old_state, new_state):
-        self.send_idle('player')
-
-    def tracklist_changed(self):
-        self.send_idle('playlist')
-
-    def playlist_changed(self, playlist):
-        self.send_idle('stored_playlist')
-
-    def playlist_deleted(self, playlist):
-        self.send_idle('stored_playlist')
-
-    def options_changed(self):
-        self.send_idle('options')
-
-    def volume_changed(self, volume):
-        self.send_idle('mixer')
-
-    def mute_changed(self, mute):
-        self.send_idle('output')
-
-    def stream_title_changed(self, title):
-        self.send_idle('playlist')
+        if subsystem:
+            listener.send(session.MpdSession, subsystem)

mopidy/mpd/dispatcher.py

@@ -47,6 +47,7 @@ class MpdDispatcher(object):
         return self._call_next_filter(request, response, filter_chain)
 
     def handle_idle(self, subsystem):
+        # TODO: validate against mopidy/mpd/protocol/status.SUBSYSTEMS
         self.context.events.add(subsystem)
 
         subsystems = self.context.subscriptions.intersection(

mopidy/mpd/session.py

@@ -41,7 +41,7 @@ class MpdSession(network.LineProtocol):
 
         self.send_lines(response)
 
-    def on_idle(self, subsystem):
+    def on_event(self, subsystem):
         self.dispatcher.handle_idle(subsystem)
 
     def decode(self, line):

tests/mpd/protocol/test_idle.py

@@ -10,7 +10,7 @@ from tests.mpd import protocol
 class IdleHandlerTest(protocol.BaseTestCase):
 
     def idle_event(self, subsystem):
-        self.session.on_idle(subsystem)
+        self.session.on_event(subsystem)
 
     def assertEqualEvents(self, events):  # noqa: N802
         self.assertEqual(set(events), self.context.events)

tests/mpd/test_actor.py

@@ -0,0 +1,44 @@
+from __future__ import absolute_import, unicode_literals
+
+import mock
+
+import pytest
+
+from mopidy.mpd import actor
+
+# NOTE: Should be kept in sync with all events from mopidy.core.listener
+
+
+@pytest.mark.parametrize("event,expected", [
+    (['track_playback_paused', 'tl_track', 'time_position'], None),
+    (['track_playback_resumed', 'tl_track', 'time_position'], None),
+    (['track_playback_started', 'tl_track'], None),
+    (['track_playback_ended', 'tl_track', 'time_position'], None),
+    (['playback_state_changed', 'old_state', 'new_state'], 'player'),
+    (['tracklist_changed'], 'playlist'),
+    (['playlists_loaded'], 'stored_playlist'),
+    (['playlist_changed', 'playlist'], 'stored_playlist'),
+    (['playlist_deleted', 'uri'], 'stored_playlist'),
+    (['options_changed'], 'options'),
+    (['volume_changed', 'volume'], 'mixer'),
+    (['mute_changed', 'mute'], 'output'),
+    (['seeked', 'time_position'], 'player'),
+    (['stream_title_changed', 'title'], 'playlist'),
+])
+def test_idle_hooked_up_correctly(event, expected):
+    config = {'mpd': {'hostname': 'foobar',
+                      'port': 1234,
+                      'zeroconf': None,
+                      'max_connections': None,
+                      'connection_timeout': None}}
+
+    with mock.patch.object(actor.MpdFrontend, '_setup_server'):
+        frontend = actor.MpdFrontend(core=mock.Mock(), config=config)
+
+    with mock.patch('mopidy.listener.send') as send_mock:
+        frontend.on_event(event[0], **{e: None for e in event[1:]})
+
+    if expected is None:
+        assert not send_mock.call_args
+    else:
+        send_mock.assert_called_once_with(mock.ANY, expected)