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

Play song as part of album #1644

Merged
merged 1 commit into from
Feb 10, 2024
Merged

Play song as part of album #1644

merged 1 commit into from
Feb 10, 2024

Conversation

1hitsong
Copy link
Member

@1hitsong 1hitsong commented Jan 13, 2024

Changes

When user clicks on a song, queue the album and start playback at requested song.

Issues

Fixes #1216

@1hitsong 1hitsong added the general-improvement Quality of life improvements that don't add new functionality. label Jan 13, 2024
@1hitsong 1hitsong requested a review from a team as a code owner January 13, 2024 00:22
Copy link
Member

@cewert cewert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I played an album from track 4 and let it play till the end. I expected to be able to skip backwards to track 1 so I could play the songs I missed but instead it kicked me back out to the screen I came from when the playlist finished.

I have a couple different fixes in mind:

  • I think the ideal fix would be to stay on the audio player screen whenever the playlist ends and let the user decide if they want to go back or keep playing music but this might require more changes than I realize and could get out of scope.
  • Remove all tracks less than the song index from the playlist. That way when the audio player is shown, it will always say 1 / X to inform the user that the other tracks were removed.

@1hitsong
Copy link
Member Author

Yeah, what occurs after playing the last song is out of scope for this PR.

The only goal for this PR is when a user clicks on a song, we load the album and start playback from the selected song's index.

As an example, user is on an album with 10 tracks. They click on track 4. The player starts playing song 4 and shows the track info as 4/10. It clearly indicates to the user a few things:

  1. We started you at the correct track in the album
  2. Once this song is over, it's going to move to the next track
  3. You can press back to go to the previous 3 songs if you desire

The Jellyfin web client works this way. Finamp, the mobile Jellyfin client, works this way. Even my car's media player works this way.

@michaelcresswell
Copy link

Very excited to see this.

I think I prefer the client showing the track list again after playback finishes so the existing behavior sounds good to me but I hear the scope argument as well.

The only other change that might be in scope would have to do with the ‘Play Album’ button: It’s a valuable addition to the client before this PR but (I believe) it’s redundant clutter afterwards. It still does it what it’s supposed to though so I can understand leaving it for another PR.

I’ll test this out tomorrow night when I have access to my Roku again and will likely daily-drive it until 2.1 comes out. I’ll let you know if I notice any issues.

@cewert
Copy link
Member

cewert commented Jan 13, 2024

Yea, that makes sense; I'm not actually trying to change the text. I just hate being kicked out so bad that changing the text to soften the blow sounded better than doing nothing.

I think I'm just used to reviewing bugfix PRs which is why I brought up a related bug that this PR shines a light on. I also figured it would be an easy fix.

The Jellyfin web client works this way. Finamp, the mobile Jellyfin client, works this way. Even my car's media player works this way.

FWIW, spotify doesn't work this way. That's what I tested with before posting and what we've used in the past for inspiration when it comes to music related features and UX.

@1hitsong
Copy link
Member Author

I think I'm just used to reviewing bugfix PRs which is why I brought up a related bug that this PR shines a light on. I also figured it would be an easy fix.

1 point of clarification here. It's not a bug that you return to the previous screen on playback completion. I prefer it working like that. There can be no confusion that playback is complete. If it stopped at the end of the last song, a user may mistakenly believe it's locked up.

I liken it to video playback. When the video is done, it exits the player and returns to the previous view.

Also on the web client, when the last song plays, it closes the player view and returns you to the previous screen.

@cewert
Copy link
Member

cewert commented Jan 13, 2024

It's not a bug that you return to the previous screen on playback completion. I prefer it working like that. There can be no confusion that playback is complete. If it stopped at the end of the last song, a user may mistakenly believe it's locked up.

Is that the only reason though? That's a valid reason but I think we could handle that in a different way to allow the user the freedom to continue using the same playlist if they want. The solution that comes to mind would be showing a gif of the typical audio bars dancing around during playback.

I liken it to video playback. When the video is done, it exits the player and returns to the previous view.

I don't think that's a good comparison mainly because consuming a video means you need to be sitting down by the TV to fully enjoy the media vs audio which is almost exclusively consumed while users are doing something else besides watching the TV.

For example, you open an "instant mix" playlist for an artist before doing some chores around the house. You are really enjoying the playlist for whatever reason. While doing chores, the playlist ends. You go to your TV to restart the playlist but the app kicked you back to the artist screen. You click "instant Mix" again but the playlist isn't the same and you have no way of getting that exact playlist back.

Another example would be the same situation but you hear a song you really like and don't recognize it. You want to go check the TV but you can't because you're busy. By the time you get over to the TV, the playlist ended and now you have no way of figuring out what song that was.

These scenarios don't really translate to videos because of how they are consumed. There are similar situations with video playlists but with videos we know the user was sitting by the TV watching it. Most videos (tv/movies) are at least 20min so that means the user had 20min to use the OSD to learn more about the media they are watching.

Another thing is that we talked about having a "global" audio player one day in the future and once that is implemented I think kicking the user back out would make even less sense because if I'm on the home page and my audio playlist ends, I would expect to open the global audio player and see the playlist that just ended. Do you agree? Wouldn't we want the audio player behaviors to match regardless of what screen the user is viewing?

Also on the web client, when the last song plays, it closes the player view and returns you to the previous screen.

I think the web client is wrong on this one for the same reasons I listed above.

@1hitsong
Copy link
Member Author

1hitsong commented Jan 13, 2024

Let's move our discussion to a different location since it's unrelated to this ticket.

Discussion post: #1652

@jellyfin-bot
Copy link
Contributor

This pull request has been inactive for 21 days and will be automatically closed in 7 days if there is no further activity.

@jellyfin-bot jellyfin-bot added the stale This issue/PR has gone stale. label Feb 4, 2024
@1hitsong 1hitsong removed the stale This issue/PR has gone stale. label Feb 6, 2024
@1hitsong 1hitsong merged commit bb96006 into jellyfin:master Feb 10, 2024
15 checks passed
@1hitsong 1hitsong deleted the musicStartAtTrack branch February 10, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
general-improvement Quality of life improvements that don't add new functionality.
Projects
Development

Successfully merging this pull request may close these issues.

When choosing to play a song from an album, start playing the whole album from that point
4 participants