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

Mark track as playing and add to history if changing track while paused. #1356

Merged
merged 4 commits into from Dec 29, 2015

Conversation

3 participants
@jcass77
Member

jcass77 commented Dec 6, 2015

Suggested fix for #1352

Replaces #1353 which was created against the wrong branch (I'm an idiot).

@@ -19,6 +19,10 @@ Bug fix release.
- MPD: Notify idling clients when a seek is performed. (Fixes: :issue:`1331`)
- Core: Fix :meth:`~mopidy.core.PlaybackController._change_track` to mark

This comment has been minimized.

@jodal

jodal Dec 6, 2015

Member

I don't think the method name is relevant for the changelog, as it's a private method.

backend.playback.prepare_change()
backend.playback.change_track(tl_track.track).get()
success = (
backend.playback.prepare_change().get and

This comment has been minimized.

@jodal

jodal Dec 6, 2015

Member

You don't call the get method here, so you check the trueness of the method itself.

This comment has been minimized.

@jodal

jodal Dec 6, 2015

Member

Also, the default implementation of backend.playback.prepare_change() returns None, so it will always fail. I think success should only take the return value of change_track().

c.tracklist.add([track1, track2])
d = mock.Mock()
c.history._add_track = d

This comment has been minimized.

@jodal

jodal Dec 6, 2015

Member

I think the test would be more readable if you use c.history._add_track instead of d throughout the test.

Same for c.tracklist._mark_playing instead of e.

@jodal jodal added this to the v1.1.2 - Bugfixes milestone Dec 6, 2015

jcass77 added some commits Dec 7, 2015

Result of prepare_change no longer affects whether a track is added t…
…o the history.

Update changelog and test cases.
@jcass77

This comment has been minimized.

Member

jcass77 commented Dec 7, 2015

I've implemented the changes as suggested by @jodal.

It may be worth considering whether resume should also handle unplayable tracks in the same way that play does. In other words, shouldn't there be a _resume that mirrors _play that also skips to the next track if the track that the user is trying to resume is unplayable?

def test_resume_does_nothing_if_track_is_unplayable(self):
self.set_current_tl_track(self.unplayable_tl_track)
self.core.playback.state = core.PlaybackState.PAUSED
self.core.playback.resume()
self.assertEqual(self.core.playback.state, core.PlaybackState.PAUSED)
self.assertFalse(self.playback1.resume.called)
self.assertFalse(self.playback2.resume.called)
makes sense if we are trying to resume a track that has become unplayable since it was paused (?), but if the user does pause -> next -> resume should Mopidy not keep skipping until it finds a track that is playable just like stop -> next -> play would?

@jcass77

This comment has been minimized.

Member

jcass77 commented Dec 20, 2015

@jodal, is there anything else that you would like to have done on this PR before it can be merged?

@adamcik

This comment has been minimized.

Member

adamcik commented Dec 28, 2015

Don't know if @jodal had anything to add but there is a merge conflict currently. If there isn't anything else we can probably also resolve that ourselves.

@jodal

This comment has been minimized.

Member

jodal commented Dec 28, 2015

@adamcik I have nothing to add. I'll resolve the conflict and merge.

@jcass77 Regarding the resume case, it sounds like you're onto something. Can you open a separate issue to track it?

@jodal jodal self-assigned this Dec 28, 2015

@jcass77

This comment has been minimized.

Member

jcass77 commented Dec 29, 2015

On the pause->next->resume issue: the way that this case is handled in 'develop' at the moment seems to be correct.

I've created a small PR #1377 to prove this point.

@jodal jodal merged commit 3cd3b45 into mopidy:release-1.1 Dec 29, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jodal added a commit that referenced this pull request Dec 29, 2015

@jodal jodal added the A-core label Dec 29, 2015

@jcass77 jcass77 deleted the jcass77:fix/1352 branch Jan 2, 2016

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