feat(reminders): AddEditReminderDialog#19109
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements a comprehensive add/edit dialog for review reminders, allowing users to create, modify, and delete review reminders with time and deck selection functionality. The implementation includes a ViewModel for state management, advanced settings with card trigger thresholds, and proper integration with the existing reminder system.
Key changes:
- Added
AddEditReminderDialogwith full CRUD functionality for review reminders - Implemented
AddEditReminderDialogViewModelfor persistent state management across configuration changes - Enhanced
ScheduleReminderswith RecyclerView adapter and database integration - Fixed deck spinner selection logic to properly handle "All Decks" option
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| styles.xml | Added TimePickerStyle theme for Material3 time picker styling |
| add_edit_reminder_dialog.xml | Complete dialog layout with time selection, deck dropdown, and expandable advanced settings |
| ScheduleRemindersAdapter.kt | New RecyclerView adapter for displaying reminder list items |
| ScheduleReminders.kt | Enhanced with database operations, UI state management, and dialog result handling |
| AddEditReminderDialogViewModel.kt | ViewModel for managing dialog state and field values |
| AddEditReminderDialog.kt | Main dialog implementation with time picker, validation, and fragment result communication |
| DeckSpinnerSelection.kt | Fixed deck selection logic to properly show "All Decks" option |
Comments suppressed due to low confidence (1)
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/AddEditReminderDialog.kt:65
- The parameter type is inconsistent with the LiveData type. The function accepts
Int?but the LiveData is typed asInt. This could lead to null pointer exceptions when setting null values.
* In particular, whether this dialog will be used to add a new review reminder or edit an existing one.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/AddEditReminderDialogViewModel.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ScheduleReminders.kt
Show resolved
Hide resolved
6fc8ff3 to
c165680
Compare
|
Resolved Copilot feedback. Implemented a Google Accessibility Scanner recommendation. |
|
Once #18964 is merged, I'll rebase this PR on top of main. Until then, this PR is ready for review! |
| <style name="TimePickerStyle" parent="ThemeOverlay.Material3.MaterialTimePicker"> | ||
| <item name="elevationOverlayEnabled">false</item> | ||
| <item name="colorTertiaryContainer">?colorPrimary</item> | ||
| <item name="colorOnTertiaryContainer">?colorOnPrimary</item> | ||
| <item name="colorPrimaryContainer">?colorPrimary</item> | ||
| </style> |
| android:orientation="horizontal"> | ||
|
|
||
| <!-- The following image's icon switches between ic_baseline_chevron_right_24 and ic_expand_more_black_24dp_xml --> | ||
|
|
There was a problem hiding this comment.
Can remove this line? or maybe comment too or is it needed?
There was a problem hiding this comment.
I don't understand the part of your request about the comment.
There was a problem hiding this comment.
@criticalAY Like Arthur, I'm also a little confused. I think what you mean is that I should remove this comment? I've gone ahead and done that. Let me know if you wanted something else instead.
| * the input type the user is using to enter the threshold into the app. In other words, this class reflects the concrete | ||
| * EditText fields in the dialog, not abstract backend data representations. | ||
| */ | ||
| class AddEditReminderDialogViewModel( |
| } | ||
|
|
||
| // 2. state -> viewModel: Configure how the UI reacts to changes in the state | ||
| viewModel.time.observe(this) { time -> |
There was a problem hiding this comment.
Why are we not using viewLifecycleOwner?
There was a problem hiding this comment.
viewModel.time.observe(viewLifecycleOwner) { time -> … }
viewModel.advancedSettingsOpen.observe(viewLifecycleOwner) { advancedSettingsOpen -> … }
Can we do better here, inspect?
There was a problem hiding this comment.
Nope, I thought about using viewLifecycleOwner, but after experimenting with it, I've determined that we shouldn't use it because this is a DialogFragment. According to the docs:
Note: When subscribing to lifecycle-aware components such as LiveData, never use viewLifecycleOwner as the LifecycleOwner in a DialogFragment that uses Dialog objects. Instead, use the DialogFragment itself, or, if you're using Jetpack Navigation, use the NavBackStackEntry.
Source: https://developer.android.com/guide/fragments/dialogs
| } | ||
|
|
||
| // Fill in initial field values | ||
| timeButton.text = viewModel.time.value.toString() |
There was a problem hiding this comment.
rely only on observers?
There was a problem hiding this comment.
You're right, that line is unnecessary. Deleted.
| return dialog | ||
| } | ||
|
|
||
| fun setUpDialogFields() { |
There was a problem hiding this comment.
split this method into smaller ones
| setFragmentResultListener(ADD_EDIT_DIALOG_RESULT_REQUEST_KEY) { _, bundle -> | ||
| val modeOfFinishedDialog = | ||
| BundleCompat.getParcelable( | ||
| requireArguments(), | ||
| ACTIVE_DIALOG_MODE_ARGUMENTS_KEY, | ||
| AddEditReminderDialog.DialogMode::class.java, | ||
| ) | ||
| val newOrModifiedReminder = BundleCompat.getParcelable(bundle, ADD_EDIT_DIALOG_RESULT_REQUEST_KEY, ReviewReminder::class.java) | ||
|
|
||
| Timber.d("Dialog result received with recent dialog mode: %s", modeOfFinishedDialog) | ||
| if (modeOfFinishedDialog == null) return@setFragmentResultListener | ||
| handleAddEditDialogResult(newOrModifiedReminder, modeOfFinishedDialog) | ||
| } |
There was a problem hiding this comment.
Too dense extract it out
There was a problem hiding this comment.
I've cleaned up the code here a bit but I don't think it can be extracted in any meaningful way. Here's what it looks like now:
setFragmentResultListener(ADD_EDIT_DIALOG_RESULT_REQUEST_KEY) { _, bundle ->
val modeOfFinishedDialog =
BundleCompat.getParcelable(
requireArguments(),
ACTIVE_DIALOG_MODE_ARGUMENTS_KEY,
AddEditReminderDialog.DialogMode::class.java,
) ?: return@setFragmentResultListener
val newOrModifiedReminder =
BundleCompat.getParcelable(
bundle,
ADD_EDIT_DIALOG_RESULT_REQUEST_KEY,
ReviewReminder::class.java,
)
Timber.d("Dialog result received with recent dialog mode: %s", modeOfFinishedDialog)
handleAddEditDialogResult(newOrModifiedReminder, modeOfFinishedDialog)
}
Is there a specific way you were thinking of extracting out some of this code?
| /** | ||
| * When a [AddEditReminderDialog] instance finishes, we handle the result of the dialog fragment via this method. | ||
| */ | ||
| private fun handleAddEditDialogResult( |
There was a problem hiding this comment.
too much in one method, attaching a patch
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/ScheduleReminders.kt
Show resolved
Hide resolved
| } | ||
| triggerUIUpdate() | ||
|
|
||
| showSnackbar( |
Arthur-Milchior
left a comment
There was a problem hiding this comment.
I tried to review, but very little to state on top of what @criticalAY mentioned
| const val DECK_SELECTION_RESULT_REQUEST_KEY = "reminder_deck_selection_result_request_key" | ||
|
|
||
| private const val SERIALIZATION_ERROR_MESSAGE = | ||
| "Something went wrong. A serialization error was encountered while working with review reminders." |
There was a problem hiding this comment.
Why is this a hard coded string?
If it's because you're not sure it'll be in the final product and don't want it translated, add a TODO please.
If it's fine to translate it, put it with resources please
There was a problem hiding this comment.
It's hard-coded because I'm not sure if its wording is in its final form, i.e. I don't want it translated yet. Thus, I've added a TODO comment. Thanks!
| android:orientation="horizontal"> | ||
|
|
||
| <!-- The following image's icon switches between ic_baseline_chevron_right_24 and ic_expand_more_black_24dp_xml --> | ||
|
|
There was a problem hiding this comment.
I don't understand the part of your request about the comment.
56ae2d0 to
3bc07f5
Compare
Ready for review again! |
3bc07f5 to
16bc8f6
Compare
|
Minor edits:
|
There was a problem hiding this comment.
Don't let this review be blocking, I'm happy, and I'm aware this is blocking other reviews. If you want this merged, let me know.
In general, more logic could be in the VIewModel
I mentioned the review threshold should be split over ~3 lines, could you test this (if you understand my intent), as I think:
- it will look nicer
- it allows you to use
.errorand simplifyonSubmit
| viewModelFactory { | ||
| initializer { | ||
| Timber.d("Initializing AddEditReminderDialogViewModel") | ||
| when (val mode = dialogMode) { |
There was a problem hiding this comment.
I believe you can use SavedStateHandle to access arguments inside a ViewModel
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/AddEditReminderDialog.kt
Show resolved
Hide resolved
| is DialogMode.Add -> "Add review reminder" | ||
| is DialogMode.Edit -> "Edit review reminder" |
There was a problem hiding this comment.
Let's lock them into R.string
| private fun setUpTimeButton() { | ||
| val timeButton = contentView.findViewById<MaterialButton>(R.id.add_edit_reminder_time_button) | ||
| timeButton.setOnClickListener { | ||
| Timber.d("Time button clicked") |
There was a problem hiding this comment.
Prefer Timber.i for user interactions:
if there's a bug, I want a brief understand of what a user pressed (no sensitive information in these)
| return | ||
| } | ||
|
|
||
| val reminderToBeReturned = |
There was a problem hiding this comment.
nit: add a TODO to move this to the ViewModel
| Timber.d("Submitted dialog") | ||
|
|
||
| // Validate numerical fields | ||
| if ((viewModel.cardTriggerThreshold.value ?: -1) < 0) { |
There was a problem hiding this comment.
Can you display validation instead (see TextInputLayout.error)?
Once you've done that, you can disable 'submit', and turn calling 'onSubmit' when it's in a bad state into an error state. (R.string.something_wrong)
AnkiDroid/src/main/java/com/ichi2/anki/reviewreminders/AddEditReminderDialogViewModel.kt
Show resolved
Hide resolved
| /** | ||
| * TODO: Move to string resources for translation once review reminders are stable. | ||
| */ | ||
| private const val SERIALIZATION_ERROR_MESSAGE = |
There was a problem hiding this comment.
The error itself isn't actionable, so a user doesn't need a translated string
I'd go with something like:
R.string.something_wrong_error_code: Something went wrong (%s), where %s: SERIALIZATION_ERROR
Feel free to pull this into another PR and it can quickly be merged
There was a problem hiding this comment.
As mentioned in Discord, I'm thinking of changing how we handle a catastrophic review reminders database read. Instead of just showing an error message, I'd like to delete the corrupted review reminders so the app can still be used. Yes, this will cause data loss, but I don't think review reminders are so important that they should block normal interaction with the app. I'll likely create a string along the lines of "Some of your review reminders were corrupted and were deleted, sorry". I'll add an ACRA log, too.
For now, I've deleted these TODO strings and will clean them up in a forthcoming PR.
|
Thanks for the review! I'll take a look at implementing your suggested changes shortly. |
e70066e to
330c8b6
Compare
Images in the PR description have been updated! Ready for review. |
330c8b6 to
2c4b023
Compare
|
GSoC 2025: Review Reminders - `initializeScheduleRemindersDeckSpinner` doesn't properly handle what happens if showAllDecks is true. This makes pressing on a deck select the wrong deck. This commit fixes this.
GSoC 2025: Review Reminders - Added TimePicker styling, used when users edit the time of a review reminder.
GSoC 2025: Review Reminders - The AddEditReminderDialog displays the current time in the TimePicker when a review reminder is being created. This change adds a utility function to ReviewReminderTime's companion object to calculate this time. I decided to put this function within ReviewReminderTime so it can be accessed by both AddEditReminderDialog and AddEditReminderDialogViewModel.
GSoC 2025: Review Reminders - XML file for AddEditReminderDialog. Allows time and deck modification. Also has a dropdown for advanced settings, including minimum card trigger threshold (with more advanced settings hopefully coming soon). The advanced settings will be grouped under a dropdown to prevent them from overwhelming users when the dialog initially opens.
GSoC 2025: Review Reminders - Creates AddEditReminderDialog. - Adds DialogMode, which represents whether the dialog is in adding or editing mode. - Created a ViewModel for the AddEditReminderDialog. A specific view model is created because this allows the dialog's state to persist across redraws, ex. if the theme changes, if the device rotates, etc. It also centralizes the data of the review reminder being edited in a single source of truth. - Sets submit, cancel, and delete actions for the dialog; this is the main way users can delete review reminders. Deletion is locked behind a confirmation box just in case the user accidentally clicks the button. - Results for the deck picker dropdown in the dialog are received in ScheduleReminders and sent to the AddEditReminderDialog via a FragmentResult. See the docstring of `onDeckSelected` in ScheduleReminders to see why this is done. - Filling in the values in the dialog fields and setting listeners for when the values change is pulled out into a separate methods for readability. - Adds `showTimePickerDialog`. Shows a TimePicker dialog for picking review reminder time via modern Material3 guidelines. Overrides `onConfigurationChanged` to properly handle device rotation, which for some reason is not handled by default by MaterialTimePicker. - An edited review reminder is passed back to ReviewRemindersDatabase as a FragmentResult, which is a simple way of passing information between fragments.
GSoC 2025: Review Reminders - Adds a fragment result listener to ScheduleReminders to detect when an AddEditReminderDialog has completed, and if so, edits the database and UI accordingly. - Filled out the `addReminder` and `editReminder` methods to show the AddEditReminderDialog.
GSoC 2025: Review Reminders - Realized a lint annotation in ScheduleRemindersAdapter is unnecessary.
2c4b023 to
c049c11
Compare
|
|
@david-allison Thanks for the fast merge on the 24h-time-formatting PR I created! Do you think we could quickly get this guy in, too? I'll be able to put more code up for review once it's in, and I think I've addressed all your feedback from the last time you looked at it. I've heard you're on vacation, though; if that's the case, please ignore this! |
david-allison
left a comment
There was a problem hiding this comment.
LGTM, cheers!
Sorry this sat!
|
No problem at all, thanks!! |

Purpose / Description
Allows users to add and edit review reminders.
All commit messages, together:
initializeScheduleRemindersDeckSpinnerdoesn't properly handle what happens if showAllDecks is true. This makes pressing on a deck select the wrong deck. This commit fixes this.onDeckSelectedin ScheduleReminders to see why this is done.showTimePickerDialog. Shows a TimePicker dialog for picking review reminder time via modern Material3 guidelines. OverridesonConfigurationChangedto properly handle device rotation, which for some reason is not handled by default by MaterialTimePicker.addReminderandeditRemindermethods to show the AddEditReminderDialog.UI Screenshots:


Dialog:
Error handling:

Sub-dialogs:

TimePickers:


Tablet:


Fixes
For GSoC 2025: Review Reminders
Approach
Creates a ViewModel to store the state of the dialog. Information is passed between the dialog and ScheduleReminders via fragment results, which is a simple and straightforward API.
How Has This Been Tested?
Learning (optional, can help others)
<item name="elevationOverlayEnabled">false</item>.Checklist