Status Update
No update yet.
Comments
du...@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
lo...@gmail.com
Thanks for the information.
Taking in into account, I think it'd be best to have filter, map, flatMap and their Sync suffixed variants run on Dispatchers.Default by default, since it's very very likely that we get CPU bound operations there, such as searching for filtering, allocating to create instances of other types from the input data…
Regarding the name of the Java API… I agree a little that "*Sync is a bad name", and I think Blocking would be more accurate on the Kotlin side (while keeping the non suffixed @JvmName for Java callers). That is related to the issue where I ask to discourage or prevent use of the API for Java callers in Kotlin code:
Maybe we could sketch out what the API should be in a shared doc?
Yes we can! You can send it here, or we can also chat on ASG or
Since it's not what I expected (not run on Dispatchers.Default, I think it's best to talk more about the design before considering a PR.
du...@google.com
Thanks - I sent you a message and started a doc, these are definitely some valid concerns!
ap...@google.com
Branch: androidx-main
commit 348fa7e78cacc892e2c2cd59c5aaadd34a640c2f
Author: Dustin Lam <dustinlam@google.com>
Date: Wed Dec 09 18:24:18 2020
Add Executor param to PagingData operators for Java
Instead of forcing Java users down the sync path without much control
for execution context, add an Executor argument and remove the -Sync
suffix of the method name.
In cases for Guava, while it's possible to build the executor
into the Future itself, it's still desirable to be able to
control which execution context it receives the result on.
Relnote: "Allow Java callers to use PagingData transform operations in
an async way, by accepting an Executor into transform operator
arguments. All of the -Sync transform operators have the -Sync suffix
removed now, and Kotlin Coroutine users will need to disambiguate by
calling the extension function which accepts a suspending block instead.
All PagingData transformation operators have been moved to extensions
under the static PagingDataTransforms class. Java users will need to
call them via static helpers e.g.,
`PagingDataTransforms.map(pagingData, transform)`
For Kotlin users, the syntax is the same but you'll need to import the
function."
Fixes: 172895919
Test: ./gradlew paging:paging-common:test
Change-Id: If688562d07d96e943b1abfa3690042132db1c4d0
M paging/common/api/current.txt
M paging/common/api/public_plus_experimental_current.txt
M paging/common/api/restricted_current.txt
M paging/common/src/main/kotlin/androidx/paging/PagingData.kt
A paging/common/src/main/kotlin/androidx/paging/PagingDataTransforms.kt
A paging/common/src/main/kotlin/androidx/paging/UiReceiver.kt
M paging/integration-tests/testapp/src/main/java/androidx/paging/integration/testapp/v3/V3ViewModel.kt
M paging/integration-tests/testapp/src/main/java/androidx/paging/integration/testapp/v3room/V3RoomViewModel.kt
M paging/samples/src/main/java/androidx/paging/samples/java/InsertSeparatorsJavaSample.java
M paging/samples/src/main/java/androidx/paging/samples/java/InsertSeparatorsJavaUiModelSample.java
M samples/SupportLeanbackDemos/src/main/java/com/example/android/leanback/PhotoViewModel.java
lo...@gmail.com
Do the suspend versions now run on Dispatchers.Default?
du...@google.com
The awaiting context is normally on main thread iirc, but since they're suspending I don't think it should really matter? I would probably argue it's better to avoid extra potential thread hops.
lo...@gmail.com
map and filter are potentially CPU intensive operations (allocation, search, etc), and Kotlin users will likely not wrap their call with dispatcher switching by default, leading to potential UI jank if the default is the main thread. That's why I think Dispatchers.Default should be the default.
du...@google.com
While I agree, part of the issue is that wrapping withContext(Disaptcher.Default) { } will also make it difficult to fast-forward / control timing when used with test scope / pauseDispatcher from kotlinx-coroutines-test. There isn't a good way for Paging to forward the fetchDispatcher to the transforms to provide the default, since the only Paging3 APIs that actually accept a bg / fetchDispatcher are in the paging-runtime adapter / differ APIs.
lo...@gmail.com
I think having a jank-free app by default is mort important than timing control in testing API, as users will ultimately only experience the former.
Now, I think there's a solution to satisfy both use cases:
Have a context: CoroutineContext parameter that defaults to Dispatchers.Default, which allows people to replace it with EmptyCoroutineContext, or whatever they want (including Dispatchers.Main if they need to sync something on the main thread).
du...@google.com
I think having a jank-free app by default is mort important than timing control in testing API, as users will ultimately only experience the former.
I agree, except in this case we would make it impossible for it to be testable, so the trade-off is not great here.
Now, I think there's a solution to satisfy both use cases: Have a context: CoroutineContext parameter that defaults to Dispatchers.Default, which allows people to replace it with EmptyCoroutineContext, or whatever they want (including Dispatchers.Main if they need to sync something on the main thread).
Adding a param seems not great either. Also what happens if developer wants to use .flowOn {} etc to change the executing context? I think generally Flow operators are not supposed to accept a CoroutineContext, but I could be wrong here. Certainly something to think about though.. I think the real solution here is to try to default .submitData to collect on fetchDispatcher, then report differ events through mainDispatcher, but I haven't tried yet so am not sure how feasible it is.
lo...@gmail.com
If EmptyCoroutineContext is used, flowOn will work as expected.
I agree it'd be best to have the collector collect in the right dispatcher. I'm curious if there's any challenges that poses.
du...@google.com
Right, I meant that it is surprising to need to pass in EmptyCoroutineContext to get .flowOn to work.
Description
Hello,
In the
PagingDataclass, there'sfilterSyncand other functions that accept a blocking lambda, but there's no information in the KDoc about which dispatcher/thread they are called on.I guess they are called on
Dispatchers.Default… is it the case?If so, I'd submit a PR on GitHub for this change on
filterSyncand friends.