Krishna/nav3 result screen #102
Conversation
…vigation.kt using Nav3 instead of driving them from CreationScreen.kt # Conflicts: # feature/results/src/main/java/com/android/developers/androidify/customize/ImageRenderer.kt # Conflicts: # feature/results/src/main/java/com/android/developers/androidify/customize/CustomizeState.kt
This commit refactors image handling in the `CreationViewModel` and `CustomizeExportViewModel`. **CreationViewModel:** - The `saveBitmapToCache` function has been removed. - Image saving is now delegated to `imageGenerationRepository.saveImage(bitmap)`. **CustomizeExportViewModel:** - Images are now loaded from a URI using a new `convertUriToBitmap(uri)` suspend function. This function uses the `application.contentResolver` to open an input stream from the URI and decodes it into a Bitmap. - The `export()` function was updated to use `convertUriToBitmap` to get the image for export instead of directly accessing `state.value.exportImageCanvas.imageBitmap`.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @srikrishnasakunia, 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 significantly refactors the application's image processing and navigation flow. The core change involves transitioning from passing large Bitmap objects directly between screens to using Uris, which are more efficient and prevent potential memory issues. New navigation routes for displaying results and customizing images have been introduced, ensuring a smoother user experience. The underlying ViewModels and UI components have been updated to support this URI-based approach, leveraging assisted injection for better dependency management and a new URI serialization mechanism for seamless data transfer.
Highlights
- Navigation System Refactoring: The application's navigation system has been significantly refactored to introduce new routes for displaying image generation results (
Result) and for customizing/exporting images (CustomizeExport). This change streamlines the user flow after an image is created. - Migration to URI-based Image Handling: Image handling throughout the application, particularly in
Creation,Results, andCustomizeExportscreens and their respective ViewModels, has been migrated from directly passingBitmapobjects to usingUri(Uniform Resource Identifier) objects. This improves efficiency and reduces potential memory issues. - URI Serialization for Navigation: A new
UriSerializerhas been implemented to enableandroid.net.Uriobjects to be seamlessly serialized and deserialized when passed as arguments between navigation destinations, leveraging Kotlin'skotlinx.serializationlibrary. - ViewModel Updates with Assisted Injection: The
CreationViewModel,ResultsViewModel, andCustomizeExportViewModelhave been updated to use Hilt's assisted injection, allowing them to receive specific arguments (like image URIs) directly during their construction, which simplifies dependency management and state initialization. - Enhanced Local File Provider: A new
loadBitmapFromUrifunction has been added to theLocalFileProviderutility, enabling efficient loading ofBitmapobjects fromUris. This function is now utilized by ViewModels that need to display or process image data from a URI.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request refactors the navigation flow for creating and viewing results, moving from a state-based navigation within CreationScreen to a proper navigation-graph-based approach using navigation-compose-v3. This is a great improvement for separation of concerns and maintainability. The use of UriSerializer for passing Uris and assisted injection for ViewModels with parameters are well-implemented. My feedback includes some suggestions for improving logging, cleaning up dead/commented code, and handling a potential race condition to make the implementation more robust.
| private fun loadInitialBitmap(uri: Uri) { | ||
| viewModelScope.launch { | ||
| try { | ||
| val bitmap = localFileProvider.loadBitmapFromUri(uri) | ||
| _state.update { | ||
| it.copy( | ||
| exportImageCanvas = it.exportImageCanvas.copy(imageBitmap = bitmap) | ||
| ) | ||
| } | ||
| } catch (e: Exception) { | ||
| _snackbarHostState.value.showSnackbar("Could not load image.") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The initial bitmap is loaded asynchronously here, but there's no state to indicate that this loading is in progress. Functions like triggerAiBackgroundGeneration depend on state.value.exportImageCanvas.imageBitmap, which is set here. If a user triggers an action that uses this bitmap before it's loaded, the action might fail silently.
Consider adding a loading state to CustomizeExportState (e.g., isBitmapLoading) that is set to true before this operation and false when it completes or fails. The UI can then use this state to disable controls or show an indicator.
| } | ||
| null | ||
| } catch (e: Exception) { | ||
| e.printStackTrace() |
There was a problem hiding this comment.
Using e.printStackTrace() is not recommended in production code as the output can be lost, making debugging difficult. It's better to use Android's Log class for proper, tagged logging.
| e.printStackTrace() | |
| android.util.Log.e("LocalFileProvider", "Error loading bitmap from URI", e) |
| fun setArguments( | ||
| resultImageUrl: Bitmap, | ||
| resultImageUrl: Uri, | ||
| originalImageUrl: Uri?, | ||
| ) { | ||
| _state.update { | ||
| CustomizeExportState( | ||
| originalImageUrl, | ||
| exportImageCanvas = it.exportImageCanvas.copy(imageBitmap = resultImageUrl), | ||
| exportImageCanvas = it.exportImageCanvas.copy(imageUri = resultImageUrl), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
The setArguments function appears to be unused now that the ViewModel is initialized with the required data through its init block using assisted injection. This seems to be leftover code from the refactoring. It's best to remove dead code to improve maintainability.
| ) | ||
| } | ||
| } catch (e: Exception) { | ||
| _snackbarHostState.value.showSnackbar("Could not load image.") |
There was a problem hiding this comment.
The error message "Could not load image." is hardcoded. For better maintainability and to support localization in the future, it's best practice to use string resources.
| _snackbarHostState.value.showSnackbar("Could not load image.") | |
| _snackbarHostState.value.showSnackbar(getApplication<Application>().getString(com.android.developers.androidify.results.R.string.error_load_image)) |
| /*resultImageUri: Uri, | ||
| originalImageUri: Uri?, | ||
| promptText: String?, | ||
| promptText: String?,*/ | ||
| modifier: Modifier = Modifier, | ||
| verboseLayout: Boolean = allowsFullContent(), | ||
| onBackPress: () -> Unit, | ||
| onAboutPress: () -> Unit, | ||
| onNextPress: () -> Unit, | ||
| viewModel: ResultsViewModel = hiltViewModel<ResultsViewModel>(), | ||
| onNextPress: (resultImageUri:Uri, originalImageUri:Uri?) -> Unit, | ||
| viewModel: ResultsViewModel, | ||
| ) { | ||
| val state = viewModel.state.collectAsStateWithLifecycle() | ||
| LaunchedEffect(resultImage, originalImageUri, promptText) { | ||
| viewModel.setArguments(resultImage, originalImageUri, promptText) | ||
| } | ||
| /*LaunchedEffect(resultImageUri, originalImageUri, promptText) { | ||
| viewModel.setArguments(resultImageUri, originalImageUri, promptText) | ||
| }*/ |
There was a problem hiding this comment.
This file contains commented-out code. It's better to remove it to keep the codebase clean and maintainable.
1 participant
No description provided.