Use ActivityAware to get the Activity context
#534
Conversation
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @emawby, @Jeasmine, @nan-li, and @tanaynigam)
android/src/main/java/com/onesignal/flutter/OneSignalPlugin.java, line 109 at r1 (raw file):
public void onAttachedToActivity(@NonNull ActivityPluginBinding binding) { // onAttachedToEngine is called first that sets the BinaryMessenger so we can pass null here. init(
onAttachedToEngine already calls init, adding this means are will be calling init more than once. Some of the logic such as channel = new MethodChannel(this.messenger, "OneSignal"); doesn't look like it should be called more than once in there.
Some options the come to mind however are check if init is being called more than once, or to split up this context gathering and calling OneSignal.initWithContext.
After the changes to support Android V2 embedding, the plugin is set up in `onAttachedToEngine` and the Application Context is not an instance of Activity. When this context is eventually passed to `setAppId`, we end up with the current activity is null which leads to some problems. This is only a problem on the first run of the app. This commit implements the `ActivityAware` interface and in `onAttachedToActivity`, updates the context to the new activity (https://docs.flutter.dev/development/packages-and-plugins/plugin-api-migration). In addition, use `activeContext()` instead of `context()` for the registrar for Flutter's v1 embedding to get the application context. Doing some testing shows the former gives, for example, the `MainActivity` while the latter gives `FlutterApplication`.
nan-li
force-pushed
the
fix/android_activity
branch
from
fe03fa0 to
c319281
Compare
I think it suffices to update |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @emawby, @Jeasmine, and @tanaynigam)
android/src/main/java/com/onesignal/flutter/OneSignalPlugin.java, line 109 at r1 (raw file):
Previously, nan-li (Nan) wrote…
I think it suffices to update
this.contextwith the application context when we receiveonAttachedToActivity. As far as I can tell, thethis.contextis only ever used byinitWithContext()which should happen later. In some simple testing,onAttachedToActivityis called immediately afteronAttachedToEngine.
Good idea, and is less code too.
|
|
||
| @Override | ||
| public void onDetachedFromActivity() { | ||
| } |
There was a problem hiding this comment.
Should our Activity reference be cleared here?
There was a problem hiding this comment.
Thanks for your input @westy92,
Since we only need to get the activity on initialization and use the activity only for setAppId, we don't to track it after.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @emawby, @Jeasmine, and @nan-li)
Description
One Line Summary
Implement the Flutter
ActivityAwareinterface to pass theActivitycontext instead ofApplicationcontext tosetAppId.Details
Motivation
Fixes #509. What we were experiencing was that on the first run of the app, the current activity would be
nullso that IAMs don't display and the app is sensed as in the background even when in the foreground. Backgrounding the app and returning resumed normal behavior.Why? After the changes to support Android V2 embedding, the plugin is set up in
onAttachedToEngineand the Application Context is not an instance of Activity.When this context is eventually passed to
setAppId, we end up with the current activity isnullwhich leads to some problems. Note that this is only a problem on the first run of the app.This PR implements the
ActivityAwareinterface and inonAttachedToActivity, updates the existing context to the activity received, following some Flutter documentation here.In addition, use
activeContext()instead ofcontext()for the registrar for Flutter's v1 embedding to get the application context. Doing some testing shows the former provides theMainActivitywhile the latter providesFlutterApplication.Scope
During testing,
onAttachedToActivityis called right afteronAttachedToEngineso we expect to get the activity context immediately.There are 3 other
ActivityAwaremethods are not implemented as they don't appear to be needed. We only need the first call to Flutter'sonAttachedToActivityduring the first run of the app. After that, the OneSignalActivityLifecycleHandlershould be triggered and set the activities.Testing
Unit testing
N/A
Manual testing
Tested on Pixel 5 emulator on API 30 and checking logs for
onAttachedToEngineandonAttachedToActivityand what context is passed tosetAppId. Also put logging for the other 3 methods inActivityAwareand they are never logged.Affected code checklist
Checklist
Overview
Testing
Final pass
This change is