Status Update
Comments
po...@google.com <po...@google.com> #2
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
an...@google.com <an...@google.com> #3
1) What will meant layoutCoordinates.parentCoordinates? currently it is easy to understand that you go to the parent layoutnode. with this change we will go to the next modifier. is it the behavior people want? and do they use this method at all?
2) Similar for layoutCoordinates.providedAlignmentLines. Right now it is a set of all alignment lines, from modifiers and from the layoutnode's measure block. will it return only the alignments passed by this exact modifier?
mo...@google.com <mo...@google.com> #4
I will add a new property to access the parent layout modifier/parent layout. I think this might be useful to developers. I think we should still keep the parentCoordinates
as-is, though -- it is likely the most commonly desired API.
For 2), I think it makes sense to include all alignment lines provided so far in the modifier chain. I don't think that is inconsistent or difficult to understand.
I will send you a CL to handle the parent coordinates.
ap...@google.com <ap...@google.com> #5
Branch: androidx-main
commit af7fc218d744cf0a2560d9352a1efe88cb02c9e9
Author: George Mount <mount@google.com>
Date: Fri Jan 22 00:00:19 2021
Added ability to get parent layout modifier's coordinates
Bug: 177926591
Relnote: "Renamed LayoutCoordinates.parentCoordinates to
LayoutCoordinates.parentLayoutCoordinates to allow for a new
parentCoordinates property. The parentCoordinates property
now offers the parent modifier's LayoutCoordintes. This will
make for more complete use cases for onSizeChanged() and
onGloballyPositioned()"
Test: new test
Change-Id: Idfbfd3012d88b8661b097cb50e4554f2b73f3b64
M compose/foundation/foundation/src/test/kotlin/androidx/compose/foundation/text/TextFieldDelegateTest.kt
M compose/ui/ui-test/src/commonMain/kotlin/androidx/compose/ui/test/Actions.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/samples/src/main/java/androidx/compose/ui/samples/OnGloballyPositionedSamples.kt
M compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/input/pointer/HitPathTrackerTest.kt
M compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/input/pointer/PointerInteropFilterTest.kt
M compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/layout/OnGloballyPositionedTest.kt
M compose/ui/ui/src/androidMain/kotlin/androidx/compose/ui/window/AndroidPopup.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/layout/LayoutCoordinates.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/node/InnerPlaceable.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/node/LayoutNodeWrapper.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/node/ModifiedLayoutNode.kt
M compose/ui/ui/src/test/kotlin/androidx/compose/ui/node/LayoutNodeTest.kt
M compose/ui/ui/src/test/kotlin/androidx/compose/ui/selection/MockCoordinates.kt
ae...@google.com <ae...@google.com> #6
After I ran myself into unexpected brittleness in unit tests caused by onGloballyPositioned's current behavior, I've come to feel this is quite important to fix. So I would ideally like to see this issue remain in scope for 1.0 (since the current behavior will become sticky once we have enough developer adoption).
Andrey, I understand it is hard for you to find time to work on this given the other top priorities on your plate though. George, do you think you might be able to squeeze this change in whenever you get a break from overscroll and CTS tests? (You can reassign to me if not. This is something I might also try to find the cycles to fix myself given it's my pet peeve and I'm familiar with the area.)
an...@google.com <an...@google.com>
an...@google.com <an...@google.com> #7
I wasn't sure I will find time for this task, but after you mentioned the importance of it I decided to explore and realised there is a pretty trivial solution. So I went ahead and uploaded aosp/1707955
ae...@google.com <ae...@google.com> #8
Great, thanks Andrey!
ap...@google.com <ap...@google.com> #9
Branch: androidx-main
commit eef6d918107789be7c43b293df82d2b9ae02f058
Author: Andrey Kulikov <andreykulikov@google.com>
Date: Fri May 14 13:12:45 2021
Change Modifier.onGloballyPositioned to report the modifier coordinates
Relnote: Modifier.onGloballyPositioned() was changed to report the coordinates of this modifier in the modifier chain, not the layout coordinates after applying all the modifiers. This means that now the ordering of modifiers is affecting what coordinates would be reported.
Fixes: 177926591
Test: OnGloballyPositionedTest
Change-Id: Ieb67da0c327c9dc323a4b0a8bf33dbb66f0611e3
M compose/animation/animation/src/androidAndroidTest/kotlin/androidx/compose/animation/AnimatedVisibilityTest.kt
M compose/foundation/foundation-layout/src/androidAndroidTest/kotlin/androidx/compose/foundation/layout/AlignmentLineTest.kt
M compose/foundation/foundation-layout/src/androidAndroidTest/kotlin/androidx/compose/foundation/layout/IntrinsicTest.kt
M compose/foundation/foundation/samples/src/main/java/androidx/compose/foundation/samples/IndicationSamples.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/textfield/TextFieldDefaultWidthTest.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/text/CoreTextField.kt
M compose/material/material/src/androidAndroidTest/kotlin/androidx/compose/material/BottomSheetScaffoldTest.kt
M compose/material/material/src/androidAndroidTest/kotlin/androidx/compose/material/ScaffoldTest.kt
M compose/material/material/src/commonMain/kotlin/androidx/compose/material/Drawer.kt
M compose/material/material/src/commonMain/kotlin/androidx/compose/material/ModalBottomSheet.kt
M compose/ui/ui-inspection/src/androidTest/java/androidx/compose/ui/inspection/rules/ComposeInspectionRule.kt
M compose/ui/ui-tooling-data/src/androidTest/java/androidx/compose/ui/tooling/data/ToolingTest.kt
M compose/ui/ui-tooling/src/androidTest/java/androidx/compose/ui/tooling/ToolingTest.kt
M compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/draw/GraphicsLayerTest.kt
M compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/layout/OnGloballyPositionedTest.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/node/LayoutNode.kt
A compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/node/OnGloballyPositionedModifierWrapper.kt
Description
Based on comment:https://android-review.googlesource.com/c/platform/frameworks/support/+/1552681/comment/e10d2507_68d1ed41/
We've changed onSizeChanged to report the size at the position of the modifier, but onGloballyPositioned() reports the size/position of the content. Should this be changed to be the size/position of the place in the modifier chain? Andrey, as you wrote the original modifier, you know the use cases better and can comment on it.