Skip to content
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

feat(local-notifications): Adding permissions for using SCHEDULE_EXACT_ALARM in Android 14 #1840

Merged
merged 43 commits into from
Dec 12, 2023

Conversation

theproducer
Copy link
Contributor

As a result of changes in Android 14, this PR makes using exact alarms for scheduling local notifications optional, and provides functions for checking and requesting permissions for using exact alarms.

@Ionitron Ionitron added this to In progress 🤺 in Capacitor Engineering ⚡️ Oct 11, 2023
@theproducer theproducer marked this pull request as ready for review October 16, 2023 14:21
Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plugin already handles the exact notifications being optional, the only difference was that on Android 13 is granted by default if the permission is present in the AndroidManifest.xml and on Android 14 is denied by default even if the permission is present, so the two methods for requesting the permission and checking its value are needed, but not the extra exact parameter. If the user adds the permission to the manifest I think that's enough indication that the want to use exact notifications without having to pass an extra parameter.

SCHEDULE_EXACT_ALARM is not a runtime permission, so it shouldn't be added to the alias, having permissions in the alias that are optional will result in errors for users that don't have the permission in the manifest.

Since it's not a runtime permission and the user won't be prompted but will be redirected to the notification settings page, not sure if the naming should be different to not confuse users, something like checkExactNotificationSetting and changeExactNotificationSetting

Don't use android.util.Log class, use our Logger wrapper so it can be disabled globally.

On Android 14 there is a new permission called USE_EXACT_ALARM, I think we should document it as an alternative to SCHEDULE_EXACT_ALARM but making it clear that is not an option for all apps.
https://developer.android.com/reference/android/Manifest.permission#USE_EXACT_ALARM

Capacitor Engineering ⚡️ automation moved this from In progress 🤺 to Needs review 🤔 Oct 16, 2023
Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changeExactNotificationSetting() method is opening the general notifications settings page, not the specific app notification settings page, it should open the app page, to do that, the creation of the intent to ACTION_REQUEST_SCHEDULE_EXACT_ALARM should include a param with the app package.

When calling changeExactNotificationSetting() when it's granted and changing it to denied, the app restarts. I don't think the restart can be prevented, but we should document that behavior, and also according to the docs all the notifications that used exact will be deleted (also verified by scheduling a notification and changed the setting and it didn't fire), so we should document that behavior too, and not sure if we should try to re schedule them, or leave that to the developer?

We should probably also provide some listener for the ACTION_SCHEDULE_EXACT_ALARM_PERMISSION_STATE_CHANGED broadcast, but it looks like only fires when changing from disabled to enabled according to the docs, so not sure if it would be very useful as I think it's more important the rejection of the exact than the approval.

@theproducer
Copy link
Contributor Author

so we should document that behavior too, and not sure if we should try to re schedule them, or leave that to the developer?

We could, but there could be scenarios where a developer wouldn't want notifications rescheduled if exact alarms werent available, so it could be argued either way. I'd say for now we just document it.

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added some minor comments

local-notifications/README.md Outdated Show resolved Hide resolved
local-notifications/src/definitions.ts Outdated Show resolved Hide resolved
# Conflicts:
#	local-notifications/ios/Plugin/LocalNotificationsPlugin.m
#	toast/ios/Tests/ToastPluginTests/ToastPluginTests.swift
@theproducer theproducer merged commit 55c31e8 into main Dec 12, 2023
12 checks passed
Capacitor Engineering ⚡️ automation moved this from Needs review 🤔 to Done 🎉 Dec 12, 2023
@theproducer theproducer deleted the local-notifications/alarm-permissions branch December 12, 2023 16:16
LaravelFreelancerNL pushed a commit to dennis-wedevise/capacitor-plugins that referenced this pull request Dec 19, 2023
…T_ALARM in Android 14 (ionic-team#1840)

Co-authored-by: Mike Summerfeldt <20338451+IT-MikeS@users.noreply.github.com>
Co-authored-by: jcesarmobile <jcesarmobile@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants