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] HomeView - Error Handling & Refreshing #1382

Closed
wants to merge 22 commits into from

Conversation

JPKribs
Copy link
Member

@JPKribs JPKribs commented Jan 2, 2025

Summary

Creation of a ErrorView for tvOS. Adds usage of the ErrorView for:

1.	ChannelLibraryView.swift
2.	HomeView.swift
3.	ItemView.swift
4.	MediaView.swift
5.	ProgramsView.swift
6.	QuickConnectView.swift
7.	SearchView.swift
8.	PagingLibraryView.swift

This should mostly mirror iOS.

Since PagingLibraryView has an optional cinematic background, I added Color.Clear to the other views. Without it, the ErrorView was offset on the HomeView which looked a little weird. See this offset below:

Without Color.Clear Offset

Simulator.Screen.Recording.-.Apple.TV.4K.3rd.generation.-.2025-01-02.at.14.00.36.mp4

In addition to this, there were some issues with how the HomeView populated the recently added section. For whatever reason, you would always need to leave Home then come back to get that to populate. I moved it so on init the ViewModel is refreshed which resolves this issue. Let me know if this is not the base route. .onFirstAppear was consistently failing to load the Recently Added which is why I made this move.

Finally, since there is an option to hide the 'Recently Added' section, if there was no recently added, next up, and resume, you would end up with the first library item at the top of the screen due to the ignoreSafeAreas() which is required for cinematic background. I have just updated this so the top item is always cinematic, including if it's the first latest in library.

While I was in there, I added a PosterType input to make sure that the 'Poster Type' selection for top item was respected. When cinematic currently, it's always landscape. Now, it will use whatever the setting is.

Demos

Server Offline

Simulator.Screen.Recording.-.Apple.TV.4K.3rd.generation.-.2025-01-02.at.13.52.23.mp4

Error Returned from Server

Simulator.Screen.Recording.-.Apple.TV.4K.3rd.generation.-.2025-01-02.at.14.04.18.mp4

Retrying Server

Simulator.Screen.Recording.-.Apple.TV.4K.3rd.generation.-.2025-01-02.at.14.02.21.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.

We need to avoid additional usages of UIScreen.main, even if tvOS is particularly on a single screen.

On iOS, we somewhat currently have static poster sizes (120, 300, etc.) that while not formal, we should keep to and not use computations screen widths. This mainly helps a lot with caching with negligent affects on the image quality. With that, we don't need to use maxWidth as well as maxHeight, just use maxWidth.

Swiftfin tvOS/Components/ErrorView.swift Outdated Show resolved Hide resolved
Swiftfin tvOS/Views/HomeView/HomeView.swift Outdated Show resolved Hide resolved
@JPKribs
Copy link
Member Author

JPKribs commented Jan 3, 2025

We need to avoid additional usages of UIScreen.main, even if tvOS is particularly on a single screen.

The only reason I was doing this was because I wasn't sure how set width worked for tvOS. Is 500 a static size so it's 1" on a 10" TV (10%) and 1" on a 100" TV (1%) or is it proportional like 1" on the 10" screen and 10" on the 100" screen?

@LePips
Copy link
Member

LePips commented Jan 3, 2025

There is no way to determine "inches" or any kind of physical measurement. We pass in the amount of points an image takes on the screen which then (currently) uses UIScreen.main to translate that into pixels based on given screen's scale.

I will eventually change those translations to use UITraitCollection.current.displayScale.

…n the refresh/retry buttons. Move transitions from `.transition(.opacity.animation(.linear(duration: 0.2)))` to `.animation(.linear(duration: 0.1), value: viewModel.state)`
@JPKribs
Copy link
Member Author

JPKribs commented Jan 4, 2025

I think I'm ready again. In addition to the original error handling, I was finding there were some conditions in the HomeView that were leading to some issues testing. Here is all that I am doing on this PR. It's all related to the HomeView but please let me know if you'd prefer I split any of this out to it's own PR:

No Valid Libraries

Simulator Screenshot - Apple TV 4K (3rd generation) - 2025-01-04 at 09 56 33

Cinematic Changes

