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

Add audio, subtitle, playback select for tvOS #859

Merged
merged 6 commits into from
Oct 13, 2023

Conversation

tonyd33
Copy link
Contributor

@tonyd33 tonyd33 commented Sep 22, 2023

Purpose

  • Audio/subtitle/playback selection was completely missing in tvOS
  • Opening up the audio/subtitle/playback selection menu opened to a button that said "None"
  • Toggling autoplay reset focus

Changes

  • Add audio, subtitle, and playback selection in tvOS
  • Move playbackSpeed property into VideoPlayerManager to keep consistency with audio and subtitle property
  • Make unselected label have unfilled circle icon, make selected label have checkmark circle filled icon

Testing

  • Check audio, subtitle and playback select work in tvOS
  • Tested both iOS and tvOS to make sure the playbackSpeed move didn't break anything
  • Removed chapter select from the enum in SmallMenuOverlay because chapter selection already exists outside of that menu
Before After
track select before track select after
image image
image image
button_reset button_reset_fix
none_button none_button_fix

iOS playback speed works

ios_playbackspeed

Notes

While working on the fix, I noticed:

  • Audio/subtitle were not set when loading video on tvOS in VideoPlayerManager, so opening the selection menu shows that no audio/subtitle track is selected until manual selection
  • Opening the SmallMenuOverlay for the first time makes the default Text("None") button because lastFocusedSection is not set by default — fixed in this PR now

Do we want these problems fixed in this PR?

TODO

  • Attach before and after images
  • (Maybe) change playbackSpeed in VideoPlayerManager to Float in case we ever want to apply a nonstandard speed — lmk if we want this.
  • (Maybe) refactor out shared code among the selection menus in SmallMenuOverlay — lmk if we want this

@tonyd33 tonyd33 marked this pull request as ready for review September 23, 2023 20:45
@tonyd33 tonyd33 force-pushed the tvos-audio-subtitles branch 2 times, most recently from 83d05a8 to 29b3e04 Compare September 26, 2023 23:23
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.

Thank you for working on this. The playback speed being pulled into the manager is great as it is related to #711, but I haven't made a good mechanism for having values "transfer" between different media items.

I see that the small overlay was expanded to fill the entire screen, which we don't want, but I'm fine with for now to fix later. I'm fine with the circles on the buttons, but I may change that later on.

Per the "lmk if we want this" comments:

1 - keep the playbackSpeed as the PlaybackSpeed type
2 - I'm always for some cleanliness, so I'm welcome to that.

In fact, I think that the entire overlay on tvOS needs a redesign, both in terms of design itself and implementation. I've just had to deal with SwiftUI and its lack of support on tvOS and I don't think it's the prettiest thing in the world. /rant

@tonyd33
Copy link
Contributor Author

tonyd33 commented Oct 10, 2023

Thank you for working on this. The playback speed being pulled into the manager is great as it is related to #711, but I haven't made a good mechanism for having values "transfer" between different media items.

I see that the small overlay was expanded to fill the entire screen, which we don't want, but I'm fine with for now to fix later. I'm fine with the circles on the buttons, but I may change that later on.

Per the "lmk if we want this" comments:

1 - keep the playbackSpeed as the PlaybackSpeed type 2 - I'm always for some cleanliness, so I'm welcome to that.

In fact, I think that the entire overlay on tvOS needs a redesign, both in terms of design itself and implementation. I've just had to deal with SwiftUI and its lack of support on tvOS and I don't think it's the prettiest thing in the world. /rant

Just noticed the "Small" in "SmallMenuOverlay", haha.

For the circles on the buttons, I added it because changing from Text (no checkmark/image) to Label caused weird issues with the button width, making the text clip out of the button's bounds. Of course, these changes are likely going to be placeholders until the tvOS overlay overhaul.

Seeing as the playback slider is completely broken on tvOS among the other design issues, I'd be happy to see a rework on tvOS and help implement it. If you have mocks or even a detailed enough description of design goals, I'd love to incrementally work on it over the next while. We can discuss more on element

@LePips LePips merged commit 289868e into jellyfin:main Oct 13, 2023
3 checks passed
@lavavex
Copy link

lavavex commented Nov 30, 2023

This menu seems to have broken in tvos 17 and later, still works fine when run in tvos 15.4 simulator

@tonyd33
Copy link
Contributor Author

tonyd33 commented Nov 30, 2023

This menu seems to have broken in tvos 17 and later, still works fine when run in tvos 15.4 simulator

Yeah, it seems like tvOS 17+ broke the whole overlay menu stuff in general, even before this commit. I'll see if there are any bandages I can come up with quickly, but we had been discussing revamping the overlay anyhow so this might get pushed until then

Edit: Ok so it turns out this line was the culprit. For some unholy reason, before tvOS 17, currentOverlayType = .main was run before currentOverlayType = .smallMenu in BarActionButtons and then the orders were reversed after tvOS 17. Removing that line fixes it.

@holow29 holow29 mentioned this pull request Nov 30, 2023
25 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants