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

[tvOS] Settings Cleanup #1163

Merged
merged 6 commits into from
Aug 7, 2024
Merged

Conversation

JPKribs
Copy link
Member

@JPKribs JPKribs commented Jul 31, 2024

Goal

This PR originally started with the goal of fixing a slight peeve I had with the settings. All buttons in Forms would clip into their boundaries when focused. See:

Button Clipping

Following the changes here: #1163 (review), I've removed the clipping. Additionally to the clipping, I have added labels to the settings items and updated it to ensure that all sections have headers.

Settings

I created a custom SettingsViewForm and SettingsViewFormSection to handle the Form and Section actions from a single location. The goal was to centralize the button/form configuration in single struct so they could all be updated if there are any customization that we want to apply to all sections. These settings can be seen reflected here:

Settings Main

Simulator Screenshot - Apple TV 4K (3rd generation) - 2024-08-03 at 17 14 57

  1. Updated the Switch User button to indicate that it will take you to another page using the Chevron button.
  2. Added the Jellyfin Section category for Jellyfin Server items (First Section)
Server Details

Simulator Screenshot - Apple TV 4K (3rd generation) - 2024-08-03 at 17 12 38

  1. Created a Checkbox Button that mirrors the Chevron Button but has a variable for true/false. This removes the need to declare the button twice as checked/unchecked while also move the checkmark to more closely resemble the Chevron buttons used elsewhere in settings.
Video Player

Simulator Screenshot - Apple TV 4K (3rd generation) - 2024-08-03 at 17 12 54

  1. Change Resume Offset button Offset in the Resume category.
  2. Create Section Headers to better split out content since the Header/Footer can start looking a little weird when there are footers at the end of a section but no headers for that section.
Maximum Bitrate

Simulator Screenshot - Apple TV 4K (3rd generation) - 2024-08-03 at 17 13 01

Customize

Simulator Screenshot - Apple TV 4K (3rd generation) - 2024-08-03 at 17 13 09

Experimental

Simulator Screenshot - Apple TV 4K (3rd generation) - 2024-08-03 at 17 13 16

This last section was not part of the Setting Form Changes but instead just a padding(.horizontal) to resolve this clipping.

Server Selection

Settings

… Ensure Forms don't get clipped by their boundries. Create consistent, reusable button sizing/coloring. Apply to all Settings Pages.
@JPKribs JPKribs marked this pull request as ready for review July 31, 2024 01:07
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 taking a look at this and the localization but I apologize to say that the clipping issue could just be fixed [1] with the following in SplitFormWindowView and attached to other Forms [2] if necessary:

Form {
    contentView()
        .eraseToAnyView()
}
.scrollClipDisabled()

Additionally, this keeps the Form styling instead of moving to Buttons in a VStack.


[1] That modifier came in tvOS 17 which I just now set as the minimum but I would have pursued a way to prevent the clipping somehow with overlay or whatever and still use Form.

[2] The entire server selection will be changed to use a Menu which was introduced in tvOS 17, so I wouldn't worry about my ugly replication.

@JPKribs
Copy link
Member Author

JPKribs commented Aug 3, 2024

Form {
    contentView()
        .eraseToAnyView()
}
.scrollClipDisabled()

That is so much cleaner of a solution than this whole build out! I can't believe I didn't find this when I researching this. Let me re-submit this with just that single line change.

@JPKribs
Copy link
Member Author

JPKribs commented Aug 3, 2024

Thank you for taking a look at this and the localization but I apologize to say that the clipping issue could just be fixed [1] with the following in SplitFormWindowView and attached to other Forms [2] if necessary:

Form {
    contentView()
        .eraseToAnyView()
}
.scrollClipDisabled()

Additionally, this keeps the Form styling instead of moving to Buttons in a VStack.

[1] That modifier came in tvOS 17 which I just now set as the minimum but I would have pursued a way to prevent the clipping somehow with overlay or whatever and still use Form.

[2] The entire server selection will be changed to use a Menu which was introduced in tvOS 17, so I wouldn't worry about my ugly replication.

@LePips I've moved back to just using the standard form items again with that one .scrollClipDisabled() change. This resolves the exact peeve I had with this so I am happy with the change. I've left in my other items of adding Labels and Section headers but let me know if you want me to drop those as well.

Lastly, I added that padding to the Server Selection Form. Let me know if you want me to keep the padding or if that should also just be .scrollClipDisabled().

I've updated the original post with screenshots to reflect the current state of this PR!

@JPKribs JPKribs requested a review from LePips August 4, 2024 04:57
…ver Details Server non-focusable.

Create a new menu for Server Details selection. This is a WIP awaiting feedback from jellyfin#1163 (comment)
@JPKribs
Copy link
Member Author

JPKribs commented Aug 6, 2024

@LePips It looks like I can for the SettingView > Username that I did here: #1163 (comment)

The focus appears to work as we expect it to but I would agree it feels a little weird. Let me know if you want me to include this here:

Simulator.Screen.Recording.-.Apple.TV.4K.3rd.generation.-.2024-08-06.at.15.19.49.mp4

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.

Everything looks good as is. The user button should be changed to match iOS soon and I'll probably clean up some other things here as well. I'm experiencing some bugs in the iOS 18 simulator around losing focus when popping a settings view but that can be fixed later.

Thanks for doing this work and going through my requests!

@LePips LePips merged commit e4fd98c into jellyfin:main Aug 7, 2024
4 checks passed
@JPKribs JPKribs deleted the tvOSSplitFormWindowViewButtons branch August 7, 2024 03:49
@LePips LePips added the bug Something isn't working label Oct 8, 2024
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.

2 participants