orbit-mvi icon indicating copy to clipboard operation
orbit-mvi copied to clipboard

TextField resets when using orbit and a samsung keyboard.

Open DarrenMiddleton opened this issue 4 years ago • 8 comments

There is an issue when using orbit with a Samsung device. The device must use the Samsung keyboard and not an alternative one to reproduce this issue.

When using a Textfield that displays a value that is from a state and updates that value in the state by using the onValueChange value with orbits intent and reduce function, the Textfield value will randomly reset if the user is using a Samsung keyboard.

Example:

class MainActivity : ComponentActivity() {
    private val viewModel = ExampleViewModel()

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContent {
            MyApplicationTheme {
                // A surface container using the 'background' color from the theme
                Surface(color = MaterialTheme.colors.background) {
                    Screen(viewModel)
                }
            }
        }
    }
}

@Composable
fun Screen(viewModel: ExampleViewModel) {

    val state = viewModel.container.stateFlow.collectAsState().value

    TextField(value = state.text, onValueChange = { it ->
        viewModel.updateText(it)
    })

}

data class ViewState(
    val text: String = ""
)

sealed class SideEffect

class ExampleViewModel : ContainerHost<ViewState, SideEffect>, ViewModel() {

    override val container = container<ViewState, SideEffect>(ViewState())

    fun updateText(text: String) = intent {

        reduce {
            state.copy(text = text)
        }
    }
}

Example project: https://github.com/DarrenMiddleton/orbit-mvi-keyboard-issue

DarrenMiddleton avatar Oct 10 '21 02:10 DarrenMiddleton

Is this an issue of configuration change?

I note you are creating the ViewModel directly in the Activity in the above sample, which means rotation won't work either. Instead you should be using either:

  • val viewModel by viewModels<ExampleViewModel>() at the top

or

  • Composes val viewModel = viewModel<TextViewModel>() within the Composable

mattmook avatar Oct 14 '21 10:10 mattmook

I've observed similar behavior in my project.

It seems that TextField doesn't support asynchronous updates. Sometimes when the process of reducing the state and emitting it takes too long, TextField starts to behave "weird". In my case, it resets the keyboard (if I had a keyboard in number mode it automatically changes to the default one, during typing).

I'm using the OnePlus phone but I assume that it doesn't matter, the error is connected to Compose and its TextField implementation.

I'm wondering what can be done about it. Maybe flag that would enable to force an immediate state reduction?

JakubMosakowski avatar Nov 01 '21 20:11 JakubMosakowski

