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

[iOS & tvOS] Add 'Enable Rewatching' and ' Max days' to Next Up #1258

Merged
merged 7 commits into from
Oct 7, 2024

Conversation

JPKribs
Copy link
Member

@JPKribs JPKribs commented Oct 6, 2024

Context

Resolves: #1211

This is a rework #1212 where I DIDN'T try to rebase on Main, resulting in a git nightmare.

Summary

This PR enables a feature found on Jellyfin-Web that limits the number of items in the 'Next Up' section. To further mirror Jellyfin-Web, this feature uses 0 as disabled (All items are shown in Next Up) and only includes the parameter for NextUpDateCutoff when this is greater than 0.

While I was in there, realized that Jellyfin-Web had an option for EnableRewatching so included that in this PR. I was basing my Parameters off of this section of Jellyfin-Web: https://github.com/jellyfin/jellyfin-web/blob/master/src/components/homesections/sections/nextUp.ts

Screenshots

iOS

New Settings (Disabled) New Settings - Disabled
New Settings (1 Day) New Settings - 1 Day
New Settings (Multiple Days) New Settings - Multiple Days

tvOS

New Settings (Location) New Settings - Location
New Settings (Disabled) New Settings - Disabled
New Settings (1 Day) New Settings - 1 Day
New Settings (Multiple Days) New Settings - Multiple Days
New Settings (Subtitle Update) New Settings - Subtitle Update

@JPKribs
Copy link
Member Author

JPKribs commented Oct 6, 2024

This is up to date, merge-able on Main now, and cleaned up with only the new PR items.

I have a single outstanding issue:

Simulator Screenshot - Apple TV 4K (3rd generation) - 2024-10-06 at 17 37 06

This works, but the Ok/Done button is always the number??? I don't set that anywhere and I've been screwing around with this trying to fix it to no avail. I'm going to try this again with some fresh eyes later in the week. Unless @LePips, are you aware of what is going on here? It's only impacting tvOS.

Other than the tvOS Alert Button, I think all other changes are ready.

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.

Per the tvOS issue of dismissal in #1212 (comment), that happens in the simulator for me when I use return on the keyboard and works as expected when I use the on-screen remote.

Per the additional button on tvOS with the alert, that looks like a SwiftUI bug and probably nothing we can do about it.

Don't use the property wrappers in non-view contexts. While they technically can still work, use the subscript instead at the usage sites.
Use the dayInterval(0 ... 1000) format instead, then we don't need maxNextUpDays.
@JPKribs JPKribs marked this pull request as ready for review October 7, 2024 15:04
@JPKribs
Copy link
Member Author

JPKribs commented Oct 7, 2024

I've updated this with the last outstanding changes from the review. There is a TODO for the weird tvOS Alert Button bug(?) and I've merge in missing changes on Main. Let me know if there is anything else needed! Otherwise, I think this is ready.

@sjorge
Copy link

sjorge commented Oct 7, 2024

I'm not a developer so this is probably a dumb question, but why not use a simple text field that only accepts numbers for this?

That should work on iOS and tvOS, there is even a simple number one I think? (Although that might be 18.x only, I've only recently seen that for the first time)

@JPKribs
Copy link
Member Author

JPKribs commented Oct 7, 2024

That should work on iOS and tvOS, there is even a simple number one I think? (Although that might be 18.x only, I've only recently seen that for the first time)

I might be mistaken but this IS using the default TextField:

TextField(
    L10n.nextUpDays,
    value: $maxNextUp,
    format: .dayInterval(range: 0 ... 1000)
)
.keyboardType(.numberPad)

The primary item we are discussing is the delivery method for this. The popup is an 'alert' which is working fine on both iOS and tvOS but for whatever reason, the ok button that all alerts just have by default is showing the wrong text on tvOS. The alert presents the TextField where edits are made without taking the user to a new screen or covering up the existing screen (see below). This is either something not working correctly with the tvOS emulator for XCode or SwiftUI itself.

iOS:
Screenshot 2024-10-07 at 14 55 55

Since we're a ways away from a tvOS build on TestFlight (NOT a statement just my assumption), I'd argue this is worth revisiting at a later point instead of engineering 2 separate solutions to account for functionality that should just work in SwiftUI.

I intend on keeping an eye on this and, as a build is available for tvOS, I can confirm if this issue is present on a non-emulator and make changes accordingly!

@LePips LePips merged commit 1405d26 into jellyfin:main Oct 7, 2024
4 checks passed
@JPKribs JPKribs deleted the limitUpNextEnableRewatch branch October 7, 2024 21:26
@sjorge
Copy link

sjorge commented Oct 9, 2024

According to Testflight this should be in 1.3 ... but It's not showing up for me. (The active devices dashboard is, so I'm definitely on the correct build)

IMG_2766

@LePips
Copy link
Member

LePips commented Oct 9, 2024

Oops, I got the notes wrong. A new build should be up shortly.

@JPKribs JPKribs changed the title Add 'Enable Rewatching' and ' Max days' to Next Up [iOS & tvOS] Add 'Enable Rewatching' and ' Max days' to Next Up Nov 25, 2024
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.

Want option to limit next up in number of days like the web client
3 participants