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

Filter by Letter [iOS] [iPadOS] #852

Closed
wants to merge 0 commits into from
Closed

Conversation

JPKribs
Copy link
Member

@JPKribs JPKribs commented Sep 19, 2023

Creation of a Filter to Filter by the Starting Letter of a media item. Resolves the request created here: #680

Relies on: #847


New Settings

Defaults to "Disabled", resulting in no Lettered Scrollbar.

Alpha Picker Selection
Letter Filter

Letter Filter

NameStartsWith Filter
Drop Down Letter List
Letter Selection
Filtered Library
Filtered Library on Non-Existing Letter

Alpha Picker

Disabled (Default)
Right
Left
Selected Letter
iPad Vertical
iPad Horizontal

Done:

  • Created a new class for ItemLetterFilter as an Enum for all of the letters.
  • Added the ItemLetterFilter to the ItemFilters and FilterDrawerSelection to be usable as a Filter and FilterDrawerButton respectively.
  • Updated the Search & Library ViewModels to include the nameStartsWith parameter to the API Call using the selection from the Letters FilterDrawerButton.
  • Created another catch to prevent endless loading when reaching the end of the page of a letter-filtered view.*
  • Created a localization label for "Letter."
  • Re-Base on Main after Filter Toggles [iOS] [iPadOS] #847
  • Created the alphaPicker enum, alphaPicker view, and alphaPickerButton view
  • Added the alphaPicker next to the libraryView (if enabled)

*Will need a second set of eyes to confirm this is best practice.

@JPKribs JPKribs marked this pull request as draft September 19, 2023 04:03
@JPKribs JPKribs changed the title Filterbyletter Filter by Letter [iOS] [iPadOS} Sep 19, 2023
@JPKribs JPKribs changed the title Filter by Letter [iOS] [iPadOS} Filter by Letter [iOS] [iPadOS] Sep 19, 2023
@LePips
Copy link
Member

LePips commented Sep 19, 2023

jumps to a letter opposed to filtering on a letter ... would require the entire library up to that letter be loaded in prior to scrolling to it.

I do agree that a jump mechanism would be nicer, however something like that would require a more advanced mechanism to how we load the library. Ideally, we would want to implement a custom UICollectionView for these libraries and refactor how we load from the server entirely and store the results in some kind of ordered set. That is pretty advanced stuff and would require a lot of dedication to it.

@JPKribs
Copy link
Member Author

JPKribs commented Sep 20, 2023

jumps to a letter opposed to filtering on a letter ... would require the entire library up to that letter be loaded in prior to scrolling to it.

I do agree that a jump mechanism would be nicer, however something like that would require a more advanced mechanism to how we load the library. Ideally, we would want to implement a custom UICollectionView for these libraries and refactor how we load from the server entirely and store the results in some kind of ordered set. That is pretty advanced stuff and would require a lot of dedication to it.

I've been able to somewhat re-create the experience of the AlphaPicker from Jellyfin-Web where it just filters on nameStartsWith then deactivates when you select the same letter. For now, my outstanding issue is I want the alphaPicker to only exist on screen orientation where the screen size is tall enough where all of the letters can fit. Right now, I have it where when onAppear() it makes that check, but if you change the orientation of your screen, it won't update. I have some downtime at the airport next week where I was going to look at cleaning this up more.

Do you think it makes sense to proceed with the AlphaPicker route as a stopgap to "mirror" Jellyfin-Web? Or, would it make more sense for me to spend some time buckling down on creating our own version of collectionView? Or, I mean, if you think this is also some thing that doesn't make sense for someone at my level to look at, I wouldn't be offended either.

@JPKribs
Copy link
Member Author

JPKribs commented Sep 22, 2023

Working on this, I'm trying to get the alphaPicker to be hidden if it will exceed the size of the screen. I am using:

NotificationCenter.default.addObserver(forName: UIDevice.orientationDidChangeNotification, object: nil, queue: .main)

Then using this as an observable object. Is this the best way to achieve this?

@LePips
Copy link
Member

LePips commented Sep 22, 2023

Only briefly looking, I'd have to see how I feel about it not allowing to be shown on smaller screens. Frankly, I'm fine with people enabling it on small devices even if they know the size problems.

But until I think about that, you can take a look at the onSizeChanged/onFrameChanged modifiers instead of listening to notifications (which I would prefer).

@JPKribs
Copy link
Member Author

JPKribs commented Sep 22, 2023

