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

Add card browser chip filtering for flags #17355

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 36 additions & 27 deletions AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import androidx.annotation.VisibleForTesting
import androidx.appcompat.widget.SearchView
import androidx.appcompat.widget.ThemeUtils
import androidx.core.content.ContextCompat
import androidx.core.os.BundleCompat
import androidx.lifecycle.ViewModelProvider
import androidx.lifecycle.lifecycleScope
import androidx.recyclerview.widget.DividerItemDecoration
Expand All @@ -63,9 +64,11 @@ import com.ichi2.anki.browser.CardBrowserViewModel.SearchState
import com.ichi2.anki.browser.CardOrNoteId
import com.ichi2.anki.browser.PreviewerIdsFile
import com.ichi2.anki.browser.SaveSearchResult
import com.ichi2.anki.browser.SearchParameters
import com.ichi2.anki.browser.SharedPreferencesLastDeckIdRepository
import com.ichi2.anki.browser.getLabel
import com.ichi2.anki.browser.toCardBrowserLaunchOptions
import com.ichi2.anki.browser.toQuery
import com.ichi2.anki.dialogs.BrowserOptionsDialog
import com.ichi2.anki.dialogs.CardBrowserMySearchesDialog
import com.ichi2.anki.dialogs.CardBrowserMySearchesDialog.Companion.newInstance
Expand Down Expand Up @@ -137,9 +140,7 @@ open class CardBrowser :
ChangeManager.Subscriber,
ExportDialogsFactoryProvider {
override fun onDeckSelected(deck: SelectableDeck?) {
deck?.let {
launchCatchingTask { selectDeckAndSave(deck.deckId) }
}
deck?.let { selectDeckAndSave(deck.deckId) }
}

private enum class TagsDialogListenerAction {
Expand Down Expand Up @@ -461,10 +462,10 @@ open class CardBrowser :
.setSelection(COLUMN2_KEYS.indexOf(column))
}

fun onFilterQueryChanged(filterQuery: String) {
fun onFilterQueryChanged(filterQuery: SearchParameters) {
// setQuery before expand does not set the view's value
searchItem!!.expandActionView()
searchView!!.setQuery(filterQuery, submit = false)
searchView!!.setQuery(filterQuery.userInput, submit = false)
}

suspend fun onDeckIdChanged(deckId: DeckId?) {
Expand Down Expand Up @@ -503,8 +504,8 @@ open class CardBrowser :
when (searchState) {
SearchState.Initializing -> { }
SearchState.Searching -> {
if ("" != viewModel.searchTerms && searchView != null) {
searchView!!.setQuery(viewModel.searchTerms, false)
if (viewModel.searchTerms.userInput.isNotEmpty() && searchView != null) {
searchView!!.setQuery(viewModel.searchTerms.userInput, false)
searchItem!!.expandActionView()
}
}
Expand Down Expand Up @@ -603,7 +604,7 @@ open class CardBrowser :
}
}

suspend fun selectDeckAndSave(deckId: DeckId) {
fun selectDeckAndSave(deckId: DeckId) {
viewModel.setDeckId(deckId)
}

Expand Down Expand Up @@ -931,7 +932,7 @@ open class CardBrowser :
viewModel.setSearchQueryExpanded(false)
// SearchView doesn't support empty queries so we always reset the search when collapsing
searchView!!.setQuery("", false)
searchCards("")
searchCards(viewModel.searchTerms.copy(userInput = ""))
Copy link
Member

Choose a reason for hiding this comment

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

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

return true
}
},
Expand All @@ -951,7 +952,7 @@ open class CardBrowser :
}

override fun onQueryTextSubmit(query: String): Boolean {
searchCards(query)
searchCards(viewModel.searchTerms.copy(userInput = query))
searchView!!.clearFocus()
return true
}
Expand All @@ -960,14 +961,14 @@ open class CardBrowser :
}
// Fixes #6500 - keep the search consistent if coming back from note editor
// Fixes #9010 - consistent search after drawer change calls invalidateOptionsMenu
if (!viewModel.tempSearchQuery.isNullOrEmpty() || viewModel.searchTerms.isNotEmpty()) {
searchItem!!.expandActionView() // This calls mSearchView.setOnSearchClickListener
val toUse = if (!viewModel.tempSearchQuery.isNullOrEmpty()) viewModel.tempSearchQuery else viewModel.searchTerms
if (!viewModel.tempSearchQuery.isNullOrEmpty() || viewModel.searchTerms.userInput.isNotEmpty()) {
searchItem!!.expandActionView() // This calls searchView.setOnSearchClickListener
val toUse = if (!viewModel.tempSearchQuery.isNullOrEmpty()) viewModel.tempSearchQuery else viewModel.searchTerms.userInput
searchView!!.setQuery(toUse!!, false)
}
searchView!!.setOnSearchClickListener {
// Provide SearchView with the previous search terms
searchView!!.setQuery(viewModel.searchTerms, false)
searchView!!.setQuery(viewModel.searchTerms.userInput, false)
}
} else {
// multi-select mode
Expand Down Expand Up @@ -1319,15 +1320,17 @@ open class CardBrowser :
}

