Consider yielding after advancing the ManualFrameClock [170708374]

Fixed

Bug

Status Update

No update yet.

Description

je...@google.com

created issue #1

Oct 13, 2020 10:23AM

If ManualFrameClock.advanceClock(nanos: Long) is called from the main dispatcher, and its awaiters are also dispatched on the main dispatcher, two subsequent calls to advanceClock without a suspending call in between will result in the second frame time being missed by the awaiting coroutines.

When awaiters get their frame time, their withFrameNanos lambda runs and they will be resumed, but the resume happens dispatched. If the caller is already occupying the main thread, the dispatch is enqueued and can only happen when the caller suspends.

If we yield right after sending the new frame time, the awaiters can be dispatched and potentially suspend on another call to withFrameNanos before the caller of advanceClock moves on.

Comments

tc...@google.com <tc...@google.com> #2Jan 7, 2021 11:11PM

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> #3Jan 11, 2021 03:54PM

Marked as fixed.

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

commit 3ac4b80f723df87ed65e475badcba309e2438562
Author: Jelle Fresen <jellefresen@google.com>
Date: Wed Jan 06 11:22:38 2021

Move frame clock test methods to test-utils

Remove the experimental advance methods for ManualFrameClock to internal
test utils, and clean up a few usages of clock control that were
incorrect.

Also yields after advancing now, which is desirable in all cases.

Fixes: 170708374
Test: ./gradlew compose:foundation:foundation:cC && \
./gradlew compose:material:material:cC && \
./gradlew compose:ui:ui-test:cC
Relnote: "Removed experimental monotonicFrameAnimationClockOf methods"

Change-Id: Ib753f5d3cb77dabc7727f677a73bb7da8dae5fe2

M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/ScrollTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/ScrollableTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/lazy/LazyColumnTest.kt
M compose/material/material/src/androidAndroidTest/kotlin/androidx/compose/material/SwipeableTest.kt
A compose/test-utils/src/commonMain/kotlin/androidx/compose/testutils/AnimationClocks.kt
A compose/test-utils/src/commonMain/kotlin/androidx/compose/testutils/MockAnimationClock.kt
M compose/ui/ui-test/api/current.txt
M compose/ui/ui-test/api/public_plus_experimental_current.txt
M compose/ui/ui-test/api/restricted_current.txt
M compose/ui/ui-test/src/androidAndroidTest/kotlin/androidx/compose/ui/test/gesturescope/SendSwipeTest.kt
M compose/ui/ui-test/src/commonMain/kotlin/androidx/compose/ui/test/TestAnimationClock.kt

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

Issue 170708374

Description

Issue summary

Comments

tc...@google.com <tc...@google.com> #2Jan 7, 2021 11:11PM

ap...@google.com <ap...@google.com> #3Jan 11, 2021 03:54PM

Add comment

Issue metadata