-
Notifications
You must be signed in to change notification settings - Fork 368
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
Fix outside package target activities (Launch URL) not opening when notification is tapped on #1469
Conversation
This fix includes any launch URLs set to open the browser or another app The issue was due to an Activity backstack not being built correctly. Both `NotificationOpenedReceiver` and the target `Activity` were launched with `FLAG_ACTIVITY_NEW_TASK` however this means when `NotificationOpenedReceiver` is finished it does not bring up the target since it is on a different task. The Android flag `allowTaskReparenting` allows this to happen, however this can effect the backstack of the main task which we don't want to do. Setting `android:taskAffinity` does `FLAG_ACTIVITY_NEW_TASK` and `allowTaskReparenting` for us. By setting the value to "" (empty string) it prevents any side effects on existing tasks as it means it has no affinity to associate it with. This allowed us to clean up some runtime logic to add `FLAG_ACTIVITY_NEW_TASK` and others we no longer need now.
App would not resume on Android 5 after the `taskAffinity=""` addition added in the last commit. This seems to be an issue with allowTaskReparenting not working in this case. Even explicitly enabling it did not work. `taskAffinity` is required for newer vesions of Android to correctly start a new task and to reparent it to the correct task. This is why two different implementations had to be added.
This is to flag that these should be cleaned up when support for Android API 22 and older is dropped.
Ran the auto convert in Android Studio and updated comment in code
9a83d47
to
00f1367
Compare
Add test to ensure correct Activity is for Notification opens when device is running Android API 23 (Android 6) or newer.
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.
Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @Jeasmine, @jkasten2, and @nan-li)
OneSignalSDK/onesignal/src/main/java/com/onesignal/GenerateNotificationOpenIntent.kt, line 11 at r1 (raw file):
private val context: Context, private val intent: Intent?, private val startApp: Boolean
Are we using this boolean anymore?
* results in the app not resuming, when using reverse Activity trampoline. | ||
* Oddly enough cold starts of the app were not a problem. | ||
*/ | ||
class NotificationOpenedReceiverAndroid22AndOlder : Activity() { |
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.
will it make sense to have a single Base NotificationOpenedReceiver and both NotificationOpenedReceiver and NotificationOpenedReceiverAndroid22AndOlder implement it?
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.
Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @emawby, @Jeasmine, and @nan-li)
OneSignalSDK/onesignal/src/main/java/com/onesignal/GenerateNotificationOpenIntent.kt, line 11 at r1 (raw file):
Previously, emawby (Elliot Mawby) wrote…
Are we using this boolean anymore?
Yes, the only place it gets used now is at the top of getIntentAppOpen
.
OneSignalSDK/onesignal/src/main/java/com/onesignal/NotificationOpenedReceiverAndroid22AndOlder.kt, line 43 at r1 (raw file):
Previously, Jeasmine (JNahui) wrote…
will it make sense to have a single Base NotificationOpenedReceiver and both NotificationOpenedReceiver and NotificationOpenedReceiverAndroid22AndOlder implement it?
ah yes, I'll do that. It will de-dup a few lines of code and prevent missing the other class when updating any of the logic in it.
Created abstract NotificationOpenedReceiverBase to share code between both activities. It is very unlikly the code would ever be different between them and prevents possible future mistakes of editing only one.
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.
Reviewable status: 3 of 6 files reviewed, 2 unresolved discussions (waiting on @emawby, @Jeasmine, and @nan-li)
OneSignalSDK/onesignal/src/main/java/com/onesignal/NotificationOpenedReceiverAndroid22AndOlder.kt, line 43 at r1 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
ah yes, I'll do that. It will de-dup a few lines of code and prevent missing the other class when updating any of the logic in it.
I addressed this in commit 17f6340
Description
One Line Summary
Fix notification open with Launch URL not showing any Activities, this was a regression bug introduced in PR #1377.
Details
Motivation
Notification Launch URL is an active supported feature used by OneSignal customers so ensuring it works correctly is high priority.
Scope
The main goal is to fix Launch URL not working but the code change in this PR effects all notification opens. The "opens" noted here is scoped to how
Activities
are shown to the user. This includes theActivity
flags used to startActivities
and the backstace it generates.No changes to how OneSignal consumes or generates
Intent
data.Why the bug was happening?
The issue was due to an Activity backstack not being built correctly. Both
NotificationOpenedReceiver
and the targetActivity
were launched withFLAG_ACTIVITY_NEW_TASK
however this means whenNotificationOpenedReceiver
is finished it does not bring up the target since it is on a different task. The Android flagallowTaskReparenting
allows this to happen, however this can effect the backstack of the main task which we don't want to do.How the bug was fixed?
Setting
android:taskAffinity
setsFLAG_ACTIVITY_NEW_TASK
andallowTaskReparenting
for us. By setting the value to "" (empty string) it prevents any side effects on existing tasks as it means it has no affinity to associate it with. This allowed us to clean up some runtime logic to addFLAG_ACTIVITY_NEW_TASK
and others we no longer need now.A caveat is that on Android 5.1 and older
android:taskAffinity=""
doesn't give us theallowTaskReparenting
behavior, nor does setting it explicitly work. To account for this anActivity
namedNotificationOpenedReceiverAndroid22AndOlder
was created without theandroid:taskAffinity
parameter.Related additional information
Tasks and the back stack
Basic understand of this topic is recommend
What
android:taskAffinity=""
doesSource: https://developer.android.com/guide/topics/manifest/activity-element#aff
Notification Trampolines
Background on what "Notification Trampolines" are and how a "Reverse Activity Trampoline" works.
Testing
Unit testing
Added test to ensure correct Activity is used for newer vs older versions of Android.
No existing exist unit tests to ensure correct Activity is infocus or the backstack is built correctly. Robolectric doesn't have fakes / mocking to simulate to this level.
Manual testing
Each scenario was tested with the following devices:
Default notification
HTTPS URL notification
com.onesignal.NotificationOpened.DEFAULT set to DISABLE
Data URL scheme - appname://test
Tested by adding the following
<intent-filter>
entries for the following Activities in the example app'sAndroidManifest.xml
.Each scenario below includes testing twice, once with
osexamplesplash://
another withosexample://
.Affected code checklist
Checklist
Overview
Testing
Final pass
This change is