One solution I came up with (a bit dirty one - I'd like to have one source of data :/) is to not update field values from the state directly.

For example:

@Composable
fun ExampleTextField(
    initialValue: String = "",
    onValueChange: (String) -> Unit
) {
    val state = remember { mutableStateOf(initialValue) }

    OutlinedTextField(
        value = state.value,
        onValueChange = {
            state.value = it
            onValueChange(it)
        }
    )
}

onValueChange informs the view model that value was changed. This solution works but unfortunately, it makes the data flow unidirectional. TextField informs the view model about changes, but the view model cannot do it. I assume there are many use cases that would require two-way - that's why a better solution is needed.

JakubMosakowski avatar Nov 01 '21 21:11 JakubMosakowski

I wonder if there's a more fundamental issue in Compose with this?

As a reference: https://issuetracker.google.com/issues/200577798 https://issuetracker.google.com/issues/204522152 https://stackoverflow.com/questions/69703448/compose-text-field-strange-behaviour-when-setting-a-max-length-for-text

mattmook avatar Nov 03 '21 15:11 mattmook

I experiment the same issue and not only on samsung devices.

I have opened an issue some time ago (https://issuetracker.google.com/issues/179694566) but it was closed as "Won't Fix (Not reproducible)".

Maybe a hint here : https://github.com/cashapp/molecule/issues/63

DupinC avatar Dec 13 '21 22:12 DupinC

So, it seems like sometimes the problem is the delay. Could be "fixed" by using Main or Unconfined dispatcher, like this

container(
  ...
  settings = Container.Settings(intentDispatcher = Dispatchers.Unconfined)
)

Anyway, this should be used with caution as any real job inside the intent{} should be then moved to the withContext block.

almozavr avatar Mar 21 '22 15:03 almozavr

So, it seems like sometimes the problem is the delay. Could be "fixed" by using Main or Unconfined dispatcher, like this

container(
  ...
  settings = Container.Settings(intentDispatcher = Dispatchers.Unconfined)
)

Anyway, this should be used with caution as any real job inside the intent{} should be then moved to the withContext block.

Does it "fixed" as well when defining specific intent {} then we use withContext(Unconfined) inside it?

mochadwi avatar Mar 22 '22 05:03 mochadwi

So I finally got round to experimenting with this and came up with a solution that should be good for typical use cases: https://github.com/orbit-mvi/orbit-mvi/pull/148

With the usual caveat that we should not do any suspending operations inside a blockingIntent - fire off a new intent if you need to e.g. call the backend when searching on textfield typing.

The article @GuilhE linked above seems to suggest this solution may not be 100% bulletproof, but is a huge improvement over the status quo and works well under our own testing. YMMV

Rosomack avatar Dec 01 '22 14:12 Rosomack

Fixed in #148

Rosomack avatar Dec 02 '22 21:12 Rosomack

Not sure if I'm doing something wrong, but I managed to reproduce this even when using blockingIntent. The difference between the example code and my code is that I'm using actions to trigger text change. Here's an example:

@Composable
private fun ViewState(
  viewModel: LoginViewModel,
) {
  val state by viewModel.collectAsState()
  LoginContent(
    state = state,
    { viewModel.sendAction(LoginUserAction.EmailChanged(it)) },
    { viewModel.sendAction(LoginUserAction.PasswordChanged(it)) },
    { viewModel.sendAction(LoginUserAction.LoginButtonClicked) }
  )
}

@Composable
private fun LoginContent(
  state: LoginViewState,
  onEmailChanged: (String) -> Unit,
  onPasswordChanged: (String) -> Unit,
  onLoginClicked: () -> Unit,
) {

  Column(
    Modifier
      .fillMaxSize()
      .padding(16.dp),
    verticalArrangement = Arrangement.Center,
    horizontalAlignment = Alignment.CenterHorizontally
  ) {
    TextField(
      modifier = Modifier
        .fillMaxWidth()
        .padding(8.dp),
      onValueChange = onEmailChanged,
      value = state.email,
      enabled = true,
      label = { Text("Username") },
      colors = TextFieldDefaults.textFieldColors(containerColor = MaterialTheme.colorScheme.background),
      isError = state.emailError.isNotEmpty()
    )

LoginViewModel

override fun processUserAction(action: LoginUserAction) {
    when (action) {
      is LoginButtonClicked -> login()
      is EmailChanged       -> emailChanged(action.email)
      is PasswordChanged    -> passwordChanged(action.password)
    }
  }
  
  private fun emailChanged(email: String) = blockingIntent {
    reduce {
      state.copy(email = email)
    }
  }

  private fun passwordChanged(password: String) = blockingIntent {
    reduce {
      state.copy(password = password)
    }
  }

I'm also attaching the video.

https://github.com/orbit-mvi/orbit-mvi/assets/1207004/a0d5d56c-1ca9-484f-9b80-b148f42083ad

EDIT: it works when calling the viewmodel functions directly, like viewModel.emailChanged(it)

vedo1608 avatar Feb 09 '24 22:02 vedo1608

Tested the same thing with BasicTextField2, where in onValueChange I called viewModel.sendAction(TextChanged(it)), and I didn't have the above bug. It must be related to the way the new basic text field handles asynchronous changes.

vedo1608 avatar Feb 15 '24 10:02 vedo1608