Only briefly looking, I'd have to see how I feel about it not allowing to be shown on smaller screens. Frankly, I'm fine with people enabling it on small devices even if they know the size problems.

But until I think about that, you can take a look at the onSizeChanged/onFrameChanged modifiers instead of listening to notifications (which I would prefer).

At least on the emulator, it looks like the full letter bar shows up in portrait mode, even on the mini/SE. I am primarily concerned with horizontal phones since there's really no spacing/sizing I could do to make it fit there.

I could turn it into a scrollView? I agree with a comment you made earlier about putting a scrollview next to the libraryview feels a little clunky... that being said, that could be a better solution than checking screen resolution? I personally prefer the static letters that are hidden but what do you think @LePips?

I think my only other outstanding item I wanted to vet is potentially adding a setting to expand the size of the letters. Just because, on the iPad, it can be a little small comparatively (see original post images).

@LePips
Copy link
Member

LePips commented Sep 23, 2023

horizontal phones

I'm not worried about that since we should be making the app portrait only on iOS except for the video player. If you say that all of the letters show in portrait, then we shouldn't need a scroll view then.

adding a setting to expand the size of the letters

I'm against this, as now we're just getting way too granular. I'm fine with checking at runtime the device type (iPhone, iPad) and having a different font size.

buckling down on creating our own version of collectionView

Let's not do that right now, I'm exhausted just even thinking about how much work that would/will take.


Localization

I haven't mentioned this but it was first on my mind when I saw this feature: localization. While I know that this mirrors the web alphabet picker, I'm unsure how this interacts with other non-English alphabets (Japanese, Korean, etc.). Now, I'm pretty uneducated in this topic of accessibility but want to eventually get into it. I only bring this up to say that we shouldn't be surprised if this solution eventually (years from now at the least tbh) gets replaced by a more general solution.

@JPKribs
Copy link
Member Author

JPKribs commented Sep 23, 2023

horizontal phones

I'm not worried about that since we should be making the app portrait only on iOS except for the video player. If you say that all of the letters show in portrait, then we shouldn't need a scroll view then.

adding a setting to expand the size of the letters

I'm against this, as now we're just getting way too granular. I'm fine with checking at runtime the device type (iPhone, iPad) and having a different font size.

buckling down on creating our own version of collectionView

Let's not do that right now, I'm exhausted just even thinking about how much work that would/will take.

Localization

I haven't mentioned this but it was first on my mind when I saw this feature: localization. While I know that this mirrors the web alphabet picker, I'm unsure how this interacts with other non-English alphabets (Japanese, Korean, etc.). Now, I'm pretty uneducated in this topic of accessibility but want to eventually get into it. I only bring this up to say that we shouldn't be surprised if this solution eventually (years from now at the least tbh) gets replaced by a more general solution.

Sounds good. Personally, locking to portrait on iPhone is a much cleaner solution so I've removed my orientation management class and logic that was trying to remove/hide the bar. If it's enabled it will always be enabled.

For now, I have them both using the same layout but I've added the option to pass in a font so that should make making an iPad vs iPhone easy.

Localization

I completely agree with you! I was asking around when I was first trying to get my head around Swift just to understand what localization for this looks like. My understanding is that this nameStartsWith parameter on the API only does Romantic letters and numbers. So, at this time there is no localization available for this (I assume this is mirror on Jellyfin-Web).

TBH, when the day comes I have no idea how this could even be localized. I suppose if we had a localization for "Alphabet" that was just [A,B,C,D...] then we could parse it? I know those localization are just strings so I don't know how that would work.

Without having to look at handling orientation changes/Alpha-Picker exceeding the screen I think I am at a good for a second set of eyes.

@JPKribs JPKribs marked this pull request as ready for review September 23, 2023 05:17
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.

I want this to go in a different direction now since I actually took some time to see how we could localize this and found out it was more accessible than expected. I won't be all encompassing on the SwiftUI changes that I want until after some refactoring.

