TextField/FilledTextField should have String overloads instead of TextFieldValue overloads [155211005]

Fixed

Feature Request

Status Update

No update yet.

Description

le...@google.com

created issue #1

Apr 28, 2020 07:38PM

Currently we have TextField variants with the following `value`, `onValueChange` parameters:

CoreTextField(EditorValue, (EditorValue) -> Unit) (in ui.text)
TextField(EditorValue, (EditorValue) -> Unit) (in ui.foundation)
TextField(TextFieldValue, (TextFieldValue) -> Unit) (in ui.foundation)
FilledTextField(TextFieldValue, (TextFieldValue) -> Unit) (in ui.material)
FilledTextField(String, (String) -> Unit) (in ui.material)

EditorValue is the most powerful API, where every aspect of the text field is controllable, but also more cumbersome to use.
TextFieldValue is middle-weight, with only string value and selection controllable, but still a bit cumbersome to use
String is the least powerful API, but also the most convenient and powerful enough to satisfy the overwhelming majority of use cases.

My suggestion is to refactor these overloads to have each one fit into one of two overloads:
1. EditorValue-based (cumbersome, powerful)
2. String-based (convenient, useful)

The string-based variants would have an `onSelectionChanged: (Selection) -> Unit` parameter, but would not allow for a selection to be *controlled* (hence the past tense on the parameter name).

It seems to me like the need to control or listen to the selection state are both somewhat rare, but the need to control it is even more so. On the other hand, the need to control the text value is virtually 100% of the time. As a result, adding the overhead of the selection controllability in the common case feels wrong, since the times you might want to do that, the more powerful EditorValue API might be something you need to reach for anyway.

As a result, my proposal is to have the following APIs:

in ui.text:
fun CoreTextField(value: EditorValue, onValueChange: (EditorValue) -> Unit) (unchanged)

in ui.foundation:
fun TextField(EditorValue, (EditorValue) -> Unit) (unchanged)
fun TextField(value: String, onValueChange: (String) -> Unit, onSelectionChanged: (Selection) -> Unit)

in ui.material:
fun FilledTextField(EditorValue, (EditorValue) -> Unit)
fun FilledTextField(value: String, onValueChange: (String) -> Unit, onSelectionChanged: (Selection) -> Unit)

Comments

si...@google.com <si...@google.com> #2May 11, 2020 05:22PM

Android 5.0, 5.1: it shows context menu at top of screen, not as popup. See screenshots.

Android framework doesn't support floating toolbar before 23. So this is intended behavior.

The crash information suggests that this is an error from Snapshot. chuckj@ can you please take a look? I don't know what exactly caused the readError in Snapshot, any suggestions? Thanks a lot!

Message last modified on May 25, 2020 07:32PM

si...@google.com <si...@google.com> May 26, 2020 01:23AM

Reassigned to si...@google.com.

si...@google.com <si...@google.com> Jun 5, 2020 12:19AM

Accepted by si...@google.com.

si...@google.com <si...@google.com> #3Jun 9, 2020 04:44AM

This looks like another instance of 193006595 which I am looking at now.

Message last modified on Jun 9, 2020 04:45AM

si...@google.com <si...@google.com> #4Jun 10, 2020 05:53AM

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

commit beeb84e7be604088ad40e080a8d0adb1bacbf695
Author: Chuck Jazdzewski <chuckj@google.com>
Date: Thu Jul 08 10:04:28 2021

Remove updating the transparent snapshot

A transparent snapshot is create for the block exeucted by
`Snapshot.observe()` which registers read and write observers
but without creating a snapshot. The code to advance the
global snapshot, however, created a new transparent snapshot in
an attempt to update it to the new global snapshot it just
created. This is not necessary, however, as the transparent
snapshot will retrieve the current global snapshot from
`currentGlobalSnapshot` instead of `previousSnapshot` if
`previousSnapshot` is `null`. With the previous code, if the
global snapshot is advanced by applying a new snapshot the
"updated" transparent snapshot will be left referring to an
older snapshot instead of the most recent global snapshot.
Any snapshot object created in the newly applied snapshot
will throw an exception when it is accessed, as reported in
the bugs. The solution is to remove the apparent vetiagal code.

