-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat(reminders): request notification permissions #19167
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
Draft
ericli3690
wants to merge
12
commits into
ankidroid:main
Choose a base branch
from
ericli3690:ericli3690-review-reminders-permission-request
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
feat(reminders): request notification permissions #19167
ericli3690
wants to merge
12
commits into
ankidroid:main
from
ericli3690:ericli3690-review-reminders-permission-request
+1,181
−53
Conversation
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
Important Maintainers: This PR contains Strings changes
|
5 tasks
37e535a
to
e53a1b6
Compare
|
e53a1b6
to
e8b030c
Compare
|
e8b030c
to
d11214a
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.
d11214a
to
d611d76
Compare
|
…rs-dialog-august' into ericli3690-review-reminders-permission-request
As discussed in Discord, we have decided to use the PermissionsActivity which shows up when the app is initially installed to request mandatory permissions from the user. For optional permissions, we have decided to try and request them in a less intrusive way. Here, I've created a PermissionsBottomSheet that can pop up and request optional permissions from the user. I use this bottom sheet in a later commit for requesting notification permissions. I've modeled PermissionsBottomSheet after PermissionsActivity. I'm fairly sure I cannot reuse PermissionsActivity's code because it is a full-screen activity, whereas PermissionsBottomSheet needs to inherit BottomSheetFragment and be a fragment.
GSoC 2025: Review Reminders Created the PermissionsFragment which will be hosted inside PermissionsBottomSheet for requesting notification permissions from the user. It is launched via `requestNotificationsPermissionIfNeeded` in Permissions. This method takes a callback so that I can do some conditional preference-setting in a later commit. Created a new PermissionSet for the notifications permission in InitialActivity, which is gated behind API 33. In a way, this means I've sort of resurrected TiramisuPermissionsFragment, so maybe I shouldn't have been so quick to delete it, whoops.
GSoC 2025: Review Reminders Adds two new boolean Preferences for saving whether we have requested notification permissions from the user: one for whether it has been done for the sync process yet (we will request notification permissions the first time the user tries to sync) and one for whether it has been done for the review reminders feature yet (we will request notification permissions the first time the user creates a review reminder). I'm aware there is a `shouldShowRequestPermissionRationale` method which Google recommends using to check if we should request notification permissions from the user. I do not believe we should use it as it is incredibly confusing and has some very weird edge-case behaviour. See [this post](https://stackoverflow.com/questions/41310510/what-is-the-difference-between-shouldshowrequestpermissionrationale-and-requestp) for more information. Rather, it makes good sense to use two quick preferences and carefully handle whether we have shown the request dialog ourselves. This is also more extensible in the future, should a future developer choose to request the notification permission in a different circumstance, too. I have decided to request the permission once for the sync feature and once for the review reminder feature because if the user denies the permission for the sync feature and then goes to enable review reminders, they may change their mind about providing AnkiDroid with notification permissions; and vice versa. If the notification is denied, I have chosen not to downgrade any review reminders functionality: the user will still be able to create, edit, and delete them. However, they will not receive any notifications. I do not believe it makes sense to restrict the user's ability to edit review reminders even though they have notifications disabled, as a user might want to customize their reminders to their liking before enabling notifications, etc. In the future, I may add a help screen to help users who are confused why they are not getting notifications and who may have notifications accidentally disabled, but otherwise, I think we should assume that if the user has denied notification permissions, then we simply should not sent notifications, and do not need to downgrade any other functionality.
GSoC 2025: Review Reminders The old code requested notification permissions via MyAccount and requests the permission each time a new user logs in, which makes no sense. Notifications are tied to the phone the app is installed on, not an individual user profile. Rather, we should be requesting notification permissions when the user first triggers a sync. This is what this change accomplishes. Deletes all old logic for requesting notification permissions. Deletes `checkNotificationPermission` as it has been moved to Permissions as `shouldRequestNotificationPermissions`. Requests notification permissions the first time the user triggers a sync.
GSoC 2025: Review Reminders Request notification permissions when the user first creates a review reminder. Involves a simple edit to AddEditReminderDialog.
d611d76
to
0703e5d
Compare
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Blocked by dependency
Currently blocked by some other dependent / related change
GSoC
Pull requests authored by a Google Summer of Code participant [Candidate/Selected], for GSoC mentors
Has Conflicts
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.
Blocked by dependencyCurrently blocked by some other dependent / related change
:
Purpose / Description
Creates a BottomSheet for requesting notification permissions from the user. This BottomSheet shows up when the user first triggers a sync and when the user first creates a review reminder, as these are the two features that rely on notifications.
UI
All Commit Messages
As discussed in Discord, we have decided to use the PermissionsActivity which shows up when the app is initially installed to request mandatory permissions from the user. For optional permissions, we have decided to try and request them in a less intrusive way. Here, I've created a PermissionsBottomSheet that can pop up and request optional permissions from the user. I use this bottom sheet in a later commit for requesting notification permissions.
I've modeled PermissionsBottomSheet after PermissionsActivity. I'm fairly sure I cannot reuse PermissionsActivity's code because it is a full-screen activity, whereas PermissionsBottomSheet needs to inherit BottomSheetFragment and be a fragment.
Created the PermissionsFragment which will be hosted inside PermissionsBottomSheet for requesting notification permissions from the user. It is launched via
requestNotificationsPermissionIfNeeded
in Permissions. This method takes a callback so that I can do some conditional preference-setting in a later commit. Created a new PermissionSet for the notifications permission in InitialActivity, which is gated behind API 33. In a way, this means I've sort of resurrected TiramisuPermissionsFragment, so maybe I shouldn't have been so quick to delete it, whoops.Adds two new boolean Preferences for saving whether we have requested notification permissions from the user: one for whether it has been done for the sync process yet (we will request notification permissions the first time the user tries to sync) and one for whether it has been done for the review reminders feature yet (we will request notification permissions the first time the user creates a review reminder).
I'm aware there is a
shouldShowRequestPermissionRationale
method which Google recommends using to check if we should request notification permissions from the user. I do not believe we should use it as it is incredibly confusing and has some very weird edge-case behaviour. See this post for more information. Rather, it makes good sense to use two quick preferences and carefully handle whether we have shown the request dialog ourselves. This is also more extensible in the future, should a future developer choose to request the notification permission in a different circumstance, too.I have decided to request the permission once for the sync feature and once for the review reminder feature because if the user denies the permission for the sync feature and then goes to enable review reminders, they may change their mind about providing AnkiDroid with notification permissions; and vice versa.
If the notification is denied, I have chosen not to downgrade any review reminders functionality: the user will still be able to create, edit, and delete them. However, they will not receive any notifications. I do not believe it makes sense to restrict the user's ability to edit review reminders even though they have notifications disabled, as a user might want to customize their reminders to their liking before enabling notifications, etc. In the future, I may add a help screen to help users who are confused why they are not getting notifications and who may have notifications accidentally disabled, but otherwise, I think we should assume that if the user has denied notification permissions, then we simply should not sent notifications, and do not need to downgrade any other functionality.
The old code requested notification permissions via MyAccount and requests the permission each time a new user logs in, which makes no sense. Notifications are tied to the phone the app is installed on, not an individual user profile. Rather, we should be requesting notification permissions when the user first triggers a sync. This is what this change accomplishes.
Deletes all old logic for requesting notification permissions. Deletes
checkNotificationPermission
as it has been moved to Permissions asshouldRequestNotificationPermissions
. Requests notification permissions the first time the user triggers a sync.Request notification permissions when the user first creates a review reminder. Involves a simple edit to AddEditReminderDialog.
Fixes
Approach
How Has This Been Tested?
Learning (optional, can help others)
Checklist