Add card browser chip filtering for flags

[lukstbit]

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]

Important

Maintainers: This PR contains Strings changes

1. Sync Translations before merging this PR and wait for the action to complete
2. Review and merge the auto-generated PR in order to sync all user-submitted translations
3. 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]

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

[david-allison]

This seems like too many parameters, is tags necessary

[lukstbit]

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

[david-allison]

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

[lukstbit]

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]

Ok, I think I fixed the conflicts and also implemented the changes:

[david-allison]

Last nit on padding: first item is inconsistent

[lukstbit]

You have good eyes 😆

[mikehardy]

Apart from the one comment I left asking for a more detailed comment, the code looks good.

I'm a little curious about the behavior though - specifically, the flags chips are in effect performing a search. But we also have a search box, and they don't seem to be connected

Consider: you have 2 cards flagged red (flag:1), 2 cards flagged orange (flag:2)

Anki default search behavior is to have an invisible/implicit "and" between terms.
section "Things to note from above" in https://docs.ankiweb.net/searching.html#simple-searches

The chip behavior here is an implicit "or" on all flags - if I select red and orange I see 4 cards now.

The search box is an "and" but if I search for "flag:1 or flag:2" I will see 4 cards.

However, if I search for "flag:1 and flag:2" while I have the red chip selected, I only see the 2 red cards, that is I am filtering again - it appears to be a two-layer filter - search box then chips.

So - with that example I think I show that it appears to be a two-step filter now - the search box then the chips. Is that what we want?

Or do we want whatever you selected in the chips to be reflected in the search box if you open it? And for whatever chip-related items you select in the search box to update the state of the chips?

Seems like more work to connect chips and search box but also then reduces it to a single-level filter, and allows users to save chip filter setups in saved searches as they would be reflected as a real search in the search box

🤔 ?

[mikehardy]

The stackoverflow link is good and all, but some text describing the issue is better than
making folks click through and wade through a bunch of stackoverflow stuff

Suggested change

	// https://stackoverflow.com/questions/41591733
	// Android material library BottomSheet has a bug where it is not expanded in landscape mode
	// So any time we load the bottom sheet or the configuration changes, we check if we are in
	// landscape mode, and if so, we force the sheet to be an expanded height
	// https://stackoverflow.com/questions/41591733

[Arthur-Milchior]

Thank you very much to you, and @oakkitten who started this long ago. Looks really nice.
I only had time to look at a single commit. Only small nits, but I love it

[Arthur-Milchior]

Why define it as an extension instead of putting it into the class itself?
That's not wrong, but seems non standard and unexpected

[Arthur-Milchior]

I think it's worth adding a comment explaining that we're resetting only the text entered by the user, but keeping all chips as-is

[Arthur-Milchior]

I'd really appreciate some documentation. I admit I find it very clear, but I'm not certain it would be the case for a new contributor.

In particular, every parameters comes from user input, whether they tap or type. I admit that I don't have a better value name. Still, it'd be nice to explain this corresponds to what the user typed in the search bar. It's expected to be a query but can actually be badly formed

[Arthur-Milchior]

Subject: [PATCH] [PATCH]
---
Index: AnkiDroid/src/main/java/com/ichi2/anki/browser/SearchParameters.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/browser/SearchParameters.kt b/AnkiDroid/src/main/java/com/ichi2/anki/browser/SearchParameters.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/browser/SearchParameters.kt	(revision c45fdd7d1dd576b6a9438ceb5721055a079140c8)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/browser/SearchParameters.kt	(date 1736379407730)
@@ -20,11 +20,24 @@
 import com.ichi2.libanki.DeckId
 import kotlinx.parcelize.Parcelize
 
+/**
+ * Represents the parameters displayed to the user in the card browser search UI. The search UI contains both the search text field where the user can type any query, and the chips.
+ * The result of this search consist in all cards satisfying all of the criteria below.
+ */
 @Parcelize
 data class SearchParameters(
     val userInput: String,
+    /**
+     * Limit the search to cards belonging to any deck of [deckIds]. Note that it does not include any subdeck.
+     */
     val deckIds: Set<DeckId>,
+    /**
+     * Limit the search to any note containing at least one tag in [tags].
+     */
     val tags: Set<String>,
+    /**
+     * Limit the search to cards whose flag belong to [flags].
+     */
     val flags: Set<Flag>,
 ) : Parcelable {
     val isEmpty get() = userInput.isEmpty() && deckIds.isEmpty() && tags.isEmpty() && flags.isEmpty()
@@ -33,9 +46,8 @@
     companion object {
         val EMPTY = SearchParameters("", emptySet(), emptySet(), emptySet())
     }
-}
 