Test: ./gradlew :compose:r:r:tDUT
Fixes:

b/193006595

b/192570897

Change-Id: I2c0d0b8f57bf70e5a98ea36ed141d97142a5e53e

M compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/snapshots/Snapshot.kt
M compose/runtime/runtime/src/test/kotlin/androidx/compose/runtime/snapshots/SnapshotTests.kt

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

si...@google.com <si...@google.com> #5Jun 10, 2020 06:28AM

I first created changes that I took a two step approach:

change ui-text and ui-text-core, while using deprecated composable in ui-foundation.
change ui-foundation and ui-material to start using new object

Because of the problem mentioned in the previous comment, I abandoned those attempts:

Created one CL that contains all the change

https://android-review.googlesource.com/c/platform/frameworks/support/+/1328513

si...@google.com <si...@google.com> #6Jun 10, 2020 09:48PM

Marked as fixed.

CL is ready to go, marking as fixed.

ap...@google.com <ap...@google.com> #7Jun 10, 2020 10:54PM

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

commit c2e5559d05db4329d7e0d3e92e53508194bf5d8d
Author: Siyamed Sinir <siyamed@google.com>
Date: Tue Jun 09 09:37:23 2020

Use TextFieldValue in all TextFields

Test: ./gradlew ui:ui-material:test
Test: ./gradlew ui:ui-material:cAT
Test: ./gradlew ui:ui-foundation:test
Test: ./gradlew ui:ui-foundation:cAT
Test: ./gradlew ui:ui-text:test
Test: ./gradlew ui:ui-text:cAT
Test: ./gradlew ui:ui-text-core:test
Test: ./gradlew ui:ui-text-core:cAT
Test: ./gradlew ui:ui-core:test
Test: ./gradlew ui:ui-core:cAT
Test: Manual test in the Demo app.

RelNote: "androidx.ui.foundation.TextFieldValue and
androidx.ui.input.EditorValue is deprecated. TextField,
FilledTextField and CoreTextField composables that uses
that type is also deprecated. Please use
androidx.ui.input.TextFieldValue instead"

Bug: 155211005
Change-Id: I4066d1f4d2e3e3514753aa3495680292dc55f89d

