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

Scheduler breaks keyboard behavior for Compose UI consumers #63

Open
DSteve595 opened this issue Nov 13, 2021 · 17 comments
Open

Scheduler breaks keyboard behavior for Compose UI consumers #63

DSteve595 opened this issue Nov 13, 2021 · 17 comments

Comments

@DSteve595
Copy link

Compose UI's CoreTextField is very picky about getting immediate updates to its values. The common (but useless) example seen in the docs, where the text state is held in a by remember { mutableStateOf("") }, has no problems updating the text field when typing on the keyboard quickly. However, if you provide the text value from something that doesn't provide updates synchronously from onValueChange, the text field starts behaving erratically; characters randomly disappear, the cursor moves around in its own, and other weird stuff.

This issue made us realize that mapping UI state on a background thread will never work. So we've been running our presenter composition on the main thread, using an immediate monotonic frame clock:

private object ImmediateMonotonicFrameClock : MonotonicFrameClock {
  override suspend fun <R> withFrameNanos(onFrame: (frameTimeNanos: Long) -> R): R {
    return onFrame(System.nanoTime())
  }
}

This completely solved the keyboard glitchiness.

We're now looking to migrate to Molecule. But in doing so, we've seen the keyboard glitchiness return!
I somewhat narrowed it down to behaviors that depend on the combination of the Molecule's Dispatcher and MonotonicFrameClock:

Dispatcher MonotonicFrameClock Behavior
AndroidUiDispatcher Choreographer Bad
AndroidUiDispatcher ImmediateMonotonicFrameClock Less bad (but still bad)
Dispatchers.Main Choreographer Bad
Dispatchers.Main ImmediateMonotonicFrameClock Good

More specifics on the glitchiness:

Try spamming a character quickly. Occasionally, some will be dropped. When that happens, this appears in logcat:

getSurroundingText on inactive InputConnection
beginBatchEdit on inactive InputConnection
getTextBeforeCursor on inactive InputConnection
getTextAfterCursor on inactive InputConnection
getSelectedText on inactive InputConnection
endBatchEdit on inactive InputConnection

An easy way to check if the issue is happening is to hold the backspace button when there's some text present. With the issue, the backspace will randomly "stop working"; characters will stop being deleted even though you're still pressing backspace.

Honestly, I'm not positive as to whether this is Molecule's or CoreTextField's fault. Input appreciated.

Repro project with those dispatcher/frameclock modes

@JakeWharton
Copy link
Collaborator

Are you using the AndroidUiDispatcher built in to this library or sharing the one used by Compose UI?

@DSteve595
Copy link
Author

The one in this library.

@JakeWharton
Copy link
Collaborator

I'll have to write a test or sample and play around

@DSteve595
Copy link
Author

I attached a sample project if it helps.

@firefinchdev
Copy link

I'll have to write a test or sample and play around

@JakeWharton, you found something? (Just asking, I wanted to contribute, but don't have enough knowledge of internal working).

@JakeWharton
Copy link
Collaborator

I have no had a chance to look, no. I can try to look when I'm back working on this library as part of the Kotlin 1.6.20 upgrade to Compose.

@acristescu
Copy link

acristescu commented Jun 28, 2022

Depending on the Compose UI version, this can actually be quite nasty. On 1.2.0-rc02 textfields are unusable (the textfield fires two onValueChanged one after the other, the second one being with an empty string). For those running into this, downgrading to 1.1.1 with Steve's tricks above makes it usable... as long as the user doesn't type too fast :)

@jingibus
Copy link
Contributor

jingibus commented Jul 28, 2022

Recent updates provide both good-ish and (probably?) bad news on this issue:

  1. With the introduction of RecompositionClock as a param, there's now support within Molecule for the immediate clock used above. Hooray-ish! (Your life is exactly the same as it was 😅 just with better support inside Molecule)
  2. We have used an immediate clock exactly like the one implemented by OP here internally for ages now (for testing). Things work, but every now and then we run into something.... weird. Double emissions, or a scenario where we don't get an emitted value. I discussed this approach with Adam Powell after merging the RecompositionClock changes, and his concerns led to PR Avoid reentrant calls to sendFrame in immediate clock #92.

The bad news is that the whole point of #92 is to let the current recomposition finish its business. The new implementation behaves so much better in every other way, but I'm concerned that it may result in the same behavior you're seeing.

@chris-horner
Copy link
Contributor

I put together a sample that allows for easy demoing of the issue and switching between the two clock types. RecompositionClock.Immediate does seem to fix the weird behaviour of input fields.

molecule_input.mp4

@ganfra
Copy link

ganfra commented Jan 6, 2023

I still got the issue using RecompositionClock.Immediate, but it's way less frequent than using RecompositionClock.ContextClock.

But I have a question: why should we go through launchMolecule method in a full compose app?
We could just call our Presenter.present method in the Compose "View"
and just use molecule for testing the Presenter.

@jingibus
Copy link
Contributor

