-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
refactor(CardTemplateEditor): add ViewModel + State infrastructure #20103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
david-allison
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand what the abstraction will turn out to be.
Mostly seems fine, even if it's just starting to move the screen to a ViewModel
AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditorState.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditorState.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditorViewModel.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditorViewModel.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditorViewModel.kt
Outdated
Show resolved
Hide resolved
Introduce CardTemplateEditorViewModel and CardTemplateEditorState as the foundation for migrating CardTemplateEditor to use the Rust backend's atomic updateNotetype() API. Part 1 of migration, infrastructure only, no behavioral changes yet.
david-allison
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to block forward progress on nitpicks, especially as this change does not affect production functionality.
This should be the last 'change request' before an approve.
Thanks!!!
| * Transitions to the Loaded state. | ||
| * Called internally after data is successfully loaded. | ||
| */ | ||
| internal fun onLoadComplete() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VisibleForTesting
I'm concerned we're testing at the wrong level of abstraction. (this is non-blocking; It's hard when there are few 'public' methods; we should be testing these private methods through public methods).
I suspect VisibleForTesting will be removed later down the line and the tests should be reworked.
internal should ideally be private, as the 'view' classes are in the same module.
| viewModel.onLoadComplete() | ||
|
|
||
| val state = viewModel.state.value | ||
| assertTrue(state is CardTemplateEditorState.Loaded) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assertInstanceOf<CardTemplateEditorState.Loaded> should work and return the value
|
|
||
| @Test | ||
| fun `initial state is Loading`() { | ||
| val viewModel = createViewModel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider a withViewModel helper, or setting a field in the constructor.
createViewModel is called in each test
Purpose / Description/ Approach
This PR adds the foundational infrastructure without changing existing behavior:
CardTemplateEditorState: Immutable data class holding UI state (loading, current template ordinal, editor view, errors, messages)CardTemplateEditorViewModel: Manages state via StateFlow with thread-safe updatesCardTemplateEditorViewModelTest: Unit tests covering all state operationsThis is Part 1 of a multiPR migration:
PR 1 (this): Infrastructure (ViewModel + State) [Done]
PR 2: Wire ViewModel to UI
PR 3: Load notetype + Template manipulation in-memory
PR 4: Atomic save with updateNotetype()
PR 5: Remove legacy code
Links without closing (Part of)
Checklist
Please, go through these checks before submitting the PR.