Skip to content

Comments

feat(reminders): AlarmManagerService and NotificationService#19118

Merged
mikehardy merged 5 commits intoankidroid:mainfrom
ericli3690:ericli3690-review-reminders-alarm-manager-and-notification-service
Nov 13, 2025
Merged

feat(reminders): AlarmManagerService and NotificationService#19118
mikehardy merged 5 commits intoankidroid:mainfrom
ericli3690:ericli3690-review-reminders-alarm-manager-and-notification-service

Conversation

@ericli3690
Copy link
Member

@ericli3690 ericli3690 commented Aug 21, 2025

Added logic for review reminder notifications being sent to the user. Alarms for sending notifications are created by AlarmManagerService, and the actual notifications themselves are fired by NotificationService.

Purpose / Description

scheduleReviewReminderNotification is the primary part of AlarmManagerService, setting the recurring notifications for a review reminder. unschedule and scheduleAll methods are also provided. Snoozing is handled by AlarmManagerService via scheduleSnoozedNotification. AlarmManagerService must be a BroadcastReceiver so that it can receive snoozing requests via onReceive from PendingIntents created by NotificationService. sendReviewReminderNotification in NotificationService does the bulk of the work for sending notifications, filling out content, etc. fireReviewReminderNotification is a separate method that handles the actual OS call to fire the notification itself. Added AlarmManagerService as a BroadcastReceiver to the AndroidManifest.xml file.

There's a method called catchAlarmManagerExceptions which was previously in BootService. It was added to solve some bugs. I've moved it to AlarmManagerService and kept it around to ensure no regressions occur.

Marked old functionality in NotificationService as legacy notification code.

I had to create a NotificationServiceAction sealed class to mark the kinds of notification requests the NotificationService gets. These different requests must have different actions set, otherwise they collide with each other and interfere. This can cause snoozes to cancel normal notifications, normal notifications to cancel snoozes, etc., hence why I added this sealed class.

You might wonder why NotificationService is a BroadcastReceiver. NotificationService must be a BroadcastReceiver because it needs to listen to PendingIntents triggered by AlarmManager alarms, which trigger the onReceive method.

Added unit tests for AlarmManagerService and NotificationService.

Added calls to set review reminder notifications on device boot-up and app start-up to AnkiDroidApp and BootService. Modifies ScheduleReminders so that it sets alarms for review reminders via AlarmManagerService.

Other small changes:

  • Renamed the inner field of ReviewReminderId to avoid confusing calls to reviewReminder.id.id.
  • Marked a useless magic number as legacy code.
  • Fixed retrievals of the NotificationManager system service to be more idiomatic.

UI

image

Fixes

  • GSoC 2025: Review Reminders

Approach

  • See above description.

How Has This Been Tested?

  • Works on a physical Samsung S23, API 34.
  • I can set and receive review reminders! They recur after a day, can be snoozed, etc. More alpha testing is likely necessary.
  • Restarting the phone automatically requeues review reminders without opening the app.
  • Snoozing review reminders works.
  • Clicking review reminders opens the corresponding deck for review.

Learning

It took a long time for me to figure out what parts of the codebase were legacy notification code and could be removed, and which parts I should keep and try to build on top of.

Checklist

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code

@github-actions
Copy link
Contributor

Important

Maintainers: This PR contains Strings changes

  1. Sync Translations before merging this PR and wait for the action to complete
  2. Review and merge the auto-generated PR in order to sync all user-submitted translations
  3. Sync Translations again and merge the PR so the huge automated string changes caused by merging this PR are by themselves and easy to review

@ericli3690 ericli3690 added Needs Review GSoC Pull requests authored by a Google Summer of Code participant [Candidate/Selected], for GSoC mentors Blocked by dependency Currently blocked by some other dependent / related change and removed Strings labels Aug 21, 2025
@ericli3690 ericli3690 force-pushed the ericli3690-review-reminders-alarm-manager-and-notification-service branch from 7c56727 to 75e41bb Compare August 21, 2025 05:01
@ericli3690 ericli3690 requested a review from Copilot August 21, 2025 05:03
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a complete review reminder notification system with AlarmManagerService for scheduling alarms and NotificationService for firing notifications. The implementation includes proper database schema migration, snoozing functionality, and comprehensive testing.

  • AlarmManagerService schedules recurring and snoozed review reminder notifications using AlarmManager
  • NotificationService sends actual notification with snooze buttons and handles card count validation
  • Database migration system implemented for handling schema changes in ReviewReminder

