[Change] Notification Activity Trampoline forward instead of reverse #1581
Conversation
jkasten2
force-pushed
the
change/notification_activity_trampoline_forward_instead_of_reverse
branch
from
6d03271
to
ea0f039
Compare
jkasten2
changed the title
This fixes a bug where the app would not be brought to the foreground when tapping on a notification if the app could not connect to onesignal.com or was offline during the full life time of the app process. This also fixes another a compatibility issue with Xiaomi devices where the app would not foreground if the app process was dead and also no task exists in the recent list. It is critical we don't wait for a network call for the notification open logic this method does the work to bring the app to the foreground. This get params network call isn't needed before we handle the notification open logic anyway. This network wait was present in 4.0.0 through 4.4.x but was not an issue in 4.5.0 when a "Reverse Activity Trampoline" was setup. However in PR #1581 we are switching back to a standard Activity Trampoline and we don't want to reintroduce this bug. A failing test was added for this in a previous commit, which now passes.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 2 of 5 files at r2, 1 of 12 files at r3.
Reviewable status: 1 of 13 files reviewed, 3 unresolved discussions (waiting on @jkasten2 and @nan-li)
OneSignalSDK/onesignal/src/main/java/com/onesignal/IntentGeneratorForAttachingToNotifications.kt line 1 at r3 (raw file):
package com.onesignal
Do we want copyright statement? I did notice a lot of new files such as under outcomes or language don't have the copyright either.
OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignal.java line 2434 at r3 (raw file):
static void openDestinationActivity( @NonNull final Activity activity, @NonNull final JSONArray pushPayload
This is an array of pushes (at least 1 push but maybe more) right? Would it be more clear to say pushPayloads?
OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignal.java line 2450 at r3 (raw file):
} else { logger.info("SDK not showing an Activity automatically due to it's settings.");
Maybe obvious, but to clarify, this is referring to android.intent.action.MAIN in the Manifest?
This commit focuses on removing all logic that setups up multiple Activities and flags to build a "Reverse Activity Trampoline". This is Part 1 of required changes, this commit results in the app not opening, resuming, or opening launch URLs, the behavior of the click being tracked is still working via the invisible NotificationOpenedReceiver Activity. This gives us a clean starting state for the next commit where we we will add logic to the notification open event to Activity Trampoline forward to the intended Activity.
Now everything uses the same forward activity trampolining behavior so everything works consistently!, including the behavior of HMS message type notifications. Removed the startLauncherActivity flag from handleNotificationOpen due this consistently as well. Renamed startOrResumeApp to openDestinationActivity as this now handles URL opens as well. There was a bug here where HMS notifications did not open launch URLs that has also been fixed here. Added logging if we don't open an Activity, to indicate the behavior was intended.
GenerateNotificationOpenIntent used to handle everything related to Intents for notifications since it had to create the full reverse Activity trampoline and attach it to the notification. Now the notification generation logic simply needs to always open the SDK's invisible click tracking Activity. When the notification is open the runtime logic will take the push payload into consideration to trampoline to the destination Activity.
While this commit isn't a git revert most of the changes are undoing the following commit: 5f72750 Additionally this fixes tests due to the dropped activity param from OneSignal_handleNotificationOpen.
Added extra foreground assert to existing test to ensure it fails. Added missing openDestinationActivity call for IAM notification handling and ensured test passed. Moved logic from NotificationOpenProcessor.handleIAMPreviewOpen into OSInAppMessagePreviewHandler.notificationOpened Remaned inAppMessagePreviewHandled to notificationReceived so it is more clear now that OSInAppMessagePreviewHandler handles both opens and received pushes.
jkasten2
force-pushed
the
change/notification_activity_trampoline_forward_instead_of_reverse
branch
from
41c6ab8
to
1c3e69e
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 13 files reviewed, 3 unresolved discussions (waiting on @nan-li)
OneSignalSDK/onesignal/src/main/java/com/onesignal/IntentGeneratorForAttachingToNotifications.kt line 1 at r3 (raw file):
Previously, nan-li (Nan) wrote…
Do we want copyright statement? I did notice a lot of new files such as under
outcomesorlanguagedon't have the copyright.
Good catch, let's discuss outside of this PR.
OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignal.java line 2434 at r3 (raw file):
Previously, nan-li (Nan) wrote…
This is an array of pushes (at least 1 push but maybe more) right? Would it be more clear to say pushPayloads?
Ah yes, renamed to pushPayloads.
OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignal.java line 2450 at r3 (raw file):
Previously, nan-li (Nan) wrote…
Maybe obvious, but to clarify, this is referring to
android.intent.action.MAINin the Manifest?
That is one of them, it could also be if they have com.onesignal.NotificationOpened.DEFAULT or com.onesignal.suppressLaunchURLs and it is a push with a URL. This could change later too so it's best not to put a comment here with that level of detail and it is easy to overlook when someone is updating this logic.
This fixes a bug where the app would not be brought to the foreground when tapping on a notification if the app could not connect to onesignal.com or was offline during the full life time of the app process. This also fixes another a compatibility issue with Xiaomi devices where the app would not foreground if the app process was dead and also no task exists in the recent list. It is critical we don't wait for a network call for the notification open logic this method does the work to bring the app to the foreground. This get params network call isn't needed before we handle the notification open logic anyway. This network wait was present in 4.0.0 through 4.4.x but was not an issue in 4.5.0 when a "Reverse Activity Trampoline" was setup. However in PR #1581 we are switching back to a standard Activity Trampoline and we don't want to reintroduce this bug. A failing test was added for this in a previous commit, which now passes.
jkasten2
deleted the
change/notification_activity_trampoline_forward_instead_of_reverse
branch
Description
One Line Summary
Reverts Reverse Activity Trampoline back to standard Activity Trampoline, which executes on notification open.
Details
Motivation
It was discovered after Reverse Activity Trampoline was released in version 4.5.0 that it was not compatible with Xiaomi devices and with HMS message type notifications.
For HMS we already went back to using an Activity Trampoline in PR #1533, this was required since the notification is generated by HMS core we don't have the level of control needed for it.
For Xiaomi devices Reverse Activity Trampoline does not work due to a bug where the back stack is not persevered on notification open.
Reverse Activity Trampoline has limitations on where it can be used. Activity Trampoline is the only option which allows us to have the same implementation for all our features and platforms we support. All the other alternative options (listed below) need to use Activity Trampolining somewhere anyway to fill in gaps to track clicks for all types of notifications, devices, and push vendors.
Scope
Reverting PR #1377 (Reverse Activity Trampoline) behavior while keeping any intended fixes and refactoring from it.
Alternatives considered
Option 1 - Detect by MIUI version, do Normal Activity Trampling
This might not be possible, even if it is it through this would fragile and extra part to maintain a list of MIUI versions and possibly other manufactures.
Option 2 - Detect missing back stack entry on open
Detect if there is a back stack entry when the notification is opened with ActivityManager.getAppTasks(). If there isn't an entry we will need to check something Intent to know if that was really what was intended. If not we need re-parse some of the payload and open the desired Activity like we do for Huawei message notification types.
Option 3 - Direct Activity, use ActivityLifecycleCallbacks to get meta data
Opening the Activity directly and the SDK still being able to get the data is possible if we setup the
ActivityLifecycleCallbackson or beforeApplication.onCreateso we don't mis any events.The problem with this is if the target was another app (such as opening a URL in Chrome) this approach would not be able to capture the click.
It is interesting to note that FCM Messaging does use this approach for their SDK. However it looks like they would have the same issue of not being able to track clicks for URLs that opens the browser / another app.
Related
Fixes Issues
Previous PRs
Background
Xiaomi - Background Restrictions
The following was observed through testing on a Xiaomi Redmi 6A running Android 9 on MIUI Global 11.0.8 | Stable 11.0.8.0 (PCBMIXM):
PendingIntents on a notification that include anActivityback stack gets lost, only keeps the top mostActivitywhen it is rehydrated.PendingIntentto start anActivityand then you trampoline to anotherActivityyou MUST callstartActivityBEFORE finishing the firstActivity. Otherwise thestartActivityis ignored.Testing
Unit testing
Most of the unit test changes in this PR are not new, but rather a revert of the ones added and removed in PR #1377.
Manual testing
Default notification
HTTPS URL notification
IAM Preview
Affected code checklist
Checklist
Overview
Testing
Final pass
This change is