-
Notifications
You must be signed in to change notification settings - Fork 87
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
Improve TokenBundleRepositoryImpl to check if API is available #1089
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,15 +22,24 @@ import androidx.activity.compose.setContent | |
import androidx.compose.foundation.layout.Arrangement | ||
import androidx.compose.foundation.layout.Column | ||
import androidx.compose.foundation.layout.fillMaxSize | ||
import androidx.compose.foundation.layout.fillMaxWidth | ||
import androidx.compose.foundation.layout.wrapContentHeight | ||
import androidx.compose.material3.Button | ||
import androidx.compose.material3.MaterialTheme | ||
import androidx.compose.material3.Surface | ||
import androidx.compose.material3.Text | ||
import androidx.compose.runtime.Composable | ||
import androidx.compose.runtime.LaunchedEffect | ||
import androidx.compose.runtime.getValue | ||
import androidx.compose.runtime.mutableStateOf | ||
import androidx.compose.runtime.remember | ||
import androidx.compose.runtime.rememberCoroutineScope | ||
import androidx.compose.runtime.setValue | ||
import androidx.compose.ui.Alignment | ||
import androidx.compose.ui.Modifier | ||
import androidx.compose.ui.graphics.Color | ||
import androidx.compose.ui.res.stringResource | ||
import androidx.compose.ui.text.style.TextAlign | ||
import androidx.compose.ui.tooling.preview.Preview | ||
import androidx.lifecycle.lifecycleScope | ||
import com.google.android.horologist.auth.data.phone.tokenshare.TokenBundleRepository | ||
|
@@ -55,13 +64,13 @@ class MainActivity : ComponentActivity() { | |
coroutineScope = lifecycleScope | ||
) | ||
|
||
tokenBundleRepositoryDefaultKey = TokenBundleRepositoryImpl.create( | ||
tokenBundleRepositoryDefaultKey = TokenBundleRepositoryImpl( | ||
registry = registry, | ||
coroutineScope = lifecycleScope, | ||
serializer = TokenBundleSerializer | ||
) | ||
|
||
tokenBundleRepositoryCustomKey = TokenBundleRepositoryImpl.create( | ||
tokenBundleRepositoryCustomKey = TokenBundleRepositoryImpl( | ||
registry = registry, | ||
coroutineScope = lifecycleScope, | ||
serializer = TokenBundleSerializer, | ||
|
@@ -75,7 +84,16 @@ class MainActivity : ComponentActivity() { | |
modifier = Modifier.fillMaxSize(), | ||
color = MaterialTheme.colorScheme.background | ||
) { | ||
val coroutineScope = rememberCoroutineScope() | ||
var apiAvailable by remember { mutableStateOf(false) } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it's safe to do the operations, then I think defaulting to true. Or making this true/false/null would be better here. Avoid glitching the screen in the hopefully typical case where it is available. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it makes sense, and it would be better to have this in a VM, and have proper states this sample app is very simple, in comparison to the wear counterpart, it's in my plans to improve it further and will address it together |
||
LaunchedEffect(Unit) { | ||
coroutineScope.launch { | ||
apiAvailable = tokenBundleRepositoryDefaultKey.isAvailable() | ||
} | ||
} | ||
|
||
MainScreen( | ||
apiAvailable = apiAvailable, | ||
onUpdateTokenDefault = ::onUpdateTokenDefault, | ||
onUpdateTokenCustom = ::onUpdateTokenCustom | ||
) | ||
|
@@ -107,6 +125,7 @@ class MainActivity : ComponentActivity() { | |
|
||
@Composable | ||
fun MainScreen( | ||
apiAvailable: Boolean, | ||
onUpdateTokenDefault: () -> Unit, | ||
onUpdateTokenCustom: () -> Unit, | ||
modifier: Modifier = Modifier | ||
|
@@ -120,15 +139,26 @@ fun MainScreen( | |
onClick = { | ||
onUpdateTokenDefault() | ||
}, | ||
modifier = Modifier.wrapContentHeight() | ||
modifier = Modifier.wrapContentHeight(), | ||
enabled = apiAvailable | ||
) { Text(stringResource(R.string.token_share_button_update_token_default)) } | ||
|
||
Button( | ||
onClick = { | ||
onUpdateTokenCustom() | ||
}, | ||
modifier = Modifier.wrapContentHeight() | ||
modifier = Modifier.wrapContentHeight(), | ||
enabled = apiAvailable | ||
) { Text(stringResource(R.string.token_share_button_update_token_custom)) } | ||
|
||
if (!apiAvailable) { | ||
Text( | ||
text = stringResource(R.string.token_share_message_api_unavailable), | ||
modifier.fillMaxWidth(), | ||
color = Color.Red, | ||
textAlign = TextAlign.Center | ||
) | ||
} | ||
} | ||
} | ||
|
||
|
@@ -137,6 +167,7 @@ fun MainScreen( | |
fun GreetingPreview() { | ||
HorologistTheme { | ||
MainScreen( | ||
apiAvailable = true, | ||
onUpdateTokenDefault = { }, | ||
onUpdateTokenCustom = { } | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure you like the God object of the registry, but if we push this up there, then it can be done once, at startup meaning it's immediately available so screen can draw correctly immediately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be ideal to have these checks in WearDataLayerRegistry, not sure about having it immediately available, it would be an assumption about the time taken by play service lib to check it
raised #1091