From a7cd900e9f9bc369c68aa57a4b283e675f1c2572 Mon Sep 17 00:00:00 2001 From: lukstbit <52494258+lukstbit@users.noreply.github.com> Date: Sun, 28 Dec 2025 22:10:03 +0200 Subject: [PATCH] Fix state restore bug in FindAndReplaceDialogFragment To get the current selected notes the fragment is querying the browser's ViewModel for the selected notes. This works most of the times but fails(we lose the selection and will target everything instead of only the selected notes) when going to the background and coming back(and fragment gets recreated). At this point the fragment tries to access again the selected notes but these are not available as they are being manually restored from a file. This bug can be seen by using the "Don't keep activities" dev option. The fix consists in using an IdsFile so the fragment always has a reference to the selected notes from the moment of its creation. --- .../ichi2/anki/browser/CardBrowserFragment.kt | 9 +- .../browser/FindAndReplaceDialogFragment.kt | 29 ++- .../FindAndReplaceDialogFragmentTest.kt | 172 ++++++++++++++++++ 3 files changed, 205 insertions(+), 5 deletions(-) create mode 100644 AnkiDroid/src/test/java/com/ichi2/anki/browser/FindAndReplaceDialogFragmentTest.kt diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/browser/CardBrowserFragment.kt b/AnkiDroid/src/main/java/com/ichi2/anki/browser/CardBrowserFragment.kt index d73d2e5da9b2..24249e21aad3 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/browser/CardBrowserFragment.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/browser/CardBrowserFragment.kt @@ -82,6 +82,7 @@ import com.ichi2.anki.dialogs.tags.TagsDialog import com.ichi2.anki.dialogs.tags.TagsDialogFactory import com.ichi2.anki.dialogs.tags.TagsDialogListener import com.ichi2.anki.export.ExportDialogFragment +import com.ichi2.anki.launchCatching import com.ichi2.anki.launchCatchingTask import com.ichi2.anki.libanki.DeckId import com.ichi2.anki.model.CardStateFilter @@ -862,7 +863,13 @@ class CardBrowserFragment : @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) fun showFindAndReplaceDialog() { - FindAndReplaceDialogFragment().show(parentFragmentManager, FindAndReplaceDialogFragment.TAG) + lifecycleScope.launch { + withProgress { + val noteIds = activityViewModel.queryAllSelectedNoteIds() + val fragment = FindAndReplaceDialogFragment.newInstance(requireContext(), noteIds) + fragment.show(parentFragmentManager, FindAndReplaceDialogFragment.TAG) + } + } } @KotlinCleanup("DeckSelectionListener is almost certainly a bug - deck!!") diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/browser/FindAndReplaceDialogFragment.kt b/AnkiDroid/src/main/java/com/ichi2/anki/browser/FindAndReplaceDialogFragment.kt index 3a54e3648da8..98ffec027706 100644 --- a/AnkiDroid/src/main/java/com/ichi2/anki/browser/FindAndReplaceDialogFragment.kt +++ b/AnkiDroid/src/main/java/com/ichi2/anki/browser/FindAndReplaceDialogFragment.kt @@ -17,15 +17,17 @@ package com.ichi2.anki.browser import android.app.Dialog +import android.content.Context import android.os.Bundle import android.widget.AdapterView import android.widget.ArrayAdapter import androidx.annotation.VisibleForTesting +import androidx.annotation.VisibleForTesting.Companion.PRIVATE import androidx.appcompat.app.AlertDialog +import androidx.core.os.BundleCompat import androidx.core.os.bundleOf import androidx.core.text.HtmlCompat import androidx.core.view.isVisible -import androidx.fragment.app.activityViewModels import androidx.fragment.app.setFragmentResult import androidx.lifecycle.lifecycleScope import com.ichi2.anki.CardBrowser @@ -41,6 +43,7 @@ import com.ichi2.anki.browser.FindAndReplaceDialogFragment.Companion.ARG_REPLACE import com.ichi2.anki.browser.FindAndReplaceDialogFragment.Companion.ARG_SEARCH import com.ichi2.anki.browser.FindAndReplaceDialogFragment.Companion.REQUEST_FIND_AND_REPLACE import com.ichi2.anki.databinding.FragmentFindReplaceBinding +import com.ichi2.anki.libanki.NoteId import com.ichi2.anki.notetype.ManageNotetypes import com.ichi2.anki.ui.internationalization.toSentenceCase import com.ichi2.anki.utils.ext.setFragmentResultListener @@ -66,7 +69,6 @@ import timber.log.Timber */ // TODO desktop offers history for inputs class FindAndReplaceDialogFragment : AnalyticsDialogFragment() { - private val browserViewModel by activityViewModels() private lateinit var binding: FragmentFindReplaceBinding override fun onCreateDialog(savedInstanceState: Bundle?): Dialog { @@ -107,7 +109,15 @@ class FindAndReplaceDialogFragment : AnalyticsDialogFragment() { (dialog as? AlertDialog)?.positiveButton?.isEnabled = false binding.contentViewsGroup.isVisible = false binding.loadingViewsGroup.isVisible = true - val noteIds = browserViewModel.queryAllSelectedNoteIds() + val idsFile = + requireNotNull( + BundleCompat.getParcelable( + requireArguments(), + ARG_IDS, + IdsFile::class.java, + ), + ) + val noteIds = idsFile.getIds() binding.onlySelectedNotesCheckBox.isChecked = noteIds.isNotEmpty() binding.onlySelectedNotesCheckBox.isEnabled = noteIds.isNotEmpty() val fieldsNames = @@ -134,7 +144,7 @@ class FindAndReplaceDialogFragment : AnalyticsDialogFragment() { } // https://github.com/ankitects/anki/blob/64ca90934bc26ddf7125913abc9dd9de8cb30c2b/qt/aqt/browser/find_and_replace.py#L118 - @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) + @VisibleForTesting(otherwise = PRIVATE) fun startFindReplace() { val search = binding.inputSearch.text val replacement = binding.inputReplace.text @@ -167,6 +177,7 @@ class FindAndReplaceDialogFragment : AnalyticsDialogFragment() { companion object { const val TAG = "FindAndReplaceDialogFragment" const val REQUEST_FIND_AND_REPLACE = "request_find_and_replace" + const val ARG_IDS = " arg_ids" const val ARG_SEARCH = "arg_search" const val ARG_REPLACEMENT = "arg_replacement" const val ARG_FIELD = "arg_field" @@ -185,6 +196,16 @@ class FindAndReplaceDialogFragment : AnalyticsDialogFragment() { * the user selected "Tags" as the field target for the find and replace action. */ const val TAGS_AS_FIELD = "find_and_replace_dialog_fragment_tags_as_field" + + fun newInstance( + context: Context, + noteIds: List, + ): FindAndReplaceDialogFragment { + val file = IdsFile(context.cacheDir, noteIds) + return FindAndReplaceDialogFragment().apply { + arguments = bundleOf(ARG_IDS to file) + } + } } } diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/browser/FindAndReplaceDialogFragmentTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/browser/FindAndReplaceDialogFragmentTest.kt new file mode 100644 index 000000000000..0a08ceb0e365 --- /dev/null +++ b/AnkiDroid/src/test/java/com/ichi2/anki/browser/FindAndReplaceDialogFragmentTest.kt @@ -0,0 +1,172 @@ +/* + * Copyright (c) 2025 lukstbit <52494258+lukstbit@users.noreply.github.com> + * + * 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 . + */ + +package com.ichi2.anki.browser + +import android.content.Context +import android.widget.Spinner +import android.widget.SpinnerAdapter +import androidx.core.os.bundleOf +import androidx.fragment.app.testing.FragmentScenario +import androidx.test.espresso.Espresso.onView +import androidx.test.espresso.assertion.ViewAssertions.matches +import androidx.test.espresso.matcher.RootMatchers.isDialog +import androidx.test.espresso.matcher.ViewMatchers.isChecked +import androidx.test.espresso.matcher.ViewMatchers.isEnabled +import androidx.test.espresso.matcher.ViewMatchers.isNotChecked +import androidx.test.espresso.matcher.ViewMatchers.isNotEnabled +import androidx.test.espresso.matcher.ViewMatchers.withId +import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.ichi2.anki.CollectionManager.TR +import com.ichi2.anki.R +import com.ichi2.anki.RobolectricTest +import com.ichi2.anki.libanki.Note +import com.ichi2.anki.libanki.NoteId +import com.ichi2.anki.ui.internationalization.toSentenceCase +import kotlinx.coroutines.test.advanceUntilIdle +import org.hamcrest.CoreMatchers.equalTo +import org.hamcrest.MatcherAssert.assertThat +import org.junit.Test +import org.junit.runner.RunWith + +@RunWith(AndroidJUnit4::class) +class FindAndReplaceDialogFragmentTest : RobolectricTest() { + @Test + fun `with no selected notes 'only selected notes' check box is not actionable`() = + runTest { + onFindReplaceFragment(emptyList()) { + // nothing selected so checkbox 'Only selected notes' should be unchecked and not enabled + onView(withId(R.id.only_selected_notes_check_box)) + .inRoot(isDialog()) + .check(matches(isNotEnabled())) + onView(withId(R.id.only_selected_notes_check_box)) + .inRoot(isDialog()) + .check(matches(isNotChecked())) + onView(withId(R.id.ignore_case_check_box)) + .inRoot(isDialog()) + .check(matches(isChecked())) + // using input as regex is not checked at start + onView(withId(R.id.input_as_regex_check_box)) + .inRoot(isDialog()) + .check(matches(isNotChecked())) + // as nothing is selected the target selector has only the two default options + assertThat(adapter.items, equalTo(targetContext.getDefaultTargets())) + } + } + + @Test + fun `shows expected find-replace targets for selected notes`() = + runTest { + val n1 = createFindReplaceTestNote("A", "Car", "Lion") + val n2 = createFindReplaceTestNote("B", "Train", "Chicken") + createFindReplaceTestNote("C", "Plane", "Vulture") + onFindReplaceFragment(listOf(n1.id, n2.id)) { + // 2 default field options + 4 fields(2 from each of the two notes selected above) + // see createFindReplaceTestNote for test field names + val allTargets = + targetContext.getDefaultTargets() + + listOf("Afield0", "Afield1", "Bfield0", "Bfield1") + assertThat(adapter.items, equalTo(allTargets)) + } + } + + // see #19929 + @Test + fun `'selected notes only' status is correct after fragment restore`() = + runTest { + val note = createFindReplaceTestNote("A", "kart", "kilogram") + val file = IdsFile(targetContext.cacheDir, listOf(note.id)) + val scenario = + FragmentScenario.launch( + fragmentClass = FindAndReplaceDialogFragment::class.java, + fragmentArgs = bundleOf(FindAndReplaceDialogFragment.ARG_IDS to file), + themeResId = R.style.Theme_Light, + ) + scenario.onFragment { fragment -> + advanceUntilIdle() + onView(withId(R.id.only_selected_notes_check_box)) + .inRoot(isDialog()) + .check(matches(isEnabled())) + onView(withId(R.id.only_selected_notes_check_box)) + .inRoot(isDialog()) + .check(matches(isChecked())) + // 2 default field options + 2 fields from the only note selected + val allTargets = + targetContext.getDefaultTargets() + listOf("Afield0", "Afield1") + assertThat(fragment.adapter.items, equalTo(allTargets)) + } + scenario.recreate() + scenario.onFragment { fragment -> + advanceUntilIdle() + onView(withId(R.id.only_selected_notes_check_box)) + .inRoot(isDialog()) + .check(matches(isEnabled())) + onView(withId(R.id.only_selected_notes_check_box)) + .inRoot(isDialog()) + .check(matches(isChecked())) + // check that the target list from before the recreate call wasn't reset + val allTargets = targetContext.getDefaultTargets() + listOf("Afield0", "Afield1") + assertThat(fragment.adapter.items, equalTo(allTargets)) + } + } + + private fun onFindReplaceFragment( + noteIds: List, + action: FindAndReplaceDialogFragment.() -> Unit, + ) { + val file = IdsFile(targetContext.cacheDir, noteIds) + FragmentScenario + .launch( + fragmentClass = FindAndReplaceDialogFragment::class.java, + fragmentArgs = bundleOf(FindAndReplaceDialogFragment.ARG_IDS to file), + themeResId = R.style.Theme_Light, + ).onFragment { fragment -> fragment.action() } + } + + /** + * 3 notetypes available(named A, B and C) each with two fields. + * Fields names follow the pattern: "${NotetypeName}field${0/1}" (ex: "Afield1"). + * "C" notetype has the same name for field 1 as notetype B! + * second is a [Pair] representing the field data(the note has only two fields) + */ + private fun createFindReplaceTestNote( + notetypeName: String, + field0: String, + field1: String, + ): Note { + addStandardNoteType("A", arrayOf("Afield0", "Afield1"), "", "") + addStandardNoteType("B", arrayOf("Bfield0", "Bfield1"), "", "") + addStandardNoteType("C", arrayOf("Cfield0", "Bfield1"), "", "") + return addNoteUsingNoteTypeName(notetypeName, field0, field1) + } + + val FindAndReplaceDialogFragment.adapter: SpinnerAdapter + get() = dialog!!.findViewById(R.id.fields_selector).adapter as SpinnerAdapter + + private val SpinnerAdapter.items: List + get() = + mutableListOf().apply { + for (position in 0 until count) { + add(getItem(position) as String) + } + } + + private fun Context.getDefaultTargets() = + listOf( + TR.browsingAllFields().toSentenceCase(this, R.string.sentence_all_fields), + TR.editingTags(), + ) +}