private fun openSaveSearchView() {
val searchTerms = searchView!!.query.toString()
showDialogFragment(
newInstance(
null,
mySearchesDialogListener,
searchTerms,
CardBrowserMySearchesDialog.CARD_BROWSER_MY_SEARCHES_TYPE_SAVE,
),
)
val searchTerms = viewModel.searchTerms.copy(userInput = searchView!!.query.toString())
launchCatchingTask {
showDialogFragment(
newInstance(
null,
mySearchesDialogListener,
searchTerms.toQuery(),
CardBrowserMySearchesDialog.CARD_BROWSER_MY_SEARCHES_TYPE_SAVE,
),
)
}
}

private fun repositionSelectedCards(): Boolean {
Expand Down Expand Up @@ -1581,19 +1584,25 @@ open class CardBrowser :

public override fun onSaveInstanceState(outState: Bundle) {
// Save current search terms
outState.putString("mSearchTerms", viewModel.searchTerms)
outState.putParcelable("mSearchTerms", viewModel.searchTerms)
exportingDelegate.onSaveInstanceState(outState)
super.onSaveInstanceState(outState)
}

public override fun onRestoreInstanceState(savedInstanceState: Bundle) {
super.onRestoreInstanceState(savedInstanceState)
searchCards(savedInstanceState.getString("mSearchTerms", ""))
val savedSearchTerms =
BundleCompat.getParcelable(
savedInstanceState,
"mSearchTerms",
SearchParameters::class.java,
) ?: return
searchCards(savedSearchTerms)
}

private fun forceRefreshSearch(useSearchTextValue: Boolean = false) {
if (useSearchTextValue && searchView != null) {
searchCards(searchView!!.query.toString())
searchCards(viewModel.searchTerms.copy(userInput = searchView!!.query.toString()))
} else {
searchCards()
}
Expand Down Expand Up @@ -1829,7 +1838,7 @@ open class CardBrowser :
}

@VisibleForTesting
fun searchCards(searchQuery: String) =
fun searchCards(searchQuery: SearchParameters) =
launchCatchingTask {
withProgress { viewModel.launchSearchForCards(searchQuery)?.join() }
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,13 +110,12 @@ class CardBrowserViewModel(

val flowOfSearchState = MutableSharedFlow<SearchState>()

var searchTerms = ""
var searchTerms = SearchParameters.EMPTY
Copy link
Member

Choose a reason for hiding this comment

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

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

private set
private var restrictOnDeck: String = ""

/** text in the search box (potentially unsubmitted) */
// this does not currently bind to the value in the UI and is only used for posting
val flowOfFilterQuery = MutableSharedFlow<String>()
val flowOfFilterQuery = MutableSharedFlow<SearchParameters>()

/**
* Whether the browser is working in Cards mode or Notes mode.
Expand Down Expand Up @@ -193,16 +192,16 @@ class CardBrowserViewModel(
val lastDeckId: DeckId?
get() = lastDeckIdRepository.lastDeckId

suspend fun setDeckId(deckId: DeckId) {
fun setDeckId(deckId: DeckId) {
Timber.i("setting deck: %d", deckId)
lastDeckIdRepository.lastDeckId = deckId
restrictOnDeck =
val newDeckRestriction =
if (deckId == ALL_DECKS_ID) {
""
setOf()
} else {
val deckName = withCol { decks.name(deckId) }
"deck:\"$deckName\" "
setOf(deckId)
Copy link
Member

Choose a reason for hiding this comment

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

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

}
searchTerms = searchTerms.copy(deckIds = newDeckRestriction)
flowOfDeckId.update { deckId }
}

Expand Down Expand Up @@ -267,14 +266,14 @@ class CardBrowserViewModel(
var selectAllDecks = false
when (options) {
is CardBrowserLaunchOptions.SystemContextMenu -> {
searchTerms = options.search.toString()
searchTerms = searchTerms.copy(userInput = options.search.toString())
}
is CardBrowserLaunchOptions.SearchQueryJs -> {
searchTerms = options.search
searchTerms = searchTerms.copy(userInput = options.search)
selectAllDecks = options.allDecks
}
is CardBrowserLaunchOptions.DeepLink -> {
searchTerms = options.search
searchTerms = searchTerms.copy(userInput = options.search)
}
null -> {}
}
Expand Down Expand Up @@ -672,7 +671,7 @@ class CardBrowserViewModel(
/** Ignores any values before [initCompleted] is set */
private fun <T> Flow<T>.ignoreValuesFromViewModelLaunch(): Flow<T> = this.filter { initCompleted }

suspend fun setFilterQuery(filterQuery: String) {
suspend fun setFilterQuery(filterQuery: SearchParameters) {
this.flowOfFilterQuery.emit(filterQuery)
launchSearchForCards(filterQuery)
}
Expand All @@ -683,7 +682,7 @@ class CardBrowserViewModel(
suspend fun searchForMarkedNotes() {
// only intended to be used if the user has no selection
if (hasSelectedAnyRows()) return
setFilterQuery("tag:marked")
setFilterQuery(searchTerms.copy(userInput = "tag:marked"))
}

/**
Expand All @@ -692,19 +691,20 @@ class CardBrowserViewModel(
suspend fun searchForSuspendedCards() {
// only intended to be used if the user has no selection
if (hasSelectedAnyRows()) return
setFilterQuery("is:suspended")
setFilterQuery(searchTerms.copy(userInput = "is:suspended"))
}

suspend fun setFlagFilter(flag: Flag) {
Timber.i("filtering to flag: %s", flag)
val flagSearchTerm = "flag:${flag.code}"
val searchTerms =
val userInput = searchTerms.userInput
val updatedInput =
when {
searchTerms.contains("flag:") -> searchTerms.replaceFirst("flag:.".toRegex(), flagSearchTerm)
searchTerms.isNotEmpty() -> "$flagSearchTerm $searchTerms"
userInput.contains("flag:") -> userInput.replaceFirst("flag:.".toRegex(), flagSearchTerm)
userInput.isNotEmpty() -> "$flagSearchTerm $userInput"
else -> flagSearchTerm
}
setFilterQuery(searchTerms)
setFilterQuery(searchTerms.copy(userInput = updatedInput))
}

suspend fun filterByTags(
Expand All @@ -717,7 +717,7 @@ class CardBrowserViewModel(
if (selectedTags.isNotEmpty()) {
sb.append("($tagsConcat)") // Only if we added anything to the tag list
}
setFilterQuery(sb.toString())
setFilterQuery(searchTerms.copy(userInput = sb.toString()))
}

/** Previewing */
Expand Down Expand Up @@ -784,7 +784,7 @@ class CardBrowserViewModel(
*/
fun endMultiSelectMode() = selectNone()

suspend fun launchSearchForCards(searchQuery: String): Job? {
suspend fun launchSearchForCards(searchQuery: SearchParameters): Job? {
searchTerms = searchQuery
return launchSearchForCards()
}
Expand All @@ -798,12 +798,7 @@ class CardBrowserViewModel(
// update the UI while we're searching
clearCardsList()

val query: String =
if (searchTerms.contains("deck:")) {
"($searchTerms)"
} else {
if ("" != searchTerms) "$restrictOnDeck($searchTerms)" else restrictOnDeck
}
val query: String = searchTerms.toQuery()

searchJob?.cancel()
searchJob =
Expand Down
50 changes: 50 additions & 0 deletions AnkiDroid/src/main/java/com/ichi2/anki/browser/SearchParameters.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/*
* This program is free software; you can redistribute it and/or modify it under
* the terms of the GNU General Public License as published by the Free Software
* Foundation; either version 3 of the License, or (at your option) any later
* version.
*
* This program is distributed in the hope that it will be useful, but WITHOUT ANY
* WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A
* PARTICULAR PURPOSE. See the GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License along with
* this program. If not, see <http://www.gnu.org/licenses/>.
*/

package com.ichi2.anki.browser

import android.os.Parcelable
import com.ichi2.anki.CollectionManager.withCol
import com.ichi2.anki.Flag
import com.ichi2.libanki.DeckId
import kotlinx.parcelize.Parcelize

@Parcelize
data class SearchParameters(
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

val userInput: String,
val deckIds: Set<DeckId>,
Copy link
Member

Choose a reason for hiding this comment

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

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.

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

companion object {
val EMPTY = SearchParameters("", emptySet(), emptySet(), emptySet())
}
}

suspend fun SearchParameters.toQuery(): String {
val targetDecksNames =
withCol {
deckIds.map { deckId -> decks.name(deckId) }
}
Comment on lines +39 to +42
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

return listOf(
userInput,
targetDecksNames.joinToString(" OR ") { "deck:\"$it\"" },
tags.joinToString(" OR ") { """"tag:$it"""" },
flags.joinToString(" OR ") { "flag:${it.code}" },
).filter { it.isNotEmpty() }
.joinToString(" ") { "($it)" }
}
10 changes: 7 additions & 3 deletions AnkiDroid/src/test/java/com/ichi2/anki/CardBrowserTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -747,14 +747,18 @@ class CardBrowserTest : RobolectricTest() {
}

val cardBrowser = browserWithNoNewCards
cardBrowser.searchCards("Hello").join()
cardBrowser.searchCards(cardBrowser.viewModel.searchTerms.copy(userInput = "Hello")).join()
waitForAsyncTasksToComplete()
assertThat(
"Card browser should have Test Deck as the selected deck",
cardBrowser.selectedDeckNameForUi,
equalTo("Test Deck"),
)
assertThat("Result should be empty", cardBrowser.viewModel.rowCount, equalTo(0))
assertThat(
"Result should be empty",
cardBrowser.viewModel.rowCount,
equalTo(0),
)

cardBrowser.searchAllDecks().join()
waitForAsyncTasksToComplete()
Expand Down Expand Up @@ -1240,7 +1244,7 @@ val CardBrowser.validDecksForChangeDeck
get() = runBlocking { getValidDecksForChangeDeck() }

suspend fun CardBrowser.searchCardsSync(query: String) {
searchCards(query)
searchCards(viewModel.searchTerms.copy(userInput = query))
viewModel.searchJob?.join()
}

Expand Down
Loading