From 592b728e322f97805f74f041a6087262c8942c16 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Fri, 4 Sep 2015 17:16:39 +0200 Subject: [PATCH] core: Refactor previous() to use pending_track for state changes --- mopidy/core/playback.py | 31 +++------ tests/core/test_playback.py | 129 +++++++++++++++++++---------------- tests/local/test_playback.py | 15 ++-- 3 files changed, 90 insertions(+), 85 deletions(-) diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index 995b76fa..9cbbf874 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -395,28 +395,19 @@ 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. """ - original_tl_track = self.get_current_tl_track() - prev_tl_track = self.core.tracklist.previous_track(original_tl_track) + state = self.get_state() + current = self._pending_tl_track or self._current_tl_track - backend = self._get_backend(prev_tl_track) - self._set_current_tl_track(prev_tl_track) - - if backend: - backend.playback.prepare_change() - # TODO: check return values of change track - backend.playback.change_track(prev_tl_track.track) - if self.get_state() == PlaybackState.PLAYING: - result = backend.playback.play().get() - elif self.get_state() == PlaybackState.PAUSED: - result = backend.playback.pause().get() + while current: + pending = self.core.tracklist.previous_track(current) + if self._change(pending, state): + break else: - result = True - - if result and self.get_state() != PlaybackState.PAUSED: - self._trigger_track_playback_started() - elif not result: - self.core.tracklist._mark_unplayable(prev_tl_track) - self.previous() + self.core.tracklist._mark_unplayable(pending) + # TODO: this could be needed to prevent a loop in rare cases + # if current == pending: + # break + current = pending # TODO: no return value? diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index 54f3a170..5880ace3 100644 --- a/tests/core/test_playback.py +++ b/tests/core/test_playback.py @@ -13,6 +13,7 @@ from mopidy.models import Track from tests import dummy_audio +# TODO: Replace this with dummy_backend no that it uses a real playbackprovider # Since we rely on our DummyAudio to actually emit events we need a "real" # backend and not a mock so the right calls make it through to audio. class TestBackend(pykka.ThreadingActor, backend.Backend): @@ -99,6 +100,76 @@ class TestNextHandling(PlaybackBaseTest): self.assertEqual(current_track, self.tracks[1]) +class TestPreviousHandling(PlaybackBaseTest): + # TODO Test previous() more + + def test_get_current_tl_track_prev(self): + tl_tracks = self.core.tracklist.get_tl_tracks() + + self.core.playback.play(tl_tracks[1]) + self.core.playback.previous() + self.replay_events() + + self.assertEqual( + self.core.playback.get_current_tl_track(), tl_tracks[0]) + + def test_get_current_track_prev(self): + tl_tracks = self.core.tracklist.get_tl_tracks() + + self.core.playback.play(tl_tracks[1]) + self.core.playback.previous() + self.replay_events() + + self.assertEqual( + self.core.playback.get_current_track(), self.tracks[0]) + + def test_previous_keeps_finished_track_in_tracklist(self): + tl_tracks = self.core.tracklist.get_tl_tracks() + + self.core.playback.play(tl_tracks[1]) + + self.core.playback.previous() + self.replay_events() + + self.assertIn(tl_tracks[1], self.core.tracklist.tl_tracks) + + def test_previous_keeps_finished_track_even_in_consume_mode(self): + tl_tracks = self.core.tracklist.get_tl_tracks() + + self.core.playback.play(tl_tracks[1]) + self.core.tracklist.consume = True + + self.core.playback.previous() + self.replay_events() + + self.assertIn(tl_tracks[1], self.core.tracklist.tl_tracks) + + @unittest.skip('Currently tests wrong events, and nothing generates them.') + @mock.patch( + 'mopidy.core.playback.listener.CoreListener', spec=core.CoreListener) + def test_previous_emits_events(self, listener_mock): + self.core.playback.play(self.tl_tracks[1]) + listener_mock.reset_mock() + + self.core.playback.previous() + + self.assertListEqual( + listener_mock.send.mock_calls, + [ + mock.call( + 'playback_state_changed', + old_state='playing', new_state='stopped'), + mock.call( + 'track_playback_ended', + tl_track=self.tl_tracks[1], time_position=mock.ANY), + mock.call( + 'playback_state_changed', + old_state='stopped', new_state='playing'), + mock.call( + 'track_playback_started', tl_track=self.tl_tracks[0]), + ]) + + class TestPlayUnknownHanlding(PlaybackBaseTest): tracks = [Track(uri='unknown:a', length=1234), @@ -286,26 +357,12 @@ class CorePlaybackTest(unittest.TestCase): self.assertEqual( self.core.playback.get_current_tl_track(), self.tl_tracks[0]) - def test_get_current_tl_track_prev(self): - self.core.playback.play(self.tl_tracks[1]) - self.core.playback.previous() - - self.assertEqual( - self.core.playback.get_current_tl_track(), self.tl_tracks[0]) - def test_get_current_track_play(self): self.core.playback.play(self.tl_tracks[0]) self.assertEqual( self.core.playback.get_current_track(), self.tracks[0]) - def test_get_current_track_prev(self): - self.core.playback.play(self.tl_tracks[1]) - self.core.playback.previous() - - self.assertEqual( - self.core.playback.get_current_track(), self.tracks[0]) - def test_get_current_tlid_none(self): self.set_current_tl_track(None) @@ -572,50 +629,6 @@ class CorePlaybackTest(unittest.TestCase): 'track_playback_started', tl_track=self.tl_tracks[1]), ]) - # TODO Test previous() more - - def test_previous_keeps_finished_track_in_tracklist(self): - tl_track = self.tl_tracks[1] - self.core.playback.play(tl_track) - - self.core.playback.previous() - - self.assertIn(tl_track, self.core.tracklist.tl_tracks) - - def test_previous_keeps_finished_track_even_in_consume_mode(self): - tl_track = self.tl_tracks[1] - self.core.playback.play(tl_track) - self.core.tracklist.consume = True - - self.core.playback.previous() - - self.assertIn(tl_track, self.core.tracklist.tl_tracks) - - @unittest.skip('Currently tests wrong events, and nothing generates them.') - @mock.patch( - 'mopidy.core.playback.listener.CoreListener', spec=core.CoreListener) - def test_previous_emits_events(self, listener_mock): - self.core.playback.play(self.tl_tracks[1]) - listener_mock.reset_mock() - - self.core.playback.previous() - - self.assertListEqual( - listener_mock.send.mock_calls, - [ - mock.call( - 'playback_state_changed', - old_state='playing', new_state='stopped'), - mock.call( - 'track_playback_ended', - tl_track=self.tl_tracks[1], time_position=mock.ANY), - mock.call( - 'playback_state_changed', - old_state='stopped', new_state='playing'), - mock.call( - 'track_playback_started', tl_track=self.tl_tracks[0]), - ]) - def test_on_about_to_finish_keeps_finished_track_in_tracklist(self): tl_track = self.tl_tracks[0] self.core.playback.play(tl_track) diff --git a/tests/local/test_playback.py b/tests/local/test_playback.py index e149ba49..490ed599 100644 --- a/tests/local/test_playback.py +++ b/tests/local/test_playback.py @@ -227,7 +227,7 @@ class LocalPlaybackProviderTest(unittest.TestCase): def test_previous(self): self.playback.play() self.playback.next().get() - self.playback.previous() + self.playback.previous().get() self.assert_current_track_is(self.tracks[0]) @populate_tracklist @@ -235,7 +235,7 @@ class LocalPlaybackProviderTest(unittest.TestCase): self.playback.play() # At track 0 self.playback.next().get() # At track 1 self.playback.next().get() # At track 2 - self.playback.previous() # At track 1 + self.playback.previous().get() # At track 1 self.assert_current_track_is(self.tracks[1]) @populate_tracklist @@ -266,11 +266,12 @@ class LocalPlaybackProviderTest(unittest.TestCase): @populate_tracklist def test_previous_skips_to_previous_track_on_failure(self): # If backend's play() returns False, it is a failure. - return_values = [True, False, True] - self.backend.playback.play = lambda: return_values.pop() + uri = self.backend.playback.translate_uri(self.tracks[1].uri).get() + self.audio.trigger_fake_playback_failure(uri) + self.playback.play(self.tl_tracks.get()[2]) self.assert_current_track_is(self.tracks[2]) - self.playback.previous() + self.playback.previous().get() self.assert_current_track_is_not(self.tracks[1]) self.assert_current_track_is(self.tracks[0]) @@ -352,7 +353,7 @@ class LocalPlaybackProviderTest(unittest.TestCase): def test_next_track_after_previous(self): self.playback.play() self.playback.next().get() - self.playback.previous() + self.playback.previous().get() self.assert_next_tl_track_is(self.tl_tracks.get()[1]) def test_next_track_empty_playlist(self): @@ -610,7 +611,7 @@ class LocalPlaybackProviderTest(unittest.TestCase): self.playback.play() # At track 0 self.playback.next().get() # At track 1 self.playback.next().get() # At track 2 - self.playback.previous() # At track 1 + self.playback.previous().get() # At track 1 self.assert_previous_tl_track_is(self.tl_tracks.get()[0]) def test_previous_track_empty_playlist(self):