Skip to content
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

Reverse Activity Trampolining Implementation #1377

Merged
merged 17 commits into from
Aug 12, 2021

Conversation

jkasten2
Copy link
Member

@jkasten2 jkasten2 commented Jul 9, 2021

Description

One Line Summary

Moved the logic of "should we open the app" from on notification tapped time to notification generation time.

Details

Pre-existing code & Android 12 restrictions

This PR focuses on providing as direct as possible path from notification open to showing the user an Activity. This change was inspired by the Notification trampoline restrictions in Android 12 which to quote this Google page was done to "To improve app performance and UX ...".
Technically the OneSignal-Android-SDK 4.0.0 and newer already meets the requirements as the notification open is starting an Activity. However this is an invisible Activity that conditionally shows another visible Activity after some runtime processing, which in spirit is basically the same as Broadcast or Service Google is trying to prevent here.

Limited Android Notification API

OneSignal starts an invisible Activity (NotificationOpenedReceiver) so it can know the user opened the notifications to provide click counts, outcomes, etc on it. This is done so this is automatic and requires zero extra code / configuration by the app developer. Android does not provide a way to attach both an Activity and a background job (Broadcast, Service, etc) to support this without some kind of trampolining.

Solution implemented - Reverse Activity Trampolining

As noted above, Android does not allow attaching both an Activity and a background Intent, however it does provide an API to attach multiple Activities with PendingIntent.getActivities. This means we can have Android Launch both our invisible NotificationOpenedReceiver Activity to track the even while also attaching the visible Activity the app developer wants the user to be shown. You can call this Reverse Activity Trampolining as the OneSignal tracking Activity will be created first but as soon as it is finished Android show the next one in the back stack automatically, hence reverse in the name.

Changes

OSNotificationOpenAppSettings - New class to read global settings for how notification opens behave.
OSNotificationOpenBehaviorFromPushPayload - Based on the notification payload and OSNotificationOpenAppSettings determine if the notification should open the app or not or if it should open a URL.
GenerateNotificationOpenIntent - This logic was moved from GenerateNotification.java and has additions to support Reverse Activity Trampolining.
GenerateNotificationOpenIntentFromPushPayload - This uses OSNotificationOpenBehaviorFromPushPayload to create a GenerateNotificationOpenIntent instance. This gets consumed by GenerateNotification.java.
OneSignal.java - The startActivity logic has been pulled out since it has been moved to notification generation time.

Fixes

Future

Interduce a OneSignal API to the OneSignal NotificationServiceExtension to allow setting an Activity Intent to control which Activity is shown directly at the notification generation time.
Today if the developer does not want to first show their launcher Activity they have to set "com.onesignal.NotificationOpened.DEFAULT" in their AndroidManifest.xml and then use startActivity from the OneSignal notification open event. Basically the benefits from this PR can't be used when the developer needs to make this customization until OneSignal adds this feature.


This change is Reviewable

@jkasten2 jkasten2 added the WIP Work In Progress label Jul 9, 2021
* This commit contains the working implementation for Reverse Activity
Trampolining Implementation.
   - Other considerations such as outcomes will
   be done in a follow up commit as part of this PR.
* From a high level we are moving the logic of "should we open the app"
to notification generation time instead of at click time.
* The way this is being done is deciding if we want to display two
Activities (OneSignal's invisible click tracking + app's launcher) or
just OneSignal's Activity.
* Comments added to the code in this commit go into more details.
* Shorter name and flips the logic to make it easier to read.
* Putting the GenerateNotificationOpenIntent class added a previous
commit to use.
* Removed getNewBaseIntent & getNewActionPendingIntent from
GenerateNotification.java as it was moved to
GenerateNotificationOpenIntent
* Moved these tests to GenerateNotificationRunner to match the source
logic changes.
* Split logic from GenerateNotificationOpenIntentFromPushPayload into
OSNotificationOpenBehaviorFromPushPayload so this can be used independly
in a follow up commit
* Added this logic back as some app developers may set them from the
notification opened callback.
   - The fallback implemention of waiting for the app resume event
   doesn't catch this case.
* This logic was moved tot the notification generation as is already
covered by tests there.
* Renamed to getShouldOpenActivity as getOpenApp was incorrectly
describing the behavior
* Fixed usage of startApp
* Added getShouldOpenActivity usage to uri
@jkasten2 jkasten2 force-pushed the feat/reverse_activity_trampolining branch from 088cfb0 to bba36be Compare August 10, 2021 21:29
* FIxed issue where summary notification did not update with all
data from the payload when a child notification is removed.
Copy link
Contributor

@emawby emawby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 14 files at r2.
Reviewable status: 6 of 14 files reviewed, 2 unresolved discussions (waiting on @jkasten2 and @nan-li)


OneSignalSDK/onesignal/src/main/java/com/onesignal/GenerateNotificationOpenIntent.kt, line 77 at r2 (raw file):

            )

        // Launch desired Activity we want the user to be take to the followed by

Lets clarify this comment


OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotificationOpenBehaviorFromPushPayload.kt, line 15 at r2 (raw file):

            if (!OSNotificationOpenAppSettings.getShouldOpenActivity(context)) return false
            if (uri != null) return false
            return true

Does the following work for this:
(OSNotificationOpenAppSettings.getShouldOpenActivity(context) && uri == null)

Copy link
Member Author

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 6 of 14 files reviewed, 2 unresolved discussions (waiting on @emawby and @nan-li)


OneSignalSDK/onesignal/src/main/java/com/onesignal/GenerateNotificationOpenIntent.kt, line 77 at r2 (raw file):

Previously, emawby (Elliot Mawby) wrote…

Lets clarify this comment

Wrote this comment section to so should be more clear now.


OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotificationOpenBehaviorFromPushPayload.kt, line 15 at r2 (raw file):

Previously, emawby (Elliot Mawby) wrote…
            if (!OSNotificationOpenAppSettings.getShouldOpenActivity(context)) return false
            if (uri != null) return false
            return true

Does the following work for this:
(OSNotificationOpenAppSettings.getShouldOpenActivity(context) && uri == null)

Yes! That makes this a lot easier to read. Done

Copy link
Contributor

@emawby emawby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 6 of 14 files reviewed, all discussions resolved (waiting on @nan-li)

@jkasten2 jkasten2 merged commit d61f3c9 into main Aug 12, 2021
@jkasten2 jkasten2 deleted the feat/reverse_activity_trampolining branch August 12, 2021 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants