-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Implemented continuous checking of the behavior of track control buttons in a playlist #7433
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
base: master
Are you sure you want to change the base?
Conversation
Cloudflare Pages deployment
|
|
@thornbill, this MR performs as described for movies and tv shows — resolving the case described in #7433 When tested against the current functionality, music playlists continue to work as expected in all modes (shuffle, repeat, no-repeat, no-shuffle). |
I was planning to leave it as it was before, but I get your point. |
Yeah, the " Your latest commit seems to handle this nicely, though. 👍 |
b4f73da to
b6e12fc
Compare
1c821b3 to
b6e12fc
Compare
…ons in a playlist
dmitrylyzo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbf, I am not sure if hiding the buttons is a good idea ("jumping buttons"), so I reviewed the PR mainly from a technical point of view. Simply disabling the buttons may be a better choice - need more opinions.
| console.error('[VideoPlayer] failed to get "btnNextTrack"', btnNextTrack); | ||
| } | ||
|
|
||
| if (currentPlaylist.length === 1 || currentPlaylistIndex === currentPlaylistIndex) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currentPlaylistIndex === currentPlaylistIndex is always true. 🤔
I believe you need to check currentVisibleMenu === 'osd' to keep the OSD hidden (imo, updateTrackButtonsState should not show the OSD).
| if (currentPlaylist.length === 1 || currentPlaylistIndex === currentPlaylistIndex) { | |
| if (currentVisibleMenu === 'osd') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, what a stupid mistake. Had a hard day, sorry
42490ee to
7968694
Compare
|
As I mentioned earlier, users may be confused about having different behaviors in two situations described in the issue #7432. This pull request actually addresses more details than the issue describes at the moment, giving a better UX (in my opinion) for controlling the current track on the playback, removing the "Previous track" button at the first track of the playlist, and removing the "Next track" button at the last track of the playlist. Why? Because as a Jellyfin user, I counted the original behavior confusing, so I thought about making it a little bit better. |



Closes #7432
Now track control buttons will disappear when the user reaches last track watching the playlist.
Changes
The behavior of track control buttons is checked each time a track is switched (pressing on the
Previous track,Next trackbuttons via interface or using keyboard shortcuts).Issues
#7432