From 58df743e9eb9b2d474c6ebbbd24d47a292337fb0 Mon Sep 17 00:00:00 2001 From: Nan Date: Mon, 10 Jun 2024 09:54:44 -0700 Subject: [PATCH 01/10] ApplicationService: Clarify a condition by naming it * There are multiple checks for the conditional `(!isInForeground || nextResumeIsFirstActivity)` used in the Application Service and it is not obvious what this compound condition denotes --- .../core/internal/application/impl/ApplicationService.kt | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/application/impl/ApplicationService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/application/impl/ApplicationService.kt index 6b4247a6db..cc9f481320 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/application/impl/ApplicationService.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/application/impl/ApplicationService.kt @@ -67,6 +67,9 @@ class ApplicationService() : IApplicationService, ActivityLifecycleCallbacks, On private var activityReferences = 0 private var isActivityChangingConfigurations = false + private val wasInBackground: Boolean + get() = !isInForeground || nextResumeIsFirstActivity + /** * Call to "start" this service, expected to be called during initialization of the SDK. * @@ -150,7 +153,7 @@ class ApplicationService() : IApplicationService, ActivityLifecycleCallbacks, On current = activity - if ((!isInForeground || nextResumeIsFirstActivity) && !isActivityChangingConfigurations) { + if (wasInBackground && !isActivityChangingConfigurations) { activityReferences = 1 handleFocus() } else { @@ -170,7 +173,7 @@ class ApplicationService() : IApplicationService, ActivityLifecycleCallbacks, On current = activity } - if ((!isInForeground || nextResumeIsFirstActivity) && !isActivityChangingConfigurations) { + if (wasInBackground && !isActivityChangingConfigurations) { activityReferences = 1 handleFocus() } @@ -373,7 +376,7 @@ class ApplicationService() : IApplicationService, ActivityLifecycleCallbacks, On } private fun handleFocus() { - if (!isInForeground || nextResumeIsFirstActivity) { + if (wasInBackground) { Logging.debug( "ApplicationService.handleFocus: application is now in focus, nextResumeIsFirstActivity=$nextResumeIsFirstActivity", ) From 5ff9c76ab1a41220a6129ac9472b9bcbcf7efe7e Mon Sep 17 00:00:00 2001 From: Nan Date: Mon, 10 Jun 2024 12:18:32 -0700 Subject: [PATCH 02/10] Application Service will fire `onFocus` even when too late * Typically, the app foregrounding will trigger the ApplicationService to fire `onFocus` to its subscribers. * However, it is possible for OneSignal to initialize too late, so the ApplicationService itself is started too late to capture the application's early lifecycle events. * In this case, the app is already foregrounded, so fire a subscriber's `onFocus` callback immediately upon it subscribing, as many services have logic to run in the first onFocus call of a cold start. --- .../internal/application/IApplicationLifecycleHandler.kt | 9 +++++++-- .../core/internal/application/impl/ApplicationService.kt | 9 ++++++++- .../core/internal/background/impl/BackgroundManager.kt | 2 +- .../core/internal/purchases/impl/TrackAmazonPurchase.kt | 2 +- .../core/internal/purchases/impl/TrackGooglePurchase.kt | 2 +- .../inAppMessages/internal/InAppMessagesManager.kt | 2 +- .../internal/controller/impl/GmsLocationController.kt | 2 +- .../internal/controller/impl/HmsLocationController.kt | 2 +- .../internal/permissions/LocationPermissionController.kt | 8 ++++++-- .../notifications/internal/NotificationsManager.kt | 2 +- .../permissions/impl/NotificationPermissionController.kt | 8 ++++++-- 11 files changed, 34 insertions(+), 14 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/application/IApplicationLifecycleHandler.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/application/IApplicationLifecycleHandler.kt index 146396d82b..ff86480635 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/application/IApplicationLifecycleHandler.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/application/IApplicationLifecycleHandler.kt @@ -10,8 +10,13 @@ import android.app.Application.ActivityLifecycleCallbacks interface IApplicationLifecycleHandler { /** * Called when the application is brought into the foreground. + * This callback can be fired immediately on subscribing to the IApplicationService (when the + * IApplicationService itself is started too late to capture the application's early lifecycle events), + * or through natural application lifecycle callbacks. + * + * @param firedOnSubscribe Method is fired from subscribing or from application lifecycle callbacks */ - fun onFocus() + fun onFocus(firedOnSubscribe: Boolean) /** * Called when the application has been brought out of the foreground, to the background. @@ -24,7 +29,7 @@ interface IApplicationLifecycleHandler { * can use this if they only want to override a subset of the callbacks that make up this interface. */ open class ApplicationLifecycleHandlerBase : IApplicationLifecycleHandler { - override fun onFocus() {} + override fun onFocus(firedOnSubscribe: Boolean) {} override fun onUnfocused() {} } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/application/impl/ApplicationService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/application/impl/ApplicationService.kt index cc9f481320..bbb013c50c 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/application/impl/ApplicationService.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/application/impl/ApplicationService.kt @@ -30,6 +30,7 @@ class ApplicationService() : IApplicationService, ActivityLifecycleCallbacks, On private val activityLifecycleNotifier = EventProducer() private val applicationLifecycleNotifier = EventProducer() private val systemConditionNotifier = EventProducer() + private var shouldFireOnFocusOnSubscribing = false override val isInForeground: Boolean get() = entryState.isAppOpen || entryState.isNotificationClick @@ -72,6 +73,7 @@ class ApplicationService() : IApplicationService, ActivityLifecycleCallbacks, On /** * Call to "start" this service, expected to be called during initialization of the SDK. + * Detects if this service should fire subscribers' onFocus() callbacks immediately on subscribing. * * @param context The context the SDK has been initialized under. */ @@ -110,6 +112,8 @@ class ApplicationService() : IApplicationService, ActivityLifecycleCallbacks, On activityReferences = 1 nextResumeIsFirstActivity = false } + // Once listeners subscribe, fire their callbacks + shouldFireOnFocusOnSubscribing = true } else { nextResumeIsFirstActivity = true entryState = AppEntryAction.APP_CLOSE @@ -120,6 +124,9 @@ class ApplicationService() : IApplicationService, ActivityLifecycleCallbacks, On override fun addApplicationLifecycleHandler(handler: IApplicationLifecycleHandler) { applicationLifecycleNotifier.subscribe(handler) + if (shouldFireOnFocusOnSubscribing) { + handler.onFocus(true) + } } override fun removeApplicationLifecycleHandler(handler: IApplicationLifecycleHandler) { @@ -387,7 +394,7 @@ class ApplicationService() : IApplicationService, ActivityLifecycleCallbacks, On entryState = AppEntryAction.APP_OPEN } - applicationLifecycleNotifier.fire { it.onFocus() } + applicationLifecycleNotifier.fire { it.onFocus(false) } } else { Logging.debug("ApplicationService.handleFocus: application never lost focus") } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/background/impl/BackgroundManager.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/background/impl/BackgroundManager.kt index 4cc37d3132..a210056104 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/background/impl/BackgroundManager.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/background/impl/BackgroundManager.kt @@ -79,7 +79,7 @@ internal class BackgroundManager( _applicationService.addApplicationLifecycleHandler(this) } - override fun onFocus() { + override fun onFocus(firedOnSubscribe: Boolean) { cancelSyncTask() } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/purchases/impl/TrackAmazonPurchase.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/purchases/impl/TrackAmazonPurchase.kt index 16658ff4c0..98646a7c88 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/purchases/impl/TrackAmazonPurchase.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/purchases/impl/TrackAmazonPurchase.kt @@ -115,7 +115,7 @@ internal class TrackAmazonPurchase( e.printStackTrace() } - override fun onFocus() { } + override fun onFocus(firedOnSubscribe: Boolean) { } override fun onUnfocused() { if (!canTrack) return diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/purchases/impl/TrackGooglePurchase.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/purchases/impl/TrackGooglePurchase.kt index 6d2f9e66ba..35f95fac5f 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/purchases/impl/TrackGooglePurchase.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/purchases/impl/TrackGooglePurchase.kt @@ -98,7 +98,7 @@ internal class TrackGooglePurchase( trackIAP() } - override fun onFocus() { + override fun onFocus(firedOnSubscribe: Boolean) { trackIAP() } diff --git a/OneSignalSDK/onesignal/in-app-messages/src/main/java/com/onesignal/inAppMessages/internal/InAppMessagesManager.kt b/OneSignalSDK/onesignal/in-app-messages/src/main/java/com/onesignal/inAppMessages/internal/InAppMessagesManager.kt index 978fb6ab2a..9a8a584879 100644 --- a/OneSignalSDK/onesignal/in-app-messages/src/main/java/com/onesignal/inAppMessages/internal/InAppMessagesManager.kt +++ b/OneSignalSDK/onesignal/in-app-messages/src/main/java/com/onesignal/inAppMessages/internal/InAppMessagesManager.kt @@ -883,7 +883,7 @@ internal class InAppMessagesManager( private var onFocusCalled: Boolean = false - override fun onFocus() { + override fun onFocus(firedOnSubscribe: Boolean) { if (onFocusCalled) return onFocusCalled = true suspendifyOnThread { diff --git a/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/GmsLocationController.kt b/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/GmsLocationController.kt index 1c489f1670..2d1ad00402 100644 --- a/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/GmsLocationController.kt +++ b/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/GmsLocationController.kt @@ -175,7 +175,7 @@ internal class GmsLocationController( refreshRequest() } - override fun onFocus() { + override fun onFocus(firedOnSubscribe: Boolean) { Logging.log(LogLevel.DEBUG, "LocationUpdateListener.onFocus()") refreshRequest() } diff --git a/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/HmsLocationController.kt b/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/HmsLocationController.kt index 19adab61d3..98dd1dec80 100644 --- a/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/HmsLocationController.kt +++ b/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/controller/impl/HmsLocationController.kt @@ -161,7 +161,7 @@ internal class HmsLocationController( refreshRequest() } - override fun onFocus() { + override fun onFocus(firedOnSubscribe: Boolean) { Logging.log(LogLevel.DEBUG, "LocationUpdateListener.onFocus()") refreshRequest() } diff --git a/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/permissions/LocationPermissionController.kt b/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/permissions/LocationPermissionController.kt index 4acc34278c..b3a5efa10f 100644 --- a/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/permissions/LocationPermissionController.kt +++ b/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/permissions/LocationPermissionController.kt @@ -107,8 +107,12 @@ internal class LocationPermissionController( // wait for focus to be regained, and check the current permission status. _applicationService.addApplicationLifecycleHandler( object : ApplicationLifecycleHandlerBase() { - override fun onFocus() { - super.onFocus() + override fun onFocus(firedOnSubscribe: Boolean) { + // Triggered by subscribing, wait for lifecycle callback + if (firedOnSubscribe) { + return + } + super.onFocus(false) _applicationService.removeApplicationLifecycleHandler(this) val hasPermission = AndroidUtils.hasPermission(currPermission, true, _applicationService) waiter.wake(hasPermission) diff --git a/OneSignalSDK/onesignal/notifications/src/main/java/com/onesignal/notifications/internal/NotificationsManager.kt b/OneSignalSDK/onesignal/notifications/src/main/java/com/onesignal/notifications/internal/NotificationsManager.kt index 99fe9d3dc1..96a6bad747 100644 --- a/OneSignalSDK/onesignal/notifications/src/main/java/com/onesignal/notifications/internal/NotificationsManager.kt +++ b/OneSignalSDK/onesignal/notifications/src/main/java/com/onesignal/notifications/internal/NotificationsManager.kt @@ -61,7 +61,7 @@ internal class NotificationsManager( } } - override fun onFocus() { + override fun onFocus(firedOnSubscribe: Boolean) { refreshNotificationState() } diff --git a/OneSignalSDK/onesignal/notifications/src/main/java/com/onesignal/notifications/internal/permissions/impl/NotificationPermissionController.kt b/OneSignalSDK/onesignal/notifications/src/main/java/com/onesignal/notifications/internal/permissions/impl/NotificationPermissionController.kt index b60f7700ac..b3feab7320 100644 --- a/OneSignalSDK/onesignal/notifications/src/main/java/com/onesignal/notifications/internal/permissions/impl/NotificationPermissionController.kt +++ b/OneSignalSDK/onesignal/notifications/src/main/java/com/onesignal/notifications/internal/permissions/impl/NotificationPermissionController.kt @@ -150,8 +150,12 @@ internal class NotificationPermissionController( // wait for focus to be regained, and check the current permission status. _applicationService.addApplicationLifecycleHandler( object : ApplicationLifecycleHandlerBase() { - override fun onFocus() { - super.onFocus() + override fun onFocus(firedOnSubscribe: Boolean) { + // Triggered by subscribing, wait for lifecycle callback + if (firedOnSubscribe) { + return + } + super.onFocus(false) _applicationService.removeApplicationLifecycleHandler(this) val hasPermission = AndroidUtils.hasPermission(ANDROID_PERMISSION_STRING, true, _applicationService) waiter.wake(hasPermission) From b08684d05692da61261f302064bc60b507229d71 Mon Sep 17 00:00:00 2001 From: Nan Date: Thu, 6 Jun 2024 11:39:32 -0700 Subject: [PATCH 03/10] session model defaults to invalid * The getter for the session model's validity property will default to `false` to drive the creation of a new session. This is commonly read on new installs. --- .../com/onesignal/session/internal/session/SessionModel.kt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/SessionModel.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/SessionModel.kt index 439d22fe91..66bc15ed99 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/SessionModel.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/SessionModel.kt @@ -19,13 +19,14 @@ class SessionModel : Model() { * Whether the session is valid. */ var isValid: Boolean - get() = getBooleanProperty(::isValid.name) { true } + get() = getBooleanProperty(::isValid.name) { false } set(value) { setBooleanProperty(::isValid.name, value) } /** * When this session started, in Unix time milliseconds. + * This is used by In-App Message triggers, and not used in detecting session time. */ var startTime: Long get() = getLongProperty(::startTime.name) { System.currentTimeMillis() } From 2910223d05d8390fe7d623a58943fb6b16f55afc Mon Sep 17 00:00:00 2001 From: Nan Date: Thu, 6 Jun 2024 11:40:17 -0700 Subject: [PATCH 04/10] session model's focused time defaults to now, not 0 * focusTime is a Unix timestamp, default it to now instead of 0 --- .../java/com/onesignal/session/internal/session/SessionModel.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/SessionModel.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/SessionModel.kt index 66bc15ed99..e49e96c26e 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/SessionModel.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/SessionModel.kt @@ -38,7 +38,7 @@ class SessionModel : Model() { * When this app was last focused, in Unix time milliseconds. */ var focusTime: Long - get() = getLongProperty(::focusTime.name) { 0 } + get() = getLongProperty(::focusTime.name) { System.currentTimeMillis() } set(value) { setLongProperty(::focusTime.name, value) } From 761b4e880f52fb402cbbb5f3ffb1ec1d8e599457 Mon Sep 17 00:00:00 2001 From: Nan Date: Mon, 10 Jun 2024 13:57:57 -0700 Subject: [PATCH 05/10] Session service creates new session on cold start * The SessionService was previously relying on the app backgrounding for 30 seconds to create a new session. It was doing this with the completion of a delayed background runnable. * Now it will also create a new session on all cold starts. * Also fix a bug where a cold start even over 30 seconds was previously not detecting the start of a session correctly. On wrappers, it is common for OneSignal to initialize too late to capture the Android lifecycle callbacks, which was the primary way of session detection. Now, there is enhanced logic to detect this in the onFocus callback and trigger subscribers correctly. Reset session model on cold starts - On cold starts, the app may not be in the background for over 30 seconds, which is when the session time is sent and the session model's `isValid` property is set to false. - Therefore, after the session model is read from cache, reset it's `isValid` property to `false` in order to drive the creation of a new session on cold starts. --- .../internal/session/impl/SessionService.kt | 27 ++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/impl/SessionService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/impl/SessionService.kt index 8f61196fee..c1d5b25780 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/impl/SessionService.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/impl/SessionService.kt @@ -45,10 +45,13 @@ internal class SessionService( private val sessionLifeCycleNotifier: EventProducer = EventProducer() private var session: SessionModel? = null private var config: ConfigModel? = null + private var shouldFireOnSubscribe = false override fun start() { session = _sessionModelStore.model config = _configModelStore.model + // Reset the session validity property to drive a new session + session!!.isValid = false _applicationService.addApplicationLifecycleHandler(this) } @@ -65,11 +68,20 @@ internal class SessionService( sessionLifeCycleNotifier.fire { it.onSessionEnded(session!!.activeDuration) } } - override fun onFocus() { - Logging.log(LogLevel.DEBUG, "SessionService.onFocus()") - + /** + * NOTE: When `firedOnSubscribe = true` + * + * Typically, the app foregrounding will trigger this callback via the IApplicationService. + * However, it is possible for OneSignal to initialize too late to capture the Android lifecycle callbacks. + * In this case, the app is already foregrounded, so this method is fired immediately on subscribing + * to the IApplicationService. Listeners of this service will not subscribe in time to capture + * the `onSessionStarted()` callback here, so fire it when they themselves subscribe. + */ + override fun onFocus(firedOnSubscribe: Boolean) { + Logging.log(LogLevel.DEBUG, "SessionService.onFocus() - fired from start: $firedOnSubscribe") if (!session!!.isValid) { // As the old session was made inactive, we need to create a new session + shouldFireOnSubscribe = firedOnSubscribe session!!.sessionId = UUID.randomUUID().toString() session!!.startTime = _time.currentTimeMillis session!!.focusTime = session!!.startTime @@ -87,14 +99,17 @@ internal class SessionService( } override fun onUnfocused() { - Logging.log(LogLevel.DEBUG, "SessionService.onUnfocused()") - // capture the amount of time the app was focused val dt = _time.currentTimeMillis - session!!.focusTime session!!.activeDuration += dt + Logging.log(LogLevel.DEBUG, "SessionService.onUnfocused adding time $dt for total: ${session!!.activeDuration}") } - override fun subscribe(handler: ISessionLifecycleHandler) = sessionLifeCycleNotifier.subscribe(handler) + override fun subscribe(handler: ISessionLifecycleHandler) { + sessionLifeCycleNotifier.subscribe(handler) + // If a handler subscribes too late to capture the initial onSessionStarted. + if (shouldFireOnSubscribe) handler.onSessionStarted() + } override fun unsubscribe(handler: ISessionLifecycleHandler) = sessionLifeCycleNotifier.unsubscribe(handler) From 974df87fe283631f7c93fc4ef87d055e04e8aa73 Mon Sep 17 00:00:00 2001 From: Nan Date: Mon, 10 Jun 2024 13:59:25 -0700 Subject: [PATCH 06/10] Session service change when it resets active duration - Now that cold starts will trigger a new session, the logic for storing active duration is updated. - Instead of active duration being reset to 0 at the start of every new session, it will reset to 0 when the session time is sent to the server. This is because cold starts may have stopped the old session time from sending, so we should not overwrite by resetting to 0 in the start method. - Remove the isValid check in the backgroundRun method. This is because a force killed app will initialize OneSignal to run this background task. By initializing OneSignal, the SDK believes this could be a cold start and set the isValid property on the session model to `false`. However, we want to send off this accumulated session time to the server, and that check would return early. - Additionally, the only listener of the onSessionEnded callback is the SessionListener. It will return early when session time is invalid. We have a similar guard in player model. --- .../internal/session/impl/SessionListener.kt | 10 ++++++++++ .../session/internal/session/impl/SessionService.kt | 13 ++++--------- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/impl/SessionListener.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/impl/SessionListener.kt index 265be9f82e..4cd8759a0a 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/impl/SessionListener.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/impl/SessionListener.kt @@ -47,6 +47,12 @@ internal class SessionListener( override fun onSessionEnded(duration: Long) { val durationInSeconds = duration / 1000 + + // Time is invalid if below 1 second or over a day + if (durationInSeconds < 1L || durationInSeconds > SECONDS_IN_A_DAY) { + return + } + _operationRepo.enqueue( TrackSessionEndOperation(_configModelStore.model.appId, _identityModelStore.model.onesignalId, durationInSeconds), ) @@ -55,4 +61,8 @@ internal class SessionListener( _outcomeEventsController.sendSessionEndOutcomeEvent(durationInSeconds) } } + + companion object { + const val SECONDS_IN_A_DAY = 86_400L + } } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/impl/SessionService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/impl/SessionService.kt index c1d5b25780..d831ef54b1 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/impl/SessionService.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/impl/SessionService.kt @@ -56,16 +56,12 @@ internal class SessionService( } override suspend fun backgroundRun() { - Logging.log(LogLevel.DEBUG, "SessionService.backgroundRun()") - - if (!session!!.isValid) { - return - } - + val activeDuration = session!!.activeDuration // end the session - Logging.debug("SessionService: Session ended. activeDuration: ${session!!.activeDuration}") + Logging.debug("SessionService.backgroundRun: Session ended. activeDuration: $activeDuration") session!!.isValid = false - sessionLifeCycleNotifier.fire { it.onSessionEnded(session!!.activeDuration) } + sessionLifeCycleNotifier.fire { it.onSessionEnded(activeDuration) } + session!!.activeDuration = 0L } /** @@ -85,7 +81,6 @@ internal class SessionService( session!!.sessionId = UUID.randomUUID().toString() session!!.startTime = _time.currentTimeMillis session!!.focusTime = session!!.startTime - session!!.activeDuration = 0L session!!.isValid = true Logging.debug("SessionService: New session started at ${session!!.startTime}") From 6c6c0b53755cec3540ecc0f93e463bc70ff2054d Mon Sep 17 00:00:00 2001 From: Nan Date: Thu, 6 Jun 2024 12:43:48 -0700 Subject: [PATCH 07/10] Refresh user on new sessions (not just cold start) * New sessions can happen when the app is backgrounded over 30 seconds. Let's refresh the user then, not just when the app is cold started. * When UserRefreshService starts up, it subscribes to the SessionService who will notify it when a new session is started either from backgrounding or from a cold start. * The user will still be refreshed only when the app is in the foreground. --- .../internal/service/UserRefreshService.kt | 30 ++++++++----------- 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/service/UserRefreshService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/service/UserRefreshService.kt index 44f90cdffe..7b04d7981e 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/service/UserRefreshService.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/service/UserRefreshService.kt @@ -1,28 +1,32 @@ package com.onesignal.user.internal.service import com.onesignal.common.IDManager -import com.onesignal.core.internal.application.IApplicationLifecycleHandler import com.onesignal.core.internal.application.IApplicationService import com.onesignal.core.internal.config.ConfigModelStore import com.onesignal.core.internal.operations.IOperationRepo import com.onesignal.core.internal.startup.IStartableService +import com.onesignal.session.internal.session.ISessionLifecycleHandler +import com.onesignal.session.internal.session.ISessionService import com.onesignal.user.internal.identity.IdentityModelStore import com.onesignal.user.internal.operations.RefreshUserOperation -// Ensure cache for the user is refreshed once per cold start when app +// Ensure user is refreshed only when app // is in the foreground. This saves resources as there are a number of // events (such as push received or non-OneSignal events) that start // the app in the background but will never read/write any user // properties. class UserRefreshService( private val _applicationService: IApplicationService, + private val _sessionService: ISessionService, private val _operationRepo: IOperationRepo, private val _configModelStore: ConfigModelStore, private val _identityModelStore: IdentityModelStore, ) : IStartableService, - IApplicationLifecycleHandler { + ISessionLifecycleHandler { private fun refreshUser() { - if (IDManager.isLocalId(_identityModelStore.model.onesignalId)) return + if (IDManager.isLocalId(_identityModelStore.model.onesignalId) || !_applicationService.isInForeground) { + return + } _operationRepo.enqueue( RefreshUserOperation( @@ -32,21 +36,11 @@ class UserRefreshService( ) } - override fun start() { - if (_applicationService.isInForeground) { - refreshUser() - } else { - _applicationService.addApplicationLifecycleHandler(this) - } - } + override fun start() = _sessionService.subscribe(this) - private var onFocusCalled: Boolean = false + override fun onSessionStarted() = refreshUser() - override fun onFocus() { - if (onFocusCalled) return - onFocusCalled = true - refreshUser() - } + override fun onSessionActive() { } - override fun onUnfocused() { } + override fun onSessionEnded(duration: Long) { } } From 69965cb88e4bed013baaba90dd257ead466ad75b Mon Sep 17 00:00:00 2001 From: Nan Date: Mon, 10 Jun 2024 14:13:05 -0700 Subject: [PATCH 08/10] IAM manager doesn't need to check for onFocus in addition to onSessionStarted * The onSessionStarted callback already will fetch messages. * Now that session starting (including cold starts) should always trigger the subscribers, there is no need to also fetch messages within onFocus --- .../inAppMessages/internal/InAppMessagesManager.kt | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/OneSignalSDK/onesignal/in-app-messages/src/main/java/com/onesignal/inAppMessages/internal/InAppMessagesManager.kt b/OneSignalSDK/onesignal/in-app-messages/src/main/java/com/onesignal/inAppMessages/internal/InAppMessagesManager.kt index 9a8a584879..2e70f1c07a 100644 --- a/OneSignalSDK/onesignal/in-app-messages/src/main/java/com/onesignal/inAppMessages/internal/InAppMessagesManager.kt +++ b/OneSignalSDK/onesignal/in-app-messages/src/main/java/com/onesignal/inAppMessages/internal/InAppMessagesManager.kt @@ -881,15 +881,7 @@ internal class InAppMessagesManager( .show() } - private var onFocusCalled: Boolean = false - - override fun onFocus(firedOnSubscribe: Boolean) { - if (onFocusCalled) return - onFocusCalled = true - suspendifyOnThread { - fetchMessages() - } - } + override fun onFocus(firedOnSubscribe: Boolean) { } override fun onUnfocused() { } } From 5aa61f7bcf0434268bceedc2e6dd479c39f0fe5b Mon Sep 17 00:00:00 2001 From: Nan Date: Fri, 7 Jun 2024 08:52:12 -0700 Subject: [PATCH 09/10] [tests] Update Session and Application Service Tests * Update existing tests * Extract helper methods them into a helper class for reuse * SessionServiceTests - Add test "session created in start when application is in the foreground" * ApplicationServiceTests - Add test "focus will occur on subscribe when activity is already started" --- .../application/ApplicationServiceTests.kt | 22 ++- .../internal/session/SessionServiceTests.kt | 133 ++++++++++-------- 2 files changed, 99 insertions(+), 56 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/application/ApplicationServiceTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/application/ApplicationServiceTests.kt index 39ae8251ce..cd9f3d1712 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/application/ApplicationServiceTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/application/ApplicationServiceTests.kt @@ -175,7 +175,27 @@ class ApplicationServiceTests : FunSpec({ // Then currentActivity shouldBe activity2 verify(exactly = 1) { mockApplicationLifecycleHandler.onUnfocused() } - verify(exactly = 1) { mockApplicationLifecycleHandler.onFocus() } + verify(exactly = 1) { mockApplicationLifecycleHandler.onFocus(false) } + } + + test("focus will occur on subscribe when activity is already started") { + // Given + val activity: Activity + + Robolectric.buildActivity(Activity::class.java).use { controller -> + controller.setup() // Moves Activity to RESUMED state + activity = controller.get() + } + + val applicationService = ApplicationService() + val mockApplicationLifecycleHandler = spyk() + + // When + applicationService.start(activity) + applicationService.addApplicationLifecycleHandler(mockApplicationLifecycleHandler) + + // Then + verify(exactly = 1) { mockApplicationLifecycleHandler.onFocus(true) } } test("wait until system condition returns false when there is no activity") { diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/session/internal/session/SessionServiceTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/session/internal/session/SessionServiceTests.kt index f1d3a1e1ea..2a912d69e6 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/session/internal/session/SessionServiceTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/session/internal/session/SessionServiceTests.kt @@ -7,60 +7,89 @@ import io.kotest.matchers.shouldBe import io.mockk.spyk import io.mockk.verify -class SessionServiceTests : FunSpec({ +// Mocks used by every test in this file +private class Mocks { + val currentTime = 1111L + + private val mockSessionModelStore = MockHelper.sessionModelStore() + + fun sessionModelStore(action: ((SessionModel) -> Unit)? = null): SessionModelStore { + if (action != null) action(mockSessionModelStore.model) + return mockSessionModelStore + } + val sessionService = SessionService(MockHelper.applicationService(), MockHelper.configModelStore(), mockSessionModelStore, MockHelper.time(currentTime)) + + val spyCallback = spyk() +} + +class SessionServiceTests : FunSpec({ test("session created on focus when current session invalid") { // Given - val currentTime = 1111L + val mocks = Mocks() + val sessionService = mocks.sessionService - val configModelStoreMock = MockHelper.configModelStore() - val sessionModelStoreMock = - MockHelper.sessionModelStore { - it.isValid = false - } - val spyCallback = spyk() + sessionService.start() + val sessionModelStore = mocks.sessionModelStore { it.isValid = false } + sessionService.subscribe(mocks.spyCallback) + + // When + sessionService.onFocus(false) + + // Then + sessionModelStore.model.isValid shouldBe true + sessionModelStore.model.startTime shouldBe mocks.currentTime + sessionModelStore.model.focusTime shouldBe mocks.currentTime + verify(exactly = 1) { mocks.spyCallback.onSessionStarted() } + } + + test("session created in start when application is in the foreground") { + // Given + val mocks = Mocks() + val sessionService = mocks.sessionService + val sessionModelStore = mocks.sessionModelStore() - val sessionService = - SessionService(MockHelper.applicationService(), configModelStoreMock, sessionModelStoreMock, MockHelper.time(currentTime)) + // When sessionService.start() - sessionService.subscribe(spyCallback) + sessionService.onFocus(true) + sessionService.subscribe(mocks.spyCallback) + + // Then + sessionModelStore.model.isValid shouldBe true + sessionModelStore.model.startTime shouldBe mocks.currentTime + sessionModelStore.model.focusTime shouldBe mocks.currentTime + verify(exactly = 1) { mocks.spyCallback.onSessionStarted() } // When - sessionService.onFocus() + sessionService.onFocus(false) // Should not trigger a second session // Then - sessionModelStoreMock.model.isValid shouldBe true - sessionModelStoreMock.model.startTime shouldBe currentTime - sessionModelStoreMock.model.focusTime shouldBe currentTime - verify(exactly = 1) { spyCallback.onSessionStarted() } + verify(exactly = 1) { mocks.spyCallback.onSessionStarted() } } test("session focus time updated when current session valid") { // Given - val currentTime = 1111L val startTime = 555L + val mocks = Mocks() + val sessionService = mocks.sessionService - val configModelStoreMock = MockHelper.configModelStore() - val sessionModelStoreMock = - MockHelper.sessionModelStore { - it.isValid = true + sessionService.start() + val sessionModelStore = + mocks.sessionModelStore { it.startTime = startTime + it.isValid = true } - val spyCallback = spyk() - - val sessionService = - SessionService(MockHelper.applicationService(), configModelStoreMock, sessionModelStoreMock, MockHelper.time(currentTime)) - sessionService.start() - sessionService.subscribe(spyCallback) + sessionService.subscribe(mocks.spyCallback) // When - sessionService.onFocus() + sessionService.onFocus(false) // Then - sessionModelStoreMock.model.isValid shouldBe true - sessionModelStoreMock.model.startTime shouldBe startTime - sessionModelStoreMock.model.focusTime shouldBe currentTime - verify(exactly = 1) { spyCallback.onSessionActive() } + sessionModelStore.model.isValid shouldBe true + sessionModelStore.model.startTime shouldBe startTime + sessionModelStore.model.focusTime shouldBe mocks.currentTime + verify(exactly = 1) { mocks.spyCallback.onSessionActive() } + verify(exactly = 0) { mocks.spyCallback.onSessionStarted() } } test("session active duration updated when unfocused") { @@ -68,53 +97,47 @@ class SessionServiceTests : FunSpec({ val startTime = 555L val focusTime = 666L val startingDuration = 1000L - val currentTime = 1111L - val configModelStoreMock = MockHelper.configModelStore() - val sessionModelStoreMock = - MockHelper.sessionModelStore { + val mocks = Mocks() + val sessionService = mocks.sessionService + + sessionService.start() + val sessionModelStore = + mocks.sessionModelStore { it.isValid = true it.startTime = startTime it.focusTime = focusTime it.activeDuration = startingDuration } - val sessionService = - SessionService(MockHelper.applicationService(), configModelStoreMock, sessionModelStoreMock, MockHelper.time(currentTime)) - sessionService.start() - // When sessionService.onUnfocused() // Then - sessionModelStoreMock.model.isValid shouldBe true - sessionModelStoreMock.model.startTime shouldBe startTime - sessionModelStoreMock.model.activeDuration shouldBe startingDuration + (currentTime - focusTime) + sessionModelStore.model.isValid shouldBe true + sessionModelStore.model.startTime shouldBe startTime + sessionModelStore.model.activeDuration shouldBe startingDuration + (mocks.currentTime - focusTime) } test("session ended when background run") { // Given val activeDuration = 555L - val currentTime = 1111L + val mocks = Mocks() + val sessionService = mocks.sessionService - val configModelStoreMock = MockHelper.configModelStore() - val sessionModelStoreMock = - MockHelper.sessionModelStore { + sessionService.start() + val sessionModelStore = + mocks.sessionModelStore { it.isValid = true it.activeDuration = activeDuration } - val spyCallback = spyk() - - val sessionService = - SessionService(MockHelper.applicationService(), configModelStoreMock, sessionModelStoreMock, MockHelper.time(currentTime)) - sessionService.start() - sessionService.subscribe(spyCallback) + sessionService.subscribe(mocks.spyCallback) // When sessionService.backgroundRun() // Then - sessionModelStoreMock.model.isValid shouldBe false - verify(exactly = 1) { spyCallback.onSessionEnded(activeDuration) } + sessionModelStore.model.isValid shouldBe false + verify(exactly = 1) { mocks.spyCallback.onSessionEnded(activeDuration) } } }) From 9b582e00f7b771b124b701f5cf0a095c23850a6f Mon Sep 17 00:00:00 2001 From: Nan Date: Wed, 12 Jun 2024 14:41:43 -0700 Subject: [PATCH 10/10] address review comments * Still send likely erroneous session time. This way the server can be aware of any issues. * Simplify state when firing subscribers of the Application Service --- .../core/internal/application/impl/ApplicationService.kt | 8 +++----- .../session/internal/session/impl/SessionListener.kt | 5 +++-- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/application/impl/ApplicationService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/application/impl/ApplicationService.kt index bbb013c50c..55f2612324 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/application/impl/ApplicationService.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/application/impl/ApplicationService.kt @@ -30,7 +30,6 @@ class ApplicationService() : IApplicationService, ActivityLifecycleCallbacks, On private val activityLifecycleNotifier = EventProducer() private val applicationLifecycleNotifier = EventProducer() private val systemConditionNotifier = EventProducer() - private var shouldFireOnFocusOnSubscribing = false override val isInForeground: Boolean get() = entryState.isAppOpen || entryState.isNotificationClick @@ -73,7 +72,6 @@ class ApplicationService() : IApplicationService, ActivityLifecycleCallbacks, On /** * Call to "start" this service, expected to be called during initialization of the SDK. - * Detects if this service should fire subscribers' onFocus() callbacks immediately on subscribing. * * @param context The context the SDK has been initialized under. */ @@ -112,8 +110,6 @@ class ApplicationService() : IApplicationService, ActivityLifecycleCallbacks, On activityReferences = 1 nextResumeIsFirstActivity = false } - // Once listeners subscribe, fire their callbacks - shouldFireOnFocusOnSubscribing = true } else { nextResumeIsFirstActivity = true entryState = AppEntryAction.APP_CLOSE @@ -124,7 +120,9 @@ class ApplicationService() : IApplicationService, ActivityLifecycleCallbacks, On override fun addApplicationLifecycleHandler(handler: IApplicationLifecycleHandler) { applicationLifecycleNotifier.subscribe(handler) - if (shouldFireOnFocusOnSubscribing) { + if (current != null) { + // When a listener subscribes, fire its callback + // The listener is too late to receive the earlier onFocus call handler.onFocus(true) } } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/impl/SessionListener.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/impl/SessionListener.kt index 4cd8759a0a..9669e05de5 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/impl/SessionListener.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/impl/SessionListener.kt @@ -4,6 +4,7 @@ import com.onesignal.common.threading.suspendifyOnThread import com.onesignal.core.internal.config.ConfigModelStore import com.onesignal.core.internal.operations.IOperationRepo import com.onesignal.core.internal.startup.IStartableService +import com.onesignal.debug.internal.logging.Logging import com.onesignal.session.internal.outcomes.IOutcomeEventsController import com.onesignal.session.internal.session.ISessionLifecycleHandler import com.onesignal.session.internal.session.ISessionService @@ -48,9 +49,9 @@ internal class SessionListener( override fun onSessionEnded(duration: Long) { val durationInSeconds = duration / 1000 - // Time is invalid if below 1 second or over a day + // Time is erroneous if below 1 second or over a day if (durationInSeconds < 1L || durationInSeconds > SECONDS_IN_A_DAY) { - return + Logging.error("SessionListener.onSessionEnded sending duration of $durationInSeconds seconds") } _operationRepo.enqueue(