google-developer-training / basic-android-kotlin-compose-training-unscramble

Apache License 2.0
93 stars 135 forks source link

Why not set the userGuess to uiState? #48

Open yangwuan55 opened 1 year ago

yangwuan55 commented 1 year ago

https://github.com/google-developer-training/basic-android-kotlin-compose-training-unscramble/blob/798422243acfabc8cb15bef05162f1908277c79d/app/src/main/java/com/example/android/unscramble/ui/GameViewModel.kt#L40

RicardoBelchior commented 1 year ago

I have the same question. Seems like this is against the concept of representing all the UI state in a single object.

@android-dev-lxl Would there be any drawback or negative side effect of moving the userGuess into the UiState class ?

android-dev-lxl commented 1 year ago

@RicardoBelchior @yangwuan55 Thank you for reaching out to us.

The reason for having 2 states is userGuess requires business logic (i.e. checkUserGuess()), it should be hoisted here in the ViewModel instead of being in the UI. However, due to the problems of state in TextFields using values coming from StateFlows (and other streams), mutableStateOf needs to be used.

Something like this:

    // Game UI state
    private val _uiState = MutableStateFlow(GameUiState())
    val uiState: StateFlow<GameUiState> = _uiState.asStateFlow()

    var userGuess by mutableStateOf("")
        private set
RicardoBelchior commented 1 year ago

Thanks for coming back!

I understand why it's hoisted in the ViewModel , just don't understand why it's not part of the GameUiState, such as:

data class GameUiState(
    val currentScrambledWord: String = "",
    ...
    val userGuess: String
)

Can you please reference a website/documentation talking about:

the problems of state in TextFields using values coming from StateFlows (and other streams)

?

Thanks again.

yangwuan55 commented 1 year ago

Thanks for coming back!

I understand why it's hoisted in the ViewModel , just don't understand why it's not part of the GameUiState, such as:

data class GameUiState(
    val currentScrambledWord: String = "",
    ...
    val userGuess: String
)

Can you please reference a website/documentation talking about:

the problems of state in TextFields using values coming from StateFlows (and other streams)

?

Thanks again.

I agree.

android-dev-lxl commented 1 year ago

You cannot have MutableState as part of the screen UI state, and because that's user input, the best and easiest way to capture it is with those APIs. https://developer.android.com/jetpack/compose/state-hoisting

yangwuan55 commented 1 year ago

You cannot have MutableState as part of the screen UI state, and because that's user input, the best and easiest way to capture it is with those APIs. https://developer.android.com/jetpack/compose/state-hoisting

It should be a part of UI state If is a part of viewmodel and will influence the UI Element.I think.

It is possible that in the future I will do some other logic on this user input, so if there are two variables that determine the UI is not pure for the MVI architecture.

However, if we initially design the rules to be uniform, and also conform to the MVI architecture, there is no need to do unnecessary refactoring when doing future extensions.

warrenlayson commented 1 year ago

@android-dev-lxl Any updates on this? I'm also wondering why it's not part of the GameUiState.

There may also be a case where the user input is saved on savedStateHandle i.e

class GameViewModel (savedStateHandle: SavedStateHandle) : ViewModel {
    private val userGuess = savedStateHandle.getStateFlow("user_guess", "")

    fun updateUserGuess(guess: String) {
        savedStateHandle["user_guess"] = guess
    }

    private val _uiState = MutableStateFlow(GameUiState())
    val uiState: StateFlow<GameUiState> =
        combine(_uiState.asStateFlow(), userGuess, ::Pair)
        .map { (uiState, guess) ->
            uiState.copy(
                userGuess = guess
            )
        }.stateIn(
            scope = viewModelScope,
            started = SharingStarted.WhileSubscribed(5_000),
            initialValue = GameUiState()
        )

   // rest of code ...
}

edit: add : ViewModel()

omarkohl commented 1 year ago

I came here wondering about the same thing since it was not clear why the userGuess was being treated differently from the rest of the UI state.

The following article delves into the issue at length. It seems that state management for TextField requires particular care in order prevent synchronization issues and unexpected behaviors.

Use MutableState to represent TextField state

Avoid using reactive streams (e.g. StateFlow) to represent your TextField state, as these structures introduce asynchronous delays. Prefer to use MutableState instead

https://medium.com/androiddevelopers/effective-state-management-for-textfield-in-compose-d6e5b070fbe5

itssinghankit commented 8 months ago

Did you understand, why they are doing this?