Skip to content
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.

Sign up for GitHub

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

Support Compose 1.6 and 1.7 #169

Merged
merged 3 commits into from
Jul 18, 2024
Merged

Support Compose 1.6 and 1.7 #169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
02a1911
fix semantics modifiers
elihart Jul 17, 2024
7594ff0
fix rest of tests
elihart Jul 17, 2024
7cd79be
clean up
elihart Jul 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion buildSrc/src/main/java/Dependencies.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ object Versions {
val KotlinCompiler = System.getProperty("square.kotlinVersion") ?: "1.9.10"

const val AndroidXTest = "1.5.0"
const val Compose = "1.5.3"
const val Compose = "1.7.0-beta05"
const val ComposeCompiler = "1.5.3"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't want to attempt to update kotlin at the same time, so I left the compose compiler version

}

object Dependencies {
Expand Down
2 changes: 1 addition & 1 deletion compose-tests/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ android {
}

composeOptions {
kotlinCompilerExtensionVersion = Versions.Compose
kotlinCompilerExtensionVersion = Versions.ComposeCompiler
}

packaging {
Expand Down
2 changes: 2 additions & 0 deletions compose-tests/src/androidTest/java/radiography/test/compose/ComposeTestRules.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package radiography.test.compose

import androidx.compose.foundation.layout.Box
import androidx.compose.runtime.Composable
import androidx.compose.runtime.currentComposer
import androidx.compose.ui.test.junit4.ComposeContentTestRule

/**
Expand All @@ -14,6 +15,7 @@ import androidx.compose.ui.test.junit4.ComposeContentTestRule
*/
fun ComposeContentTestRule.setContentWithExplicitRoot(content: @Composable () -> Unit) {
setContent {
currentComposer.collectParameterInformation()
Box {
content()
}
Expand Down
57 changes: 38 additions & 19 deletions compose-tests/src/androidTest/java/radiography/test/compose/ComposeUiTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import androidx.compose.material.Button
import androidx.compose.material.Checkbox
import androidx.compose.material.TextField
import androidx.compose.runtime.Composable
import androidx.compose.runtime.currentComposer
import androidx.compose.ui.ExperimentalComposeUiApi
import androidx.compose.ui.Modifier
import androidx.compose.ui.layout.SubcomposeLayout
Expand Down Expand Up @@ -380,6 +381,8 @@ class ComposeUiTest {

@Test fun scanningHandlesDialog() {
composeRule.setContent {
currentComposer.collectParameterInformation()

Box(Modifier.testTag("parent")) {
Dialog(onDismissRequest = {}) {
Box(Modifier.testTag("child"))
Expand All @@ -393,8 +396,8 @@ class ComposeUiTest {

assertThat(hierarchy).isEqualTo(
"""
|CompositionLocalProvider:
|${BLANK}CompositionLocalProvider { test-tag:"parent" }
|Box:
Copy link
Contributor Author

@elihart elihart Jul 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure where this change comes from exactly, but these node names are now different and it seems to be more correct. I am guessing it is a behavior change within compose that provides more accurate naming for nodes some how. I didn't look into it further though

|${BLANK}Box { test-tag:"parent" }
|${BLANK}╰─Dialog
|${BLANK} ╰─CompositionLocalProvider { DIALOG }
|${BLANK} ╰─Box { test-tag:"child" }
Expand All @@ -409,6 +412,8 @@ class ComposeUiTest {
}

composeRule.setContent {
currentComposer.collectParameterInformation()

Box(Modifier.testTag("parent")) {
CustomTestDialog {
Box(Modifier.testTag("child"))
Expand All @@ -422,8 +427,8 @@ class ComposeUiTest {

assertThat(hierarchy).isEqualTo(
"""
|CompositionLocalProvider:
|${BLANK}CompositionLocalProvider { test-tag:"parent" }
|Box:
|${BLANK}Box { test-tag:"parent" }
|${BLANK}╰─CustomTestDialog
|${BLANK} ╰─CompositionLocalProvider { DIALOG }
|${BLANK} ╰─Box { test-tag:"child" }
Expand All @@ -434,6 +439,8 @@ class ComposeUiTest {

@Test fun scanningHandlesSingleSubcomposeLayout_withSingleChild() {
composeRule.setContent {
currentComposer.collectParameterInformation()

Box(Modifier.testTag("parent")) {
SingleSubcompositionLayout(Modifier.testTag("subcompose-layout")) {
Box(Modifier.testTag("child"))
Expand All @@ -447,8 +454,8 @@ class ComposeUiTest {

assertThat(hierarchy).isEqualTo(
"""
|CompositionLocalProvider:
|${BLANK}CompositionLocalProvider { test-tag:"parent" }
|Box:
|${BLANK}Box { test-tag:"parent" }
|${BLANK}╰─SingleSubcompositionLayout { test-tag:"subcompose-layout" }
|${BLANK} ╰─<subcomposition of SingleSubcompositionLayout>
|${BLANK} ╰─Box { test-tag:"child" }
Expand All @@ -459,6 +466,8 @@ class ComposeUiTest {

@Test fun scanningHandlesSingleSubcomposeLayout_withMultipleChildren() {
composeRule.setContent {
currentComposer.collectParameterInformation()

Box(Modifier.testTag("parent")) {
SingleSubcompositionLayout(Modifier.testTag("subcompose-layout")) {
Box(Modifier.testTag("child1"))
Expand All @@ -473,8 +482,8 @@ class ComposeUiTest {

assertThat(hierarchy).isEqualTo(
"""
|CompositionLocalProvider:
|${BLANK}CompositionLocalProvider { test-tag:"parent" }
|Box:
|${BLANK}Box { test-tag:"parent" }
|${BLANK}╰─SingleSubcompositionLayout { test-tag:"subcompose-layout" }
|${BLANK} ╰─<subcomposition of SingleSubcompositionLayout>
|${BLANK} ├─Box { test-tag:"child1" }
Expand All @@ -486,6 +495,8 @@ class ComposeUiTest {

@Test fun scanningHandlesSingleSubcomposeLayout_withMultipleSubcompositionsAndChildren() {
composeRule.setContent {
currentComposer.collectParameterInformation()

Box(Modifier.testTag("parent")) {
MultipleSubcompositionLayout(Modifier.testTag("subcompose-layout"),
firstChildren = {
Expand All @@ -505,8 +516,8 @@ class ComposeUiTest {

assertThat(hierarchy).isEqualTo(
"""
|CompositionLocalProvider:
|${BLANK}CompositionLocalProvider { test-tag:"parent" }
|Box:
|${BLANK}Box { test-tag:"parent" }
|${BLANK}╰─MultipleSubcompositionLayout { test-tag:"subcompose-layout" }
|${BLANK} ├─<subcomposition of MultipleSubcompositionLayout>
|${BLANK} │ ├─Box { test-tag:"child1.1" }
Expand All @@ -521,6 +532,8 @@ class ComposeUiTest {

@Test fun scanningHandlesSiblingSubcomposeLayouts() {
composeRule.setContent {
currentComposer.collectParameterInformation()

Box(Modifier.testTag("parent")) {
SingleSubcompositionLayout(Modifier.testTag("subcompose-layout1")) {
Box(Modifier.testTag("child1"))
Expand All @@ -537,8 +550,8 @@ class ComposeUiTest {

assertThat(hierarchy).isEqualTo(
"""
|CompositionLocalProvider:
|${BLANK}CompositionLocalProvider { test-tag:"parent" }
|Box:
|${BLANK}Box { test-tag:"parent" }
|${BLANK}├─SingleSubcompositionLayout { test-tag:"subcompose-layout1" }
|${BLANK}│ ╰─<subcomposition of SingleSubcompositionLayout>
|${BLANK}│ ╰─Box { test-tag:"child1" }
Expand All @@ -552,6 +565,8 @@ class ComposeUiTest {

@Test fun scanningHandlesWithConstraints() {
composeRule.setContent {
currentComposer.collectParameterInformation()

Box(Modifier.testTag("parent")) {
BoxWithConstraints(Modifier.testTag("with-constraints")) {
Box(Modifier.testTag("child"))
Expand All @@ -565,8 +580,8 @@ class ComposeUiTest {

assertThat(hierarchy).isEqualTo(
"""
|CompositionLocalProvider:
|${BLANK}CompositionLocalProvider { test-tag:"parent" }
|Box:
|${BLANK}Box { test-tag:"parent" }
|${BLANK}╰─BoxWithConstraints { test-tag:"with-constraints" }
|${BLANK} ╰─<subcomposition of BoxWithConstraints>
|${BLANK} ╰─Box { test-tag:"child" }
Expand All @@ -577,6 +592,8 @@ class ComposeUiTest {

@Test fun scanningHandlesLazyLists() {
composeRule.setContent {
currentComposer.collectParameterInformation()

Box(Modifier.testTag("parent")) {
LazyColumn(Modifier.testTag("list")) {
items(listOf(1, 2, 3)) {
Expand All @@ -595,9 +612,9 @@ class ComposeUiTest {

assertThat(hierarchy).isEqualTo(
"""
|CompositionLocalProvider:
|${BLANK}CompositionLocalProvider { test-tag:"parent" }
|${BLANK}╰─LazyColumn { test-tag:"list", vertical-scroll-axis-range:"ScrollAxisRange(value=0.0, maxValue=0.0)" }
|Box:
|${BLANK}Box { test-tag:"parent" }
|${BLANK}╰─LazyColumn { vertical-scroll-axis-range:"ScrollAxisRange(value=0.0, maxValue=0.0)", test-tag:"list" }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the order of the attributes changed, so I updated the expected value to match. Perhaps the renderer should sort them by name to keep them consistent?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems like a good idea, but we can fix in a follow-up.

|${BLANK} ├─<subcomposition of LazyColumn>
|${BLANK} │ ╰─SkippableItem { test-tag:"child:1" }
|${BLANK} ├─<subcomposition of LazyColumn>
Expand All @@ -612,6 +629,8 @@ class ComposeUiTest {

@Test fun scanningSubcomposition_includesSize() {
composeRule.setContent {
currentComposer.collectParameterInformation()

// Convert 10 px to DP, since output is always in px.
val sizeDp = with(LocalDensity.current) { 10.toDp() }

Expand Down Expand Up @@ -646,8 +665,8 @@ class ComposeUiTest {

assertThat(hierarchy).isEqualTo(
"""
|CompositionLocalProvider:
|${BLANK}CompositionLocalProvider { 10×30px, test-tag:"parent" }
|Box:
|${BLANK}Box { 10×30px, test-tag:"parent" }
|${BLANK}╰─MultipleSubcompositionLayout { 10×30px, test-tag:"subcompose-layout" }
|${BLANK} ├─<subcomposition of MultipleSubcompositionLayout>
|${BLANK} │ ├─Box { 10×10px, test-tag:"child1" }
Expand Down
2 changes: 1 addition & 1 deletion compose-unsupported-tests/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ android {
}

composeOptions {
kotlinCompilerExtensionVersion = Versions.Compose
kotlinCompilerExtensionVersion = Versions.ComposeCompiler
}

packaging {
Expand Down
1 change: 1 addition & 0 deletions radiography/src/main/java/radiography/ExperimentalRadiographyComposeApi.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,5 @@ package radiography
message = "This API is experimental, may only work with a specific version of Compose, " +
"and may change or break at any time. Use with caution."
)
@Retention(AnnotationRetention.BINARY)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint complained that experimental annotations should have binary retention

public annotation class ExperimentalRadiographyComposeApi
18 changes: 18 additions & 0 deletions radiography/src/main/java/radiography/ScannableView.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package radiography
import android.view.View
import android.view.ViewGroup
import androidx.compose.ui.Modifier
import androidx.compose.ui.node.ModifierNodeElement
import androidx.compose.ui.node.SemanticsModifierNode
import androidx.compose.ui.semantics.SemanticsConfiguration
import androidx.compose.ui.semantics.SemanticsModifier
import androidx.compose.ui.semantics.SemanticsNode
Expand Down Expand Up @@ -68,9 +70,25 @@ public sealed class ScannableView {
public val semanticsConfigurations: List<SemanticsConfiguration>
get() {
return semanticsNodes.map { it.config }.ifEmpty { null }
// Before Compose 1.6, semantics could be held in a SemanticsModifier
?: modifiers
.filterIsInstance<SemanticsModifier>()
.map { it.semanticsConfiguration }
.ifEmpty { null }
// After Compose 1.6, semantics are held in nodes. While this usually gets represented
// by semanticsNodes, sometimes they instead are only found in SemanticsModifierNode,
// which we must then extract.
?: modifiers
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed to add this to get a few tests to pass. In some cases, the semanticsNodes are not present for a layout node (and I'm not sure why). However, I noticed that they are instead found within a special type of modifier node that we can leverage.

I would love if someone more familiar with compose internals can validate this approach. This fixes all test cases, but I am not 100% confident that it is the best way to do so. Ideally I think the computeLayoutInfos would be able to identify the semanticsNodes for each LayoutNodeInfo instead of needing to do this

.filterIsInstance<ModifierNodeElement<*>>()
.map { it.create() }
.filterIsInstance<SemanticsModifierNode>()
.map { semanticsModifierNode ->
semanticsModifierNode.run {
val semanticsConfiguration = SemanticsConfiguration()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this creates a new SemanticsConfiguration() for each node - I wasn't sure if that was best, or if we should use the same SemanticsConfiguration() instance and apply each semantics node to it. having a list of SemanticsConfiguration seems to be consistent with previous behavior though

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure exactly how we'd want to handle semantics merging here. I think the existing Radiography implementation uses merged config, but since it also gets the configuration per-node, it might not be what "merged config" usually means? It might not matter much - since we are already grouping by node, we can probably just always merge semantics for all modifier nodes on the current node. So we'd probably want to share a SemanticsConfiguration object for everything on the same node, and make sure we're calling the modifiers in the right order. We might also need to watch out for clearAndSetSemantics, but again I'm not sure we care for Radiography's use case.

semanticsConfiguration.applySemantics()
semanticsConfiguration
}
}
}
}

Expand Down
6 changes: 2 additions & 4 deletions radiography/src/main/java/radiography/ViewStateRenderers.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import android.widget.TextView
import androidx.compose.ui.ExperimentalComposeUiApi
import androidx.compose.ui.layout.LayoutIdParentData
import androidx.compose.ui.semantics.ScrollAxisRange
import androidx.compose.ui.semantics.SemanticsModifier
import androidx.compose.ui.semantics.SemanticsProperties
import androidx.compose.ui.semantics.SemanticsProperties.EditableText
import androidx.compose.ui.semantics.SemanticsProperties.Text
Expand Down Expand Up @@ -72,11 +71,10 @@ public object ViewStateRenderers {

// Semantics
composeView
.modifiers
.filterIsInstance<SemanticsModifier>()
.semanticsConfigurations
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this, and the change in Semantics.kt, are the main changes to support 1.7, where we need to use the semantics nodes instead of the modifiers

// Technically there can be multiple semantic modifiers on a single node, so read them
// all.
.flatMap { semantics -> semantics.semanticsConfiguration }
.flatten()
.forEach { (key, value) ->
when (key) {
SemanticsProperties.TestTag -> appendLabeledValue("test-tag", value)
Expand Down
7 changes: 2 additions & 5 deletions radiography/src/main/java/radiography/internal/Semantics.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,16 @@
package radiography.internal

import androidx.compose.ui.semantics.SemanticsModifier
import androidx.compose.ui.semantics.SemanticsProperties
import radiography.ScannableView.ComposeView
import radiography.ExperimentalRadiographyComposeApi

/** Returns all tag strings set on the composable via `Modifier.testTag`. */
@OptIn(ExperimentalRadiographyComposeApi::class)
internal fun ComposeView.findTestTags(): Sequence<String> {
return modifiers
return semanticsConfigurations
.asSequence()
.filterIsInstance<SemanticsModifier>()
.flatMap { semantics ->
semantics.semanticsConfiguration.asSequence()
.filter { it.key == SemanticsProperties.TestTag }
semantics.filter { it.key == SemanticsProperties.TestTag }
}
.mapNotNull { it.value as? String }
}
5 changes: 3 additions & 2 deletions sample-compose/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ plugins {
}

/** Use a separate property for the sample so we can test with different versions easily. */
val sampleComposeVersion = "1.5.3"
val sampleComposeVersion = "1.6.8"
val sampleComposeCompilerVersion = "1.5.3"

android {
compileSdk = 34
Expand All @@ -28,7 +29,7 @@ android {
}

composeOptions {
kotlinCompilerExtensionVersion = sampleComposeVersion
kotlinCompilerExtensionVersion = sampleComposeCompilerVersion
}

packaging {
Expand Down
2 changes: 2 additions & 0 deletions sample-compose/src/main/java/com/squareup/radiography/sample/compose/MainActivity.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@ package com.squareup.radiography.sample.compose
import android.os.Bundle
import androidx.activity.compose.setContent
import androidx.appcompat.app.AppCompatActivity
import androidx.compose.runtime.currentComposer

class MainActivity : AppCompatActivity() {
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
setContent {
currentComposer.collectParameterInformation()
ComposeSampleApp()
}
}
Expand Down
Loading