Add card browser chip filtering for flags #17355

lukstbit · 2024-11-04T12:05:16Z

Purpose / Description

Adds a part of the CardBrowser new chip filtering, targeting flags. The PR uses code from the original attempt at #16859 where possible. See more info in the messages of commits.

UI note: I think it would be better if we just show Flags +x instead of FlagName +1:

I find it confusing to have a chip checked and its text to read "No flag"
while for single flag selection having the name makes sense, this loses its advantage when you also have +x because you don't have any info about the other flags so you're forced to open the sheet again anyway
makes it easier to keep all filtering chips on screen. If we have a longer tag name + flag name it's easy to push the last filtering chip(state) outside of the screen
would have a more consistent UI if all chip filtering would use the same system: Tags +2 Flags +2 Card state +2

Some images:

Fixes

Towards Browser chips #12554

How Has This Been Tested?

Ran the tests and filtered by flag.

Checklist

You have a descriptive commit message with a short title (first line, max 50 chars).
You have commented your code, particularly in hard-to-understand areas
You have performed a self-review of your own code
UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
UI Changes: You have tested your change using the Google Accessibility Scanner

github-actions · 2024-11-04T12:05:32Z

Important

Maintainers: This PR contains Strings changes

Sync Translations before merging this PR and wait for the action to complete
Review and merge the auto-generated PR in order to sync all user-submitted translations
Sync Translations again and merge the PR so the huge automated string changes caused by merging this PR are by themselves and easy to review

lukstbit · 2024-12-08T08:33:16Z

I fixed the conflicts for this.

Note: I also did the filtering for card states but I didn't add the PR yet because it sits on top of this.
I also thought of an addition to chip filtering area that I think it would be nice but I'll propose that change after these two are done.

david-allison

Clicking on the flag icons has no effect other than a ripple

Also for the X in the 'clear filter'

Clear filter should allow a user to click anywhere in the row, as with the flag icons, rather than just the text

Other than that, THANK YOU SO MUCH! Looks great

AnkiDroid/src/main/java/com/ichi2/anki/browser/Chips.kt

david-allison · 2024-12-08T09:51:56Z

AnkiDroid/src/main/java/com/ichi2/anki/browser/SearchParameters.kt

