fix: handle manually disabling notifications pre-Tiramisu by ericli3690 · Pull Request #21400 · ankidroid/Anki-Android

ericli3690 · 2026-07-27T06:37:04Z

Note

Assisted-by: Claude Opus 5

Purpose / Description

I discovered that the function we were using for checking if notification permissions have been granted is flawed below API 33. Recall that it was:

Build.VERSION.SDK_INT < Build.VERSION_CODES.TIRAMISU ||
ContextCompat.checkSelfPermission(context, Manifest.permission.POST_NOTIFICATIONS) ==
PackageManager.PERMISSION_GRANTED

However, it is actually NOT the case that devices below API 33 unconditionally allow notifications to fire. It is possible to enter the settings and manually disable the ability for an app to fire notifications, meaning the above was returning a false positive. The correct API call is NotificationManagerCompat.from(context).areNotificationsEnabled(), which is equivalent to the old check at and above API 33, but which also handles pre-Tiramisu devices correctly.

Below: Google Pixel, API 25, OS settings page for notifications:

The "new" UI is simply to handle the legacy notification case. No visual changes:

Fixes

Fixes a false positive on the review reminder troubleshooting page that stated reminders were working on pre-Tiramisu devices when they were not.

Approach

See commit messages.

How Has This Been Tested?

Initial install and launch works
Unit tests
Manually granting and revoking permissions is updated accordingly
Buttons within review reminders UI and troubleshooting UI for enabling and revoking permissions works
BottomSheet shows correctly both before and after API 33
Permission explanation screen works correctly both before and after API 33
Tested on a physical Lenovo Tab M11, API 35
Tested on a physical Samsung S23, API 36
Tested on an emulated Pixel, API 25
Tested on an emulated Pixel 6 Pro, API 31

Learning

Discovered while working on feat(reminders): linking to OS notification settings #21398
For notes on why I decided to create a separate pref for the BottomSheet, see the new pref's docstring
I'm kind of unhappy that I had to create a new dummy fake permission string for legacy notifications to enable openAppSettingsScreenForPermission, but I can't see any other way to do it. It doesn't cause any problems in the codebase right now, but it's kind of hacky. Being able to use singleOrNull in PermissionItems is nice, though. If anyone has a better idea, please let me know.

Checklist

You have a descriptive commit message with a short title (first line, max 50 chars).
You have commented your code, particularly in hard-to-understand areas
You have performed a self-review of your own code

github-actions · 2026-07-27T06:44:01Z

Snapshot diff report vs main. Open screenshot-diff for diffs.

PreferencesScreenshotTest: 1 change
ReviewRemindersScreenshotTest: 11 changes

All 12 changed screenshots

PreferencesScreenshotTest

ScheduleRemindersFragment_compare.png

ReviewRemindersScreenshotTest

settingsHostTablet_scheduleReminders_compare.png
settingsHostTablet_troubleshooting_compare.png
settingsHost_scheduleReminders_compare.png
settingsHost_scheduleReminders_scrolled_compare.png
settingsHost_troubleshooting_compare.png
standaloneActivityHost_scheduleReminders_compare.png
standaloneActivityHost_troubleshooting_compare.png
studyOptionsFragmentHost_scheduleReminders_compare.png
studyOptionsFragmentHost_troubleshooting_compare.png
studyOptionsFrameHost_scheduleReminders_compare.png
studyOptionsFrameHost_troubleshooting_compare.png

ericli3690 · 2026-07-27T06:44:35Z

 * should be used to request optional permissions from the user, and can be launched as the user gradually
 * encounters features that require permissions rather than being shoved in the face of every first-time user.
 */
-@RequiresApi(Build.VERSION_CODES.TIRAMISU)


Removed because this is no longer Tiramisu-specific

ericli3690 · 2026-07-27T06:45:22Z

 * Permissions screen for requesting permissions until API 29.
 *
 * Requested permissions:
- * 1. Storage access: [Permissions.legacyStorageAccessPermissions].


This name was out of date and the IDE was screaming at me to fix it haha

ericli3690 · 2026-07-27T06:46:35Z

+     * There is no formal permission to request, but it is possible for the user to manually disable notifications in Settings.
+     * If they want notifications, we need to direct the user to the OS settings via the [NotificationsPermissionFragment] so they can re-enable them.
+     */
+    LEGACY_NOTIFICATIONS(listOf(LEGACY_POST_NOTIFICATIONS), NotificationsPermissionFragment::class.java),


Note that the permissions field of both NOTIFICATIONS and LEGACY_NOTIFICATIONS is not actually used anywhere... that field is only read for the external storage permission sets via hasRequiredPermissions.

Thus theoretically I could roll these into one entry in PermissionSet... but that would require removing Permissions.notificationsPermission from the list and would require more documentation

ericli3690 · 2026-07-27T06:48:01Z

-    val tiramisuPhotosAndVideosPermissions =
-        listOf(
-            Manifest.permission.READ_MEDIA_IMAGES,
-            Manifest.permission.READ_MEDIA_VIDEO,
-        )


Unused and it was bothering me

ericli3690 · 2026-07-27T06:48:51Z

