Anki-Android icon indicating copy to clipboard operation
Anki-Android copied to clipboard

Created ReviewRemindersDatabase and ReviewReminder

Open ericli3690 opened this issue 7 months ago • 14 comments

Purpose / Description

Review Reminders database system.

  • Created ReviewRemindersDatabase, which contains methods for reading and writing to a Preferences Datastore instance for storing review reminder data
  • Created ReviewReminder, which defines the review reminder data class
  • Created Robolectric unit tests for ReviewRemindersDatabase and ReviewReminder

Fixes

  • For GSoC 2025: Review Reminders

Approach

Using Preferences Datastore was the original method I proposed for storing review reminder data in my original project proposal: see [1]

Why did I initially choose Preferences Datastore for storing review reminder data?

  1. Google recommends using it over Shared Preferences: see [2]
  2. It has been used before. Past attempts at creating a review reminders system (see [3]) used Preferences Datastore.

However, we've now chosen to use SharedPreferences instead. It's consistent with what the rest of AnkiDroid uses and doesn't necessitate a whole new import. It's also a lot simpler and straightforward.

How Has This Been Tested?

  • ReviewRemindersDatabaseTest directly or indirectly exercises all methods within ReviewRemindersDatabase.
  • ReviewReminderTest exercises the ID allocation method of ReviewReminder
  • On a physical Samsung S23, API 34., buttons to add, edit, and delete reminders (coming soon in a later PR) cause expected changes to the database and the database changes persist between app sessions.

Learning

I've tried my best to make my code clean and safe. In particular, allocating and deallocating IDs is handled internally and any developer creating a new ReviewReminder should be unable to manually forge an ID.

Originally, I planned to create different types of review reminders. I've now abandoned that in favour of a simpler approach involving snooze functionality.

Checklist

  • [x] You have a descriptive commit message with a short title (first line, max 50 chars).
  • [x] You have commented your code, particularly in hard-to-understand areas
  • [x] You have performed a self-review of your own code
  • [x] UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • [x] UI Changes: You have tested your change using the Google Accessibility Scanner

ericli3690 avatar May 24 '25 04:05 ericli3690

Hi, here's a first draft of what I'd like the review reminder storage local database storage system to be like. Looking forward to constructive criticism. For now, I’ve put these files in anki/notifications since that seemed a reasonable place to put them. Let me know if I should move them elsewhere, such as by creating a new anki/reviewreminders folder.

ericli3690 avatar May 24 '25 04:05 ericli3690

Had to do a second force push because I forgot to move the files to a new package as outlined at #18318.

ericli3690 avatar May 28 '25 05:05 ericli3690

Repeated force push because I forgot to rebase, my bad.

ericli3690 avatar Jun 01 '25 03:06 ericli3690

Hi, I've refactored to use SharedPreferences instead of Preferences Datastore. In retrospect, thanks for telling me to use it, it really is a lot more simple and I should have started with this method. Using Datastore was like trying to crack a nut with a sledgehammer. The only negative consequence is that my branch I'm trying to merge these changes in from is still called review-reminders-datastore, so that's a bit confusing-looking but shouldn't be an issue.

Regarding other overly-complex points I've simplified: the fancy ID-allocation system is now gone, and I've decided to move the addReminderForDeck, editReminderForDeck, and deleteReminderForDeck methods to the UI onClick methods since that should be the only place they're called from, and so they don't need to be in ReviewRemindersDatabase.

I've also done my best to clean up the docstrings, as per feedback. Previously they were more ad-hoc, now I've tried my best to make them genuine pieces of documentation.

ericli3690 avatar Jun 01 '25 04:06 ericli3690

I realized the SharedPreferences keys are too short. Since there's tons of other things being stored in SharedPreferences, it's probably a good idea to append review_reminders_ to the beginning of the keys I use in these files so they don't collide with any other keys.

ericli3690 avatar Jun 01 '25 06:06 ericli3690

I realized while I was building out the ScheduleReminders RecyclerView more that:

  1. There are typos in this commit, whoops.
  2. I need to store a review reminder's corresponding deck ID within ReviewReminder for the RecyclerView to work properly in GLOBAL_DECK_SPECIFIC scope mode.
  3. I need to add a method to retrieve the reminders in all decks to ReviewRemindersDatastore, to be used in GLOBAL_DECK_SPECIFIC scope mode.

ericli3690 avatar Jun 01 '25 23:06 ericli3690

Rebased and fixed a List that should have been a MutableList.

ericli3690 avatar Jun 06 '25 01:06 ericli3690

  • Added a ReviewReminderId typealias.
  • Tweaked the contracts of the database's "edit" methods to now require a lambda which returns a List rather than strictly a MutableList.
  • Added an editAllDeckSpecificReminders method. I realized I needed it for the deck-specific scope mode's editing while creating the bulk deletion feature.
  • Added a new test to ReviewRemindersDatabaseTest to test editAllDeckSpecificReminders.
  • Cleaned up some syntax in the two new test files.

ericli3690 avatar Jun 08 '25 06:06 ericli3690

