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

feat: Support multiple filter selection #5478

Merged
merged 10 commits into from
Jan 12, 2024
Merged

Conversation

RafaelsRamos
Copy link
Contributor

@RafaelsRamos RafaelsRamos commented Jan 11, 2024

What does this PR do?

  1. Fixed typo on filter preference key;
  2. Create a bottom sheet for filtering & sorting;
  3. Ensures Home & Subscription screens filter their content accordingly;
  4. closes Adding multiple selection to the subscription feed filter  #5453

Notes

Transition

Fixing the typo on the filter preference key results in resetting the users' current filter preference. This is intended since it greatly simplifies the logic to support multiple filters. If we decide that we want to transition to multiple filter support without a reset, that can be achieved by reverting the typo fix + adjusting the following property on ContentFilter.kt:

private val enabledFiltersSet get() = PreferenceHelper
    .getString(SELECTED_FEED_FILTERS, "1,2,3")
    // Line below supports a transition from the currently selected filter
    .let { filtersPref -> if (filtersPref == "0") "1,2,3" else filtersPref } 
    .split(',')
    .toMutableSet()

If you think it is worth it, we can go ahead and add support for a transition from the currently selected filter.

Showcase

sample.mp4

Closing Notes

Thank you for reviewing! Please let me know if you disagree with the approaches I chose.

@RafaelsRamos RafaelsRamos marked this pull request as draft January 11, 2024 13:05
@RafaelsRamos RafaelsRamos marked this pull request as ready for review January 11, 2024 13:07
@Bnyro
Copy link
Member

Bnyro commented Jan 11, 2024

Fixing the typo on the filter preference key results in resetting the users' current filter preference. This is intended since it greatly simplifies the logic to support multiple filters. If we decide that we want to transition to multiple filter support without a reset, that can be achieved by reverting the typo fix + adjusting the following property on ContentFilter.kt:

I think it's fine to change the preference key for the transition, I already did the same thing multiple times before when refactoring a preference.

@Bnyro
Copy link
Member

Bnyro commented Jan 11, 2024

Looks pretty good and well overthought from the UI/UX perspective, only left some recommendations for the code implementation, which would make things a bit cleaner overall 👍

Thank you for looking into this!

@M00NJ
Copy link

M00NJ commented Jan 11, 2024

Awsome!
Since the player screen design ideas where well recieved I wanted to look at some other screens in the app and propose improvements. This was one of the ideas to make the subscription tab ui a bit cleaner. Cool that it's already implemented :)

@RafaelsRamos
Copy link
Contributor Author

Awsome! Since the player screen design ideas where well recieved I wanted to look at some other screens in the app and propose improvements. This was one of the ideas to make the subscription tab ui a bit cleaner. Cool that it's already implemented :)

That's on @SameenAhnaf , he's the one that proposed that UI 💪

@RafaelsRamos
Copy link
Contributor Author

Looks pretty good and well overthought from the UI/UX perspective, only left some recommendations for the code implementation, which would make things a bit cleaner overall 👍

Thank you for looking into this!

Thank you for the review Bnyro!

I hope I have not forgotten any of the changes we discussed 😊

@Bnyro
Copy link
Member

Bnyro commented Jan 12, 2024

The above suggestions (except the SVG thing) are only a small refactor to avoid crashes when the bottom sheet is being recreated, since the var sortOptions = ... value won't be kept either when the sheet is killed. Hence we must pass it as an intent data argument in order to not become killed by Android lifecycle.

@RafaelsRamos
Copy link
Contributor Author

Hey Bnyro, thank you for catching this issue. It would lead to some unnecessary crashes...

Updated the icon's formatting + passed the parameters through arguments.

One point to note. I was not able to use the recommended arguments.parcelable(...) due to the fact that the argument being passed is a collection of items. For that reason, I used the arguments.getParcelableArray, I hope that's OK.

@Bnyro
Copy link
Member

Bnyro commented Jan 12, 2024

One point to note. I was not able to use the recommended arguments.parcelable(...) due to the fact that the argument being passed is a collection of items. For that reason, I used the arguments.getParcelableArray, I hope that's OK.

Sure 👍

I didn't test the code at all before commenting, it was just more of a general hint how I'd imagine the changes to be :)

Copy link
Member

@Bnyro Bnyro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm now, thank you!

Would you mind rebasing squashing your commits into two or three so that we can make the history of commits cleaner. Or should I just squash the whole PR into one commit when merging?

@RafaelsRamos
Copy link
Contributor Author

RafaelsRamos commented Jan 12, 2024

Yeah go ahead! I just like committing multiple times to keep track of what was done (it helps me with the PR description).

@Bnyro Bnyro merged commit 444eb69 into libre-tube:master Jan 12, 2024
3 of 4 checks passed
@RafaelsRamos RafaelsRamos deleted the feat/5453 branch January 22, 2024 13:49
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.

Adding multiple selection to the subscription feed filter
3 participants