Add kdocs to ui/uistate/capture classes #465
Conversation
Summary of ChangesHello @davidjiagoogle, 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 focuses on improving the documentation of the UI state definitions within the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
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 adds comprehensive KDoc documentation to numerous UI state classes, which is a great improvement for code clarity and maintainability. The added comments are clear and accurate. I've suggested a couple of minor enhancements to improve the robustness of the data classes by adding validation logic that aligns with the expectations set in the new documentation.
...apture/src/main/java/com/google/jetpackcamera/ui/uistate/capture/CaptureModeToggleUiState.kt
Show resolved
Hide resolved
...te/capture/src/main/java/com/google/jetpackcamera/ui/uistate/capture/StabilizationUiState.kt
Show resolved
Hide resolved
ui/uistateadapter/src/main/java/com/example/uistateadapter/SnackBarUiStateAdapter.kt
Outdated
Show resolved
Hide resolved
...teadapter/src/main/java/com/google/jetpackcamera/ui/uistateadapter/SnackBarUiStateAdapter.kt
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request primarily focuses on improving code documentation by adding KDocs to numerous UI state classes in the ui/uistate/capture module. Additionally, it refactors SnackBarUiState and its adapter out of the capture-specific modules into more general ui/uistate and ui/uistateadapter modules. This is a good architectural change that decouples the postcapture feature from the capture feature. The new KDocs are clear and will significantly improve maintainability. I've found one issue with the package name of the moved adapter file, which should be corrected for consistency.
ui/uistateadapter/src/main/java/com/example/uistateadapter/SnackBarUiStateAdapter.kt
Outdated
Show resolved
Hide resolved
| * @property flipLensUiState The UI state for the flip lens (front/back camera) button. | ||
| * @property snackBarUiState The UI state for the snack bar, used for showing messages to the user. | ||
| * @property previewDisplayUiState The UI state for the preview display. | ||
| * @property lastBlinkTimeStamp The timestamp of the last preview blink animation. |
There was a problem hiding this comment.
| * @property lastBlinkTimeStamp The timestamp of the last preview blink animation. | |
| * @property lastBlinkTimeStamp The timestamp of the last image capture blink animation. |
| * @property snackBarUiState The UI state for the snack bar, used for showing messages to the user. | ||
| * @property previewDisplayUiState The UI state for the preview display. | ||
| * @property lastBlinkTimeStamp The timestamp of the last preview blink animation. | ||
| * @property externalCaptureMode The external capture mode, if any, that launched the camera. |
There was a problem hiding this comment.
| * @property externalCaptureMode The external capture mode, if any, that launched the camera. | |
| * @property externalCaptureMode The external capture mode used by the intent that launched the camera. Default is [ExternalCaptureMode.Standard]. |
| * @property videoQuality The currently selected video quality. | ||
| * @property audioUiState The UI state for audio recording. | ||
| * @property elapsedTimeUiState The UI state for the elapsed time display during video recording. | ||
| * @property captureButtonUiState The UI state for the main capture button. |
There was a problem hiding this comment.
main implies there is a secondary capture button, lets omit that word.
| * @property captureButtonUiState The UI state for the main capture button. | |
| * @property captureButtonUiState The UI state for the capture button. |
| * @property audioUiState The UI state for audio recording. | ||
| * @property elapsedTimeUiState The UI state for the elapsed time display during video recording. | ||
| * @property captureButtonUiState The UI state for the main capture button. | ||
| * @property imageWellUiState The UI state for the image well, which shows the last captured media. |
There was a problem hiding this comment.
| * @property imageWellUiState The UI state for the image well, which shows the last captured media. | |
| * @property imageWellUiState The UI state for the image well, which displays a thumbnail of a captured media. |
| * @param lastBlinkTimeStamp The timestamp of the last time the preview blinked. This is used to | ||
| * trigger the blink animation. |
There was a problem hiding this comment.
| * @param lastBlinkTimeStamp The timestamp of the last time the preview blinked. This is used to | |
| * trigger the blink animation. | |
| * @param lastBlinkTimeStamp The timestamp of the most recent capture blink animation. |
| * This sealed interface represents all possible states for the quick settings UI, which provides | ||
| * easy access to common camera settings like flash, aspect ratio, and more. |
There was a problem hiding this comment.
I think this can just be deleted.
| * This sealed interface represents all possible states for the quick settings UI, which provides | |
| * easy access to common camera settings like flash, aspect ratio, and more. |
| * This sealed interface represents the different states of the focus and metering UI, which | ||
| * includes tap-to-focus indicators and the general focus status. |
There was a problem hiding this comment.
| * This sealed interface represents the different states of the focus and metering UI, which | |
| * includes tap-to-focus indicators and the general focus status. | |
| * This sealed interface represents the different states of the focus and metering UI |
| ) : FocusMeteringUiState | ||
|
|
||
| /** | ||
| * Represents the status of a user-initiated focus and metering action. |
There was a problem hiding this comment.
I think this is a good area to point directly as to what these Status enums correspond to and where those values come from, since it is a bit nonobvious
| * Defines the UI state for the image well, which displays the most recently captured media. | ||
| * | ||
| * This sealed interface represents the possible states of the image well, such as being | ||
| * unavailable or displaying information about the last capture. |
There was a problem hiding this comment.
Like i mentioned before, the ImageWellUiState and its corresponding component aren't aware of any of the context to the media.
| * Defines the UI state for the image well, which displays the most recently captured media. | |
| * | |
| * This sealed interface represents the possible states of the image well, such as being | |
| * unavailable or displaying information about the last capture. | |
| * Defines the UI state for the image well, a component used to display the thumbnail of a captured media. |
3 participants
As titled
SnackBarUiState is moved from ui/uistate/capture to ui/uistate. SnackBarUiStateAdapter is moved to ui/uistateadapter. This is so that postcapture doesn't have to depend on capture