Fix missing minus symbol when updating tags in card browser

[thedroiddiv]

Purpose / Description

Fix missing minus symbol when selecting several cards at once in the card browser to update tags.

Fixes

• Fixes Card browser: Tags incorrectly shown #19083

Approach

Problem

This happened because the previous implementation

• treated all notes present in any note as "checked"
• remaining all were marked as "unchecked

Expected

• tags present in all notes : "checked"
• tags present in some, but not all notes: "indeterminate"
• tags present in none of the notes: unchecked

Fix

• check all notes, check if each tag is present or absent
• add to checked list if present else add to unchecked list
• this operations might result in few tags being present in both lists
• intersecting tags are the "indeterminate" ones which are correctly captured by [TagsList]
• tags coming from intent's extras are treated as absolute checked and cannot be indeterminate

How Has This Been Tested?

Added unit test for "indeterminate"
Existing tests pass
Manually Tested

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

[thedroiddiv]

Also, the tests assume NoteList works as expected, any error in NoteList will result in ViewModelTest to fail, even though actual issue might not be in the ViewModel.

[david-allison]

Thanks!

As a request: could the conclusion of this be moved into the class/argument documentation:

#19499 (comment)

[thedroiddiv]

As a request: could the conclusion of this be moved into the class/argument documentation:

It's there as a test, will include in docs too.

Todos before next approval:

• Add the conclusion to docs 19499#discussion_r2532184695
• Replace if-else partitioning with Array::partition
• Lift up db call out of loop
• Add a todo for "Lift up db call out of loop"

[david-allison]

You asked for a re-review. I was happy, and still treat this as an approve, but there's an optional improvement I felt was relevant

[david-allison]

This seems to be in the wrong place, as does the line below

[david-allison]

I feel this documentation is a little too far away from the caller to understand what EXTRAS is

Fully optional, or moving it to another issue

Subject: [PATCH]
---
Index: AnkiDroid/src/test/java/com/ichi2/anki/dialogs/tags/TagsDialogViewModelTest.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/dialogs/tags/TagsDialogViewModelTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/dialogs/tags/TagsDialogViewModelTest.kt
--- a/AnkiDroid/src/test/java/com/ichi2/anki/dialogs/tags/TagsDialogViewModelTest.kt	(revision 3957d757ab0e6000579a4586595c25819ebd63d1)
+++ b/AnkiDroid/src/test/java/com/ichi2/anki/dialogs/tags/TagsDialogViewModelTest.kt	(date 1764544045091)
@@ -16,8 +16,10 @@
 
 package com.ichi2.anki.dialogs.tags
 
+import androidx.lifecycle.SavedStateHandle
 import androidx.test.ext.junit.runners.AndroidJUnit4
 import com.ichi2.anki.RobolectricTest
+import com.ichi2.anki.dialogs.tags.TagsDialog.DialogType
 import com.ichi2.anki.libanki.NoteId
 import com.ichi2.anki.libanki.testutils.ext.newNote
 import kotlinx.coroutines.ExperimentalCoroutinesApi
