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.

Sign up for GitHub

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

Jump to bottom

NF: Use com.ichi2.anki.Ease as much as possible #17430

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

NF: Use com.ichi2.anki.Ease as much as possible #17430

wants to merge 1 commit into from
+240 −245

Conversation

Arthur-Milchior
Copy link
Member

This allows to remove else in when case, and generally improve typing. Also, allows to remove some constant that were duplicate of the enum entries.

@Arthur-Milchior Arthur-Milchior mentioned this pull request Nov 12, 2024
Open
david-allison
david-allison requested changes Nov 13, 2024
View reviewed changes
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.

Implementer's choice on most of these. Thanks so much for splitting them up!

AnkiDroid/src/main/java/com/ichi2/anki/Ease.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/Ease.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/provider/CardContentProvider.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/reviewer/EaseButton.kt Show resolved Hide resolved
AnkiDroid/src/test/java/com/ichi2/anki/ReviewerKeyboardInputTest.kt Outdated Show resolved Hide resolved
api/src/main/java/com/ichi2/anki/api/Ease.kt Outdated Show resolved Hide resolved
api/src/main/java/com/ichi2/anki/FlashCardsContract.kt Outdated Show resolved Hide resolved
api/src/main/java/com/ichi2/anki/FlashCardsContract.kt Outdated Show resolved Hide resolved
@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Nov 13, 2024
david-allison
david-allison requested changes Nov 14, 2024
View reviewed changes
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.

Marking as 'second approval' with trust that the API documentation issues will be resolved

Minor concern we're moving away from upstream with the tests, but most of which will be in Rust by now.

AnkiDroid/src/main/java/com/ichi2/anki/AbstractFlashcardViewer.kt Outdated Show resolved Hide resolved
AnkiDroid/src/test/java/com/ichi2/anki/ReviewerTest.kt Outdated Show resolved Hide resolved
AnkiDroid/src/test/java/com/ichi2/libanki/SchedulerTest.kt Outdated Show resolved Hide resolved
AnkiDroid/src/test/java/com/ichi2/libanki/SchedulerTest.kt Outdated Show resolved Hide resolved
api/src/main/java/com/ichi2/anki/api/Ease.kt Outdated Show resolved Hide resolved
@Arthur-Milchior Arthur-Milchior removed the Needs Author Reply Waiting for a reply from the original author label Nov 14, 2024
@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Nov 14, 2024
@david-allison
Copy link
Member 

/home/runner/work/Anki-Android/Anki-Android/api/src/main/java/com/ichi2/anki/api/Ease.kt:22: Error: This block comment looks like it was intended to be a KDoc comment [WrongCommentType]
 * @param value The so called value of the button. For the sake of consistency with upstream and our API
   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

BrayanDSO
BrayanDSO requested changes Nov 14, 2024
View reviewed changes
AnkiDroid/src/main/java/com/ichi2/anki/Ease.kt Outdated
*The function associating to [value] the Ease whose value is [value].
* E.g. it sends 1 to [Again] and 4 to [Easy]
*/
fun fromValue(value: Int) = Ease.entries[value - 1]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation assumes that the values in the enum are ordered. This is the case now, and tbh it is unlikely to change, but I prefer to avoid future human errors when possible.

Suggested change
fun fromValue(value: Int) = Ease.entries[value - 1]
fun fromValue(value: Int) = entries.first { it.value == value }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, this is guaranteed by the documentation. https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.enums/-enum-entries.html But fine. I doubt this is in the loop where it would be an issue to iterate over four elements

AnkiDroid/src/main/java/com/ichi2/anki/Ease.kt Outdated
Comment on lines 31 to 34
/**
*The function associating to [value] the Ease whose value is [value].
* E.g. it sends 1 to [Again] and 4 to [Easy]
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the name of the enum property and the name of this method, I think that it's quite intuitive to know its purpose and anything different from that would be unexpected IMO, so I don't think that this comment is useful.

@Arthur-Milchior
NF: Use com.ichi2.anki.Ease as much as possible
4221eb5 
This allo to remove else in `when` case, and generally improve
typing. Also, allows to remove some constant that were duplicate of
the enum entries.
@BrayanDSO BrayanDSO removed the Needs Author Reply Waiting for a reply from the original author label Nov 15, 2024
@BrayanDSO BrayanDSO added the Needs Second Approval Has one approval, one more approval to merge label Nov 15, 2024
david-allison
david-allison requested changes Nov 15, 2024
View reviewed changes
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.

One architectural request

AnkiDroid/src/main/java/com/ichi2/anki/Ease.kt
Comment on lines +30 to +32
companion object {
fun Int.toEase() = entries.first { this == it.value }
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ease.fromValue (or similar).

Adding global extensions to Int gets messy quickly.

AnkiDroid/src/main/java/com/ichi2/anki/provider/CardContentProvider.kt
var timeTaken: Long = -1
var bury = -1
var suspend = -1
for ((key) in valueSet) {
when (key) {
FlashCardsContract.ReviewInfo.NOTE_ID -> noteID = values.getAsLong(key)
FlashCardsContract.ReviewInfo.CARD_ORD -> cardOrd = values.getAsInteger(key)
FlashCardsContract.ReviewInfo.EASE -> ease = values.getAsInteger(key)
FlashCardsContract.ReviewInfo.EASE -> {
// The value corresponds to
Copy link
Member

@david-allison david-allison Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sentence fragment

@lukstbit lukstbit added the Needs Author Reply Waiting for a reply from the original author label Nov 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@david-allison david-allison david-allison requested changes

@BrayanDSO BrayanDSO BrayanDSO approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Needs Author Reply Waiting for a reply from the original author Needs Second Approval Has one approval, one more approval to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Arthur-Milchior @david-allison @BrayanDSO @lukstbit