Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,12 @@ 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* @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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*
* @see <a href="https://github.com/ankidroid/Anki-Android/pull/19499#discussion_r2532184695">Extra checked tags</>
*/
class TagsDialogViewModel(
noteIds: Collection<NoteId> = emptyList(),
Expand All @@ -43,17 +46,26 @@ class TagsDialogViewModel(
init {
tags =
asyncIO {
val allTags = withCol { tags.all() }.toSet()
val allCheckedTags =
noteIds
.flatMapIndexedTo(mutableSetOf()) { index, nid ->
_initProgress.emit(InitProgress.FetchingNoteTags(index + 1, noteIds.size))
withCol { getNote(nid) }.tags
}.apply {
addAll(checkedTags)
}
val allTags = withCol { tags.all() }
val allCheckedTags = mutableSetOf<String>()
val uncheckedTags = mutableSetOf<String>()
// For each note, put the checked tag in checked list and unchecked tags in unchecked list.
// This will result in few tags being present in both lists, checked and
// unchecked as they might be present in one note but absent in any other.
// Such tags are referred as `indeterminateTags` in [TagsList]
noteIds.forEachIndexed { index, nid ->
// TODO: Lift up withCol{ } call out of loop. Performs `N` expensive db queries.
val noteTags = withCol { getNote(nid) }.tags
_initProgress.emit(InitProgress.FetchingNoteTags(index + 1, noteIds.size))
val (checked, unchecked) = allTags.partition { noteTags.contains(it) }
allCheckedTags.addAll(checked)
uncheckedTags.addAll(unchecked)
}
// add the extra checked tags, these are to be shown as `checked` and cannot be indeterminate
val extraCheckedTags = checkedTags.toSet()
allCheckedTags.addAll(extraCheckedTags)
uncheckedTags.removeAll(extraCheckedTags)
_initProgress.emit(InitProgress.Processing)
val uncheckedTags = allTags - allCheckedTags
if (isCustomStudying) {
TagsList(
allTags = allCheckedTags,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ class TagsList(
private val checkedTags: MutableSet<String> = TreeSet(java.lang.String.CASE_INSENSITIVE_ORDER)

/**
* A Set containing the tags with indeterminate state
* A Set containing the tags with indeterminate state.
* For a tag to be in indeterminate state it should be present in checkedTags and also in uncheckedTags.
*/
private val indeterminateTags: MutableSet<String>

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
/*
* Copyright (c) 2025 Divyansh Kushwaha <[email protected]>
*
* This program is free software; you can redistribute it and/or modify it under
* the terms of the GNU General Public License as published by the Free Software
* Foundation; either version 3 of the License, or (at your option) any later
* version.
*
* This program is distributed in the hope that it will be useful, but WITHOUT ANY
* WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A
* PARTICULAR PURPOSE. See the GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License along with
* this program. If not, see <http://www.gnu.org/licenses/>.
*/

package com.ichi2.anki.dialogs.tags

import androidx.test.ext.junit.runners.AndroidJUnit4
import com.ichi2.anki.RobolectricTest
import com.ichi2.anki.libanki.NoteId
import com.ichi2.anki.libanki.testutils.ext.newNote
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.test.StandardTestDispatcher
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import kotlin.properties.Delegates

@OptIn(ExperimentalCoroutinesApi::class)
@RunWith(AndroidJUnit4::class)
class TagsDialogViewModelTest : RobolectricTest() {
private var note1 by Delegates.notNull<NoteId>()
private var note2 by Delegates.notNull<NoteId>()

private val testDispatcher = StandardTestDispatcher()

/**
* Initialize the collections with three notes with each having some tags
* ```
* note1 a,b,c
* note2 b,d
* note3 e,f
* ```
*/
@Before
fun setupNotes() {
setupTestDispatcher(testDispatcher)

val n1 = col.newNote()
col.addNote(n1, 0)
col.tags.bulkAdd(listOf(n1.id), "a b c")
note1 = n1.id

val n2 = col.newNote()
col.addNote(n2, 0)
col.tags.bulkAdd(listOf(n2.id), "b d")
note2 = n2.id

val n3 = col.newNote()
col.addNote(n3, 0)
col.tags.bulkAdd(listOf(n3.id), "e f")
}

@Test
fun `single note produces correct checked, indeterminate and all sets`() =
runTest(testDispatcher) {
val vm =
TagsDialogViewModel(
noteIds = listOf(note1),
checkedTags = listOf("a"),
isCustomStudying = false,
)
val tags = vm.tags.await()
// only one note, tags can either be checked or unchecked only
// all : {a,b,c,d,e,f}
// indeterminate: { }
// checked: {a,b,c}
assertEquals(listOf("a", "b", "c"), tags.copyOfCheckedTagList().sorted())
assertTrue(tags.copyOfIndeterminateTagList().isEmpty())
assertEquals(listOf("a", "b", "c", "d", "e", "f"), tags.copyOfAllTagList().sorted())
}

@Test
fun `multiple notes create indeterminate tags correctly`() =
runTest(testDispatcher) {
// note 1 a, b, c
// note 2 b, d
val vm =
TagsDialogViewModel(
noteIds = listOf(note1, note2),
checkedTags = emptyList(),
isCustomStudying = false,
)

val tags = vm.tags.await()
val checked = tags.copyOfCheckedTagList()
val ind = tags.copyOfIndeterminateTagList()

// All tags in the collection: {a,b,c,d,e,f}
// Assuming indeterminate logic in [TagsList] is correct
// a [-] - indeterminate
// b [x] - checked
// c [-] - indeterminate
// d [-] - indeterminate
// e [ ] - unchecked
// f [ ] - unchecked
assertEquals(listOf("b"), checked.sorted())
assertEquals(listOf("a", "c", "d"), ind.sorted())
assertFalse(tags.isChecked("e"))
assertFalse(tags.isChecked("f"))
}

@Test
fun `extra checkedTags is absolute checked and not indeterminate`() =
runTest(testDispatcher) {
val vm =
TagsDialogViewModel(
noteIds = listOf(note1),
checkedTags = listOf("d", "x"),
isCustomStudying = false,
)

// note1.tags {a,b}
// extraCheckedTags {d, x}
// allTags {a,b,c,d,e}
// implies inside dialog
// checkedTags {a, b, d, x}
// uncheckedTags { c, e }
// indeterminate { }
val tags = vm.tags.await()
assertTrue(tags.isChecked("d"))
assertFalse(tags.isIndeterminate("d"))
assertTrue(tags.isChecked("x"))
assertFalse(tags.isIndeterminate("d"))
assertTrue(tags.copyOfAllTagList().contains("x"))
}

@Test
fun `custom study mode makes all tags unchecked`() =
runTest(testDispatcher) {
val vm =
TagsDialogViewModel(
noteIds = listOf(note1, note2),
checkedTags = listOf("a"),
isCustomStudying = true,
)
val tags = vm.tags.await()
// tags from both notes = {a,b,c,d}
val all = tags.copyOfAllTagList().sorted()
assertEquals(listOf("a", "b", "c", "d"), all)
// custom study: checked = empty
assertTrue(tags.copyOfCheckedTagList().isEmpty())
// custom study: indeterminate = empty
assertTrue(tags.copyOfIndeterminateTagList().isEmpty())
// custom study:
// checked - empty, unchecked - empty, all - not empty
// implies unchecked = allTags
assertTrue(tags.copyOfAllTagList().isNotEmpty())
}
}