IssueTracker

We might just need to implement equals and hashcode for the modifier

Hi, I can't reproduce this behavior. It is possible that the composable in your code is recomposing because of a different reason. Is the tag you pass for example read from a class variable? That would cause your composable to be recomposed because the compiler cannot infer that all input to the testTag is @Stable.

The test below shows that adding testTag does not prevent Compose to skip the composable:

import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.width
import androidx.compose.material.Text
import androidx.compose.runtime.Composable
import androidx.compose.runtime.Stable
import androidx.compose.runtime.produceState
import androidx.compose.ui.Modifier
import androidx.compose.ui.platform.testTag
import androidx.compose.ui.test.junit4.createComposeRule
import androidx.compose.ui.unit.dp
import androidx.test.ext.junit.runners.AndroidJUnit4
import com.google.common.truth.Truth.assertThat
import kotlinx.coroutines.delay
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith

@RunWith(AndroidJUnit4::class)
class TestTagTest {
    @get:Rule
    val rule = createComposeRule()

    @Stable
    private class RecompositionCounter {
        var withoutTag = 0
        var withTag = 0
    }

    @Test
    fun test() {
        val counter = RecompositionCounter()
        rule.setContent {
            val state = produceState(initialValue = 0) {
                repeat(10) {
                    delay(500)
                    value++
                }
            }

            Column {
                Text(text = "value: ${state.value}")
                MyComponent(modifier = Modifier.width(200.dp), "Data1", counter)
                MyComponent(modifier = Modifier.width(200.dp).testTag("Data2"), "Data2", counter)
            }
        }

        repeat(10) {
            rule.waitForIdle()
            rule.mainClock.advanceTimeBy(500)
        }

        rule.onNodeWithText("value: 10").assertExists()
        assertThat(counter.withoutTag).isEqualTo(1)
        assertThat(counter.withTag).isEqualTo(1)
    }

    @Composable
    private fun MyComponent(modifier: Modifier = Modifier, text: String, rc: RecompositionCounter) {
        if (text == "Data1") {
            rc.withoutTag++
        } else if (text == "Data2") {
            rc.withTag++
        }
        Text(modifier = modifier, text = text)
    }
}

Hi,

I have change the code a bit so I can use it outside a test on a ComponentActivity.

Here the code:

package de.telekom.testapp

import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.width
import androidx.compose.material.Text
import androidx.compose.runtime.Composable
import androidx.compose.runtime.Stable
import androidx.compose.runtime.produceState
import androidx.compose.runtime.remember
import androidx.compose.ui.Modifier
import androidx.compose.ui.platform.testTag
import androidx.compose.ui.unit.dp
import kotlinx.coroutines.delay

@Stable
private data class RecompositionCounter(
    var withoutTag: Int = 0,
    var withTag: Int = 0
)

@Composable
fun TestModifier() {

    val counter = remember { RecompositionCounter() }

    val state = produceState(initialValue = 0) {
        repeat(10) {
            delay(500)
            value++
        }
    }

    Column {

        Text(text = "value: ${state.value}  $counter")

        MyComponent(modifier = Modifier.width(200.dp), "Data1", counter)
        MyComponent(
            modifier = Modifier
                .width(200.dp)
                .testTag("Data2"), "Data2", counter
        )
    }
}

@Composable
private fun MyComponent(modifier: Modifier = Modifier, text: String, rc: RecompositionCounter) {
    if (text == "Data1") {
        rc.withoutTag++
    } else if (text == "Data2") {
        rc.withTag++
    }
    Text(modifier = modifier, text = text)
}

And the counter "withTag" still increments. Attached you find a screenshot after 10 repeats.

Additional info

compose version: '1.1.0-beta02' gradle: gradle-7.2-all Emulator: API 26 x86

Hmm, this is odd. I can reproduce the bug when using the latest snapshot build of Compose from androidx.dev (7901760), but the same test passes when run from within our Compose project on the tip-of-tree. I wouldn't think there is much difference between the two, but I'll need to verify to make sure. I'll investigate further

Here is what happened:

The Live Literals feature in Android Studio switches out literals with state values. This will influence static inference by the compiler.

Without Live Literals on, the compiler will infer that Modifier.testTag("tag") is only using static input and will not even call equals on it to verify if the value has changed (and consequently if parts of the code can be skipped); it knows that it will never change. With Live Literals on however, Modifier.testTag("tag") is no longer considered static and during composition Modifier.testTag("tag") == Modifier.testTag("tag") will be false, resulting in not skipping that part of the code.

This is a consequence of the underlying Modifier.composed(..), which is not equal to another Modifier.composed(..) with exactly the same lambda.

For now, you can turn off Live Literals by adding @file:NoLiveLiterals to the impacted files when benchmarking compositions. Also, in release builds Live Literals will always be turned off.

I filed b/205851704 to start fixing the equality of testTag issue

In release builds or with @file:NoLiveLiterals it works.

Thanks for the details.

I didn't specifically test the testTag/skipping combination, but I think the above change probably fixed it in Compose 1.3. Feel free to reopen if it didn't.

Coming back to this to give me 5 cents.

Modifier.scrollable itself doesn't have any notion of layout at all, so I'm unsure if that would be the right place for it. Thing to node is that we also need to provide a proper nestedscroll support (+ account for nested scroll as well, as you want your toolbar to collapse if the textField is below the soft input, don't you?).

It seems like we would need to account for that in the scrollable implementations, e.g Modifier.verticalScroll or LazyColumn, as they also might want to process it differently. Some bits might live in the Modifier.scrollable as well, but it's mostly a layout thing I suppose.

Another alternative: could it be possible to utilise semantics for this purpose? I don't know how idiomatic it would be but it seems like it is somewhat related. WE could basically ask whatever container to scroll to the bounds (which we could retrieve from the layout node) and the scroll bit will just work out of the box if the semantics are set right.

Ralston, let me know if you need any help or input from the scrolling side! Thanks!

Bulk edit: This bug is currently marked as blocking Compose 1.0. Once rc01 is cut, all changes will need to go through our cherry-pick process.

If you intend to fix it in Compose 1.0, no action is needed except prioritizing finishing this work.

Otherwise, please remove this bug from the appropriate 1.0 hotlist (likely Jetpack Compose 1.0 (May 2021)) and add it to Post-1.0 (or not-yet-prioritized).

Thanks!

For this fix we need TextField to use RelocationRequester.bringIntoView() which is an experimental UI API, and it so it can't be fixed for Compose 1.0

Is there something that has to be done from the developer perspective for the textfield to scroll into view when the keyboard comes up to cover them?