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: Increase library list item size #881

Merged
merged 3 commits into from
Oct 30, 2023

Conversation

ericswpark
Copy link
Contributor

The current library list view seems a bit crowded:

Screenshot 2023-10-13 at 12 24 08 PM

With the increased size, we have three columns with a bit of padding between the items:

Screenshot 2023-10-13 at 12 25 12 PM

Unfortunately testing on a smaller screen (13 mini simulator) results in overlap with no padding. I'm not sure how to fix this, so leaving this PR as a draft for now:

Screenshot 2023-10-13 at 12 26 02 PM

@tonyd33
Copy link
Contributor

tonyd33 commented Oct 13, 2023

PagingLibraryView.gridLayout returns .adaptive(withMinItemSize: libraryGridPosterType.width + (UIDevice.isIPad ? 10 : 0)), which binds the min size to exactly the poster width. Giving some padding to both iPad and iOS should work.

On a side note, I'm not a main contributor, but these views seem like they warrant a SwiftUI rework anyway so this might get overridden when it happens

@ericswpark
Copy link
Contributor Author

@tonyd33 thank you, that was it! Now it shows in a two-column layout on the 13 mini:

Screenshot 2023-10-13 at 3 53 26 PM

Regarding the rework I don't really mind if this change gets overwritten, just something in the interim so that it looks a bit nicer.

@ericswpark ericswpark marked this pull request as ready for review October 13, 2023 19:57
@LePips
Copy link
Member

LePips commented Oct 13, 2023

This is a fairly complicated and annoying issue (with SwiftUI), so I'll give a fairly descriptive overview.


The library view is just a wrapper around UICollectionView, which is required because LazyVStack + ScrollView isn't a solution for large collections of items because it doesn't implement the idea of dequeuing views. So, I made my own basic wrapper that attempts to find the size that fits the given by the SwiftUI view, which I referenced from other SwiftUI UICollectionView wrappers:

The overlap is caused as the collection view hasn't yet determined the size required as the view/cell is becoming rendered, which can happen if you scroll fast.

At the same time, I was pretty ignorant on how UICollectionViewLayout worked so I went with something that "just worked". I've since then worked a bit on UICollectionViews and have become comfortable with the layout system. The PosterButton's width is ultimately done statically, and can further be changed through scaling the item. This causes the "spacing" issues between screen devices, because the widths are the same across devices and the underlying UICollectionView spaces them out because there is space to do so.

Frankly, I now see this as a dumb implementation, which was implemented by yours truly.


A solution we could implement "right now" is to change the layout to CollectionView and either scale the PosterButton per the detected width of the device. We can just do this "right now" because it uses the existing CollectionView, but I am also unsure whether it would actually fix the overlap issue, I don't believe it would. This also would require figuring out the sizes manually and maintaining it which I find annoying.

The proper solution is to rewrite the library view (and frankly some other views...) into a UICollectionView in the project. This would frankly increase performance and fix these layout issues. While it is annoying that we would be reworking some things into UIKit away from SwiftUI, SwiftUI just doesn't have what we need and this is a general consensus among other iOS developers I know.


I am fine for having this "in the interim" as you say, but yes these views will be rewritten at some point in the future.

Shared/Objects/PosterType.swift Outdated Show resolved Hide resolved
@ericswpark
Copy link
Contributor Author

@LePips I've modified the commit so that it now applies the scaling like you mentioned. I had to refactor out the magic number while changing it because not applying it to the withMinItemSize caused the overlap issue again on smaller screens.

I kind of wanted to move the entire ternary statement out to a variable, but this didn't work:

private let posterScale = libraryGridPosterType == .landscape && UIDevice.isPhone ? 0.85 : 1.25

I'm pretty sure there is a way to set a variable like this so I can reference it in both gridLayout and libraryGridView, but again, not very good with Swift. Please let me know if there's anything else I need to change or add!

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.

As stated, I'm fine with this for now.

@LePips LePips merged commit 333530a into jellyfin:main Oct 30, 2023
3 checks passed
@ericswpark ericswpark deleted the library-item-size branch October 30, 2023 22:53
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