Skip to content

Unify ExperimentalSettingsView#1936

Merged
LePips merged 7 commits intojellyfin:mainfrom
JPKribs:ExperimentalSettingsView
Mar 12, 2026
Merged

Unify ExperimentalSettingsView#1936
LePips merged 7 commits intojellyfin:mainfrom
JPKribs:ExperimentalSettingsView

Conversation

@JPKribs
Copy link
Copy Markdown
Member

@JPKribs JPKribs commented Mar 10, 2026

Summary

This might be a tad more controversial. The goal of this one is to get ExperimentalSettingsView unified for both iOS and tvOS and wire it up so any future experimental settings are minimally invasive to implement.

This leaves a platform specific settings group for iOS or tvOS only settings. Then, there is a shared settings sections for anything on both platforms. I have this wired up in the SettingsView to only show if isEnabled is set to true for this platform.

In an ideal world, I'd love isEnabled to just dynamically trigger based on if the platformSettings or sharedSettings .isNotEmpty but I was unable to find a way to do this.

The benefit of this PR is that all of the work for enabling a setting in ExperimentalSettingsView is done inside the view without needing to interact with the SettingsView or anything else. This also centralized shared experimental settings so they only need to go into this view instead of iOS and tvOS versions of this view.

Flask icon for tvOS shown below:

Simulator.Screen.Recording.-.Apple.TV.-.2026-03-10.at.10.04.09.mov

@JPKribs JPKribs added developer Alters the developer experience iOS Impacts iOS or iPadOS tvOS Impacts tvOS labels Mar 10, 2026
@JPKribs JPKribs requested a review from LePips March 10, 2026 16:24
@JPKribs
Copy link
Copy Markdown
Member Author

JPKribs commented Mar 10, 2026

@LePips Let me know what you think. It's a tad overkill. The alternatives to this are just PlatformView with 2 empty xOSView until needed or keeping 2 views. It's small enough it doesn't impact too much.

Copy link
Copy Markdown
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.

We don't need to introduce this boilerplate just for shared vs platform settings, this should be a fairly slim view itself.

@JPKribs
Copy link
Copy Markdown
Member Author

JPKribs commented Mar 11, 2026

We don't need to introduce this boilerplate just for shared vs platform settings, this should be a fairly slim view itself.

Sounds good. I've reduced this down. I also suppose the goal is to avoid platform specific changes in the future. If needed, build flags can be used for those specific settings as well.

I left the static let isEnabled so it can trigger the SettingsView section below:

if ExperimentalSettingsView.isEnabled {
    ChevronButton(L10n.experimental) {
        router.route(to: .experimentalSettings)
    }
}

Alternative is to just comment that out and un-comment it if it's needed. I prefer the static let isEnabled but I'm also fine removing that if that isn't wanted.

@JPKribs JPKribs requested a review from LePips March 11, 2026 05:28
@JPKribs
Copy link
Copy Markdown
Member Author

JPKribs commented Mar 11, 2026

Also, I think this is the last view to combine for now! I was looking at LiveTV, Item, Home, & Search Views but I noticed these seem to be covered in your work in #1752. I have aspirations for a LiveTV GuideView down the road but I think that will still need your changes since you refactor ChannelLibraryViewModel.

I have some other outstanding PRs but I think what I have open is all I will work on until #1752 is ready! Sorry for side tracking you with all these PRs I appreciate the reviews and feedback!

GuideView WIP
WIP.mov

@LePips LePips merged commit c41fb6e into jellyfin:main Mar 12, 2026
4 checks passed
@JPKribs JPKribs deleted the ExperimentalSettingsView branch March 12, 2026 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

developer Alters the developer experience iOS Impacts iOS or iPadOS tvOS Impacts tvOS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants