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

Infinite recursion loop for errors encountered when playing single repeat track #1221

Closed
jcass77 opened this issue Jul 16, 2015 · 8 comments
Closed
Labels
A-core Area: Core layer C-bug Category: This is a bug

Comments

@jcass77
Copy link
Member

jcass77 commented Jul 16, 2015

This is similar to #17, and the "skip-to-next-track-upon-error" code (https://github.com/mopidy/mopidy/blob/749c8baceb077f8a5155e1dab33b7154ab1ba481/mopidy/core/playback.py#L361-363) also causes an infinite recursion loop for errors encountered when playing a single track in repeat mode.

I've submitted a pull request which adapts the code to just remove these unplayable tracks from the tracklist (similar to what is being done in shuffle mode currently), see: PR #1205.

It does however raise the question of whether there should be a more elegant way of dealing with error conditions and notifying the front-end user, instead of just swallowing the errors and skipping to the next track in the back-end?

@adamcik
Copy link
Member

adamcik commented Dec 5, 2015

I was looking at this again today. I'm currently considering if we should have tracklist keep all unplayable TLIDs and then skip them in eot/next/prev track. Meanwhile this is kinda waiting for EOT track handling taking unplayable tracks into account.

@jcass77
Copy link
Member Author

jcass77 commented Dec 7, 2015

I can see how one could abstract all of that away from front- and backend extension developers and still keep the unplayable TLIDs.

What would the use case be for hanging on to unplayable tracks though? It will make the code a little more complex and the tracklist a little less intuitive to use, and I'm not sure what one would be able to do with them down stream?

@jodal
Copy link
Member

jodal commented Dec 10, 2015

I don't remember exactly, but I suspect that we simply don't know that they are unplayable until we try playing them. What's the least surprising behavior?

  • A track disappears when you try to play it.
  • A track is skipped every time you try to play it.

If we want to do better than this, we need to implement a way to send error messages to the clients first.

@jcass77
Copy link
Member Author

jcass77 commented Dec 10, 2015

Yes that makes a lot of sense. There is a different fringe condition that we may be able to deal with in the interim though:

  • when consume mode is turned on, tracks are removed from the tracklist as they are played:
    def _mark_played(self, tl_track):
    """Internal method for :class:`mopidy.core.PlaybackController`."""
    if self.consume and tl_track is not None:
    self.remove({'tlid': [tl_track.tlid]})
    return True
    return False
  • however if the track is not playable it will simply be logged and left in the tracklist:
    def _mark_unplayable(self, tl_track):
    """Internal method for :class:`mopidy.core.PlaybackController`."""
    logger.warning('Track is not playable: %s', tl_track.track.uri)
    if self.get_random() and tl_track in self._shuffled:
    self._shuffled.remove(tl_track)

The result is that, when playing a tracklist in consume mode, the user can end up in a situation where the only tracks left and show in the tracklist are the unplayable ones. This seems counter-intuitive and a little surprising, as one would expect the tracklist to be empty after having run through all of the tracks with consume turned on.

What are your thoughts? If there is agreement on the idea that unplayable tracks should be removed from the tracklist in consume mode just like regular tracks then I will open a new issue and start working on a PR?

@tkem
Copy link
Member

tkem commented Dec 10, 2015

+1 for removing unplayable tracks in consume mode, at least. I strongly support @jcass77's argument for this case when wearing my client-developer hat.

@jodal
Copy link
Member

jodal commented Dec 10, 2015

+1, I agree that unplayable tracks should be consumed in consume mode.

@jcass77
Copy link
Member Author

jcass77 commented Dec 11, 2015

I've opened a separate issue for investigating how unplayable tracks are consumed: #1358.

@jodal
Copy link
Member

jodal commented Mar 26, 2016

With #1358 + #1359 and #1454 + #1455, this should be covered elsewhere.

@jodal jodal closed this as completed Mar 26, 2016
@jodal jodal added this to the v2.0.1 - Bug fixes milestone Mar 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-core Area: Core layer C-bug Category: This is a bug
Projects
None yet
Development

No branches or pull requests

4 participants