Remove legacy collection loading code from AnkiFragment #17457
Merged
Conversation
This file contains 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
This class was using the legacy CollectionLoader to return the already available for use, CollectionManager.getColUnsafe(). The try-catch replicates CollectionLoader's behavior which invokes the activity's onCollectionLoadError() method to go to DeckPicker when faced with any exception during loading the collection. Probably fixes ankidroid#17380 where a NullPointerException is thrown for one of AnkiFragment's subclasses, NoteEditor, because the culprit property editorNote is assigned a valid note in the callback onCollectionLoaded() which may or may not be executed at the right time.
|
@criticalAY I made this PR although you were assigned because I don't think the bug is related to multimedia stuff. |
|
Cool cool, that was not related to multimedia anyway thanks for taking some weight off ;) |
mikehardy
approved these changes
Nov 18, 2024
mikehardy
added
Review High Priority
david-allison
approved these changes
Nov 18, 2024
david-allison
added
Pending Merge
|
On it re: the testing request |
|
Worked fine, going to merge etc test: Seems good anki-card-context-menu-clean-open.webmWow, should have made that lower resolution / quality, sorry. Quite a big file. |
mikehardy
removed
Review High Priority
github-actions
bot
removed
the
Pending Merge
mikehardy
added
Cherry Picked to Stable Branch
and removed
Queued for Cherry Pick to Stable Branch
labels
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
AnkiFragment was using CollectionManager.getColUnsafe() and also set up a loading mechanism similar with the CollectionLoader used by AnkiActivity returning the same CollectionManager.getColUnsafe(). This PR removes the loading mechanism and lets NoteEditor to only use CollectionManager.getColUnsafe(). This way, the setup that was done in onCollectionLoaded() is inserted in the fragment lifecycle and should prevent bugs from the bad timing around onCollectionLoaded().
The progress bar in the layout was removed because it was only used for the collection loading mechanism.
I was kind of able to reproduce the error in #17380 by using the same steps from #17407(modify the collection loading in ANkiFragment) and opening NoteEditor -> attach image. I said kind of because there's a bug in the multimedia image selection(see #17455) where the return to NoteEditor doesn't work quite right with Don't keep activities enabled. IMO both bugs in #17380 and #17407 are happening because there's bad timing between the code expecting the collection to be available and the collection not being yet available( meaning onCollectionLoaded() executed).
At reviewers discretion if this should go in 2.19.x.
Fixes
NoteEditor.getCurrentMultimediaEditableNote: NullPointerException #17380How Has This Been Tested?
Tried to add things, ran the tests.
Checklist