Revert "Delete shrinewear" #180
Conversation
This reverts commit b6c6980.
Summary of ChangesHello @JohnZoellerG, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request serves as a revert, bringing back the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request reverts the deletion of the shrinewear module, effectively re-introducing a significant amount of code for a Wear OS sample application. My review focuses on treating this as new code. I've identified several issues, including a critical file with incorrect content that will prevent compilation, a couple of high-severity issues that could lead to app crashes due to unhandled exceptions, and several medium-severity issues related to code quality, use of deprecated APIs, and incomplete documentation. Addressing these points will improve the stability and maintainability of the sample app.
| /* | ||
| * Copyright 2025 The Android Open Source Project | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package com.google.webkit.webviewsample.theme | ||
|
|
||
| import androidx.compose.ui.graphics.Color | ||
|
|
||
| val Purple200 = Color(0xFFBB86FC) | ||
| val Purple500 = Color(0xFF6200EE) | ||
| val Purple700 = Color(0xFF3700B3) | ||
| val Teal200 = Color(0xFF03DAC5) |
There was a problem hiding this comment.
This file has incorrect content and package name. It appears to contain color definitions from a different sample (com.google.webkit.webviewsample.theme) instead of the NetworkException class that is used elsewhere in the code. This will cause a compilation failure.
/*
* Copyright 2025 The Android Open Source Project
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.authentication.shrinewear.network
/**
* Custom exception for network-related errors.
*/
class NetworkException(message: String) : Exception(message)| wear-compose-foundation = { module = "androidx.wear.compose:compose-foundation", version.ref = "wearComposeMaterial3" } | ||
| wear-compose-material3 = { module = "androidx.wear.compose:compose-material3", version.ref = "wearComposeMaterial3" } | ||
| wear-compose-navigation = { module = "androidx.wear.compose:compose-navigation", version.ref = "wearComposeMaterial3" } | ||
| wear-compose-ui-tooling = { group = "androidx.wear.compose", name = "compose-ui-tooling"} |
There was a problem hiding this comment.
The wear-compose-ui-tooling dependency is missing a version. This will likely cause build failures. You should add a version reference, probably wearComposeMaterial3 which is used by other wear.compose artifacts in this file.
| wear-compose-ui-tooling = { group = "androidx.wear.compose", name = "compose-ui-tooling"} | |
| wear-compose-ui-tooling = { group = "androidx.wear.compose", name = "compose-ui-tooling", version.ref = "wearComposeMaterial3"} |
| buildConfigField( | ||
| "String", "GOOGLE_SIGN_IN_SERVER_CLIENT_ID", | ||
| "\"PASTE_YOUR_SERVER_CLIENT_ID_HERE\"" | ||
| ) |
There was a problem hiding this comment.
The GOOGLE_SIGN_IN_SERVER_CLIENT_ID has a placeholder value. While this is expected in a sample, the accompanying readme.md file does not instruct the developer to replace it. This is crucial for the Google Sign-In functionality to work. Please update the documentation to include this step.
| federatedSessionId = federationOptions.sessionId | ||
| ?: throw IllegalStateException("Session ID was null in server response") |
There was a problem hiding this comment.
Throwing an IllegalStateException when the session ID is null from the server response can cause the application to crash. It's better to handle this case gracefully by logging an error and returning false, which is consistent with how other failures are handled in this class.
| federatedSessionId = federationOptions.sessionId | |
| ?: throw IllegalStateException("Session ID was null in server response") | |
| federatedSessionId = federationOptions.sessionId | |
| if (federatedSessionId == null) { | |
| Log.e(TAG, "Session ID was null in server response for federation options") | |
| signOut() | |
| return false | |
| } |
| val currentSessionId = sessionId | ||
| ?: throw IllegalStateException("Requested Passkey was not provided with a valid session.") |
There was a problem hiding this comment.
Throwing an IllegalStateException when the session ID is null can cause the app to crash, as the sessionId parameter is nullable. It would be safer to handle this case gracefully, for example by returning an error result or throwing a more specific, documented exception that can be caught upstream.
| @@ -0,0 +1,141 @@ | |||
| / | |||
There was a problem hiding this comment.
There is a stray / character at the beginning of the file. It should be removed.
| object DemoInstructionsState { | ||
| var isFirstLaunch: Boolean = true | ||
| } | ||
|
|
||
| /** | ||
| * Displays an [AlertDialog] containing introductory demo instructions for the user. | ||
| * | ||
| * This dialog is shown upon the initial launch of the application and can be dismissed | ||
| * by the user. | ||
| * | ||
| * Note: The `AlertDialog` API used here (`edgeButton` and `visible`) might be from an | ||
| * older or specific alpha version of `androidx.wear.compose.material3`. For newer | ||
| * versions, consider using `confirmButton` and `dismissButton` for actions, and | ||
| * conditionally rendering the dialog using an `if` statement. | ||
| */ | ||
| @Composable | ||
| private fun DemoInstructions() { | ||
| if (!DemoInstructionsState.isFirstLaunch) { | ||
| return | ||
| } | ||
|
|
||
| AlertDialog( | ||
| visible = true, | ||
| onDismissRequest = { | ||
| DemoInstructionsState.isFirstLaunch = false | ||
| }, | ||
| edgeButton = { | ||
| AlertDialogDefaults.EdgeButton(onClick = { | ||
| DemoInstructionsState.isFirstLaunch = false | ||
| }) | ||
| }, | ||
| title = { | ||
| Text( | ||
| text = stringResource(R.string.shrine_sample), | ||
| textAlign = TextAlign.Center, | ||
| ) | ||
| }, | ||
| text = { Text(stringResource(R.string.see_readme_md_for_usage_directions)) }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
There are a couple of areas for improvement in the DemoInstructions composable:
- The
DemoInstructionsStateobject uses a global mutablevarto track if the dialog has been shown. This is not a robust solution as the state will be reset on process death, causing the dialog to reappear unexpectedly. A better approach is to userememberSaveablewithin the composable to persist this state. - The
visibleparameter ofAlertDialogis deprecated. The dialog should be conditionally composed based on a state variable.
Here's a suggested refactoring that addresses both points.
@Composable
private fun DemoInstructions() {
var showDialog by rememberSaveable { mutableStateOf(true) }
if (showDialog) {
AlertDialog(
onDismissRequest = { showDialog = false },
edgeButton = {
AlertDialogDefaults.EdgeButton(onClick = { showDialog = false })
},
title = {
Text(
text = stringResource(R.string.shrine_sample),
textAlign = TextAlign.Center,
)
},
text = { Text(stringResource(R.string.see_readme_md_for_usage_directions)) },
)
}
}| AlertDialog( | ||
| visible = showDialog, | ||
| onDismissRequest = { signOut() }, | ||
| edgeButton = { AlertDialogDefaults.EdgeButton(onClick = { signOut() }) }, | ||
| title = { Text(text = "You are now signed in", textAlign = TextAlign.Center) }, | ||
| text = { Text("To sign out and start over, tap the check mark") }, | ||
| ) |
There was a problem hiding this comment.
The visible parameter of AlertDialog is deprecated. The recommended practice is to conditionally compose the dialog based on a state variable rather than controlling its visibility with this parameter.
if (showDialog) {
AlertDialog(
onDismissRequest = { signOut() },
edgeButton = { AlertDialogDefaults.EdgeButton(onClick = { signOut() }) },
title = { Text(text = "You are now signed in", textAlign = TextAlign.Center) },
text = { Text("To sign out and start over, tap the check mark") },
)
}| } catch (e: CancellationException) { | ||
| throw e | ||
| } catch (e: Exception) { | ||
| e.printStackTrace() |
There was a problem hiding this comment.
Calling e.printStackTrace() is discouraged in production code. It's better to use Android's logging framework, like Log.e(TAG, "...", e), to ensure exceptions are properly logged and can be collected by tools like Logcat. This should be fixed in all catch blocks in this file (lines 114, 170, 191).
| e.printStackTrace() | |
| Log.e(TAG, "Failed to retrieve verification info", e) |
| /** | ||
| * Simple implementation of a POST request. Normally you'd use a library to do these requests. | ||
| */ | ||
| private suspend fun doPostRequest( | ||
| url: String, | ||
| params: Map<String, String>, | ||
| dispatcher: CoroutineDispatcher = Dispatchers.IO, | ||
| ): JSONObject { | ||
| return withContext(dispatcher) { | ||
| val conn = (URL(url).openConnection() as HttpURLConnection) | ||
| val postData = StringBuilder() | ||
| for ((key, value) in params) { | ||
| if (postData.isNotEmpty()) postData.append('&') | ||
| postData.append(URLEncoder.encode(key, "UTF-8")) | ||
| postData.append('=') | ||
| postData.append(URLEncoder.encode(value, "UTF-8")) | ||
| } | ||
| val postDataBytes = postData.toString().toByteArray(charset("UTF-8")) | ||
|
|
||
| conn.apply { | ||
| requestMethod = "POST" | ||
| setRequestProperty("Content-Type", "application/x-www-form-urlencoded") | ||
| setRequestProperty("Content-Length", postDataBytes.size.toString()) | ||
| doOutput = true | ||
| outputStream.write(postDataBytes) | ||
| } | ||
|
|
||
| val inputReader = BufferedReader(InputStreamReader(conn.inputStream, "UTF-8")) | ||
| val response = inputReader.readText() | ||
|
|
||
| Log.d(TAG, DEBUG_POST_REQUEST_UTIL_RESPONSE.format(response)) | ||
|
|
||
| JSONObject(response) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Simple implementation of a GET request. Normally you'd use a library to do these requests. | ||
| */ | ||
| private suspend fun doGetRequest( | ||
| url: String, | ||
| requestHeaders: Map<String, String>, | ||
| dispatcher: CoroutineDispatcher = Dispatchers.IO, | ||
| ): JSONObject { | ||
| return withContext(dispatcher) { | ||
| val conn = (URL(url).openConnection() as HttpURLConnection) | ||
| requestHeaders.onEach { (key, value) -> | ||
| conn.setRequestProperty(key, value) | ||
| } | ||
| val inputReader = BufferedReader(InputStreamReader(conn.inputStream, "UTF-8")) | ||
| val response = inputReader.readText() | ||
|
|
||
| Log.d(TAG, DEBUG_REQUEST_UTIL_RESPONSE.format(response)) | ||
|
|
||
| JSONObject(response) | ||
| } | ||
| } |
There was a problem hiding this comment.
This ViewModel implements doPostRequest and doGetRequest using the low-level HttpURLConnection. Since the project already includes OkHttp as a dependency (used in AuthNetworkClient), it would be better to use OkHttp here as well for consistency and to leverage its features like connection pooling, interceptors, and a simpler API. This would reduce boilerplate code and make the networking logic more robust and maintainable.
1 participant
Reverts #179