-
Notifications
You must be signed in to change notification settings - Fork 233
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
Conversation
There was a problem hiding this 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); |
There was a problem hiding this comment.
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.
be4a59e
to
5100f94
Compare
Ahh that makes sense, and I also missed that the |
* 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.
5100f94
to
c611221
Compare
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
Continue.with
helper is only available on API 24 (Android 7.0).Continuation
class directly.Motivation
#981
Testing
Unit testing
None
Manual testing
Not tested on old Android versions.
Tested on emulator with API 33:
false
to the caller.true
to the caller.true
immediately.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 returnfalse
to the caller and returning to the app after turning on permission in settings returnstrue
to the caller.Affected code checklist
Checklist
Overview
Testing
Final pass
This change is