Support composed{} based modifier in parameter inspection [163494569]

Fixed

Feature Request

Status Update

No update yet.

Description

ma...@google.com

created issue #1

Aug 11, 2020 01:11PM

We want to be able to expect modifiers that are based on the composed{} construct and it's not possible right now because we need to have composition scope to be able to materialize them.

For example, Modifier.border is based on composed{} and we're not able to currently inspect its parameters.

Comments

ch...@google.com <ch...@google.com> #2Aug 12, 2020 06:13PM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit b90079595f33f58fece04026a97faa0d243acdb1
Author: Yuichi Araki <yaraki@google.com>
Date: Wed Sep 18 16:55:49 2019

Change the way to detect mismatch between POJO and query

This fixes cursor mismatch warnings with expandProjection.

Bug: 140759491
Test: QueryMethodProcessorTest
Change-Id: I7659002e5e0d1ef60fc1af2a625c0c36da0664d8

M room/compiler/src/main/kotlin/androidx/room/processor/QueryMethodProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/solver/query/result/PojoRowAdapter.kt
M room/compiler/src/test/kotlin/androidx/room/processor/QueryMethodProcessorTest.kt
M room/compiler/src/test/kotlin/androidx/room/testing/TestProcessor.kt

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

https://goto.google.com/android-sha1/b90079595f33f58fece04026a97faa0d243acdb1

jl...@google.com <jl...@google.com> #3Aug 12, 2020 10:00PM

Reassigned to ch...@google.com.

The following changes were cherrypicked through Coastguard (

https://android-build.googleplex.com/coastguard/)
Release Track: (

https://android-build.googleplex.com/coastguard/dashboard/6093116828680192/#/cherrypickHistory/all)
Created: 2019-10-14 17:23:42.610596+00:00
Changes: aosp/1123258

ComposedModifier.png

337 KB

View

Download

ch...@google.com <ch...@google.com> #4Aug 12, 2020 10:42PM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit bdde5a1a970ddc9007b28de4aa29d60ffa588f08
Author: Yigit Boyar <yboyar@google.com>
Date: Thu Apr 16 16:47:05 2020

Re-factor how errors are dismissed when query is re-written

This CL changes how we handle errors/warnings if query is
re-written.
There was a bug in expandProjection where we would report warnings
for things that Room already fixes automatically (

b/140759491

).
The solution to that problem (I7659002e5e0d1ef60fc1af2a625c0c36da0664d8)
solved it by deferring validating of columns until after re-write
decision is made. Unfortunately, this required changing PojoRowAdapter
to have a dummy mapping until it is validating, make it hard to use
as it does have a non-null mapping which is not useful.

This CL partially reverts that change and instead rely on the log
deferring logic we have in Context. This way, we don't need to break
the stability of PojoRowAdapter while still having the ability to
drop warnings that room fixes. This will also play nicer when we
have different query re-writing options that can use more information
about the query results.

Bug: 153387066
Bug: 140759491
Test: existing tests pass
Change-Id: I2ec967c763d33d7a3ff02c1a13c6953b460d1e5f

M room/compiler/src/main/kotlin/androidx/room/log/RLog.kt
M room/compiler/src/main/kotlin/androidx/room/processor/QueryMethodProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/solver/query/result/PojoRowAdapter.kt

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

jl...@google.com <jl...@google.com> #5Aug 12, 2020 10:51PM

Reassigned to jl...@google.com.

Oh, OK. Thanks.
Let me try that.

jl...@google.com <jl...@google.com> #6Aug 13, 2020 01:00AM

Reassigned to ch...@google.com.

Hi Chuck,
Now the question is: how do I determine which NodeGroup to get the modifierInfo from.
Here is a dump of part of the SlotTable. The top node is a Text node that in the source code has a border modifier (BorderModifier).
However the NodeGroup with the modifierInfo does not appear before much later in the tree (underneath "Text", "CoreText", "Layout", "Layout" - and a bunch of other call nodes).

How do I determine that the modifier parameter of the Text node is actually the modifiers found much later ?

CallGroup("Text") - with a "modifier" parameter
- CallGroup(null)
- CallGroup("remember")
- CallGroup("Text")
- CallGroup(null)
- CallGroup(null)
- CallGroup("remember")
- CallGroup("CoreText")
- CallGroup("Layout")
- CallGroup("remember")
- CallGroup(null)
- CallGroup("Layout")
- CallGroup("Layout")
- CallGroup(null)
- CallGroup(null)
- CallGroup("remember")
- CallGroup(null)
- CallGroup("remember")
- CallGroup(null)
- CallGroup("remember")
- CallGroup("emit")
- NodeGroup (this NodeGroup has the BorderModifier). <----------------------------- Here is the BorderModifier that was a parameter for the first Text group node
- CallGroup(null)
- CallGroup("onCommit")
- CallGroup("remember")

ch...@google.com <ch...@google.com> #7Aug 13, 2020 03:47PM

That I am not clear about since it depends on the implementation. There is no requirement that it even be used by it is highly likely to be.

However, it should always be the first NodeGroup in the call group as anything else means the implementation is probably wrong.

jl...@google.com <jl...@google.com> #8Aug 13, 2020 04:11PM

Reassigned to mo...@google.com.

Hi George,

Now that a modifier can be based the "composed{}" construct, the Layout Inspector has trouble displaying these modifier parameters.
An example here is:
Text(Modifier.border(width = 10.0.dp, color = Color.Black)

The parameter value seen on the CallGroup for the Text node is a "ComposedModifier" i.e. the un-materialized modifier.
Obviously the system must have calculated the materialized version somewhere.
And in fact as seen in #6 the materialized border is found in a NodeGroup under a child node 4 levels deep: Text -> Text -> CoreText -> Layout -> Layout
It would be nice we could determine that the "ComposedModifier" found for the top level Text is in fact the "BorderModifier" seen in NodeGroup.
But I fail to see how I can make that association.
Do you see a way?

mo...@google.com <mo...@google.com> #9Aug 18, 2020 03:57PM

I think I should be able to give access to the uncomposaed modifier so that you can access its information.

jl...@google.com <jl...@google.com> #10Aug 18, 2020 04:00PM

That would be great.
Thank you.

ap...@google.com <ap...@google.com> #11Oct 8, 2020 09:05PM

Marked as fixed.

Project: platform/frameworks/support
Branch: androidx-master-dev

commit 4faed1d7c573e5b475f549f159bbfe41278d47cd
Author: Jens Ole Lauridsen <jlauridsen@google.com>
Date: Wed Oct 07 15:01:31 2020

Add inspector info DSL to the composed modifier

This would allow the layout inspector to display information for
the unmaterialized modifiers found in the Compose parameters.

In this change: apply this for border modifiers.

Fixes: 163494569
Test: Unit test reenabled
Relnote: Add ability to specify inspector info in composed modifier
Change-Id: Idee08841816fb7dfc8f0621eb5a32c3663131aa1

M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/Border.kt
M compose/ui/ui/api/current.txt
M compose/ui/ui/api/public_plus_experimental_current.txt
M compose/ui/ui/api/restricted_current.txt
A compose/ui/ui/samples/src/main/java/androidx/compose/ui/samples/InspectorInfoInComposedModifierSamples.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/ComposedModifier.kt
M ui/ui-tooling/src/androidTest/java/androidx/ui/tooling/inspector/ParameterFactoryTest.kt

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