Fix cloze number when existing clozes are present#20398
Fix cloze number when existing clozes are present#20398Alok-Silswal wants to merge 4 commits intoankidroid:mainfrom
Conversation
david-allison
left a comment
There was a problem hiding this comment.
Could you add a unit test?
In |
|
Yep |
|
Done! |
AnkiDroid/src/main/java/com/ichi2/anki/instantnoteeditor/InstantEditorViewModel.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/instantnoteeditor/InstantEditorViewModel.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/instantnoteeditor/InstantEditorViewModel.kt
Outdated
Show resolved
Hide resolved
|
minor guidance: Could you spend a little more time on the self-review phase of submissions, it looks like a lot of code was changed unnecessarily If I'm wrong, could you add tests to showcase the bugs you're fixing The test you added was great! Failed on |
Actually, this needs 3 different unit test cases (one already committed) as mentioned by me in #20350 (comment) i.e.
|
|
@david-allison On deeper inspection, I found that there is no unit case to verify the cloze counter adjustment when switching from Shall I add that unit test case in this PR only, or open a separate Issue for that? I have the function ready for that. |
|
When you're bugfixing, keep the commit as lean as possible:
If you notice more tests could be added, feel free to either add them as a new commit, or raise a separate PR. No issue necessary |
Purpose / Description
Instant Note was not displaying cloze number properly with cloze text.
Fixes
Approach
syncClozeState(text)function that makes cloze counter consistent with actual text to reflect correct cloze value by scanning text.How Has This Been Tested?
Verified that existing unit tests pass (
InstantEditorViewModelTest.kt).Tested on my phone (Android 14, Xiaomi / HyperOS) using a debug build.
Used the following two texts for testing:
Two screen recording attached as evidence.
Checklist
Please, go through these checks before submitting the PR.
Cloze_fix_evidence_1.mp4
Cloze_fix_evidence_2.mp4