Reviewed Changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
libanki/src/main/java/com/ichi2/anki/libanki/sched/Scheduler.kt Adds allDecksCounts() method for getting total card counts across all decks
AnkiDroid/src/main/java/com/ichi2/anki/services/AlarmManagerService.kt New service for scheduling review reminder alarms using AlarmManager
AnkiDroid/src/main/java/com/ichi2/anki/services/NotificationService.kt Enhanced service for firing review reminder notifications with snooze functionality
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt Database migration system and deck existence validation
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminderMigrationSettings.kt Schema migration configuration and versioning system
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminder.kt Updated to implement ReviewReminderSchema interface with renamed ID field
AnkiDroid/src/main/java/com/ichi2/anki/services/BootService.kt Integrates review reminder scheduling on device boot
AnkiDroid/src/main/AndroidManifest.xml Registers AlarmManagerService as broadcast receiver

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@ericli3690 ericli3690 force-pushed the ericli3690-review-reminders-alarm-manager-and-notification-service branch from 75e41bb to dd789c2 Compare August 21, 2025 05:12
@ericli3690 ericli3690 force-pushed the ericli3690-review-reminders-alarm-manager-and-notification-service branch 2 times, most recently from 58d3247 to 3285df9 Compare August 27, 2025 01:48
@ericli3690
Copy link
Member Author

Rebased. Now it's only blocked by one PR: #19065

@ericli3690 ericli3690 added Blocked by dependency Currently blocked by some other dependent / related change and removed Blocked by dependency Currently blocked by some other dependent / related change Has Conflicts labels Aug 27, 2025
@ericli3690 ericli3690 force-pushed the ericli3690-review-reminders-alarm-manager-and-notification-service branch from 3285df9 to b47b008 Compare August 29, 2025 00:19
@ericli3690
Copy link
Member Author

Non-functional test file change to make NotificationServiceTest a little simpler.

@ericli3690 ericli3690 force-pushed the ericli3690-review-reminders-alarm-manager-and-notification-service branch 2 times, most recently from 6ca25d6 to 496aae3 Compare August 29, 2025 00:25
@ericli3690
Copy link
Member Author

Rebased on main.

@ericli3690 ericli3690 force-pushed the ericli3690-review-reminders-alarm-manager-and-notification-service branch 3 times, most recently from ce80716 to a57edf1 Compare September 7, 2025 17:14
@ericli3690
Copy link
Member Author

  • Fixed a bug where notifications would still be scheduled for review reminders for deleted decks. Pulled deck deletion logic out into a cleaner helper method, deleteReviewRemindersIfDeckDeleted.
  • Added tests for deleteReviewRemindersIfDeckDeleted.
  • Rebased.

@ericli3690 ericli3690 marked this pull request as draft September 26, 2025 04:12
@ericli3690 ericli3690 marked this pull request as draft October 27, 2025 15:29
GSoC 2025: Review Reminders

Renamed the inner field of ReviewReminderId to avoid confusing calls to `reviewReminder.id.id`.
GSoC 2025: Review Reminders

This magic number was used for the old notifications system. It's useless now.
David let me know about a better way to retrieve the NotificationManager system service. I realized I used the old method in this test file and I've now fixed it.
@ericli3690 ericli3690 force-pushed the ericli3690-review-reminders-alarm-manager-and-notification-service branch from 450c5c6 to fc2c0d5 Compare November 3, 2025 07:15
GSoC 2025: Review Reminders

Added logic for review reminder notifications being sent to the user. Alarms for sending notifications are created by AlarmManagerService, and the actual notifications themselves are fired by NotificationService. `scheduleReviewReminderNotification` is the primary part of AlarmManagerService, setting the recurring notifications for a review reminder. `unschedule` and `scheduleAll` methods are also provided. Snoozing is handled by AlarmManagerService via `scheduleSnoozedNotification`. AlarmManagerService must be a BroadcastReceiver so that it can receive snoozing requests via onReceive from PendingIntents created by NotificationService. `sendReviewReminderNotification` in NotificationService does the bulk of the work for sending notifications, filling out content, etc. `fireReviewReminderNotification` is a separate method that handles the actual OS call to fire the notification itself. Added AlarmManagerService as a BroadcastReceiver to the AndroidManifest.xml file.

There's a method called `catchAlarmManagerExceptions` which was previously in BootService. It was added to solve some bugs. I've moved it to AlarmManagerService and kept it around to ensure no regressions occur.

Marked old functionality in NotificationService as legacy notification code.

I had to create a NotificationServiceAction sealed class to mark the kinds of notification requests the NotificationService gets. These different requests must have different actions set, otherwise they collide with each other and interfere. This can cause snoozes to cancel normal notifications, normal notifications to cancel snoozes, etc., hence why I added this sealed class.

You might wonder why NotificationService is a BroadcastReceiver. NotificationService must be a BroadcastReceiver because it needs to listen to PendingIntents triggered by AlarmManager alarms, which trigger the onReceive method.

Added unit tests for AlarmManagerService and NotificationService.
GSoC 2025: Review Reminders

Added calls to set review reminder notifications on device boot-up and app start-up to AnkiDroidApp and BootService. Modifies ScheduleReminders so that it sets alarms for review reminders via AlarmManagerService.
@ericli3690 ericli3690 force-pushed the ericli3690-review-reminders-alarm-manager-and-notification-service branch from fc2c0d5 to 0e78342 Compare November 3, 2025 07:45
@ericli3690
Copy link
Member Author

