Skip to content

Don't crash with Android < 7.0 (when requesting permission) #1007

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

Merged
merged 1 commit into from
Feb 24, 2025

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Feb 19, 2025

Description

One Line Summary

Don't crash on Android < 7.0 when request permission is called, by using the lower-level API directly instead of a helper.

Details

  • The OneSignal-defined Continue.with helper is only available on API 24 (Android 7.0).
  • We still aim to support Android 5 and 6, so don't use this wrapper helper and use the Continuation class directly.

Motivation

#981

Testing

Unit testing

None

Manual testing

Not tested on old Android versions.
Tested on emulator with API 33:

  1. Request permission and deny. Function suspends and returns false to the caller.
  2. Request permission and accept. Function suspends and returns true to the caller.
  3. Permission is already on, and request permission. Function doesn't suspend and returns true immediately.
  4. Request permission with fallback to settings set to true and deny the prompt (do this 3 times until the fallback to settings prompt shows). Now go to settings and turn on permission. Outcome: the 3 denies return false to the caller and returning to the app after turning on permission in settings returns true to the caller.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

This change is Reviewable

@nan-li nan-li changed the title Avoid unavailable API with Android < 7.0 (when requesting permission) Don't crash with Android < 7.0 (when requesting permission) Feb 19, 2025
Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

Believe Continue.with can work on Android 6 and below, however lambdas do not. If you try use it in our Java native example project you will get a crash like this:

java.lang.NoClassDefFoundError: com.onesignal.sdktest.application.MainApplication$$ExternalSyntheticLambda2

Google does have a Desugaring library to allow lambdas, this is probably common on native Android apps, but not something we can easily use for wrappers, as it has to be on the app level .gradle file.

What might be the best solution short term solutions is to use the Continuation directly, that way the fallback to settings still works.

Incomplete code but doesn't crash:

        OneSignal.getNotifications().requestPermission(true, new Continuation<Boolean>() {
            @Override
            public void resumeWith(@NonNull Object o) {

            }

            @NonNull
            @Override
            public CoroutineContext getContext() {
                return (CoroutineContext) Dispatchers.getMain();
            }
        });

Long term we could switch to Kotlin, I believe it translates down to Android 5 correctly.

@@ -67,7 +69,8 @@ else if (call.method.contentEquals("OneSignal#addNativeClickListener"))
private void requestPermission(MethodCall call, Result result) {
boolean fallback = (boolean) call.argument("fallbackToSettings");
// if permission already exists, return early as the method call will not resolve
if (OneSignal.getNotifications().getPermission()) {
// Continue.with API below requires Android 7
if (OneSignal.getNotifications().getPermission() || Build.VERSION.SDK_INT < Build.VERSION_CODES.N) {
replySuccess(result, true);
Copy link
Member

@jkasten2 jkasten2 Feb 20, 2025

Choose a reason for hiding this comment

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

On Android 5 and 6 there is no prompting, however the end-user can disable notifications in the settings. The code in the PR will always report true, which is not always the case.

@nan-li nan-li force-pushed the support_android_5_6 branch from be4a59e to 5100f94 Compare February 21, 2025 17:54
@nan-li
Copy link
Contributor Author

nan-li commented Feb 21, 2025

Ahh that makes sense, and I also missed that the Continue class is a OneSignal-defined class, not a coroutines API.

* The OneSignal-defined `Continue.with` is only available on API 24 (Android 7.0)
* We still aim to support Android 5 and 6, so don't use this wrapper helper and use the Continuation class directly.
@nan-li nan-li force-pushed the support_android_5_6 branch from 5100f94 to c611221 Compare February 21, 2025 17:58
@nan-li nan-li requested a review from jkasten2 February 21, 2025 18:06
Base automatically changed from android_remove_deprecated_v1_embedding to main February 24, 2025 17:38
@nan-li nan-li merged commit 68d1c53 into main Feb 24, 2025
2 checks passed
@nan-li nan-li deleted the support_android_5_6 branch February 24, 2025 17:49
@nan-li nan-li mentioned this pull request Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants