Skip to content

Commit a7cd900

Browse files
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 a7cd900

File tree

3 files changed

+205
-5
lines changed

3 files changed

+205
-5
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

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)