-
Notifications
You must be signed in to change notification settings - Fork 377
Fix IAM display rendering issues on foldable Android devices (Samsung Galaxy Fold/Flip) #2597
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| package com.onesignal.common | ||
|
|
||
| import com.onesignal.debug.internal.logging.Logging | ||
|
|
||
| /** | ||
| * Global feature switch for foldable device IAM display improvements. | ||
| * When enabled, uses modern WindowMetrics API and detects screen size changes. | ||
| */ | ||
| internal object FoldableIAMFeature { | ||
| @Volatile | ||
| var isEnabled: Boolean = false | ||
| private set | ||
|
|
||
| fun updateEnabled( | ||
| enabled: Boolean, | ||
| source: String, | ||
| ) { | ||
| val previous = isEnabled | ||
| isEnabled = enabled | ||
|
|
||
| if (previous != enabled) { | ||
| Logging.info("OneSignal: FoldableIAMFeature changed to isEnabled=$enabled (source=$source)") | ||
| } else { | ||
| Logging.debug("OneSignal: FoldableIAMFeature unchanged (isEnabled=$enabled, source=$source)") | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ | |
| import androidx.fragment.app.FragmentManager | ||
| import com.onesignal.common.AndroidUtils | ||
| import com.onesignal.common.DeviceUtils | ||
| import com.onesignal.common.FoldableIAMFeature | ||
| import com.onesignal.common.events.EventProducer | ||
| import com.onesignal.common.threading.Waiter | ||
| import com.onesignal.core.internal.application.ActivityLifecycleHandlerBase | ||
|
|
@@ -83,6 +84,9 @@ | |
|
|
||
| val configuration = | ||
| object : ComponentCallbacks { | ||
| private var lastScreenWidthDp: Int = 0 | ||
| private var lastScreenHeightDp: Int = 0 | ||
|
|
||
| override fun onConfigurationChanged(newConfig: Configuration) { | ||
| // If Activity contains the configChanges orientation flag, re-create the view this way | ||
| if (current != null && | ||
|
|
@@ -93,6 +97,28 @@ | |
| ) { | ||
| onOrientationChanged(newConfig.orientation, current!!) | ||
| } | ||
|
|
||
| // Handle foldable device screen size changes (fold/unfold events) | ||
| // Only enabled when FoldableIAMFeature is on | ||
| // Foldable devices trigger CONFIG_SCREEN_SIZE without orientation change | ||
| if (FoldableIAMFeature.isEnabled && current != null && hasScreenSizeChanged(newConfig)) { | ||
| Logging.debug( | ||
| "ApplicationService.onConfigurationChanged: Screen size changed " + | ||
| "(foldable device fold/unfold detected) - " + | ||
| "width: ${newConfig.screenWidthDp}dp, height: ${newConfig.screenHeightDp}dp", | ||
| ) | ||
|
Check failure on line 109 in OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/application/impl/ApplicationService.kt
|
||
| onScreenSizeChanged(current!!) | ||
| } | ||
|
Comment on lines
90
to
+111
|
||
| lastScreenWidthDp = newConfig.screenWidthDp | ||
| lastScreenHeightDp = newConfig.screenHeightDp | ||
| } | ||
|
|
||
| private fun hasScreenSizeChanged(newConfig: Configuration): Boolean { | ||
| if (lastScreenWidthDp == 0 && lastScreenHeightDp == 0) { | ||
| return false | ||
| } | ||
| return newConfig.screenWidthDp != lastScreenWidthDp || | ||
| newConfig.screenHeightDp != lastScreenHeightDp | ||
| } | ||
|
Comment on lines
+116
to
122
|
||
|
|
||
| override fun onLowMemory() {} | ||
|
|
@@ -368,6 +394,23 @@ | |
| handleFocus() | ||
| } | ||
|
|
||
| /** | ||
| * Handles screen size changes that occur on foldable devices when folding/unfolding. | ||
| * Unlike orientation changes, foldable devices can change screen dimensions significantly | ||
| * without changing orientation (e.g., Samsung Galaxy Fold going from cover screen to main screen). | ||
| * This triggers the same view recreation flow as orientation changes to ensure IAMs are | ||
| * properly resized and repositioned. | ||
| */ | ||
| private fun onScreenSizeChanged(activity: Activity) { | ||
| // Remove view | ||
| activityLifecycleNotifier.fire { it.onActivityStopped(activity) } | ||
|
|
||
| // Show view with new dimensions | ||
| activityLifecycleNotifier.fire { it.onActivityAvailable(activity) } | ||
|
|
||
| activity.window.decorView.viewTreeObserver.addOnGlobalLayoutListener(this) | ||
|
Check warning on line 411 in OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/application/impl/ApplicationService.kt
|
||
|
Comment on lines
+405
to
+411
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The new Extended reasoning...What
activity.window.decorView.viewTreeObserver.addOnGlobalLayoutListener(this)But value.window.decorView.viewTreeObserver.addOnGlobalLayoutListener(this)The whole point of this fix is to handle config changes where the activity is not destroyed/recreated (i.e. the manifest declares
Step-by-step proof
Addressing the refutationThe refutation correctly notes that (a) the same pattern pre-exists in However, the issue is still worth flagging in this specific PR because:
Suggested fixThe simplest fix is to drop the line entirely from private fun onScreenSizeChanged(activity: Activity) {
activityLifecycleNotifier.fire { it.onActivityStopped(activity) }
activityLifecycleNotifier.fire { it.onActivityAvailable(activity) }
// (removed: addOnGlobalLayoutListener — already registered in the `current` setter)
}Alternatively, pair |
||
| } | ||
|
Comment on lines
+409
to
+412
|
||
|
|
||
| private fun handleLostFocus() { | ||
| if (isInForeground) { | ||
| Logging.debug("ApplicationService.handleLostFocus: application is now out of focus") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| package com.onesignal.core.internal.features | ||
|
|
||
| import com.onesignal.common.FoldableIAMFeature | ||
| import com.onesignal.common.modeling.ISingletonModelStoreChangeHandler | ||
| import com.onesignal.common.modeling.ModelChangeTags | ||
| import com.onesignal.common.modeling.ModelChangedArgs | ||
|
|
@@ -113,17 +114,21 @@ | |
| enabled = enabled, | ||
| source = "FeatureManager:${feature.activationMode}" | ||
| ) | ||
| FeatureFlag.SDK_050800_FOLDABLE_IAM_FIX -> | ||
| FoldableIAMFeature.updateEnabled( | ||
| enabled = enabled, | ||
| source = "FeatureManager:${feature.activationMode}" | ||
| ) | ||
|
Comment on lines
+117
to
+121
|
||
| } | ||
| } | ||
|
|
||
| companion object { | ||
| /** | ||
| * Local-only test hook for forcing features ON without backend config. | ||
| * Add feature keys here while testing locally, e.g.: | ||
| * setOf(FeatureFlag.BACKGROUND_THREADING.key) | ||
| * setOf(FeatureFlag.SDK_050800_FOLDABLE_IAM_FIX.key) | ||
| */ | ||
| private val localFeatureOverrides: Set<String> = emptySet() | ||
| // private val localFeatureOverrides: Set<String> = | ||
| // setOf(FeatureFlag.BACKGROUND_THREADING.key) | ||
| private val localFeatureOverrides: Set<String> = | ||
|
Check failure on line 131 in OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureManager.kt
|
||
|
Comment on lines
130
to
+131
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 The latest commit (d46b773) hardcodes Extended reasoning...What the bug is. private val localFeatureOverrides: Set<String> =
setOf(FeatureFlag.SDK_050800_FOLDABLE_IAM_FIX.key)The KDoc directly above (lines 125-129) is explicit that this is a "Local-only test hook for forcing features ON without backend config" and shows it being used as a local-development example. The most recent commit on this branch — The code path that triggers it. val enabledFeatureKeys = (model.features + localFeatureOverrides).toSet()
if (localFeatureOverrides.isNotEmpty()) {
Logging.warn("OneSignal: Local feature override enabled for testing only: $localFeatureOverrides")
}Because Why existing code doesn't prevent it. There is no environment / debug-build guard around Impact. (1) Every consumer of this SDK build silently activates the new fold/unfold detection and Fix. Revert the constant to private val localFeatureOverrides: Set<String> = emptySet()
// private val localFeatureOverrides: Set<String> =
// setOf(FeatureFlag.SDK_050800_FOLDABLE_IAM_FIX.key)This matches the pattern used for Step-by-step proof.
|
||
| setOf(FeatureFlag.SDK_050800_FOLDABLE_IAM_FIX.key) | ||
| } | ||
|
Comment on lines
+131
to
133
|
||
| } | ||
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.
🔴 The new screen-size branch in
onConfigurationChanged(lines 100-109) firesonScreenSizeChangedunconditionally, unlike the orientation branch directly above which gates onAndroidUtils.hasConfigChangeFlag(current!!, ActivityInfo.CONFIG_ORIENTATION). Most apps do not declarescreenSizeinandroid:configChanges, so on a fold/unfold Android destroys+recreates the activity while this code simultaneously firesonActivityStopped/onActivityAvailableon the dying activity and re-attaches a layout listener to its doomed decor view — racing the real lifecycle and producing duplicate IAM teardown/recreate. Fix by mirroring the orientation gate:if (FoldableIAMFeature.isEnabled && current != null && AndroidUtils.hasConfigChangeFlag(current!!, ActivityInfo.CONFIG_SCREEN_SIZE) && hasScreenSizeChanged(newConfig))(and likely alsoCONFIG_SMALLEST_SCREEN_SIZEon API 24+).Extended reasoning...
What is wrong
In
ApplicationService.onConfigurationChanged, the orientation handler at lines 92-99 deliberately gatesonOrientationChanged()behindAndroidUtils.hasConfigChangeFlag(current!!, ActivityInfo.CONFIG_ORIENTATION):The new screen-size branch added in this PR (lines 100-109) does not include the analogous
CONFIG_SCREEN_SIZEcheck:Why the gate exists
Per the Android documentation,
Application.ComponentCallbacks.onConfigurationChangedis invoked on every configuration change. However, if the host activity has not declared the relevant flag in its manifestandroid:configChanges, Android destroys and recreates the activity for that change. The normalActivityLifecycleCallbackspath (onActivityStoppedon the old activity, thenonActivityAvailableon the new one) already re-shows the IAM. Manually re-firing those callbacks ourselves is therefore redundant and races the real lifecycle. This is exactly why the orientation branch checks the flag first.Step-by-step proof of the race
Assume a typical app whose
<activity android:configChanges="...">does not includescreenSize(the default — most apps do not declare this). On a Galaxy Fold unfold:screenWidthDp/screenHeightDp.screenSize, the OS schedules the activity to be destroyed and recreated.Application.ComponentCallbacks.onConfigurationChangedfires — this is independent of activity recreation.currentstill references the old (about-to-die) activity at this point.FoldableIAMFeature.isEnabled && current != null && hasScreenSizeChanged(newConfig)→ all true, soonScreenSizeChanged(current!!)runs.onScreenSizeChanged, the SDK firesactivityLifecycleNotifier.fire { it.onActivityStopped(activity) }and thenit.onActivityAvailable(activity)on the doomed activity, and callsactivity.window.decorView.viewTreeObserver.addOnGlobalLayoutListener(this)on a decor view that is about to be torn down.WebViewManager.onActivityAvailablerunscalculateHeightAndShowWebViewAfterNewActivity()(becauselastActivityName == currentActivityName), which callssetWebViewToMaxSizeon the dying activity and posts work to the message-view mutex.onActivityStoppedfor the dying activity (whichWebViewManager.onActivityStoppedhandles by callingmessageView!!.removeAllViews()), then a freshonActivityStarted/onActivityAvailablefor the recreated activity — which itself triggers anothercalculateHeightAndShowWebViewAfterNewActivity().Net effect: two layered teardown/recreate flows running concurrently against different activity references, plus a
OnGlobalLayoutListenerregistered on a decor view that will be detached. At minimum this is duplicate work guarded by a mutex; at worst it is laying out an IAM on a dying window.Why the existing checks do not save us
current != nullis true at this point —currentis set to null only inonActivityStoppedafter--activityReferences <= 0, which has not happened yet.hasScreenSizeChanged(newConfig)is comparing dp dimensions; on a fold/unfold these change regardless of whether the activity handles the change.FoldableIAMFeature.isEnabledflag is force-on in this PR vialocalFeatureOverrides = setOf(FeatureFlag.SDK_050800_FOLDABLE_IAM_FIX.key)(FeatureManager.kt) plus the previous commitd46b773("force SDK_050800_FOLDABLE_IAM_FIX on via localFeatureOverrides"), so the buggy path is live by default for any app consuming this build.Impact
For the common case (apps that have not opted into
android:configChanges="screenSize"), every fold/unfold triggers a redundant IAM stop+available+layout-listener cycle on a dying activity simultaneously with the OS-driven recreate. Beyond duplicated work, attaching to a soon-detached decor view and laying out IAMs on a doomed window can cause visible glitches and is exactly the kind of race the orientation gate was added to avoid.Fix
Mirror the orientation branch:
Optionally also accept
CONFIG_SMALLEST_SCREEN_SIZE(and possiblyCONFIG_SCREEN_LAYOUT) since on API 24+ apps that handle fold/unfold typically declaresmallestScreenSize|screenSize|screenLayouttogether. The point is the same as the orientation case: only do the manual SDK recreation when the host activity is not going to be recreated by the OS.