-
-
Notifications
You must be signed in to change notification settings - Fork 309
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
Conversation
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.
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.
Swiftfin tvOS/Views/ItemView/Components/ActionButtons/ActionButtonHStack.swift
Outdated
Show resolved
Hide resolved
Swiftfin tvOS/Views/ItemView/Components/ActionButtons/SelectVersionButton.swift
Outdated
Show resolved
Hide resolved
Co-authored-by: Ethan Pippin <ethanpippin2343@gmail.com>
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 |
Ah, I forgot about trailers. We'll get there when we need it. |
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.
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. |
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 theSelectedMediaSource
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)
iOS Fix