feat(reminders): only notify if no reviews#19157
Conversation
9f44e2d to
787f50a
Compare
|
787f50a to
f8c1584
Compare
|
f8c1584 to
5b3ac81
Compare
|
5b3ac81 to
1178a0e
Compare
|
Rebased, fully unblocked, and ready for review! |
criticalAY
left a comment
There was a problem hiding this comment.
Nothing to add, LGTM!
david-allison
left a comment
There was a problem hiding this comment.
Looks GREAT!
One request on the SQL concatenation
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/AddEditReminderDialogViewModel.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminder.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/services/NotificationService.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/services/NotificationService.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/test/java/com/ichi2/anki/services/NotificationServiceTest.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/test/java/com/ichi2/anki/services/NotificationServiceTest.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/test/java/com/ichi2/anki/services/NotificationServiceTest.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/test/java/com/ichi2/anki/services/NotificationServiceTest.kt
Outdated
Show resolved
Hide resolved
1178a0e to
f2d2cce
Compare
|
While addressing David's feedback (thanks David!) I found a very embarrassing bug where it turns out the feature... didn't work? I could've sworn it worked in my testing when I first pushed the PR, but it seems I was comparing seconds to milliseconds the entire time, so I've added new tests to monitor this blindspot and I'm fairly sure it's all now working according to plan. Now unblocked and ready for review! |
f2d2cce to
2f9d231
Compare
|
criticalAY
left a comment
There was a problem hiding this comment.
Looks good! Thankyou!!
| private fun createAndSaveDummyDeckSpecificReminder(did: DeckId): ReviewReminder { | ||
| val reviewReminder = | ||
| ReviewReminder.createReviewReminder( | ||
| cardTriggerThreshold = ReviewReminderCardTriggerThreshold(1), | ||
| scope = ReviewReminderScope.DeckSpecific(did), | ||
| ) | ||
| ReviewRemindersDatabase.editRemindersForDeck(did) { mapOf(ReviewReminderId(0) to reviewReminder) } | ||
| return reviewReminder | ||
| } | ||
|
|
||
| private fun createAndSaveDummyAppWideReminder(): ReviewReminder { | ||
| val reviewReminder = | ||
| ReviewReminder.createReviewReminder( | ||
| cardTriggerThreshold = ReviewReminderCardTriggerThreshold(1), | ||
| ) | ||
| ReviewRemindersDatabase.editAllAppWideReminders { mapOf(ReviewReminderId(1) to reviewReminder) } | ||
| return reviewReminder | ||
| } | ||
|
|
||
| private fun triggerDummyReminderNotification(reviewReminder: ReviewReminder) { | ||
| val intent = | ||
| NotificationService.getIntent( | ||
| context, | ||
| reviewReminder, | ||
| NotificationService.NotificationServiceAction.ScheduleRecurringNotifications, | ||
| ) | ||
| NotificationService().onReceive(context, intent) | ||
| } | ||
|
|
There was a problem hiding this comment.
Can do with better helper methods to accept optional parameters so you can reuse them everywhere but not gonna block it over it, Looks good to me apart from the nitpicks David suggested
There was a problem hiding this comment.
Hm, I get what you mean, but respectfully I'm probably going to leave it as is. Based on my understanding of this test file, making a bunch of helper functions would be really complicated and likely not worth the effort. Thanks for the speedy review!!
2f9d231 to
c765099
Compare
|
Thanks to the both of you for the rapid reviews and kind words! I'm going to yank this guy off the merge queue to get these last few nits handled, though, as making a separate PR to patch those will give me a headache. Now just waiting for David's clarification on a few comments that I unfortunately don't understand entirely. No rush! |
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ReviewReminder.kt
Outdated
Show resolved
Hide resolved
| fun createReviewReminder( | ||
| time: ReviewReminderTime, | ||
| cardTriggerThreshold: ReviewReminderCardTriggerThreshold, | ||
| time: ReviewReminderTime = ReviewReminderTime(0, 0), |
There was a problem hiding this comment.
Why is the default midnight? Most people would be sleeping, it's also not close to the default rollover time
Could you add a comment with justification (or remove it if you want callers to always be explicit with the time)
There was a problem hiding this comment.
Hm, ok, after thinking this through some more, you're right - ideally callers should be explicit with the time, so I should remove this default argument. I initially added the default argument to streamline creating review reminders in the test files, but I can accomplish that by adding a small helper function that auto-fills the time instead. While I'm at it, I can make setting the threshold and scope etc. easier via the helper, too:
private fun createTestReminder(
deckId: DeckId? = null,
thresholdInt: Int = 1,
onlyNotifyIfNoReviews: Boolean = false,
) = ReviewReminder.createReviewReminder(
time = ReviewReminderTime(hour = 12, minute = 0),
cardTriggerThreshold = ReviewReminderCardTriggerThreshold(thresholdInt),
scope = if (deckId != null) ReviewReminderScope.DeckSpecific(deckId) else ReviewReminderScope.Global,
onlyNotifyIfNoReviews = onlyNotifyIfNoReviews,
)I initially shied away from this approach because it felt a bit ugly, but after writing it out it actually looks pretty good. Implemented!
c765099 to
7c6553a
Compare
GSoC 2025: Review Reminders Adds an advanced review reminder option. When this setting is enabled, the review reminder only triggers notifications if the deck the review reminder is for has not been reviewed yet today. Adds a checkbox to the AddEditReminderDialog to toggle this setting on or off. Adds a method to NotificationService to check if a deck (or all decks) have been reviewed yet today. The check is accomplished via a database query of the `revlog` and `cards` tables. In my experience there is no noticeable latency, the query I've written should be fairly efficient. Adds a boolean field to store the state of this setting to ReviewReminder. Adds unit tests. Adds default arguments to parameters in ReviewReminder to make AlarmManagerServiceTest and NotificationServiceTest less verbose.
7c6553a to
bae06e0
Compare
|
Addressed David's feedback! David, if everything looks good to you, feel free to chuck this guy in the merge queue, I'm happy with it now. Cheers! |
Purpose / Description
Adds an advanced review reminder option. When this setting is enabled, the review reminder only triggers notifications if the deck the review reminder is for has not been reviewed yet today.
Adds a checkbox to the AddEditReminderDialog to toggle this setting on or off. Adds a method to NotificationService to check if a deck (or all decks) have been reviewed yet today. The check is accomplished via a database query of the
revlogandcardstables. In my experience there is no noticeable latency, the query I've written should be fairly efficient.UI
Fixes
Approach
The SQL query is:
Where
startOfTodayis calculated viasched.dayCutoff - 1.days.inWholeSeconds.How Has This Been Tested?
Learning (optional, can help others)
I found the database schema wiki page useful: https://github.com/ankidroid/Anki-Android/wiki/Database-Structure
Checklist