-suspend fun SearchParameters.toQuery(): String {
+suspend fun toQuery(): String {
     val targetDecksNames =
         withCol {
             deckIds.map { deckId -> decks.name(deckId) }
@@ -48,3 +60,4 @@
     ).filter { it.isNotEmpty() }
         .joinToString(" ") { "($it)" }
 }
+}
\ No newline at end of file

[Arthur-Milchior]

I assume the reason why it's a set and not an arbitrary collection/iterable is that it ensures that it's parcelizable. If so, I'd suggest maybe adding a comment somewhere, because intuitively, it seems strange to me to restrict it to being a set specifically.

[Arthur-Milchior]

I am very confused by the fact that the variable is called searchTerms and the type SearchParameters.
I understand keeping searchTerms avoid to change 37 lines of code. Still, I think it'd be nicer to be consistent between the variable and the type name. Unless there is a nuance I'm missing

[Arthur-Milchior]

My bad, there was one subtle breaking change here.

I'd appreciate if you maybe could add a regression test to ensure that we would catch in the future if a search stopped showing cards in subdeck of selected deck

[Arthur-Milchior]

Subject: [PATCH] [PATCH]
---
Index: AnkiDroid/src/main/java/com/ichi2/anki/browser/SearchParameters.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/browser/SearchParameters.kt b/AnkiDroid/src/main/java/com/ichi2/anki/browser/SearchParameters.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/browser/SearchParameters.kt	(revision c45fdd7d1dd576b6a9438ceb5721055a079140c8)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/browser/SearchParameters.kt	(date 1736379407730)
@@ -20,11 +20,24 @@
 import com.ichi2.libanki.DeckId
 import kotlinx.parcelize.Parcelize
 
+/**
+ * Represents the parameters displayed to the user in the card browser search UI. The search UI contains both the search text field where the user can type any query, and the chips.
+ * The result of this search consist in all cards satisfying all of the criteria below.
+ */
 @Parcelize
 data class SearchParameters(
     val userInput: String,
+    /**
+     * Limit the search to cards belonging to any deck of [deckIds]. Note that it does not include any subdeck.
+     */
     val deckIds: Set<DeckId>,
+    /**
+     * Limit the search to any note containing at least one tag in [tags].
+     */
     val tags: Set<String>,
+    /**
+     * Limit the search to cards whose flag belong to [flags].
+     */
     val flags: Set<Flag>,
 ) : Parcelable {
     val isEmpty get() = userInput.isEmpty() && deckIds.isEmpty() && tags.isEmpty() && flags.isEmpty()
@@ -33,9 +46,8 @@
     companion object {
         val EMPTY = SearchParameters("", emptySet(), emptySet(), emptySet())
     }
-}
 
-suspend fun SearchParameters.toQuery(): String {
+suspend fun toQuery(): String {
     val targetDecksNames =
         withCol {
             deckIds.map { deckId -> decks.name(deckId) }
@@ -48,3 +60,4 @@
     ).filter { it.isNotEmpty() }
         .joinToString(" ") { "($it)" }
 }
+}
\ No newline at end of file

[Arthur-Milchior]

Nope nope nope nope. Please keep using the deck name instead of deck id. The meaning is very different. deck name also search for the cards in the subdecks. Deck id only search cards whose did is exactly this value. You were accidentally changing the semantic of selecting a deck in a way that would totally break the card browser for anyone with use of subdecks

[Arthur-Milchior]

Anki default search behavior is to have an invisible/implicit "and" between terms. section "Things to note from above" in https://docs.ankiweb.net/searching.html#simple-searches

I think what you should compare this feature too is what occurs if you use the entries in the left menu of anki card browser.
If you don't hold any key down, the whole search is replaced by "card 1" when you tap "flag:1".
If you press ctrl it adds "flag:1" at the end of the query (potentially adding parenthesis to the query) . Whether or not it makes sense, this is consistent with what ctrl always does. And shift add "or flag:1". In any case, the user has control with the keyboard keys. You can use and and or. You can't just select/deselect chip.

