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

[iOS & tvOS] Fix Version Selection #1429

Merged
merged 9 commits into from
Feb 20, 2025
Merged

Conversation

JPKribs
Copy link
Member

@JPKribs JPKribs commented Feb 9, 2025

Summary

Please let me know if this interferes with #1203 and I can close this out.

Resolves: #1096

This adds a function to ItemViewModel to allow you to update the SelectedMediaSource statefully. This resolves the issue that the version selection did not update the value prior.

Additionally, since tvOS did not have versioning, I added a Version menu to the ellipsis if there is more than one version present. This is the only item I may need some advice on. I just added it to ellipsis menu that exists for refresh/delete. Would we prefer this to be more of a standalone menu like how it is on iOS? I can absolutely make that change as needed but, as a warning, that likely means we're out of room for more buttons for this spot on tvOS. We can definitely go to another row but for the first row, 3 buttons + ellipsis is likely our limit.

Screenshots

I don't do multiple versions on my server so I just manually merged two random movies. Hence the two totally different movies. This did assist in testing as I could confirm that the selected movie was the one being played.

New Menu (tvOS) Screenshot 2025-02-09 at 16 24 24 Screenshot 2025-02-09 at 16 22 33
iOS Fix

Simulator Screenshot - iPhone 16 Pro - 2025-02-09 at 16 26 13

@JPKribs JPKribs added the bug Something isn't working label Feb 9, 2025
@JPKribs
Copy link
Member Author

JPKribs commented Feb 11, 2025

This is the alternative approach where this is next to the existing buttons:

Screenshot 2025-02-11 at 09 56 12 Screenshot 2025-02-11 at 09 56 18

Copy link
Member

@LePips LePips left a comment

Choose a reason for hiding this comment

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

This doesn't directly conflict with #1203 and thanks for fixing this. I actually do like the variant with the 3 buttons and then the menu if necessary, so we can switch to that.

Co-authored-by: Ethan Pippin <ethanpippin2343@gmail.com>
@JPKribs
Copy link
Member Author

JPKribs commented Feb 16, 2025

I actually do like the variant with the 3 buttons and then the menu if necessary, so we can switch to that.

Sounds good! I can make that swap. As a more general question, what would be the route for other future buttons like "Trailer(s)" or "Add to Playlist"? Would those go down to a second row? Or, is that a "We'll get to it when we need it." Kind of question?

I only ask because I was wondering about the adding/creating part of #1428

@LePips
Copy link
Member

LePips commented Feb 16, 2025

Ah, I forgot about trailers. We'll get there when we need it.

@JPKribs
Copy link
Member Author

JPKribs commented Feb 16, 2025

No worries! I've made the change for this PR. Below are the various states that can exist:

Standard, Standard with Refresh/Delete Menu, Standard with Multiple Versions, and Multiple Versions with Refresh/Delete Menu.

Screenshot 2025-02-16 at 16 05 49

Below is what our drop down will show as. As a note, I just merged two totally unrelated movies to test this so the reality it would be like "Theatrical" and "Director's Cut" but in my case it's just 2 different movies haha.

Screenshot 2025-02-16 at 16 09 18

@JPKribs JPKribs requested a review from LePips February 16, 2025 23:14
Copy link
Member

@LePips LePips left a comment

Choose a reason for hiding this comment

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

To not crowd the bottom stack we can take a look in the future of making this menu on the trailing side of the play button. We also need a way to surface what version is currently selected without going into the menu, perhaps with the version's title in the play button underneath the play label, although I don't know how that will look.

@JPKribs
Copy link
Member Author

JPKribs commented Feb 19, 2025

To not crowd the bottom stack we can take a look in the future of making this menu on the trailing side of the play button. We also need a way to surface what version is currently selected without going into the menu, perhaps with the version's title in the play button underneath the play label, although I don't know how that will look.

Sounds good! I agree that providing some visibility would be helpful. I wanted to look at the play button in another PR. Specifically, bringing back the resume/play from beginning options. I could look at this as well. That being said, I was likely going to hold off until after #1203 just so you wouldn't have to deal with weird merge conflicts where possible.

@LePips LePips merged commit 137f0db into jellyfin:main Feb 20, 2025
4 checks passed
@JPKribs JPKribs deleted the fixVersioning branch February 20, 2025 05:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unable to select different movie file when multiple are merged
2 participants