Make AutofillManager as non-expect interface [376080755]

Fixed

Bug

Status Update

No update yet.

Description

ig...@jetbrains.com

created issue #1

Oct 29, 2024 03:52PM

- Jetpack Compose version: androidx-main, 0be31ce
- Jetpack Compose component used: AutofillManager

AutofillManager was transformed from common interface to an expect class in

https://android-review.googlesource.com/c/platform/frameworks/support/+/3313535/3/compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/autofill/AutofillManager.kt#25

Can it be reverted back to non-expect interface before the 1.8 API freeze? The reasoning is described in the linked comment.

It is not critical, as this seems as compatible change, but it can complicate adoption for multiplatform.

Comments

mn...@google.com <mn...@google.com> Oct 29, 2024 04:24PM

Reassigned to mn...@google.com.

se...@google.com <se...@google.com> #2Oct 29, 2024 10:54PM

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

ig...@jetbrains.com <ig...@jetbrains.com> #3Oct 30, 2024 01:10PM

Thanks for the quick responses!

This is because of the CMP policy of not making expect/actual unless it is really needed. We talked about it in the workshop presentation.

The reasons for this policy are deep, and we came to it eventually by trial and error, taking into account the feedback of our users and our product requirements.

We have 2 product requirements:

Maximum of the code should be crossplatform (write once - run everywhere). If some public API is different between platforms (i.e. we have additional methods for some platform), it will force all using code also be platform specific and defined in platform sourcesets. Instead of thinking which platform are supported for this specific feature, we want users to think which capabilities are supported:

if (autofillManager.isRequestAutofillSupported) { // optional, as it is okay if requestAutofill won't do anything
    autofillManager.requestAutofill()
}

This also helps adding new platforms by us - all possible users won't be forced to implement the new added platform for the API they use.

Clients can adopt Compose for a new platform without re-compilation of Compose Multiplatform by using the platform independent core ComposeScene and injecting platform implementation via PlatformContext in runtime. Having platform implementation defined via expect complicates fulfilment of this requirement. We also use this approach for virtual testing on the desktop platform - we can run it on a real desktop, or run it in the headless state, rendering it into an image. In this case no real windows, or even AWT API is used. It is a pure third-party code.

We think that only a few cases are good for creating expect/actual:

to communicate with the platform API. Usually we do that only in the implementation of the platform
to optimize some code, because we know how it compiles and how it runs on a specific platform
to define different defaults for different platforms
to provide API for interop with the platform. Things like LocalContext, LocalView, event.nativeKeyEvent, etc. Ideally they shouldn't be used by Compose core itself, only by external users.

Cases that aren't good for expect/actual:

to provide a public API
to have different implementations on higher levels (material), where we accessing god objects like LocalContext, LocalView and bypassing designing public API's on lower levels (ui)

We made this change intentionally to allow platforms to extend Autofill as needed to support their bespoke behaviors.

As I mentioned above, this will make all clients of Autofill to be platform-specific. According the policy, platform-specifics API's are justified only for the interop case (when we need to access a real platform type). But even in this case it is discouraged to access it in Compose itself, as it is indicator that we miss some public low-level common API. We experienced it with Dialog/Popup API, Drag&Drop API, and the new text input session API - they accessed Context/View directly, prohibiting decomposition of a high-level common feature to low-level features that are still common.

We currently don't extend any expect interface created in commonMain for iOS/Desktop/Web, as it is not needed for us. Instead we try to communicate this approach with you in all future Compose changes, and encourage to not make public expect API.

Message last modified on Oct 30, 2024 01:14PM

se...@google.com <se...@google.com> #4Oct 30, 2024 05:21PM

Thanks! We're taking a look

ap...@google.com <ap...@google.com> #5Nov 1, 2024 10:38PM

Marked as fixed.

Project: platform/frameworks/support
Branch: androidx-main
Author: Melba Nuzen <mnuzen@google.com>
Link: https://android-review.googlesource.com/3323771

Refactor Autofill manager from expect/actual to interface

Expand for full commit details

Refactor Autofill manager from expect/actual to interface 
 
This CL also lifts the `requestAutofillForActiveElement` into common. Since the text toolbar changes in aosp/3312341 were added when `requestAutofill` was an Android-only API, this CL also removes all the logic added in that CL that was needed when the API was not in common. 
 
See comments in aosp/3313535 for more notes. 
 
Test: ensure existing tests pass 
Fixes: 376080755 
RelNote: "Changes Autofill manager to be an interface." 
Change-Id: I8491407de5699bf7273b9f88bda11cc438e2fd62

Files:

M compose/foundation/foundation/src/androidMain/kotlin/androidx/compose/foundation/text/input/internal/selection/TextFieldSelectionState.android.kt
M compose/foundation/foundation/src/androidMain/kotlin/androidx/compose/foundation/text/selection/TextFieldSelectionManager.android.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/text/input/internal/selection/TextFieldSelectionState.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/text/selection/TextFieldSelectionManager.kt
D compose/foundation/foundation/src/commonStubsMain/kotlin/androidx/compose/foundation/text/input/internal/selection/TextFieldSelectionState.commonStubs.kt
M compose/foundation/foundation/src/commonStubsMain/kotlin/androidx/compose/foundation/text/selection/TextFieldSelectionManager.commonStubs.kt
M compose/ui/ui/api/current.txt
M compose/ui/ui/api/restricted_current.txt
M compose/ui/ui/src/androidInstrumentedTest/kotlin/androidx/compose/ui/autofill/AndroidAutofillManagerTest.kt
M compose/ui/ui/src/androidMain/kotlin/androidx/compose/ui/autofill/AndroidAutofillManager.android.kt
M compose/ui/ui/src/androidMain/kotlin/androidx/compose/ui/platform/AndroidComposeView.android.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/autofill/AutofillManager.kt
D compose/ui/ui/src/commonStubsMain/kotlin/androidx/compose/ui/autofill/AutofillManager.commonStubs.kt

Hash: 88e844c277eaf5cd576e4a4075e63632600c78f8
Date: Tue Oct 29 11:28:54 2024

na...@google.com <na...@google.com> #6Nov 13, 2024 06:19PM

The following release(s) address this bug.It is possible this bug has only been partially addressed:

androidx.compose.foundation:foundation:1.8.0-alpha06
androidx.compose.foundation:foundation-android:1.8.0-alpha06
androidx.compose.foundation:foundation-jvmstubs:1.8.0-alpha06
androidx.compose.foundation:foundation-linuxx64stubs:1.8.0-alpha06
androidx.compose.ui:ui:1.8.0-alpha06
androidx.compose.ui:ui-android:1.8.0-alpha06
androidx.compose.ui:ui-jvmstubs:1.8.0-alpha06
androidx.compose.ui:ui-linuxx64stubs:1.8.0-alpha06

Issue 376080755