Skip to content

Fix stuff #1

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.

Sign up for GitHub

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

Open
Zhuinden wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix stuff #1

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 83 additions & 30 deletions app/src/main/java/com/example/composesigninsample/MainActivity.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,26 @@ import android.app.Application
import android.content.Context
import android.content.SharedPreferences
import android.os.Bundle
import android.os.Handler
import android.os.Looper
import android.preference.PreferenceManager
import androidx.activity.ComponentActivity
import androidx.activity.compose.setContent
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.Spacer
import androidx.compose.foundation.layout.fillMaxSize
import androidx.compose.foundation.layout.height
import androidx.compose.foundation.layout.padding
import androidx.compose.material.*
import androidx.compose.runtime.*
import androidx.compose.runtime.saveable.rememberSaveable
import androidx.compose.ui.text.TextStyle
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.text.style.TextAlign
import androidx.compose.ui.unit.dp
import androidx.hilt.navigation.compose.hiltViewModel
import androidx.lifecycle.ViewModel
import androidx.navigation.NavController
import androidx.navigation.compose.NavHost
import androidx.navigation.compose.composable
import androidx.navigation.compose.rememberNavController
Expand All @@ -37,7 +46,9 @@ class MainActivity : ComponentActivity() {
setContent {
ComposeSignInSampleTheme {
Surface(color = MaterialTheme.colors.background) {
AppRouter()
Box(modifier = Modifier.fillMaxSize(), contentAlignment = Alignment.Center) {
AppRouter()
}
}
}
}
Expand All @@ -49,11 +60,35 @@ fun AppRouter() {
val navController = rememberNavController()
NavHost(navController = navController, startDestination = Screen.HomeScreen.route) {
composable(Screen.HomeScreen.route) {
HomeScreen({
navController.navigate(Screen.SignInScreen.route)
})
val homeViewModel: HomeViewModel = hiltViewModel()

HomeScreen(
navigateToSignIn = {
navController.navigate(Screen.SignInScreen.route) {
popUpTo(Screen.HomeScreen.route) {
inclusive = true
}
Comment on lines +68 to +70
Copy link
Owner

@ColtonIdle ColtonIdle Jul 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just so I'm understanding correctly. This means that when I'm on the HomeScreen, and I navigate to signIn, the homeScreen is immediately popped off, right? Makes sense to me, but pop* always confuses me.

Copy link
Author

@Zhuinden Zhuinden Jul 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if you navigate from home to signin, then home is popped off

}
},
signOut = homeViewModel::signOut,
isSignedIn = homeViewModel.loggedInState,
)
}
composable(Screen.SignInScreen.route) {
val signInViewModel: SignInViewModel = hiltViewModel()

SignInScreen(
navigateToHome = {
navController.navigate(Screen.HomeScreen.route) {
popUpTo(Screen.SignInScreen.route) {
inclusive = true
}
}
},
setLoggedIn = signInViewModel::signIn,
isSignedIn = signInViewModel.loggedInState,
Comment on lines +81 to +89
Copy link
Owner

@ColtonIdle ColtonIdle Jul 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach so that SignInScreen has no dependency on SignInViewModel

Copy link
Author

@Zhuinden Zhuinden Jul 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, same

)
}
composable(Screen.SignInScreen.route) { SignInScreen({ navController.popBackStack() }) }
}
}

Expand All @@ -62,17 +97,14 @@ sealed class Screen(val route: String) {
object SignInScreen : Screen("signin")
}

/**
* SignIn Screen and ViewModel
*/
@Composable
fun SignInScreen(loggedOutEvent: () -> Unit, viewModel: SignInViewModel = hiltViewModel()) {
if (viewModel.loggedInState) {
LaunchedEffect(Unit) {
loggedOutEvent()
}
} else {
Column {
fun SignInScreen(
navigateToHome: () -> Unit,
setLoggedIn: () -> Unit,
isSignedIn: Boolean,
) {
if(!isSignedIn) {
Column(horizontalAlignment = Alignment.CenterHorizontally) {
var text by rememberSaveable { mutableStateOf("") }

TextField(
Expand All @@ -81,6 +113,9 @@ fun SignInScreen(loggedOutEvent: () -> Unit, viewModel: SignInViewModel = hiltVi
label = { Text("Email") },
singleLine = true
)

Spacer(modifier = Modifier.height(8.dp))

var text2 by rememberSaveable { mutableStateOf("") }

TextField(
Expand All @@ -89,12 +124,27 @@ fun SignInScreen(loggedOutEvent: () -> Unit, viewModel: SignInViewModel = hiltVi
label = { Text("Password") },
singleLine = true
)

Spacer(modifier = Modifier.height(24.dp))

Text(text = "Welcome to the app. Sign in to begin")
Button(onClick = viewModel::signIn) {

Spacer(modifier = Modifier.height(8.dp))

Button(onClick = setLoggedIn) {
Text(text = "Sign In")
}
}
}

DisposableEffect(key1 = isSignedIn, effect = {
if (isSignedIn) {
navigateToHome()
}

onDispose {
}
})
Comment on lines +140 to +147
Copy link
Owner

@ColtonIdle ColtonIdle Jul 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this since I don't need an if statement for isSignedIn and instead basically declare the effect with it's key.

I am new to LaunchedEffect and DisposableEffect though so I will have to read up the differences on why you chose to replace LaunchedEffect with Disposable.

Copy link
Author

@Zhuinden Zhuinden Jul 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replaced it with DisposableEffect because you don't actually need the coroutine that is executed with LaunchedEffect.

}

@HiltViewModel
Expand All @@ -109,22 +159,26 @@ class SignInViewModel @Inject constructor(
}
}

/**
* Home Screen and ViewModel
*/
@Composable
fun HomeScreen(navigateHome: () -> Unit, homeViewModel: HomeViewModel = hiltViewModel()) {
if (homeViewModel.loggedInState) {
Column {
Text(style = Typography.h1, text = "You are on the home screen!")
fun HomeScreen(navigateToSignIn: () -> Unit, signOut: () -> Unit, isSignedIn: Boolean) {
if (isSignedIn) {
Column(horizontalAlignment = Alignment.CenterHorizontally, modifier = Modifier.padding(16.dp)) {
Text(style = Typography.h1, text = "You are on the home screen!", textAlign = TextAlign.Center)
Spacer(modifier = Modifier.height(8.dp))
Text(style = Typography.body1, text = "Only for super secret signed in users only!")
Button(onClick = homeViewModel::signOut) {
Spacer(modifier = Modifier.height(16.dp))
Button(onClick = signOut) {
Text(text = "Sign Out")
}
}
} else {
LaunchedEffect(Unit) {
navigateHome()
}

DisposableEffect(isSignedIn) {
if (!isSignedIn) {
navigateToSignIn()
}

onDispose {
}
}
Comment on lines +176 to 183
Copy link
Owner

@ColtonIdle ColtonIdle Jul 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the above. I like that I can almost have a section of "effects" vs just having if statesments littered throughout.

Copy link
Author

@Zhuinden Zhuinden Jul 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more accurate to say that if your effects aren't defined like this then you'll just have bugs

Copy link
Owner

@ColtonIdle ColtonIdle Jul 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bugs you say... I wonder what kind would stem from this?

Copy link
Author

@Zhuinden Zhuinden Jul 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly that the effect shouldn't be hidden behind a conditional.

That's part of why your navigation didn't work previously.

}
Expand All @@ -139,7 +193,6 @@ class HomeViewModel @Inject constructor(
prefs.edit().putBoolean("authenticated", false).commit()
loggedInState = false
}

}

/**
Expand Down