refactor(reminders): split up AlarmManagerService#21271
Open
ericli3690 wants to merge 2 commits into
Open
Conversation
ericli3690
added
the
Blocked by dependency
3 tasks
ericli3690
force-pushed
the
ericli3690-alarm-management
branch
from
bcf19b4 to
0f9917a
Compare
For some reason, my past self thought it was a good idea to pass around ReviewReminders as a Parcelable extra within an intent. This violates the single-source-of-truth nature of the stored review reminders, as when a notification tries to fire, it passes in a ReviewReminder that might be stale and has been updated on disk since the intent was created. There's no bugs caused by this, as editing review reminders via the UI deletes all pending intents and recreates them, but it's still bad design. I've refactored both the notification path and the snooze path to only pass around review reminder IDs in intent extras and to refer to the database when actually retrieving review reminder details.
AlarmManagerService has acquired three responsibilities: 1. It is a BroadcastReceiver because it listens to snooze intents 2. It has the core functionality for scheduling review reminder notifications 3. It is supposed to be the centralized point for handling AlarmManager in AnkiDroid This is too much responsibility for one file. This commit breaks it up into SnoozeService, ReviewReminderAlarmManager, and AlarmManagement respectively for the three responsibilities above. Additionally, I found a lot of inconsistent logic scattered throughout the codebase for dealing with AlarmManager. I've centralized it all in AlarmManagement. In the past, I found that AlarmManagerService, and specifically its logic relating to snoozing, were the hardest parts of the review reminders code to understand. I hope to improve on this through this change and an incoming documentation PR. Assisted-by: Claude Sonnet 4.6 (class splitting and unit test splitting grunt work)
ericli3690
force-pushed
the
ericli3690-alarm-management
branch
from
0f9917a to
db8a17b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Warning
Note
Assisted-by: Claude Sonnet 4.6 (class splitting and unit test splitting grunt work)
Purpose / Description
AlarmManagerService has acquired three responsibilities:
This is too much responsibility for one file. This PR breaks it up into SnoozeService, ReviewReminderAlarmManager, and AlarmManagement respectively for the three responsibilities above.
Additionally, I found a lot of inconsistent logic scattered throughout the codebase for dealing with AlarmManager. I've centralized it all in AlarmManagement.
In the past, I found that AlarmManagerService, and specifically its logic relating to snoozing, were the hardest parts of the review reminders code to understand. I hope to improve on this through this PR and an incoming documentation PR.
Fixes
Approach
Simple split up.
How Has This Been Tested?
Learning
Checklist