From efe9430c7af630dd36df52677ea6834100c11af0 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sun, 22 Mar 2015 22:12:51 +0100 Subject: [PATCH 1/3] core: Update playback code to take change track into account. This change has us checking the return value of change_track when deciding if the play call was a success or if the track is unplayable. Which ensures that the following can no longer happen: 1) play stream 2) play stream that fails change_track 3) stream 1) continues playing. Correct behavior being the next stream playing instead. --- mopidy/core/playback.py | 4 ++-- tests/core/test_playback.py | 19 ++++++++++++++++++- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index 4f51f328..d5736808 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -312,8 +312,8 @@ class PlaybackController(object): if backend: backend.playback.prepare_change() - backend.playback.change_track(tl_track.track) - success = backend.playback.play().get() + success = (backend.playback.change_track(tl_track.track).get() and + backend.playback.play().get()) if success: self.core.tracklist._mark_playing(tl_track) diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index e84e7301..5f305c4e 100644 --- a/tests/core/test_playback.py +++ b/tests/core/test_playback.py @@ -12,6 +12,7 @@ from mopidy.models import Track from tests import dummy_audio as audio +# TODO: split into smaller easier to follow tests. setup is way to complex. class CorePlaybackTest(unittest.TestCase): def setUp(self): # noqa: N802 self.backend1 = mock.Mock() @@ -113,7 +114,7 @@ class CorePlaybackTest(unittest.TestCase): self.playback2.change_track.assert_called_once_with(self.tracks[1]) self.playback2.play.assert_called_once_with() - def test_play_skips_to_next_on_unplayable_track(self): + def test_play_skips_to_next_on_track_without_playback_backend(self): self.core.playback.play(self.unplayable_tl_track) self.playback1.prepare_change.assert_called_once_with() @@ -124,6 +125,22 @@ class CorePlaybackTest(unittest.TestCase): self.assertEqual( self.core.playback.current_tl_track, self.tl_tracks[3]) + def test_play_skips_to_next_on_unplayable_track(self): + """Checks that we handle change track failing.""" + self.playback2.change_track().get.return_value = False + + self.core.tracklist.clear() + self.core.tracklist.add(self.tracks[:2]) + tl_tracks = self.core.tracklist.tl_tracks + + self.core.playback.play(tl_tracks[0]) + self.core.playback.play(tl_tracks[1]) + + # TODO: we really want to check that the track was marked unplayable + # and that next was called. This is just an indirect way of checking + # this :( + self.assertEqual(self.core.playback.state, core.PlaybackState.STOPPED) + @mock.patch( 'mopidy.core.playback.listener.CoreListener', spec=core.CoreListener) def test_play_when_stopped_emits_events(self, listener_mock): From a3e295026ac312c37e5d456e20a478164191da36 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sun, 22 Mar 2015 22:37:47 +0100 Subject: [PATCH 2/3] docs: Add changelog for core play behaviour change --- docs/changelog.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index eea122f9..91c83006 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -69,6 +69,10 @@ v1.0.0 (UNRELEASED) - :meth:`mopidy.core.TracklistController.mark_playing` - :meth:`mopidy.core.TracklistController.mark_unplayable` +- Updated :meth:`mopidy.core.PlaybackController.play` to take + :meth:`mopidy.backend.PlaybackProvider.change_track` into account when + determining success. (PR: :issue:`1071`) + **Backend API** - Remove default implementation of From 28f8a9909089f1f65feacb07579fb62d92824fd8 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sun, 22 Mar 2015 23:14:29 +0100 Subject: [PATCH 3/3] review: Fixed mock use and docstring --- tests/core/test_playback.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index 5f305c4e..3a665d85 100644 --- a/tests/core/test_playback.py +++ b/tests/core/test_playback.py @@ -126,8 +126,8 @@ class CorePlaybackTest(unittest.TestCase): self.core.playback.current_tl_track, self.tl_tracks[3]) def test_play_skips_to_next_on_unplayable_track(self): - """Checks that we handle change track failing.""" - self.playback2.change_track().get.return_value = False + """Checks that we handle backend.change_track failing.""" + self.playback2.change_track.return_value.get.return_value = False self.core.tracklist.clear() self.core.tracklist.add(self.tracks[:2])