Merge pull request #1064 from adamcik/fix/1052-break-backend-play

backend: Change playback API (breaking change!)

docs/changelog.rst

@@ -75,6 +75,25 @@ v1.0.0 (UNRELEASED)
   :attr:`mopidy.backend.PlaylistsProvider.playlists`. This is potentially
   backwards incompatible. (PR: :issue:`1046`)
 
+- Changed the API for :class:`mopidy.backend.PlaybackProvider`, note that this
+  change is **not** backwards compatible for certain backends. These changes
+  are crucial to adding gapless in one of the upcoming releases.
+  (Fixes: :issue:`1052`, PR: :issue:`1064`)
+
+  - :meth:`mopidy.backend.PlaybackProvider.translate_uri` has been added. It is
+    strongly recommended that all backends migrate to using this API for
+    translating "Mopidy URIs" to real ones for playback.
+
+  - The semantics and signature of :meth:`mopidy.backend.PlaybackProvider.play`
+    has changed. The method is now only used to set the playback state to
+    playing, and no longer takes a track.
+
+    Backends must migrate to
+    :meth:`mopidy.backend.PlaybackProvider.translate_uri` or
+    :meth:`mopidy.backend.PlaybackProvider.change_track` to continue working.
+
+  - :meth:`mopidy.backend.PlaybackProvider.prepare_change` has been added.
+
 **Commands**
 
 - Make the ``mopidy`` command print a friendly error message if the

mopidy/backend.py

@@ -177,31 +177,66 @@ 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 translate_uri(self, uri):
+        """
+        Convert custom URI scheme to real playable uri.
+
+        *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 URI
+        to a real URI and then return it. If you can't convert the URI just
+        return :class:`None`.
+
+        :param uri: the URI to translate.
+        :type uri: string
+        :rtype: string or :class:`None` if the URI could not be translated.
+        """
+        return uri
+
     def change_track(self, track):
         """
         Swith to provided track.
 
         *MAY be reimplemented by subclass.*
 
+        It is 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.
+
+        The default implementation will call :method:`translate_uri` which
+        is what you want to implement.
+
         :param track: the track to play
         :type track: :class:`mopidy.models.Track`
         :rtype: :class:`True` if successful, else :class:`False`
         """
-        self.audio.set_uri(track.uri).get()
+        uri = self.translate_uri(track.uri)
+        if not uri:
+            return False
+        self.audio.set_uri(uri).get()
         return True
 
     def resume(self):
@@ -232,6 +267,9 @@ class PlaybackProvider(object):
 
         *MAY be reimplemented by subclass.*
 
+        Should not be used for tracking if tracks have been played or 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)

mopidy/local/playback.py

@@ -1,16 +1,10 @@
 from __future__ import absolute_import, unicode_literals
 
-import logging
-
 from mopidy import backend
 from mopidy.local import translator
 
 
-logger = logging.getLogger(__name__)
-
-
 class LocalPlaybackProvider(backend.PlaybackProvider):
-    def change_track(self, track):
-        track = track.copy(uri=translator.local_track_uri_to_file_uri(
-            track.uri, self.backend.config['local']['media_dir']))
-        return super(LocalPlaybackProvider, self).change_track(track)
+    def translate_uri(self, uri):
+        return translator.local_track_uri_to_file_uri(
+            uri, self.backend.config['local']['media_dir'])

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()