SwipeableState.value visibility [163129614]

Fixed

Bug

Status Update

No update yet.

Description

ni...@google.com

created issue #1

Aug 7, 2020 03:53PM

SwipeableState.value property is currently writeable but should likely be private to encourage consumers to use the [animate|snap]To methods. See (internal) discussion.

Comments

ma...@google.com <ma...@google.com> #2Aug 7, 2020 04:09PM

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

Message last modified on Aug 7, 2020 04:09PM

ni...@google.com <ni...@google.com> #3Aug 7, 2020 04:10PM

I'm on #TeamAnimate.

ni...@google.com <ni...@google.com> #4Aug 10, 2020 09:23AM

To expand on this, I feel like swipeable is intended for use cases with continuous changes so animating with the 'obvious' API feels right here. The only issue I see is that value feels like a property, so passing extra params like an animation spec feels odd.

That said, I dislike value being a snap as I feel like it makes it harder to create a smooth experience.

ap...@google.com <ap...@google.com> #5Aug 11, 2020 01:55PM

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

commit 3d00acd443e0e5dd2cc33eb314a65f78f21cec61
Author: Calin Tataru <calintat@google.com>
Date: Mon Aug 10 17:59:53 2020

Modifier.swipeable polishing

This CL does the following things:
* Expand the documentation in SwipeableState.
* Make the setter of SwipeableState.value private.
* Rename some of the properties in SwipeableState, and slightly change
the definition of swipeTarget (now called targetValue) so that it uses
the target value of the ongoing animation if isAnimationRunning = true.
* Add a rememberSwipeableState function to help reduce boilerplate.
* Add Savers for [Drawer/BottomDrawer/Dismiss/Swipeable]State, so that
we can use rememberSavedInstanceState in the rememberFooState functions.

Bug: 163129614
Bug: 163132293
Test: Ran SwipeableTest and other tests depending on swipeable
Relnote: "Renamed some properties in SwipeableState: swipeTarget ->
targetValue, swipeProgress -> progress, swipeDirection -> direction.
Added a rememberSwipeableState function for creating SwipeableStates."
Change-Id: I2fc9c3af465b579a18359ae0aa0853c2d2b02abe

M ui/ui-material/api/current.txt
M ui/ui-material/api/public_plus_experimental_current.txt
M ui/ui-material/api/restricted_current.txt
M ui/ui-material/samples/src/main/java/androidx/compose/material/samples/SwipeToDismissSamples.kt
M ui/ui-material/samples/src/main/java/androidx/compose/material/samples/SwipeableSamples.kt
M ui/ui-material/src/androidAndroidTest/kotlin/androidx/compose/material/SwipeableTest.kt
M ui/ui-material/src/commonMain/kotlin/androidx/compose/material/Drawer.kt
M ui/ui-material/src/commonMain/kotlin/androidx/compose/material/SwipeToDismiss.kt
M ui/ui-material/src/commonMain/kotlin/androidx/compose/material/Swipeable.kt
M ui/ui-material/src/commonMain/kotlin/androidx/compose/material/Switch.kt

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

ca...@google.com <ca...@google.com> #6Aug 11, 2020 02:13PM

I have made the setter of value private, so developers will have to use either snapTo or animateTo to change the value. This is the safest option and it's consistent with other classes like AnimatedValue.

ca...@google.com <ca...@google.com> #7Aug 11, 2020 02:16PM

Marked as fixed.

BTW the reason marking the setter as internal didn't work is that internal fields are visisble in subclasses and hence any modules that see those subclasses, which is interesting. Thanks again Nick for finding this!

Issue 163129614