ericli3690 commented Nov 3, 2025

@david-allison Review addressed!

If the collection is unopenable (potentially occurs if syncing)
Does this block the 'rescheduling' of notifications, potentially meaning a user could 'lose' a notification until a phon reboot?

After double-checking what I've written, I'm fairly sure this isn't possible. The only operation that opens the collection in this PR is the act of firing a review reminder itself, and I ensure that happens after the next recurring reminder is scheduled:

// Schedule the next instance of this review reminder notification if this is a recurring notification
if (action == NotificationServiceAction.ScheduleRecurringNotifications.actionString) {
    Timber.d("Scheduling next review reminder notification")
    AlarmManagerService.scheduleReviewReminderNotification(context, reviewReminder)
}

runGloballyWithTimeout(SEND_REVIEW_REMINDER_TIMEOUT) {
    // We run this on the global scope for simplicity's sake, as BroadcastReceivers do not have CoroutineScopes.
    // Theoretically we could also use an expedited Worker, but AnkiDroid is only allotted a fixed number
    // of expedited Worker calls per day, and these expedited calls are also used by the sync service,
    // so it's best to conserve them.
    try {
        sendReviewReminderNotification(context, reviewReminder)
    } catch (e: BackendException) {
        // This may occur if the collection is blocked, in which case we should fail gracefully
        Timber.w(e, "Aborted review reminder notification due to backend exception")
    }
}

Is there a process to recover from an error along these lines, or is there a guarantee that errors are handled correctly in the case of issues opening/querying the collection?

See the above code snippet. I've tried wrapping the act of sending a review reminder notification in a try-catch block. Am I correct to filter by BackendException? Again, the only thing I've written in this PR that accesses the collection is sendReviewReminderNotification. I was also considering your idea of recovering from this sort of scenario: theoretically, a sync shouldn't take too long, meaning that after a while, it should be safe to attempt notification firing again. Should I create some logic to retry the notification at most three or so times if a BackendException occurs? The alternative would be to just drop the notification for the day, which might be alarming for some users but is a straightforward failure case.

@ericli3690 ericli3690 marked this pull request as ready for review November 3, 2025 08:07
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.

Fantastic, thanks so much!

)
ReviewRemindersDatabase.editRemindersForDeck(did1) { mapOf(ReviewReminderId(0) to reviewReminder) }

CollectionManager.emulatedOpenFailure = CollectionManager.CollectionOpenFailure.LOCKED
Copy link
Member

Choose a reason for hiding this comment

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

just to confirm: I believe this throws on access?

If so, perfect! Thanks so much for the test

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, the access throws and the BackendException that results is caught silently by NotificationService. Thanks for the approval! Do we still want logic to retry the notification three or so times if a BackendException occurs, or is that unnecessary?

@david-allison david-allison added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Nov 7, 2025
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

This looks really good - so I'm sure this has been discussed, but the hard-coded strings for the reminder title and buttons - are those strings going to be extracted for translation or ?

@ericli3690
Copy link
Member Author

Thanks!! I think the original plan with the hard-coded strings was to wait until the feature was almost out the door into an alpha or beta before extracting them out, but if you'd like I can do the extraction now. Your call! If not, feel free to slap this guy into the merge queue -- I think everything's been addressed and if David wants notification retries upon BackendExceptions we can handle that in a follow-up.

@mikehardy
Copy link
Member

@david-allison should we do the strings now or wait? I understand while trialing ideas it doesn't make a lot of sense, but if we are set to merge this (and...I'm set to merge it at least...) then it seems like now is the time for strings to get set up for localization

@david-allison
Copy link
Member

david-allison commented Nov 13, 2025

I think: hold off for a little while longer

One of the strings contains "ETA", which we're likely to fully deprecate

One of the strings is using the deck name, which may be an issue:

  • Long/nested decks may be badly handled
  • How the notification would look, given the deck name isn't differentiated from our text
    • "It's time to study <expletive>"

@mikehardy
Copy link
Member

Okay :-), peeled that to a separate issue so I/we don't lose sight aaaaand 🥁 🥁 queued this for merge. Let's go!

Merged via the queue into ankidroid:main with commit 07e3c9c Nov 13, 2025
15 checks passed
@github-actions github-actions bot removed the Needs Second Approval Has one approval, one more approval to merge label Nov 13, 2025
@github-actions github-actions bot added this to the 2.23 release milestone Nov 13, 2025
@ericli3690
Copy link
Member Author

Thanks for the merge and for creating the new issue! I'm a bit swamped these few days but I'll get to patching up the advanced review reminder option PRs (which are unblocked now) and addressing everything in my inbox as soon as possible. Cheers

@mikehardy
Copy link
Member

@ericli3690 my pleasure - only can offer apologies it took this long. Just went through the notifications one as well so it's no longer blocked, I think it can go in pretty quickly - I'm excited to see this stuff merge :-). Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

GSoC Pull requests authored by a Google Summer of Code participant [Candidate/Selected], for GSoC mentors

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants