Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve support for BasicTextField2 on Desktop #1496

Merged
merged 12 commits into from
Aug 30, 2024
Merged

Improve support for BasicTextField2 on Desktop #1496

merged 12 commits into from
Aug 30, 2024

Conversation

m-sasha
Copy link
Member

@m-sasha m-sasha commented Aug 13, 2024

  • Add support for input methods.
  • Replace deprecated PlatformTextInputService with PlatformContextTextInputService (name suggestions welcome) in public APIs.

Fixes (mostly?) https://youtrack.jetbrains.com/issue/CMP-1253/Support-BasicTextField2-for-desktop
Fixes https://youtrack.jetbrains.com/issue/CMP-5913/Input-method-cannot-switch-to-Chinses-in-BasicTextField-when-using-TextFieldState

Testing

  • WindowTypeTest now tests the new BasicTextField2, in addition to the old TextField.

This should be tested by QA

Release Notes

Features - Desktop

  • Added support for input methods (languages such as Chinese, Korean, Arabic) to BasicTextField(TextFieldState, ...).

@m-sasha m-sasha requested a review from igordmn August 13, 2024 15:34
* This is a non-deprecated version of [PlatformTextInputService], which is needed because we can't
* expose deprecated APIs in [PlatformContext].
*/
interface PlatformContextTextInputService {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should it be a public stable API?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move into a separate file

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should it be a public stable API?

As the comment above it explains, we can't expose a deprecated API in PlatformContext.

}

@Suppress("DEPRECATION")
fun PlatformContextTextInputService.asPlatformTextInputService() = object : PlatformTextInputService {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we already have PlatformTextInputService why do we need a new copy?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because PlatformTextInputService is deprecated.

* implementing input methods.
*/
interface TextFieldStateAdapter {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another stable public API

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment above it explains why it's needed.

Copy link
Member

@MatkovIvan MatkovIvan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that introducing a new public stable API is a way to go, and "because common API is deprecated" doesn't sound as a good reason. We need to work with Google to share more code in common, but it's a step into the opposite direction.

@m-sasha
Copy link
Member Author

m-sasha commented Aug 19, 2024

I don't think that introducing a new public stable API is a way to go

Please discuss this with @igordmn

@igordmn
Copy link
Collaborator

igordmn commented Aug 27, 2024

Changed:

Added support for input methods (languages such as Chinese, Korean, Arabic) to BasicTextField2.

to

Added support for input methods (languages such as Chinese, Korean, Arabic) to BasicTextField(TextFieldState, ...).

Added:

Fixes https://youtrack.jetbrains.com/issue/CMP-5913/Input-method-cannot-switch-to-Chinses-in-BasicTextField-when-using-TextFieldState

Copy link
Collaborator

@igordmn igordmn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation uses immutable text state, which isn't for what BasicTextField(TextFieldState is designed for. It will be slow for long text fields.

We need to design a new API, that can mutate TextFieldState directly, not via old PlatformTextInputService, which doesn't support that.

* This is a non-deprecated version of [PlatformTextInputService], which is needed because we can't
* expose deprecated APIs in [PlatformContext].
*/
interface PlatformContextTextInputService {
Copy link
Collaborator

@igordmn igordmn Aug 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that introducing a new public stable API is a way to go

Please discuss this with @igordmn

Yes, it was suggested by me. We discussed it with Ivan, and I also was convinced that it is better to not create a duplicate. I remember you also was in favour of reusing TextInputService.

The current suggested way to provide a public non-deprecated API between ui <- foundation in skikoMain was dictated by our old approach, when we lived separately from Android. Android currently doesn't provide API ui <- foundation for text input and just uses Android SDK <- foundation (more context why we want to avoid this is here)

But it is better to unify approach on Android as well and move it to commonMain. An abstraction between ui and foundation already exists - it is TextInputService.

It will be a more difficult path, but we could it this way:

1. Remove a copy of PlatformTextInputService
2. Undeprecate PlatformTextInputService in the fork
3. [somewhere in the future during upstreaming]
4. Undeprecate it in AOSP as well
5. Replace actual's from foundation/androidMain by PlatformTextInputService calls.
6. If PlatformTextInputService won't be enough to replace these actual's, extend it (or replace it by something like PlatformTextInputService2)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to undeprecate it here and realised, that its deprecation was justified. The old PlatformTextInputService manipulates immutable text field states and copies the whole text field instead of mutating it. The new BasicTextField(TextFieldState) on the other hand is designed that it just changes some portion in some data structure (probably a gap buffer).

So, to implement it properly, we have to design the new API that doesn't use immutable states.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed removing new public API. @MatkovIvan, please check.

We can merge it as is without optimisations in 1.7, and then do them in 1.8 (if needed)

import kotlinx.coroutines.flow.MutableSharedFlow

// TODO(https://youtrack.jetbrains.com/issue/COMPOSE-733/Merge-1.6.-Apply-changes-for-the-new-text-input) implement
internal actual suspend fun PlatformTextInputSession.platformSpecificTextInputSession(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked Chinese on this example, it works:

        val text = remember {
            TextFieldState("Hello \uD83E\uDDD1\uD83C\uDFFF\u200D\uD83E\uDDB0")
        }

        BasicTextField(
            state = text,
            modifier = Modifier.onPreviewKeyEvent {
                when {
                    (it.isShiftPressed && it.key == Key.Enter) -> {
                        text.edit {
                            append("Compose")
                        }
                        true
                    }
                    else -> false
                }
            }
        )

@@ -41,7 +41,7 @@ class DesktopInputComponentTest {
val processor = EditProcessor()

val input = DesktopTextInputService(PlatformComponent.Empty)
val inputService = TextInputService(input)
val inputService = TextInputService(input.asPlatformTextInputService())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't take my other comment into account (about undeprecation TextInputService), and if we would go a designing a new API way, then we would need to completely remove the link between ui and foundation via the deprecated TextInputService.

In the current code all foundation implementation still uses the deprecated TextInputService, but it should use the new API PlatformContextTextInputService. This is the point of providing a stable new API. If we need it just for ComposeScene, then there is no need in public, we can use @InternalComposeUiApi.

@igordmn igordmn force-pushed the m-sasha/btf2 branch 4 times, most recently from dbe9fa2 to 6f8e9a8 Compare August 29, 2024 23:50
Copy link
Member

@MatkovIvan MatkovIvan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing deprecation for 1.7 LGTM

…ha/btf2

# Conflicts:
#	compose/ui/ui/api/desktop/ui.api
#	compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/node/RootNodeOwner.skiko.kt
@igordmn igordmn merged commit 588452e into jb-main Aug 30, 2024
5 of 6 checks passed
@igordmn igordmn deleted the m-sasha/btf2 branch August 30, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants