From ee04b0c9407278b7d3b356f7a54613d5033b8d36 Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Tue, 30 Aug 2022 14:22:35 +1000 Subject: [PATCH] Migrate SearchCards to coroutines The preloading of the topmost cards has been moved into the browser, since it's not useful for the JS API, but this makes the searchCardsNumberOfResultCount() test harder to port. Since the CardCache should be removed in the future, I don't think it's worth the effort of getting the test working again. Closes #12223 --- .../java/com/ichi2/anki/AnkiDroidJsAPI.kt | 80 +++++------ .../main/java/com/ichi2/anki/CardBrowser.kt | 136 ++++++++---------- .../java/com/ichi2/async/CollectionTask.kt | 27 ---- .../java/com/ichi2/anki/CardBrowserTest.kt | 26 +--- 4 files changed, 95 insertions(+), 174 deletions(-) diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidJsAPI.kt b/AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidJsAPI.kt index 28d5d5dd03..24923bdfeb 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidJsAPI.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidJsAPI.kt @@ -24,17 +24,12 @@ import android.content.Intent import android.net.Uri import android.text.TextUtils import android.webkit.JavascriptInterface -import android.webkit.WebView import com.github.zafarkhaja.semver.Version import com.google.android.material.snackbar.Snackbar import com.ichi2.anim.ActivityTransitionAnimation import com.ichi2.anki.UIUtils.showThemedToast -import com.ichi2.anki.servicelayer.SearchService import com.ichi2.anki.snackbar.setMaxLines import com.ichi2.anki.snackbar.showSnackbar -import com.ichi2.async.CollectionTask.SearchCards -import com.ichi2.async.TaskListener -import com.ichi2.async.TaskManager import com.ichi2.libanki.Card import com.ichi2.libanki.CardId import com.ichi2.libanki.Consts.CARD_QUEUE @@ -44,6 +39,7 @@ import com.ichi2.libanki.SortOrder import com.ichi2.utils.JSONException import com.ichi2.utils.JSONObject import com.ichi2.utils.isActiveNetworkMetered +import kotlinx.coroutines.runBlocking import timber.log.Timber open class AnkiDroidJsAPI(private val activity: AbstractFlashcardViewer) { @@ -465,47 +461,41 @@ open class AnkiDroidJsAPI(private val activity: AbstractFlashcardViewer) { @JavascriptInterface fun ankiSearchCardWithCallback(query: String) { - val task = SearchCards(query, SortOrder.UseCollectionOrdering(), 0, 0, 0) - val listener = SearchCardListener(activity.webView!!, context) + val cards = try { + runBlocking { + searchForCards(query, SortOrder.UseCollectionOrdering()) + } + } catch (exc: Exception) { + activity.webView!!.evaluateJavascript( + "console.log('${context.getString(R.string.search_card_js_api_no_results)}')", + null + ) + return + } + val searchResult: MutableList = ArrayList() + for (s in cards) { + val jsonObject = JSONObject() + val fieldsData = s.card.note().fields + val fieldsName = s.card.model().fieldsNames + + val noteId = s.card.note().id + val cardId = s.card.id + jsonObject.put("cardId", cardId) + jsonObject.put("noteId", noteId) + + val jsonFieldObject = JSONObject() + fieldsName.zip(fieldsData).forEach { pair -> + jsonFieldObject.put(pair.component1(), pair.component2()) + } + jsonObject.put("fieldsData", jsonFieldObject) + + searchResult.add(jsonObject.toString()) + } + + // quote result to prevent JSON injection attack + val jsonEncodedString = org.json.JSONObject.quote(searchResult.toString()) activity.runOnUiThread { - TaskManager.launchCollectionTask(task, listener) - } - } - - class SearchCardListener(private val webView: WebView, private val context: Context) : TaskListener, SearchService.SearchCardsResult?>() { - override fun onPreExecute() { - // nothing to do - } - - override fun onPostExecute(result: SearchService.SearchCardsResult?) { - val searchResult: MutableList = ArrayList() - - if (result!!.result == null) { - webView.evaluateJavascript("console.log('${context.getString(R.string.search_card_js_api_no_results)}')", null) - } - - for (s in result.result!!) { - val jsonObject = JSONObject() - val fieldsData = s.card.note().fields - val fieldsName = s.card.model().fieldsNames - - val noteId = s.card.note().id - val cardId = s.card.id - jsonObject.put("cardId", cardId) - jsonObject.put("noteId", noteId) - - val jsonFieldObject = JSONObject() - fieldsName.zip(fieldsData).forEach { pair -> - jsonFieldObject.put(pair.component1(), pair.component2()) - } - jsonObject.put("fieldsData", jsonFieldObject) - - searchResult.add(jsonObject.toString()) - } - - // quote result to prevent JSON injection attack - val jsonEncodedString = org.json.JSONObject.quote(searchResult.toString()) - webView.evaluateJavascript("ankiSearchCard($jsonEncodedString)", null) + activity.webView!!.evaluateJavascript("ankiSearchCard($jsonEncodedString)", null) } } diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt b/AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt index efa11a2e80..223f8c7217 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt @@ -41,6 +41,7 @@ import com.ichi2.anim.ActivityTransitionAnimation import com.ichi2.anki.AnkiFont.Companion.getTypeface import com.ichi2.anki.CardUtils.getAllCards import com.ichi2.anki.CardUtils.getNotes +import com.ichi2.anki.CollectionManager.withCol import com.ichi2.anki.UIUtils.saveCollectionInBackground import com.ichi2.anki.UIUtils.showThemedToast import com.ichi2.anki.dialogs.* @@ -61,7 +62,6 @@ import com.ichi2.anki.servicelayer.SchedulerService.NextCard import com.ichi2.anki.servicelayer.SchedulerService.RepositionCards import com.ichi2.anki.servicelayer.SchedulerService.RescheduleCards import com.ichi2.anki.servicelayer.SchedulerService.ResetCards -import com.ichi2.anki.servicelayer.SearchService.SearchCardsResult import com.ichi2.anki.servicelayer.UndoService.Undo import com.ichi2.anki.snackbar.showSnackbar import com.ichi2.anki.widgets.DeckDropDownAdapter.SubtitleListener @@ -71,7 +71,6 @@ import com.ichi2.async.CollectionTask.CheckCardSelection import com.ichi2.async.CollectionTask.DeleteNoteMulti import com.ichi2.async.CollectionTask.MarkNoteMulti import com.ichi2.async.CollectionTask.RenderBrowserQA -import com.ichi2.async.CollectionTask.SearchCards import com.ichi2.async.CollectionTask.SuspendCardMulti import com.ichi2.async.CollectionTask.UpdateMultipleNotes import com.ichi2.async.CollectionTask.UpdateNote @@ -1470,7 +1469,6 @@ open class CardBrowser : } private fun invalidate() { - TaskManager.cancelAllTasks(SearchCards::class.java) TaskManager.cancelAllTasks(RenderBrowserQA::class.java) TaskManager.cancelAllTasks(CheckCardSelection::class.java) mCards.clear() @@ -1482,7 +1480,7 @@ open class CardBrowser : searchCards() } - @KotlinCleanup("isNotEmpty()") + @RustCleanup("remove card cache; switch to RecyclerView and browserRowForId (#11889)") private fun searchCards() { // cancel the previous search & render tasks if still running invalidate() @@ -1495,25 +1493,51 @@ open class CardBrowser : } else { if ("" != mSearchTerms) "$mRestrictOnDeck($mSearchTerms)" else mRestrictOnDeck } - if (colIsOpen() && mCardsAdapter != null) { - // clear the existing card list - mCards.reset() - mCardsAdapter!!.notifyDataSetChanged() - // estimate maximum number of cards that could be visible (assuming worst-case minimum row height of 20dp) - // Perform database query to get all card ids - TaskManager.launchCollectionTask( - SearchCards( - searchText!!, - if (mOrder == CARD_ORDER_NONE) NoOrdering() else UseCollectionOrdering(), - numCardsToRender(), - mColumn1Index, - mColumn2Index - ), - mSearchCardsHandler - ) + // clear the existing card list + mCards.reset() + mCardsAdapter!!.notifyDataSetChanged() + val query = searchText!! + val order = if (mOrder == CARD_ORDER_NONE) NoOrdering() else UseCollectionOrdering() + launchCatchingTask { + val cards = withProgress { searchForCards(query, order) } + // Render the first few items + for (i in 0 until Math.min(numCardsToRender(), cards.size)) { + cards[i].load(false, mColumn1Index, mColumn2Index) + } + redrawAfterSearch(cards) } } + fun redrawAfterSearch(cards: MutableList) { + mCards.replaceWith(cards) + Timber.i("CardBrowser:: Completed searchCards() Successfully") + updateList() + if (mSearchView == null || mSearchView!!.isIconified) { + return + } + if (hasSelectedAllDecks()) { + showSnackbar(subtitleText, Snackbar.LENGTH_SHORT) + } else { + // If we haven't selected all decks, allow the user the option to search all decks. + val message = if (cardCount == 0) { + getString(R.string.card_browser_no_cards_in_deck, selectedDeckNameForUi) + } else { + subtitleText + } + showSnackbar(message, Snackbar.LENGTH_INDEFINITE) { + setAction(R.string.card_browser_search_all_decks) { searchAllDecks() } + } + } + if (mShouldRestoreScroll) { + mShouldRestoreScroll = false + val newPosition = newPositionOfSelectedCard + if (newPosition != CARD_NOT_AVAILABLE) { + autoScrollTo(newPosition) + } + } + updatePreviewMenuItem() + } + @VisibleForTesting protected open fun numCardsToRender(): Int { return ceil( @@ -1820,66 +1844,6 @@ open class CardBrowser : } } - private val mSearchCardsHandler = SearchCardsHandler(this) - - @VisibleForTesting - internal inner class SearchCardsHandler(browser: CardBrowser) : ListenerWithProgressBar, SearchCardsResult?>(browser) { - override fun actualOnProgressUpdate(context: CardBrowser, value: List) { - // Need to copy the list into a new list, because the original list is modified, and - // ListAdapter crash - mCards.replaceWith(java.util.ArrayList(value)) - updateList() - } - - override fun actualOnPostExecute(context: CardBrowser, result: SearchCardsResult?) { - if (result!!.hasResult) { - mCards.replaceWith(result.result!!.toMutableList()) - updateList() - handleSearchResult() - } - if (result.hasError) { - showThemedToast(this@CardBrowser, result.error, true) - } - if (mShouldRestoreScroll) { - mShouldRestoreScroll = false - val newPosition = newPositionOfSelectedCard - if (newPosition != CARD_NOT_AVAILABLE) { - autoScrollTo(newPosition) - } - } - updatePreviewMenuItem() - hideProgressBar() - } - - private fun handleSearchResult() { - Timber.i("CardBrowser:: Completed doInBackgroundSearchCards Successfully") - updateList() - if (mSearchView == null || mSearchView!!.isIconified) { - return - } - - if (hasSelectedAllDecks()) { - showSnackbar(subtitleText, Snackbar.LENGTH_SHORT) - } else { - // If we haven't selected all decks, allow the user the option to search all decks. - val message = if (cardCount == 0) { - getString(R.string.card_browser_no_cards_in_deck, selectedDeckNameForUi) - } else { - subtitleText - } - - showSnackbar(message, Snackbar.LENGTH_INDEFINITE) { - setAction(R.string.card_browser_search_all_decks) { searchAllDecks() } - } - } - } - - override fun actualOnCancelled(context: CardBrowser) { - super.actualOnCancelled(context) - hideProgressBar() - } - } - private fun saveScrollingState(position: Int) { mOldCardId = mCards[position].id mOldCardTopOffset = calculateTopOffset(position) @@ -2711,3 +2675,15 @@ open class CardBrowser : } } } + +suspend fun searchForCards( + query: String, + order: SortOrder +): MutableList { + return withCol { + findCards(query, order).asSequence() + .mapIndexed { idx, cid -> + CardBrowser.CardCache(cid, col, idx) + }.toMutableList() + } +} diff --git a/AnkiDroid/src/main/java/com/ichi2/async/CollectionTask.kt b/AnkiDroid/src/main/java/com/ichi2/async/CollectionTask.kt index 04930a2d1b..d99d752b6c 100644 --- a/AnkiDroid/src/main/java/com/ichi2/async/CollectionTask.kt +++ b/AnkiDroid/src/main/java/com/ichi2/async/CollectionTask.kt @@ -541,33 +541,6 @@ open class CollectionTask(val task: TaskDelegateBase, SearchCardsResult>() { - override fun task(col: Collection, collectionTask: ProgressSenderAndCancelListener>): SearchCardsResult { - Timber.d("doInBackgroundSearchCards") - val searchResult: MutableList = ArrayList() - val searchResult_: List - searchResult_ = try { - col.findCards(query, order) - } catch (e: Exception) { - // exception can occur via normal operation - Timber.w(e) - return SearchCardsResult.error(e) - } - Timber.d("The search found %d cards", searchResult_.size) - var position = 0 - for (cid in searchResult_) { - val card = CardCache(cid, col, position++) - searchResult.add(card) - } - // Render the first few items - for (i in 0 until Math.min(numCardsToRender, searchResult.size)) { - searchResult[i].load(false, column1Index, column2Index) - } - // Finish off the task - return SearchCardsResult.success(searchResult) - } - } - class SearchNotes(private val query: String, private val order: SortOrder, private val numCardsToRender: Int, private val column1Index: Int, private val column2Index: Int) : TaskDelegate, SearchCardsResult>() { override fun task(col: Collection, collectionTask: ProgressSenderAndCancelListener>): SearchCardsResult { Timber.d("doInBackgroundSearchCards") diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/CardBrowserTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/CardBrowserTest.kt index a383c69a3c..5622a3e16b 100644 --- a/AnkiDroid/src/test/java/com/ichi2/anki/CardBrowserTest.kt +++ b/AnkiDroid/src/test/java/com/ichi2/anki/CardBrowserTest.kt @@ -28,12 +28,9 @@ import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 import com.google.errorprone.annotations.CheckReturnValue import com.ichi2.anki.CardBrowser.CardCache -import com.ichi2.async.CollectionTask.SearchCards -import com.ichi2.async.TaskManager import com.ichi2.libanki.CardId import com.ichi2.libanki.Consts import com.ichi2.libanki.Note -import com.ichi2.libanki.SortOrder.NoOrdering import com.ichi2.testutils.AnkiActivityUtils.getDialogFragment import com.ichi2.testutils.AnkiAssert import com.ichi2.testutils.IntentAssert @@ -356,7 +353,7 @@ class CardBrowserTest : RobolectricTest() { } @Test - fun tagWithBracketsDisplaysProperly() { + fun tagWithBracketsDisplaysProperly() = runTest { val n = addNoteUsingBasicModel("Hello", "World") n.addTag("sketchy::(1)") n.flush() @@ -369,7 +366,7 @@ class CardBrowserTest : RobolectricTest() { } @Test - fun filterByFlagDisplaysProperly() { + fun filterByFlagDisplaysProperly() = runTest { val cardWithRedFlag = addNoteUsingBasicModel("Card with red flag", "Reverse") flagCardForNote(cardWithRedFlag, 1) @@ -583,7 +580,7 @@ class CardBrowserTest : RobolectricTest() { /** 8027 */ @Test - fun checkSearchString() { + fun checkSearchString() = runTest { addNoteUsingBasicModel("Hello", "John") val deck = addDeck("Deck 1") col.decks.select(deck) @@ -641,7 +638,7 @@ class CardBrowserTest : RobolectricTest() { } @Test - fun checkIfSearchAllDecksWorks() { + fun checkIfSearchAllDecksWorks() = runTest { addNoteUsingBasicModel("Hello", "World") val deck = addDeck("Test Deck") col.decks.select(deck) @@ -776,21 +773,6 @@ class CardBrowserTest : RobolectricTest() { renderOnScroll.onScroll(cardBrowser.mCardsListView!!, 0, 0, 2) } - @Test - fun searchCardsNumberOfResultCount() { - val cardsToRender = 1 - - val cardBrowser = getBrowserWithNotes(2, CardBrowserSizeOne::class.java) - - val task = SearchCards("", NoOrdering(), cardsToRender, 0, 0) - - TaskManager.launchCollectionTask(task, cardBrowser.SearchCardsHandler(cardBrowser)) - val cards = cardBrowser.mCards - assertThat(2, equalTo(cards.size())) - assertTrue(cards[0].isLoaded) - assertFalse(cards[1].isLoaded) - } - @Test fun truncateAndExpand() { val cardBrowser = getBrowserWithNotes(3)