M compose/compose-runtime/samples/src/main/java/androidx/compose/samples/ModelSamples.kt
M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/input/EditProcessorBenchmark.kt
M ui/integration-tests/demos/src/main/java/androidx/ui/demos/DemoApp.kt
M ui/integration-tests/demos/src/main/java/androidx/ui/demos/DemoFilter.kt
M ui/ui-core/integration-tests/ui-core-demos/src/main/java/androidx/ui/core/demos/PopupDemo.kt
M ui/ui-core/integration-tests/ui-core-demos/src/main/java/androidx/ui/core/demos/autofill/ExplicitAutofillTypesDemo.kt
M ui/ui-core/src/androidTest/java/androidx/ui/input/RecordingInputConnectionTest.kt
M ui/ui-core/src/main/java/androidx/ui/input/InputState.kt
M ui/ui-core/src/main/java/androidx/ui/input/RecordingInputConnection.kt
M ui/ui-core/src/main/java/androidx/ui/input/TextInputServiceAndroid.kt
M ui/ui-core/src/test/java/androidx/ui/input/RecordingInputConnectionUpdateTextFieldValueTest.kt
M ui/ui-core/src/test/java/androidx/ui/input/TextInputServiceAndroidTest.kt
M ui/ui-desktop/src/jvmMain/kotlin/androidx/ui/desktop/DesktopPlatformInput.kt
M ui/ui-foundation/api/0.1.0-dev14.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev14.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev14.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/samples/src/main/java/androidx/ui/foundation/samples/TextFieldSample.kt
M ui/ui-foundation/src/androidTest/java/androidx/ui/foundation/SoftwareKeyboardTest.kt
M ui/ui-foundation/src/androidTest/java/androidx/ui/foundation/TextFieldFocusTest.kt
M ui/ui-foundation/src/androidTest/java/androidx/ui/foundation/TextFieldOnValueChangeTextFieldValueTest.kt
M ui/ui-foundation/src/androidTest/java/androidx/ui/foundation/TextFieldTest.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/TextField.kt
M ui/ui-layout/src/androidTest/java/androidx/ui/layout/test/TextLayoutDirectionModifierTest.kt
M ui/ui-material/api/0.1.0-dev14.txt
M ui/ui-material/api/current.txt
M ui/ui-material/api/public_plus_experimental_0.1.0-dev14.txt
M ui/ui-material/api/public_plus_experimental_current.txt
M ui/ui-material/api/restricted_0.1.0-dev14.txt
M ui/ui-material/api/restricted_current.txt
M ui/ui-material/samples/src/main/java/androidx/ui/material/samples/TextFieldSamples.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/FilledTextFieldTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextField.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/TextInputServiceForTests.kt
M ui/ui-text-core/api/0.1.0-dev14.txt
M ui/ui-text-core/api/current.txt
M ui/ui-text-core/api/public_plus_experimental_0.1.0-dev14.txt
M ui/ui-text-core/api/public_plus_experimental_current.txt
M ui/ui-text-core/api/restricted_0.1.0-dev14.txt
M ui/ui-text-core/api/restricted_current.txt
M ui/ui-text-core/build.gradle
M ui/ui-text-core/src/commonMain/kotlin/androidx/ui/input/EditProcessor.kt
D ui/ui-text-core/src/commonMain/kotlin/androidx/ui/input/EditorValue.kt
A ui/ui-text-core/src/commonMain/kotlin/androidx/ui/input/TextFieldValue.kt
M ui/ui-text-core/src/commonMain/kotlin/androidx/ui/input/TextInputService.kt
M ui/ui-text-core/src/test/java/androidx/ui/input/EditProcessorTest.kt
M ui/ui-text-core/src/test/java/androidx/ui/text/TextInputServiceTest.kt
M ui/ui-text/api/0.1.0-dev14.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev14.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev14.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/integration-tests/ui-text-compose-demos/src/main/java/androidx/ui/text/demos/ComposeInputField.kt
M ui/ui-text/integration-tests/ui-text-compose-demos/src/main/java/androidx/ui/text/demos/ComposeInputFieldFocusTransition.kt
M ui/ui-text/integration-tests/ui-text-compose-demos/src/main/java/androidx/ui/text/demos/ComposeInputFieldTrickyUseCase.kt
M ui/ui-text/integration-tests/ui-text-compose-demos/src/main/java/androidx/ui/text/demos/ComposeVariousInputField.kt
M ui/ui-text/integration-tests/ui-text-compose-demos/src/main/java/androidx/ui/text/demos/TailFollowingTextField.kt
M ui/ui-text/integration-tests/ui-text-compose-demos/src/main/java/androidx/ui/text/demos/TextFieldWIthScroller.kt
M ui/ui-text/src/androidTest/java/androidx/ui/text/TextFieldDelegateIntegrationTest.kt
M ui/ui-text/src/main/java/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/main/java/androidx/ui/text/TextFieldDelegate.kt
M ui/ui-text/src/test/java/androidx/ui/text/TextFieldDelegateTest.kt

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

si...@google.com <si...@google.com> #8Jun 15, 2020 11:28PM

Status: Assigned (reopened)

FYI: this caused far too many work. I regret pushing the change in.

si...@google.com <si...@google.com> Jun 16, 2020 01:47AM