diff --git a/docs/changelog.rst b/docs/changelog.rst index 8cc886ce..4c304be1 100644 --- a/docs/changelog.rst +++ b/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 ------- diff --git a/mopidy/core/listener.py b/mopidy/core/listener.py index 8feb0324..b8ef734d 100644 --- a/mopidy/core/listener.py +++ b/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): """ diff --git a/mopidy/listener.py b/mopidy/listener.py index 9bcab0e0..79c53570 100644 --- a/mopidy/listener.py +++ b/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)) diff --git a/mopidy/mpd/actor.py b/mopidy/mpd/actor.py index 7439e73f..067d20c5 100644 --- a/mopidy/mpd/actor.py +++ b/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) diff --git a/mopidy/mpd/dispatcher.py b/mopidy/mpd/dispatcher.py index 099a2f18..175d8b32 100644 --- a/mopidy/mpd/dispatcher.py +++ b/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( diff --git a/mopidy/mpd/session.py b/mopidy/mpd/session.py index 68550f3b..d484d986 100644 --- a/mopidy/mpd/session.py +++ b/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): diff --git a/tests/mpd/protocol/test_idle.py b/tests/mpd/protocol/test_idle.py index 075da845..f93bf8aa 100644 --- a/tests/mpd/protocol/test_idle.py +++ b/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) diff --git a/tests/mpd/test_actor.py b/tests/mpd/test_actor.py new file mode 100644 index 00000000..843e46d3 --- /dev/null +++ b/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)