Support Compose 1.6 and 1.7 #169
Conversation
| @@ -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" | |||
There was a problem hiding this comment.
I didn't want to attempt to update kotlin at the same time, so I left the compose compiler version
| @@ -393,8 +396,8 @@ class ComposeUiTest { | |||
|
|
|||
| assertThat(hierarchy).isEqualTo( | |||
| """ | |||
| |CompositionLocalProvider: | |||
| |${BLANK}CompositionLocalProvider { test-tag:"parent" } | |||
| |Box: | |||
There was a problem hiding this comment.
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}╰─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" } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
That seems like a good idea, but we can fix in a follow-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) | |||
There was a problem hiding this comment.
Lint complained that experimental annotations should have binary retention
| @@ -72,11 +71,10 @@ public object ViewStateRenderers { | |||
|
|
|||
| // Semantics | |||
| composeView | |||
| .modifiers | |||
| .filterIsInstance<SemanticsModifier>() | |||
| .semanticsConfigurations | |||
There was a problem hiding this comment.
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
| // 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 |
There was a problem hiding this comment.
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<SemanticsModifierNode>() | ||
| .map { semanticsModifierNode -> | ||
| semanticsModifierNode.run { | ||
| val semanticsConfiguration = SemanticsConfiguration() |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
Thanks Eli! @pyricau @zach-klippenstein, do you have bandwidth to review? |
This PR makes a few updates to get all tests passing on the latest compose 1.7 beta version, addressing some changes for 1.6 and 1.7.
First, it addresses an issue due to 1.6 (#159) by adding calls to
currentComposer.collectParameterInformation()within test functions. This fixes tests within Radiography to make the project compatible, but does not address the potential of providing some functionality to end users to apply this easily. It is a fairly easy function to add, so perhaps the documentation should just be updated to included a note about this (I didn't attempt to do that in this PR though)The other main fix is to the
findTestTagsfunction and compose renderer - these broke because they were trying to read semantic information from the compose modifiers; in newer versions of compose the semantics information is only present in semantic nodes. I had previously updated Radiography to expose that data for Compose 1.5 (#157) but these usage sites hadn't been updated and had broken.