-
Notifications
You must be signed in to change notification settings - Fork 132
Refactor GenAI Image Description sample to use UI state #68
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
Conversation
This commit refactors the GenAIImageDescription sample to use a `UiState` sealed class in the `GenAIImageDescriptionViewModel`. This improves the state management and provides a better user experience by showing the status of the feature, including download progress. The changes are based on the implementation of the GenAISummarization sample.
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.
Summary of Changes
Hello @calren, 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 GenAI Image Description and GenAI Summarization sample applications to implement a more robust and reactive UI state management system. By introducing a comprehensive UiState sealed class, the samples now offer detailed feedback to users throughout the AI model's lifecycle, from feature availability checks and download progress to content generation and error handling. This change not only enhances the user experience with clearer status indications but also improves the maintainability and architectural clarity of the sample code.
Highlights
- Unified UI State Management: Introduced a
sealed class UiStatefor both the GenAI Image Description and GenAI Summarization samples, providing a structured way to represent various stages of AI model interaction (initial, checking status, downloading, generating, success, error). - Enhanced User Feedback: The UI now dynamically updates to show granular progress and status, such as feature download progress (bytes downloaded/total bytes) and specific error messages, significantly improving the user experience.
- Improved ViewModel Architecture: Both
ViewModelimplementations were migrated toAndroidViewModelto properly handleApplicationcontext, and the logic was refactored to manage the newUiStateflow, including cancellation of ongoing operations. - Dependency Updates: Added
androidx.lifecycle.runtime.composeto both samplebuild.gradle.ktsfiles to supportcollectAsStateWithLifecyclefor lifecycle-aware state collection.
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 or fill out our survey to provide feedback.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors the GenAI Image Description sample to use a UI state holder, which is a great architectural improvement. The changes align well with modern Android development practices like Unidirectional Data Flow.
My review includes a few suggestions to further improve the code:
- Adhering more closely to the repository's style guide regarding
AndroidViewModel. - Improving the safety of state updates by making them atomic to prevent potential race conditions and crashes.
- Removing a piece of unreachable code for better clarity.
These changes will make the sample more robust and a better reference for developers.
...c/main/java/com/android/ai/samples/genai_image_description/GenAIImageDescriptionViewModel.kt
Outdated
Show resolved
Hide resolved
...c/main/java/com/android/ai/samples/genai_image_description/GenAIImageDescriptionViewModel.kt
Outdated
Show resolved
Hide resolved
.../src/main/java/com/android/ai/samples/genai_image_description/GenAIImageDescriptionScreen.kt
Outdated
Show resolved
Hide resolved
...c/main/java/com/android/ai/samples/genai_image_description/GenAIImageDescriptionViewModel.kt
Show resolved
Hide resolved
…b.com/android/ai-samples into refactor/genai-image-description-mvvm
.../src/main/java/com/android/ai/samples/genai_image_description/GenAIImageDescriptionScreen.kt
Outdated
Show resolved
Hide resolved
| if (uiState !is GenAIImageDescriptionUiState.Initial) { | ||
| val bottomSheetText = when (val state = uiState) { | ||
| is GenAIImageDescriptionUiState.DownloadingFeature -> stringResource( | ||
| id = R.string.genai_image_description_downloading, |
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.
-
I see that this string doesn't accept any params:
"Downloading feature" -
Should we use "image_desc_downloading" instead?
Downloading feature: %1$d / %2$d
And it looks like image_desc_downloading is never used. -
Also maybe the string should be: "Downloading: %1$d of %2$d bytes"
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.
ah yes, this should have been using image_desc_downloading, fixed
| ) : GenAIImageDescriptionUiState() | ||
|
|
||
| private var imageDescriber: ImageDescriber? = null | ||
| data class Generating(val generatedOutput: String) : GenAIImageDescriptionUiState() |
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.
Do we show generatedOutput during generation?
If yes, then maybe we can combine Generating and Success states into one? And then the generatedOutput string would have a single home.
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.
In this case it is fine to have them in both states, because one is partial and one is complete so it is conceivable that the text UI might look different, you would also want to show that the generation is still in progress with a loading indicator. You could name them something different to make that more clear, maybe just partialOutput in generating
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.
renamed to partialOutput here
|
|
||
| override fun onDownloadProgress(bytesDownloaded: Long) { | ||
| _uiState.update { | ||
| (it as? GenAIImageDescriptionUiState.DownloadingFeature)?.copy(bytesDownloaded = bytesDownloaded) ?: it |
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.
I wish I could test this somehow, but I don't think the download actually gets cancelled by the coroutine. That means this might still fire and mess with your UI state after you have set it back to initial from dismissing the bottom sheet.
The easiest way to fix is just to not support cancellation and remove the bottom sheet
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.
okay yeah i tested it, it gets really wonky if i hide the bottom sheet while it's still downloading.
removed the bottom sheet implementation
| ) : GenAIImageDescriptionUiState() | ||
|
|
||
| private var imageDescriber: ImageDescriber? = null | ||
| data class Generating(val generatedOutput: String) : GenAIImageDescriptionUiState() |
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.
In this case it is fine to have them in both states, because one is partial and one is complete so it is conceivable that the text UI might look different, you would also want to show that the generation is still in progress with a loading indicator. You could name them something different to make that more clear, maybe just partialOutput in generating
…b.com/android/ai-samples into refactor/genai-image-description-mvvm
|
thanks everybody for the review! addressed ben and kate's comments, so this is ready for another round of review |
bentrengrove
left a comment
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.
One minor question but LGTM
| fun clearResult() { | ||
| _uiState.value = GenAIImageDescriptionUiState.Initial | ||
| imageDescriptionJob?.cancel() | ||
| } |
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.
Do you still need this function? If you do, you still have the same bug
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.
ah yes, good call, removed
No description provided.