So - with that example I think I show that it appears to be a two-step filter now - the search box then the chips. Is that what we want?

I'd argue for a strong yes.
Ankidroid does not explain anywhere the search query language. So, either the user already now the query language, in which case I assume they should be able to understand or multiple constraint interact.
Or they don't know the query language, which I assume will be the case for most people, in which case the basic use case of searching for words in fields will work perfectly.

To state it another way. If they just want a simple search, to find a few specific cards, this interface is quite simpler, so that's great. If they really need the full query language to do complex search, anyway, I think the complex part is learning how to do boolean combination of search terms, and understanding the logic of this extra "and" is probably easy

Or do we want whatever you selected in the chips to be reflected in the search box if you open it? And for whatever chip-related items you select in the search box to update the state of the chips?

Strong no for me. The query can be absurdly complex. There is no way that we'd have a clear meaning for what it means to unselect a flag when the flag restriction is deep in the query. And reciprocally, if the flag restriction is not at the top level of the AST, it's not clear whether or not you want to select the flag in the chip. It's even worse if you use a query of the kind "-flag:0".

If we ever get complaints that we don't offer enough flexibility and somewhat have a nice answer, maybe let's reconsider it. I think this will be a nightmare to do, and there will never be a right solution for the problem you want to solve

[snowtimeglass]

Some thoughts:

• Explicit indication of the “OR” behavior may be better.
• In the chip, it isn't necessarily needed to refer to the initially-selected flag by text because the icon already represents, for example, "green flag".
• It may be better to introduce some kind of new icon for "No Flag" (and for the concept of "Flags" itself) here and to some other places in the app, as in the desktop version:
  

Images:

[mikehardy]

Well-reasoned by @Arthur-Milchior, thanks. I yield ;-) - +1

[mikehardy]

@snowtimeglass a crossed out flag (vs an empty one) for "no flag" might be nice but I think that could be separate

I think if the main search query language is "and" combination, perhaps we should follow that. I'm trying to recall filter systems I use - not many unfortunately - and the filters are all "and" if you have multiple in the ones I can think of.

[oakkitten]

I am once again asking (insert Bernie.jpg) to use more symmetrical padding (horizontal and vertical). (Also between items?)

Also, I again suggest to not use “x” as an icon to clear filters.

I understand the reasoning for “or” but if we think about other possible chips, we probably don't want to do AND anywhere inside them. Do we want to have the word “or” for decks or tags? I will not be trying to judge how easy it would be to grasp the chips for the end user, but if we do AND for chips and OR for items inside chips, the overall design would be at the very least describable as not complex. So I'd opt for the universal +1 for simplicity (and note no space!). Also, don't forget that flag names are customizable by the user.

Also, “No flag” should probably not be selectable together with other flags, and vice versa.

Also, there's too much space between chips and sort/type selectors.

Also, the browser header now takes something like ¼ of my screen. Can we block chips until the header is collapsible on scrolling?

So - with that example I think I show that it appears to be a two-step filter now - the search box then the chips. Is that what we want?

Or do we want whatever you selected in the chips to be reflected in the search box if you open it? And for whatever chip-related items you select in the search box to update the state of the chips?

I think that with chips users will expect 2 levels.

Reflecting it in the search box would be good for power users—if they want to negate things, or change parameters for things with parameters. But:

• These changes will have to be reflected in the chips. This would be like super duper hard to do, and in the case of parameters just not feasible;
• It would present search constraints in two places at the same time, which is just confusing.

We could do something like a one-way conversion from chips to query string, with hiding the chip bar.

[oakkitten]

Also this crashes on dark mode change (I actually bothered to run the code!)

E  ACRA caught a NumberFormatException for com.ichi2.anki.debug
java.lang.NumberFormatException: For input string: ""
	at java.lang.Integer.parseInt(Integer.java:807)
	at java.lang.Integer.parseInt(Integer.java:915)
	at com.ichi2.anki.browser.BrowserFlagsFilteringFragment.onViewCreated(BrowserFlagsFilteringFragment.kt:63)
	at androidx.fragment.app.Fragment.performViewCreated(Fragment.java:3152)
	at androidx.fragment.app.FragmentStateManager.createView(FragmentStateManager.java:608)
	at androidx.fragment.app.FragmentStateManager.moveToExpectedState(FragmentStateManager.java:286)
	at androidx.fragment.app.FragmentStore.moveToExpectedState(FragmentStore.java:114)
	at androidx.fragment.app.FragmentManager.moveToState(FragmentManager.java:1685)
	at androidx.fragment.app.FragmentManager.dispatchStateChange(FragmentManager.java:3319)
	at androidx.fragment.app.FragmentManager.dispatchActivityCreated(FragmentManager.java:3237)
	at androidx.fragment.app.FragmentController.dispatchActivityCreated(FragmentController.java:263)
	at androidx.fragment.app.FragmentActivity.onStart(FragmentActivity.java:350)
	at androidx.appcompat.app.AppCompatActivity.onStart(AppCompatActivity.java:251)
	at com.ichi2.anki.AnkiActivity.onStart(AnkiActivity.kt:143)
	at android.app.Instrumentation.callActivityOnStart(Instrumentation.java:1582)
	at android.app.Activity.performStart(Activity.java:9034)
	at android.app.ActivityThread.handleStartActivity(ActivityThread.java:4206)
	at android.app.servertransaction.TransactionExecutor.performLifecycleSequence(TransactionExecutor.java:225)
	at android.app.servertransaction.TransactionExecutor.cycleToPath(TransactionExecutor.java:205)
	at android.app.servertransaction.TransactionExecutor.executeLifecycleState(TransactionExecutor.java:177)
	at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:98)
	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2693)
	at android.os.Handler.dispatchMessage(Handler.java:106)
	at android.os.Looper.loopOnce(Looper.java:230)
	at android.os.Looper.loop(Looper.java:319)
	at android.app.ActivityThread.main(ActivityThread.java:9063)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:588)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1103)

[oakkitten]

Can we change SearchParameters to use deck names in the first place? If not, I suggest adding a comment in the code regarding the necessity of this, instead of using the commit message for this.

[snowtimeglass]

@mikehardy

I think if the main search query language is "and" combination, perhaps we should follow that. I'm trying to recall filter systems I use - not many unfortunately - and the filters are all "and" if you have multiple in the ones I can think of.

I suppose "or" is working for multiple-selected normal labels inside "Labels" chips of Gmail app:

The same goes for multiple-selected email addresses inside "From" chip:

Especially in "From" chip in Gmail and "Flags" chip in AnkiDroid, "and" combination can do nothing because each mail and card cannot have two items at the same time (for example, at least in general, one mail doesn't have two mail address of its sender IIUC; one card cannot have two flags), so "or" combination for multiple-selected items inside these chips seems acceptably reasonable to me. (I'm not trying to say the "or" combination search for flags chip would be super useful and absolutely indispensable, though.)

[mikehardy]

I appreciate you all listening to my semi-vague questions about syncing search bar and chips (Arthur) and the and-vs-or question I had (snowtimeglass and oakkitten)

Well reasoned responses on and-vs-or, seems like "or" might be the only way to do it that makes sense (c.f. oakkitten) and it has usage that way in gmail which everyone uses (c.f. snowtimeglass)

Okay then, "or" on chips is fine by me.

I don't have any other musings. I did also run the code myself and really played with it - just didn't try a theme switch like oakkitten did. Appears that's an issue but I didn't see anything that would take away my +1

[snowtimeglass]

@oakkitten

So I'd opt for the universal +1 for simplicity (and note no space!).

• Gmail uses the "+1" style. I feel it is a bit confusing for "or" combination, but its simplicity is attractive.

• "FlagName +1" style with ellipsis (for example, "Green +1", "Customiz… +1") is understandable to me, but "Flags +1" looks unnatural.

“No flag” should probably not be selectable together with other flags, and vice versa.

Could you please elaborate? I assume "or" combination would be applied to "No Flag" with other flags:

[oakkitten]

“No flag” should probably not be selectable together with other flags, and vice versa.

Could you please elaborate? I assume "or" combination would be applied to "No Flag" with other flags:

Sorry, disregard that bit, I did a silly