Refactor: study options ViewModel migration#20720
Refactor: study options ViewModel migration#20720criticalAY wants to merge 2 commits intoankidroid:mainfrom
Conversation
|
Can we defer this until 2.25? Lots of risk just before we go to beta |
david-allison
added
the
Next version
david-allison
left a comment
david-allison
left a comment
There was a problem hiding this comment.
The first commit introduces code without moving it, making it difficult for 'git blame' to work effectively.
The tests should ideally be introduced in the commit which introduces them in the ViewModel.
- Extract business logic from StudyOptionsFragment into a dedicated ViewModel. - Introduces DeckStudyData, StudyOptionsState sealed interface, and moves collection queries and deck operations (rebuild, empty, unbury) into the ViewModel. - refactor(study-options): wire StudyOptionsFragment to use ViewModel Replace direct collection access and mutable state in the fragment with ViewModel delegation. The fragment now observes StudyOptionsState via StateFlow and delegates operations (refresh, rebuild, empty, unbury) to StudyOptionsViewModel. Removes currentContentView, updateValuesFromDeckJob, retryMenuRefreshJob, and the col property getter.
criticalAY
force-pushed
the
refactor/study-options-viewmodel
branch
from
d08ebb4 to
e885ed9
Compare
criticalAY
removed
the
Next version
criticalAY
added
Next version
david-allison
left a comment
david-allison
left a comment
There was a problem hiding this comment.
Comments were removed
I'd combine the tests and the implementation, but implementer's choice
Logic looks solid. Much less hairy than I expected
| viewLifecycleOwner.lifecycleScope.launch { | ||
| viewLifecycleOwner.repeatOnLifecycle(Lifecycle.State.STARTED) { | ||
| viewModel.state.collect { state -> | ||
| rebuildUi(state) | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider launchCollectionInLifecycleScope
| a.setResult(result) | ||
| a.finish() | ||
| } else if (a == null) { | ||
| // getActivity() can return null if reference to fragment lingers after parent activity has been closed, |
There was a problem hiding this comment.
Why were comments removed in this method?
| val state: StateFlow<StudyOptionsState> | ||
| field = MutableStateFlow<StudyOptionsState>(StudyOptionsState.Loading) | ||
|
|
||
| val isFilteredDeck: StateFlow<Boolean> | ||
| field = MutableStateFlow(false) | ||
|
|
||
| val haveBuried: StateFlow<Boolean> | ||
| field = MutableStateFlow(false) | ||
|
|
||
| val selectedDeckId: StateFlow<Long> | ||
| field = MutableStateFlow(0L) |
There was a problem hiding this comment.
Consider renaming and exposing some properties as both the flow and the value. Using viewModel.selectedDeckId would simplify some calling code
| val data = | ||
| when (state) { | ||
| is StudyOptionsState.StudyOptions -> state.data | ||
| is StudyOptionsState.Congrats -> state.data | ||
| is StudyOptionsState.Empty -> state.data | ||
| is StudyOptionsState.Loading -> return | ||
| } | ||
|
|
||
| newCountText.text = data.newCardsToday.toString() | ||
| learningCountText.text = data.lrnCardsToday.toString() | ||
| reviewCountText.text = data.revCardsToday.toString() |
There was a problem hiding this comment.
can you improve this? val data = state.dataOrNull() ?: return comes to mind
| @@ -492,67 +416,30 @@ class StudyOptionsFragment : | |||
| count, | |||
| count, | |||
|
|
||
| fun refreshData(): Job = | ||
| viewModelScope.launch { | ||
| if (!CollectionManager.isOpenUnsafe()) return@launch |
There was a problem hiding this comment.
Make a note that this can be shown alongside the DeckPicker, where it's possible the collection is unusable
david-allison
added
Needs Author Reply
2 participants
Purpose / Description
Migrate the StudyOptionsFragment to use VM
Fixes
NA
Approach
See commits
How Has This Been Tested?
Manual test on Pixel 10 and unit test
Learning (optional, can help others)
NA
Checklist
Please, go through these checks before submitting the PR.