New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

backend: Change playback API (breaking change!) #1064

Merged
merged 5 commits into from Mar 22, 2015

Conversation

2 participants
@adamcik
Member

adamcik commented Mar 20, 2015

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.

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.
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

This comment has been minimized.

@jodal

jodal Mar 20, 2015

Member

s/mopidy/Mopidy/

@@ -232,6 +246,9 @@ def stop(self):
*MAY be reimplemented by subclass.*
Should not be used for tracking if tracks have been played / when we

This comment has been minimized.

@jodal

jodal Mar 20, 2015

Member

s#/#or#

@jodal

This comment has been minimized.

Member

jodal commented Mar 20, 2015

Otherwise looking good. If you're rebasing anyway, do s/to switch to switch/to switch/ on the commit message.

@jodal jodal added this to the v1.0 - Audio cleanup 1 milestone Mar 20, 2015

@jodal jodal self-assigned this Mar 20, 2015

@jodal jodal added the 2 - Working label Mar 20, 2015

@jodal

This comment has been minimized.

Member

jodal commented Mar 20, 2015

Oh, and a changelog entry!

backend: Add translate_uri for simpler API for the simple case.
change_track(track) simply calls translate_uri(uri) by default now so that 90%
of clients with custom URIs should be able to just implement this one method on
the backend any ignore everything else.
@adamcik

This comment has been minimized.

Member

adamcik commented Mar 21, 2015

I'll get to the review fixes and change log, just wanted to draw attention to that I've just added backend.playback.translate_uri(uri) to make the simple case much simpler for backends.

def translate_uri(self, uri):
"""
Convert custom URI scheme to real playable uri.

This comment has been minimized.

@jodal

jodal Mar 21, 2015

Member

s/uri/URI/

Convert custom URI scheme to real playable uri.
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

This comment has been minimized.

@jodal

jodal Mar 21, 2015

Member

s/mopidy/Mopidy/

@jodal

This comment has been minimized.

Member

jodal commented Mar 21, 2015

Does these changes map clearly to Mopidy-Spotify 1/2?

@adamcik

This comment has been minimized.

Member

adamcik commented Mar 21, 2015

Mopidy-Spotify just needs to move it's play -> change track and it should work. I intend to do this change as a sanity check of all this anyway.

@adamcik

This comment has been minimized.

Member

adamcik commented Mar 22, 2015

I also have a stashed fix+test for the problem @kingosticks brought to my attention on IRC. That is that we don't respected the return value of changed track when checking for success in playback. This should be fixed as it's the only reason that mopidy-tunein is doing funny things with play() in it's backend. And the fact that this is a bug for Mopidy in general which should have been fixed in core, not worked around in a single extensions :-)

So sneak it in to this PR or should I fire it off from it's own branch once this is in?

@jodal

This comment has been minimized.

Member

jodal commented Mar 22, 2015

A PR of its own sounds good.

@jodal jodal added 3 - Done and removed 2 - Working labels Mar 22, 2015

jodal added a commit that referenced this pull request Mar 22, 2015

Merge pull request #1064 from adamcik/fix/1052-break-backend-play
backend: Change playback API (breaking change!)

@jodal jodal merged commit 56dca0e into mopidy:develop Mar 22, 2015

3 checks passed

Scrutinizer 4 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 76.38%
Details

@jodal jodal removed the 3 - Done label Mar 22, 2015

jodal added a commit that referenced this pull request Mar 22, 2015

backend: Minor docstring adjustments
Did it myself rather than holding off PR #1064 any longer.

@jodal jodal referenced this pull request Mar 23, 2015

Closed

Break backend playback API to prepare for gapless playback #1052

7 of 7 tasks complete

@adamcik adamcik deleted the adamcik:fix/1052-break-backend-play branch May 12, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment