Skip to content

Fix cloze number when existing clozes are present#20398

Open
Alok-Silswal wants to merge 4 commits intoankidroid:mainfrom
Alok-Silswal:fix-instant-cloze-sync-new
Open

Fix cloze number when existing clozes are present#20398
Alok-Silswal wants to merge 4 commits intoankidroid:mainfrom
Alok-Silswal:fix-instant-cloze-sync-new

Conversation

@Alok-Silswal
Copy link

@Alok-Silswal Alok-Silswal commented Mar 5, 2026

Purpose / Description

Instant Note was not displaying cloze number properly with cloze text.

Fixes

Approach

  • Cloze counter and intClozeList is derived from actual text.

How Has This Been Tested?

Used the following two texts for testing:

This is the {{c1::first cloze}}. This should be the second cloze.
This is the {{c1::first cloze}}. This should be the {{c2::second cloze}}. Similarly, this one as {{c3::third cloze}}.

Two screen recording attached as evidence.

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
Cloze_fix_evidence_1.mp4
Cloze_fix_evidence_2.mp4

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.

Could you add a unit test?

@Alok-Silswal
Copy link
Author

Could you add a unit test?

In InstantEditorViewModelTest.kt?

@david-allison
Copy link
Member

Yep

@Alok-Silswal
Copy link
Author

Alok-Silswal commented Mar 5, 2026

Done!
@david-allison Does it suffice?

@david-allison
Copy link
Member

david-allison commented Mar 6, 2026

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 main, passes on your branch

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

Alok-Silswal commented Mar 6, 2026

Could you add a unit test?

Actually, this needs 3 different unit test cases (one already committed) as mentioned by me in #20350 (comment) i.e.

On first load, the existing c1 is not detected, so the cloze counter starts from 1 again, and the next insertion reuses c1.

After toggling [...] +, the state refresh causes the existing cloze to be detected correctly, and further insertions increment as expected.


I tested this further with text that already has multiple clozes (c1, c2, c3).

Observations:

  • On first load, when I select a new word in [...] + mode, it does not continue from c4. It starts again from c1.
  • The next selections then go c2, c3, but this numbering is clearly independent of the existing clozes in the text.
  • After toggling [...] +, selecting another word still does not continue from the highest existing index. Instead, it starts from c2.

So the counter does not seem to be derived from the current clozes present in the text. It looks like it’s using some local counter that gets reset or out of sync.


@Alok-Silswal
Copy link
Author

Alok-Silswal commented Mar 6, 2026

@david-allison On deeper inspection, I found that there is no unit case to verify the cloze counter adjustment when switching from INCREMENT to NO_INCREMENT mode and vice-versa for cloze text.

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.

@david-allison
Copy link
Member

When you're bugfixing, keep the commit as lean as possible:

  • fix
  • test

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

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.

Cheers

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.

Adding additional clozes doesn't work properly for Instant Cards in AnkiDroid

2 participants