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

@@ -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.change_track(next_tl_track)
-            self.play(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
 

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)

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)