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

Video Player Settings Cleanup #1199

Closed
wants to merge 10 commits into from
Closed

Conversation

JPKribs
Copy link
Member

@JPKribs JPKribs commented Aug 19, 2024

Summary

The goal of this PR is to add descriptors to the Video Player Settings and make sure that iOS and tvOS Video Player Settings mirror each other as closely as possible. Since the only Native Player setting is the Resume Offset, the Video Player Settings should switch based on which player is selected.

Native Player Settings is a section in iOS that changes the same Resume Offset variable as the one in the VideoPlayerSettings. Since that's the only real Native Player Setting, I have just set it up to use a single VideoPlayerSettings page with a switch case to hide everything not in use when Native Player is selected.

tvOS currently has a lot of settings missing vs iOS. That being said, I don't know how many of these settings are in-use or will be in use after #1081. Let me know if there are any settings that need to be removed. For now, the goal of this PR is to just add everything from iOS to tvOS and make sure it works. I'm more than happy to trim that down.

tvOS resumeOffset was calling an overlay which meant that any other setting that used the same number stepper couldnt get called. I moved these settings into their own views and called them using the router. I found that the number stepper used to have an issue where it checked if the value was inside the range before stepping so you could step outside the desired range (-1 or 31). I've resolved this as well.

Under the hood, I noticed that most of the sections in the Video Player Settings are called from other views but there were still a couple settings in iOS that existed inside the main view. I moved everything to its own section file and called it from the extensions. Since tvOS didn't have this structure, I mirrored this layout in tvOS as well.

Still In Progress

  • Cleanup Screenshots Below
    Figure Out a ColorPicker Solution for tvOS
  • Localizations

Under the Hood

tvOS/iOS Section Layout Screenshot 2024-08-18 at 18 57 02
VideoPlayerSettings Switch Case Screenshot 2024-08-18 at 18 58 46

iOS

Video Player Player Pt. 1 Screenshot 2024-08-18 at 18 58 46
Video Player Player Pt. 2 Screenshot 2024-08-18 at 18 58 46
Video Player Player - Native Player Screenshot 2024-08-18 at 19 30 16

tvOS

Video Player Player Pt. 1 Screenshot 2024-08-18 at 18 58 46
Video Player Player Pt. 2 Screenshot 2024-08-18 at 18 58 46
Resume Offset View Screenshot 2024-08-18 at 18 58 46
Video Player Player - Native Player Screenshot 2024-08-18 at 18 58 46

…ayer Sections into their own files. Make tvOS mirror iOS for VideoPlayerSettings with the exception of the ColorPickers and the Gestures. Remove the Native Player Settings in favor of a single settings screen with a switch case.
@JPKribs JPKribs marked this pull request as ready for review August 19, 2024 19:33
@LePips
Copy link
Member

LePips commented Aug 24, 2024

iOS and tvOS Video Player Settings mirror each other as closely as possible

I apologize, but this is not my intended goal. tvOS will be less customizable and more simple, mainly due to the fact that I don't see the need for more player or app customization and tvOS is already a pain to develop for so it makes it easy. For iOS, I have been more generous with player customization because it was easy but even I think I went too far for our own good.

@JPKribs
Copy link
Member Author

JPKribs commented Aug 24, 2024

That makes sense to me. Are there any specifics you want me to pull from the tvOS side? My goal is to get all the shared elements laid out in the same way but anything we don't need on tvOS I'd be happy to strip out of this PR.

@LePips
Copy link
Member

LePips commented Aug 27, 2024

I haven't taken a deeper look, but the only options that should be customizable are:

  • jump backward/forward length
  • resume offset
  • subtitle font
  • subtitle size
  • chapter slider (maybe)

We shouldn't have any settings regarding gesture or UI (labels, button/menu placement).

Since mpv provides more in-player customization options (subtitle position offset, color grading, etc.) I think those will have more impact than overlay customizations.

@JPKribs
Copy link
Member Author

JPKribs commented Aug 28, 2024

@LePips I've removed the unwanted tvOS sections. In PlayerSettings, iOS and tvOS should mirror each other in layout and descriptions with the exception of items that are not found in tvOS. I've updated the screenshots to reflect the current state of this PR. All labels should be the same minus the Subtitle description no longer mentions color on tvOS.

All strings should be localized. Let me know if there are any that need to get changed out

@JPKribs
Copy link
Member Author

JPKribs commented Oct 16, 2024

I'm going to close this out.

I think it's a good idea eventually go clean up the settings / customize settings but this PR is pretty out of date and the amount of work to copy/paste sections around really isn't that hard.

I will approach this and #1222 after I'm done making new settings as part of #1271. I want to focus more on getting the Admin Dashboard functionality first!

@JPKribs JPKribs closed this Oct 16, 2024
@LePips
Copy link
Member

LePips commented Oct 16, 2024

I was going to look at this more after #1203 since I'm removing/reworking many settings and have ideas for previewing certain settings as well as Learn More dialogs.

Apologies for not being more explicit on that.

@JPKribs JPKribs deleted the iOSSettingsCleanup branch October 17, 2024 23:53
@JPKribs
Copy link
Member Author

JPKribs commented Oct 19, 2024

I was going to look at this more after #1203 since I'm removing/reworking many settings and have ideas for previewing certain settings as well as Learn More dialogs.

Apologies for not being more explicit on that.

No worries!

Let me know if this is the wrong direction, but do we eventually want all settings in StoredValues instead of Defaults? I feel like if I'm moving things around, I should be looking at migrating those to stored values while I'm at in? Plus, for anything new, should it be in StoredValues instead like the customDeviceProfiles?

@LePips
Copy link
Member

LePips commented Oct 19, 2024

Probably. However, we will first have to think about how we will perform our own manual versioned migrations. We will need to migrate the values from defaults to stored values. Or, not all values necessarily need to be moved over. We will see.

However, I consider that low priority.

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.

2 participants