From 8234d38a5daee190e06bfcb3a845a4f956f16856 Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Sat, 7 Nov 2015 15:28:13 +0000 Subject: [PATCH 01/50] docs: deprecated section headings were mixed up --- docs/api/core.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/api/core.rst b/docs/api/core.rst index 5f1e406f..ead6d651 100644 --- a/docs/api/core.rst +++ b/docs/api/core.rst @@ -226,8 +226,8 @@ TracklistController .. autoattribute:: mopidy.core.TracklistController.repeat .. autoattribute:: mopidy.core.TracklistController.single -PlaylistsController -------------------- +PlaybackController +------------------ .. automethod:: mopidy.core.PlaybackController.get_mute .. automethod:: mopidy.core.PlaybackController.get_volume @@ -244,8 +244,8 @@ LibraryController .. automethod:: mopidy.core.LibraryController.find_exact -PlaybackController ------------------- +PlaylistsController +------------------- .. automethod:: mopidy.core.PlaylistsController.filter .. automethod:: mopidy.core.PlaylistsController.get_playlists From 95f526b60e69922f79f5e5274aa2e454f683d199 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Wed, 18 Nov 2015 14:51:27 +0100 Subject: [PATCH 02/50] docs: Use sphinx_rtd_theme As for Debian packaging, the theme is packaged in jessie. --- docs/conf.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/docs/conf.py b/docs/conf.py index 88ff29eb..bd553ae4 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -111,11 +111,7 @@ modindex_common_prefix = ['mopidy.'] # -- Options for HTML output -------------------------------------------------- -# 'sphinx_rtd_theme' is bundled with Sphinx 1.3, which we don't have when -# building the docs as part of the Debian packages on e.g. Debian wheezy. -# html_theme = 'sphinx_rtd_theme' -html_theme = 'default' -html_theme_path = ['_themes'] +html_theme = 'sphinx_rtd_theme' html_static_path = ['_static'] html_use_modindex = True From cc9a382f772b2f6a042d4bc62c756a296a4ea48e Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Wed, 18 Nov 2015 14:55:52 +0100 Subject: [PATCH 03/50] docs: Fix two links in same page with identical text --- docs/installation/raspberrypi.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/installation/raspberrypi.rst b/docs/installation/raspberrypi.rst index 495b0776..9bf9f550 100644 --- a/docs/installation/raspberrypi.rst +++ b/docs/installation/raspberrypi.rst @@ -181,7 +181,7 @@ Appendix C: Installation on XBian Similar to the Raspbmc issue outlined in Appendix B, it's not possible to install Mopidy on XBian without first resolving a dependency problem between ``gstreamer0.10-plugins-good`` and ``libtag1c2a``. More information can be -found in `this post +found in `this issue `_. Run the following commands to remedy this and then install Mopidy as normal:: From e48ac186f083930cfd19c91ece90b4f4ee46405a Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Wed, 18 Nov 2015 15:03:12 +0100 Subject: [PATCH 04/50] models: Deprecate Album.images Fixes #1325 --- docs/changelog.rst | 7 +++++++ mopidy/core/library.py | 2 +- mopidy/models/__init__.py | 10 +++++++--- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 4f49b8e5..0f9dacb6 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -16,6 +16,13 @@ Core API - Start ``tlid`` counting at 1 instead of 0 to keep in sync with MPD's ``songid``. +Models +------ + +- **Deprecated:** :attr:`mopidy.models.Album.images` is deprecated. Use + :meth:`mopidy.core.LibraryController.get_images` instead. (Fixes: + :issue:`1325`) + Local backend -------------- diff --git a/mopidy/core/library.py b/mopidy/core/library.py index f254172f..30064e5a 100644 --- a/mopidy/core/library.py +++ b/mopidy/core/library.py @@ -149,7 +149,7 @@ class LibraryController(object): """Lookup the images for the given URIs Backends can use this to return image URIs for any URI they know about - be it tracks, albums, playlists... The lookup result is a dictionary + be it tracks, albums, playlists. The lookup result is a dictionary mapping the provided URIs to lists of images. Unknown URIs or URIs the corresponding backend couldn't find anything diff --git a/mopidy/models/__init__.py b/mopidy/models/__init__.py index 9f93a01b..1e63d02f 100644 --- a/mopidy/models/__init__.py +++ b/mopidy/models/__init__.py @@ -146,6 +146,10 @@ class Album(ValidatedImmutableObject): :type musicbrainz_id: string :param images: album image URIs :type images: list of strings + + .. deprecated:: 1.2 + The ``images`` field is deprecated. + Use :meth:`mopidy.core.LibraryController.get_images` instead. """ #: The album URI. Read-only. @@ -170,10 +174,10 @@ class Album(ValidatedImmutableObject): musicbrainz_id = fields.Identifier() #: The album image URIs. Read-only. + #: + #: .. deprecated:: 1.2 + #: Use :meth:`mopidy.core.LibraryController.get_images` instead. images = fields.Collection(type=compat.string_types, container=frozenset) - # XXX If we want to keep the order of images we shouldn't use frozenset() - # as it doesn't preserve order. I'm deferring this issue until we got - # actual usage of this field with more than one image. class Track(ValidatedImmutableObject): From da7ec9b202e82dbd5833129b3e720e8fc380eaef Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 19 Nov 2015 22:45:55 +0100 Subject: [PATCH 05/50] core: Cleanup track ended event handling Trigger playback ended on: - stream changed - EOS - stop via stream changed events Old behavior was to manually trigger on: - next - prev - play with other track and old state != STOPPED - stop --- mopidy/core/playback.py | 25 +++++++++++++------------ tests/core/test_playback.py | 12 ++++++------ 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index fc20d412..5fe8d01b 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -25,6 +25,7 @@ class PlaybackController(object): self._current_tl_track = None self._pending_tl_track = None + self._last_position = None if self._audio: self._audio.set_about_to_finish_callback( @@ -197,10 +198,18 @@ class PlaybackController(object): def _on_end_of_stream(self): self.set_state(PlaybackState.STOPPED) + if self._current_tl_track: + self._trigger_track_playback_ended(self.get_time_position()) self._set_current_tl_track(None) - # TODO: self._trigger_track_playback_ended? def _on_stream_changed(self, uri): + if self._last_position is None: + position = self.get_time_position() + else: + # This code path handles the stop() case, uri should be none. + position, self._last_position = self._last_position, None + self._trigger_track_playback_ended(position) + self._stream_title = None if self._pending_tl_track: self._set_current_tl_track(self._pending_tl_track) @@ -221,8 +230,6 @@ class PlaybackController(object): }) def _on_about_to_finish(self): - self._trigger_track_playback_ended(self.get_time_position()) - # TODO: check that we always have a current track original_tl_track = self.get_current_tl_track() next_tl_track = self.core.tracklist.eot_track(original_tl_track) @@ -235,6 +242,7 @@ class PlaybackController(object): if backend: backend.playback.change_track(next_tl_track.track).get() + # TODO: move to stream changed and eos or just via trigger ended self.core.tracklist._mark_played(original_tl_track) def _on_tracklist_change(self): @@ -259,8 +267,6 @@ class PlaybackController(object): state = self.get_state() current = self._pending_tl_track or self._current_tl_track - # TODO: move to pending track? - self._trigger_track_playback_ended(self.get_time_position()) self.core.tracklist._mark_played(self._current_tl_track) while current: @@ -325,9 +331,6 @@ class PlaybackController(object): current = self._pending_tl_track or self._current_tl_track pending = tl_track or current or self.core.tracklist.next_track(None) - if original != pending and self.get_state() != PlaybackState.STOPPED: - self._trigger_track_playback_ended(self.get_time_position()) - if pending: # TODO: remove? self.set_state(PlaybackState.PLAYING) @@ -387,8 +390,6 @@ class PlaybackController(object): The current playback state will be kept. If it was playing, playing will continue. If it was paused, it will still be paused, etc. """ - self._trigger_track_playback_ended(self.get_time_position()) - state = self.get_state() current = self._pending_tl_track or self._current_tl_track @@ -470,11 +471,10 @@ class PlaybackController(object): def stop(self): """Stop playing.""" if self.get_state() != PlaybackState.STOPPED: + self._last_position = self.get_time_position() backend = self._get_backend(self.get_current_tl_track()) - time_position_before_stop = self.get_time_position() if not backend or backend.playback.stop().get(): self.set_state(PlaybackState.STOPPED) - self._trigger_track_playback_ended(time_position_before_stop) def _trigger_track_playback_paused(self): logger.debug('Triggering track playback paused event') @@ -509,6 +509,7 @@ class PlaybackController(object): logger.debug('Triggering track playback ended event') if self.get_current_tl_track() is None: return + # TODO: Use the lowest of track duration and position. listener.CoreListener.send( 'track_playback_ended', tl_track=self.get_current_tl_track(), diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index 0869b3ec..b5796827 100644 --- a/tests/core/test_playback.py +++ b/tests/core/test_playback.py @@ -346,12 +346,12 @@ class EventEmissionTest(BaseTest): self.assertListEqual( listener_mock.send.mock_calls, [ - mock.call( - 'track_playback_ended', - tl_track=tl_tracks[0], time_position=mock.ANY), mock.call( 'playback_state_changed', old_state='paused', new_state='playing'), + mock.call( + 'track_playback_ended', + tl_track=tl_tracks[0], time_position=mock.ANY), mock.call( 'track_playback_started', tl_track=tl_tracks[1]), ]) @@ -370,12 +370,12 @@ class EventEmissionTest(BaseTest): self.assertListEqual( listener_mock.send.mock_calls, [ - mock.call( - 'track_playback_ended', - tl_track=tl_tracks[0], time_position=mock.ANY), mock.call( 'playback_state_changed', old_state='playing', new_state='playing'), + mock.call( + 'track_playback_ended', + tl_track=tl_tracks[0], time_position=mock.ANY), mock.call( 'track_playback_started', tl_track=tl_tracks[2]), ]) From 01086a4cf4ad6e26a76e8439109ed41003c2d1a3 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 21 Nov 2015 00:34:27 +0100 Subject: [PATCH 06/50] core: Mark tracks as played via playback ended events --- mopidy/core/playback.py | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index 5fe8d01b..300a6a89 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -25,7 +25,9 @@ class PlaybackController(object): self._current_tl_track = None self._pending_tl_track = None + self._last_position = None + self._previous = False if self._audio: self._audio.set_about_to_finish_callback( @@ -208,6 +210,7 @@ class PlaybackController(object): else: # This code path handles the stop() case, uri should be none. position, self._last_position = self._last_position, None + self._trigger_track_playback_ended(position) self._stream_title = None @@ -242,9 +245,6 @@ class PlaybackController(object): if backend: backend.playback.change_track(next_tl_track.track).get() - # TODO: move to stream changed and eos or just via trigger ended - self.core.tracklist._mark_played(original_tl_track) - def _on_tracklist_change(self): """ Tell the playback controller that the current playlist has changed. @@ -267,8 +267,6 @@ class PlaybackController(object): state = self.get_state() current = self._pending_tl_track or self._current_tl_track - self.core.tracklist._mark_played(self._current_tl_track) - while current: pending = self.core.tracklist.next_track(current) if self._change(pending, state): @@ -327,7 +325,6 @@ class PlaybackController(object): self.resume() return - original = self._current_tl_track current = self._pending_tl_track or self._current_tl_track pending = tl_track or current or self.core.tracklist.next_track(None) @@ -344,8 +341,6 @@ class PlaybackController(object): current = pending pending = self.core.tracklist.next_track(current) - # TODO: move to top and get rid of original? - self.core.tracklist._mark_played(original) # TODO return result? def _change(self, pending_tl_track, state): @@ -390,6 +385,7 @@ class PlaybackController(object): The current playback state will be kept. If it was playing, playing will continue. If it was paused, it will still be paused, etc. """ + self._previous = True state = self.get_state() current = self._pending_tl_track or self._current_tl_track @@ -495,20 +491,25 @@ class PlaybackController(object): time_position=self.get_time_position()) def _trigger_track_playback_started(self): - # TODO: replace with stream-changed - logger.debug('Triggering track playback started event') if self.get_current_tl_track() is None: return + logger.debug('Triggering track playback started event') tl_track = self.get_current_tl_track() self.core.tracklist._mark_playing(tl_track) self.core.history._add_track(tl_track.track) listener.CoreListener.send('track_playback_started', tl_track=tl_track) def _trigger_track_playback_ended(self, time_position_before_stop): - logger.debug('Triggering track playback ended event') if self.get_current_tl_track() is None: return + + logger.debug('Triggering track playback ended event') + + if not self._previous: + self.core.tracklist._mark_played(self._current_tl_track) + self._previous = False + # TODO: Use the lowest of track duration and position. listener.CoreListener.send( 'track_playback_ended', @@ -522,5 +523,6 @@ class PlaybackController(object): old_state=old_state, new_state=new_state) def _trigger_seeked(self, time_position): + # TODO: Trigger this from audio events? logger.debug('Triggering seeked event') listener.CoreListener.send('seeked', time_position=time_position) From 216bd8e412cd44b5d97ff6903d987d637835cf2f Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 21 Nov 2015 14:28:47 +0100 Subject: [PATCH 07/50] tests: Reorder listener_mock.send.mock_calls in assertEqual --- tests/core/test_playback.py | 40 ++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index b5796827..0a86881c 100644 --- a/tests/core/test_playback.py +++ b/tests/core/test_playback.py @@ -321,14 +321,14 @@ class EventEmissionTest(BaseTest): self.replay_events() self.assertListEqual( - listener_mock.send.mock_calls, [ mock.call( 'playback_state_changed', old_state='stopped', new_state='playing'), mock.call( 'track_playback_started', tl_track=tl_tracks[0]), - ]) + ], + listener_mock.send.mock_calls) def test_play_when_paused_emits_events(self, listener_mock): tl_tracks = self.core.tracklist.get_tl_tracks() @@ -344,7 +344,6 @@ class EventEmissionTest(BaseTest): self.replay_events() self.assertListEqual( - listener_mock.send.mock_calls, [ mock.call( 'playback_state_changed', @@ -354,7 +353,8 @@ class EventEmissionTest(BaseTest): tl_track=tl_tracks[0], time_position=mock.ANY), mock.call( 'track_playback_started', tl_track=tl_tracks[1]), - ]) + ], + listener_mock.send.mock_calls) def test_play_when_playing_emits_events(self, listener_mock): tl_tracks = self.core.tracklist.get_tl_tracks() @@ -368,7 +368,6 @@ class EventEmissionTest(BaseTest): # TODO: Do we want to emit playing->playing for this case? self.assertListEqual( - listener_mock.send.mock_calls, [ mock.call( 'playback_state_changed', old_state='playing', @@ -378,7 +377,8 @@ class EventEmissionTest(BaseTest): tl_track=tl_tracks[0], time_position=mock.ANY), mock.call( 'track_playback_started', tl_track=tl_tracks[2]), - ]) + ], + listener_mock.send.mock_calls) def test_pause_emits_events(self, listener_mock): tl_tracks = self.core.tracklist.get_tl_tracks() @@ -392,7 +392,6 @@ class EventEmissionTest(BaseTest): self.core.playback.pause() self.assertListEqual( - listener_mock.send.mock_calls, [ mock.call( 'playback_state_changed', @@ -400,7 +399,8 @@ class EventEmissionTest(BaseTest): mock.call( 'track_playback_paused', tl_track=tl_tracks[0], time_position=1000), - ]) + ], + listener_mock.send.mock_calls) def test_resume_emits_events(self, listener_mock): tl_tracks = self.core.tracklist.get_tl_tracks() @@ -415,7 +415,6 @@ class EventEmissionTest(BaseTest): self.core.playback.resume() self.assertListEqual( - listener_mock.send.mock_calls, [ mock.call( 'playback_state_changed', @@ -423,7 +422,8 @@ class EventEmissionTest(BaseTest): mock.call( 'track_playback_resumed', tl_track=tl_tracks[0], time_position=1000), - ]) + ], + listener_mock.send.mock_calls) def test_stop_emits_events(self, listener_mock): tl_tracks = self.core.tracklist.get_tl_tracks() @@ -437,7 +437,6 @@ class EventEmissionTest(BaseTest): self.replay_events() self.assertListEqual( - listener_mock.send.mock_calls, [ mock.call( 'playback_state_changed', @@ -445,7 +444,8 @@ class EventEmissionTest(BaseTest): mock.call( 'track_playback_ended', tl_track=tl_tracks[0], time_position=1000), - ]) + ], + listener_mock.send.mock_calls) def test_next_emits_events(self, listener_mock): tl_tracks = self.core.tracklist.get_tl_tracks() @@ -460,14 +460,14 @@ class EventEmissionTest(BaseTest): # TODO: should we be emitting playing -> playing? self.assertListEqual( - listener_mock.send.mock_calls, [ mock.call( 'track_playback_ended', tl_track=tl_tracks[0], time_position=mock.ANY), mock.call( 'track_playback_started', tl_track=tl_tracks[1]), - ]) + ], + listener_mock.send.mock_calls) def test_on_end_of_track_emits_events(self, listener_mock): tl_tracks = self.core.tracklist.get_tl_tracks() @@ -479,14 +479,14 @@ class EventEmissionTest(BaseTest): self.trigger_about_to_finish() self.assertListEqual( - listener_mock.send.mock_calls, [ mock.call( 'track_playback_ended', tl_track=tl_tracks[0], time_position=mock.ANY), mock.call( 'track_playback_started', tl_track=tl_tracks[1]), - ]) + ], + listener_mock.send.mock_calls) def test_seek_emits_seeked_event(self, listener_mock): tl_tracks = self.core.tracklist.get_tl_tracks() @@ -511,14 +511,14 @@ class EventEmissionTest(BaseTest): self.replay_events() self.assertListEqual( - listener_mock.send.mock_calls, [ mock.call( 'track_playback_ended', tl_track=tl_tracks[0], time_position=mock.ANY), mock.call( 'track_playback_started', tl_track=tl_tracks[1]), - ]) + ], + listener_mock.send.mock_calls) def test_previous_emits_events(self, listener_mock): tl_tracks = self.core.tracklist.get_tl_tracks() @@ -531,14 +531,14 @@ class EventEmissionTest(BaseTest): self.replay_events() self.assertListEqual( - listener_mock.send.mock_calls, [ mock.call( 'track_playback_ended', tl_track=tl_tracks[1], time_position=mock.ANY), mock.call( 'track_playback_started', tl_track=tl_tracks[0]), - ]) + ], + listener_mock.send.mock_calls) class UnplayableURITest(BaseTest): From e767cb3f415fbfb0be913ec1b56501c0df5597a3 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 21 Nov 2015 22:31:25 +0100 Subject: [PATCH 08/50] tests: Convert local tracklist test to use core actor proxy. --- tests/local/test_tracklist.py | 242 ++++++++++++++++++++-------------- 1 file changed, 141 insertions(+), 101 deletions(-) diff --git a/tests/local/test_tracklist.py b/tests/local/test_tracklist.py index b7ed7dcb..6c9532e8 100644 --- a/tests/local/test_tracklist.py +++ b/tests/local/test_tracklist.py @@ -38,7 +38,9 @@ class LocalTracklistProviderTest(unittest.TestCase): self.audio = dummy_audio.create_proxy() self.backend = actor.LocalBackend.start( config=self.config, audio=self.audio).proxy() - self.core = core.Core(self.config, mixer=None, backends=[self.backend]) + self.core = core.Core.start(audio=self.audio, + backends=[self.backend], + config=self.config).proxy() self.controller = self.core.tracklist self.playback = self.core.playback @@ -47,216 +49,254 @@ class LocalTracklistProviderTest(unittest.TestCase): def tearDown(self): # noqa: N802 pykka.ActorRegistry.stop_all() + def assert_state_is(self, state): + self.assertEqual(self.playback.get_state().get(), state) + + def assert_current_track_is(self, track): + self.assertEqual(self.playback.get_current_track().get(), track) + def test_length(self): - self.assertEqual(0, len(self.controller.tl_tracks)) - self.assertEqual(0, self.controller.length) + self.assertEqual(0, len(self.controller.get_tl_tracks().get())) + self.assertEqual(0, self.controller.get_length().get()) self.controller.add(self.tracks) - self.assertEqual(3, len(self.controller.tl_tracks)) - self.assertEqual(3, self.controller.length) + self.assertEqual(3, len(self.controller.get_tl_tracks().get())) + self.assertEqual(3, self.controller.get_length().get()) def test_add(self): for track in self.tracks: - tl_tracks = self.controller.add([track]) - self.assertEqual(track, self.controller.tracks[-1]) - self.assertEqual(tl_tracks[0], self.controller.tl_tracks[-1]) - self.assertEqual(track, tl_tracks[0].track) + added = self.controller.add([track]).get() + tracks = self.controller.get_tracks().get() + tl_tracks = self.controller.get_tl_tracks().get() + + self.assertEqual(track, tracks[-1]) + self.assertEqual(added[0], tl_tracks[-1]) + self.assertEqual(track, added[0].track) def test_add_at_position(self): for track in self.tracks[:-1]: - tl_tracks = self.controller.add([track], 0) - self.assertEqual(track, self.controller.tracks[0]) - self.assertEqual(tl_tracks[0], self.controller.tl_tracks[0]) - self.assertEqual(track, tl_tracks[0].track) + added = self.controller.add([track], 0).get() + tracks = self.controller.get_tracks().get() + tl_tracks = self.controller.get_tl_tracks().get() + + self.assertEqual(track, tracks[0]) + self.assertEqual(added[0], tl_tracks[0]) + self.assertEqual(track, added[0].track) @populate_tracklist def test_add_at_position_outside_of_playlist(self): for track in self.tracks: - tl_tracks = self.controller.add([track], len(self.tracks) + 2) - self.assertEqual(track, self.controller.tracks[-1]) - self.assertEqual(tl_tracks[0], self.controller.tl_tracks[-1]) - self.assertEqual(track, tl_tracks[0].track) + added = self.controller.add([track], len(self.tracks) + 2).get() + tracks = self.controller.get_tracks().get() + tl_tracks = self.controller.get_tl_tracks().get() + + self.assertEqual(track, tracks[-1]) + self.assertEqual(added[0], tl_tracks[-1]) + self.assertEqual(track, added[0].track) @populate_tracklist def test_filter_by_tlid(self): - tl_track = self.controller.tl_tracks[1] - self.assertEqual( - [tl_track], self.controller.filter({'tlid': [tl_track.tlid]})) + tl_track = self.controller.get_tl_tracks().get()[1] + result = self.controller.filter({'tlid': [tl_track.tlid]}).get() + self.assertEqual([tl_track], result) @populate_tracklist def test_filter_by_uri(self): - tl_track = self.controller.tl_tracks[1] - self.assertEqual( - [tl_track], self.controller.filter({'uri': [tl_track.track.uri]})) + tl_track = self.controller.get_tl_tracks().get()[1] + result = self.controller.filter({'uri': [tl_track.track.uri]}).get() + self.assertEqual([tl_track], result) @populate_tracklist def test_filter_by_uri_returns_nothing_for_invalid_uri(self): - self.assertEqual([], self.controller.filter({'uri': ['foobar']})) + self.assertEqual([], self.controller.filter({'uri': ['foobar']}).get()) def test_filter_by_uri_returns_single_match(self): t = Track(uri='a') self.controller.add([Track(uri='z'), t, Track(uri='y')]) - self.assertEqual(t, self.controller.filter({'uri': ['a']})[0].track) + + result = self.controller.filter({'uri': ['a']}).get() + self.assertEqual(t, result[0].track) def test_filter_by_uri_returns_multiple_matches(self): track = Track(uri='a') self.controller.add([Track(uri='z'), track, track]) - tl_tracks = self.controller.filter({'uri': ['a']}) + tl_tracks = self.controller.filter({'uri': ['a']}).get() self.assertEqual(track, tl_tracks[0].track) self.assertEqual(track, tl_tracks[1].track) def test_filter_by_uri_returns_nothing_if_no_match(self): self.controller.playlist = Playlist( tracks=[Track(uri='z'), Track(uri='y')]) - self.assertEqual([], self.controller.filter({'uri': ['a']})) + self.assertEqual([], self.controller.filter({'uri': ['a']}).get()) def test_filter_by_multiple_criteria_returns_elements_matching_all(self): t1 = Track(uri='a', name='x') t2 = Track(uri='b', name='x') t3 = Track(uri='b', name='y') self.controller.add([t1, t2, t3]) - self.assertEqual( - t1, self.controller.filter({'uri': ['a'], 'name': ['x']})[0].track) - self.assertEqual( - t2, self.controller.filter({'uri': ['b'], 'name': ['x']})[0].track) - self.assertEqual( - t3, self.controller.filter({'uri': ['b'], 'name': ['y']})[0].track) + + result1 = self.controller.filter({'uri': ['a'], 'name': ['x']}).get() + self.assertEqual(t1, result1[0].track) + + result2 = self.controller.filter({'uri': ['b'], 'name': ['x']}).get() + self.assertEqual(t2, result2[0].track) + + result3 = self.controller.filter({'uri': ['b'], 'name': ['y']}).get() + self.assertEqual(t3, result3[0].track) def test_filter_by_criteria_that_is_not_present_in_all_elements(self): track1 = Track() track2 = Track(uri='b') track3 = Track() + self.controller.add([track1, track2, track3]) - self.assertEqual( - track2, self.controller.filter({'uri': ['b']})[0].track) + result = self.controller.filter({'uri': ['b']}).get() + self.assertEqual(track2, result[0].track) @populate_tracklist def test_clear(self): - self.controller.clear() - self.assertEqual(len(self.controller.tracks), 0) + self.controller.clear().get() + self.assertEqual(len(self.controller.get_tracks().get()), 0) def test_clear_empty_playlist(self): - self.controller.clear() - self.assertEqual(len(self.controller.tracks), 0) + self.controller.clear().get() + self.assertEqual(len(self.controller.get_tracks().get()), 0) @populate_tracklist def test_clear_when_playing(self): - self.playback.play() - self.assertEqual(self.playback.state, PlaybackState.PLAYING) - self.controller.clear() - self.assertEqual(self.playback.state, PlaybackState.STOPPED) + self.playback.play().get() + self.assert_state_is(PlaybackState.PLAYING) + self.controller.clear().get() + self.assert_state_is(PlaybackState.STOPPED) def test_add_appends_to_the_tracklist(self): self.controller.add([Track(uri='a'), Track(uri='b')]) - self.assertEqual(len(self.controller.tracks), 2) + + tracks = self.controller.get_tracks().get() + self.assertEqual(len(tracks), 2) + self.controller.add([Track(uri='c'), Track(uri='d')]) - self.assertEqual(len(self.controller.tracks), 4) - self.assertEqual(self.controller.tracks[0].uri, 'a') - self.assertEqual(self.controller.tracks[1].uri, 'b') - self.assertEqual(self.controller.tracks[2].uri, 'c') - self.assertEqual(self.controller.tracks[3].uri, 'd') + + tracks = self.controller.get_tracks().get() + self.assertEqual(len(tracks), 4) + self.assertEqual(tracks[0].uri, 'a') + self.assertEqual(tracks[1].uri, 'b') + self.assertEqual(tracks[2].uri, 'c') + self.assertEqual(tracks[3].uri, 'd') def test_add_does_not_reset_version(self): - version = self.controller.version + version = self.controller.get_version().get() self.controller.add([]) - self.assertEqual(self.controller.version, version) + self.assertEqual(self.controller.get_version().get(), version) @populate_tracklist def test_add_preserves_playing_state(self): - self.playback.play() - track = self.playback.current_track - self.controller.add(self.controller.tracks[1:2]) - self.assertEqual(self.playback.state, PlaybackState.PLAYING) - self.assertEqual(self.playback.current_track, track) + self.playback.play().get() + + track = self.playback.get_current_track().get() + tracks = self.controller.get_tracks().get() + self.controller.add(tracks[1:2]).get() + + self.assert_state_is(PlaybackState.PLAYING) + self.assert_current_track_is(track) @populate_tracklist def test_add_preserves_stopped_state(self): - self.controller.add(self.controller.tracks[1:2]) - self.assertEqual(self.playback.state, PlaybackState.STOPPED) - self.assertEqual(self.playback.current_track, None) + tracks = self.controller.get_tracks().get() + self.controller.add(tracks[1:2]).get() + + self.assert_state_is(PlaybackState.STOPPED) + self.assert_current_track_is(None) @populate_tracklist def test_add_returns_the_tl_tracks_that_was_added(self): - tl_tracks = self.controller.add(self.controller.tracks[1:2]) - self.assertEqual(tl_tracks[0].track, self.controller.tracks[1]) + tracks = self.controller.get_tracks().get() + + added = self.controller.add(tracks[1:2]).get() + tracks = self.controller.get_tracks().get() + self.assertEqual(added[0].track, tracks[1]) @populate_tracklist def test_move_single(self): self.controller.move(0, 0, 2) - tracks = self.controller.tracks + tracks = self.controller.get_tracks().get() self.assertEqual(tracks[2], self.tracks[0]) @populate_tracklist def test_move_group(self): self.controller.move(0, 2, 1) - tracks = self.controller.tracks + tracks = self.controller.get_tracks().get() self.assertEqual(tracks[1], self.tracks[0]) self.assertEqual(tracks[2], self.tracks[1]) @populate_tracklist def test_moving_track_outside_of_playlist(self): - tracks = len(self.controller.tracks) + num_tracks = len(self.controller.get_tracks().get()) with self.assertRaises(AssertionError): - self.controller.move(0, 0, tracks + 5) + self.controller.move(0, 0, num_tracks + 5).get() @populate_tracklist def test_move_group_outside_of_playlist(self): - tracks = len(self.controller.tracks) + num_tracks = len(self.controller.get_tracks().get()) with self.assertRaises(AssertionError): - self.controller.move(0, 2, tracks + 5) + self.controller.move(0, 2, num_tracks + 5).get() @populate_tracklist def test_move_group_out_of_range(self): - tracks = len(self.controller.tracks) + num_tracks = len(self.controller.get_tracks().get()) with self.assertRaises(AssertionError): - self.controller.move(tracks + 2, tracks + 3, 0) + self.controller.move(num_tracks + 2, num_tracks + 3, 0).get() @populate_tracklist def test_move_group_invalid_group(self): with self.assertRaises(AssertionError): - self.controller.move(2, 1, 0) + self.controller.move(2, 1, 0).get() def test_tracks_attribute_is_immutable(self): - tracks1 = self.controller.tracks - tracks2 = self.controller.tracks + tracks1 = self.controller.tracks.get() + tracks2 = self.controller.tracks.get() self.assertNotEqual(id(tracks1), id(tracks2)) @populate_tracklist def test_remove(self): - track1 = self.controller.tracks[1] - track2 = self.controller.tracks[2] - version = self.controller.version + track1 = self.controller.get_tracks().get()[1] + track2 = self.controller.get_tracks().get()[2] + version = self.controller.get_version().get() self.controller.remove({'uri': [track1.uri]}) - self.assertLess(version, self.controller.version) - self.assertNotIn(track1, self.controller.tracks) - self.assertEqual(track2, self.controller.tracks[1]) + self.assertLess(version, self.controller.get_version().get()) + self.assertNotIn(track1, self.controller.get_tracks().get()) + self.assertEqual(track2, self.controller.get_tracks().get()[1]) @populate_tracklist def test_removing_track_that_does_not_exist_does_nothing(self): - self.controller.remove({'uri': ['/nonexistant']}) + self.controller.remove({'uri': ['/nonexistant']}).get() def test_removing_from_empty_playlist_does_nothing(self): - self.controller.remove({'uri': ['/nonexistant']}) + self.controller.remove({'uri': ['/nonexistant']}).get() @populate_tracklist def test_remove_lists(self): - track0 = self.controller.tracks[0] - track1 = self.controller.tracks[1] - track2 = self.controller.tracks[2] - version = self.controller.version + version = self.controller.get_version().get() + tracks = self.controller.get_tracks().get() + track0 = tracks[0] + track1 = tracks[1] + track2 = tracks[2] + self.controller.remove({'uri': [track0.uri, track2.uri]}) - self.assertLess(version, self.controller.version) - self.assertNotIn(track0, self.controller.tracks) - self.assertNotIn(track2, self.controller.tracks) - self.assertEqual(track1, self.controller.tracks[0]) + + tracks = self.controller.get_tracks().get() + self.assertLess(version, self.controller.get_version().get()) + self.assertNotIn(track0, tracks) + self.assertNotIn(track2, tracks) + self.assertEqual(track1, tracks[0]) @populate_tracklist def test_shuffle(self): random.seed(1) self.controller.shuffle() - shuffled_tracks = self.controller.tracks + shuffled_tracks = self.controller.get_tracks().get() self.assertNotEqual(self.tracks, shuffled_tracks) self.assertEqual(set(self.tracks), set(shuffled_tracks)) @@ -266,7 +306,7 @@ class LocalTracklistProviderTest(unittest.TestCase): random.seed(1) self.controller.shuffle(1, 3) - shuffled_tracks = self.controller.tracks + shuffled_tracks = self.controller.get_tracks().get() self.assertNotEqual(self.tracks, shuffled_tracks) self.assertEqual(self.tracks[0], shuffled_tracks[0]) @@ -275,20 +315,20 @@ class LocalTracklistProviderTest(unittest.TestCase): @populate_tracklist def test_shuffle_invalid_subset(self): with self.assertRaises(AssertionError): - self.controller.shuffle(3, 1) + self.controller.shuffle(3, 1).get() @populate_tracklist def test_shuffle_superset(self): - tracks = len(self.controller.tracks) + num_tracks = len(self.controller.get_tracks().get()) with self.assertRaises(AssertionError): - self.controller.shuffle(1, tracks + 5) + self.controller.shuffle(1, num_tracks + 5).get() @populate_tracklist def test_shuffle_open_subset(self): random.seed(1) self.controller.shuffle(1) - shuffled_tracks = self.controller.tracks + shuffled_tracks = self.controller.get_tracks().get() self.assertNotEqual(self.tracks, shuffled_tracks) self.assertEqual(self.tracks[0], shuffled_tracks[0]) @@ -296,22 +336,22 @@ class LocalTracklistProviderTest(unittest.TestCase): @populate_tracklist def test_slice_returns_a_subset_of_tracks(self): - track_slice = self.controller.slice(1, 3) + track_slice = self.controller.slice(1, 3).get() self.assertEqual(2, len(track_slice)) self.assertEqual(self.tracks[1], track_slice[0].track) self.assertEqual(self.tracks[2], track_slice[1].track) @populate_tracklist def test_slice_returns_empty_list_if_indexes_outside_tracks_list(self): - self.assertEqual(0, len(self.controller.slice(7, 8))) - self.assertEqual(0, len(self.controller.slice(-1, 1))) + self.assertEqual(0, len(self.controller.slice(7, 8).get())) + self.assertEqual(0, len(self.controller.slice(-1, 1).get())) def test_version_does_not_change_when_adding_nothing(self): - version = self.controller.version + version = self.controller.get_version().get() self.controller.add([]) - self.assertEqual(version, self.controller.version) + self.assertEqual(version, self.controller.get_version().get()) def test_version_increases_when_adding_something(self): - version = self.controller.version + version = self.controller.get_version().get() self.controller.add([Track()]) - self.assertLess(version, self.controller.version) + self.assertLess(version, self.controller.get_version().get()) From 3a57a5792bd8da519c513a27fea316d35c27475a Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 21 Nov 2015 22:43:40 +0100 Subject: [PATCH 09/50] core: Make sure we always emit state_changed between tracks Gapless broke this, so this change makes sure that next/prev/play and gapless track changes all correctly emit events. Note that this only ensures we get PLAYING -> PLAYING events. Not the old STOPPED -> PLAYING and then PLAYING -> STOPPED. --- mopidy/core/playback.py | 8 ++++---- tests/core/test_playback.py | 32 +++++++++++++++++++++++--------- tests/local/test_playback.py | 2 +- 3 files changed, 28 insertions(+), 14 deletions(-) diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index 300a6a89..45e1b4ba 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -217,6 +217,7 @@ class PlaybackController(object): if self._pending_tl_track: self._set_current_tl_track(self._pending_tl_track) self._pending_tl_track = None + self.set_state(PlaybackState.PLAYING) self._trigger_track_playback_started() def _on_about_to_finish_callback(self): @@ -233,6 +234,9 @@ class PlaybackController(object): }) def _on_about_to_finish(self): + if self._state == PlaybackState.STOPPED: + return + # TODO: check that we always have a current track original_tl_track = self.get_current_tl_track() next_tl_track = self.core.tracklist.eot_track(original_tl_track) @@ -328,10 +332,6 @@ class PlaybackController(object): current = self._pending_tl_track or self._current_tl_track pending = tl_track or current or self.core.tracklist.next_track(None) - if pending: - # TODO: remove? - self.set_state(PlaybackState.PLAYING) - while pending: # TODO: should we consume unplayable tracks in this loop? if self._change(pending, PlaybackState.PLAYING): diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index 0a86881c..4ae3b4ef 100644 --- a/tests/core/test_playback.py +++ b/tests/core/test_playback.py @@ -314,6 +314,8 @@ class TestCurrentAndPendingTlTrack(BaseTest): 'mopidy.core.playback.listener.CoreListener', spec=core.CoreListener) class EventEmissionTest(BaseTest): + maxDiff = None + def test_play_when_stopped_emits_events(self, listener_mock): tl_tracks = self.core.tracklist.get_tl_tracks() @@ -345,12 +347,12 @@ class EventEmissionTest(BaseTest): self.assertListEqual( [ - mock.call( - 'playback_state_changed', - old_state='paused', new_state='playing'), mock.call( 'track_playback_ended', tl_track=tl_tracks[0], time_position=mock.ANY), + mock.call( + 'playback_state_changed', + old_state='paused', new_state='playing'), mock.call( 'track_playback_started', tl_track=tl_tracks[1]), ], @@ -366,15 +368,14 @@ class EventEmissionTest(BaseTest): self.core.playback.play(tl_tracks[2]) self.replay_events() - # TODO: Do we want to emit playing->playing for this case? self.assertListEqual( [ - mock.call( - 'playback_state_changed', old_state='playing', - new_state='playing'), mock.call( 'track_playback_ended', tl_track=tl_tracks[0], time_position=mock.ANY), + mock.call( + 'playback_state_changed', old_state='playing', + new_state='playing'), mock.call( 'track_playback_started', tl_track=tl_tracks[2]), ], @@ -458,18 +459,20 @@ class EventEmissionTest(BaseTest): self.core.playback.next() self.replay_events() - # TODO: should we be emitting playing -> playing? self.assertListEqual( [ mock.call( 'track_playback_ended', tl_track=tl_tracks[0], time_position=mock.ANY), + mock.call( + 'playback_state_changed', + old_state='playing', new_state='playing'), mock.call( 'track_playback_started', tl_track=tl_tracks[1]), ], listener_mock.send.mock_calls) - def test_on_end_of_track_emits_events(self, listener_mock): + def test_gapless_track_change_emits_events(self, listener_mock): tl_tracks = self.core.tracklist.get_tl_tracks() self.core.playback.play(tl_tracks[0]) @@ -483,6 +486,9 @@ class EventEmissionTest(BaseTest): mock.call( 'track_playback_ended', tl_track=tl_tracks[0], time_position=mock.ANY), + mock.call( + 'playback_state_changed', + old_state='playing', new_state='playing'), mock.call( 'track_playback_started', tl_track=tl_tracks[1]), ], @@ -515,6 +521,9 @@ class EventEmissionTest(BaseTest): mock.call( 'track_playback_ended', tl_track=tl_tracks[0], time_position=mock.ANY), + mock.call( + 'playback_state_changed', + old_state='playing', new_state='playing'), mock.call( 'track_playback_started', tl_track=tl_tracks[1]), ], @@ -535,6 +544,9 @@ class EventEmissionTest(BaseTest): mock.call( 'track_playback_ended', tl_track=tl_tracks[1], time_position=mock.ANY), + mock.call( + 'playback_state_changed', + old_state='playing', new_state='playing'), mock.call( 'track_playback_started', tl_track=tl_tracks[0]), ], @@ -612,6 +624,8 @@ class SeekTest(BaseTest): tl_tracks = self.core.tracklist.get_tl_tracks() self.core.playback.play(tl_tracks[0]) + self.replay_events() + self.core.playback.pause() self.replay_events() diff --git a/tests/local/test_playback.py b/tests/local/test_playback.py index 92fbe5b9..f0dcf20b 100644 --- a/tests/local/test_playback.py +++ b/tests/local/test_playback.py @@ -998,7 +998,7 @@ class LocalPlaybackProviderTest(unittest.TestCase): self.playback.next().get() self.assert_next_tl_track_is_not(None) self.assert_state_is(PlaybackState.STOPPED) - self.playback.play() + self.playback.play().get() self.assert_state_is(PlaybackState.PLAYING) @populate_tracklist From 149fa15cab206979f245348aa7b82191ee68c5f9 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sun, 22 Nov 2015 21:05:31 +0100 Subject: [PATCH 10/50] docs: Fix return value reference Fixes #1332 --- mopidy/ext.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mopidy/ext.py b/mopidy/ext.py index 7fd68f96..fe8d0daf 100644 --- a/mopidy/ext.py +++ b/mopidy/ext.py @@ -54,7 +54,7 @@ class Extension(object): def get_config_schema(self): """The extension's config validation schema - :returns: :class:`~mopidy.config.schema.ExtensionConfigSchema` + :returns: :class:`~mopidy.config.schemas.ConfigSchema` """ schema = config_lib.ConfigSchema(self.ext_name) schema['enabled'] = config_lib.Boolean() From fa817dee352ac2c75b10c96d036900b8f6f117bc Mon Sep 17 00:00:00 2001 From: Daniel Hahler Date: Sat, 28 Nov 2015 23:00:28 +0100 Subject: [PATCH 11/50] Fix wording with TypeError in ImmutableObject.replace --- mopidy/models/immutable.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mopidy/models/immutable.py b/mopidy/models/immutable.py index 8bbf568b..18de7d76 100644 --- a/mopidy/models/immutable.py +++ b/mopidy/models/immutable.py @@ -112,7 +112,7 @@ class ImmutableObject(object): for key, value in kwargs.items(): if not self._is_valid_field(key): raise TypeError( - 'copy() got an unexpected keyword argument "%s"' % key) + 'replace() got an unexpected keyword argument "%s"' % key) other._set_field(key, value) return other From ebb413b6b1e59e141c50d9466ab8b95d1628b84a Mon Sep 17 00:00:00 2001 From: Daniel Hahler Date: Sat, 28 Nov 2015 19:51:27 +0100 Subject: [PATCH 12/50] Handle exceptions in load_extensions This will skip those extensions, instead of crashing mopidy, e.g. when mopidy-mopify fails because of a missing HOME environment variable during mopidy's tests. Ref: https://github.com/dirkgroenen/mopidy-mopify/issues/119 --- mopidy/ext.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/mopidy/ext.py b/mopidy/ext.py index fe8d0daf..48a623bb 100644 --- a/mopidy/ext.py +++ b/mopidy/ext.py @@ -198,7 +198,12 @@ def load_extensions(): for entry_point in pkg_resources.iter_entry_points('mopidy.ext'): logger.debug('Loading entry point: %s', entry_point) - extension_class = entry_point.load(require=False) + try: + extension_class = entry_point.load(require=False) + except Exception as e: + logger.exception("Failed to load extension %s: %s" % ( + entry_point.name, e)) + continue try: if not issubclass(extension_class, Extension): From e495e382e7d2c4b2b507b28424086e141bd49b35 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sat, 28 Nov 2015 23:54:44 +0100 Subject: [PATCH 13/50] docs: Update changelog and authors --- AUTHORS | 1 + docs/changelog.rst | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/AUTHORS b/AUTHORS index 439b8ed7..38c394dc 100644 --- a/AUTHORS +++ b/AUTHORS @@ -75,3 +75,4 @@ - kozec - Jelle van der Waa - Alex Malone +- Daniel Hahler diff --git a/docs/changelog.rst b/docs/changelog.rst index 0f9dacb6..3bda9237 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -23,6 +23,12 @@ Models :meth:`mopidy.core.LibraryController.get_images` instead. (Fixes: :issue:`1325`) +Extension support +----------------- + +- Log exception and continue if an extension crashes during setup. Previously, + we let Mopidy crash if an extension's setup crashed. (PR: :issue:`1337`) + Local backend -------------- From 02f3fc24bbe18ca4f94d610ba0db77790ea234d9 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sun, 29 Nov 2015 00:53:50 +0100 Subject: [PATCH 14/50] travis: coveralls 1.0 has been released --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index eb8aadfe..5f01f223 100644 --- a/.travis.yml +++ b/.travis.yml @@ -27,7 +27,7 @@ script: - "tox -e $TOX_ENV" after_success: - - "if [ $TOX_ENV == 'py27' ]; then pip install coveralls==1.0b1; coveralls; fi" + - "if [ $TOX_ENV == 'py27' ]; then pip install coveralls; coveralls; fi" branches: except: From 3c24d2d7826c08823e4a280df8adf3ea6985dca2 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sun, 29 Nov 2015 00:45:27 +0100 Subject: [PATCH 15/50] travis: Test on Ubuntu 14.04 infrastructure This change also minimizes the set of Python packages installed with APT so that we're testing against the latest PyPI releases when possible. --- .travis.yml | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/.travis.yml b/.travis.yml index 5f01f223..964ae89f 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,18 +1,11 @@ -sudo: false +sudo: required +dist: trusty language: python python: - "2.7_with_system_site_packages" -addons: - apt: - sources: - - mopidy-stable - packages: - - graphviz-dev - - mopidy - env: - TOX_ENV=py27 - TOX_ENV=py27-tornado23 @@ -20,6 +13,11 @@ env: - TOX_ENV=docs - TOX_ENV=flake8 +before_install: + - "sudo sed -i '/127.0.1.1/d' /etc/hosts" # Workaround tornadoweb/tornado#1573 + - "sudo apt-get update -qq" + - "sudo apt-get install -y graphviz-dev gstreamer0.10-plugins-good python-gst0.10" + install: - "pip install tox" From e74eafb38a75b43bd1d7408339d255f4f5442fd9 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 3 Dec 2015 22:27:22 +0100 Subject: [PATCH 16/50] core: Switch back to correct track if seek happens before stream changed Technically the seek still needs to be postponed for this to work right, but it's a step closer. --- mopidy/core/playback.py | 8 ++++---- tests/core/test_playback.py | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index 45e1b4ba..96fac4d9 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -437,10 +437,10 @@ class PlaybackController(object): if self.get_state() == PlaybackState.STOPPED: self.play() - # TODO: uncomment once we have tests for this. Should fix seek after - # about to finish doing wrong track. - # if self._current_tl_track and self._pending_tl_track: - # self.play(self._current_tl_track) + # Make sure we switch back to previous track if we get a seek while we + # have a pending track. + if self._current_tl_track and self._pending_tl_track: + self._change(self._current_tl_track, self.get_state()) # We need to prefer the still playing track, but if nothing is playing # we fall back to the pending one. diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index 4ae3b4ef..6ea5313f 100644 --- a/tests/core/test_playback.py +++ b/tests/core/test_playback.py @@ -6,6 +6,8 @@ import mock import pykka +import pytest + from mopidy import backend, core from mopidy.internal import deprecation from mopidy.models import Track @@ -529,6 +531,25 @@ class EventEmissionTest(BaseTest): ], listener_mock.send.mock_calls) + @pytest.mark.xfail + def test_seek_race_condition_emits_events(self, listener_mock): + tl_tracks = self.core.tracklist.get_tl_tracks() + + self.core.playback.play(tl_tracks[0]) + self.trigger_about_to_finish(replay_until='stream_changed') + listener_mock.reset_mock() + + self.core.playback.seek(1000) + self.replay_events() + + # When we trigger seek after an about to finish the other code that + # emits track stopped/started and playback state changed events gets + # triggered as we have to switch back to the previous track. + # The correct behavior would be to only emit seeked. + self.assertListEqual( + [mock.call('seeked', time_position=1000)], + listener_mock.send.mock_calls) + def test_previous_emits_events(self, listener_mock): tl_tracks = self.core.tracklist.get_tl_tracks() @@ -632,6 +653,19 @@ class SeekTest(BaseTest): self.core.playback.seek(1000) self.assertEqual(self.core.playback.state, core.PlaybackState.PAUSED) + def test_seek_race_condition_after_about_to_finish(self): + tl_tracks = self.core.tracklist.get_tl_tracks() + + self.core.playback.play(tl_tracks[0]) + self.replay_events() + + self.trigger_about_to_finish(replay_until='stream_changed') + self.core.playback.seek(1000) + self.replay_events() + + current_tl_track = self.core.playback.get_current_tl_track() + self.assertEqual(current_tl_track, tl_tracks[0]) + class TestStream(BaseTest): From aeb881896b16f2f627a3d3aade1c47d982a34f81 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 3 Dec 2015 23:00:52 +0100 Subject: [PATCH 17/50] core: Trigger position changed from audio events. Makes sure to only fire when the position changed to our intended seek target. Otherwise we would also be triggering this when playback starts. --- mopidy/core/actor.py | 3 +++ mopidy/core/playback.py | 12 ++++++++---- tests/core/test_playback.py | 3 +++ 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/mopidy/core/actor.py b/mopidy/core/actor.py index e365e4b7..93cb814e 100644 --- a/mopidy/core/actor.py +++ b/mopidy/core/actor.py @@ -90,6 +90,9 @@ class Core( def stream_changed(self, uri): self.playback._on_stream_changed(uri) + def position_changed(self, position): + self.playback._on_position_changed(position) + def state_changed(self, old_state, new_state, target_state): # XXX: This is a temporary fix for issue #232 while we wait for a more # permanent solution with the implementation of issue #234. When the diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index 96fac4d9..ea725004 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -26,6 +26,7 @@ class PlaybackController(object): self._current_tl_track = None self._pending_tl_track = None + self._pending_position = None self._last_position = None self._previous = False @@ -220,6 +221,11 @@ class PlaybackController(object): self.set_state(PlaybackState.PLAYING) self._trigger_track_playback_started() + def _on_position_changed(self, position): + if self._pending_position == position: + self._trigger_seeked(position) + self._pending_position = None + def _on_about_to_finish_callback(self): """Callback that performs a blocking actor call to the real callback. @@ -455,14 +461,12 @@ class PlaybackController(object): self.next() return True + self._pending_position = time_position backend = self._get_backend(self.get_current_tl_track()) if not backend: return False - success = backend.playback.seek(time_position).get() - if success: - self._trigger_seeked(time_position) - return success + return backend.playback.seek(time_position).get() def stop(self): """Stop playing.""" diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index 6ea5313f..8ff37861 100644 --- a/tests/core/test_playback.py +++ b/tests/core/test_playback.py @@ -434,6 +434,7 @@ class EventEmissionTest(BaseTest): self.core.playback.play(tl_tracks[0]) self.replay_events() self.core.playback.seek(1000) + self.replay_events() listener_mock.reset_mock() self.core.playback.stop() @@ -456,6 +457,7 @@ class EventEmissionTest(BaseTest): self.core.playback.play(tl_tracks[0]) self.replay_events() self.core.playback.seek(1000) + self.replay_events() listener_mock.reset_mock() self.core.playback.next() @@ -504,6 +506,7 @@ class EventEmissionTest(BaseTest): listener_mock.reset_mock() self.core.playback.seek(1000) + self.replay_events() listener_mock.send.assert_called_once_with( 'seeked', time_position=1000) From 454077afebeab1d0acb0ae230328f386b68db364 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 3 Dec 2015 23:23:55 +0100 Subject: [PATCH 18/50] core: Make sure certain events are ignored when doing eot-seeks --- mopidy/core/playback.py | 9 ++++++--- tests/core/test_playback.py | 3 --- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index ea725004..f9c9295a 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -212,14 +212,17 @@ class PlaybackController(object): # This code path handles the stop() case, uri should be none. position, self._last_position = self._last_position, None - self._trigger_track_playback_ended(position) + if self._pending_position is None: + self._trigger_track_playback_ended(position) self._stream_title = None if self._pending_tl_track: self._set_current_tl_track(self._pending_tl_track) self._pending_tl_track = None - self.set_state(PlaybackState.PLAYING) - self._trigger_track_playback_started() + + if self._pending_position is None: + self.set_state(PlaybackState.PLAYING) + self._trigger_track_playback_started() def _on_position_changed(self, position): if self._pending_position == position: diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index 8ff37861..f0d6d477 100644 --- a/tests/core/test_playback.py +++ b/tests/core/test_playback.py @@ -6,8 +6,6 @@ import mock import pykka -import pytest - from mopidy import backend, core from mopidy.internal import deprecation from mopidy.models import Track @@ -534,7 +532,6 @@ class EventEmissionTest(BaseTest): ], listener_mock.send.mock_calls) - @pytest.mark.xfail def test_seek_race_condition_emits_events(self, listener_mock): tl_tracks = self.core.tracklist.get_tl_tracks() From eeb1f91ed1e83f2e7da5e4e8fe37ef8e403f2a8f Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 3 Dec 2015 23:33:48 +0100 Subject: [PATCH 19/50] core: Actually perform delayed "eot-seek" on stream changed --- mopidy/core/playback.py | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index f9c9295a..e877866f 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -223,6 +223,8 @@ class PlaybackController(object): if self._pending_position is None: self.set_state(PlaybackState.PLAYING) self._trigger_track_playback_started() + else: + self._seek(self._pending_position) def _on_position_changed(self, position): if self._pending_position == position: @@ -446,11 +448,6 @@ class PlaybackController(object): if self.get_state() == PlaybackState.STOPPED: self.play() - # Make sure we switch back to previous track if we get a seek while we - # have a pending track. - if self._current_tl_track and self._pending_tl_track: - self._change(self._current_tl_track, self.get_state()) - # We need to prefer the still playing track, but if nothing is playing # we fall back to the pending one. tl_track = self._current_tl_track or self._pending_tl_track @@ -464,11 +461,20 @@ class PlaybackController(object): self.next() return True + # Store our target position. self._pending_position = time_position + + # Make sure we switch back to previous track if we get a seek while we + # have a pending track. + if self._current_tl_track and self._pending_tl_track: + self._change(self._current_tl_track, self.get_state()) + else: + return self._seek(time_position) + + def _seek(self, time_position): backend = self._get_backend(self.get_current_tl_track()) if not backend: return False - return backend.playback.seek(time_position).get() def stop(self): From 9f23757cc3f9f7fc3b48eabeeccef720cc03fb49 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 4 Dec 2015 20:59:01 +0100 Subject: [PATCH 20/50] core: Return pending position during active seek. This covers over that audio will fail query position while a seek is in progress. It also means that instead of returning zero we at least return something which is much closer to the time that we will soon end up playing from. --- mopidy/core/playback.py | 2 ++ tests/core/test_playback.py | 2 -- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index e877866f..89bd92ee 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -131,6 +131,8 @@ class PlaybackController(object): def get_time_position(self): """Get time position in milliseconds.""" + if self._pending_position is not None: + return self._pending_position backend = self._get_backend(self.get_current_tl_track()) if backend: return backend.playback.get_time_position().get() diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index f0d6d477..0da59b4d 100644 --- a/tests/core/test_playback.py +++ b/tests/core/test_playback.py @@ -855,7 +855,6 @@ class BackendSelectionTest(unittest.TestCase): self.core.playback.play(self.tl_tracks[0]) self.trigger_stream_changed() - self.core.playback.seek(10000) self.core.playback.time_position self.playback1.get_time_position.assert_called_once_with() @@ -865,7 +864,6 @@ class BackendSelectionTest(unittest.TestCase): self.core.playback.play(self.tl_tracks[1]) self.trigger_stream_changed() - self.core.playback.seek(10000) self.core.playback.time_position self.assertFalse(self.playback1.get_time_position.called) From 7ab26652925095ea22f485caadffb6351b00c687 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 4 Dec 2015 21:20:24 +0100 Subject: [PATCH 21/50] mpd: Switch MpdSession to using on_event and re-use listener helper. --- mopidy/mpd/actor.py | 6 ++---- mopidy/mpd/session.py | 2 +- tests/mpd/protocol/test_idle.py | 2 +- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/mopidy/mpd/actor.py b/mopidy/mpd/actor.py index 7439e73f..7770ef60 100644 --- a/mopidy/mpd/actor.py +++ b/mopidy/mpd/actor.py @@ -4,7 +4,7 @@ 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 @@ -57,9 +57,7 @@ class MpdFrontend(pykka.ThreadingActor, CoreListener): process.stop_actors_by_class(session.MpdSession) def send_idle(self, subsystem): - listeners = pykka.ActorRegistry.get_by_class(session.MpdSession) - for listener in listeners: - getattr(listener.proxy(), 'on_idle')(subsystem) + listener.send(session.MpdSession, subsystem) def playback_state_changed(self, old_state, new_state): self.send_idle('player') 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) From 49b0580c394bf26c8bb4e6efb003fb5b355fc748 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 4 Dec 2015 22:08:56 +0100 Subject: [PATCH 22/50] mpd: Fix call signature for core playlist_deleted event --- mopidy/mpd/actor.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mopidy/mpd/actor.py b/mopidy/mpd/actor.py index 7770ef60..b7e3ab0d 100644 --- a/mopidy/mpd/actor.py +++ b/mopidy/mpd/actor.py @@ -68,7 +68,7 @@ class MpdFrontend(pykka.ThreadingActor, CoreListener): def playlist_changed(self, playlist): self.send_idle('stored_playlist') - def playlist_deleted(self, playlist): + def playlist_deleted(self, uri): self.send_idle('stored_playlist') def options_changed(self): From 7d4da4ac8c910d1e900cd695902cff91b015513f Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 4 Dec 2015 22:10:27 +0100 Subject: [PATCH 23/50] mpd: Add integration test for core events and idle --- mopidy/mpd/actor.py | 3 +++ tests/mpd/test_actor.py | 44 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) create mode 100644 tests/mpd/test_actor.py diff --git a/mopidy/mpd/actor.py b/mopidy/mpd/actor.py index b7e3ab0d..af78c331 100644 --- a/mopidy/mpd/actor.py +++ b/mopidy/mpd/actor.py @@ -24,6 +24,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, diff --git a/tests/mpd/test_actor.py b/tests/mpd/test_actor.py new file mode 100644 index 00000000..968fbfe7 --- /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'], None), + (['playlist_changed', 'playlist'], 'stored_playlist'), + (['playlist_deleted', 'uri'], 'stored_playlist'), + (['options_changed'], 'options'), + (['volume_changed', 'volume'], 'mixer'), + (['mute_changed', 'mute'], 'output'), + (['seeked', 'time_position'], None), + (['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) From 19daa89e15efa64cc128e742f6f8d3426f1adb0b Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 4 Dec 2015 22:11:55 +0100 Subject: [PATCH 24/50] mpd: Add missing seeked event handling for idle --- mopidy/mpd/actor.py | 3 +++ tests/mpd/test_actor.py | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/mopidy/mpd/actor.py b/mopidy/mpd/actor.py index af78c331..9b1cbc1b 100644 --- a/mopidy/mpd/actor.py +++ b/mopidy/mpd/actor.py @@ -85,3 +85,6 @@ class MpdFrontend(pykka.ThreadingActor, CoreListener): def stream_title_changed(self, title): self.send_idle('playlist') + + def seeked(self, time_position): + self.send_idle('player') diff --git a/tests/mpd/test_actor.py b/tests/mpd/test_actor.py index 968fbfe7..9975e1cc 100644 --- a/tests/mpd/test_actor.py +++ b/tests/mpd/test_actor.py @@ -22,7 +22,7 @@ from mopidy.mpd import actor (['options_changed'], 'options'), (['volume_changed', 'volume'], 'mixer'), (['mute_changed', 'mute'], 'output'), - (['seeked', 'time_position'], None), + (['seeked', 'time_position'], 'player'), (['stream_title_changed', 'title'], 'playlist'), ]) def test_idle_hooked_up_correctly(event, expected): From a086857dd71b605ed7b4ba3419f60a637c82f07e Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 4 Dec 2015 22:16:35 +0100 Subject: [PATCH 25/50] docs: Update changelog with MPD idle fixes --- docs/changelog.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index 3bda9237..387a67bf 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -52,6 +52,11 @@ MPD frontend - Start ``songid`` counting at 1 instead of 0 to match the original MPD server. +- Idle events are now emitted on ``seekeded`` events. (Fixes: :issue:`1331`) + +- Event handler for ``playlist_deleted`` has been unbroken. Likely fixes + unreported / diagnosed crashes. + Zeroconf -------- From 438b4b7a35681ddefdfcf5318a50324e663055f0 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 4 Dec 2015 23:09:48 +0100 Subject: [PATCH 26/50] docs: Update changelog with seek related changes --- docs/changelog.rst | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index 3bda9237..610788d6 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -16,6 +16,11 @@ Core API - Start ``tlid`` counting at 1 instead of 0 to keep in sync with MPD's ``songid``. +- `get_time_position` now returns the seek target while a seek is in progress. + This gives better results than just failing the position query. + + (Fixes: :issue:`312` PR: :pr:`1346`) + Models ------ @@ -83,6 +88,11 @@ Gapless - Tests have been updated to always use a core actor so async state changes don't trip us up. +- Seek events are now triggered when the seek completes. Further changes have + been made to make seek work correctly for gapless related corner cases. + + (Fixes: :issue:`1305` PR: :pr:`1346`) + v1.1.2 (UNRELEASED) =================== From aa010e03e995d68913a071793f1847a1ac8cd356 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 4 Dec 2015 23:38:55 +0100 Subject: [PATCH 27/50] listener: Try and protect actors against "bad" events --- mopidy/core/listener.py | 3 ++- mopidy/listener.py | 6 +++++- mopidy/mpd/dispatcher.py | 1 + 3 files changed, 8 insertions(+), 2 deletions(-) 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..b4ea5ec3 100644 --- a/mopidy/listener.py +++ b/mopidy/listener.py @@ -41,4 +41,8 @@ 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', event) 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( From 28224cef8c1b32cf81a0f5d6d14b58fa8b3031d3 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Fri, 4 Dec 2015 10:23:41 +0100 Subject: [PATCH 28/50] audio: Fix tests not exiting normally --- tests/audio/test_actor.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/audio/test_actor.py b/tests/audio/test_actor.py index 314d1d42..0cfbdaf3 100644 --- a/tests/audio/test_actor.py +++ b/tests/audio/test_actor.py @@ -163,7 +163,7 @@ class AudioEventTest(BaseTest): self.listener = DummyAudioListener.start().proxy() def tearDown(self): # noqa: N802 - super(AudioEventTest, self).setUp() + super(AudioEventTest, self).tearDown() def assertEvent(self, event, **kwargs): # noqa: N802 self.assertIn((event, kwargs), self.listener.get_events().get()) From 435ca5064aa5b6c690fe810cdb50f5c3d344b923 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Dec 2015 11:05:45 +0100 Subject: [PATCH 29/50] listener: Log kwargs in failed send calls --- mopidy/listener.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/mopidy/listener.py b/mopidy/listener.py index b4ea5ec3..79c53570 100644 --- a/mopidy/listener.py +++ b/mopidy/listener.py @@ -45,4 +45,5 @@ class Listener(object): getattr(self, event)(**kwargs) except Exception: # Ensure we don't crash the actor due to "bad" events. - logger.exception('Triggering event failed: %s', event) + logger.exception( + 'Triggering event failed: %s(%s)', event, ', '.join(kwargs)) From 07328e7dd2d504279e64fea6e7c263314750374d Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Dec 2015 11:08:33 +0100 Subject: [PATCH 30/50] mpd: Map playlists_loaded to idle event stored_playlist --- mopidy/mpd/actor.py | 3 +++ tests/mpd/test_actor.py | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/mopidy/mpd/actor.py b/mopidy/mpd/actor.py index 9b1cbc1b..ff2385c8 100644 --- a/mopidy/mpd/actor.py +++ b/mopidy/mpd/actor.py @@ -68,6 +68,9 @@ class MpdFrontend(pykka.ThreadingActor, CoreListener): def tracklist_changed(self): self.send_idle('playlist') + def playlists_loaded(self): + self.send_idle('stored_playlist') + def playlist_changed(self, playlist): self.send_idle('stored_playlist') diff --git a/tests/mpd/test_actor.py b/tests/mpd/test_actor.py index 9975e1cc..843e46d3 100644 --- a/tests/mpd/test_actor.py +++ b/tests/mpd/test_actor.py @@ -16,7 +16,7 @@ from mopidy.mpd import actor (['track_playback_ended', 'tl_track', 'time_position'], None), (['playback_state_changed', 'old_state', 'new_state'], 'player'), (['tracklist_changed'], 'playlist'), - (['playlists_loaded'], None), + (['playlists_loaded'], 'stored_playlist'), (['playlist_changed', 'playlist'], 'stored_playlist'), (['playlist_deleted', 'uri'], 'stored_playlist'), (['options_changed'], 'options'), From da84c0f59c67fa7c76b8a5d989fce8116c8e04bd Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Dec 2015 11:22:33 +0100 Subject: [PATCH 31/50] docs: Update changelog for MPD idle fixes --- docs/changelog.rst | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 387a67bf..cf6bc54d 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -52,10 +52,17 @@ MPD frontend - Start ``songid`` counting at 1 instead of 0 to match the original MPD server. -- Idle events are now emitted on ``seekeded`` events. (Fixes: :issue:`1331`) +- 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`) -- Event handler for ``playlist_deleted`` has been unbroken. Likely fixes - unreported / diagnosed crashes. +- 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 -------- @@ -76,6 +83,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 ------- From 8c6bb639636725bfc9e2cd1d21668fecd3aa4e82 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Dec 2015 11:34:15 +0100 Subject: [PATCH 32/50] docs: Improve changelog PR#1346 per review comments --- docs/changelog.rst | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 610788d6..87e7470e 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -16,10 +16,9 @@ Core API - Start ``tlid`` counting at 1 instead of 0 to keep in sync with MPD's ``songid``. -- `get_time_position` now returns the seek target while a seek is in progress. - This gives better results than just failing the position query. - - (Fixes: :issue:`312` PR: :pr:`1346`) +- :meth:`~mopidy.core.PlaybackController.get_time_position` now returns the + seek target while a seek is in progress. This gives better results than just + failing the position query. (Fixes: :issue:`312` PR: :issue:`1346`) Models ------ @@ -88,10 +87,10 @@ Gapless - Tests have been updated to always use a core actor so async state changes don't trip us up. -- Seek events are now triggered when the seek completes. Further changes have - been made to make seek work correctly for gapless related corner cases. - - (Fixes: :issue:`1305` PR: :pr:`1346`) +- Seek events are now triggered when the seek completes. Previously the event + was emitted when the seek was requested, not when it completed. Further + changes have been made to make seek work correctly for gapless related corner + cases. (Fixes: :issue:`1305` PR: :pr:`1346`) v1.1.2 (UNRELEASED) From 98e19e803bb4241be6b70e43603c9dc508811a3f Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Dec 2015 12:48:14 +0100 Subject: [PATCH 33/50] mpd: Deleting unkown playlist should return not found --- mopidy/mpd/protocol/stored_playlists.py | 2 ++ tests/mpd/protocol/test_stored_playlists.py | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/mopidy/mpd/protocol/stored_playlists.py b/mopidy/mpd/protocol/stored_playlists.py index 647a1464..1a24608c 100644 --- a/mopidy/mpd/protocol/stored_playlists.py +++ b/mopidy/mpd/protocol/stored_playlists.py @@ -328,6 +328,8 @@ def rm(context, name): Removes the playlist ``NAME.m3u`` from the playlist directory. """ uri = context.lookup_playlist_uri_from_name(name) + if not uri: + raise exceptions.MpdNoExistError('No such playlist') context.core.playlists.delete(uri).get() diff --git a/tests/mpd/protocol/test_stored_playlists.py b/tests/mpd/protocol/test_stored_playlists.py index e212af09..36635505 100644 --- a/tests/mpd/protocol/test_stored_playlists.py +++ b/tests/mpd/protocol/test_stored_playlists.py @@ -330,6 +330,10 @@ class PlaylistsHandlerTest(protocol.BaseTestCase): self.assertInResponse('OK') self.assertIsNone(self.backend.playlists.lookup('dummy:a1').get()) + def test_rm_unknown_playlist_acks(self): + self.send_request('rm "name"') + self.assertInResponse('ACK [50@0] {rm} No such playlist') + def test_save(self): self.send_request('save "name"') From c3393d3d859e0c93fe0c98f2c2f30dccc59f76e5 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Dec 2015 12:49:45 +0100 Subject: [PATCH 34/50] mpd: Refresh mapping when name is not present (Fixes #1348) --- mopidy/mpd/uri_mapper.py | 2 +- tests/mpd/protocol/test_regression.py | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/mopidy/mpd/uri_mapper.py b/mopidy/mpd/uri_mapper.py index 9e7ec2dd..bb627a47 100644 --- a/mopidy/mpd/uri_mapper.py +++ b/mopidy/mpd/uri_mapper.py @@ -71,7 +71,7 @@ class MpdUriMapper(object): """ Helper function to retrieve a playlist URI from its unique MPD name. """ - if not self._uri_from_name: + if name not in self._uri_from_name: self.refresh_playlists_mapping() return self._uri_from_name.get(name) diff --git a/tests/mpd/protocol/test_regression.py b/tests/mpd/protocol/test_regression.py index 1688d064..40a3f103 100644 --- a/tests/mpd/protocol/test_regression.py +++ b/tests/mpd/protocol/test_regression.py @@ -233,3 +233,24 @@ class IssueGH1120RegressionTest(protocol.BaseTestCase): response2 = self.send_request('lsinfo "/"') self.assertEqual(response1, response2) + + +class IssueGH1348RegressionTest(protocol.BaseTestCase): + + """ + The issue: http://github.com/mopidy/mopidy/issues/1348 + """ + + def test(self): + self.backend.library.dummy_library = [Track(uri='dummy:a')] + + # Create a dummy playlist and trigger population of mapping + self.send_request('playlistadd "testing1" "dummy:a"') + self.send_request('listplaylists') + + # Create an other playlist which isn't in the map + self.send_request('playlistadd "testing2" "dummy:a"') + self.assertEqual(['OK'], self.send_request('rm "testing2"')) + + playlists = self.backend.playlists.as_list().get() + self.assertEqual(['testing1'], [ref.name for ref in playlists]) From b21debf6eec60f146c9f6f6c7378a1be5d813528 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Dec 2015 13:51:36 +0100 Subject: [PATCH 35/50] mpd: Sanity check stored playlist names --- mopidy/mpd/exceptions.py | 9 ++++++ mopidy/mpd/protocol/stored_playlists.py | 11 +++++++ tests/mpd/protocol/test_stored_playlists.py | 34 +++++++++++++++++++++ 3 files changed, 54 insertions(+) diff --git a/mopidy/mpd/exceptions.py b/mopidy/mpd/exceptions.py index b64a6cf0..65771f2a 100644 --- a/mopidy/mpd/exceptions.py +++ b/mopidy/mpd/exceptions.py @@ -84,6 +84,15 @@ class MpdSystemError(MpdAckError): error_code = MpdAckError.ACK_ERROR_SYSTEM +class MpdInvalidPlaylistName(MpdAckError): + error_code = MpdAckError.ACK_ERROR_ARG + + def __init__(self, *args, **kwargs): + super(MpdInvalidPlaylistName, self).__init__(*args, **kwargs) + self.message = ('playlist name is invalid: playlist names may not ' + 'contain slashes, newlines or carriage returns') + + class MpdNotImplemented(MpdAckError): error_code = 0 diff --git a/mopidy/mpd/protocol/stored_playlists.py b/mopidy/mpd/protocol/stored_playlists.py index 1a24608c..fd395696 100644 --- a/mopidy/mpd/protocol/stored_playlists.py +++ b/mopidy/mpd/protocol/stored_playlists.py @@ -2,6 +2,7 @@ from __future__ import absolute_import, division, unicode_literals import datetime import logging +import re import warnings from mopidy.compat import urllib @@ -10,6 +11,11 @@ from mopidy.mpd import exceptions, protocol, translator logger = logging.getLogger(__name__) +def _check_playlist_name(name): + if re.search('[/\n\r]', name): + raise exceptions.MpdInvalidPlaylistName() + + @protocol.commands.add('listplaylist') def listplaylist(context, name): """ @@ -149,6 +155,7 @@ def playlistadd(context, name, track_uri): ``NAME.m3u`` will be created if it does not exist. """ + _check_playlist_name(name) uri = context.lookup_playlist_uri_from_name(name) old_playlist = uri is not None and context.core.playlists.lookup(uri).get() if not old_playlist: @@ -219,6 +226,7 @@ def playlistclear(context, name): The playlist will be created if it does not exist. """ + _check_playlist_name(name) uri = context.lookup_playlist_uri_from_name(name) playlist = uri is not None and context.core.playlists.lookup(uri).get() if not playlist: @@ -240,6 +248,7 @@ def playlistdelete(context, name, songpos): Deletes ``SONGPOS`` from the playlist ``NAME.m3u``. """ + _check_playlist_name(name) uri = context.lookup_playlist_uri_from_name(name) playlist = uri is not None and context.core.playlists.lookup(uri).get() if not playlist: @@ -327,6 +336,7 @@ def rm(context, name): Removes the playlist ``NAME.m3u`` from the playlist directory. """ + _check_playlist_name(name) uri = context.lookup_playlist_uri_from_name(name) if not uri: raise exceptions.MpdNoExistError('No such playlist') @@ -343,6 +353,7 @@ def save(context, name): Saves the current playlist to ``NAME.m3u`` in the playlist directory. """ + _check_playlist_name(name) tracks = context.core.tracklist.get_tracks().get() uri = context.lookup_playlist_uri_from_name(name) playlist = uri is not None and context.core.playlists.lookup(uri).get() diff --git a/tests/mpd/protocol/test_stored_playlists.py b/tests/mpd/protocol/test_stored_playlists.py index 36635505..e33b1bc2 100644 --- a/tests/mpd/protocol/test_stored_playlists.py +++ b/tests/mpd/protocol/test_stored_playlists.py @@ -232,6 +232,10 @@ class PlaylistsHandlerTest(protocol.BaseTestCase): self.assertEqual(0, len(self.core.tracklist.tracks.get())) self.assertEqualResponse('ACK [50@0] {load} No such playlist') + # No invalid name check for load. + self.send_request('load "unknown/playlist"') + self.assertEqualResponse('ACK [50@0] {load} No such playlist') + def test_playlistadd(self): tracks = [ Track(uri='dummy:a'), @@ -259,6 +263,12 @@ class PlaylistsHandlerTest(protocol.BaseTestCase): self.assertInResponse('OK') self.assertIsNotNone(self.backend.playlists.lookup('dummy:name').get()) + def test_playlistadd_invalid_name_acks(self): + self.send_request('playlistadd "foo/bar" "dummy:a"') + self.assertInResponse('ACK [2@0] {playlistadd} playlist name is ' + 'invalid: playlist names may not contain ' + 'slashes, newlines or carriage returns') + def test_playlistclear(self): self.backend.playlists.set_dummy_playlists([ Playlist( @@ -276,6 +286,12 @@ class PlaylistsHandlerTest(protocol.BaseTestCase): self.assertInResponse('OK') self.assertIsNotNone(self.backend.playlists.lookup('dummy:name').get()) + def test_playlistclear_invalid_name_acks(self): + self.send_request('playlistclear "foo/bar"') + self.assertInResponse('ACK [2@0] {playlistclear} playlist name is ' + 'invalid: playlist names may not contain ' + 'slashes, newlines or carriage returns') + def test_playlistdelete(self): tracks = [ Track(uri='dummy:a'), @@ -292,6 +308,12 @@ class PlaylistsHandlerTest(protocol.BaseTestCase): self.assertEqual( 2, len(self.backend.playlists.get_items('dummy:a1').get())) + def test_playlistdelete_invalid_name_acks(self): + self.send_request('playlistdelete "foo/bar" "0"') + self.assertInResponse('ACK [2@0] {playlistdelete} playlist name is ' + 'invalid: playlist names may not contain ' + 'slashes, newlines or carriage returns') + def test_playlistmove(self): tracks = [ Track(uri='dummy:a'), @@ -334,8 +356,20 @@ class PlaylistsHandlerTest(protocol.BaseTestCase): self.send_request('rm "name"') self.assertInResponse('ACK [50@0] {rm} No such playlist') + def test_rm_invalid_name_acks(self): + self.send_request('rm "foo/bar"') + self.assertInResponse('ACK [2@0] {rm} playlist name is invalid: ' + 'playlist names may not contain slashes, ' + 'newlines or carriage returns') + def test_save(self): self.send_request('save "name"') self.assertInResponse('OK') self.assertIsNotNone(self.backend.playlists.lookup('dummy:name').get()) + + def test_save_invalid_name_acks(self): + self.send_request('save "foo/bar"') + self.assertInResponse('ACK [2@0] {save} playlist name is invalid: ' + 'playlist names may not contain slashes, ' + 'newlines or carriage returns') From 5de9495eaaa4002b7e62c82431542097f9cc013c Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Dec 2015 14:01:43 +0100 Subject: [PATCH 36/50] mpd: Update playlistdelete to handle unknown names and indexes --- mopidy/mpd/protocol/stored_playlists.py | 9 ++++++--- tests/mpd/protocol/test_stored_playlists.py | 9 +++++++++ 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/mopidy/mpd/protocol/stored_playlists.py b/mopidy/mpd/protocol/stored_playlists.py index fd395696..affd1126 100644 --- a/mopidy/mpd/protocol/stored_playlists.py +++ b/mopidy/mpd/protocol/stored_playlists.py @@ -254,9 +254,12 @@ def playlistdelete(context, name, songpos): if not playlist: raise exceptions.MpdNoExistError('No such playlist') - # Convert tracks to list and remove requested - tracks = list(playlist.tracks) - tracks.pop(songpos) + try: + # Convert tracks to list and remove requested + tracks = list(playlist.tracks) + tracks.pop(songpos) + except IndexError: + raise exceptions.MpdArgError('Bad song index') # Replace tracks and save playlist playlist = playlist.replace(tracks=tracks) diff --git a/tests/mpd/protocol/test_stored_playlists.py b/tests/mpd/protocol/test_stored_playlists.py index e33b1bc2..6b568667 100644 --- a/tests/mpd/protocol/test_stored_playlists.py +++ b/tests/mpd/protocol/test_stored_playlists.py @@ -314,6 +314,15 @@ class PlaylistsHandlerTest(protocol.BaseTestCase): 'invalid: playlist names may not contain ' 'slashes, newlines or carriage returns') + def test_playlistdelete_unknown_playlist_acks(self): + self.send_request('playlistdelete "foobar" "0"') + self.assertInResponse('ACK [50@0] {playlistdelete} No such playlist') + + def test_playlistdelete_unknown_index_acks(self): + self.send_request('save "foobar"') + self.send_request('playlistdelete "foobar" "0"') + self.assertInResponse('ACK [2@0] {playlistdelete} Bad song index') + def test_playlistmove(self): tracks = [ Track(uri='dummy:a'), From 9ac1760dd1867a5f97a2c77dd45b63334bb4b957 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Dec 2015 14:11:27 +0100 Subject: [PATCH 37/50] mpd: Update playlistmove to check names and indexes --- mopidy/mpd/protocol/stored_playlists.py | 15 ++++++--- tests/mpd/protocol/test_stored_playlists.py | 36 +++++++++++++++++++++ 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/mopidy/mpd/protocol/stored_playlists.py b/mopidy/mpd/protocol/stored_playlists.py index affd1126..2a096c07 100644 --- a/mopidy/mpd/protocol/stored_playlists.py +++ b/mopidy/mpd/protocol/stored_playlists.py @@ -286,6 +286,10 @@ def playlistmove(context, name, from_pos, to_pos): documentation, but just the ``SONGPOS`` to move *from*, i.e. ``playlistmove {NAME} {FROM_SONGPOS} {TO_SONGPOS}``. """ + if from_pos == to_pos: + return + + _check_playlist_name(name) uri = context.lookup_playlist_uri_from_name(name) playlist = uri is not None and context.core.playlists.lookup(uri).get() if not playlist: @@ -293,10 +297,13 @@ def playlistmove(context, name, from_pos, to_pos): if from_pos == to_pos: return # Nothing to do - # Convert tracks to list and perform move - tracks = list(playlist.tracks) - track = tracks.pop(from_pos) - tracks.insert(to_pos, track) + try: + # Convert tracks to list and perform move + tracks = list(playlist.tracks) + track = tracks.pop(from_pos) + tracks.insert(to_pos, track) + except IndexError: + raise exceptions.MpdArgError('Bad song index') # Replace tracks and save playlist playlist = playlist.replace(tracks=tracks) diff --git a/tests/mpd/protocol/test_stored_playlists.py b/tests/mpd/protocol/test_stored_playlists.py index 6b568667..da214486 100644 --- a/tests/mpd/protocol/test_stored_playlists.py +++ b/tests/mpd/protocol/test_stored_playlists.py @@ -340,6 +340,42 @@ class PlaylistsHandlerTest(protocol.BaseTestCase): "dummy:c", self.backend.playlists.get_items('dummy:a1').get()[0].uri) + def test_playlistmove_invalid_name_acks(self): + self.send_request('playlistmove "foo/bar" "0" "1"') + self.assertInResponse('ACK [2@0] {playlistmove} playlist name is ' + 'invalid: playlist names may not contain ' + 'slashes, newlines or carriage returns') + + def test_playlistmove_unknown_playlist_acks(self): + self.send_request('playlistmove "foobar" "0" "1"') + self.assertInResponse('ACK [50@0] {playlistmove} No such playlist') + + def test_playlistmove_unknown_position_acks(self): + self.send_request('save "foobar"') + self.send_request('playlistmove "foobar" "0" "1"') + self.assertInResponse('ACK [2@0] {playlistmove} Bad song index') + + def test_playlistmove_same_index_shortcircuits_everything(self): + # Bad indexes on unknown playlist: + self.send_request('playlistmove "foobar" "0" "0"') + self.assertInResponse('OK') + + self.send_request('playlistmove "foobar" "100000" "100000"') + self.assertInResponse('OK') + + # Bad indexes on known playlist: + self.send_request('save "foobar"') + + self.send_request('playlistmove "foobar" "0" "0"') + self.assertInResponse('OK') + + self.send_request('playlistmove "foobar" "10" "10"') + self.assertInResponse('OK') + + # Invalid playlist name: + self.send_request('playlistmove "foo/bar" "0" "0"') + self.assertInResponse('OK') + def test_rename(self): self.backend.playlists.set_dummy_playlists([ Playlist( From 00b52da6ab360f4a6fc1668aa0575747d6434650 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Dec 2015 14:30:18 +0100 Subject: [PATCH 38/50] mpd: Make sure rename error handling is correct --- mopidy/mpd/exceptions.py | 4 ++++ mopidy/mpd/protocol/stored_playlists.py | 18 ++++++++++++--- tests/mpd/protocol/test_stored_playlists.py | 25 +++++++++++++++++++++ 3 files changed, 44 insertions(+), 3 deletions(-) diff --git a/mopidy/mpd/exceptions.py b/mopidy/mpd/exceptions.py index 65771f2a..05762683 100644 --- a/mopidy/mpd/exceptions.py +++ b/mopidy/mpd/exceptions.py @@ -80,6 +80,10 @@ class MpdNoExistError(MpdAckError): error_code = MpdAckError.ACK_ERROR_NO_EXIST +class MpdExistError(MpdAckError): + error_code = MpdAckError.ACK_ERROR_EXIST + + class MpdSystemError(MpdAckError): error_code = MpdAckError.ACK_ERROR_SYSTEM diff --git a/mopidy/mpd/protocol/stored_playlists.py b/mopidy/mpd/protocol/stored_playlists.py index 2a096c07..68ae1e9e 100644 --- a/mopidy/mpd/protocol/stored_playlists.py +++ b/mopidy/mpd/protocol/stored_playlists.py @@ -322,16 +322,28 @@ def rename(context, old_name, new_name): Renames the playlist ``NAME.m3u`` to ``NEW_NAME.m3u``. """ - uri = context.lookup_playlist_uri_from_name(old_name) - uri_scheme = urllib.parse.urlparse(uri).scheme - old_playlist = uri is not None and context.core.playlists.lookup(uri).get() + _check_playlist_name(old_name) + _check_playlist_name(new_name) + + old_uri = context.lookup_playlist_uri_from_name(old_name) + if not old_uri: + raise exceptions.MpdNoExistError('No such playlist') + + old_playlist = context.core.playlists.lookup(old_uri).get() if not old_playlist: raise exceptions.MpdNoExistError('No such playlist') + new_uri = context.lookup_playlist_uri_from_name(new_name) + if new_uri and context.core.playlists.lookup(new_uri).get(): + raise exceptions.MpdExistError('Playlist already exists') + # TODO: should we purge the mapping in an else? + # Create copy of the playlist and remove original + uri_scheme = urllib.parse.urlparse(old_uri).scheme new_playlist = context.core.playlists.create(new_name, uri_scheme).get() new_playlist = new_playlist.replace(tracks=old_playlist.tracks) saved_playlist = context.core.playlists.save(new_playlist).get() + if saved_playlist is None: raise exceptions.MpdFailedToSavePlaylist(uri_scheme) context.core.playlists.delete(old_playlist.uri).get() diff --git a/tests/mpd/protocol/test_stored_playlists.py b/tests/mpd/protocol/test_stored_playlists.py index da214486..fc3e8214 100644 --- a/tests/mpd/protocol/test_stored_playlists.py +++ b/tests/mpd/protocol/test_stored_playlists.py @@ -387,6 +387,31 @@ class PlaylistsHandlerTest(protocol.BaseTestCase): self.assertIsNotNone( self.backend.playlists.lookup('dummy:new_name').get()) + def test_rename_unknown_playlist_acks(self): + self.send_request('rename "foo" "bar"') + self.assertInResponse('ACK [50@0] {rename} No such playlist') + + def test_rename_to_existing_acks(self): + self.send_request('save "foo"') + self.send_request('save "bar"') + + self.send_request('rename "foo" "bar"') + self.assertInResponse('ACK [56@0] {rename} Playlist already exists') + + def test_rename_invalid_name_acks(self): + expected = ('ACK [2@0] {rename} playlist name is invalid: playlist ' + 'names may not contain slashes, newlines or carriage ' + 'returns') + + self.send_request('rename "foo/bar" "bar"') + self.assertInResponse(expected) + + self.send_request('rename "foo" "foo/bar"') + self.assertInResponse(expected) + + self.send_request('rename "bar/foo" "foo/bar"') + self.assertInResponse(expected) + def test_rm(self): self.backend.playlists.set_dummy_playlists([ Playlist( From 1c2850bc3eb0bab9d0624be0eadb7fcdd74fbf59 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Dec 2015 21:24:18 +0100 Subject: [PATCH 39/50] docs: Use :issue: instead of :pr: --- docs/changelog.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 87e7470e..8cc886ce 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -90,7 +90,7 @@ Gapless - Seek events are now triggered when the seek completes. Previously the event was emitted when the seek was requested, not when it completed. Further changes have been made to make seek work correctly for gapless related corner - cases. (Fixes: :issue:`1305` PR: :pr:`1346`) + cases. (Fixes: :issue:`1305` PR: :issue:`1346`) v1.1.2 (UNRELEASED) From c2fc313151aeb7fef8abbebaf16be69ba8f5d7f1 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Dec 2015 21:26:43 +0100 Subject: [PATCH 40/50] mpd: Update event handling to warn about unknown events --- mopidy/mpd/actor.py | 57 +++++++++++++++++++++------------------------ 1 file changed, 26 insertions(+), 31 deletions(-) diff --git a/mopidy/mpd/actor.py b/mopidy/mpd/actor.py index ff2385c8..067d20c5 100644 --- a/mopidy/mpd/actor.py +++ b/mopidy/mpd/actor.py @@ -11,6 +11,23 @@ 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): @@ -59,35 +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): - listener.send(session.MpdSession, subsystem) - - def playback_state_changed(self, old_state, new_state): - self.send_idle('player') - - def tracklist_changed(self): - self.send_idle('playlist') - - def playlists_loaded(self): - self.send_idle('stored_playlist') - - def playlist_changed(self, playlist): - self.send_idle('stored_playlist') - - def playlist_deleted(self, uri): - 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') - - def seeked(self, time_position): - self.send_idle('player') + if subsystem: + listener.send(session.MpdSession, subsystem) From ede5b8abff6ff269f7180288f98ff1f68c3de1fc Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 5 Dec 2015 22:44:39 +0100 Subject: [PATCH 41/50] logging: Catch errors when loading logging/config_file --- docs/changelog.rst | 3 +++ mopidy/internal/log.py | 10 ++++++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 4c304be1..49b7f263 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -90,6 +90,9 @@ Cleanups - 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. +- Catch errors when loading :confval:`logging/config_file`. + (Fixes: :issue:`1320`) + Gapless ------- diff --git a/mopidy/internal/log.py b/mopidy/internal/log.py index 9c40da4f..011a70d2 100644 --- a/mopidy/internal/log.py +++ b/mopidy/internal/log.py @@ -19,6 +19,8 @@ LOG_LEVELS = { TRACE_LOG_LEVEL = 5 logging.addLevelName(TRACE_LOG_LEVEL, 'TRACE') +logger = logging.getLogger(__name__) + class DelayedHandler(logging.Handler): @@ -54,8 +56,12 @@ def setup_logging(config, verbosity_level, save_debug_log): if config['logging']['config_file']: # Logging config from file must be read before other handlers are # added. If not, the other handlers will have no effect. - logging.config.fileConfig(config['logging']['config_file'], - disable_existing_loggers=False) + try: + path = config['logging']['config_file'] + logging.config.fileConfig(path, disable_existing_loggers=False) + except Exception as e: + # Catch everything as logging does not specify what can go wrong. + logger.error('Loading logging config %r failed. %s', path, e) setup_console_logging(config, verbosity_level) if save_debug_log: From ef1468d8d6b5d1cdb929ee1ecaf286da2cccde4e Mon Sep 17 00:00:00 2001 From: Thomas Kemmer Date: Sun, 13 Dec 2015 19:02:33 +0100 Subject: [PATCH 42/50] core: Add PlaylistsController.get_uri_schemes(). --- docs/api/core.rst | 2 ++ docs/changelog.rst | 3 +++ mopidy/core/playlists.py | 10 ++++++++++ tests/core/test_playlists.py | 4 ++++ 4 files changed, 19 insertions(+) diff --git a/docs/api/core.rst b/docs/api/core.rst index ead6d651..aaa692d2 100644 --- a/docs/api/core.rst +++ b/docs/api/core.rst @@ -161,6 +161,8 @@ Playlists controller .. class:: mopidy.core.PlaylistsController +.. automethod:: mopidy.core.PlaylistsController.get_uri_schemes + Fetching -------- diff --git a/docs/changelog.rst b/docs/changelog.rst index 49b7f263..1fdd2af3 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -20,6 +20,9 @@ Core API seek target while a seek is in progress. This gives better results than just failing the position query. (Fixes: :issue:`312` PR: :issue:`1346`) +- Add :meth:`mopidy.core.PlaylistsController.get_uri_schemes`. (PR: + :issue:`1362`) + Models ------ diff --git a/mopidy/core/playlists.py b/mopidy/core/playlists.py index e3e2ac20..87790c25 100644 --- a/mopidy/core/playlists.py +++ b/mopidy/core/playlists.py @@ -33,6 +33,16 @@ class PlaylistsController(object): self.backends = backends self.core = core + def get_uri_schemes(self): + """ + Get the list of URI schemes that support playlists. + + :rtype: list of string + + .. versionadded:: 1.2 + """ + return list(sorted(self.backends.with_playlists.keys())) + def as_list(self): """ Get a list of the currently available playlists. diff --git a/tests/core/test_playlists.py b/tests/core/test_playlists.py index 029254a8..c908af6a 100644 --- a/tests/core/test_playlists.py +++ b/tests/core/test_playlists.py @@ -248,6 +248,10 @@ class PlaylistTest(BasePlaylistsTest): self.assertFalse(self.sp1.save.called) self.assertFalse(self.sp2.save.called) + def test_get_uri_schemes(self): + result = self.core.playlists.get_uri_schemes() + self.assertEquals(result, ['dummy1', 'dummy2']) + class DeprecatedFilterPlaylistsTest(BasePlaylistsTest): From 128de6cd015ecdc6c44c9c209484e436b2dab31c Mon Sep 17 00:00:00 2001 From: Bryan Bennett Date: Wed, 23 Dec 2015 11:31:22 -0500 Subject: [PATCH 43/50] Set hostname/name to None Works around mopidy/mopidy#1335 --- mopidy/zeroconf.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/mopidy/zeroconf.py b/mopidy/zeroconf.py index 4ca49b69..7af2f77b 100644 --- a/mopidy/zeroconf.py +++ b/mopidy/zeroconf.py @@ -55,18 +55,20 @@ class Zeroconf(object): self.bus = None self.server = None self.group = None + self.display_hostname = None + self.name = None + try: self.bus = dbus.SystemBus() self.server = dbus.Interface( self.bus.get_object('org.freedesktop.Avahi', '/'), 'org.freedesktop.Avahi.Server') + self.display_hostname = '%s' % self.server.GetHostName() + self.name = string.Template(name).safe_substitute( + hostname=self.display_hostname, port=port) except dbus.exceptions.DBusException as e: logger.debug('%s: Server failed: %s', self, e) - self.display_hostname = '%s' % self.server.GetHostName() - self.name = string.Template(name).safe_substitute( - hostname=self.display_hostname, port=port) - def __str__(self): return 'Zeroconf service "%s" (%s at [%s]:%d)' % ( self.name, self.stype, self.host, self.port) From d210f3223fc9715df0c2494e82302d60ffa2411e Mon Sep 17 00:00:00 2001 From: Bryan Bennett Date: Wed, 23 Dec 2015 11:32:54 -0500 Subject: [PATCH 44/50] Call dbus dependent code only if dbus imported Addresses another symptom of mopidy/mopidy#1335 --- mopidy/zeroconf.py | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/mopidy/zeroconf.py b/mopidy/zeroconf.py index 7af2f77b..9b7b3808 100644 --- a/mopidy/zeroconf.py +++ b/mopidy/zeroconf.py @@ -58,16 +58,17 @@ class Zeroconf(object): self.display_hostname = None self.name = None - try: - self.bus = dbus.SystemBus() - self.server = dbus.Interface( - self.bus.get_object('org.freedesktop.Avahi', '/'), - 'org.freedesktop.Avahi.Server') - self.display_hostname = '%s' % self.server.GetHostName() - self.name = string.Template(name).safe_substitute( - hostname=self.display_hostname, port=port) - except dbus.exceptions.DBusException as e: - logger.debug('%s: Server failed: %s', self, e) + if dbus: + try: + self.bus = dbus.SystemBus() + self.server = dbus.Interface( + self.bus.get_object('org.freedesktop.Avahi', '/'), + 'org.freedesktop.Avahi.Server') + self.display_hostname = '%s' % self.server.GetHostName() + self.name = string.Template(name).safe_substitute( + hostname=self.display_hostname, port=port) + except dbus.exceptions.DBusException as e: + logger.debug('%s: Server failed: %s', self, e) def __str__(self): return 'Zeroconf service "%s" (%s at [%s]:%d)' % ( From 22690ee5a9f22ade9dcc1c7c5b9a607e6dde5ac9 Mon Sep 17 00:00:00 2001 From: Thomas Kemmer Date: Wed, 23 Dec 2015 18:14:19 +0100 Subject: [PATCH 45/50] m3u: Derive track name from file name for non-extended M3U playlists. --- docs/changelog.rst | 6 ++++++ mopidy/m3u/translator.py | 4 +++- tests/data/comment-ext.m3u | 2 +- tests/data/one-ext.m3u | 2 +- tests/data/two-ext.m3u | 4 ++-- tests/m3u/test_translator.py | 18 +++++++++++------- 6 files changed, 24 insertions(+), 12 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 49b7f263..c795eacd 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -39,6 +39,12 @@ Local backend - Made :confval:`local/data_dir` really deprecated. This change breaks older versions of Mopidy-Local-SQLite and Mopidy-Local-Images. +M3U backend +----------- + +- Derive track name from file name for non-extended M3U + playlists. (Fixes: :issue:`1364`, PR: :issue:`1369`) + MPD frontend ------------ diff --git a/mopidy/m3u/translator.py b/mopidy/m3u/translator.py index f60cedfe..764cf84b 100644 --- a/mopidy/m3u/translator.py +++ b/mopidy/m3u/translator.py @@ -99,7 +99,9 @@ def parse_m3u(file_path, media_dir=None): if extended and line.startswith('#EXTINF'): track = m3u_extinf_to_track(line) continue - + if not track.name: + name = os.path.basename(os.path.splitext(line)[0]) + track = track.replace(name=urllib.parse.unquote(name)) if urllib.parse.urlsplit(line).scheme: tracks.append(track.replace(uri=line)) elif os.path.normpath(line) == os.path.abspath(line): diff --git a/tests/data/comment-ext.m3u b/tests/data/comment-ext.m3u index 95983d06..a9b675b8 100644 --- a/tests/data/comment-ext.m3u +++ b/tests/data/comment-ext.m3u @@ -1,5 +1,5 @@ #EXTM3U # test -#EXTINF:-1,song1 +#EXTINF:-1,Song #1 # test song1.mp3 diff --git a/tests/data/one-ext.m3u b/tests/data/one-ext.m3u index 7e94d5e9..a8a51c2f 100644 --- a/tests/data/one-ext.m3u +++ b/tests/data/one-ext.m3u @@ -1,3 +1,3 @@ #EXTM3U -#EXTINF:-1,song1 +#EXTINF:-1,Song #1 song1.mp3 diff --git a/tests/data/two-ext.m3u b/tests/data/two-ext.m3u index c2bf3e75..f50feb94 100644 --- a/tests/data/two-ext.m3u +++ b/tests/data/two-ext.m3u @@ -1,5 +1,5 @@ #EXTM3U -#EXTINF:-1,song1 +#EXTINF:-1,Song #1 song1.mp3 -#EXTINF:60,song2 +#EXTINF:60,Song #2 song2.mp3 diff --git a/tests/m3u/test_translator.py b/tests/m3u/test_translator.py index f1e14301..88387cb3 100644 --- a/tests/m3u/test_translator.py +++ b/tests/m3u/test_translator.py @@ -20,13 +20,15 @@ encoded_path = path_to_data_dir('æøå.mp3') song1_uri = path.path_to_uri(song1_path) song2_uri = path.path_to_uri(song2_path) song3_uri = path.path_to_uri(song3_path) +song4_uri = 'http://example.com/foo%20bar.mp3' encoded_uri = path.path_to_uri(encoded_path) -song1_track = Track(uri=song1_uri) -song2_track = Track(uri=song2_uri) -song3_track = Track(uri=song3_uri) -encoded_track = Track(uri=encoded_uri) -song1_ext_track = song1_track.replace(name='song1') -song2_ext_track = song2_track.replace(name='song2', length=60000) +song1_track = Track(name='song1', uri=song1_uri) +song2_track = Track(name='song2', uri=song2_uri) +song3_track = Track(name='φοο', uri=song3_uri) +song4_track = Track(name='foo bar', uri=song4_uri) +encoded_track = Track(name='æøå', uri=encoded_uri) +song1_ext_track = song1_track.replace(name='Song #1') +song2_ext_track = song2_track.replace(name='Song #2', length=60000) encoded_ext_track = encoded_track.replace(name='æøå') @@ -84,9 +86,11 @@ class M3UToUriTest(unittest.TestCase): def test_file_with_uri(self): with tempfile.NamedTemporaryFile(delete=False) as tmp: tmp.write(song1_uri) + tmp.write('\n') + tmp.write(song4_uri) try: tracks = self.parse(tmp.name) - self.assertEqual([song1_track], tracks) + self.assertEqual([song1_track, song4_track], tracks) finally: if os.path.exists(tmp.name): os.remove(tmp.name) From 3488e6442de65254e47961edb64d3b28f7212b51 Mon Sep 17 00:00:00 2001 From: jcass Date: Sat, 26 Dec 2015 15:28:07 +0200 Subject: [PATCH 46/50] Handle missing or empty 'port' configuration parameter. --- mopidy/httpclient.py | 4 ++-- tests/test_httpclient.py | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/mopidy/httpclient.py b/mopidy/httpclient.py index 682a78bd..6be127ca 100644 --- a/mopidy/httpclient.py +++ b/mopidy/httpclient.py @@ -21,8 +21,8 @@ def format_proxy(proxy_config, auth=True): if not proxy_config.get('hostname'): return None - port = proxy_config.get('port', 80) - if port < 0: + port = proxy_config.get('port') + if not port or port < 0: port = 80 if proxy_config.get('username') and proxy_config.get('password') and auth: diff --git a/tests/test_httpclient.py b/tests/test_httpclient.py index 63591f80..30f03d8d 100644 --- a/tests/test_httpclient.py +++ b/tests/test_httpclient.py @@ -9,6 +9,7 @@ from mopidy import httpclient @pytest.mark.parametrize("config,expected", [ ({}, None), + ({'hostname': ''}, None), ({'hostname': 'proxy.lan'}, 'http://proxy.lan:80'), ({'scheme': None, 'hostname': 'proxy.lan'}, 'http://proxy.lan:80'), ({'scheme': 'https', 'hostname': 'proxy.lan'}, 'https://proxy.lan:80'), @@ -16,6 +17,8 @@ from mopidy import httpclient ({'password': 'pass', 'hostname': 'proxy.lan'}, 'http://proxy.lan:80'), ({'hostname': 'proxy.lan', 'port': 8080}, 'http://proxy.lan:8080'), ({'hostname': 'proxy.lan', 'port': -1}, 'http://proxy.lan:80'), + ({'hostname': 'proxy.lan', 'port': None}, 'http://proxy.lan:80'), + ({'hostname': 'proxy.lan', 'port': ''}, 'http://proxy.lan:80'), ({'username': 'user', 'password': 'pass', 'hostname': 'proxy.lan'}, 'http://user:pass@proxy.lan:80'), ]) From 188bd1110658f49b378eecfaea67028e6145eaf6 Mon Sep 17 00:00:00 2001 From: jcass Date: Sat, 26 Dec 2015 15:36:14 +0200 Subject: [PATCH 47/50] Fix typo. --- docs/config.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/config.rst b/docs/config.rst index 292a6a09..382c860e 100644 --- a/docs/config.rst +++ b/docs/config.rst @@ -204,7 +204,7 @@ Proxy configuration ------------------- Not all parts of Mopidy or all Mopidy extensions respect the proxy -server configuration when connecting to the Internt. Currently, this is at +server configuration when connecting to the Internet. Currently, this is at least used when Mopidy's audio subsystem reads media directly from the network, like when listening to Internet radio streams, and by the Mopidy-Spotify extension. With time, we hope that more of the Mopidy ecosystem will respect From 33a668c6c781284daa803901be6a720470d0252a Mon Sep 17 00:00:00 2001 From: jcass Date: Sat, 26 Dec 2015 18:50:58 +0200 Subject: [PATCH 48/50] Fix documentation typos and inconsistencies. --- docs/clients/mpd.rst | 2 +- docs/ext/frontends.rst | 2 +- docs/ext/mixers.rst | 2 +- docs/ext/web.rst | 18 +++--------------- 4 files changed, 6 insertions(+), 18 deletions(-) diff --git a/docs/clients/mpd.rst b/docs/clients/mpd.rst index c7d6ca7b..b070092a 100644 --- a/docs/clients/mpd.rst +++ b/docs/clients/mpd.rst @@ -167,5 +167,5 @@ projects are a real match made in heaven." Partify ------- -`Partify `_ is a web based MPD client focusing on +`Partify `_ is a web based MPD client focussing on making music playing collaborative and social. diff --git a/docs/ext/frontends.rst b/docs/ext/frontends.rst index 50dc348f..1e2ad3f4 100644 --- a/docs/ext/frontends.rst +++ b/docs/ext/frontends.rst @@ -81,4 +81,4 @@ Mopidy-Webhooks https://github.com/paddycarey/mopidy-webhooks Extension for sending HTTP POST requests with JSON payloads to a remote server -on when Mopidy core triggers an event and on regular intervals. +when Mopidy core triggers an event and on regular intervals. diff --git a/docs/ext/mixers.rst b/docs/ext/mixers.rst index 88fd27dd..5023f285 100644 --- a/docs/ext/mixers.rst +++ b/docs/ext/mixers.rst @@ -17,7 +17,7 @@ Mopidy-ALSAMixer https://github.com/mopidy/mopidy-alsamixer -Extension for controlling volume one a Linux system using ALSA. +Extension for controlling volume on a Linux system using ALSA. Mopidy-Arcam diff --git a/docs/ext/web.rst b/docs/ext/web.rst index bbdf4d0c..a6e2d748 100644 --- a/docs/ext/web.rst +++ b/docs/ext/web.rst @@ -35,8 +35,8 @@ Mopidy-Local-Images https://github.com/tkem/mopidy-local-images -Not a full-featured Web client, but rather a local library and Web -extension which allows other Web clients access to album art embedded +Not a full-featured web client, but rather a local library and web +extension which allows other web clients access to album art embedded in local media files. .. image:: /ext/local_images.jpg @@ -69,7 +69,7 @@ Mopidy-Mobile https://github.com/tkem/mopidy-mobile -A Mopidy Web client extension and hybrid mobile app, made with Ionic, +A Mopidy web client extension and hybrid mobile app, made with Ionic, AngularJS and Apache Cordova by Thomas Kemmer. .. image:: /ext/mobile.png @@ -132,18 +132,6 @@ To install, run:: pip install Mopidy-MusicBox-Webclient -Mopidy-Party -============ - -https://github.com/Lesterpig/mopidy-party - -Minimal web client designed for collaborative music management during parties. - -.. image:: /ext/mopidy_party.png - -To install, run:: - - pip install Mopidy-Party Mopidy-Party ============ From 8ca871cad9b4e22476f46842c59d29fb56d394ce Mon Sep 17 00:00:00 2001 From: jcass Date: Sun, 27 Dec 2015 08:04:32 +0200 Subject: [PATCH 49/50] docs: Provide details on procedure for submitting bug fixes for a minor release of Mopidy. --- docs/contributing.rst | 8 ++++++++ docs/releasing.rst | 1 + 2 files changed, 9 insertions(+) diff --git a/docs/contributing.rst b/docs/contributing.rst index b5230b18..199c6b2a 100644 --- a/docs/contributing.rst +++ b/docs/contributing.rst @@ -126,3 +126,11 @@ Pull request guidelines #. Send a pull request to the ``develop`` branch. See the `GitHub pull request docs `_ for help. + +.. note:: + + If you are contributing a bug fix for a specific minor version of Mopidy + you should create the branch based on ``release-x.y`` instead of + ``develop``. When the release is done the changes will be merged back into + ``develop`` automatically as part of the normal release process. See + :ref:`creating-releases`. diff --git a/docs/releasing.rst b/docs/releasing.rst index 4c2d8373..8d489146 100644 --- a/docs/releasing.rst +++ b/docs/releasing.rst @@ -6,6 +6,7 @@ Here we try to keep an up to date record of how Mopidy releases are made. This documentation serves both as a checklist, to reduce the project's dependency on key individuals, and as a stepping stone to more automation. +.. _creating-releases: Creating releases ================= From 07a0f8ff3ea197464253895fb8d8670e4cc0fccc Mon Sep 17 00:00:00 2001 From: jcass Date: Tue, 29 Dec 2015 07:54:49 +0200 Subject: [PATCH 50/50] test: Test case to ensure that unplayable tracks are skipped over in PAUSE state. Ensures that pause->next->resume handles unplayable tracks just like stop->next->play does. --- tests/core/test_playback.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index 0da59b4d..06d516ac 100644 --- a/tests/core/test_playback.py +++ b/tests/core/test_playback.py @@ -115,6 +115,23 @@ class TestPlayHandling(BaseTest): current_tl_track = self.core.playback.get_current_tl_track() self.assertEqual(tl_tracks[1], current_tl_track) + def test_resume_skips_to_next_on_unplayable_track(self): + """Checks that we handle backend.change_track failing when + resuming playback.""" + tl_tracks = self.core.tracklist.get_tl_tracks() + + self.core.playback.play(tl_tracks[0]) + self.core.playback.pause() + + self.audio.trigger_fake_playback_failure(tl_tracks[1].track.uri) + + self.core.playback.next() + self.core.playback.resume() + self.replay_events() + + current_tl_track = self.core.playback.get_current_tl_track() + self.assertEqual(tl_tracks[2], current_tl_track) + def test_play_tlid(self): tl_tracks = self.core.tracklist.get_tl_tracks()