Refactor: Move deck operations business logic from DeckPicker to DeckPickerViewModel #19245

sanjaysargam · 2025-09-16T18:16:41Z

Purpose / Description

This PR moves 8 deck-related business logic functions from DeckPicker.kt to DeckPickerViewModel.kt to improve code architecture and follow MVVM pattern properly.

Fixes

Related ANR: DeckPicker:onResume => updateDeckList #18886

Approach

Moved business logic functions to ViewModel:
1. getDeckName() - Gets deck name for rename operations
2. onSubDeckCreated() - Notifies when subdeck is created
3. rebuildFilteredDeck() - Rebuilds filtered deck with current settings
4. exportDeck() - Handles deck export requests
5. createIcon() - Prepares data for deck shortcut creation
6. disableDeckAndChildrenShortcuts() - Gets deck IDs for disabling shortcuts
7. findDeckPosition() - Finds deck position in flattened list
8. isDeckAndSubdeckEmpty() - Checks if deck and subdecks are empty

Added reactive flows for UI communication:
- flowOfExportDeck - For export requests
- flowOfCreateShortcut - For shortcut creation with ShortcutData
- flowOfDisableShortcuts - For shortcut disabling
- flowOfSubDeckCreated - For subdeck creation notifications

Updated DeckPicker.kt:
- Modified methods to use ViewModel instead of direct collection access
- Added flow listeners for reactive UI updates

How Has This Been Tested?

Physical Device (Redmi)

Checklist

Please, go through these checks before submitting the PR.

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

sanjaysargam · 2025-09-16T18:33:03Z

DeckPickerTest > [0] > ContextMenu creates deck shortcut when selecting CREATE_SHORTCUT[0] FAILED

Uff, I'll resolve this tomorrow

AnkiDroid/src/main/java/com/ichi2/anki/deckpicker/DeckPickerViewModel.kt

AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt

AnkiDroid/src/test/java/com/ichi2/anki/DeckPickerTest.kt

david-allison · 2025-09-21T14:30:56Z

Replying to a comment

sanjaysargam · 2025-09-21T20:03:32Z

Replying to a comment

@david-allison Not sure I get this — looks like you haven’t replied to the comments

david-allison

Hmm... This should have submitted them

david-allison · 2025-09-18T18:58:17Z

AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt


        fun onFocusedDeckChanged(deckId: DeckId?) {
-            val position = deckId?.let { findDeckPosition(it) } ?: 0
+            val position = deckId?.let { viewModel.findDeckPosition(it, deckListAdapter.currentList) } ?: 0


Ideally the viewModel should be the source of truth for the collection of decks

david-allison · 2025-09-18T19:00:35Z

AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt

        // This code should not be reachable with lower versions
        val shortcut =
            ShortcutInfoCompat
-                .Builder(this, did.toString())


Add a unit test in libanki on DeckId.toString being correct.

It's subject to being broken if we move to a value class

david-allison · 2025-09-18T19:01:46Z

AnkiDroid/src/main/java/com/ichi2/anki/deckpicker/DeckPickerViewModel.kt

+    /**
+     * Gets the current name of a deck
+     */
+    suspend fun getDeckName(deckId: DeckId): String = withCol { decks.name(deckId) }


Todo to remove this or make it private, fine for now

Probably remove, that's just a backend call.

david-allison · 2025-09-18T19:02:28Z

AnkiDroid/src/main/java/com/ichi2/anki/deckpicker/DeckPickerViewModel.kt

+    fun onSubDeckCreated() =
+        viewModelScope.launch {
+            flowOfSubDeckCreated.emit(Unit)
+            flowOfDeckCountsChanged.emit(Unit)


Todo: this should be using ChangeManager

david-allison · 2025-09-18T19:03:59Z

AnkiDroid/src/main/java/com/ichi2/anki/deckpicker/DeckPickerViewModel.kt

 fun DeckNode.onlyHasDefaultDeck() = children.singleOrNull()?.did == DEFAULT_DECK_ID
+
+/** Data for creating a deck shortcut */
+data class ShortcutData(


Maybe comment an example of the long/short values

AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt

lukstbit · 2025-09-22T08:42:36Z

AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt

+        val isEmpty =
+            withCol {
+                val node = sched.deckDueTree().find(did)
+                node?.all { decks.isEmpty(it.did) } ?: true
+            }


Suggested change

val isEmpty =

withCol {

val node = sched.deckDueTree().find(did)

node?.all { decks.isEmpty(it.did) } ?: true

}

val isEmpty = withCol { decks.cardCount(did, includeSubdecks = true)

lukstbit · 2025-09-22T08:45:51Z

AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt

-        val deck = getNodeByDid(did)
-        if (deck.hasCardsReadyToStudy()) {
+
+        val deck = withCol { sched.deckDueTree().find(did) }


Suggested change

val deck = withCol { sched.deckDueTree().find(did) }

val deck = withCol { decks.getLegacy(did) }

I noticed that decks.getLegacy(did) returns a Deck object, which doesn’t provide the hasCardsReadyToStudy() method. On the other hand, sched.deckDueTree().find(did) returns a DeckNode, and this one does have hasCardsReadyToStudy().

Because of this, I think your suggestion may not fully apply in this context — we specifically need the hasCardsReadyToStudy() functionality, which is only available via DeckNode

lukstbit · 2025-09-22T08:47:47Z

AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt

        }

-        if (!deck.filtered && isDeckAndSubdeckEmpty(did)) {
+        val isFiltered = withCol { decks.isFiltered(did) }


We already have the deck instance from above and we can use its isFiltered property, so there's no need for this backend cal:

deck?.isFiltered.

lukstbit · 2025-09-22T08:50:11Z

AnkiDroid/src/test/java/com/ichi2/anki/DeckPickerTest.kt

+                viewModel.flowOfCreateShortcut.test {
+                    supportFragmentManager.selectContextMenuOption(DeckPickerContextMenuOption.CREATE_SHORTCUT, didA)
+                    advanceUntilIdle()
+                    awaitItem()
+                }


My comment was about the two lines below:

Suggested change

viewModel.flowOfCreateShortcut.test {

supportFragmentManager.selectContextMenuOption(DeckPickerContextMenuOption.CREATE_SHORTCUT, didA)

advanceUntilIdle()

awaitItem()

}

supportFragmentManager.selectContextMenuOption(DeckPickerContextMenuOption.CREATE_SHORTCUT, didA)

advanceUntilIdle()

being enough for the test to pass. If we require test {} then there's something wrong in the code.

lukstbit · 2025-09-22T09:00:12Z

AnkiDroid/src/main/java/com/ichi2/anki/deckpicker/DeckPickerViewModel.kt

                hasSubDecks = tree.children.any { it.children.any() },
            )
-        }
+        }.stateIn(viewModelScope, SharingStarted.Eagerly, initialValue = FlattenedDeckList.empty)


Why stateIn?

Using stateIn with SharingStarted.Eagerly is doing a few things:

Keeps state – Any new UI part (like a screen) directly gets the latest deck list without waiting.

Shares result – If many parts of the UI are watching, they all use the same result instead of redoing the work.

But yes, Eagerly can be wasteful since it starts the flow even if no UI is looking at it.

Other choices:

Use WhileSubscribed() so it runs only when some UI is observing.

Remove stateIn if normally only one UI part collects it.

Keep Eagerly if calculation is heavy and we want to keep it cached always.

I would prefer WhileSubscribed() unless we really need eager caching.

- Move 8 deck-related functions from DeckPicker to DeckPickerViewModel - Add reactive flows for UI communication (export, shortcuts, subdeck creation) - Update DeckPicker to use ViewModel methods instead of direct collection access

sanjaysargam added the Needs Review label Sep 16, 2025

sanjaysargam force-pushed the deck-view-model branch from 60bf12c to 11bd7f9 Compare September 17, 2025 05:54

lukstbit added the Needs Author Reply Waiting for a reply from the original author label Sep 17, 2025

sanjaysargam force-pushed the deck-view-model branch from 11bd7f9 to b629a0b Compare September 18, 2025 18:36

david-allison removed the Needs Author Reply Waiting for a reply from the original author label Sep 18, 2025

lukstbit requested changes Sep 21, 2025

View reviewed changes

david-allison reviewed Sep 21, 2025

View reviewed changes

sanjaysargam force-pushed the deck-view-model branch from b629a0b to 1040c94 Compare September 21, 2025 20:49

david-allison added the Needs Author Reply Waiting for a reply from the original author label Sep 22, 2025

lukstbit reviewed Sep 22, 2025

View reviewed changes

sanjaysargam force-pushed the deck-view-model branch from 1040c94 to 29b594c Compare September 22, 2025 16:53

sanjaysargam requested a review from lukstbit September 23, 2025 15:28

sanjaysargam removed the Needs Author Reply Waiting for a reply from the original author label Sep 24, 2025

github-actions bot added the Has Conflicts label Oct 3, 2025

sanjaysargam force-pushed the deck-view-model branch from 29b594c to b943542 Compare October 23, 2025 04:08

sanjaysargam removed the Has Conflicts label Oct 23, 2025

github-actions bot added the Has Conflicts label Oct 29, 2025

sanjaysargam force-pushed the deck-view-model branch from b943542 to 08ed8b9 Compare November 5, 2025 16:40

sanjaysargam force-pushed the deck-view-model branch from 08ed8b9 to e755821 Compare November 5, 2025 17:52

	val deck = withCol { sched.deckDueTree().find(did) }
	val deck = withCol { decks.getLegacy(did) }

Uh oh!

Refactor: Move deck operations business logic from DeckPicker to DeckPickerViewModel #19245

Are you sure you want to change the base?

Refactor: Move deck operations business logic from DeckPicker to DeckPickerViewModel #19245

Conversation

sanjaysargam commented Sep 16, 2025

Purpose / Description

Fixes

Approach

How Has This Been Tested?

Checklist

Uh oh!

sanjaysargam commented Sep 16, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

david-allison commented Sep 21, 2025

Uh oh!

sanjaysargam commented Sep 21, 2025

Uh oh!

david-allison left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants