feat: add card flags to CardContentProvider and api#21322
feat: add card flags to CardContentProvider and api#21322ColgateTotal77 wants to merge 2 commits into
Conversation
|
First PR! 🚀 We sincerely appreciate that you have taken the time to propose a change to AnkiDroid! Please have patience with us as we are all volunteers - we will get to this as soon as possible. |
sanjaysargam
left a comment
sanjaysargam
left a comment
There was a problem hiding this comment.
Welcome to the community @ColgateTotal77
Thanks for contributing 🚀
| } | ||
|
|
||
| Timber.d("CardContentProvider: flags update...") | ||
| currentCard.flags = flags |
There was a problem hiding this comment.
It overwrites the whole flags field, but only the last 3 bits are meant to be the color, the rest can hold other data.
Please use the existing helper instead, which only touches those 3 bits:
currentCard.setUserFlag(flags)And also I'd suggest a test that verifies setFlagRaw() preserves any non-flag bits already present in flags, which would also serve as a regression test
sanjaysargam
added
Needs Author Reply
…r.kt and update testSetFlagCard
criticalAY
left a comment
criticalAY
left a comment
There was a problem hiding this comment.
Really strong first PR, thank you!
| FlashCardsContract.Card.FSRS_DESIRED_RETENTION -> rb.add(currentCard.fsrsDesiredRetention) | ||
| FlashCardsContract.Card.FSRS_DECAY -> rb.add(currentCard.decay) | ||
| FlashCardsContract.Card.LAST_REVIEW_TIME_SECONDS -> rb.add(currentCard.lastReviewTimeSecs) | ||
| FlashCardsContract.Card.FLAGS -> rb.add(currentCard.flags) |
There was a problem hiding this comment.
With card.flags = 8 + flag 4, this query returns 12 but two things break:
- The FLAGS contract KDoc says reads are "in the range from 0 to 7", and 12 isn't.
- Round-trip fails: a client that reads 12 and calls setFlagRaw(12) hits "must be in the range from 0 to 7" we hand back a value our own writer rejects.
There was a problem hiding this comment.
If you are asking about test (assertEquals(12, it.getInt(it.getColumnIndex(FlashCardsContract.Card.FLAGS)))), idea to test setUserFlag (that this function affects only first 3 bits) was suggested by @sanjaysargam. So I set 8 (1000) manually, and then set 4 (100) with setUserFlag (via contentResolver.update), so if we override only first 3 bits, we won't affect fourth, so it will be 12 (1100).
Maybe I should move it to separate test and add comments with explanations?
| * | ||
| * param code The code of the flag. | ||
| */ | ||
| public enum class Flag( |
There was a problem hiding this comment.
nit: nothing enforces the "keep in sync with com.ichi2.anki.Flag" comment. A tiny test asserting the code values match would catch drift later. Optional.
| val tooLargeValue = | ||
| ContentValues().apply { | ||
| put(FlashCardsContract.Card.FLAGS, 8) | ||
| } | ||
| val exception2 = assertThrows<IllegalArgumentException> { contentResolver.update(noteCardUri, tooLargeValue, null, null) } | ||
| assertEquals(exception2.message, "Flags value must be in the range from 0 to 7") |
There was a problem hiding this comment.
@david-allison the forward-compat check you asked for isn't quite this: the 0–7 bound is hardcoded in the provider, so this keeps passing even after a backend bump. Deriving the max from the Flag enum would make it a real canary. Do you want that in this PR or a follow-up?
3 participants
Purpose / Description
Expand CardContentProvider and api to work with card flags.
Fixes
Approach
Added read/write for card flags in CardContentProvider and api.
How Has This Been Tested?
Implemented test for negative, too large, and valid flag codes.
Learning (optional, can help others)
I just looked at other api functions and content provider cases. Most of the code I just copied and a little tweaked for my case.
Checklist