Skip to content

refactor(notetypeFieldEditor): NotetypeFieldEditor ViewModel#20480

Closed
S-H-Y-A wants to merge 6 commits intoankidroid:mainfrom
S-H-Y-A:notetypeFieldEditor_viewModel
Closed

refactor(notetypeFieldEditor): NotetypeFieldEditor ViewModel#20480
S-H-Y-A wants to merge 6 commits intoankidroid:mainfrom
S-H-Y-A:notetypeFieldEditor_viewModel

Conversation

@S-H-Y-A
Copy link
Contributor

@S-H-Y-A S-H-Y-A commented Mar 15, 2026

Purpose / Description

This commit brings ViewModel to NotetypeFieldEditor, including no UI change.
This is the first one of the split PRs of #20453.

Approach

  • Moved NotetypeFieldEditor.kt to group it and other new files.
  • Moved UI states and the operations of editing collection to ViewModel.
  • Extracted edit dialogs to from NotetypeFieldEditor.kt, to clarify these dialogs' roles.

How Has This Been Tested?

Physical device
Pixel 7a
Android 16

Learning (optional, can help others)

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

@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Mar 15, 2026
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Please remove the merge commit from the history and fix the lint errors

@S-H-Y-A S-H-Y-A force-pushed the notetypeFieldEditor_viewModel branch from f902c97 to df487ef Compare March 15, 2026 14:33
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Is there any way you can rebase this to make it easier to review?

I'm looking through 8d21a89 and most of the changes feel unnecessary/incorrect

for example:

- AnkiActivity(R.layout.note_type_field_editor) {
+ com.ichi2.anki.AnkiActivity(_root_ide_package_.com.ichi2.anki.R.layout.note_type_field_editor) {

@S-H-Y-A S-H-Y-A force-pushed the notetypeFieldEditor_viewModel branch 2 times, most recently from 4ea4517 to c68f093 Compare March 15, 2026 17:47
@S-H-Y-A
Copy link
Contributor Author

S-H-Y-A commented Mar 15, 2026

Thank you for quick look.
I rebased the branch and fixed as many wrong/inappropriate changes as I could recognize.
If there are any additional required changes, please tell me those.

@S-H-Y-A S-H-Y-A requested a review from david-allison March 15, 2026 18:08
noteFields = notetype.fields
fieldsLabels = notetype.fieldsNames
binding.fields.adapter = NoteFieldAdapter(this, fieldNamesWithKind())
lifecycleScope.launch {
Copy link
Member

Choose a reason for hiding this comment

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

launchCollectionInLifecycleScope

@@ -0,0 +1,148 @@
/*
* Copyright (c) 2024 Neel Doshi <neeldoshi147@gmail.com>
Copy link
Member

Choose a reason for hiding this comment

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

Your copyright as well

Comment on lines +271 to +272
viewModel.state.value.noteFields[viewModel.state.value.currentPos]
.name
Copy link
Member

Choose a reason for hiding this comment

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

This is verbose.

The use of currentPos is questionable, but why not define this as viewModel.currentField.name

val noteFields: Fields = notetype.fields,
val fieldsLabels: List<String> = notetype.fieldsNames,
val isLoading: Boolean = false,
)
Copy link
Member

Choose a reason for hiding this comment

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

Most of these properties are derived properties which are unchanged, and therefore shouldn't be in the constructor.

@@ -0,0 +1,29 @@
/*
* Copyright (c) 2024 Neel Doshi <neeldoshi147@gmail.com>
Copy link
Member

Choose a reason for hiding this comment

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

Not Neel's

) : ViewModel() {
private val _state by lazy {
val noteTypeID = savedStateHandle.get<Long>(EXTRA_NOTETYPE_ID) ?: 0
val notetype = getColUnsafe().notetypes.get(noteTypeID)!!
Copy link
Member

Choose a reason for hiding this comment

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

Don't use getColUnsafe() in ViewModels

class NoteTypeFieldEditorViewModel(
private val savedStateHandle: SavedStateHandle,
) : ViewModel() {
private val _state by lazy {
Copy link
Member

Choose a reason for hiding this comment

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

I find it's OK to expose MutableStateFlow publicly

Now we're on Kotlin 2.3, we can specify a different type for the public view of the type

Copy link
Contributor Author

@S-H-Y-A S-H-Y-A Mar 16, 2026

Choose a reason for hiding this comment

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

According to the lint, "-Xexplicit-backing-fields" need to use Explicit Backing Fields.
The lint failed due to it.
(I oppressed it on my local machine when coding.)

import kotlinx.coroutines.launch
import timber.log.Timber

class NoteTypeFieldEditorViewModel(
Copy link
Member

Choose a reason for hiding this comment

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

Please add tests

@S-H-Y-A S-H-Y-A force-pushed the notetypeFieldEditor_viewModel branch from 1ed1160 to a2fefbf Compare March 16, 2026 02:56
@S-H-Y-A
Copy link
Contributor Author

S-H-Y-A commented Mar 16, 2026

I have been working on #20453 this week.
As I mentioned in #20453, I spent my vacation on these commits, but it has become clear that I won't be able to finish them before my vacation ends.
I believe it's better to close this PR for now rather than leaving it in an unfinished state. I will suspend work on #20453, too. I know it's selfish, but forgive me; I'm staying on as a user because I love this app.

@david-allison
Copy link
Member

david-allison commented Mar 16, 2026

That's not a problem. We'll get it in. Thanks so much for your time!

Do get in touch before a large change next time, and we can speed things along

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Author Reply Waiting for a reply from the original author Needs Review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants