Fix state restore bug in FindAndReplaceDialogFragment #19981
+205
−5
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 tasks
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.
lukstbit
force-pushed
the
fix_findReplaceBackgroundRestore
branch
from
bdffe19 to
a7cd900
Compare
david-allison
requested changes
Jan 5, 2026
Member
david-allison
left a comment
david-allison
left a comment
There was a problem hiding this comment.
Needs to handle IdsFile being missing
This is a common issue
| companion object { | ||
| const val TAG = "FindAndReplaceDialogFragment" | ||
| const val REQUEST_FIND_AND_REPLACE = "request_find_and_replace" | ||
| const val ARG_IDS = " arg_ids" |
Member
There was a problem hiding this comment.
Suggested change
| const val ARG_IDS = " arg_ids" | |
| const val ARG_IDS = "arg_ids" |
Comment on lines
+112
to
+120
| val idsFile = | ||
| requireNotNull( | ||
| BundleCompat.getParcelable( | ||
| requireArguments(), | ||
| ARG_IDS, | ||
| IdsFile::class.java, | ||
| ), | ||
| ) | ||
| val noteIds = idsFile.getIds() |
Member
There was a problem hiding this comment.
Add handling for the file being deleted
david-allison
requested changes
Jan 5, 2026
| context: Context, | ||
| noteIds: List<NoteId>, | ||
| ): FindAndReplaceDialogFragment { | ||
| val file = IdsFile(context.cacheDir, noteIds) |
Member
There was a problem hiding this comment.
We have a common issue that IdsFile is not deleted after the activity/fragment/dialog is disposed.
Is it feasible to handle this in the PR for this fragment?
| context: Context, | ||
| noteIds: List<NoteId>, | ||
| ): FindAndReplaceDialogFragment { | ||
| val file = IdsFile(context.cacheDir, noteIds) |
Member
There was a problem hiding this comment.
Please provide a name to the IdsFile, this will make it easier to clean up our 'bad' IdsFile implementation at a future date
Member
🤔 which bug ? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose / Description
Fixes a state restore bug for find and replace fragment where previously selected notes would get ignored when coming back from the background. This is not great because if the user doesn't notice he's going to run the find and replace operation over his entire collection. See bug:
Screen_recording_20251230_192801.webm
I used our IdsFile implementation to not query the browser's ViewModel. I'm not enthusiastic about this but it works, the browser could probably be changed to fix this but I didn't look into it.
With the code in this PR:
Screen_recording_20251230_191350.webm
Fixes
How Has This Been Tested?
Checked the bug scenario, ran tests.
Checklist