https://kotlinlang.org logo
Title
c

Colton Idle

07/25/2021, 1:30 AM
Code review request! Please be nice as compose + navigation makes my brain hurt. Project = login/logout screens + compose + AAC compose nav + Hilt + AAC ViewModel These ~100 lines of code will serve as the base of my project. Appreciate any input you may have. https://github.com/ColtonIdle/ComposeSignInSample/blob/main/app/src/main/java/com/example/composesigninsample/MainActivity.kt
b

Bryan L

07/25/2021, 2:23 AM
I prefer making sealed classes for my navigation. Easy to give each screen different parameters. For example, in the event you use it for a bottom nav bar.. you can call the icon or the title of the screen's name
@Composable
fun AppRouter() {
    val navController = rememberNavController()
    NavHost(navController = navController, startDestination = Screen.HomeScreen.route) {
        composable(Screen.HomeScreen.route) { HomeScreen(navController = navController) }
        composable(Screen.SignInScreen.route) { SignInScreen(navController = navController) }
    }
}

sealed class Screen(val route: String) {
    object HomeScreen : Screen("home")
    object SignInScreen : Screen("signin")

}
@Composable
fun AppRouter() {
    val navController = rememberNavController()
    NavHost(navController = navController, startDestination = Screen.HomeScreen.route) {
        composable(Screen.HomeScreen.route) { HomeScreen(navController = navController) }
        composable(Screen.LoginScreen.SignInScreen.route) { SignInScreen(navController = navController) }
    }
}

sealed class Screen(val route: String) {
    object HomeScreen : Screen("home")

    sealed class LoginScreen {

        object SignInScreen : Screen("signin")
        object RegisterScreen : Screen("register")
    }
}
You could also do something like this if you'd prefer to show it as subroute. Someone else will give better insight. But just some food for thought! (still only calls "signin". You can play around with it. 🙂 )
fun AppRouter() {
    val navController = rememberNavController()
    NavHost(navController = navController, startDestination = Screen.HomeScreen.route) {
        composable(Screen.HomeScreen.route) { HomeScreen(navController = navController) }
        composable(Screen.LoginScreen.SignInScreen.subRoute) { SignInScreen(navController = navController) }
    }
}

sealed class Screen(val route: String) {
    object HomeScreen : Screen("home")

    sealed class LoginScreen(val subRoute: String) : Screen("login") {

        object SignInScreen : LoginScreen("signin")
        object RegisterScreen : LoginScreen("register")
    }
1
a

Abhishek Dewan

07/25/2021, 2:33 AM
I think perhaps passing lambdas instead of passing the entire navigation controller might be better idea. It isolates you from debugging navigation related down the stack in the future when the app becomes more complicated
6
c

Colton Idle

07/25/2021, 2:35 AM
@Bryan L thanks! Sealed classes makes sense. I'll update.
@Abhishek Dewan makes sense to me too, but I think when listening to the latest Q&A session with Murat and Ian, I think Ian mentioned that it should be safe to only pass in a navController into the top level screen, but he wouldn't rec passing it down any deeper than that.
💯 2
a

Abhishek Dewan

07/25/2021, 2:47 AM
if its a single developer on a project it shouldn't be a problem at all since you can control everything. My project I pass lambdas just to make sure I kinda get used to using them like that so that when my work adapts compose I can impart it to our team :)
c

Colton Idle

07/25/2021, 8:54 PM
Does anyone here see anything wrong with how the sign in/out logic is handled? For example, if you run the repo, it doesn't actually work correctly.
a

Adam Powell

07/25/2021, 9:18 PM
Don't mutate the nav controller directly in a composable function
2
c

Colton Idle

07/26/2021, 2:08 AM
a

Adam Powell

07/26/2021, 2:15 AM
your call, you could go either way depending on the shape of the rest of your code. The lambda is more generic/probably more testable. And like in the other thread, don't call it directly in composition. 🙂
c

Colton Idle

07/26/2021, 2:16 AM
you could go either way depending on the shape of the rest of your code.
What's either way? I figured the only option is to pass in a lambda, or do what I'm doing currently (calling navigate directly). Is there another option?
a

Adam Powell

07/26/2021, 2:17 AM
two orthogonal topics: what to call vs. where/how/when to call it.
c

Colton Idle

07/26/2021, 2:22 AM
I'm going to make a commit or two to this repo, to see if I'm understanding correctly. 🙃
@Bryan L made your change recommendation here! https://github.com/ColtonIdle/ComposeSignInSample/commit/2df04c93cd17a6153f30e0d4486a02a601cefdf2 Let me know if that's what you were thinking of. Now moving onto Adams recommendation.
@Adam Powell was this what you were referring to in terms of using LaunchedEffect? Or am I still wrong. https://github.com/ColtonIdle/ComposeSignInSample/commit/adef3b2897d794a9e20cc96ca0fa09acb891148e
a

Adam Powell

07/26/2021, 2:39 AM
that'll ensure that it only happens once when the effect enters the composition, yes
there's some details if you wanted to be 100% correct like probably use
navController
instead of
Unit
as the key parameter to
LaunchedEffect
c

Colton Idle

07/26/2021, 2:43 AM
Okay. So if my composables recomposes a 100000 times, then using LaunchedEffect makes sure that it only happens once?
And what if I didn't want to pass in navController, and instead wanted to pass in a lambda. What would go into the launched effect as the key?
a

Adam Powell

07/26/2021, 2:44 AM
well, now we're back to product requirements, but in this case the product is your API surface here 🙂
strictly speaking
LaunchedEffect
launches a coroutine to run the trailing lambda block when it first enters composition and cancels it when it leaves the composition. If any of the key parameters change while the
LaunchedEffect
is still present, then it will cancel the coroutine and start a new one again.
now, lambda captures can change.
LaunchedEffect
won't change its lambda capture out from under the running coroutine, at least not on its own.
You could even argue that changing for the
navController
here is wrong and
Unit
is more correct in this situation.
🤯 1
c

Colton Idle

07/26/2021, 2:47 AM
first enters composition and cancels it when it leaves the composition.
Hm. Let me make sure I understand. "first enters composition" is like when a HomeScreen composable is added to the NavHost? or by "enter composition" do you mean everytime it recomposes it "enters composition"
a

Adam Powell

07/26/2021, 2:49 AM
by "enter composition" I mean that it was not present before and it is present now
c

Colton Idle

07/26/2021, 2:51 AM
Got it. That's what I thought. It's kinda like the lifecycle of the composable. So even thought a composable can recompose 10000 times... it typically only enters composition and exits composition once during a use case of a composable being an entire screen. Hence why launchedEffect works nicely for what I want to do
a

Adam Powell

07/26/2021, 2:52 AM
if you're curious, the whole thing works based on RememberObserver
c

Colton Idle

07/26/2021, 2:52 AM
Desparately need a lint rule to somehow tell people they are doing something bad during recomposition. lol
a

Adam Powell

07/26/2021, 2:52 AM
if you
remember { }
any object that implements
RememberObserver
, compose will invoke its callbacks during these events of the composition lifecycle
👍 1
c

Colton Idle

07/26/2021, 2:53 AM
Okay. Updated the repo once more... I think I'm getting closer to something not completely terrible? https://github.com/ColtonIdle/ComposeSignInSample/commit/e648a8839f6bfd3afaa8425a482acbbfd7442326
a

Adam Powell

07/26/2021, 2:54 AM
yep, seems like a solid place to start from
👍 1