customerio / customerio-android

This is the official Customer.io SDK for Android.
MIT License
13 stars 9 forks source link

šŸ”µ Donā€™t always restart the app whenever a push is opened #179

Closed tfcporciuncula closed 11 months ago

tfcporciuncula commented 1 year ago

Is your feature request related to a problem? Please describe. If users open a push with the app open, the app restarts. That means discarding whatever state and progress users had in the app, and potentially re-running unnecessary app start logic. We donā€™t see a reason for having this default behavior and we believe the app should be the one in control of what happens with the navigation stack when a push notification is opened. Weā€™d love to hear from your end if thereā€™s any reason for the existing behavior, but itā€™s currently not bringing any benefits for us.

Describe the solution you'd like The root cause is here:

private val notificationIntentFlags: Int = Intent.FLAG_ACTIVITY_NEW_TASK or
    Intent.FLAG_ACTIVITY_CLEAR_TOP or Intent.FLAG_ACTIVITY_SINGLE_TOP

So the easiest solution is to simply drop these flags.

Describe alternatives you've considered In order to keep the current behavior for existing clients, you could introduce a way to configure this so clients can opt out of these flags.

Additional context Right now the iOS SDK doesnā€™t restart the iOS app whenever a push is opened, so this change would make the SDK behavior more consistent across platforms.

Shahroz16 commented 1 year ago

Hey @tfcporciuncula, thank you for reaching out. I believe one of the flags was added to trigger activity from service, others were added to be flexible around deep linking and handle more rather than relying on the user.

We will investigate more about the use case for which these flags were added and either remove them or try to tackle them in an alternative manner. Thank you for patience while we resolve this for you.

tfcporciuncula commented 1 year ago

Thanks @Shahroz16! I understand there might have a need for the FLAG_ACTIVITY_NEW_TASK, so if we keep it but drop the other two (especially FLAG_ACTIVITY_CLEAR_TOP) it would be fine for us.

Shahroz16 commented 1 year ago

Hey @tfcporciuncula, I am back with a snapshot build for you to test and see if it will work for you. You can find it in repository

 maven { 
            url 'https://s01.oss.sonatype.org/content/repositories/snapshots/'
        }

and you can access the release by adding the dependency,

    def cioVersion = "feat-push-intent-flags-SNAPSHOT"
    implementation "io.customer.android:tracking:$cioVersion"
    implementation "io.customer.android:messaging-push-fcm:$cioVersion"
    implementation "io.customer.android:messaging-in-app:$cioVersion"

And this is how you can override the intent flags

  CustomerIO.Builder(
            siteId = siteId ?: BuildConfig.SITE_ID,
            apiKey = apiKey ?: BuildConfig.API_KEY,
            appContext = application
        ).apply {
            addCustomerIOModule(ModuleMessagingInApp())
            addCustomerIOModule(
                ModuleMessagingPushFCM(
                    config = MessagingPushModuleConfig.Builder().apply {
                        setNotificationCallback(this@CustomerIOInitializer)
                    }.build()
                )
            )
            setLogLevel(CioLogLevel.DEBUG)
            build()
        }

        // the overridden method
        override fun getDeepLinkIntentFlags(intent: Intent, uri: Uri): Int? {
        // whatever flag you want to add
        // note, sending it null would just use the default ones
        return Intent.FLAG_ACTIVITY_SINGLE_TOP
    }

Once you verify, it resolves your use case, I'll proceed with it. Thank you and please feel free to reach out, in case there is any clarification/support needed.

tfcporciuncula commented 1 year ago

Thanks a lot for the snapshot build, it was incredibly helpful! Thanks to that I realized I misjudged the root cause of our issue, I apologize šŸ˜ž even without the flags our app is still restarting, and after investigating further I confirmed the root case is actually this.

The TaskStackBuilder is whatā€™s actually forcing a restart of our app, even if we remove all flags and even if we set our (single) activity to be singleTask or singleInstance. I still think allowing clients to customize the flags might be a beneficial change, but itā€™s not solving our issue so we may drop it as well.

I see weā€™re only using TaskStackBuilder when the app targets Android 12 and above, and that this was introduced when fixing deep link on Android 12. Looking at the PR, thereā€™s this explanation:

For apps targeting Android 12 or greater, updated CustomerIOPushNotificationHandler to create PendingIntent for correct navigation instead of starting CustomerIOPushReceiver

Do you know exactly what ā€œcorrect navigationā€ means here and what was the issue before the TaskStackBuilder was introduced? The real solution for us would be to replace it with a PendingIntent.getActivity() call instead for creating the PendingIntent. Based on what the docs say here, I think it would make sense for the SDK to support this.

Shahroz16 commented 1 year ago

Hey @tfcporciuncula, thank you for getting back and apologies that I missed the notification for the response. I think your diagnosis is correct regarding what might the root cause be, while implementing we also considered this to be the root cause rather than the flags.

Let me explain the problem in a bit of detail, From Android 12 onwards the concept of notifications trampoline has been introduced which doesn't let us track push metrics or open deep links the same way as we did pre-12. That is to add a pending intent to notification and run a broadcast/service that will trigger and run for us and we can perform operations in it.

Android 12 and onwards now Google recommends following the Application lifecycle. So when the application gets launched, we will get the desired information from data inside the intent. Now the only way, SDK can get that information is by following the application lifecycle. So we observe the application and in the onCreate, we check if data in available in intent and do the operation of push metrics tracking, deep linking, etc.

Hence we introduced the concept of TaskStackBuilder which is recommended by Google for push notification navigation handling for regular intents and relaunch the application so we don't miss out on the opened metric tracking and correct deep linking.

Thank you for your suggestion, we will look into it and see how we can utilize it or maybe try to get intent data on any other lifecycle event, but it might require some RnD and we will need to make sure it doesn't break the deep link navigation.

Shahroz16 commented 1 year ago

@tfcporciuncula thank you for your patience, so we looked into it more and tried a couple of things even your suggestion but in all those cases, Android 12 and above, "opened" event tracking doesn't give 100% results. Because of the reason explained earlier, we have to rely on the application lifecycle for open tracking of push Android 12 and onwards.

We can provide a way to do it, but then that would mean moving the responsibility of opened metric tracking onto the customer/application. This is another solution, but not ideal because it could lead to skewed metrics if the customers fail to comply.

So, I am going to keep this ticket open as a reminder that we need to come back to this but this will go into our roadmap. Thank you again for raising this.

tfcporciuncula commented 1 year ago

Thanks for looking into it. It would be great if we're able to decouple the tracking with the deep link handling, since at the end of the day for us we're basically stuck always restarting the app when a push is open just because otherwise we don't get reliable tracking. Thanks for keeping this open!

Shahroz16 commented 11 months ago

Changes are available as follows:

Android: 3.8.0 (Doc updates) React Native: 3.3.2 (Doc updates) Flutter: 1.3.1 (Doc updates)