-    val postNotification =
-        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) {
-            Manifest.permission.POST_NOTIFICATIONS
-        } else {
-            null
-        }


As stated in my commit message, this fancy if was causing more problems than it was solving and the type checker was not smart enough to consider it, leading to duplicate checks that the SDK was >= Tiramisu

ericli3690 · 2026-07-27T06:49:13Z

-    @RequiresApi(Build.VERSION_CODES.TIRAMISU)
-    val tiramisuAudioPermission = Manifest.permission.READ_MEDIA_AUDIO


ericli3690 · 2026-07-27T06:49:30Z

-    fun canPostNotifications(context: Context): Boolean =
-        Build.VERSION.SDK_INT < Build.VERSION_CODES.TIRAMISU ||
-            ContextCompat.checkSelfPermission(context, Manifest.permission.POST_NOTIFICATIONS) == PackageManager.PERMISSION_GRANTED


Moved to common

ericli3690 · 2026-07-27T06:51:50Z

    <com.ichi2.anki.ui.windows.permissions.PermissionsItem
        android:id="@+id/post_notification_permission_item"
        android:layout_width="match_parent"
        android:layout_height="wrap_content"
        app:permissionTitle="@string/notification_pref"
        app:permissionSummary="@string/notifications_permission_explanation"
        app:permissionIcon="@drawable/ic_notifications"
        app:permission="@string/post_notification_permission"
        android:visibility="gone"
        tools:visibility="visible"
        />

+    <com.ichi2.anki.ui.windows.permissions.PermissionsItem
+        android:id="@+id/legacy_post_notification_permission_item"
+        android:layout_width="match_parent"
+        android:layout_height="wrap_content"
+        app:permissionTitle="@string/notification_pref"
+        app:permissionSummary="@string/notifications_permission_explanation"
+        app:permissionIcon="@drawable/ic_notifications"
+        app:permission="@string/legacy_post_notification_permission"
+        android:visibility="gone"
+        tools:visibility="visible"
+        />


We need one for API 33+ and one for API <33. The "duplicate" is necessary because revokeIfGrantedOnClickElse will need to manually open the OS settings when a permission is revoked, but in order to open the notifications settings subpage on pre-Tiramisu devices, we need to pass in the proper app:permission. No UI change results from this, we conditionally render in AllPermissionsExplanationFragment.

ericli3690 · 2026-07-27T07:41:07Z

+ *   Used to view and cancel sync progress.
+ *   Used for review reminder notifications.
+ */
+class LegacyNotificationsPermissionFragment : PermissionsFragment(R.layout.fragment_legacy_notifications_permission) {


Can be triggered pre-Tiramisu by, for example, freshly installing the app, disabling notifications manually in settings, then trying to create a review reminder.

ericli3690 · 2026-07-27T07:54:06Z

Note screenshot tests now display the permission-granted state rather than the permission-not-granted state due to funky logic with Robolectric interacting with our switch from ContextCompat.checkSelfPermission to NotificationManagerCompat.from(context).areNotificationsEnabled().

BrayanDSO

2nd commit implements too many things, which is noted on its description. Splitting the commit would ease the reviewing process

ericli3690 · 2026-07-27T19:52:09Z

Fair point, sorry about that. Will break it up.

ericli3690 · 2026-07-28T04:27:01Z

@BrayanDSO Broken up into separate commits! Ready for review.

Lint is failing but it's also failing on main... I think something weird got into the main branch.

`Permissions.postNotification` was declared as a nullable value: `Manifest.permission.POST_NOTIFICATIONS` at and above API 33, and `null` below it. Every call site had to unwrap it with `?.let`. Several call sites then went on to check `Build.VERSION.SDK_INT` anyway because static analysis thought that API 33 was not enforced, so the API level check was duplicated rather than avoided. This commit simplifies the logic and replaces `postNotification` with `Permissions.notificationsPermission`, a non-null `@RequiresApi(TIRAMISU)` constant. No behaviour change. Assisted-by: Claude Opus 5

When a permission can no longer be requested through the OS dialog, we send the user into the system settings to grant it by hand. Where the OS provides one, we should land them on the settings subscreen for that specific permission rather than on the generic app info page, which requires them to hunt for the right toggle. This should be the default interface function for opening the OS for a permission. Further branches can be added to the `when` block in the future. Assisted-by: Claude Opus 5

Below API 33 there is no `POST_NOTIFICATIONS` permission to request, but the user can still turn notifications off for the app in the system settings, so there is still something for us to represent and to send the user off to fix. Our permission machinery is keyed on permission strings throughout (`PermissionSet` holds a list of them, `PermissionsItem` takes one via the `app:permission` layout attribute, `openAppSettingsScreenForPermission` switches on one) so representing "notifications, below API 33" means we need a string to key on. Adds `LEGACY_POST_NOTIFICATIONS`, a dummy sentinel string, alongside a matching `legacy_post_notification_permission` string resource so layouts can refer to it too, and teaches `openAppSettingsScreenForPermission` to route it to the notification settings screen. The sentinel is not a real permission, so handing it to `ContextCompat.checkSelfPermission` would always report it as denied. `hasPermission` therefore resolves it through `canPostNotifications` before it can reach `checkSelfPermission`. Nothing passes the sentinel to `hasPermission` today, but `PermissionSet` and `PermissionsItem` both funnel into it. That resolution is also why `canPostNotifications` moves into `common` next to `hasPermission`. Assisted-by: Claude Opus 5

"Get notifications turned on for this app" means two different things depending on the API level. At and above API 33 we request `POST_NOTIFICATIONS` through the system dialog, falling back to the notification settings screen once the OS stops letting us ask. Below API 33 there is no permission to request at all, so the only thing we can do is send the user to the notification settings screen directly. A small utility function is created. Currently, this is only used in one place, but a little bit of extra abstraction is not too harmful especially since it allows for some unit tests. The one place this is currently used is ReminderTroubleshootingFragment, where "Grant permission" action previously returned `null` below API 33, meaning the check could report a problem while offering no way to act on it. Assisted-by: Claude Opus 5

The check we were using to decide whether notifications can be posted was flawed below API 33. It was: ```kotlin Build.VERSION.SDK_INT < Build.VERSION_CODES.TIRAMISU || ContextCompat.checkSelfPermission(context, Manifest.permission.POST_NOTIFICATIONS) == PackageManager.PERMISSION_GRANTED ``` That short-circuit assumes devices below API 33 unconditionally allow notifications to fire. That is not the case: on any API level the user can go into the system settings and switch notifications off for the app entirely. So on older devices the check returned a false positive, and we would happily report that notifications were fine when in fact nothing we posted would ever be shown. `NotificationManagerCompat.from(context).areNotificationsEnabled()` is equivalent to the old check at and above API 33 and also handles pre-Tiramisu devices correctly, so we should use that instead. Assisted-by: Claude Opus 5

Both of the places where we ask the user for notifications were gated behind API 33, on the assumption that there was nothing to ask for below it. Now that we detect notifications being switched off by the user on older devices, both are useful there as well. They just have to send the user to the system settings rather than request a permission, since there is no permission to request. This needs a way to avoid spam. At and above API 33 we keep showing the sheet until the OS tells us to stop, via `shouldShowRequestPermissionRationale`. Below API 33 there is no such signal, so we have to remember for ourselves whether the sheet has been shown, which is what the new `notificationsBottomSheetShownBelowAPI33` pref is for. We could technically have reused `notificationsPermissionRequested`, but that would give it two jobs and blur its definition as "has the OS system dialog for requesting notifications been presented to the user before". Below API 33 that dialog does not exist, so `notificationsPermissionRequested` should always be false there. Assisted-by: Claude Opus 5

ericli3690 · 2026-07-29T20:55:11Z

Rebased, CI passing. Thanks to lukstbit for fixing the lint issues on main. Ready for review.

ericli3690 self-assigned this Jul 27, 2026

github-actions Bot added the Needs Review label Jul 27, 2026

ericli3690 commented Jul 27, 2026

View reviewed changes

ericli3690 marked this pull request as draft July 27, 2026 06:55

ericli3690 removed the Needs Review label Jul 27, 2026

ericli3690 force-pushed the ericli3690-pre-tiramisu-notifs branch from 6270040 to f217d22 Compare July 27, 2026 07:38

ericli3690 commented Jul 27, 2026

View reviewed changes

ericli3690 marked this pull request as ready for review July 27, 2026 07:56

ericli3690 added the Needs Review label Jul 27, 2026

BrayanDSO reviewed Jul 27, 2026

View reviewed changes

ericli3690 marked this pull request as draft July 27, 2026 19:52

ericli3690 removed the Needs Review label Jul 27, 2026

ericli3690 force-pushed the ericli3690-pre-tiramisu-notifs branch from f217d22 to 4400761 Compare July 28, 2026 04:00

ericli3690 marked this pull request as ready for review July 28, 2026 04:25

ericli3690 added the Needs Review label Jul 28, 2026

ericli3690 added 4 commits July 29, 2026 15:14

chore: delete unused permissions constants, fix docstring

8da168f

ericli3690 added 3 commits July 29, 2026 15:14

ericli3690 force-pushed the ericli3690-pre-tiramisu-notifs branch from 4400761 to 36d7d7e Compare July 29, 2026 19:15

		@RequiresApi(Build.VERSION_CODES.TIRAMISU)
		val tiramisuAudioPermission = Manifest.permission.READ_MEDIA_AUDIO

Uh oh!

Uh oh!

Conversation

ericli3690 commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose / Description

Fixes

Approach

How Has This Been Tested?

Learning

Checklist

Uh oh!

github-actions Bot commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericli3690 Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericli3690 commented Jul 27, 2026

Uh oh!

BrayanDSO left a comment

Choose a reason for hiding this comment

Uh oh!

ericli3690 commented Jul 27, 2026

Uh oh!

ericli3690 commented Jul 28, 2026

Uh oh!

ericli3690 commented Jul 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ericli3690 commented Jul 27, 2026 •

edited

Loading

github-actions Bot commented Jul 27, 2026 •

edited

Loading

ericli3690 Jul 27, 2026 •

edited

Loading