Skip to content
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

fix: cloze_numbers_in_string treating {{c0::}} as valid #3059

Merged
merged 2 commits into from
Mar 12, 2024
Merged

fix: cloze_numbers_in_string treating {{c0::}} as valid #3059

merged 2 commits into from
Mar 12, 2024

Conversation

BrayanDSO
Copy link
Contributor

@BrayanDSO BrayanDSO commented Mar 7, 2024

cloze_numbers_in_string and consequently cloze_numbers_in_note returned the zero of {{c0::}}, although they shouldn't

@dae
Copy link
Member

dae commented Mar 8, 2024

@BrayanDSO mind elaborating on what issue the current code was causing?

@abdnh Will this change cause problems for the text annotations in I/O?

@BrayanDSO
Copy link
Contributor Author

BrayanDSO commented Mar 8, 2024

@BrayanDSO mind elaborating on what issue the current code was causing?

Note.cloze_numbers_in_fields, which is an available method for Add-on creators, has 0 among the returned result if {{c0::}} is present in the card, although it shouldn't.

anki/pylib/anki/notes.py

Lines 137 to 138 in 6ac60f8

def cloze_numbers_in_fields(self) -> Sequence[int]:
return self.col._backend.cloze_numbers_in_note(self._to_backend_note())

Also, it is a current cause of an issue in AnkiDroid if {{c0::}} is present when a card is previewed because of the faulty return value of cloze_numbers_in_note

Can I just remove it from the returned result? Yes, but fixing the root cause is the appropriate solution IMO


also, I'm not sure if this will implicate in any problems with IO since I'm not very proefficient with Rust/Anki codebase

@dae
Copy link
Member

dae commented Mar 9, 2024

I'm not sure there's a "right" answer here - I suspect you could make a case for 0 being excluded here, or at a different layer instead. If it doesn't break anything, I don't have any objections. Will be interested to hear from @abdnh.

rslib/src/cloze.rs Outdated Show resolved Hide resolved
@abdnh
Copy link
Collaborator

abdnh commented Mar 11, 2024

@abdnh Will this change cause problems for the text annotations in I/O?

It won't cause problems.

@dae
Copy link
Member

dae commented Mar 12, 2024

Thank you both!

@dae dae merged commit 1bd6d28 into ankitects:main Mar 12, 2024
1 check passed
@BrayanDSO BrayanDSO deleted the cloze-fix branch March 12, 2024 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants