fix: TagsDialog crashes

[david-allison]

Purpose / Description

• Fix a TransactionTooLargeException and an OutOfMemoryError

This is a tradeoff of stability vs performance. We should do much better here in a follow-up

Fixes

• Fixes [BUG]: crash on clicking "Edit tags" #16226
• Fixes Crash on switching app while edit tags dialog is open #16351

Approach

• Serialize to a file, rather than use Args
• Use asSequence to fix an OOM
  • This makes the dialog process 2x slower: we need to iterate the selected notes twice
  • This may actually be faster after the PR on some collection, as we perform select distinct nid BEFORE loading the notes
• Add a progress dialog to help the user

How Has This Been Tested?

My S21, AnKing deck (31479 cards)

No longer crashes on AnKing
⚠️ It is still FAR too slow, needing to individually load 31k cards. This requires a follow-up issue

Checklist

• You have a descriptive commit message with a short title (first line, max 50 chars).
• You have commented your code, particularly in hard-to-understand areas
• You have performed a self-review of your own code
• UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
• UI Changes: You have tested your change using the Google Accessibility Scanner

[david-allison]

For future

Index: AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt b/AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt	(revision 3c3f39d7de6756959acdd2b1fdc9c89473a257ff)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt	(date 1714909328656)
@@ -1295,14 +1295,21 @@
                 // TODO!! This is terribly slow on AnKing
                 // PERF: This MUST be combined with the above sequence - this becomes O(2n) on a
                 // database operation performed over 30k times
+                val potentialTags = allTags.toMutableSet()
                 val uncheckedTags = withCol {
                     selectedNoteIds
                         .asSequence() // reduce memory pressure
                         .flatMap { nid: NoteId ->
                             progress++
-                            val note = getNote(nid) // requires withCol
-                            val noteTags: List<String?> = note.tags
-                            allTags.filter { t: String? -> !noteTags.contains(t) }
+
+                            // avoid DB access if no more tags to check
+                            if (potentialTags.isEmpty()) return@flatMap emptyList<String>()
+
+                            val noteTags = getNote(nid).tags.toSet() // requires withCol
+                            val uncheckedTagsForNote = potentialTags - noteTags
+                            // PERF: if we're returning these tags, we don't need to check them again
+                            potentialTags -= uncheckedTagsForNote
+                            uncheckedTagsForNote
                         }
                         .distinct()
                         .toList()

[david-allison]

I've added a progress dialog as well

[BrayanDSO]

Performance-wise, it definitely can be improved and should be improved, but I agree with you that fixing and testing it should be the priority

[BrayanDSO]

Is @Serializable necessary here? I don't see a TagsData instance being serialized anywhere.

[david-allison]

                                                                             kotlinx.serialization.SerializationException: Serializer for class 'TagsData' is not found.
                                                                             Please ensure that class is marked as '@Serializable' and that the serialization compiler plugin is applied.

[BrayanDSO]

I think that replacing this with @Parcelize should work (and adding : Parcelable to the class), and Parcelables tend to be faster than Serializables, but I'm not going to block on that

[david-allison]

I couldn't get this working, happy for someone to take on a follow-up

[mikehardy]

@david-allison this one looks like it needs a look from you?

[david-allison]

@mikehardy defer to 2.18.1 I feel. Nether of these are regressions in 2.18, and this needs more time

[mikehardy]

esp with the progress dialog this seems like a huge improvement -

first - just not crashing
second - does have some user feedback now

seems very worthwhile to get in as a quick fix so AnKing users aren't crashing

[BrayanDSO]

I think that replacing this with @Parcelize should work (and adding : Parcelable to the class), and Parcelables tend to be faster than Serializables, but I'm not going to block on that