From 1d108752f64fc31ba96e75b415cda5ffb7a23df8 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Tue, 14 Jan 2014 01:06:10 +0100 Subject: [PATCH 1/3] core: Make events emitted on playback consistent (fixes #629) This commit does not try to make the events correct/perfect with regard to GStreamer states, end-of-stream signalling, etc. It only tries to make the events work consistently across all the methods on the playback controller. * play(track) while already playing has changed from: - playback_state_changed(old_state='playing', new_state='playing') - track_playback_started(track=...) to: - playback_state_changed(old_state='playing', new_state='stopped') - track_playback_ended(track=..., time_position=...) - playback_state_changed(old_state='stopped', new_state='playing') - track_playback_started(track=...) * next() has changed from: - track_playback_ended(track=..., time_position=...) - playback_state_changed(old_state='playing', new_state='stopped') - track_playback_ended(track=..., time_position=0) - playback_state_changed(old_state='stopped', new_state='playing') - track_playback_started(track=...) to same as play() above. * previous() has changed in the same way as next(). * on_end_of_track() has changed from: - track_playback_ended(track=..., time_position=...) - playback_state_changed(old_state='playing', new_state='playing') - track_playback_started(track=...) to same as play() above. * stop() has reordered its events from: - track_playback_ended(track=..., time_position=...) - playback_state_changed(old_state='playing', new_state='stopped') to: - playback_state_changed(old_state='playing', new_state='stopped') - track_playback_ended(track=..., time_position=...) --- mopidy/core/playback.py | 10 +- tests/core/events_test.py | 62 ----------- tests/core/playback_test.py | 210 ++++++++++++++++++++++++++++++++++++ 3 files changed, 215 insertions(+), 67 deletions(-) diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index 96d13017..b2acb35a 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -160,8 +160,7 @@ class PlaybackController(object): next_tl_track = self.core.tracklist.eot_track(original_tl_track) if next_tl_track: - self._trigger_track_playback_ended() - self.play(next_tl_track) + self.change_track(next_tl_track) else: self.stop(clear_current_track=True) @@ -185,7 +184,6 @@ class PlaybackController(object): """ tl_track = self.core.tracklist.next_track(self.current_tl_track) if tl_track: - self._trigger_track_playback_ended() self.change_track(tl_track) else: self.stop(clear_current_track=True) @@ -228,6 +226,9 @@ class PlaybackController(object): assert tl_track in self.core.tracklist.tl_tracks + if self.state == PlaybackState.PLAYING: + self.stop() + self.current_tl_track = tl_track self.state = PlaybackState.PLAYING backend = self._get_backend() @@ -251,7 +252,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() tl_track = self.current_tl_track self.change_track( self.core.tracklist.previous_track(tl_track), on_error_step=-1) @@ -307,8 +307,8 @@ class PlaybackController(object): if self.state != PlaybackState.STOPPED: backend = self._get_backend() if not backend or backend.playback.stop().get(): - self._trigger_track_playback_ended() self.state = PlaybackState.STOPPED + self._trigger_track_playback_ended() if clear_current_track: self.current_tl_track = None diff --git a/tests/core/events_test.py b/tests/core/events_test.py index 17d2eb84..d975ae29 100644 --- a/tests/core/events_test.py +++ b/tests/core/events_test.py @@ -25,59 +25,6 @@ class BackendEventsTest(unittest.TestCase): self.assertEqual(send.call_args[0][0], 'playlists_loaded') - def test_pause_sends_track_playback_paused_event(self, send): - tl_tracks = self.core.tracklist.add([Track(uri='dummy:a')]).get() - self.core.playback.play().get() - send.reset_mock() - - self.core.playback.pause().get() - - self.assertEqual(send.call_args[0][0], 'track_playback_paused') - self.assertEqual(send.call_args[1]['tl_track'], tl_tracks[0]) - self.assertEqual(send.call_args[1]['time_position'], 0) - - def test_resume_sends_track_playback_resumed(self, send): - tl_tracks = self.core.tracklist.add([Track(uri='dummy:a')]).get() - self.core.playback.play() - self.core.playback.pause().get() - send.reset_mock() - - self.core.playback.resume().get() - - self.assertEqual(send.call_args[0][0], 'track_playback_resumed') - self.assertEqual(send.call_args[1]['tl_track'], tl_tracks[0]) - self.assertEqual(send.call_args[1]['time_position'], 0) - - def test_play_sends_track_playback_started_event(self, send): - tl_tracks = self.core.tracklist.add([Track(uri='dummy:a')]).get() - send.reset_mock() - - self.core.playback.play().get() - - self.assertEqual(send.call_args[0][0], 'track_playback_started') - self.assertEqual(send.call_args[1]['tl_track'], tl_tracks[0]) - - def test_stop_sends_track_playback_ended_event(self, send): - tl_tracks = self.core.tracklist.add([Track(uri='dummy:a')]).get() - self.core.playback.play().get() - send.reset_mock() - - self.core.playback.stop().get() - - self.assertEqual(send.call_args_list[0][0][0], 'track_playback_ended') - self.assertEqual(send.call_args_list[0][1]['tl_track'], tl_tracks[0]) - self.assertEqual(send.call_args_list[0][1]['time_position'], 0) - - def test_seek_sends_seeked_event(self, send): - self.core.tracklist.add([Track(uri='dummy:a', length=40000)]) - self.core.playback.play().get() - send.reset_mock() - - self.core.playback.seek(1000).get() - - self.assertEqual(send.call_args[0][0], 'seeked') - self.assertEqual(send.call_args[1]['time_position'], 1000) - def test_tracklist_add_sends_tracklist_changed_event(self, send): send.reset_mock() @@ -153,12 +100,3 @@ class BackendEventsTest(unittest.TestCase): self.core.playlists.save(playlist).get() self.assertEqual(send.call_args[0][0], 'playlist_changed') - - def test_set_volume_sends_volume_changed_event(self, send): - self.core.playback.set_volume(10).get() - send.reset_mock() - - self.core.playback.set_volume(20).get() - - self.assertEqual(send.call_args[0][0], 'volume_changed') - self.assertEqual(send.call_args[1]['volume'], 20) diff --git a/tests/core/playback_test.py b/tests/core/playback_test.py index 806de40e..6796cfe7 100644 --- a/tests/core/playback_test.py +++ b/tests/core/playback_test.py @@ -12,11 +12,15 @@ class CorePlaybackTest(unittest.TestCase): self.backend1 = mock.Mock() self.backend1.uri_schemes.get.return_value = ['dummy1'] self.playback1 = mock.Mock(spec=backend.PlaybackProvider) + self.playback1.get_time_position().get.return_value = 1000 + self.playback1.reset_mock() self.backend1.playback = self.playback1 self.backend2 = mock.Mock() self.backend2.uri_schemes.get.return_value = ['dummy2'] self.playback2 = mock.Mock(spec=backend.PlaybackProvider) + self.playback2.get_time_position().get.return_value = 2000 + self.playback2.reset_mock() self.backend2.playback = self.playback2 # A backend without the optional playback provider @@ -38,6 +42,12 @@ class CorePlaybackTest(unittest.TestCase): self.tl_tracks = self.core.tracklist.tl_tracks self.unplayable_tl_track = self.tl_tracks[2] + # TODO Test get_current_tl_track + + # TODO Test get_current_track + + # TODO Test state + def test_play_selects_dummy1_backend(self): self.core.playback.play(self.tl_tracks[0]) @@ -59,6 +69,45 @@ class CorePlaybackTest(unittest.TestCase): self.assertEqual( self.core.playback.current_tl_track, self.tl_tracks[3]) + @mock.patch( + 'mopidy.core.playback.listener.CoreListener', spec=core.CoreListener) + def test_play_when_stopped_emits_events(self, listener_mock): + self.core.playback.play(self.tl_tracks[0]) + + 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=self.tl_tracks[0]), + ]) + + @mock.patch( + 'mopidy.core.playback.listener.CoreListener', spec=core.CoreListener) + def test_play_when_playing_emits_events(self, listener_mock): + self.core.playback.play(self.tl_tracks[0]) + listener_mock.reset_mock() + + self.core.playback.play(self.tl_tracks[3]) + + 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[0], time_position=1000), + mock.call( + 'playback_state_changed', + old_state='stopped', new_state='playing'), + mock.call( + 'track_playback_started', tl_track=self.tl_tracks[3]), + ]) + def test_pause_selects_dummy1_backend(self): self.core.playback.play(self.tl_tracks[0]) self.core.playback.pause() @@ -81,6 +130,25 @@ class CorePlaybackTest(unittest.TestCase): self.assertFalse(self.playback1.pause.called) self.assertFalse(self.playback2.pause.called) + @mock.patch( + 'mopidy.core.playback.listener.CoreListener', spec=core.CoreListener) + def test_pause_emits_events(self, listener_mock): + self.core.playback.play(self.tl_tracks[0]) + listener_mock.reset_mock() + + self.core.playback.pause() + + self.assertListEqual( + listener_mock.send.mock_calls, + [ + mock.call( + 'playback_state_changed', + old_state='playing', new_state='paused'), + mock.call( + 'track_playback_paused', + tl_track=self.tl_tracks[0], time_position=1000), + ]) + def test_resume_selects_dummy1_backend(self): self.core.playback.play(self.tl_tracks[0]) self.core.playback.pause() @@ -106,6 +174,26 @@ class CorePlaybackTest(unittest.TestCase): self.assertFalse(self.playback1.resume.called) self.assertFalse(self.playback2.resume.called) + @mock.patch( + 'mopidy.core.playback.listener.CoreListener', spec=core.CoreListener) + def test_resume_emits_events(self, listener_mock): + self.core.playback.play(self.tl_tracks[0]) + self.core.playback.pause() + listener_mock.reset_mock() + + self.core.playback.resume() + + self.assertListEqual( + listener_mock.send.mock_calls, + [ + mock.call( + 'playback_state_changed', + old_state='paused', new_state='playing'), + mock.call( + 'track_playback_resumed', + tl_track=self.tl_tracks[0], time_position=1000), + ]) + def test_stop_selects_dummy1_backend(self): self.core.playback.play(self.tl_tracks[0]) self.core.playback.stop() @@ -129,6 +217,103 @@ class CorePlaybackTest(unittest.TestCase): self.assertFalse(self.playback1.stop.called) self.assertFalse(self.playback2.stop.called) + @mock.patch( + 'mopidy.core.playback.listener.CoreListener', spec=core.CoreListener) + def test_stop_emits_events(self, listener_mock): + self.core.playback.play(self.tl_tracks[0]) + listener_mock.reset_mock() + + self.core.playback.stop() + + 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[0], time_position=1000), + ]) + + # TODO Test next() more + + @mock.patch( + 'mopidy.core.playback.listener.CoreListener', spec=core.CoreListener) + def test_next_emits_events(self, listener_mock): + self.core.playback.play(self.tl_tracks[0]) + listener_mock.reset_mock() + + self.core.playback.next() + + 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[0], 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[1]), + ]) + + # TODO Test previous() more + + @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]), + ]) + + # TODO Test on_end_of_track() more + + @mock.patch( + 'mopidy.core.playback.listener.CoreListener', spec=core.CoreListener) + def test_on_end_of_track_emits_events(self, listener_mock): + self.core.playback.play(self.tl_tracks[0]) + listener_mock.reset_mock() + + self.core.playback.on_end_of_track() + + 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[0], 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[1]), + ]) + def test_seek_selects_dummy1_backend(self): self.core.playback.play(self.tl_tracks[0]) self.core.playback.seek(10000) @@ -152,6 +337,17 @@ class CorePlaybackTest(unittest.TestCase): self.assertFalse(self.playback1.seek.called) self.assertFalse(self.playback2.seek.called) + @mock.patch( + 'mopidy.core.playback.listener.CoreListener', spec=core.CoreListener) + def test_seek_emits_seeked_event(self, listener_mock): + self.core.playback.play(self.tl_tracks[0]) + listener_mock.reset_mock() + + self.core.playback.seek(1000) + + listener_mock.send.assert_called_once_with( + 'seeked', time_position=1000) + def test_time_position_selects_dummy1_backend(self): self.core.playback.play(self.tl_tracks[0]) self.core.playback.seek(10000) @@ -177,6 +373,20 @@ class CorePlaybackTest(unittest.TestCase): self.assertFalse(self.playback1.get_time_position.called) self.assertFalse(self.playback2.get_time_position.called) + # TODO Test on_tracklist_change + + # TODO Test volume + + @mock.patch( + 'mopidy.core.playback.listener.CoreListener', spec=core.CoreListener) + def test_set_volume_emits_volume_changed_event(self, listener_mock): + self.core.playback.set_volume(10) + listener_mock.reset_mock() + + self.core.playback.set_volume(20) + + listener_mock.send.assert_called_once_with('volume_changed', volume=20) + def test_mute(self): self.assertEqual(self.core.playback.mute, False) From d1630e00f1b5fb341181550c9c63c3ddeae74e2b Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Tue, 14 Jan 2014 01:24:26 +0100 Subject: [PATCH 2/3] docs: Update changelog --- docs/changelog.rst | 4 ++++ docs/conf.py | 1 + 2 files changed, 5 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index 64f4848d..fb7fa12a 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -20,6 +20,10 @@ v0.18.0 (UNRELEASED) virtual file system of tracks. Backends can implement support for this by implementing :meth:`mopidy.backends.base.BaseLibraryController.browse`. +- Events emitted on play/stop, pause/resume, next/previous and on end of track + has been cleaned up to work consistenly. See the message of + :commit:`1d108752f6` for the full details. + **Backend API** - Move the backend API classes from :mod:`mopidy.backends.base` to diff --git a/docs/conf.py b/docs/conf.py index bb9b7c2f..737fb07a 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -161,6 +161,7 @@ man_pages = [ extlinks = { 'issue': ('https://github.com/mopidy/mopidy/issues/%s', '#'), + 'commit': ('https://github.com/mopidy/mopidy/commit/%s', 'commit '), 'mpris': ( 'https://github.com/mopidy/mopidy-mpris/issues/%s', 'mopidy-mpris#'), } From d96ea03ac168f1fa1221acde91fd7f48e821a54e Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Tue, 14 Jan 2014 10:07:54 +0100 Subject: [PATCH 3/3] docs: Add issue ref to changelog --- docs/changelog.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index fb7fa12a..3ca2eb6a 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -22,7 +22,7 @@ v0.18.0 (UNRELEASED) - Events emitted on play/stop, pause/resume, next/previous and on end of track has been cleaned up to work consistenly. See the message of - :commit:`1d108752f6` for the full details. + :commit:`1d108752f6` for the full details. (Fixes: :issue:`629`) **Backend API**