Skip to content

Commit bdffe19

Browse files
lukstbitlukstbit
committed
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.
1 parent 8bb59ec commit bdffe19

File tree

4 files changed

+205
-80
lines changed

4 files changed

+205
-80
lines changed

‎AnkiDroid/src/main/java/com/ichi2/anki/browser/CardBrowserFragment.kt‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@ import com.ichi2.anki.dialogs.tags.TagsDialog
8282
import com.ichi2.anki.dialogs.tags.TagsDialogFactory
8383
import com.ichi2.anki.dialogs.tags.TagsDialogListener
8484
import com.ichi2.anki.export.ExportDialogFragment
85+
import com.ichi2.anki.launchCatching
8586
import com.ichi2.anki.launchCatchingTask
8687
import com.ichi2.anki.libanki.DeckId
8788
import com.ichi2.anki.model.CardStateFilter
@@ -862,7 +863,13 @@ class CardBrowserFragment :
862863

863864
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
864865
fun showFindAndReplaceDialog() {
865-
FindAndReplaceDialogFragment().show(parentFragmentManager, FindAndReplaceDialogFragment.TAG)
866+
lifecycleScope.launch {
867+
withProgress {
868+
val noteIds = activityViewModel.queryAllSelectedNoteIds()
869+
val fragment = FindAndReplaceDialogFragment.newInstance(requireContext(), noteIds)
870+
fragment.show(parentFragmentManager, FindAndReplaceDialogFragment.TAG)
871+
}
872+
}
866873
}
867874

868875
@KotlinCleanup("DeckSelectionListener is almost certainly a bug - deck!!")

‎AnkiDroid/src/main/java/com/ichi2/anki/browser/FindAndReplaceDialogFragment.kt‎

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,15 +17,17 @@
1717
package com.ichi2.anki.browser
1818

1919
import android.app.Dialog
20+
import android.content.Context
2021
import android.os.Bundle
2122
import android.widget.AdapterView
2223
import android.widget.ArrayAdapter
2324
import androidx.annotation.VisibleForTesting
25+
import androidx.annotation.VisibleForTesting.Companion.PRIVATE
2426
import androidx.appcompat.app.AlertDialog
27+
import androidx.core.os.BundleCompat
2528
import androidx.core.os.bundleOf
2629
import androidx.core.text.HtmlCompat
2730
import androidx.core.view.isVisible
28-
import androidx.fragment.app.activityViewModels
2931
import androidx.fragment.app.setFragmentResult
3032
import androidx.lifecycle.lifecycleScope
3133
import com.ichi2.anki.CardBrowser
@@ -41,6 +43,7 @@ import com.ichi2.anki.browser.FindAndReplaceDialogFragment.Companion.ARG_REPLACE
4143
import com.ichi2.anki.browser.FindAndReplaceDialogFragment.Companion.ARG_SEARCH
4244
import com.ichi2.anki.browser.FindAndReplaceDialogFragment.Companion.REQUEST_FIND_AND_REPLACE
4345
import com.ichi2.anki.databinding.FragmentFindReplaceBinding
46+
import com.ichi2.anki.libanki.NoteId
4447
import com.ichi2.anki.notetype.ManageNotetypes
4548
import com.ichi2.anki.ui.internationalization.toSentenceCase
4649
import com.ichi2.anki.utils.ext.setFragmentResultListener
@@ -66,7 +69,6 @@ import timber.log.Timber
6669
*/
6770
// TODO desktop offers history for inputs
6871
class FindAndReplaceDialogFragment : AnalyticsDialogFragment() {
69-
private val browserViewModel by activityViewModels<CardBrowserViewModel>()
7072
private lateinit var binding: FragmentFindReplaceBinding
7173

7274
override fun onCreateDialog(savedInstanceState: Bundle?): Dialog {
@@ -107,7 +109,15 @@ class FindAndReplaceDialogFragment : AnalyticsDialogFragment() {
107109
(dialog as? AlertDialog)?.positiveButton?.isEnabled = false
108110
binding.contentViewsGroup.isVisible = false
109111
binding.loadingViewsGroup.isVisible = true
110-
val noteIds = browserViewModel.queryAllSelectedNoteIds()
112+
val idsFile =
113+
requireNotNull(
114+
BundleCompat.getParcelable(
115+
requireArguments(),
116+
ARG_IDS,
117+
IdsFile::class.java,
118+
),
119+
)
120+
val noteIds = idsFile.getIds()
111121
binding.onlySelectedNotesCheckBox.isChecked = noteIds.isNotEmpty()
112122
binding.onlySelectedNotesCheckBox.isEnabled = noteIds.isNotEmpty()
113123
val fieldsNames =
@@ -134,7 +144,7 @@ class FindAndReplaceDialogFragment : AnalyticsDialogFragment() {
134144
}
135145

136146
// https://github.com/ankitects/anki/blob/64ca90934bc26ddf7125913abc9dd9de8cb30c2b/qt/aqt/browser/find_and_replace.py#L118
137-
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
147+
@VisibleForTesting(otherwise = PRIVATE)
138148
fun startFindReplace() {
139149
val search = binding.inputSearch.text
140150
val replacement = binding.inputReplace.text
@@ -167,6 +177,7 @@ class FindAndReplaceDialogFragment : AnalyticsDialogFragment() {
167177
companion object {
168178
const val TAG = "FindAndReplaceDialogFragment"
169179
const val REQUEST_FIND_AND_REPLACE = "request_find_and_replace"
180+
const val ARG_IDS = " arg_ids"
170181
const val ARG_SEARCH = "arg_search"
171182
const val ARG_REPLACEMENT = "arg_replacement"
172183
const val ARG_FIELD = "arg_field"
@@ -185,6 +196,16 @@ class FindAndReplaceDialogFragment : AnalyticsDialogFragment() {
185196
* the user selected "Tags" as the field target for the find and replace action.
186197
*/
187198
const val TAGS_AS_FIELD = "find_and_replace_dialog_fragment_tags_as_field"
199+
200+
fun newInstance(
201+
context: Context,
202+
noteIds: List<NoteId>,
203+
): FindAndReplaceDialogFragment {
204+
val file = IdsFile(context.cacheDir, noteIds)
205+
return FindAndReplaceDialogFragment().apply {
206+
arguments = bundleOf(ARG_IDS to file)
207+
}
208+
}
188209
}
189210
}
190211

‎AnkiDroid/src/test/java/com/ichi2/anki/CardBrowserTest.kt‎

Lines changed: 0 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,6 @@ import android.annotation.SuppressLint
1919
import android.content.Intent
2020
import android.os.Bundle
2121
import android.text.TextUtils
22-
import android.widget.Spinner
23-
import android.widget.SpinnerAdapter
2422
import android.widget.TextView
2523
import androidx.core.app.ActivityCompat
2624
import androidx.core.content.edit
@@ -38,7 +36,6 @@ import androidx.test.espresso.matcher.RootMatchers.isDialog
3836
import androidx.test.espresso.matcher.ViewMatchers
3937
import androidx.test.espresso.matcher.ViewMatchers.isChecked
4038
import androidx.test.espresso.matcher.ViewMatchers.isNotChecked
41-
import androidx.test.espresso.matcher.ViewMatchers.isNotEnabled
4239
import androidx.test.espresso.matcher.ViewMatchers.withEffectiveVisibility
4340
import androidx.test.espresso.matcher.ViewMatchers.withId
4441
import androidx.test.espresso.matcher.ViewMatchers.withSpinnerText
@@ -91,7 +88,6 @@ import com.ichi2.anki.scheduling.ForgetCardsDialog
9188
import com.ichi2.anki.servicelayer.PreferenceUpgradeService
9289
import com.ichi2.anki.servicelayer.PreferenceUpgradeService.PreferenceUpgrade.UpgradeBrowserColumns.Companion.LEGACY_COLUMN1_KEYS
9390
import com.ichi2.anki.servicelayer.PreferenceUpgradeService.PreferenceUpgrade.UpgradeBrowserColumns.Companion.LEGACY_COLUMN2_KEYS
94-
import com.ichi2.anki.ui.internationalization.toSentenceCase
9591
import com.ichi2.anki.utils.ext.getCurrentDialogFragment
9692
import com.ichi2.anki.utils.ext.showDialogFragment
9793
import com.ichi2.testutils.IntentAssert
@@ -1243,69 +1239,6 @@ class CardBrowserTest : RobolectricTest() {
12431239
}
12441240
}
12451241

1246-
@Test
1247-
fun `FindReplace - dialog has expected ui at start`() {
1248-
withBrowser {
1249-
showFindAndReplaceDialog()
1250-
// nothing selected so checkbox 'Only selected notes' is not available
1251-
onView(withId(R.id.only_selected_notes_check_box)).inRoot(isDialog()).check(matches(isNotEnabled()))
1252-
onView(withId(R.id.only_selected_notes_check_box)).inRoot(isDialog()).check(matches(isNotChecked()))
1253-
val fieldSelectorAdapter = getFindReplaceFieldsAdapter()
1254-
onView(withId(R.id.ignore_case_check_box)).inRoot(isDialog()).check(matches(isChecked()))
1255-
onView(withId(R.id.input_as_regex_check_box)).inRoot(isDialog()).check(matches(isNotChecked()))
1256-
// as nothing is selected the fields selector has only the two default options
1257-
assertNotNull(fieldSelectorAdapter, "Fields adapter was not set")
1258-
assertEquals(2, fieldSelectorAdapter.count)
1259-
assertEquals(
1260-
TR.browsingAllFields().toSentenceCase(targetContext, R.string.sentence_all_fields),
1261-
fieldSelectorAdapter.getItem(0),
1262-
)
1263-
assertEquals(TR.editingTags(), fieldSelectorAdapter.getItem(1))
1264-
}
1265-
}
1266-
1267-
@Test
1268-
fun `FindReplace - shows expected fields target based on browser selection`() {
1269-
fun SpinnerAdapter.getAdapterData(): List<String> =
1270-
mutableListOf<String>().apply {
1271-
for (position in 0 until count) {
1272-
add(getItem(position) as String)
1273-
}
1274-
}
1275-
1276-
createFindReplaceTestNote("A", "Car", "Lion")
1277-
createFindReplaceTestNote("B", "Train", "Chicken")
1278-
withBrowser {
1279-
assertEquals(2, viewModel.rowCount)
1280-
viewModel.selectRowAtPosition(1)
1281-
openFindAndReplace()
1282-
var fieldSelectorAdapter = getFindReplaceFieldsAdapter()
1283-
// 2 default field options + 2 fields from the one note selected
1284-
assertEquals(4, fieldSelectorAdapter.count)
1285-
val defaultFields =
1286-
listOf(
1287-
TR.browsingAllFields().toSentenceCase(targetContext, R.string.sentence_all_fields),
1288-
TR.editingTags(),
1289-
)
1290-
assertEquals(defaultFields + listOf("Bfield0", "Bfield1"), fieldSelectorAdapter.getAdapterData())
1291-
closeFindAndReplace()
1292-
viewModel.selectAll() // 2 notes above
1293-
openFindAndReplace()
1294-
fieldSelectorAdapter = getFindReplaceFieldsAdapter()
1295-
// 2 default field options + 4 from the two notes
1296-
assertEquals(6, fieldSelectorAdapter.count)
1297-
assertEquals(defaultFields + listOf("Afield0", "Afield1", "Bfield0", "Bfield1"), fieldSelectorAdapter.getAdapterData())
1298-
closeFindAndReplace()
1299-
viewModel.selectNone() // 2 notes above
1300-
openFindAndReplace()
1301-
fieldSelectorAdapter = getFindReplaceFieldsAdapter()
1302-
// selection is reset just 2 default field options
1303-
assertEquals(2, fieldSelectorAdapter.count)
1304-
assertEquals(defaultFields, fieldSelectorAdapter.getAdapterData())
1305-
closeFindAndReplace()
1306-
}
1307-
}
1308-
13091242
@Test
13101243
fun `FindReplace - dialog handles correctly match case checkbox set to true`() {
13111244
val note0 = createFindReplaceTestNote("A", "kchicKen", "kilogram")
@@ -1564,14 +1497,6 @@ class CardBrowserTest : RobolectricTest() {
15641497
advanceRobolectricLooper()
15651498
}
15661499

1567-
private fun CardBrowser.getFindReplaceFieldsAdapter(): SpinnerAdapter {
1568-
val findReplaceDialog = supportFragmentManager.findFragmentByTag(FindAndReplaceDialogFragment.TAG) as? DialogFragment
1569-
assertNotNull(findReplaceDialog, "Find and replace dialog is not available")
1570-
val adapter = findReplaceDialog.dialog?.findViewById<Spinner>(R.id.fields_selector)?.adapter
1571-
assertNotNull(adapter, "Find and replace fields adapter is not available")
1572-
return adapter
1573-
}
1574-
15751500
fun NotetypeJson.addNote(
15761501
field: String,
15771502
vararg fields: String,

‎AnkiDroid/src/test/java/com/ichi2/anki/browser/FindAndReplaceDialogFragmentTest.kt‎

Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,172 @@
1+
/*
2+
* Copyright (c) 2025 lukstbit <52494258+lukstbit@users.noreply.github.com>
3+
*
4+
* This program is free software; you can redistribute it and/or modify it under
5+
* the terms of the GNU General Public License as published by the Free Software
6+
* Foundation; either version 3 of the License, or (at your option) any later
7+
* version.
8+
*
9+
* This program is distributed in the hope that it will be useful, but WITHOUT ANY
10+
* WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A
11+
* PARTICULAR PURPOSE. See the GNU General Public License for more details.
12+
*
13+
* You should have received a copy of the GNU General Public License along with
14+
* this program. If not, see <http://www.gnu.org/licenses/>.
15+
*/
16+
17+
package com.ichi2.anki.browser
18+
19+
import android.content.Context
20+
import android.widget.Spinner
21+
import android.widget.SpinnerAdapter
22+
import androidx.core.os.bundleOf
23+
import androidx.fragment.app.testing.FragmentScenario
24+
import androidx.test.espresso.Espresso.onView
25+
import androidx.test.espresso.assertion.ViewAssertions.matches
26+
import androidx.test.espresso.matcher.RootMatchers.isDialog
27+
import androidx.test.espresso.matcher.ViewMatchers.isChecked
28+
import androidx.test.espresso.matcher.ViewMatchers.isEnabled
29+
import androidx.test.espresso.matcher.ViewMatchers.isNotChecked
30+
import androidx.test.espresso.matcher.ViewMatchers.isNotEnabled
31+
import androidx.test.espresso.matcher.ViewMatchers.withId
32+
import androidx.test.ext.junit.runners.AndroidJUnit4
33+
import com.ichi2.anki.CollectionManager.TR
34+
import com.ichi2.anki.R
35+
import com.ichi2.anki.RobolectricTest
36+
import com.ichi2.anki.libanki.Note
37+
import com.ichi2.anki.libanki.NoteId
38+
import com.ichi2.anki.ui.internationalization.toSentenceCase
39+
import kotlinx.coroutines.test.advanceUntilIdle
40+
import org.hamcrest.CoreMatchers.equalTo
41+
import org.hamcrest.MatcherAssert.assertThat
42+
import org.junit.Test
43+
import org.junit.runner.RunWith
44+
45+
@RunWith(AndroidJUnit4::class)
46+
class FindAndReplaceDialogFragmentTest : RobolectricTest() {
47+
@Test
48+
fun `with no selected notes 'only selected notes' check box is not actionable`() =
49+
runTest {
50+
onFindReplaceFragment(emptyList()) {
51+
// nothing selected so checkbox 'Only selected notes' should be unchecked and not enabled
52+
onView(withId(R.id.only_selected_notes_check_box))
53+
.inRoot(isDialog())
54+
.check(matches(isNotEnabled()))
55+
onView(withId(R.id.only_selected_notes_check_box))
56+
.inRoot(isDialog())
57+
.check(matches(isNotChecked()))
58+
onView(withId(R.id.ignore_case_check_box))
59+
.inRoot(isDialog())
60+
.check(matches(isChecked()))
61+
// using input as regex is not checked at start
62+
onView(withId(R.id.input_as_regex_check_box))
63+
.inRoot(isDialog())
64+
.check(matches(isNotChecked()))
65+
// as nothing is selected the target selector has only the two default options
66+
assertThat(adapter.items, equalTo(targetContext.getDefaultTargets()))
67+
}
68+
}
69+
70+
@Test
71+
fun `shows expected find-replace targets for selected notes`() =
72+
runTest {
73+
val n1 = createFindReplaceTestNote("A", "Car", "Lion")
74+
val n2 = createFindReplaceTestNote("B", "Train", "Chicken")
75+
createFindReplaceTestNote("C", "Plane", "Vulture")
76+
onFindReplaceFragment(listOf(n1.id, n2.id)) {
77+
// 2 default field options + 4 fields(2 from each of the two notes selected above)
78+
// see createFindReplaceTestNote for test field names
79+
val allTargets =
80+
targetContext.getDefaultTargets() +
81+
listOf("Afield0", "Afield1", "Bfield0", "Bfield1")
82+
assertThat(adapter.items, equalTo(allTargets))
83+
}
84+
}
85+
86+
// see #19929
87+
@Test
88+
fun `'selected notes only' status is correct after fragment restore`() =
89+
runTest {
90+
val note = createFindReplaceTestNote("A", "kart", "kilogram")
91+
val file = IdsFile(targetContext.cacheDir, listOf(note.id))
92+
val scenario =
93+
FragmentScenario.launch(
94+
fragmentClass = FindAndReplaceDialogFragment::class.java,
95+
fragmentArgs = bundleOf(FindAndReplaceDialogFragment.ARG_IDS to file),
96+
themeResId = R.style.Theme_Light,
97+
)
98+
scenario.onFragment { fragment ->
99+
advanceUntilIdle()
100+
onView(withId(R.id.only_selected_notes_check_box))
101+
.inRoot(isDialog())
102+
.check(matches(isEnabled()))
103+
onView(withId(R.id.only_selected_notes_check_box))
104+
.inRoot(isDialog())
105+
.check(matches(isChecked()))
106+
// 2 default field options + 2 fields from the only note selected
107+
val allTargets =
108+
targetContext.getDefaultTargets() + listOf("Afield0", "Afield1")
109+
assertThat(fragment.adapter.items, equalTo(allTargets))
110+
}
111+
scenario.recreate()
112+
scenario.onFragment { fragment ->
113+
advanceUntilIdle()
114+
onView(withId(R.id.only_selected_notes_check_box))
115+
.inRoot(isDialog())
116+
.check(matches(isEnabled()))
117+
onView(withId(R.id.only_selected_notes_check_box))
118+
.inRoot(isDialog())
119+
.check(matches(isChecked()))
120+
// check that the target list from before the recreate call wasn't reset
121+
val allTargets = targetContext.getDefaultTargets() + listOf("Afield0", "Afield1")
122+
assertThat(fragment.adapter.items, equalTo(allTargets))
123+
}
124+
}
125+
126+
private fun onFindReplaceFragment(
127+
noteIds: List<NoteId>,
128+
action: FindAndReplaceDialogFragment.() -> Unit,
129+
) {
130+
val file = IdsFile(targetContext.cacheDir, noteIds)
131+
FragmentScenario
132+
.launch(
133+
fragmentClass = FindAndReplaceDialogFragment::class.java,
134+
fragmentArgs = bundleOf(FindAndReplaceDialogFragment.ARG_IDS to file),
135+
themeResId = R.style.Theme_Light,
136+
).onFragment { fragment -> fragment.action() }
137+
}
138+
139+
/**
140+
* 3 notetypes available(named A, B and C) each with two fields.
141+
* Fields names follow the pattern: "${NotetypeName}field${0/1}" (ex: "Afield1").
142+
* "C" notetype has the same name for field 1 as notetype B!
143+
* second is a [Pair] representing the field data(the note has only two fields)
144+
*/
145+
private fun createFindReplaceTestNote(
146+
notetypeName: String,
147+
field0: String,
148+
field1: String,
149+
): Note {
150+
addStandardNoteType("A", arrayOf("Afield0", "Afield1"), "", "")
151+
addStandardNoteType("B", arrayOf("Bfield0", "Bfield1"), "", "")
152+
addStandardNoteType("C", arrayOf("Cfield0", "Bfield1"), "", "")
153+
return addNoteUsingNoteTypeName(notetypeName, field0, field1)
154+
}
155+
156+
val FindAndReplaceDialogFragment.adapter: SpinnerAdapter
157+
get() = dialog!!.findViewById<Spinner>(R.id.fields_selector).adapter as SpinnerAdapter
158+
159+
private val SpinnerAdapter.items: List<String>
160+
get() =
161+
mutableListOf<String>().apply {
162+
for (position in 0 until count) {
163+
add(getItem(position) as String)
164+
}
165+
}
166+
167+
private fun Context.getDefaultTargets() =
168+
listOf(
169+
TR.browsingAllFields().toSentenceCase(this, R.string.sentence_all_fields),
170+
TR.editingTags(),
171+
)
172+
}

0 commit comments

Comments
 (0)