This one requires a little more context. There is a way to end up with NO cinematic views on the HomeView. Not only does that look kind of lame, but it creates this issue where we are ignoring safe areas to get the Cinematic View to fully populate the top of the screen. As a result, the libraries start to bleed outside the top of the screen.

To get to a point where there are no cinematic views, you need to turn off the Recently Added section, and have nothing in Next or Resume. You can also get there based on a race condition. We are looking for where viewModels.isEmpty / isNotEmpty when we should be looking at their states. I've updated all of the views on the HomeView to validate their state is .content prior to showing nothing! This gives me a reliable 100% of the expected sections always exist where before there was a decent chance at least one section was missing due to the race condition.

For the cinematic views, I combined all of the views and added a cinematic flag to each to use the cinematic view instead. Addditionally, I configured it to if there is nothing else above it, the top item should always be cinematic. This includes the first library if that's all that's there.

Cinematic View should also respect the postType setting in Settings > Posters. If I set NextUp to have a portrait, it will now have a portrait even if it's the top, cinematic view.

Screenshot 2025-01-04 at 10 30 44 Screenshot 2025-01-04 at 10 31 05

Refresh

I added this to speed up some of my testing but I like the idea of keeping this. Ideally, we should be automatically refreshing the HomeView like we do on iOS but even iOS I have some scenarios where I like have the .refreshable like when I add something to Jellyfin and it's not on the HomeView yet. I'm not against pulling this, it's a very self contained piece of code so just let me know!

Simulator Screenshot - Apple TV 4K (3rd generation) - 2025-01-04 at 09 59 46
Simulator Screenshot - Apple TV 4K (3rd generation) - 2025-01-04 at 09 59 48

@JPKribs JPKribs requested a review from LePips January 5, 2025 09:49
@JPKribs
Copy link
Member Author

JPKribs commented Jan 9, 2025

I went back to test on this branch again and I realized this was still not working as intended. My latest commit should resolve the HomeView issues I was experiencing in testing but I might be getting a bit out of scope for a single PR.

My finding was that the HomeViewModel did not check the state of of the child 'ViewModels' before returning .content. So I added a check on each of the ViewModels but that was giving be some trouble too. So I published the underlying child ViewModels on HomeViewModel. Still had issues.

The primary issue was the view would draw before the data was there or not there. So all the viewModel.element.isNotEmpty checks would hide sections when there was data, just late.

Finally, I added all of the child ViewModel refreshes like:

                    let recentlyAddedTask = Task {
                        self?.recentlyAddedViewModel.send(.refresh)
                        while self?.recentlyAddedViewModel.state == .refreshing {
                            try await Task.sleep(nanoseconds: childPollDuration)
                        }
                    }

And this works! The HomeView is reactive as well to changes and all of the segments that should exist do exist! That being said, I don't know if this is the best way to approach this. I would prefer to just wait for viewModel.*child*.state but I was not having luck with that.

I'd appreciate some feedback but I'm trying to determine if this PR should just be ErrorView for tvOS or if it makes sense for HomeView Fixes?

Up for whatever works best!

…`ContinueWatchingView` to mirror iOS. Ensure `RecentlyAddedView` is hidden if toggled off. Remove unused Focus.
@JPKribs JPKribs changed the title [tvOS] HomeView - Error Handling [tvOS] HomeView - Error Handling & Refreshing Jan 10, 2025
@chickdan
Copy link
Contributor

chickdan commented Jan 10, 2025

My finding was that the HomeViewModel did not check the state of of the child 'ViewModels' before returning .content
...
The primary issue was the view would draw before the data was there or not there. So all the viewModel.element.isNotEmpty checks would hide sections when there was data, just late.

This seems like an anti-pattern and why you had the views changing before data came back. Is there a reason why the section view models are nested within the home model?

If the nesting is necessary my initial thought is to use a Binding that is passed into the child view models. Maybe one for each so you don't have to poll them?

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.

There are still a few UIScreen.main usages that should instead be changed to some static size.

For the changes to HomeViewModel and HomeView, I do know there are improvements that need to be made for how libraries are loaded and then presented but I don't think that this is it. I don't necessarily want to make explicit cases for the types of libraries (Homeview.CinematicRow, hasResumeContent, conditionalContentView, etc., except for showRecentlyAdded as a feature) which seems more error prone to any changes and maintenance and see a more general solution available.

