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 Toggles [iOS] [iPadOS] #847

Merged
merged 11 commits into from
Sep 20, 2023
Merged

Filter Toggles [iOS] [iPadOS] #847

merged 11 commits into from
Sep 20, 2023

Conversation

JPKribs
Copy link
Member

@JPKribs JPKribs commented Sep 16, 2023

Added an option in Settings > Accessibility > Customization for enabling/disabling Library & Search Filters. When enabled, this removed the Filter Pill from the HStack for the particular selected filter. Filters can be reordered from the same screen. This should be a simple change but the goal is to create this section/setting so as future filters are added they can be enabled/disabled to reduce clutter and improve UX.

If all filters are disabled, I chose to hide the entire Filter HStack. Please see the example images below:

Settings Location

Adjusting Search Filters
Search Results

Adjusting Library Filters
Library Results

Disabling All of the Filters
Search Results
Library Results

The eventual goal is to enable the functionality from this: #680 but I wanted to ensure that this option could be disabled if unwanted. Instead of just adding an option for this "Filter by Letter" filter, this will allow for any filter to be disabled or re-ordered.

JPKribs and others added 4 commits September 15, 2023 21:01
Settings in the Customization menu to enable/disable Library Filters.
Set all Filter Defaults to True.
From the Customization > Filters option, enable or disable a filter.
…ters are disabled. If one or more filters are enabled, show the HStack.
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 like the idea! 👍

However, I would want this to have a similar implementation as the video player action buttons. I found it a lot more customizable and even more manageable than a bunch of toggles - it makes adding new stuff almost trivial.

Relevant settings view: https://github.com/jellyfin/Swiftfin/blob/main/Swiftfin/Views/SettingsView/VideoPlayerSettingsView/Components/ActionButtonSelectorView.swift

Additionally, you can implement the "general implementation" I note at the top for this kind of interaction, if you want.


PR Description Meta

I do really like having screenshots in PRs, however I like them a lot better formatted so they make things easier to navigate/read. I use the below format on my portrait screenshots.

<details>
<summary>Screenshot Title</summary>

<img src="<image url>" width="30%" height="30%">
</details>

After you drag the photo I take the image url out of the markdown and put it into the src. It does take some time but makes things a lot more manageable.

@JPKribs
Copy link
Member Author

JPKribs commented Sep 16, 2023

Sounds good! Sorry about the formatting, I've only ever used VS + TFVC for code so I'm still a bit of a git novice.

Just to confirm, implement a filterSettingsView Enum that points back to the Defaults.Filters. Use this enum to pass in the labels and filters using a ForEach(_selectedButtons) back on the filterDrawerHStack?

@JPKribs
Copy link
Member Author

JPKribs commented Sep 16, 2023

I like the idea! 👍

However, I would want this to have a similar implementation as the video player action buttons. I found it a lot more customizable and even more manageable than a bunch of toggles - it makes adding new stuff almost trivial.

Relevant settings view: https://github.com/jellyfin/Swiftfin/blob/main/Swiftfin/Views/SettingsView/VideoPlayerSettingsView/Components/ActionButtonSelectorView.swift

Additionally, you can implement the "general implementation" I note at the top for this kind of interaction, if you want.
PR Description Meta

I do really like having screenshots in PRs, however I like them a lot better formatted so they make things easier to navigate/read. I use the below format on my portrait screenshots.

<details>
<summary>Screenshot Title</summary>

<img src="<image url>" width="30%" height="30%">
</details>

After you drag the photo I take the image url out of the markdown and put it into the src. It does take some time but makes things a lot more manageable.

Just to confirm, is this what you meant:

Changed the Filter Section to a Sub-Section of Library
Updated the Filters from Individual Enums to a Single Array

I also added those icons on the selection but my heart isn't sold on them. I was just trying to mirror the Video Player buttons.

@LePips
Copy link
Member

LePips commented Sep 16, 2023

Yep just like that, but I don't think we need the icons either as they aren't shown anywhere else.

@JPKribs
Copy link
Member Author

JPKribs commented Sep 16, 2023

Yep just like that, but I don't think we need the icons either as they aren't shown anywhere else.

I have this working now with one exception. I've moved the label, filter, and selectorType into the enum for the Filters. The only issue I have now is that the "Activated" flag is determined by:

viewModel.CurrentFilters.<filter/genres> != []
I could just add a function for isActive but I don't think we want a function on the view nor the enum. What would you say is the best route for this?

My Revisions to FilterDrawerHStack. Highlighted the Activated call.
Original FilterDrawerHStack. Highlighted the Activated call.

@LePips
Copy link
Member

LePips commented Sep 16, 2023

Ah, I see. I'm find with a function on the new enum that takes on an ItemFilters parameter and determines whether that filter is active or not.

…ans. Since this is now an array, this allows re-ordering the filters. FilterDrawerHStack will now be populated by a ForEach building the HStack from the items enabled in the Array. Created a second, possibly unnecessary function on the FilterDrawerHStack that finds the property value for the ItemsFilter to identify which filters should be highlighted as active and which are inactive.
@JPKribs JPKribs marked this pull request as draft September 17, 2023 00:55
@JPKribs JPKribs marked this pull request as ready for review September 17, 2023 01:01
@JPKribs
Copy link
Member Author

JPKribs commented Sep 17, 2023

Ah, I see. I'm find with a function on the new enum that takes on an ItemFilters parameter and determines whether that filter is active or not.

I've made these changes! I added an enum for filterDrawerActionButton that contains all of the components for adding in new filters. Moving forward, this should enable new filters by just updating the enum here and it will be populated in the settings and HStack.