+    val userInput: String,
+    val deckIds: Set<DeckId>,
+    val tags: Set<String>,
+    val flags: Set<Flag>
+) : Parcelable {
+    val isEmpty get() = userInput.isEmpty() && deckIds.isEmpty() && tags.isEmpty() && flags.isEmpty()
+    val isNotEmpty get() = !isEmpty


This seems like too many parameters, is tags necessary

Yes, we plan to add a chip for filtering by tags(or so it was done in your original PR).

AnkiDroid/src/main/res/layout/fragment_flags_filter_sheet.xml

AnkiDroid/src/main/res/layout/item_browser_flag_filter.xml

david-allison · 2024-12-08T09:53:29Z

AnkiDroid/src/main/res/values/07-cardbrowser.xml

+    <string name="chip_filter_multiple_selections" comment="Arg 1: Selected filter ('Red Flag')
+    Arg 2: Number of additional selected filters (2: Green and Blue Flags)
+    Example shown to use: 'Red +2'
+    When displaying a chip control for a filter, if one element is selected, we display it
+    If more than one element is selected, we also show the count of extra filters">%1$s +%2$d</string>


I don't think this needs to be a plural, but I'm not certain

I'm not sure I understand. This string can't be a plural because the quantity is represented by numeric values.

david-allison · 2024-12-08T09:54:37Z

AnkiDroid/src/test/java/com/ichi2/anki/browser/CardBrowserViewModelTest.kt

@@ -41,7 +41,6 @@ import com.ichi2.anki.model.SortType.EASE
 import com.ichi2.anki.model.SortType.NO_SORTING
 import com.ichi2.anki.model.SortType.SORT_FIELD
 import com.ichi2.anki.servicelayer.NoteService
-import com.ichi2.anki.setFlagFilterSync
 import com.ichi2.anki.utils.ext.ifNotZero


Do we want additional test coverage here?

What do you have in mind? Extra tests for the flag filtering itself or for the code that was introduced to support filtering(SearchParameters)?

Note: this is the original code from the first attempt, I didn't modified it in any way.

david-allison · 2024-12-24T07:29:38Z

@lukstbit is this still awaiting comment resolution, and can I help at all? Would love to see it in

TODO: Thank Oakkitten for the code Cherry-picked from ankidroid#16859, commit 2b89111 There was a conflict related to imports in card browser that was fixed.

This fixes the bug ankidroid@2b89111#r1731116500 where using a top level deck id in the search query will result in an empty result set although its child decks do have cards. The fix consists in properly creating the search query by switching from "did" to "deck" for deck selection(ex did:435435 -> deck:"Deck name"). Looking though the docs, it seems "did" isn't a valid selector for a deck and the selector "deck" must be used instead. Note that the deck name is put in "" to accommodate decks with spaces in name. Note: I choose to make toQuery() suspended to access the collection for the decks names to avoid SearchParameters referencing the decks names and then having to synchronize with outside name changes.

Cherry-picked from ankidroid#16859, commit 6565924 + other changes Other changes: - renamed bottom_sheet_item -> item_browser_flag_filter - refactor item_browser_flag_filter to simplify and remove unused views - removed the BottomSheetFragment, TreeAdapter and TemplatedTreeAdapter classes as they will not be used for flags filtering - removed bg_input.xml and bottom_sheet_list_without_filter.xml as they are unused - removed the chip for flags filtering, it will be added in a separate commit - moved BottomSheetDialogFragmentFix to the anki.workarounds package, I assumed we might want this in other places as well - renamed the id of the ChipGroup from chips_group to filtering_chips_group

Code partially cherry-picked from ankidroid#16859, commit 4254250 References to flags filtering were removed and will(partially) be added in the next commit. The code works now as before related to flags filtering.

The flags filtering was moved from a menu option to an embedded chip view triggering a separate bottom sheet dialog. Contains some code from commit 4254250 that was about flags filtering. Note: see the changes in CardBrowserTest.checkIfLongSelectChecksAllCardsInBetween(). For some reason after adding the tests related to flag filtering, this test started to throw NullPointerExceptions which also triggered another test to crash as well due to unhandled exception from another test. Using a position that should be displayed seems to fix this.

lukstbit · 2024-12-24T13:18:41Z

Sorry, I was just busy and didn't had time to update the code.

david-allison

I'm so sorry, didn't realize this was open for review! This is great, one blocker

⚠️ Filter by flag in the menu does nothing + can be removed

design nitpicks, non-blocking

If 'clear' is not visible, the top of the sheet could do with a few more dp blank space
Could do with a few dp padding to the end of the flag icon
I /think/ the font may be a little larger/bolder than gmail and could be reduced by a few dp

lukstbit added the Needs Review label Nov 4, 2024

github-actions bot added the Strings label Nov 4, 2024

github-actions bot added the Has Conflicts label Dec 4, 2024

lukstbit force-pushed the feat_browserChipFiltering branch from fa1f5c9 to e34147f Compare December 8, 2024 08:17

lukstbit removed the Has Conflicts label Dec 8, 2024

lukstbit added this to the 2.21 release milestone Dec 8, 2024

david-allison requested changes Dec 8, 2024

View reviewed changes

david-allison added the Needs Author Reply Waiting for a reply from the original author label Dec 9, 2024

github-actions bot added the Has Conflicts label Dec 9, 2024

This comment has been minimized.

Sign in to view

github-actions bot added the Stale label Dec 23, 2024

david-allison removed the Stale label Dec 24, 2024

oakkitten and others added 5 commits December 24, 2024 15:07

Encapsulate card browser search queries in their own class

e83c292

TODO: Thank Oakkitten for the code Cherry-picked from ankidroid#16859, commit 2b89111 There was a conflict related to imports in card browser that was fixed.

Add more scaffolding code for enabling chip filtering in CardBrowser

eac81db

Code partially cherry-picked from ankidroid#16859, commit 4254250 References to flags filtering were removed and will(partially) be added in the next commit. The code works now as before related to flags filtering.

lukstbit force-pushed the feat_browserChipFiltering branch from e34147f to 00c8f0e Compare December 24, 2024 13:11

lukstbit removed the Has Conflicts label Dec 24, 2024

lukstbit removed the Needs Author Reply Waiting for a reply from the original author label Jan 2, 2025

david-allison added Needs Author Reply Waiting for a reply from the original author and removed Needs Author Reply Waiting for a reply from the original author labels Jan 4, 2025

david-allison requested changes Jan 4, 2025

View reviewed changes

david-allison added Needs Author Reply Waiting for a reply from the original author Review High Priority Request for high priority review labels Jan 4, 2025

github-actions bot added the Has Conflicts label Jan 5, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add card browser chip filtering for flags #17355

Add card browser chip filtering for flags #17355

lukstbit commented Nov 4, 2024 •

edited

Loading

github-actions bot commented Nov 4, 2024

lukstbit commented Dec 8, 2024

david-allison left a comment

david-allison Dec 8, 2024 •

edited

Loading

lukstbit Dec 24, 2024

david-allison Dec 8, 2024

lukstbit Dec 24, 2024

david-allison Dec 8, 2024

lukstbit Dec 24, 2024

This comment has been minimized.

david-allison commented Dec 24, 2024

lukstbit commented Dec 24, 2024

david-allison left a comment •

edited

Loading

Add card browser chip filtering for flags #17355

Are you sure you want to change the base?

Add card browser chip filtering for flags #17355

Conversation

lukstbit commented Nov 4, 2024 • edited Loading

Purpose / Description

Fixes

How Has This Been Tested?

Checklist

github-actions bot commented Nov 4, 2024

lukstbit commented Dec 8, 2024

david-allison left a comment

Choose a reason for hiding this comment

david-allison Dec 8, 2024 • edited Loading

Choose a reason for hiding this comment

lukstbit Dec 24, 2024

Choose a reason for hiding this comment

david-allison Dec 8, 2024

Choose a reason for hiding this comment

lukstbit Dec 24, 2024

Choose a reason for hiding this comment

david-allison Dec 8, 2024

Choose a reason for hiding this comment

lukstbit Dec 24, 2024

Choose a reason for hiding this comment

This comment has been minimized.

david-allison commented Dec 24, 2024

lukstbit commented Dec 24, 2024

david-allison left a comment • edited Loading

Choose a reason for hiding this comment

lukstbit commented Nov 4, 2024 •

edited

Loading

david-allison Dec 8, 2024 •

edited

Loading

david-allison left a comment •

edited

Loading