diff --git a/docs/changelog.rst b/docs/changelog.rst index da6e6bd7..eea122f9 100644 --- a/docs/changelog.rst +++ b/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 diff --git a/mopidy/backend.py b/mopidy/backend.py index 7e020b77..ffefe047 100644 --- a/mopidy/backend.py +++ b/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() diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index 0b714598..4f51f328 100644 --- a/mopidy/core/playback.py +++ b/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) diff --git a/mopidy/local/playback.py b/mopidy/local/playback.py index 92dc6e15..82f27fdd 100644 --- a/mopidy/local/playback.py +++ b/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']) diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index e6dc3ce1..e84e7301 100644 --- a/tests/core/test_playback.py +++ b/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( diff --git a/tests/dummy_backend.py b/tests/dummy_backend.py index d0816096..d4441673 100644 --- a/tests/dummy_backend.py +++ b/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): diff --git a/tests/local/test_playback.py b/tests/local/test_playback.py index 3ccd8d8f..4c4ded24 100644 --- a/tests/local/test_playback.py +++ b/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()