Swiftfin
Swiftfin copied to clipboard
jellyfin
Create Library Alpha Picker
This feature creates a new filter in the ItemFilters for AlphaPicker. The goal is to (eventually) mirror the functionality of the Jellyfin-Web AlphaPicker which filters the library based on the letter selected. This PR focuses on the creation of the Filter as to split out the underlying Filtering components that exist within the existing ItemFilter / FilterDrawer objects.
Letters for the filter are pulled from the localized language on the device. The filter sets the API Parameter nameStartsWith to the letter selected or uses nameLessThan: 'A' for symbols. This reflects the behavior seen in other implementations of this feature. To assist in this, I have an AlphaPicker class that returns the appropriate nameStartsWith and nameLessThan parameters based on the AlphaPicker.selection.
When testing, I found that the current unstable/development branch of Swiftfin has an issue where the Genre & Filter filters were only referenced in the _getDefaultParameters call. This means that when you updated a genre filter, it would not be reflected until you next hit the requestItems() call. This issue did not exist with the SortBy and SortOrder parameters since these are written during the requestItems() call. To resolve this, I moved the filter objects outside of the defaultParameters since 1) the default call for parameters doesn't need to be filtered and these parameters are being created and set to nil and 2) this resolves the issue and the filters now apply directly after being set. Let me know if there is a better way to handle this!
If this PR is merged, I will work on adding the AlphaPickerBar to the LibraryView to mirror Roku/Web.
Potential Concern: https://github.com/jellyfin/jellyfin-roku/pull/1716
- The Roku App uses the same nameLessThan: 'A' for filtering on symbols but there are some symbols that come after Z alphabetically. The Roku App only uses #,A-Z so this works for their implementation since it will only ever be Latin Letters & Symbols. The filter I created takes the localized alphabet from the device. As a result, non-Latin Characters will exist as a selection and anything nameGreaterThan: 'Z' will also include these characters.
- Roku & Jellyfin-Web both only allow Latin Letters. I do not know if the API works with non-Latin letters.
This is a rewrite of something I started lasted year. I chose to take some time to learn Swift better and hopefully rewrite this feature with a little more direction. https://github.com/jellyfin/Swiftfin/pull/852
I have completely refactored how filters are implemented in #905. I'm almost done and adding this work should be easier to implement, so I ask that this work be redone once that's in, apologies.
The filter I created takes the localized alphabet from the device.
This is the extent of what we can do given that the server doesn't give a list of section titles. However, that would be a great addition to collection folder/folder BaseItemDtos.
Roku & Jellyfin-Web both only allow Latin Letters. I do not know if the API works with non-Latin letters.
By my n=1 testing of playing with a movie with Korean characters, it does. So as far as I can tell, we have full UTF-8 support for names.
I have completely refactored how filters are implemented in #905. I'm almost done and adding this work should be easier to implement, so I ask that this work be redone once that's in, apologies.
No worries at all! I'll keep an eye out for when that's ready then I'll take another look at this.
By my n=1 testing of playing with a movie with Korean characters, it does. So as far as I can tell, we have full UTF-8 support for names.
Sounds good. I like the localized letters if they work but I think this means we don't catch Æ types. I'm comfortable with that implementation but I just wanted to make note that other clients are having this conversation.
One idea I had after I made this would be 3 API searches.
- NameLessThan A
- NameGreaterThan Z + NameLessThan first non-Latin letter found
- NameGreaterThan last non-Latin letter found
Getting NameGreaterThan using nameStartsWithOrGreater then removing items that start with that letter... The end result is # == Any non-Localized Letter. I'd be happy to take a crack at that once that new filters are in but it sounds hard to make performative.
#905 has now been merged, so this feature can continue. We don't need to make another class for holding the letters, we only need to add another ItemFilterType and hopefully everything should statically make sure that the case it handled correctly.
First and foremost, #905 resolves my only pain point I've ran into with Swiftfin! The change in the library scrolling is awesome. Literally 0 hitching even on remote loading. It looks like a lot of hard work went into making this and it really shows. I'm excited for when this makes its way to the App Store!
Regarding this PR, I'm going to take a look at this tonight/this weekend. I might just nuke my branch and start over with the changes. PR#1 I'll add the filter itself then, with the library changes, I think I need to go back to the drawing board for the AlphaPicker itself. I personally like the idea of mirroring other clients/web with the letters down the side but I'd need to figure out how to avoid cramping into the library. I'll ping once I have something worth looking at!
Any interest in continuing this work? If not, I can implement the letter filter and leave the library picker as later work for you.
Hey, sorry work has caught up to me as of late. I had a chance to review your new filters. It looks a lot cleaner/intuitive but when I took a crack at it I ran into some trouble understanding how to populate the filter values using the section titles (#, A-Z).
I want to say I'll be able to knock this out soon but soon could mean early May just with how things are going as of late...
I don't want to hold anything back so if this is holding anything back I wouldn't be offended if you did it. Otherwise, I can work on getting this wrapped up soon (May) from my end so I'm not adding to your plate for Swiftfin. Your call!
No worries at all. I just happen to have a lot of time on my hands to get things done and think this would be a great addition. I've implemented the letter filter in the PR below and I would anticipate the picker to be quick when you get to it. You are free to continue this PR or close it.
- https://github.com/jellyfin/Swiftfin/pull/1024
That sounds great! Thanks for doing that. I think the work that I have for the letter bar should be pretty easy to port over to a new filter once the filter exists. I was just having trouble trying to understand how to populate a filter with a static list of letters opposed to an enum/returned api list.
When the letter filter exists, it should only take me an hour or so to point the letter bar to that instead of the old version I was working with. The only quirk I am unsure about is how it will interact with the new library/list view. In particular, with how you are determining the size of posters and how much available screen space there is. But, we can get there when we get there. I'll keep an eye out for the new PR you made I will update this branch, and see about implementing the letter bar. From there, there might be a point where we need to have a discussion about spacing, but I can ping you when I get to that point!
@LePips Thank you for setting up the filter! Getting this migrated to the new filter was a cakewalk. Please see the filter below set to the Letter X. Re-selecting X disables the filter.
Filter in Action
My next step is I want to re-add a setting to enable, disable, and choose Left vs Right side of the screen. That shouldn't take too long but I wanted to make sure that:
- PagingLibraryView was the right place to put this
- Does that spacing look okay with this AlphaPicker there?
- For the setting, Letter Bar an okay name for it? AlphaPicker? I'm terrible with naming so whatever sounds best to you.
With this last push, I think I have all of the items I am looking for but there are some rough parts that I want to address before pushing.
Completed Items:
Created an Alpha Picker Setting
Created New Labels for these Elements (Alpha Picker **Title** so if that changes we can just update the title)
Functioning Filters from Alpha Picker Selection
Flip from Static View to ScrollView if there are more than 27 Characters
Outstanding Considerations
- The flip from View to ScrollView for Non-English looks a tad cramped?
- Is the Paging Library View the best place for this to be? Is the alphaPickerWrapperView() a solution to this or does that just add noise?
- I removed the .ignoresSafeArea() on the Paging Library View to keep the AlphaPicker with Non-English characters in view. Let me know if that will cause any issues or if there is a better implementation.
- The setting is Left, Right, None with the Default to Right to mirror Jellyfin-Web. The backend names for these are .leading, .trailing, and .none. Let me know if that is the right way to use that or if the label should match the case.
- [Edit Consideration] I put the Alpha Picker logic in the Shared folder with the hopes that this would have some cross purpose between tvOS and iOS. The main changes would need to occur in the alphaPickerButton for sizing but I think it should otherwise be usable. This does NOT impact the ability to build tvOS so it appears find from my end but let me know if this would be better in only the iOS folder.
Let me know!
@LePips I've resolved most of the items you mentioned. I have a questions about some of the items you said I could remove just to make sure I understood. Lastly, the only outstanding issue is this item: https://github.com/jellyfin/Swiftfin/pull/980#discussion_r1614816506
Non-English/Romantic characters create an issue where the scrollbar starts in the safeArea and as a result the letters at the top and bottom cannot be scrolled to for smaller iPhones. iPad and Max iPhones are not impacted but there is the added issue that those devices, I incorrectly turn it into a scroll when a static bar works better and the scroll is not required. So I wasn't sure if this item was more incorporated into a scroll(ifLargerThan:) change or if that would still be impacted. I've included a screenshot.
This should now work without filters. This works for non-English/Romantic characters and stays in screen but I think it's a little hacky since I am running the check for non-Romantic but skipping it for Romantic since it seems to loop forever with English. I'm 100% up for changes on that! SwiftUI is definitely my weakpoint here haha.
Alright! I think I finally have everything cleaned up @LePips. I think there is one final outstanding item that I can see and that's that the position of the scroll is reset when the letter is selected. I've hit a bit of a wall. I know I've resolved something like that in the past but I'm drawing a blank tonight. I can take a look at that another day.
The only thing that is new/not suggested by you is the LetterPickerOverflow class to handle the non-romantic characters. This was modeled off of what I read here: https://stackoverflow.com/questions/62463142/make-scrollview-scrollable-only-if-it-exceeds-the-height-of-the-window. Let me know if there is anything there I should be changing. This does the scroll based on screen size now. In testing this was my finding:
- iPhone SE: Always Scrolls even in English
- iPhone 15 Pro: Static in English but Scrolls in Korean
- iPad 13": Always Static even in Korean