NF: rename model to note type #17598
Open
+1,376
−1,371
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As David mentioned recently, Anki don't use "model" anymore, it should not be in the codebase. There are two places where `model` should not be replaced. When the variable refers to a model that is not a note type (e.g. a viewModel, the Card Browser's model..). And when the word "model" appears in the API. Indeed we should not break the existing code using the API. While it may be considered to extend our API, duplicating the Model commands to introduce a NoteType command with the same meaning. And, maybe one day, deprecate the previous API. But I don't think there is even a need to deprecate the API, it does not really lead to confusion. On top of replacing "model", "models", "Model", "Models", "MODEL", "MODELS", we also rename the variables "m", "m2" and "mm" as those variable where abbreviations for "Model". I renamed some layout, menu, In one case, the documentation was wrong. It mentioned "model" instead of "template". I thus replaced "model" by "template" in order to correct the documentation. Note that all the changes were done manually. I used Android-Studio to rename function, variables, values and constant. But I generally avoided the "search and replace" feature which may have led to erroneous update. (Example of exception: `m` and `mm` in `modelTest`
david-allison
requested changes
Dec 14, 2024
There was a problem hiding this comment.
This is a huge change, could it be broken down to PRs in the ~300LOC level, so it's reasonable to review in a sitting?
- This contains breaking changes to our API
- Note: Renaming parameters in Kotlin is a breaking change
- Renaming an activity is a breaking change to our public API
addNoteUsingBasicModelmight as well beaddBasicNoteNoteTypeTypeis not a good name
| @@ -199,7 +199,7 @@ class ModelFieldEditor : | |||
| } | |||
| } | |||
| c.setConfirm(confirm) | |||
| this@ModelFieldEditor.showDialogFragment(c) | |||
| this@NoteTypeFieldEditor.showDialogFragment(c) | |||
There was a problem hiding this comment.
I believe the @ shouldn't be necessary at all
| @Suppress("SameParameterValue") front: String, | ||
| @Suppress("SameParameterValue") back: String, | ||
| ): Note = addNoteUsingModelName("Basic (type in the answer)", front, back) | ||
| ): Note = addNoteUsingNoteTypeName("Basic (type in the answer)", front, back) |
There was a problem hiding this comment.
addNoteOfType would seem sensible
lukstbit
added
the
Needs Author Reply
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As David mentioned recently, Anki don't use "model" anymore, it should not be in the codebase.
There are two places where
modelshould not be replaced. When the variable refers to a model that is not a note type (e.g. a viewModel, the Card Browser's model..). And when the word "model" appears in the API. Indeed we should not break the existing code using the API.While it may be considered to extend our API, duplicating the Model commands to introduce a NoteType command with the same meaning. And, maybe one day, deprecate the previous API. But I don't think there is even a need to deprecate the API, it does not really lead to confusion.
On top of replacing "model", "models", "Model", "Models", "MODEL", "MODELS", we also rename the variables "m", "m2" and "mm" as those variable where abbreviations for "Model". I renamed some layout, menu,
In one case, the documentation was wrong. It mentioned "model" instead of "template". I thus replaced "model" by "template" in order to correct the documentation.
Note that all the changes were done manually. I used Android-Studio to rename function, variables, values and constant. But I generally avoided the "search and replace" feature which may have led to erroneous update. (Example of exception:
mandmminmodelTest