Skip to content
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

Avoid endless loop if all tracks are unplayable. #1455

Closed

Conversation

dublok
Copy link
Contributor

@dublok dublok commented Feb 17, 2016

Limit the number of tries for changing to the next track.
The limit is 2 * tracklist length to get all tracks in a shuffled playlist.

Please have a closer look to _on_about_to_finish. Seems there was a loop if backend==None.

Fixes #1454

Limit the number of tries for changing to the nest track.
The limit is 2 * tracklist length get all tracks in a shuffled playlist.
@adamcik
Copy link
Member

@adamcik adamcik commented Feb 17, 2016

Looks like a fairly simple way to get around the shuffle problem which has blocked previous attempts at getting the corner cases for this. Don't we need the same for next and prev. It would also be good with some tests for this, even if they ignore the shuffle corner case.

@dublok
Copy link
Contributor Author

@dublok dublok commented Feb 17, 2016

Probably yes. I'll check tomorrow.

@adamcik
Copy link
Member

@adamcik adamcik commented Feb 17, 2016

Thanks for taking the time to help fix this :-)

@dublok
Copy link
Contributor Author

@dublok dublok commented Feb 17, 2016

I added the check for next and previous.
The the tests have to wait..

(Can we delay the test for a follow-up PR?)

Test PlaybackController functions play(), next(), previous()
and _on_about_to_finish() that they will not loop forever
if all tracks are unplayable.
@dublok
Copy link
Contributor Author

@dublok dublok commented Feb 18, 2016

Surprisingly it is possible write test that ensure that the functions will not loop forever ;-)
Tests done.

@jodal jodal added this to the v2.0.1 - Bug fixes milestone Feb 19, 2016
@adamcik
Copy link
Member

@adamcik adamcik commented Mar 25, 2016

Looked over the code now, just needs to be merged correctly into release-2.0 as we want this in our 2.0.1 release based on the assigned milestone.

@jodal
Copy link
Member

@jodal jodal commented Mar 26, 2016

I rebased and merged this into release-2.0 in commit 5a371b8.

@jodal jodal closed this Mar 26, 2016
@jodal jodal self-assigned this Mar 26, 2016
tmyersjstar pushed a commit to tmyersjstar/mopidy that referenced this issue Apr 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants