Updating bottom padding of PaddingValues and then passing it to LazyColumn(contentPadding = ...) sometimes does not recompose the lambda inside of rememberLazyListMeasurePolicy() [263402091]

Fixed

Bug

Status Update

No update yet.

Description

ke...@gmail.com

created issue #1

Dec 22, 2022 07:02AM

Jetpack Compose version: 1.4.0-alpha03

Jetpack Compose component(s) used: LazyColumn

Android Studio Build: Android Studio Electric Eel | 2022.1.1 RC 1

Kotlin version: 1.7.20

Originally, I asked this question on #compose slack channel but I was asked to report it here.

"Hi. I'm currently using Compose 1.4.0-alpha03 and I'm experiencing a weird behavior where updating bottom padding of PaddingValue which is then passed to LazyColumn(contentPadding = ...) doesn't actually use the new padding inside of the rememberLazyListMeasurePolicy. The bottom padding is updated with the keyboard height everytime it appears/disappears. I'm placing two breakpoints 1. Inside of the internal fun LazyList(...) function on state.placementAnimator = placementAnimator line. 2. Inside of the private fun rememberLazyListMeasurePolicy() function on checkScrollableContainerConstraints() line. Then I'm clicking a textfield to make the keyboard appear. Once the first breakpoint is hit I first check that the bottom padding is the value I'm expecting to see (~300dp) then I place a new breakpoint inside of PaddingValuesImpl.equals() method. Then I resume the app, PaddingValuesImpl.equals() is getting called a couple of times but it's called not from the place I'm expecting it to get called from. I'm expecting a call from remember<LazyLayoutMeasureScope.(Constraints) -> MeasureResult>() { ... } line which is in rememberLazyListMeasurePolicy() method but it never gets called from that method. Then the second breakpoint which is on checkScrollableContainerConstraints() line is getting hit and when I check the contentPadding I'm seeing the old value (24dp). So I'm assuming that the remember<LazyLayoutMeasureScope.(Constraints) -> MeasureResult>() { ... } doesn't work for some reason in this situation so the lambda is not re-remebered in other words it's the previous lambda that captured the previous contentPadding. I would really like to see what's going on inside remember<LazyLayoutMeasureScope.(Constraints) -> MeasureResult>() { ... } but I can't place a breakpoint there because it tells me to download the source code and when I click download it can't find it. I tried installing the compose-runtime dependency but it still won't show me the source code of that remember<LazyLayoutMeasureScope.(Constraints) -> MeasureResult>() { ... } function. This feels like a bug in that one remember function. However I also tried to create a new project and copy some of the code to try and reproduce the issue but it works just fine there (same compose version). So I'm kinda lost now. I'm not sure I should report this on the issue tracker because I have no MRE and it also works in a new test project but doesn't work in the main project. The app I'm developing is open-source so I can provide the whole project if needed."

STR:

Clone this branch: https://github.com/K1rakishou/KurobaExLite/tree/lazycolumn_remember_weird_behavior
Click "Select a catalog" text in the toolbar.
Scroll to the very bottom (100th item).
Click the "Search" icon.
Keyboard will show up. Scroll to bottom again you will see that it can't scroll past ~94th item.

If you do the steps described above (the one with breakpoints) you will see the bug. I'm also attaching a video where I'm doing those steps.

As I have already said above I tried to create a MRE in a new project but was surprised to see that it works there flawlessly. Also I have multiple places in this project where I'm using the same method to set the bottom padding of the LazyColumn when the keyboard shows up and it works in some of those places.

studio64_MvsnAZTM9k.mp4

11 MB

Download

Comments

ma...@google.com <ma...@google.com> Dec 22, 2022 10:56AM

Reassigned to as...@google.com.

as...@google.com <as...@google.com> #2Dec 23, 2022 03:21AM

Ignore this msg

Message last modified on Dec 23, 2022 03:22AM

ke...@gmail.com <ke...@gmail.com> #3Dec 23, 2022 04:23AM

Project: platform/frameworks/support
Branch: androidx-main

commit e7b4b06e67951ab47157d06ff314a5c8b2e8fdc6
Author: Andrei Shikov <ashikov@google.com>
Date: Mon Jan 08 15:54:51 2024

Count recursive local declarations as captures

Local declarations were already counted as captures for composable lambdas before, but the traversal ordering missed the recursive captures, converting some of them into singletons.

Test: Compiler test
Fixes: 318745941
Change-Id: I9097d1be71fb67b73e5027f723fd187c4272f6b4

M compose/compiler/compiler-hosted/integration-tests/src/androidUnitTest/kotlin/androidx/compose/compiler/plugins/kotlin/ComposeBytecodeCodegenTest.kt
M compose/compiler/compiler-hosted/integration-tests/src/jvmTest/kotlin/androidx/compose/compiler/plugins/kotlin/AbstractCodegenTest.kt
M compose/compiler/compiler-hosted/src/main/java/androidx/compose/compiler/plugins/kotlin/lower/ComposerLambdaMemoization.kt

https://android-review.googlesource.com/2900890

as...@google.com <as...@google.com> #4Dec 29, 2022 07:34PM

Isolated repro:

@Composable
private fun Demo() {
    var isGreen by remember { mutableStateOf(true) }

    val unstable = mutableListOf<Nothing>()
    Content(isGreen, unstable)

    LaunchedEffect(Unit) {
        while (true) {
            delay(1000)
            isGreen = !isGreen
        }
    }
}

@Composable
fun Content(isGreen: Boolean, unstable: MutableList<*>) {
    val useUnstable = unstable
    BoxWithConstraints {
        val remembered = remember(isGreen) { if (isGreen) Color.Green else Color.Red }
        Box(
            Modifier
                .size(100.dp)
                .background(remembered)
        )
    }
}

Removing unstable, replacing BoxWithConstraints or removing remember fixes the issue.

Message last modified on Dec 29, 2022 07:43PM

as...@google.com <as...@google.com> #5Dec 29, 2022 08:34PM

Previous comment actually highlighted a different issue ($changed value of 000 being propagated to intrinsic remember).

Here's the version that matches the case encountered in the app:

@Composable
fun Demo() {
    var isGreen by remember { mutableStateOf(true) }

    ContentWrapper(isGreen)

    LaunchedEffect(Unit) {
        while (true) {
            delay(1000)
            isGreen = !isGreen
        }
    }
}

@Composable
fun ContentWrapper(isGreen: Boolean) {
    var counter by remember { mutableStateOf(0) }
    counter++
    // this scope is restarted two times
    // (once because new param, second time because isGreenAgain was changed)
    // also ensures that $changed for isGreen propagates on recomposition
    Content(isGreen, counter)
}

@Composable
fun Content(isGreen: Boolean, count: Int) {
    println(count)
    // put breakpoint here and check $dirty value on recomposition (Different first time, Same second time)
    BoxWithConstraints {
        val remembered = remember(isGreen) { if (isGreen) Color.Green else Color.Red }
        // breakpoint here - recomposed one time and $dirty is always Same
        Box(
            Modifier
                .size(100.dp)
                .background(remembered)
        )
    }
}

Message last modified on Jan 5, 2023 08:51PM

as...@google.com <as...@google.com> #6Dec 29, 2022 08:36PM

It also happens even without subcomposition involved, e.g. replacing BoxWithConstraints with the following composable yields the same result:

@Composable
fun BoxNoInline(content: @Composable () -> Unit) {
    Box {
        content()
    }
}

ap...@google.com <ap...@google.com> #7Oct 17, 2023 01:43PM

Project: platform/frameworks/support
Branch: androidx-main

commit db3699fb957b490fd4050e3e2415631a323390e1
Author: Andrei Shikov <ashikov@google.com>
Date: Mon Oct 09 17:02:15 2023

Restrict $dirty capture to inline lambdas

Makes sure we use $dirty only in the body of the param owner. This makes sure that backwards writes to `composableLambda` are propagating comparisons correctly, while slightly increasing amount of slots used by Compose.

Test: updated compiler tests
Bug: 263402091
Change-Id: I67f435f73ba8c5e3c96bdd21e94827364a9b5a84

M compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.ComposerParamTransformTests/testStableDelegateCall[useFir = false].txt
M compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.ComposerParamTransformTests/testStableDelegateCall[useFir = true].txt
M compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.FunctionBodySkippingTransformTests/testComposableWithAndWithoutDefaultParams[useFir = false].txt
M compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.FunctionBodySkippingTransformTests/testComposableWithAndWithoutDefaultParams[useFir = true].txt
M compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.FunctionBodySkippingTransformTests/testIfInLambda[useFir = false].txt
M compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.FunctionBodySkippingTransformTests/testIfInLambda[useFir = true].txt
M compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.FunctionBodySkippingTransformTests/testLocalComposableFunctions[useFir = false].txt
M compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.FunctionBodySkippingTransformTests/testLocalComposableFunctions[useFir = true].txt
M compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.FunctionBodySkippingTransformTests/testLocalFunction[useFir = false].txt
M compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.FunctionBodySkippingTransformTests/testLocalFunction[useFir = true].txt
M compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.FunctionBodySkippingTransformTests/testNestedCalls[useFir = false].txt
M compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.FunctionBodySkippingTransformTests/testNestedCalls[useFir = true].txt
M compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.LambdaMemoizationTransformTests/testRememberComposableLambda[useFir = false].txt
M compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.LambdaMemoizationTransformTests/testRememberComposableLambda[useFir = true].txt
M compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.TargetAnnotationsTransformTests/testCallingLayout[useFir = false].txt
M compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.TargetAnnotationsTransformTests/testCallingLayout[useFir = true].txt
M compose/compiler/compiler-hosted/src/main/java/androidx/compose/compiler/plugins/kotlin/lower/ComposableFunctionBodyTransformer.kt

https://android-review.googlesource.com/2779188

ap...@google.com <ap...@google.com> #8Nov 9, 2023 02:54PM

Project: platform/frameworks/support
Branch: androidx-main

commit 96e2d4dc0289ef5de540198079440c7cafef5f29
Author: Andrei Shikov <ashikov@google.com>
Date: Fri Nov 03 15:37:16 2023

Account for Uncertain param state in intrinsic remember

Previous implementation of intrinsic remember is always expecting `$dirty` mask to contain different or same state for stable params. It is not always the case, however, as some restartable groups don't have the skipping code generated, which causes $dirty to never be updated.

This change accounts for it by delaying $dirty handling until we know that skipping code is going to be generated, applying fixups for intrinsic remember calls in the body.

Test: RememberIntrinsicTransformTests
Bug: 263402091
Change-Id: I7402075693806462be90e1697b8f726f1085a7a6

M compose/compiler/compiler-hosted/integration-tests/src/jvmTest/kotlin/androidx/compose/compiler/plugins/kotlin/RememberIntrinsicTransformTests.kt
A compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.RememberIntrinsicTransformTests/testRememberWithUnstableUnused_InInlineLambda[useFir = false].txt
A compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.RememberIntrinsicTransformTests/testRememberWithUnstableUnused_InInlineLambda[useFir = true].txt
A compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.RememberIntrinsicTransformTests/testRememberWithUnstable_InInlineLambda[useFir = false].txt
A compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.RememberIntrinsicTransformTests/testRememberWithUnstable_InInlineLambda[useFir = true].txt
A compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.RememberIntrinsicTransformTests/testRememberWithUnstable_inLambda[useFir = false].txt
A compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.RememberIntrinsicTransformTests/testRememberWithUnstable_inLambda[useFir = true].txt
M compose/compiler/compiler-hosted/src/main/java/androidx/compose/compiler/plugins/kotlin/lower/AbstractComposeLowering.kt
M compose/compiler/compiler-hosted/src/main/java/androidx/compose/compiler/plugins/kotlin/lower/ComposableFunctionBodyTransformer.kt

https://android-review.googlesource.com/2759147

al...@google.com <al...@google.com> #9Mar 14, 2024 07:54PM

Is this in the correct Buganizer component?

as...@google.com <as...@google.com> #10Mar 14, 2024 07:57PM

Marked as fixed.

Yes, also already fixed.