backend: Change playback API (breaking change!)

While trying to remove traces of stop calls in core to get gapless working I
found we had no way to switch to switch tracks without triggering a play. This
change fixes this by changing the backends playback provider API.

- play() now _only_ starts playback and does not take any arguments.
- prepare_change() has been added, this could have been avoided with a kwarg to
  change_track(track), but that would break more backends.
- core has been updated to call prepare_change+change_track+play as needed.
- tests have been updated to handle this change.

Longer term I hope to completely rework the playback API in backends, as 99% of
our backends only use change_track(track) to translate URIs. So we should make
simple case simple, and handle mopidy-spotify / appsrc in some other way.

Cherry picked from the WIP gapless branch.

mopidy/backend.py

@@ -177,26 +177,40 @@ class PlaybackProvider(object):
         """
         return self.audio.pause_playback().get()
 
-    def play(self, track):
+    def play(self):
         """
-        Play given track.
+        Start playback.
 
         *MAY be reimplemented by subclass.*
 
-        :param track: the track to play
-        :type track: :class:`mopidy.models.Track`
         :rtype: :class:`True` if successful, else :class:`False`
         """
-        self.audio.prepare_change()
-        self.change_track(track)
         return self.audio.start_playback().get()
 
+    def prepare_change(self):
+        """
+        Indicate that an URI change is about to happen.
+
+        *MAY be reimplemented by subclass.*
+
+        It is extremely unlikely it makes sense for any backends to override
+        this. For most practical purposes it should be considered an internal
+        call between backends and core that backend authors should not touch.
+        """
+        self.audio.prepare_change().get()
+
     def change_track(self, track):
         """
         Swith to provided track.
 
         *MAY be reimplemented by subclass.*
 
+        This is very likely the *only* thing you need to override as a backend
+        author. Typically this is where you convert any mopidy specific URIs
+        to real URIs and then return::
+
+            return super(MyBackend, self).change_track(track.copy(uri=new_uri))
+
         :param track: the track to play
         :type track: :class:`mopidy.models.Track`
         :rtype: :class:`True` if successful, else :class:`False`
@@ -232,6 +246,9 @@ class PlaybackProvider(object):
 
         *MAY be reimplemented by subclass.*
 
+        Should not be used for tracking if tracks have been played / when we
+        are done playing them.
+
         :rtype: :class:`True` if successful, else :class:`False`
         """
         return self.audio.stop_playback().get()

mopidy/core/playback.py

@@ -308,7 +308,12 @@ class PlaybackController(object):
         self.set_current_tl_track(tl_track)
         self.set_state(PlaybackState.PLAYING)
         backend = self._get_backend()
-        success = backend and backend.playback.play(tl_track.track).get()
+        success = False
+
+        if backend:
+            backend.playback.prepare_change()
+            backend.playback.change_track(tl_track.track)
+            success = backend.playback.play().get()
 
         if success:
             self.core.tracklist._mark_playing(tl_track)

tests/core/test_playback.py

@@ -100,19 +100,25 @@ class CorePlaybackTest(unittest.TestCase):
     def test_play_selects_dummy1_backend(self):
         self.core.playback.play(self.tl_tracks[0])
 
-        self.playback1.play.assert_called_once_with(self.tracks[0])
+        self.playback1.prepare_change.assert_called_once_with()
+        self.playback1.change_track.assert_called_once_with(self.tracks[0])
+        self.playback1.play.assert_called_once_with()
         self.assertFalse(self.playback2.play.called)
 
     def test_play_selects_dummy2_backend(self):
         self.core.playback.play(self.tl_tracks[1])
 
         self.assertFalse(self.playback1.play.called)
-        self.playback2.play.assert_called_once_with(self.tracks[1])
+        self.playback2.prepare_change.assert_called_once_with()
+        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):
         self.core.playback.play(self.unplayable_tl_track)
 
-        self.playback1.play.assert_called_once_with(self.tracks[3])
+        self.playback1.prepare_change.assert_called_once_with()
+        self.playback1.change_track.assert_called_once_with(self.tracks[3])
+        self.playback1.play.assert_called_once_with()
         self.assertFalse(self.playback2.play.called)
 
         self.assertEqual(

tests/dummy_backend.py

@@ -62,15 +62,23 @@ class DummyLibraryProvider(backend.LibraryProvider):
 class DummyPlaybackProvider(backend.PlaybackProvider):
     def __init__(self, *args, **kwargs):
         super(DummyPlaybackProvider, self).__init__(*args, **kwargs)
+        self._uri = None
         self._time_position = 0
 
     def pause(self):
         return True
 
-    def play(self, track):
+    def play(self):
+        return self._uri and self._uri != 'dummy:error'
+
+    def change_track(self, track):
         """Pass a track with URI 'dummy:error' to force failure"""
+        self._uri = track.uri
         self._time_position = 0
-        return track.uri != 'dummy:error'
+        return True
+
+    def prepare_change(self):
+        pass
 
     def resume(self):
         return True
@@ -80,6 +88,7 @@ class DummyPlaybackProvider(backend.PlaybackProvider):
         return True
 
     def stop(self):
+        self._uri = None
         return True
 
     def get_time_position(self):

tests/local/test_playback.py

@@ -154,7 +154,8 @@ class LocalPlaybackProviderTest(unittest.TestCase):
     @populate_tracklist
     def test_play_skips_to_next_track_on_failure(self):
         # If backend's play() returns False, it is a failure.
-        self.backend.playback.play = lambda track: track != self.tracks[0]
+        return_values = [True, False]
+        self.backend.playback.play = lambda: return_values.pop()
         self.playback.play()
         self.assertNotEqual(self.playback.current_track, self.tracks[0])
         self.assertEqual(self.playback.current_track, self.tracks[1])
@@ -214,7 +215,8 @@ 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.
-        self.backend.playback.play = lambda track: track != self.tracks[1]
+        return_values = [True, False, True]
+        self.backend.playback.play = lambda: return_values.pop()
         self.playback.play(self.tracklist.tl_tracks[2])
         self.assertEqual(self.playback.current_track, self.tracks[2])
         self.playback.previous()
@@ -281,7 +283,8 @@ class LocalPlaybackProviderTest(unittest.TestCase):
     @populate_tracklist
     def test_next_skips_to_next_track_on_failure(self):
         # If backend's play() returns False, it is a failure.
-        self.backend.playback.play = lambda track: track != self.tracks[1]
+        return_values = [True, False, True]
+        self.backend.playback.play = lambda: return_values.pop()
         self.playback.play()
         self.assertEqual(self.playback.current_track, self.tracks[0])
         self.playback.next()
@@ -455,7 +458,8 @@ class LocalPlaybackProviderTest(unittest.TestCase):
     @populate_tracklist
     def test_end_of_track_skips_to_next_track_on_failure(self):
         # If backend's play() returns False, it is a failure.
-        self.backend.playback.play = lambda track: track != self.tracks[1]
+        return_values = [True, False, True]
+        self.backend.playback.play = lambda: return_values.pop()
         self.playback.play()
         self.assertEqual(self.playback.current_track, self.tracks[0])
         self.playback.on_end_of_track()