For the tracking of library's state to change the HomeViewModel state, polling is an okay workaround but has some issues. Most notably that if a single library errors then it will poll forever but I also don't think erroring the HomeViewModel on an error is appropriate either. I have some other concerns with that observation implementation and would instead see if we could create a TaskGroup with all of the library's tasks themselves. Or, I will want to investigate skeleton loading views.

I'm fine with the refresh button as I used to have a button just like that before, but I won't want that in the long run as a solution to being a manual refresh mechanism. The existing itemMetadataDidChange notification observation and sinceLastDisappear interval refreshing does good enough for now and can be improved.

@JPKribs
Copy link
Member Author

JPKribs commented Jan 10, 2025

For the changes to HomeViewModel and HomeView, I do know there are improvements that need to be made for how libraries are loaded and then presented but I don't think that this is it. I don't necessarily want to make explicit cases for the types of libraries (Homeview.CinematicRow, hasResumeContent, conditionalContentView, etc., except for showRecentlyAdded as a feature) which seems more error prone to any changes and maintenance and see a more general solution available.

The reason I took this approach was because we start having some spacing weirdness. Would the preferred solution be to make the ignoreSafeAreas conditional instead? My route was to try and always ensure the top item was cinematic:

Simulator Screenshot - Apple TV 4K (3rd generation) - 2025-01-09 at 21 23 41

If the ignoreSafeAreas was conditional, this would look like:

Simulator Screenshot - Apple TV 4K (3rd generation) - 2025-01-09 at 21 43 21

@JPKribs
Copy link
Member Author

JPKribs commented Jan 10, 2025

For Skeleton Loading, are we talking about something like this loads then populates IF data exists then goes away if the data does not exist?

Simulator.Screen.Recording.-.Apple.TV.4K.3rd.generation.-.2025-01-10.at.13.32.17.mp4

I think I have that on my latest commit but there are still some items I want to work out in the interim.

@JPKribs JPKribs marked this pull request as draft January 10, 2025 21:05
@LePips
Copy link
Member

LePips commented Jan 10, 2025

always ensure the top item was cinematic

I do agree this should be the design, at least for now. I'm really trying to find a better way to articulate "I have the intention to largely refactor how the sections on home are loaded and presented" without being hand wavy and getting into details I don't currently have. Part of that is having a flat list of the sections to be presented and then (on tvOS) the first section is presented in the cinematic view. The refactor will be larger on iOS first, while tvOS will actually be a smaller change.

For Skeleton Loading

Similar to that, yes. However I would ask that we hold off on anything like that for a bit.

@JPKribs
Copy link
Member Author

JPKribs commented Jan 10, 2025

Absolutely no worries at all!

I can revert this back to just being the ErrorView if we feel like that's worth having. Otherwise, if you think the ErrorView will be a part of the bigger factor, I can just close this out as well.

Primarily, my descent down the rabbit hole was the ErrorView reloading the HomeView gets a little bit Wonky on Main.

If we did want to proceed with the ErrorView, I would just put the asterisk on it that it would need to be part of a bigger factor. To avoid weird loading afterwards

Either way works for me!

@JPKribs
Copy link
Member Author

JPKribs commented Jan 14, 2025

Absolutely no worries at all!

I can revert this back to just being the ErrorView if we feel like that's worth having. Otherwise, if you think the ErrorView will be a part of the bigger factor, I can just close this out as well.

Primarily, my descent down the rabbit hole was the ErrorView reloading the HomeView gets a little bit Wonky on Main.

If we did want to proceed with the ErrorView, I would just put the asterisk on it that it would need to be part of a bigger factor. To avoid weird loading afterwards

Either way works for me!

Closing out in favor of a broader tvOS change. Additionally, I am going to be a bit busier in the coming months so I am closing this out to clear some of the outstanding PRs as I believe only the ErrorView is valid from this PR and the rest likely needs reworking at a later date.

@JPKribs JPKribs closed this Jan 14, 2025
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