But I have a question: why should we go through launchMolecule method in a full compose app?
We could just call our Presenter.present method in the Compose "View"
and just use molecule for testing the Presenter.

Yep, that is correct.

If you want your business logic to have the lifetime of e.g. your ViewModel, though, and your activity undergoes frequent recreation for configuration changes (e.g. for rotation), you will still need to use Molecule to run composable business logic independently of Compose UI.

Google's guidance is to use Compose to handle rotation changes instead of relying on activity recreation, so you may not need to do that.

@ganfra
Copy link

ganfra commented Jan 10, 2023

Yes, was my thoughts, thanks for the confirmation!

@prashanOS
Copy link

Running into this as well.

Our current workaround is to break the UI -> Presenter -> UI loop by keeping a local mutable state to hold the text (we still send an event to the presenter so it can update its internal state):

var localText by remember { mutableStateOf(initialText) }
TextField(
  value = localText,
  onValueChange = {
    localText = it
    presenter.handleEvent(TextUpdateEvent(it)) 
})

Definitely feels architecturally impure though.

@prashanOS
Copy link

Actually, disregard the previous comment. Tested a bit more. Breaking the ui -> presenter -> ui loop is not necessary. Simply having a mutable state that you update within the onValueChange lambda is sufficient:

@Composable fun EditableText(
    text: String,
    onTextChange: (String) -> Unit,
  ) {
  var localText by remember(text) { mutableStateOf(text) }
  TextField(
    value = localText,
    onValueChange = {
      localText = it
      onTextChange(TextUpdateEvent(it)) 
  })
}

mnonnenmacher added a commit to oss-review-toolkit/ort-workbench that referenced this issue Apr 30, 2023
If the text of a `TextField` is updated from a flow, this can lead to a
jumping cursor and swallowed characters [1]. For a discussion of the
issue see [2].

Fix this by introducing a local mutable state for the text value as
suggested in [3].

Fixes #109.

[1]: JetBrains/compose-multiplatform#3089
[2]: https://medium.com/androiddevelopers/effective-state-management-for-textfield-in-compose-d6e5b070fbe5
[3]: cashapp/molecule#63 (comment)

Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@gmail.com>
mnonnenmacher added a commit to oss-review-toolkit/ort-workbench that referenced this issue Apr 30, 2023
If the text of a `TextField` is updated from a flow, this can lead to a
jumping cursor and swallowed characters [1]. For a discussion of the
issue see [2].

Fix this by introducing a local mutable state for the text value as
suggested in [3].

Fixes #109.

[1]: JetBrains/compose-multiplatform#3089
[2]: https://medium.com/androiddevelopers/effective-state-management-for-textfield-in-compose-d6e5b070fbe5
[3]: cashapp/molecule#63 (comment)

Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@gmail.com>
@StylianosGakis
Copy link

StylianosGakis commented Jul 29, 2023

I got curious about this behavior, and got inspired by watching "Reimagining text fields in Compose" by Zach Klippenstein https://www.droidcon.com/2023/07/20/reimagining-text-fields-in-compose/ to give it a shot to see how these two would play together.

After doing some trials on top of the repro made by Chris Horner, seems like TextFieldState (from BasicTextField2) is what will solve this problem for molecule without having to use the Immediate RecompositionMode.

Also no need to break the UI -> Presenter -> UI as prashanOS described. Nor do you have to have to have another state in the composable UI itself which would effectively create two sources of truth for that text.

This PR that I made here along with the video here shows that it all seems to work well together, while still allowing you to do other async work which doesn't affect TextFieldState which is also provided to the UI Composable though molecule

The gist is that inside molecule you can do val textFieldState = remember { TextFieldState("") } inside molecule's body composable and provide it inside your exposed Model directly.

@kevincianfarini
Copy link
Contributor

We were seeing this issue using even the immediate recomposition mode. Switching to use BasicTextField2 seems to resolve the weird interaction.

@kevincianfarini
Copy link
Contributor

kevincianfarini commented Apr 25, 2024

Upon further investigation, we've settled on a new architecture that slightly disobeys unidirectional data flow to work around this. It turns out that BasicTextField2 encourages consumers of the API to hoist its TextFieldState into the presentation layer. Our team cannot do this since we share our presentation layer between iOS and Android. As a result, we've decided that asynchronously sending text field state to our presenters and then back to the text field is an anti-pattern.

Instead, we'll do a few things differently.

  1. Allow the text field to control its own state at the UI layer. Only once it requires action, for example when a submit button is pressed, do we observe the value of that text field and send it to our presentation layer for processing.
  2. The value of the text field may be continuously streamed to our presentation layer for auxiliary text field concerns only. Examples of this include text field validation and enabling/disabling form buttons based on the contents of the text fields.
  3. Initial values of the text field may be set from a presentation layer model, but once set the text field should maintain its own state in the UI layer until a further action like submitting a form is taken.

For other people encountering this issue in a multiplatform context, I hope our research helps you out.

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

No branches or pull requests

10 participants