The only item that I feel warrants some discussion is how I am setting the Filter Pills to Active/Inactive:

func getFilterProperty<T>(from object: T, propertyName: String) -> [ItemFilters.Filter] {
        let mirror = Mirror(reflecting: object)

        for child in mirror.children {
            if let label = child.label, label == propertyName {
                if let filterProperties = child.value as? [ItemFilters.Filter] {
                    return filterProperties
                }
            }
        }

        return []
    }

The purpose of this function is to find the the ItemFilters.Filters using the String value of the Filter. In my testing it works well but it does add a ForLoop so I'm up for other ideas. The reason I chose this approach is that it enables all of the filter configuration to be done in the filterDrawerActionButton Enum. This prevents the need for additional updates to logic in the FilterDrawerHStack when adding new filters.

Let me know if you can see anything else that needs attention!

@JPKribs JPKribs requested a review from LePips September 17, 2023 06:30
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 left a few comments on what I could see after pulling this down. Since there was a reference issue with the file and I had some time, I went ahead and did a more general implementation below, but it isn't complete and I leave it to you to fill in the gaps, like the colors on the buttons in the general solution are not right.

This was a great start, and I had to think also about how this is used in conjunction with search since that uses the filters too. So, this entire feature was split into its own section in the settings that controls how all the filters work. I don't really care for a Library vs Search filters customization - even though I think someone will ask for that in the future.

I did rename some things but I'm agnostic, you can change them back if you would like. However I don't think these represent "actions" like the video player buttons do. Additionally, the filters themselves have been broken for some time and I may redo the entire filter implementation (but this feature would have minor modifications to accommodate that).

Shared/Coordinators/SettingsCoordinator.swift Outdated Show resolved Hide resolved
Shared/Coordinators/SettingsCoordinator.swift Outdated Show resolved Hide resolved
@JPKribs
Copy link
Member Author

JPKribs commented Sep 18, 2023

I left a few comments on what I could see after pulling this down. Since there was a reference issue with the file and I had some time, I went ahead and did a more general implementation below, but it isn't complete and I leave it to you to fill in the gaps, like the colors on the buttons in the general solution are not right.

* https://github.com/LePips/SwiftFin/tree/filter-drawer-customization

This was a great start, and I had to think also about how this is used in conjunction with search since that uses the filters too. So, this entire feature was split into its own section in the settings that controls how all the filters work. I don't really care for a Library vs Search filters customization - even though I think someone will ask for that in the future.

I did rename some things but I'm agnostic, you can change them back if you would like. However I don't think these represent "actions" like the video player buttons do. Additionally, the filters themselves have been broken for some time and I may redo the entire filter implementation (but this feature would have minor modifications to accommodate that).

If we are breaking Filters out into its own section, I would be happy to break it out into Library Filters and Search Filters. Then, just add another input to the fitlerDrawerHStack for filterType. Add an enum for FilterType as "Search" or "Library". Then just pull a separate setting for each of them. I would agree that I don't see this as needing to be separate but it makes sense to just knock out both of these in a single go. If you think that would be best to do, I can do that and re-submit with your requested changes.

Otherwise, I can try and get this version of this ready to go then make a new PR for "Split out Library and Search Filters." to keep it all easier to version.

Joe Kribs added 2 commits September 18, 2023 10:32
…self. All edits for filters are still done on the enum itself. Alphabetized the FilterDrawer entry in the SettingssCoordinator. I *think* changed the SettingsCoordinator to submit from the project folder and not from my documents. Turned off the filter drawer from the library/search view instead of returning and empty filterdrawer.
…ction in customization. Created a FilterDrawerType Enum for Library vs Search filters. Allows for expansion later. Changed logic for the LibrarySearch Views to point to the correct HStack Filter Drawer.
Swiftfin.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
Shared/Objects/FilterDrawerType.swift Outdated Show resolved Hide resolved
Shared/Objects/FilterDrawerButton.swift Outdated Show resolved Hide resolved
Shared/Objects/FilterDrawerButton.swift Outdated Show resolved Hide resolved
Shared/Objects/FilterDrawerButton.swift Outdated Show resolved Hide resolved
Swiftfin/Views/LibraryView.swift Outdated Show resolved Hide resolved
Shared/Objects/FilterDrawerType.swift Outdated Show resolved Hide resolved
Shared/Objects/FilterDrawerButton.swift Outdated Show resolved Hide resolved
Shared/Objects/FilterDrawerButton.swift Outdated Show resolved Hide resolved
Joe Kribs added 2 commits September 18, 2023 19:48
… duplicate Class Names. Removed the FilterDrawerTypes by instead just passing the Default.Filters.Search/Library from the respective Search/LibraryView. Updated all of the reference points with the name names. Re-added the default FilterDrawerSelection Defaults for usage in determining if the button should be active. Updated the Search/LibraryView to ACTUALLY drop the NavBarDrawer if unused instead of returning an empty bar.
@JPKribs
Copy link
Member Author

JPKribs commented Sep 19, 2023

I believe all changes are in with the exception of potentially changing #847 (comment) to be cleaner. Updated the original post with new screenshots and a more accurate description of what this PR does.

LePips
LePips previously approved these changes Sep 19, 2023
…ied maximum width." Resolves tvOS Build errors.
@LePips LePips merged commit 48e608e into jellyfin:main Sep 20, 2023
3 checks passed
@JPKribs JPKribs deleted the filtertoggles branch October 16, 2023 20:55
@LePips LePips added the enhancement New feature or request label Apr 8, 2024
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.

2 participants