AmbientAware composable unnecessarily recreates the whole tree

[marcin-adamczewski]

Looks like AmbientAware composable unnecessarily recreates the whole block that is passed when the isAlwaysScreenOn parameter changes. Instead, it should just do recomposition on this block.

Here's how it looks now:

@Composable
fun AmbientAware(
    isAlwaysOnScreen: Boolean = true,
    block: @Composable (AmbientStateUpdate) -> Unit,
) {
    val activity = LocalContext.current.findActivityOrNull()
    // Using AmbientAware correctly relies on there being an Activity context. If there isn't, then
    // gracefully allow the composition of [block], but no ambient-mode functionality is enabled.
    if (activity != null && isAlwaysOnScreen) {
        AmbientAwareEnabled(activity, block)
    } else {
        AmbientAwareDisabled(block)
    }
}

When isAlwaysOnScreen changes the block Composable is recreated. It's especially not nice when the navigation component is wrapped with AmbientAware.
This seems to be against the guidelines here -> https://github.com/androidx/androidx/blob/androidx-main/compose%2Fdocs%2Fcompose-component-api-guidelines.md#lifecycle-expectations-for-slot-parameters

Seems like the if else is not necessary. Here's an alternative solution

@Composable
fun MyAmbientAware(
    isAlwaysOnScreen: Boolean = true,
    block: @Composable (AmbientStateUpdate) -> Unit,
) {
    var ambientUpdate by remember(isAlwaysOnScreen) {
        mutableStateOf(if (isAlwaysOnScreen) null else AmbientStateUpdate(AmbientState.Interactive))
    }

    val activity = LocalContext.current.findActivityOrNull()
    // Using AmbientAware correctly relies on there being an Activity context. If there isn't, then
    // gracefully allow the composition of [block], but no ambient-mode functionality is enabled.
    if (activity != null && isAlwaysOnScreen) {
        val lifecycle = LocalLifecycleOwner.current.lifecycle
        val observer = remember {
            val callback = object : AmbientLifecycleObserver.AmbientLifecycleCallback {
                override fun onEnterAmbient(ambientDetails: AmbientLifecycleObserver.AmbientDetails) {
                    ambientUpdate = AmbientStateUpdate(AmbientState.Ambient(ambientDetails))
                }

                override fun onExitAmbient() {
                    ambientUpdate = AmbientStateUpdate(AmbientState.Interactive)
                }

                override fun onUpdateAmbient() {
                    val lastAmbientDetails =
                        (ambientUpdate?.ambientState as? AmbientState.Ambient)?.ambientDetails
                    ambientUpdate = AmbientStateUpdate(AmbientState.Ambient(lastAmbientDetails))
                }
            }
            AmbientLifecycleObserver(activity, callback).also {
                // Necessary to populate the initial value
                val initialAmbientState = if (it.isAmbient) {
                    AmbientState.Ambient(null)
                } else {
                    AmbientState.Interactive
                }
                ambientUpdate = AmbientStateUpdate(initialAmbientState)
            }
        }

        DisposableEffect(Unit) {
            lifecycle.addObserver(observer)

            onDispose {
                lifecycle.removeObserver(observer)
            }
        }
    }

    ambientUpdate?.let {
        block(it)
    }
}

With this approach the if is only on adding the observer and not on showing the content. When ambientUpdate changes the block just recomposes.

version 0.5.21

[yschimke]

@garanj Do you want to take a look?

[yschimke]

@marcin-adamczewski thanks for the feedback.