-
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
jkasten2
left a comment
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.
| 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.withhelper is only available on API 24 (Android 7.0).Continuationclass directly.Motivation
#981
Testing
Unit testing
None
Manual testing
Not tested on old Android versions.
Tested on emulator with API 33:
falseto the caller.trueto the caller.trueimmediately.trueand 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 returnfalseto the caller and returning to the app after turning on permission in settings returnstrueto the caller.Affected code checklist
Checklist
Overview
Testing
Final pass
This change is