Status Update
No update yet.
Comments
si...@google.com
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
ae...@google.com
There is only one remaining data class in text module, AnnotatedString.Range<T> (AnnotatedString.kt line 222). The 45 others are elsewhere in Compose.
lp...@google.com
Are data classes something we never want to use in public API? Is it worth filing a feature request for Metalava to throw an error if you try and use one?
ae...@google.com
Not sure other people's opinion, but I'm thinking it probably would make sense to simply lint/metalava-ban it, yes. In some rare cases there is a deliberate intention to support destructuring as a recommended API usage, such as the old val (value, setValue) = state { 0 } API. But if so, that can be done using explicit component1, component2 functions instead (that's clearer about API intent and also doesn't bring in the equals implementation which might not be needed).
I get the impression the majority of the time people use "data" keyword basically just for the explanatory value of "this is Plain Old Data like a C struct: I intend to keep this simple and to make copies of it" which is a fine enough attitude in an internal/private class, but not appropriate for a public API.
an...@google.com
ae...@google.com
Hmm. I notice when attempting to remove data keyword from ProgressBarRangeInfo, one of the ProgressSemanticsTest fails because the test matchers use ==. When data is removed, then == silently has the same meaning as === in Kotlin.
So it is hard to statically determine whether there are any uses of == with a data class, and a systematic removal may introduce bugs which may not all be caught by tests. I guess the safe way to remove data is to make sure we also add equals to all of them at the same time. (As I understand it, the other methods: hashCode, toString, componentN, and copy would all cause a compile error if something relies on them, so we don't need to worry about those as much.)
si...@google.com
hashcode sounds extra dangerous.
ToString is a good utility while debugging our code, and developers
debugging our code.since all 3 methods can be automatically created using
android studio, i dont see a reason/value to create equals but not hashcode
and tostring.
On Mon, Feb 1, 2021, 16:34 aelias <buganizer-system+aelias@google.com>
wrote:
ae...@google.com
Makes sense. Also, equals/hashCode/toString are the three methods on Any, so overriding them doesn't affect our public API at all (they don't change any lines in current.txt).
ae...@google.com
OK, I did a survey of all the data classes and uploaded a patch to remove a bunch at aosp/1569820 . In the end, I decided the majority of the data classes looked pretty appropriate to use (things like IntRect) and I left them as is, so I changed my mind about the lint rule, it would be too cumbersome to ban it. The API review process can reasonably take care of it in the future.
ap...@google.com
Branch: androidx-main
commit 9b3a65a1038c2da421e0dda0781d2189563e2c6f
Author: Alexandre Elias <aelias@google.com>
Date: Mon Feb 01 21:42:52 2021
Replace data classes with equals/hashCode/toString
This replaces "data" with the Android Studio autogenerated
implementations of equals/hashCode/toString in several classes, in order
to minimize public API surface and improve extensibility.
Note that the majority of the uses of "data" are in dead-simple classes
in graphics, geometry and layout like "IntRect", and I decided it was
appropriate to keep using "data" for those.
Bug: 178659281
Test: Existing tests
Relnote: "Destructuring and copy() methods have been removed from
several classes where they were rarely used."
Change-Id: I267021d3a45314acc9a169f6bbdfbfb4295a448c
M compose/animation/animation-core/api/current.txt
M compose/animation/animation-core/api/public_plus_experimental_current.txt
M compose/animation/animation-core/api/restricted_current.txt
M compose/animation/animation-core/src/commonMain/kotlin/androidx/compose/animation/core/DynamicTargetAnimation.kt
M compose/foundation/foundation/api/current.txt
M compose/foundation/foundation/api/public_plus_experimental_current.txt
M compose/foundation/foundation/api/restricted_current.txt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/BorderStroke.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/animation/FlingConfig.kt
M compose/material/material/api/current.txt
M compose/material/material/api/public_plus_experimental_current.txt
M compose/material/material/api/restricted_current.txt
M compose/material/material/src/commonMain/kotlin/androidx/compose/material/Swipeable.kt
M compose/ui/ui-graphics/api/current.txt
M compose/ui/ui-graphics/api/public_plus_experimental_current.txt
M compose/ui/ui-graphics/api/restricted_current.txt
M compose/ui/ui-graphics/src/commonMain/kotlin/androidx/compose/ui/graphics/Shadow.kt
M compose/ui/ui-graphics/src/commonMain/kotlin/androidx/compose/ui/graphics/drawscope/DrawScope.kt
M compose/ui/ui-graphics/src/commonMain/kotlin/androidx/compose/ui/graphics/painter/ColorPainter.kt
M compose/ui/ui-graphics/src/commonMain/kotlin/androidx/compose/ui/graphics/painter/ImagePainter.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/commonMain/kotlin/androidx/compose/ui/autofill/Autofill.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/semantics/SemanticsProperties.kt
Description
With the below command, I see Compose has 46 data classes in our public API. This commits us to supporting APIs like equals() and component1(). It means among other things that changing the order of non-constructor fields in the class would be a breaking API change. We should do a cleanup pass and remove "data" except in the few cases where destructuring style is part of our documented sample code.
frameworks/support/compose$ git grep 'method public.*component1();' -- */current.txt