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

TF2 floating cursor #1598

Merged
merged 6 commits into from
Feb 17, 2025

Conversation

alexzhirkevich
Copy link

@alexzhirkevich alexzhirkevich commented Sep 26, 2024

This should be enough for iOS TF2 floating cursor to work.

Release Notes

iOS - Features

  • Floating cursor support for BasicTextField(TextFieldState)

@ASalavei
Copy link
Collaborator

@alexzhirkevich , is the MR still in a draft state, or we should review it?
Current'y, we're experimenting with adoption of the UITextInput to make direct connection between iOS and Compose input, that will solve lots of issues, including floating cursor: https://youtrack.jetbrains.com/issue/CMP-7103

@alexzhirkevich
Copy link
Author

alexzhirkevich commented Feb 10, 2025

Hi. You can review this, it is quite small and just enables cursor from the old text field
You can also take a look at new haptics i added yesterday - #1831. There are nice new feedback types that i had to implement manually before

we're experimenting with adoption of the UITextInput

Sounds awesome, is there an active branch?

@alexzhirkevich alexzhirkevich marked this pull request as ready for review February 10, 2025 13:03
@mazunin-v-jb
Copy link

mazunin-v-jb commented Feb 12, 2025

LGTM. Actually, this fix is useful despite the textfield experiment: it forwards TextLayoutResult, which is required for many UITextInput methods, it would help to support native iOS text input experience in BTF2

@m-sasha, could you please also review Skiko part?

@ASalavei ASalavei requested a review from m-sasha February 12, 2025 13:10
Copy link
Member

@m-sasha m-sasha left a comment

Choose a reason for hiding this comment

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

We've been passing data from platformSpecificTextInputSession to the PlatformTextInputServices via direct calls on PlatformTextInputSession (e.g. notifyFocusedRect), but I think doing it with a flow instead is better, because it means in cases where the data isn't needed, it won't be computed.

@@ -112,6 +114,7 @@ internal actual suspend fun PlatformTextInputSession.platformSpecificTextInputSe
onEditCommand = ::onEditCommand,
onImeAction = onImeAction,
editProcessor = editProcessor,
textLayoutResult = snapshotFlow(layoutState::layoutResult).filterNotNull(),
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure we don't want to forward nulls to the text input service? I'm not entirely sure when the layout can be null, but isn't it possible the text input service will remain stuck with a stale non-null value if we're filtering nulls?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not completely sure, but notifyFocusedRect you mentioned above does also ignore null rects. And PlatformTextInputService for TF1 is not notified about null layouts too

@m-sasha
Copy link
Member

m-sasha commented Feb 12, 2025

Let's run the CI, and I think we can merge if it passes.

@m-sasha m-sasha self-requested a review February 13, 2025 14:25
@@ -34,4 +36,6 @@ actual interface PlatformTextInputMethodRequest {
val onImeAction: ((ImeAction) -> Unit)?
@ExperimentalComposeUiApi
val editProcessor: EditProcessor?
@ExperimentalComposeUiApi
val textLayoutResult: Flow<TextLayoutResult>
Copy link
Collaborator

@ASalavei ASalavei Feb 13, 2025

Choose a reason for hiding this comment

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

With this change you need to update API dump. Please run ./gradlew desktopApiDump and commit changes.

Copy link
Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks!
@m-sasha , could you please re-approve API updates?

@ASalavei ASalavei requested a review from m-sasha February 14, 2025 20:45
@mazunin-v-jb mazunin-v-jb merged commit 7f58455 into JetBrains:jb-main Feb 17, 2025
2 checks passed
@alexzhirkevich alexzhirkevich deleted the btf2-floating-cursor branch February 21, 2025 09:28
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.

4 participants