Closed vjr closed 2 years ago
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.
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?
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.
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.
Always keep the previous
and next
buttons enabled/sensitive?
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.
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 :-)
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.
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 :+1: We could probably drop that action enable/disable code here as well so that we're making things simpler and not adding more conditionals
@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.
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.