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

Why not set the userGuess to uiState? #48

Open
yangwuan55 opened this issue Feb 1, 2023 · 9 comments
Open

Why not set the userGuess to uiState? #48

yangwuan55 opened this issue Feb 1, 2023 · 9 comments
Assignees

Comments

@yangwuan55
Copy link

@RicardoBelchior
Copy link

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 android-dev-lxl self-assigned this Feb 13, 2023
@android-dev-lxl
Copy link
Collaborator

@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

@android-dev-lxl android-dev-lxl pinned this issue Feb 13, 2023
@RicardoBelchior
Copy link

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link

warrenlayson commented Jul 9, 2023

@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
Copy link

omarkohl commented Sep 14, 2023

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
Copy link

Did you understand, why they are doing this?

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

6 participants