Status Update
No update yet.
Comments
cl...@google.com
an...@google.com
Branch: androidx-master-dev
commit fcf76b3241844f18106d6e850004952046d4bfb6
Author: Leland Richardson <lelandr@google.com>
Date: Wed May 13 16:58:59 2020
Deprecate @Model
Relnote: “
@Model annotation is now deprecated. Use state and mutableStateOf as alternatives. This deprecation decision was reached after much careful discussion.
Justification
=============
Rationale includes but is not limited to:
- Reduces API surface area and concepts we need to teach
- More closely aligns with other comparable toolkits (Swift UI, React, Flutter)
- Reversible decision. We can always bring @Model back later.
- Removes corner-case usage and difficult to answer questions about configuring @Model as things we need to handle
- @Model data classes, equals, hashcode, etc.
- How do I have some properties “observed” and others not?
- How do I specify structural vs. referential equality to be used in observation?
- Reduces “magic” in the system. Would reduce the likelihood of someone assuming system was smarter than it is (ie, it knowing how to diff a list)
- Makes the granularity of observation more intuitive.
- Improves refactorability from variable -> property on class
- Potentially opens up possibilities to do hand-crafted State-specific optimizations
- More closely aligns with the rest of the ecosystem and reduces ambiguity towards immutable or us “embracing mutable state”
Migration Notes
===============
Almost all existing usages of @Model are fairly trivially transformed in one of two ways. The example below has a @Model class with two properties just for the sake of example, and has it being used in a composable.
```
@Model class Position(
var x: Int,
var y: Int
)
@Composable fun Example() {
var p = remember { Position(0, 0) }
PositionChanger(
position=p,
onXChange={ p.x = it }
onYChange={ p.y = it }
)
}
```
Alternative 1: Use State<OriginalClass> and create copies.
----------------------------------------------------------
This approach is made easier with Kotlin’s data classes. Essentially, make all previously `var` properties into `val` properties of a data class, and then use `state` instead of `remember`, and assign the state value to cloned copies of the original using the data class `copy(...)` convenience method.
It’s important to note that this approach only works when the only mutations to that class were done in the same scope that the `State` instance is created. If the class is internally mutating itself outside of the scope of usage, and you are relying on the observation of that, then the next approach is the one you will want to use.
```
data class Position(
val x: Int,
val y: Int
)
@Composable fun Example() {
var p by state { Position(0, 0) }
PositionChanger(
position=p,
onXChange={ p = p.copy(x=it) }
onYChange={ p = p.copy(y=it) }
)
}
```
Alternative 2: Use mutableStateOf and property delegates
--------------------------------------------------------
This approach is made easier with Kotlin’s property delegates and the `mutableStateOf` API which allows you to create MutableState instances outside of composition. Essentially, replace all `var` properties of the original class with `var` properties with `mutableStateOf` as their property delegate. This has the advantage that the usage of the class will not change at all, only the internal implementation of it. The behavior is not completely identical to the original example though, as each property is now observed/subscribed to individually, so the recompositions you see after this refactor could be more narrow (a good thing).
```
class Position(x: Int, y: Int) {
var x by mutableStateOf(x)
var y by mutableStateOf(y)
}
// source of Example is identical to original
@Composable fun Example() {
var p = remember { Position(0, 0) }
PositionChanger(
position=p,
onXChange={ p.x = it }
onYChange={ p.y = it }
)
}
```
“
Bug: 156546430
Bug: 152993135
Bug: 152050010
Bug: 148866188
Bug: 148422703
Bug: 148394427
Bug: 146362815
Bug: 146342522
Bug: 143413369
Bug: 135715219
Bug: 126418732
Bug: 147088098
Bug: 143263925
Bug: 139653744
Change-Id: I409e8c158841eae1dd548b33f1ec80bb609cba31
M compose/compose-compiler-hosted/integration-tests/src/test/java/androidx/compose/plugins/kotlin/frames/FrameDiagnosticTests.kt
M compose/compose-runtime/api/0.1.0-dev12.txt
M compose/compose-runtime/api/current.txt
M compose/compose-runtime/api/public_plus_experimental_0.1.0-dev12.txt
M compose/compose-runtime/api/public_plus_experimental_current.txt
M compose/compose-runtime/api/restricted_0.1.0-dev12.txt
M compose/compose-runtime/api/restricted_current.txt
M compose/compose-runtime/compose-runtime-benchmark/src/androidTest/java/androidx/compose/benchmark/ComposeBenchmark.kt
M compose/compose-runtime/compose-runtime-benchmark/src/androidTest/java/androidx/compose/benchmark/SiblingBenchmark.kt
M compose/compose-runtime/compose-runtime-benchmark/src/androidTest/java/androidx/compose/benchmark/dbmonster/DbMonster.kt
M compose/compose-runtime/compose-runtime-benchmark/src/androidTest/java/androidx/compose/benchmark/realworld4/RealWorld4_DataModels.kt
M compose/compose-runtime/samples/src/main/java/androidx/compose/samples/ModelSamples.kt
M compose/compose-runtime/src/commonMain/kotlin/androidx/compose/FrameManager.kt
M compose/compose-runtime/src/commonMain/kotlin/androidx/compose/Model.kt
M compose/compose-runtime/src/commonMain/kotlin/androidx/compose/MutableState.kt
M compose/compose-runtime/src/commonMain/kotlin/androidx/compose/Observe.kt
M compose/compose-runtime/src/commonMain/kotlin/androidx/compose/Recompose.kt
M compose/compose-runtime/src/commonMain/kotlin/androidx/compose/StableMarker.kt
M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/ModelObserverBenchmark.kt
M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/RadioGroupBenchmark.kt
M ui/integration-tests/demos/src/main/java/androidx/ui/demos/DemoColorPalette.kt
M ui/integration-tests/src/main/java/androidx/ui/integration/test/foundation/RectsInColumnSharedModelTestCase.kt
M ui/ui-android-view/integration-tests/android-view-demos/src/main/java/androidx/ui/androidview/demos/WebComponentActivity.kt
M ui/ui-animation/src/main/java/androidx/ui/animation/AnimatedValueEffects.kt
M ui/ui-animation/src/main/java/androidx/ui/animation/Transition.kt
M ui/ui-core/src/androidTest/java/androidx/ui/core/test/AndroidLayoutDrawTest.kt
M ui/ui-core/src/androidTest/java/androidx/ui/core/test/AndroidViewCompatTest.kt
M ui/ui-core/src/androidTest/java/androidx/ui/core/test/ClipTest.kt
M ui/ui-core/src/androidTest/java/androidx/ui/core/test/DrawShadowTest.kt
M ui/ui-core/src/androidTest/java/androidx/ui/core/test/ModelReadsTest.kt
M ui/ui-core/src/androidTest/java/androidx/ui/core/test/OpacityTest.kt
D ui/ui-core/src/androidTest/java/androidx/ui/core/test/ValueModel.kt
M ui/ui-core/src/androidTest/java/androidx/ui/core/test/WithConstraintsTest.kt
M ui/ui-core/src/androidTest/java/androidx/ui/res/ResourcesTest.kt
M ui/ui-core/src/androidTest/java/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/main/java/androidx/ui/core/ModelObserver.kt
M ui/ui-core/src/main/java/androidx/ui/res/Resources.kt
M ui/ui-core/src/test/java/androidx/ui/core/selection/SelectionManagerDragTest.kt
M ui/ui-core/src/test/java/androidx/ui/core/selection/SelectionManagerLongPressDragTest.kt
M ui/ui-core/src/test/java/androidx/ui/core/selection/SelectionManagerTest.kt
M ui/ui-foundation/src/androidTest/java/androidx/ui/foundation/DeterminateProgressTest.kt
M ui/ui-layout/src/androidTest/java/androidx/ui/layout/test/ContainerTest.kt
M ui/ui-layout/src/androidTest/java/androidx/ui/layout/test/OnPositionedTest.kt
M ui/ui-material/api/0.1.0-dev12.txt
M ui/ui-material/api/current.txt
M ui/ui-material/api/public_plus_experimental_0.1.0-dev12.txt
M ui/ui-material/api/public_plus_experimental_current.txt
M ui/ui-material/api/restricted_0.1.0-dev12.txt
M ui/ui-material/api/restricted_current.txt
M ui/ui-material/integration-tests/material-demos/src/main/java/androidx/ui/material/demos/DynamicThemeActivity.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/DrawerTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ProgressIndicatorTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioGroupUiTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/Color.kt
M ui/ui-material/src/main/java/androidx/ui/material/Scaffold.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AndroidComposeTestCaseRunnerTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/MultipleComposeRootsTest.kt
M ui/ui-text/src/main/java/androidx/ui/text/CoreText.kt
M ui/ui-text/src/main/java/androidx/ui/text/CoreTextField.kt
ma...@google.com
ma...@google.com
I gave it some thinking and my current inclination is that it doesn't make sense at all to make it a foundational requirement.
It's a grey areas, but still it strikes me as a design choice more than a general concept, so I wonder if we can just add delay(80) to our ripple logic to solve the issue, but allow other indication/design system devs to decide on their own.
Louis, wdyt?
an...@google.com
This shouldn't be inside our Ripple, as it applies not only for this ripple. Plus once we migrate to the ripple imlementation based on RippleDrawable we would still need this delay. I think this should affect all indications
lp...@google.com
I agree, I think this is core touch slop behavior for Android - it doesn't matter what the indication type is, we probably just don't want to propagate the down event until we are sure it wont' be the start of a scroll event
lp...@google.com
Maybe it's reasonable to make this configurable (per platform?) but I don't think we should have any logic in indication / ripple at this level
bv...@gmail.com
ma...@google.com
We can utilize the ViewConfiguration that we have and that lives in commonMain for that. Would be great if we can somehow manage to incorporate it into the generic foundation indication then.
I don't like the idea of not propagating any events without any particular reason. It seems like the issue is with the indication, not the touch behaviour itself. The item should remain interactable and clickable even if click (down + up) fits into the 80msa or whatever timeframe we have for delaying the indication. Please, let me know if there are reasons why we would want to delay the pointer event itself.
Unless we have a strong reason, I'd like to keep this change localised in the indication api surface, not the touch surface. But I agree that generic foundation indication might be a better fit than ripples.
Folks, let me know what you think about it.
lp...@google.com
I don't like the idea of not propagating any events without any particular reason.
Sorry, my message was confusing. What I mean is propagating the PressInteraction inside clickable, etc. I think these components should wait before sending the interaction - the actual motion events should not be touched.
If I read View.java correctly, this is how it works in the framework as well:
// For views inside a scrolling container, delay the pressed feedback for
// a short period in case this is a scroll.
if (isInScrollingContainer) {
mPrivateFlags |= PFLAG_PREPRESSED;
if (mPendingCheckForTap == null) {
mPendingCheckForTap = new CheckForTap();
}
mPendingCheckForTap.x = event.getX();
mPendingCheckForTap.y = event.getY();
postDelayed(mPendingCheckForTap, ViewConfiguration.getTapTimeout());
}
and
private final class CheckForTap implements Runnable {
public float x;
public float y;
@Override
public void run() {
mPrivateFlags &= ~PFLAG_PREPRESSED;
setPressed(true, x, y);
final long delay =
ViewConfiguration.getLongPressTimeout() - ViewConfiguration.getTapTimeout();
checkForLongClick(delay, x, y, TOUCH_GESTURE_CLASSIFIED__CLASSIFICATION__LONG_PRESS);
}
}
So setPressed() is only called after the delay, when inside a scrolling container.
Do we have any current metadata for 'in a scrolling container' (presumably we do in semantics somewhere?) - can we use this in a similar way? It doesn't makes sense to delay the indication when there is no scroll action
ma...@google.com
Thanks for checking, this rouse makes sense to me.
100% agree that we need to do it only in the scroll container -- that's a separate issue on it's own and I don't think semantics will help much here, but maybe?
talking out-loud: I was wondering if there's any reason other than indication when you want to delay the press interaction other than for the indication? probably if you long the event you can also want to not to have flakes when scrolling or similar, so it's the similar problem then.
And if we have ViewConfiguration, than you can always opt out specifically from the time out by providing a different ViewConfiguration in LocalViewConfiguration ambient
lp...@google.com
And if we have ViewConfiguration, than you can always opt out specifically from the time out by providing a different ViewConfiguration in LocalViewConfiguration ambient
Oh I didn't realize we already had a common abstraction here, nice - yeah this looks really flexible to me.
talking out-loud: I was wondering if there's any reason other than indication when you want to delay the press interaction other than for the indication?
Yeah I think every case that cares (indication, changing a button to appear pressed (like elevation), sending logging metrics to a server when it is pressed, etc) should have this delay - if a scroll happens in the timeout then we cancel the click, so I think this should not emit the PressInteraction as well
bv...@gmail.com
lp...@google.com
Good point, the same is probably true for clickable list items with a swipe to dismiss action as well (and similar) - anything where the parent or this layout can be dragged / swiped.
ma...@google.com
ap...@google.com
Branch: androidx-main
commit 9066ec36bd05e42ffe8f2c37e0c74da2d0d5ba08
Author: Louis Pullen-Freilich <lpf@google.com>
Date: Wed May 19 23:07:07 2021
Adds a tap timeout to clickable / toggleable to prevent showing a ripple during a scroll / drag
This results in the following behavior:
Press AND release within the timeout - show a ripple after release
Press, wait until timeout - show a ripple after timeout
Press, drag / scroll within the timeout - no ripple
Press, drag / scroll after the timout - ripple, then cancelled
Currently this happens for all cases, but in the future we should investigate disabling the timeout if the content is not in a scrollable container / if there is nothing that would consume move events.
Bug:
Test: ClickableTest
Relnote: "Adds a tap timeout to clickable / toggleable to prevent showing a ripple during a scroll / drag"
Change-Id: Ia27044999597dd9411344119a7b77180943d9a25
M compose/foundation/foundation/api/1.0.0-beta08.txt
M compose/foundation/foundation/api/current.txt
M compose/foundation/foundation/api/public_plus_experimental_1.0.0-beta08.txt
M compose/foundation/foundation/api/public_plus_experimental_current.txt
M compose/foundation/foundation/api/restricted_1.0.0-beta08.txt
M compose/foundation/foundation/api/restricted_current.txt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/ClickableTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/IndicationTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/SelectableTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/ToggleableTest.kt
A compose/foundation/foundation/src/androidMain/kotlin/androidx/compose/foundation/Clickable.android.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/Clickable.kt
A compose/foundation/foundation/src/desktopMain/kotlin/androidx/compose/foundation/Clickable.desktop.kt
M compose/material/material/src/androidAndroidTest/kotlin/androidx/compose/material/ButtonScreenshotTest.kt
M compose/material/material/src/androidAndroidTest/kotlin/androidx/compose/material/CardTest.kt
M compose/material/material/src/androidAndroidTest/kotlin/androidx/compose/material/SurfaceTest.kt
M compose/material/material/src/androidAndroidTest/kotlin/androidx/compose/material/SwitchScreenshotTest.kt
lp...@google.com
Back to Matvei to consider providing scrolling-container metadata so we can only enable this timeout if there could be a scroll / drag in the post 1.0 timeframe.
lp...@google.com
Filed
Description
Solution: use ViewConfiguration.getTapTimeout() to delay drawing the effect and ViewConfiguration.getScaledTouchSlop() to cancel the effect when the touches move.
A sample project and a screenrecord is attached.
Component used: Box, Text, Any (modifier)
Version used: 1.0.0-alpha02
Devices/Android versions reproduced on: Pixel 3A emulator API 29