Enable the NEXT button if repeat mode is set to ALL or ONE.

[vjr]

If the repeat mode is set to "all" and the last track is playing, enable the "next" button instead of the earlier disabled state preventing you from jumping to the first track.

Also, if the repeat mode is set to "one" enable the "next" button here too instead of the earlier disabled state preventing you from moving to the next track.

[jeremypw]

In "Repeat All" with one item in the list, "Next" is sensitive but does nothing. It should restart the track, I guess (or not be sensitive).

I have adjusted the formatting of the multi-line if clause.

[vjr]

Thanks @jeremypw !

For a single track in the queue I feel that the buttons should be sensitive and clicking on any of them should start the track from the beginning, I'll update this PR as soon as I can.

[vjr]

In "Repeat All" with one item in the list, "Next" is sensitive but does nothing. It should restart the track, I guess (or not be sensitive).

I have adjusted the formatting of the multi-line if clause.

@danrabbit @jeremypw I believe I've addressed Jeremy's review although I wonder whether we should simplify the previous/next sensitivities (in a separate PR?) by leaving them sensitive at all times when there's at least one track in the Q? The playback behaviour stays, meaning when EOS (end of stream) then call next () and leave the behaviour of the previous/next buttons as is in this PR?

addressed

[jeremypw]

Not sure what the desired behaviour is tbh. You can argue that the Next and Previous buttons should always skip to the track that is physically next or previous in the queue list (regardless of mode) so would be insensitive in a one track list, or you could argue that they should skip to the next/previous logical track dependent on the mode - which is more complicated. In the latter case you would have to decide what the meaning of "next" and "previous" is in the context of a one track list in "Repeat All" and "Repeat One" modes. In a loop containing one item that item is both next and previous??

There should probably be another button to skip back to the beginning of the current track - if there is then you can make Next/Previous insensitive in a one item list because the user can always repeat the track using the Back to Start button.

I'll wait for @danrabbit 's input.

[vjr]

Thanks @jeremypw for your time looking at this PR :-) and yes looking forward to Dani's comments.

Here are my thoughts, just as feedback from a single user of the Music app and by no means am I anywhere close to being experienced at UX.

1. Always keep the previous and next buttons enabled/sensitive?

2. The repeat-mode setting only affects the "automatic flow" of the playback, meaning, for example, if the mode is ALL then when the last track finishes the first one starts playing. If the mode is DISABLED then the playback stops at the end of the last track. Which is what is the current behaviour.

3. Explicit user input (clicking the previous/next buttons) simply ignores the repeat-mode setting altogether and perhaps behaves as if ALL mode is set, meaning, if the last track is playing (or completed) then clicking NEXT will jump to the first track. If the first track is playing, clicking PREVIOUS will jump to the last track? I mean, if the last track is playing, WHY would a user click PREVIOUS or NEXT if not to explicitly change the current playing track, right?

All these "navigation" tweaks I feel would simplify the code making it less brittle with fewer instances of if-else blocks around the place. I guess the shuffle option behaviour remains as it is.

I think I'll just open a quick separate PR as a "demo" (both code and actual app usage) to play around with, so anyone can comment about it there, instead of trying to comprehend my written notes which may have something lost in translation :-)

[jeremypw]

Yes, I think I would prefer the simple predictable behaviour of ignoring the mode and just skipping to the next/previous track in the list.

[danirabbit]

Yeah that makes sense that the next and repeat buttons always behave as if the repeat mode is ALL and we only take the repeat mode into account when EOS is called. I'm on board with that 👍 We could probably drop that action enable/disable code here as well so that we're making things simpler and not adding more conditionals

[vjr]

@jeremypw I believe I've addressed the latest review/discussion points but for the life of me unable to get an invalid file into the Q like I was a couple of weeks ago. I wonder if there's been some changes in the way Files handles mime types or something like that? Would you try this latest PR branch and let us know your feedback?

As far as I could I believe I've addressed things after using Music and navigating with various files (3 files, 1 file) and sequence of button clicks for previous/next and repeat mode.

[jeremypw]

This behaves as expected and the code looks good apart from some minor comments.

I could get an invalid file into the queue simply by renaming a text file to give it an audio extension e.g. .ogg. Then it could opened in Music but was correctly skipped.

addressed

[jeremypw]

Lets go!