4

i am working on compose project. I have simple login page. After i click login button, loginState is set in viewmodel. The problem is when i set loginState after service call, my composable recomposed itself multiple times. Thus, navcontroller navigates multiple times. I don't understand the issue. Thanks for helping.

My composable :

@Composable
fun LoginScreen(
    navController: NavController,
    viewModel: LoginViewModel = hiltViewModel()
) {
    Column(
        modifier = Modifier.fillMaxSize(),
        verticalArrangement = Arrangement.SpaceEvenly
    ) {
        val email by viewModel.email
        val password by viewModel.password
        val enabled by viewModel.enabled

        if (viewModel.loginState.value) {
            navController.navigate(Screen.HomeScreen.route) {
              popUpTo(Screen.LoginScreen.route) {
                 inclusive = true
              }
            }
        }

        LoginHeader()
        LoginForm(
            email = email,
            password = password,
            onEmailChange = { viewModel.onEmailChange(it) },
            onPasswordChange = { viewModel.onPasswordChange(it) }
        )
        LoginFooter(
            enabled,
            onLoginClick = {
                viewModel.login()
            },
            onRegisterClick = {
                navController.navigate(Screen.RegisterScreen.route)
            }
        )
    }

ViewModel Class:

@HiltViewModel
class LoginViewModel @Inject constructor(
    private val loginRepository: LoginRepository,
) : BaseViewModel() {

    val email = mutableStateOf(EMPTY)
    val password = mutableStateOf(EMPTY)
    val enabled = mutableStateOf(false)
    val loginState = mutableStateOf(false)

    fun onEmailChange(email: String) {
        this.email.value = email
        checkIfInputsValid()
    }

    fun onPasswordChange(password: String) {
        this.password.value = password
        checkIfInputsValid()
    }

    private fun checkIfInputsValid() {
        enabled.value =
            Validator.isEmailValid(email.value) && Validator.isPasswordValid(password.value)
    }

    fun login() = viewModelScope.launch {
        val response = loginRepository.login(LoginRequest(email.value, password.value))
        loginRepository.saveSession(response)
        loginState.value = response.success ?: false
    }
}
Berkay Kireçci
  • 651
  • 5
  • 18

1 Answers1

4

You should not cause side effects or change the state directly from the composable builder, because this will be performed on each recomposition.

Instead you can use side effects. In your case, LaunchedEffect can be used.

if (viewModel.loginState.value) {
    LaunchedEffect(Unit) {
        navController.navigate(Screen.HomeScreen.route) {
            popUpTo(Screen.LoginScreen.route) {
                inclusive = true
            }
        }
    }
}

But I think that much better solution is not to listen for change of loginState, but to make login a suspend function, wait it to finish and then perform navigation. You can get a coroutine scope which will be bind to your composable with rememberCoroutineScope. It can look like this:

suspend fun login() : Boolean {
    val response = loginRepository.login(LoginRequest(email.value, password.value))
    loginRepository.saveSession(response)
    return response.success ?: false
}

Also check out Google engineer thoughts about why you shouldn't pass NavController as a parameter in this answer (As per the Testing guide for Navigation Compose ...)


So your view after updates will look like:

@Composable
fun LoginScreen(
    viewModel: LoginViewModel = hiltViewModel(),
    onLoggedIn: () -> Unit,
    onRegister: () -> Unit,
) {
    // ...
    val scope = rememberCoroutineScope()
    LoginFooter(
        enabled,
        onLoginClick = {
            scope.launch {
                if (viewModel.login()) {
                    onLoggedIn()
                }
            }
        },
        onRegisterClick = onRegister
    )
    // ...
}

And your navigation route:

composable(route = "login") {
    LoginScreen(
        onLoggedIn = {
            navController.navigate(Screen.HomeScreen.route) {
                popUpTo(Screen.LoginScreen.route) {
                    inclusive = true
                }
            }
        },
        onRegister = {
            navController.navigate(Screen.RegisterScreen.route)
        }
    )
}
Phil Dukhov
  • 67,741
  • 15
  • 184
  • 220
  • LaunchEffect works perfectly, i have tried it but looks like i was using it wrong. Thanks for the help and advice about testing! – Berkay Kireçci Sep 05 '21 at 12:10
  • @BerkayKireçci you're welcome=) have you tried making your function suspend too? I really think it's much cleaner solution. – Phil Dukhov Sep 05 '21 at 12:16
  • But i still don't get why does view recompose itself multiple times despite changing state once. Is this how compose works or am i doing something wrong? – Berkay Kireçci Sep 05 '21 at 12:16
  • @BerkayKireçci from the code I see everything else seems fine. You can try adding `print` to see if it recomposes after adding `LaunchEffect`(obviously print should be outside of `LaunchEffect`). It shouldn't – Phil Dukhov Sep 05 '21 at 12:19
  • I logged inside and outside LaunchEffect and there is one log inside it as exptected. But after onLoggedIn() works, it's still logging outside it. This is why i am confused. It looks like: ->"if" ->"launcheffect" ->"if" ->"if" – Berkay Kireçci Sep 05 '21 at 12:37
  • I've checked on a sample code, and yes it looks like "rendering" two more times after the `navigate`. Not sure if that's a bug, anyway your view can be recomposed many times for many reasons, that's not something bad, you just shoudn't perform any heavy calculations inside view builder anyway. – Phil Dukhov Sep 05 '21 at 13:32