Shared/Objects/AlphaPickerSelection.swift Outdated Show resolved Hide resolved
Shared/Components/AlphaPicker/AlphaPickerButton.swift Outdated Show resolved Hide resolved
Shared/Components/AlphaPicker/AlphaPickerView.swift Outdated Show resolved Hide resolved
Shared/Objects/AlphaPickerSelection.swift Outdated Show resolved Hide resolved
Shared/Strings/Strings.swift Outdated Show resolved Hide resolved
Shared/Objects/ItemLetterFilter.swift Outdated Show resolved Hide resolved
Shared/Components/AlphaPicker/AlphaPickerButton.swift Outdated Show resolved Hide resolved
Shared/Components/AlphaPicker/AlphaPickerButton.swift Outdated Show resolved Hide resolved
Shared/Components/AlphaPicker/AlphaPickerView.swift Outdated Show resolved Hide resolved
Shared/Components/AlphaPicker/AlphaPickerButton.swift Outdated Show resolved Hide resolved
@JPKribs JPKribs requested a review from LePips September 26, 2023 05:26
@JPKribs
Copy link
Member Author

JPKribs commented Sep 26, 2023

I believe I've made the changes requested! I have three outstanding "maybes" that I would appreciate a second set of eyes on.

  1. The creation of AlphaPickerCharacters to hold the letters/characters from the localized language. Is this the best way to do this? Should this be an enum or a function?

The goal would be to eventually be able to swap this out. The reason I would want to swap this out is to be more dynamic and instead of "Filter based on localization" it should be "Filter based on the first found characters for the media present." This way instead of having the potentially of a bunch of characters that return an empty view, we only have letters for existing media. The biggest downside to this would be that sometimes the AlphaPicker would be very small. That would make appropriate theming/formatting harder to consistently look nice. Additionally, I'm not sure how to achieve getting all used first letters at this point without being VERY performance heavy but I think this would be the ideal solution personally pending the formatting solution.

  1. Should I declare another filter for nameLessThan or am I fine with the logic on the libraryViewModel? Instead of a second filter, I just have logic on the Model View for for "IF # THEN nameLessThan ELSE nameStartsWith" The current logic works but I just want to make sure there isn't a better way. I personally like this solution since there are less items to maintain but the alternative solution is a boolean filter for nameLessThan but then I would need additional logic to ignore the nameStartsWith... I like the current solution better but let me know if there is a better way to approach this.

  2. In the AlphaPicker I have turning into a ScrollView when > 27 characters. This is because 27 characters will be the Romantic + #. Since the API referenced returns Localized + Romantic + #, this will always exceed 27 on non-Romantic languages. In which case, I change this to a ScrollView to fit additional letters. Is this an okay solution or do we want to only include localized characters and not Romantic + localized? I lean in favor of Romantic + localized since I know there is a lot media created in western countries so non-romantic Language speakers will still likely utilize Romantic Languages for their media. This is just my thought for now unless we have a route to achieve (1.

@JPKribs JPKribs marked this pull request as draft September 29, 2023 18:20
@JPKribs JPKribs marked this pull request as ready for review September 29, 2023 18:21
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.

After having looked at my filters implementation in these PRs, I will definitely think of refactoring them. I don't like the type erasure (Filters) and having multiple layers (FilterViewModel + ItemFilters).

Swiftfin/Components/AlphaPicker/AlphaPickerButton.swift Outdated Show resolved Hide resolved
Swiftfin/Components/AlphaPicker/AlphaPickerButton.swift Outdated Show resolved Hide resolved
Swiftfin/Components/AlphaPicker/AlphaPickerView.swift Outdated Show resolved Hide resolved
Swiftfin/Components/AlphaPicker/AlphaPickerView.swift Outdated Show resolved Hide resolved
Swiftfin/Components/AlphaPicker/AlphaPickerButton.swift Outdated Show resolved Hide resolved
Shared/Strings/Strings.swift Outdated Show resolved Hide resolved
@JPKribs
Copy link
Member Author

JPKribs commented Oct 9, 2023

After having looked at my filters implementation in these PRs, I will definitely think of refactoring them. I don't like the type erasure (Filters) and having multiple layers (FilterViewModel + ItemFilters).

Would be the same route/rework for implementing filters on tvOS? Or is that a whole other beast?

@LePips
Copy link
Member

LePips commented Oct 9, 2023

The refactoring would change how filters works on both platforms, but I wouldn't be implementing them on tvOS with that work.

@JPKribs
Copy link
Member Author

JPKribs commented Oct 11, 2023

All reviews should be resolved in this latest commit. I will likely have some linting changes that need to be addressed but at this time I believe the code portion of this is ready.

@JPKribs JPKribs requested a review from LePips October 11, 2023 23:03
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.

Syntax and fix merge conflicts.

Swiftfin/Components/AlphaPicker/AlphaPickerButton.swift Outdated Show resolved Hide resolved
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.

2 participants