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

use CollectionView on tvOS LibraryView #182

Merged
merged 6 commits into from
Oct 15, 2021

Conversation

jhays
Copy link
Contributor

@jhays jhays commented Oct 14, 2021

Replaces LazyVGrid with SwiftUICollectionView on the tvOS LibraryView.
This dramatically improves scrolling performance on tvOS!

However, there is an issue:
When scrolling through the library and reaching the end of a page, the next page is requested via LibraryViewModel. Currently the next page request seems to have a blocking effect on the UI, and it is frozen until the next page request is complete.

I'd appreciate any feedback on how to address that issue so fetching the next page does not block the UI.

Thanks!

@jhays jhays requested a review from acvigue as a code owner October 14, 2021 16:50
@LePips LePips added the enhancement New feature or request label Oct 14, 2021
Shared/ViewModels/LibraryViewModel.swift Outdated Show resolved Hide resolved
JellyfinPlayer tvOS/LibraryView.swift Outdated Show resolved Hide resolved
@LePips
Copy link
Member

LePips commented Oct 14, 2021

Per your comment about the main thread pausing when getting the next page, I think that it's weird because the network call should be asynchronous. For this work about the collection view, it isn't a blocker and will be it's own bug to be fixed.

LePips
LePips previously approved these changes Oct 14, 2021
@jhays
Copy link
Contributor Author

jhays commented Oct 15, 2021

My latest commit fixes a bug in my row math that was causing some duplication / id collisions between rows, which I think may have been primarily responsible for the performance hitch at next page request.

There is still a small stutter at the point of loading next page, but it is much better now.

I am also considering adding a loading spinner in the final cell that can be activated when next page is loading.

@sonarcloud
Copy link

sonarcloud bot commented Oct 15, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@jhays
Copy link
Contributor Author

jhays commented Oct 15, 2021

Ok, added one more commit that adds a progress view / loading spinner as the final cell when a next page exists. This makes the page loading moment feel a little better.

I can be done editing this branch now.

@jhays jhays requested a review from LePips October 15, 2021 13:25
@acvigue acvigue merged commit f1127ac into jellyfin:main Oct 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants