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

refactor: Convert Sched to Kotlin #12301

Merged
merged 2 commits into from
Sep 4, 2022
Merged

refactor: Convert Sched to Kotlin #12301

merged 2 commits into from
Sep 4, 2022

Conversation

david-allison
Copy link
Member

No description provided.

@david-allison
refactor: Rename Sched.java to .kt
a69211f 
com.ichi2.libanki.sched.Sched
lukstbit
lukstbit requested changes Sep 3, 2022
View reviewed changes
Copy link
Member

@lukstbit lukstbit left a comment

Choose a reason for hiding this comment

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

Looks good, just some minor cleanups.

AnkiDroid/src/main/java/com/ichi2/libanki/sched/Sched.kt Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/libanki/sched/Sched.kt Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/libanki/sched/Sched.kt Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/libanki/sched/Sched.kt Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/libanki/sched/Sched.kt Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/libanki/sched/Sched.kt Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/libanki/sched/Sched.kt Show resolved Hide resolved
@lukstbit lukstbit added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Sep 3, 2022
@Arthur-Milchior Arthur-Milchior mentioned this pull request Sep 4, 2022
Closed
Arthur-Milchior
Arthur-Milchior reviewed Sep 4, 2022
View reviewed changes
Copy link
Member

@Arthur-Milchior Arthur-Milchior left a comment

Choose a reason for hiding this comment

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

I probably missed some cleanup, this class is incredibly long. (And I plead guilty for part of it)
Still, I believe a bunch of cleanup that may improve code eventually

AnkiDroid/src/main/java/com/ichi2/libanki/sched/Sched.kt Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/libanki/sched/Sched.kt Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/libanki/sched/Sched.kt
if (!TextUtils.isEmpty(p)) {
Integer[] parentLims = lims.get(Decks.normalizeName(p));
val parentLims = lims[Decks.normalizeName(p)]
Copy link
Member

Choose a reason for hiding this comment

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

I'd consider putting the !! here, to make clear that it's directly non null

Copy link
Member Author

Choose a reason for hiding this comment

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

This breaks the Assert below

AnkiDroid/src/main/java/com/ichi2/libanki/sched/Sched.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/libanki/sched/Sched.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/libanki/sched/Sched.kt
List<Long> ids = getCol().findCards(search, orderLimit);
private fun _fillDyn(deck: Deck): List<Long> {
val terms = deck.getJSONArray("terms").getJSONArray(0)
var search = terms.getString(0)
Copy link
Member

Choose a reason for hiding this comment

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

KotlinCleanup: transform this into val

Copy link
Member Author

Choose a reason for hiding this comment

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

This breaks libAnki compat

AnkiDroid/src/main/java/com/ichi2/libanki/sched/Sched.kt Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/libanki/sched/Sched.kt Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/libanki/sched/Sched.kt
// update interval
card.setLastIvl(card.getIvl());
card.lastIvl = card.ivl
Copy link
Member

Choose a reason for hiding this comment

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

KotlinCleanup: Either card.apply or even define it as Card._rescheduleRev.
Same for other methods rescheduling cards

Copy link
Member Author

Choose a reason for hiding this comment

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

This breaks libAnki compat

AnkiDroid/src/main/java/com/ichi2/libanki/sched/Sched.kt Show resolved Hide resolved
@david-allison
refactor: Convert Sched to Kotlin
df4b952 
com.ichi2.libanki.sched.Sched
@dae
Copy link
Contributor

dae commented Sep 4, 2022

I don't recommend expending much effort on this file, since reviewing with the v1 scheduler is no longer possible since #11808

@david-allison
Copy link
Member Author

Agreed, just want to get to 0% Java so we can cut down the excess code and hopefully reduce compile times

@david-allison david-allison mentioned this pull request Sep 4, 2022
Merged
5 tasks
@Arthur-Milchior Arthur-Milchior merged commit 61ec185 into ankidroid:main Sep 4, 2022
@github-actions github-actions bot added this to the 2.16 release milestone Sep 4, 2022
@github-actions github-actions bot removed the Needs Second Approval Has one approval, one more approval to merge label Sep 4, 2022
@Arthur-Milchior
Copy link
Member

Merged. If we don't care about cleanup because the code will disappear, no need to wait for the cleanup

@david-allison david-allison deleted the kotlin-sched branch September 4, 2022 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Arthur-Milchior Arthur-Milchior Arthur-Milchior left review comments

@lukstbit lukstbit lukstbit approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.16 release
Development

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