Provide an API to group modifiers under one InspectorInfo [191017532]

Fixed

Bug

Status Update

No update yet.

Description

lp...@google.com

created issue #1

Jun 14, 2021 01:41PM

Currently we use Modifier.composed in many places to 'wrap' modifiers with a new InspectorInfo, while ignoring any infos used internally in the implementation detail of the modifiers.

For example, Modifier.selectable currently looks like:

composed(
    factory = {
        Modifier.clickable(
            enabled = enabled,
            role = role,
            interactionSource = interactionSource,
            indication = indication,
            onClick = onClick
        ).semantics {
            this.selected = selected
        }
    },
    inspectorInfo = debugInspectorInfo {
        name = "selectable"
        properties["selected"] = selected
        properties["enabled"] = enabled
        properties["role"] = role
        properties["interactionSource"] = interactionSource
        properties["indication"] = indication
        properties["onClick"] = onClick
    }
)

In this case using composed is important, as we want to hide the clickable's InspectorInfo, and replace it with a more meaningful one for selectable.

However, using Modifier.composed like this when there are no Composable functions being called in the implementation is expensive - it means that the modifier can never be skipped, and this can cause performance degradation. As a result, this shouldn't be composed, and we will need a new specific API to ignore the internal info.

Comments

lp...@google.com <lp...@google.com> #2Jun 14, 2021 01:42PM

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

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

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

ap...@google.com <ap...@google.com> #3Jun 17, 2021 12:41AM

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

commit 1fde6045b506ad00e3dd3d3acbecec4cc1aa7eec
Author: Louis Pullen-Freilich <lpf@google.com>
Date: Mon Jun 14 14:52:11 2021

Adds lint check to warn against using Modifier.composed unnecessarily

Using Modifier.composed without calling any Composable functions is unnecessary, and since Modifier.composed results in an unskippable modifier, it is worse for performance. Suppresses existing usages that cannot be migrated until we have a new API for overriding / grouping modifiers into one InspectorInfo without using Modifier.composed.

Fixes:

b/190036254

Bug:

b/191017532

Test: ComposedModifierDetectorTest
Test: lintDebug
Change-Id: Icd9d738a90b837fe628e644f7b2cc676c15c1ddb

M buildSrc/src/main/kotlin/androidx/build/AndroidXComposePlugin.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/Border.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/selection/Selectable.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/selection/Toggleable.kt
M compose/lint/common/src/main/java/androidx/compose/lint/ComposableUtils.kt
M compose/lint/common/src/main/java/androidx/compose/lint/Names.kt
M compose/material/material/src/commonMain/kotlin/androidx/compose/material/ModalBottomSheet.kt
A compose/ui/ui-lint/src/main/java/androidx/compose/ui/lint/ComposedModifierDetector.kt
M compose/ui/ui-lint/src/main/java/androidx/compose/ui/lint/UiIssueRegistry.kt
A compose/ui/ui-lint/src/test/java/androidx/compose/ui/lint/ComposedModifierDetectorTest.kt
M compose/ui/ui/samples/src/main/java/androidx/compose/ui/samples/InspectorInfoInComposedModifierSamples.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/draw/Shadow.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/input/key/KeyInputModifier.kt
M compose/ui/ui/src/test/kotlin/androidx/compose/ui/ComposedModifierTest.kt

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

jl...@google.com <jl...@google.com> Aug 10, 2021 10:34PM

Accepted by jl...@google.com.

ap...@google.com <ap...@google.com> #4Aug 20, 2021 10:31PM

Marked as fixed.

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

commit abd35d05cfb230005cd43871e45f44b03270b643
Author: Jens Ole Lauridsen <jlauridsen@google.com>
Date: Fri Jul 09 15:36:36 2021

Provide API to group modifiers for inspection

Give an alternate to Modifier.composed such that it is possible
to create a modifier with inspector info that wraps one or more
modifiers.

Also: Update the ModifierInspectorInfo lint check to accept the
added inspectable modifier.

Fixes: 191017532
Test: Added unit test
Change-Id: I1909b068b72041b8363c04c7ec619fcf26702da4
Relnote: Added Modifier.inspectable for wrapping other modifiers.

M compose/lint/internal-lint-checks/src/main/java/androidx/compose/lint/ModifierInspectorInfoDetector.kt
M compose/lint/internal-lint-checks/src/test/java/androidx/compose/lint/ModifierInspectorInfoDetectorTest.kt
M compose/ui/ui-inspection/src/androidTest/java/androidx/compose/ui/inspection/inspector/ParameterFactoryTest.kt
M compose/ui/ui-inspection/src/main/java/androidx/compose/ui/inspection/inspector/ParameterFactory.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/InspectableModifierSample.kt
A compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/platform/InspectableValueTest.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/platform/InspectableValue.kt

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

lp...@google.com <lp...@google.com> #5Aug 21, 2021 07:09PM

Status: Assigned (reopened)

Not entirely sure what is wrong, but I think there might be some problem with the implementation of InspectableModifierImpl - in my CL to migrate some unnecessary Modifier.composed usages to Modifier.inspectable I'm running into test failures.

http://aosp/1804063

If you revert the changes to Modifier.shadow, then these tests seem to pass again - so maybe there is some problem with the folding logic.

I tried adding some logging code to see what is going on, printing the layout node wrapper chain shows the following:

With composed:

With inspectable:

[androidx.compose.ui.node.ModifiedLayoutNode@f2be456, androidx.compose.ui.input.nestedscroll.NestedScrollDelegatingWrapper@5c69cd7, androidx.compose.ui.node.PointerInputDelegatingWrapper@ea479c4, androidx.compose.ui.semantics.SemanticsWrapper@ab9c0ad id: 3 config: SemanticsConfiguration@848c743{ Expand : AccessibilityAction(label=null, action=Function0<java.lang.Boolean>) }, androidx.compose.ui.node.ModifiedLayoutNode@a516e30, androidx.compose.ui.node.ModifiedLayoutNode@d5ebaa9, androidx.compose.ui.node.ModifiedLayoutNode@5eb282e, androidx.compose.ui.input.nestedscroll.NestedScrollDelegatingWrapper@cf24fcf, androidx.compose.ui.node.PointerInputDelegatingWrapper@973d5c, androidx.compose.ui.semantics.SemanticsWrapper@63f5865 id: 4 config: SemanticsConfiguration@48571c0{ Expand : AccessibilityAction(label=null, action=Function0<java.lang.Boolean>) }, androidx.compose.ui.node.ModifiedLayoutNode@8ee19eb, androidx.compose.ui.node.ModifiedDrawNode@8d348, androidx.compose.ui.node.ModifiedLayoutNode@5d215e1, androidx.compose.ui.semantics.SemanticsWrapper@a240f06 id: 5 config: SemanticsConfiguration@ffa1ef9{ }, androidx.compose.ui.node.PointerInputDelegatingWrapper@b7bdbf4]

So naively it looks like we are adding things twice. It might be a good idea to add a test for the folding logic in any case, since we are overriding it and doing something special here.

ap...@google.com <ap...@google.com> #6Aug 26, 2021 07:13PM

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

commit ed0f91bff603994abc7102301310bf452d9e1637
Author: Jens Ole Lauridsen <jlauridsen@google.com>
Date: Wed Aug 25 09:14:36 2021

Redo the implementation of inspectable

The foldIn and foldOut logic did not work well when combined with
composed modifiers which implementation is calling foldIn repeatedly.

Instead use a start and end marker in the list of Modifier elements.

This fixes a set of tests that would fail with the earlier
implementation.

Bug: 191017532
Test: Fixes existing tests after using inspectable
Relnote: Redo implementation of inspectable
Change-Id: I927bc731712d81f47aefdf946f778047c7645b26

M compose/ui/ui-inspection/src/main/java/androidx/compose/ui/inspection/inspector/ParameterFactory.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
M compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/platform/InspectableValueTest.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/platform/InspectableValue.kt

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

ap...@google.com <ap...@google.com> #7Aug 26, 2021 07:13PM

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

commit fd22208ed7a4d4b9978e55d2547b6762a41210ef
Author: Jens Ole Lauridsen <jlauridsen@google.com>
Date: Mon Aug 23 14:22:53 2021

Do not include preceding modifiers in wrapped inspectable

Bug: 191017532
Test: Fixes existing tests after using inspectable
Relnote: Changed parameter name of inspectable to match composed
Change-Id: I3a482c9f64ef0f7cc5a5a59cecaec010ca001e3f

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
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/platform/InspectableValue.kt

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

ap...@google.com <ap...@google.com> #8Aug 26, 2021 07:13PM

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

commit e5a77235e4d05b3d0690ee9f3c8297a34ebc17ef
Author: Louis Pullen-Freilich <lpf@google.com>
Date: Sat Aug 21 01:57:02 2021

Fixes existing UnnecessaryComposedModifier issues

Bug:

b/191017532

Test: lintDebug
Change-Id: I1a4db29446accdd136e4d94a855fc0f3e25b58d7

M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/SelectableTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/ToggleableTest.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/Border.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/selection/Selectable.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/selection/Toggleable.kt
M compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/draw/ShadowTest.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/draw/Shadow.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/input/key/KeyInputModifier.kt
M compose/ui/ui/src/test/kotlin/androidx/compose/ui/input/key/KeyInputModifierTest.kt

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

jl...@google.com <jl...@google.com> #9Aug 30, 2021 10:40PM