Skip to content

refactor(reminders): remove race conditions, simplify database interface #20755

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

Open
ericli3690 wants to merge 3 commits into
base: main
Choose a base branch
from
Open

refactor(reminders): remove race conditions, simplify database interface #20755

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
90690b7
fix(reminders): remove race conditions, clean up database
ericli3690 Apr 12, 2026
b626b0d
style: minor review reminder comments and reorganization
ericli3690 Apr 13, 2026
4011dd8
style: remove unnecessary companion imports
ericli3690 Apr 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidApp.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,8 @@ open class AnkiDroidApp :
var progressDialogShown = false

/**
* On application creation.
* On application creation, i.e. when the application process starts.
* This is called before any activities, services, or receivers are created.
*/
@KotlinCleanup("analytics can be moved to attachBaseContext()")
override fun onCreate() {
Expand Down Expand Up @@ -207,7 +208,9 @@ open class AnkiDroidApp :
val context = this.withAppLocale()
if (Prefs.newReviewRemindersEnabled) {
Timber.i("Setting review reminder notifications if they have not already been set")
AlarmManagerService.scheduleAllNotifications(context)
applicationScope.launch {
AlarmManagerService.scheduleAllNotifications(context)
}
} else {
// Register for notifications
Timber.i("AnkiDroidApp: Starting Services")
Expand Down
14 changes: 8 additions & 6 deletions AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminder.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,9 @@ sealed class ReviewReminderScope : Parcelable {
* reminders with invalid IDs are never created. This class is annotated
* with @ConsistentCopyVisibility to ensure copy() is private too and does not leak the constructor.
*
* Edits to instances of this class are not automatically persisted to SharedPreferences;
* that functionality is provided by [ReviewRemindersDatabase].
*
Comment thread
david-allison marked this conversation as resolved.
* About the old schema migration process:
*
* To any developer who changes this class in the future, note that these review reminders are stored
Expand Down Expand Up @@ -188,7 +191,7 @@ sealed class ReviewReminderScope : Parcelable {
* multiple profiles might be active simultaneously.
* @param latestNotifTime The time at which this review reminder last attempted to fire a routine daily (non-snooze)
* notification, in epoch milliseconds, or the time at which it was created if no notification has ever been fired.
* See [shouldImmediatelyFire].
* See [latestNotifDelivered].
* @param onlyNotifyIfNoReviews If true, only notify the user if this scope has not been reviewed today yet.
*/
@Serializable
Expand Down Expand Up @@ -239,11 +242,10 @@ data class ReviewReminder private constructor(
}

/**
* Checks if this review reminder has tried to fire a routine daily (non-snooze) notification in the time between
* its latest scheduled firing time and now. If not, this method returns true, indicating that a notification
* should be immediately fired for this review reminder.
* Checks if this review reminder has successfully attempted to deliver a routine daily (non-snooze)
* notification in the time between its latest scheduled firing time and now. If so, this method returns true.
*/
fun shouldImmediatelyFire(): Boolean {
fun latestNotifDelivered(): Boolean {
val (hour, minute) = this.time

val currentTimestamp = TimeManager.time.calendar()
Expand All @@ -257,7 +259,7 @@ data class ReviewReminder private constructor(
}
}

return latestNotifTime < latestScheduledTimestamp.timeInMillis
return latestNotifTime >= latestScheduledTimestamp.timeInMillis
}
Comment on lines 244 to 263
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

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

latestNotifDelivered() is documented as checking whether a notification was “successfully delivered”, but latestNotifTime is updated on attempt (see updateLatestNotifTime() and its call site in NotificationService.handleReviewReminderNotification()), including cases where sendReviewReminderNotification() exits early (deck not accessible / onlyNotifyIfNoReviews / threshold). This makes the name/doc misleading and can confuse future changes. Consider either updating the KDoc to reflect attempt/processing semantics, or renaming the method to match what it actually represents (e.g., “latestNotifAttempted” / “latestNotifTimeUpToDate”).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

@ericli3690 ericli3690 Apr 16, 2026

Choose a reason for hiding this comment

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

Modified the docstring for this file to make this more clear. Unfortunately it's a little too late to change this, as latestNotifTime was in the schema migration and is stored on-device, so changing this would not just be cosmetic, we'd have to write an entire new migration. That's probably not worth it.


/**
Expand Down
21 changes: 11 additions & 10 deletions AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminderGroup.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@ package com.ichi2.anki.reviewreminders
import kotlinx.serialization.SerializationException
import kotlinx.serialization.json.Json

/**
* Convenience typealias for the mutation functions passed to editors of [ReviewReminderGroup].
*/
typealias ReviewReminderGroupEditor = (ReviewReminderGroup) -> ReviewReminderGroup
Copy link
Copy Markdown
Member Author

@ericli3690 ericli3690 Apr 16, 2026

Choose a reason for hiding this comment

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

Moved to top of file as requested by David


/**
* A group of review reminders, all for the same [ReviewReminderScope],
* persisted as a single JSON string in SharedPreferences.
Expand All @@ -27,6 +32,9 @@ import kotlinx.serialization.json.Json
* explicitly defined to restrict what can be done with the interface and to eliminate the
* need to verbosely type out "HashMap<ReviewReminderId, ReviewReminder>" everywhere.
*
* Edits to instances of this class are not automatically persisted to SharedPreferences;
* that functionality is provided by [ReviewRemindersDatabase].
*
* A HashMap is used to allow for O(1) access to individual reminders by [ReviewReminderId].
*/
class ReviewReminderGroup(
Expand All @@ -39,9 +47,7 @@ class ReviewReminderGroup(
/**
* Manually construct a [ReviewReminderGroup] from key-value pairs.
*/
constructor(vararg pairs: Pair<ReviewReminderId, ReviewReminder>) : this(
buildMap { pairs.forEach { put(it.first, it.second) } },
)
constructor(vararg pairs: Pair<ReviewReminderId, ReviewReminder>) : this(hashMapOf(*pairs))
Comment thread
david-allison marked this conversation as resolved.

/**
* Merge multiple [ReviewReminderGroup]s into one.
Expand Down Expand Up @@ -87,8 +93,8 @@ class ReviewReminderGroup(
underlyingMap.remove(id)
}

fun forEach(action: (ReviewReminderId, ReviewReminder) -> Unit) {
underlyingMap.forEach { (id, reminder) -> action(id, reminder) }
fun forEach(action: (Pair<ReviewReminderId, ReviewReminder>) -> Unit) {
underlyingMap.forEach { (id, reminder) -> action(id to reminder) }
Comment thread
david-allison marked this conversation as resolved.
}

/**
Expand All @@ -112,8 +118,3 @@ class ReviewReminderGroup(
* Convenience extension constructor for merging a list of [ReviewReminderGroup]s into one.
*/
fun List<ReviewReminderGroup>.mergeAll() = ReviewReminderGroup(*this.toTypedArray())

/**
* Convenience typealias for the mutation functions passed to editors of [ReviewReminderGroup].
*/
typealias ReviewReminderGroupEditor = (ReviewReminderGroup) -> ReviewReminderGroup
3 changes: 3 additions & 0 deletions AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminderSchema.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,9 @@ data class ReviewReminderSchemaV2(
/**
* Schema migration settings for testing purposes.
* Consult this as an example of how to save old schemas and define their [ReviewReminderSchema.migrate] methods.
* Also see the unit tests for [ReviewRemindersDatabase], where these classes are exercised.
* These classes cannot be moved directly to the test file because [ReviewReminderSchema] is a sealed interface,
* meaning all implementations of it must be within the same module.
Comment thread
david-allison marked this conversation as resolved.
*/
object TestingReviewReminderMigrationSettings {
/**
Expand Down
Loading
Loading