@@ -69,7 +71,7 @@
     fun `single note produces correct checked, indeterminate and all sets`() =
         runTest(testDispatcher) {
             val vm =
-                TagsDialogViewModel(
+                createViewModel(
                     noteIds = listOf(note1),
                     checkedTags = listOf("a"),
                     isCustomStudying = false,
@@ -90,7 +92,7 @@
             // note 1           a, b, c
             // note 2              b, d
             val vm =
-                TagsDialogViewModel(
+                createViewModel(
                     noteIds = listOf(note1, note2),
                     checkedTags = emptyList(),
                     isCustomStudying = false,
@@ -118,7 +120,7 @@
     fun `extra checkedTags is absolute checked and not indeterminate`() =
         runTest(testDispatcher) {
             val vm =
-                TagsDialogViewModel(
+                createViewModel(
                     noteIds = listOf(note1),
                     checkedTags = listOf("d", "x"),
                     isCustomStudying = false,
@@ -143,7 +145,7 @@
     fun `custom study mode makes all tags unchecked`() =
         runTest(testDispatcher) {
             val vm =
-                TagsDialogViewModel(
+                createViewModel(
                     noteIds = listOf(note1, note2),
                     checkedTags = listOf("a"),
                     isCustomStudying = true,
@@ -162,3 +164,21 @@
             assertTrue(tags.copyOfAllTagList().isNotEmpty())
         }
 }
+
+private fun RobolectricTest.createViewModel(
+    noteIds: List<NoteId>,
+    checkedTags: List<String>,
+    isCustomStudying: Boolean,
+): TagsDialogViewModel {
+    val bundle = TagsDialog().withArguments(targetContext,
+        type = if (isCustomStudying) DialogType.CUSTOM_STUDY else DialogType.EDIT_TAGS,
+        noteIds = noteIds,
+        checkedTags = ArrayList(checkedTags)
+    ).arguments
+
+    val savedState = SavedStateHandle.createHandle(
+        restoredState = null,
+        defaultState = bundle
+    )
+    return TagsDialogViewModel(savedState)
+}
Index: AnkiDroid/src/main/java/com/ichi2/anki/dialogs/tags/TagsDialogViewModel.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/dialogs/tags/TagsDialogViewModel.kt b/AnkiDroid/src/main/java/com/ichi2/anki/dialogs/tags/TagsDialogViewModel.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/dialogs/tags/TagsDialogViewModel.kt	(revision 3957d757ab0e6000579a4586595c25819ebd63d1)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/dialogs/tags/TagsDialogViewModel.kt	(date 1764543713358)
@@ -15,29 +15,50 @@
  */
 package com.ichi2.anki.dialogs.tags
 
+import androidx.lifecycle.SavedStateHandle
 import androidx.lifecycle.ViewModel
 import com.ichi2.anki.CollectionManager.withCol
 import com.ichi2.anki.asyncIO
-import com.ichi2.anki.libanki.NoteId
+import com.ichi2.anki.browser.IdsFile
+import com.ichi2.anki.dialogs.tags.TagsDialog.Companion.ARG_CHECKED_TAGS
+import com.ichi2.anki.dialogs.tags.TagsDialog.Companion.ARG_DIALOG_TYPE
+import com.ichi2.anki.dialogs.tags.TagsDialog.Companion.ARG_TAGS_FILE
+import com.ichi2.anki.dialogs.tags.TagsDialog.DialogType
 import kotlinx.coroutines.Deferred
 import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.flow.asStateFlow
 
-/**
- * @param noteIds IDs of notes whose tags should bfe retrieved and marked as "checked"
- * @param checkedTags additional list of checked tags.
- * These tags coming from EXTRAS are treated as absolute checked and cannot be indeterminate.
- * @param isCustomStudying true if all inputs are to be handled as unchecked tags, false otherwise(
- * this is a temporary parameter until custom study by tags is modified)
- *  They are joined with the tags retrieved from noteIds
- *
- *  @see <a href="https://github.com/ankidroid/Anki-Android/pull/19499#discussion_r2532184695">Extra checked tags</>
- */
-class TagsDialogViewModel(
-    noteIds: Collection<NoteId> = emptyList(),
-    checkedTags: Collection<String> = emptyList(),
-    isCustomStudying: Boolean = false,
-) : ViewModel() {
+class TagsDialogViewModel(savedStateHandle: SavedStateHandle) : ViewModel() {
+
+    /**
+     * IDs of notes whose tags should bfe retrieved and marked as "checked"
+     */
+    private val noteIds =
+        requireNotNull(savedStateHandle.get<IdsFile>(ARG_TAGS_FILE)) {
+            "$ARG_TAGS_FILE is required"
+        }.getIds()
+
+    /**
+     * Additional list of checked tags.
+     *
+     * These tags are tags of a current note, and cannot be indeterminate.
+     * These tags may not yet be saved to the collection.
+     *
+     * @see <a href="https://github.com/ankidroid/Anki-Android/pull/19499#discussion_r2532184695">Extra checked tags</>
+     */
+    private val checkedTags =
+        requireNotNull(savedStateHandle.get<ArrayList<String>>(ARG_CHECKED_TAGS)) {
+            "$ARG_CHECKED_TAGS is required"
+        }
+
+    /**
+     * `true` if all inputs are to be handled as unchecked tags, `false` otherwise
+     * (this is a temporary parameter until custom study by tags is modified)
+     */
+    val isCustomStudying = savedStateHandle.get<DialogType>(ARG_DIALOG_TYPE)
+        ?.let { it == DialogType.CUSTOM_STUDY } == true
+
+
     val tags: Deferred<TagsList>
 
     private val _initProgress = MutableStateFlow<InitProgress>(InitProgress.Processing)
Index: AnkiDroid/src/main/java/com/ichi2/anki/dialogs/tags/TagsDialog.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/dialogs/tags/TagsDialog.kt b/AnkiDroid/src/main/java/com/ichi2/anki/dialogs/tags/TagsDialog.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/dialogs/tags/TagsDialog.kt	(revision 3957d757ab0e6000579a4586595c25819ebd63d1)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/dialogs/tags/TagsDialog.kt	(date 1764543713364)
@@ -26,8 +26,6 @@
 import androidx.fragment.app.viewModels
 import androidx.lifecycle.flowWithLifecycle
 import androidx.lifecycle.lifecycleScope
-import androidx.lifecycle.viewmodel.initializer
-import androidx.lifecycle.viewmodel.viewModelFactory
 import androidx.recyclerview.widget.LinearLayoutManager
 import androidx.recyclerview.widget.RecyclerView
 import com.ichi2.anki.CollectionManager.TR
@@ -90,30 +88,7 @@
     private lateinit var selectedOption: CardStateFilter
 
     @VisibleForTesting
-    val viewModel: TagsDialogViewModel by viewModels {
-        val idsFile =
-            requireNotNull(
-                BundleCompat.getParcelable(requireArguments(), ARG_TAGS_FILE, IdsFile::class.java),
-            ) {
-                "$ARG_TAGS_FILE is required"
-            }
-        val noteIds = idsFile.getIds()
-        val checkedTags =
-            requireNotNull(requireArguments().getStringArrayList(ARG_CHECKED_TAGS)) {
-                "$ARG_CHECKED_TAGS is required"
-            }
-        val type = BundleCompat.getParcelable(requireArguments(), ARG_DIALOG_TYPE, DialogType::class.java)
-        val isCustomStudying = type != null && type == DialogType.CUSTOM_STUDY
-        viewModelFactory {
-            initializer {
-                TagsDialogViewModel(
-                    noteIds = noteIds,
-                    checkedTags = checkedTags,
-                    isCustomStudying = isCustomStudying,
-                )
-            }
-        }
-    }
+    val viewModel: TagsDialogViewModel by viewModels()
 
     /**
      * Constructs a new [TagsDialog] that will communicate the results using the provided listener.
@@ -439,8 +414,8 @@
 
     companion object {
         const val ARG_TAGS_FILE = "tagsFile"
-        private const val ARG_DIALOG_TYPE = "dialogType"
-        private const val ARG_CHECKED_TAGS = "checkedTags"
+        const val ARG_DIALOG_TYPE = "dialogType"
+        const val ARG_CHECKED_TAGS = "checkedTags"
 
         /**
          * The filter that constrains the inputted tag.

[thedroiddiv]

I think, let's keep this refactor independent from this, maybe done in another PR. This is already as you highlighted a high-risk change, so keeping the changes minimal.

Can definitely re-look on the docs aspect.

[mikehardy]

@david-allison per discussion on discord, queue for release-2.23 pick or not? I think we decided not - but perhaps later after alpha testing verifies it's working?

[david-allison]

As discussed, a little too much risk. My mistake last year with tags still haunts me.

Let's aim for a fairly quick 2.24 with this in

[thedroiddiv]

Hey, I was OOO last entire week, will sit on this PR once again coming week.

[thedroiddiv]

@david-allison @mikehardy I'm not sure what actionable item do I have here after "Queued for cherry pick..." tag.

Do I apply the suggested patch and push? Or the current code is mergable?

[david-allison]

No action needed, awaiting second approval.

After a brief discussion, tag changes are a little too high risk to cherry pick to the 2.23 release. I screwed up here last year.

[david-allison]

For second reviewer: Ensure this use case is tested, or re-open the issue once this is merged:

• cannot append/remove a tag to multiple notes without them inheriting all of each other's tags #20189 (comment)