Please do not review this PR. I'm actively refactoring it as I've realized I can streamline the UX. Sorry for asking for reviews and then going back and changing things again, I really do appreciate the constructive criticism of my code and am sorry for pulling it out of the merge pipeline when it's so close to the end.

ericli3690 avatar Jun 09 '25 05:06 ericli3690

Refactored. Removed ReviewReminderTypes and replaced it with snoozing. Looking forward to reviews!

ericli3690 avatar Jun 14 '25 18:06 ericli3690

Thanks Ashish! I've added some try-catch blocks to handle corrupt JSON strings and added a few unit tests to test that behaviour.

ericli3690 avatar Jun 14 '25 23:06 ericli3690

  • Added getAllDeckSpecificRemindersGrouped
  • Refactored getAllDeckSpecificReminders and editAllDeckSpecificReminders to use it

ericli3690 avatar Jun 15 '25 16:06 ericli3690

  • Replaced List with HashMap to make editing individual review reminders more efficient
  • Added two utility methods to ReviewRemindersDatabase to flatten and group maps of review reminders
  • Updated ReviewRemindersDatabaseTest, which no longer needs to inefficiently sort lists of review reminders when asserting

ericli3690 avatar Jun 15 '25 19:06 ericli3690

  • Added the enabled property to ReviewReminder

ericli3690 avatar Jun 15 '25 22:06 ericli3690

@ericli3690 I wanted to check—when we delete a deck, do its associated reminders still remain stored in SharedPreferences?

sanjaysargam avatar Jun 20 '25 02:06 sanjaysargam

Thanks for the reviews.

@SanjaySargam Regarding whether review reminders should remain in SharedPreferences after the corresponding deck is deleted: no, they should not, and don't worry, it's on my to-do list. Thanks for reminding me!

ericli3690 avatar Jun 21 '25 04:06 ericli3690

  • Added try-catch for JSON encoding
  • Created a APP_WIDE_REMINDER_DECK_ID constant
  • Added ReviewReminderTime and SnoozeAmount classes
  • Cleaned up references to context.sharedPrefs() to instead use Prefs or AnkiDroidApp.sharedPrefs()
  • Added a new preferences resource to allow for Prefs.getString()

ericli3690 avatar Jun 21 '25 06:06 ericli3690

  • Used an inline value class for ReviewReminderId, modified ReviewReminderTest accordingly
  • Modified ReviewReminderTime's methods to better use the Duration API
  • Modified ReviewReminderCardTriggerThreshold to be an inline value class
  • Moved ReviewReminder field types out of ReviewReminder so that they are no longer nested classes; this allows for less verbose code in other files
  • ReviewReminderScope from ScheduleReminders is now used in ReviewReminder to indicate deck ID, it will be used in both places, as fundamentally indicating what scope is being edited by ScheduleReminders and which scope a ReviewReminder is attached to can be done by the same sealed data class; I personally think this is an elegant abstraction
  • Used by intPref for the next free review reminder ID
  • Shortened the docstring of getAndIncrementNextFreeReminderId
  • Implemented the backend of the ReviewReminder migration process, including attemptSchemaMigration and OldReviewReminderSchema; the use of these backend methods from ScheduleReminders is also complete and has unit tests, but will be delivered in the upcoming RecyclerView PR
  • Added an explanation of the ReviewReminder migration process to the docstring of ReviewReminder
  • Added getAllReviewReminderSharedPrefsAsMap and deleteAllReviewReminderSharedPrefs helper methods to ReviewRemindersDatabase and added tests for them to ReviewRemindersDatabaseTest
  • Cleaned up tests in ReviewRemindersDatabaseTest to minimize code duplication
  • Made ReviewRemindersDatabase entirely Context-free; error-checking and dialog-throwing is complete in the form of ScheduleReminders.catchDatabaseExceptions, but will be delivered in the upcoming RecyclerView PR
  • Added lines to ReviewRemindersDatabase docstrings indicating that methods may throw SerializationExceptions/IllegalArgumentExceptions and should be caught within the upcoming ScheduleReminders.catchDatabaseExceptions method

ericli3690 avatar Jun 30 '25 06:06 ericli3690

  • Rebased on main
  • Fixed a bug where migrated review reminder HashMaps would have incorrect, unmigrated keys; unit tests to check for this have been developed but will be lumped together with the RecyclerView PR

ericli3690 avatar Jul 01 '25 03:07 ericli3690

  • Rebased on main
  • Fixed typo: Added the missing enabled param to the docstring of ReviewReminder
  • Tweaked a few tests to validate manually passing the optional enabled field to createReviewReminder

ericli3690 avatar Jul 01 '25 17:07 ericli3690

  • Moved getAndIncrementNewFreeReminderId to ReviewReminderId
  • Modified require clauses of ReviewReminderSnoozeAmount
  • Renamed timeInterval property of ReviewReminderSnoozeAmount to interval
  • Added more documentation to the docstrings of ReviewReminderSnoozeAmount and ReviewReminderScope.DeckSpecific.getDeckName
  • Rebased on main

ericli3690 avatar Jul 03 '25 05:07 ericli3690