-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat(reminders): handle deck deletion #19065
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.
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
feat(reminders): handle deck deletion #19065
Conversation
There was a problem hiding this 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 implements deck deletion handling for review reminders, ensuring that reminders associated with deleted decks are automatically cleaned up. The implementation uses a lazy deletion approach that checks deck existence when reminders are accessed rather than eagerly deleting upon deck removal.
Key changes include:
- Added lazy deck existence checking using the
decks.havemethod before returning deck-specific reminders - Implemented a
deleteAllRemindersForDeckhelper method for explicit cleanup - Converted database methods to suspend functions to support deck existence validation
- Enhanced test coverage with deck deletion scenarios
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| libanki/src/main/java/com/ichi2/anki/libanki/Decks.kt | Removed unused annotation from have method |
| AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabaseTest.kt | Updated tests to use runTest and added deck deletion test cases |
| AnkiDroid/src/test/java/com/ichi2/anki/reviewreminders/ReviewReminderMigrationSettingsTest.kt | Added schema version validation test |
| AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt | Added deck existence checks and deletion logic to database methods |
| AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminderMigrationSettings.kt | Added schema migration infrastructure |
| AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminder.kt | Made ReviewReminder implement ReviewReminderSchema interface |
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt
Outdated
Show resolved
Hide resolved
|
Ready for review! #18856 should be merged first before this is merged, though. This PR is based on top of that one. |
bd07c88 to
b0e76c4
Compare
|
b0e76c4 to
4fe2961
Compare
4fe2961 to
bffdf9f
Compare
|
Rebased on main. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks solid code-wise
What was the consensus on how multiple collections would be handled.
If I change 'AnkiDroid Directory', I presume this would remove the reminders
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewRemindersDatabase.kt
Outdated
Show resolved
Hide resolved
|
Tested switching the Alternatively I could change the code so that if a review reminder's associated deck does not exist, we don't delete the review reminder but rather leave it hanging there and just don't display it in the UI / mute its notifications. Then, if the deck comes back into existence later, the previously-dead review reminder is reactivated. The downside here is that dead review reminders would gradually accumulate in SharedPreferences. Perhaps the current system is the best? We could mark changing the AnkiDroid Directory as a destructive operation, perhaps. After all, it is an advanced feature. Regarding the problem of multiple collections, I think the general consensus that was reached in the Discord Right now, the review reminder deck deletion logic handling is simply checking if the deck exists in the collection, and if it doesn't, it deletes the associated review reminders. I'm not sure what the multiprofile equivalent of checking for deck existence would be. @criticalAY has taken over the project, I believe. Ashish, what's your plan for having multiple collections, and will this PR's method of checking if a deck exists by simply running |
bffdf9f to
a89e22c
Compare
|
|
Update: see discussion in Discord from September 10th, 2025. The core idea is:
This will fix the problem with AnkiDroid Directory and a whole host of other issues with the current deck deletion code. It also resolves my past self's concerns here:
...since the user will be able to manually view and delete accumulating dead review reminders. What's nice about this approach is that it also works in an acceptable way with the proposed multiprofile implementation. When a profile is switched to, so long as each profile has its own review reminders SharedPrefs file, the user will only get review reminders for that specific profile and not any others. Of course, this is not the intended way the multiprofile feature will work in its final version. According to discussion in #multiprofile, all profiles should have their valid review reminder notifications fired, even if the profile is not active. Hence, a multiprofile developer will need to edit NotificationService later on to make sure all profiles have their valid review reminder notifications fired by checking the corresponding profile. They will also need to edit BootService and other boot-related files to ensure all profiles have their review reminder notifications set after a device reboot. As I mentioned above, I'm proposing adding a profileID field to ReviewReminder so that when a notification is about to be fired, the corresponding collection can be checked to see if the deck still exists. Hopefully when Ashish sees this he'll let us know what the plan is for what the profileID is going to be (uuid, string, etc.) I'll work on implementing this as soon as possible. This PR is likely now dependent on #19109. Marking this as a draft; I'll mark it as open once it's ready for review! |
a89e22c to
159304c
Compare
|
Done! Now, instead of deleting review reminders when a deck cannot be found, we simply grey the review reminder out. This means all previous concerns about actions like switching the AnkiDroid Directory or restoring from backups are moot. This does mean that dead review reminders will gradually accumulate in SharedPreferences, but since the user can view dead reminders by going to I've also received an answer from Ashish regarding a profileID in the ReviewReminder schema and implemented it at #19242, so I've removed the "needs reviewer reply" tag from this PR. Ready for review! |
159304c to
52d681f
Compare
|
Rebased and resolved merge conflict. |
GSoC 2025: Review Reminders Edits ScheduleRemindersAdapter so that if a deck is not accessible to the user, the corresponding item in the ScheduleReminders list is grey and crossed-out. This is done by `errorReminderIfDeckNotFound`. Since global review reminders don't have a corresponding deck, they are always displayed as active. To ensure the style works with all possible app themes, I determine which theme is active using `attr` constants and convert them to theme colors via a `getThemeColor` method. However, since this method will run a lot (twice for each element in the list), I make it cache its return values. Checking whether a deck is accessible to the user via `canUserAccessDeck` requires a suspending call to the collection, which the Adapter cannot trigger, so I moved this logic to ScheduleReminders with a callback to continue Adapter logic after whether the deck is accessible has been retrieved. I also realized that this pattern of using a callback could also be applied to another already-existing function in ScheduleReminders that I thought was a bit messy: `setDeckNameFromScopeForView`. I originally created this method because I needed to retrieve the deck's name, which required launching a coroutine, which needed to be done in ScheduleReminders. However, the fact that `setDeckNameFromScopeForView` was also handling UI made the method feel impure and ugly. By using a callback, I can keep all the UI logic for individual list elements within ScheduleRemindersAdapter while still allowing the code flow to pass through ScheduleReminders in order to retrieve the deck's name.
52d681f to
bbbe6e4
Compare
|
Rebased and resolved merge conflict. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solid solution. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does seem like a nice solution
Review reminders for a deck should be greyed out when the deck they are tied to cannot be found. This PR implements this logic.
Description and Approach
Edits ScheduleRemindersAdapter so that if a deck is not accessible to the user, the corresponding item in the ScheduleReminders list is grey and crossed-out. This is done by
errorReminderIfDeckNotFound. Since global review reminders don't have a corresponding deck, they are always displayed as active.To ensure the style works with all possible app themes, I determine which theme is active using
attrconstants and convert them to theme colors via agetThemeColormethod. However, since this method will run a lot (twice for each element in the list), I make it cache its return values.Checking whether a deck is accessible to the user via
canUserAccessDeckrequires a suspending call to the collection, which the Adapter cannot trigger, so I moved this logic to ScheduleReminders with a callback to continue Adapter logic after whether the deck is accessible has been retrieved.I also realized that this pattern of using a callback could also be applied to another already-existing function in ScheduleReminders that I thought was a bit messy:
setDeckNameFromScopeForView. I originally created this method because I needed to retrieve the deck's name, which required launching a coroutine, which needed to be done in ScheduleReminders. However, the fact thatsetDeckNameFromScopeForViewwas also handling UI made the method feel impure and ugly. By using a callback, I can keep all the UI logic for individual list elements within ScheduleRemindersAdapter while still allowing the code flow to pass through ScheduleReminders in order to retrieve the deck's name.Fixes
How Has This Been Tested?
Learning
Old PR description:
Checklist