-
Notifications
You must be signed in to change notification settings - Fork 51
Add isCameraRunning state #425
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
base: main
Are you sure you want to change the base?
Conversation
Introduces an `isCameraRunning` flag to the `CameraState` to accurately reflect the underlying camera's status. This state is derived from the CameraX `CameraState` and is used to disable the capture button when the camera is not open or available. This commit also renames `torchEnabled` to `isTorchEnabled` for clarity and consistency, and ensures the `ZoomState` is being correctly updated in the `CameraState`. The capture button's UI has been improved to prevent flickering during brief transitional states (e.g. switching cameras). This is achieved by visually debouncing the disabled state. While the button's semantics and pointer input are disabled immediately, the visual change to a disabled appearance is delayed by one second. If the button becomes enabled again within this period, the distracting flicker is avoided. Color transitions are animated for a smoother user experience. Finally, the test suite has been made more robust by updating the tests to wait for the capture button to be both displayed and enabled before interaction.
Summary of ChangesHello @temcguir, 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 improves the camera's state management and user interface responsiveness. It introduces a robust "isCameraRunning" flag to precisely track the camera's active status, which is then used to intelligently control the capture button. A key enhancement is the visual debouncing of the capture button's disabled state, eliminating distracting flickers during rapid camera transitions. Additionally, it includes a minor refactoring for naming consistency and strengthens the test suite for better reliability. 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
|
Summary of ChangesHello @temcguir, 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 refines the camera's state management by introducing a dedicated "isCameraRunning" flag, which is crucial for accurately reflecting the camera's live status. This new state is then utilized to enhance the user experience of the capture button, preventing distracting visual flickers during camera transitions through a debouncing mechanism. Additionally, the PR includes a minor renaming for clarity and strengthens the existing test suite to ensure reliable interactions with the updated UI components. 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
|
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 is well-structured and effectively introduces the isCameraRunning state to manage the capture button's availability. The UI enhancement to prevent flickering using a visual debounce is a thoughtful improvement, and the corresponding tests have been made more robust. The code is clean and follows best practices. I have one suggestion to enhance maintainability by extracting a piece of UI logic into a reusable composable function, in line with the project's style guide.
...ture/src/main/java/com/google/jetpackcamera/ui/components/capture/CaptureButtonComponents.kt
Outdated
Show resolved
Hide resolved
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 introduces some great improvements. The new isCameraRunning state provides a more accurate way to manage the capture button's availability, and the visual debouncing to prevent flickering is a thoughtful UX enhancement. Renaming torchEnabled to isTorchEnabled improves clarity and consistency across the codebase. The test suite is also made more robust by waiting for the capture button to be enabled, which will help prevent flaky tests.
I've left one suggestion in CaptureButtonComponents.kt to simplify the debouncing logic using snapshotFlow and collectLatest, which should make it more readable and declarative. Overall, this is a high-quality contribution.
...ture/src/main/java/com/google/jetpackcamera/ui/components/capture/CaptureButtonComponents.kt
Outdated
Show resolved
Hide resolved
|
this change looks good, though I notice a new visual "jaggedness" for the capture button's center when switching between capture modes. |
|
/gemini review |
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 introduces a new CaptureButtonUiState.Unavailable state for the capture button, which is activated when the camera is not running. The UI now visually disables the capture button with a debounced fade animation and programmatically disables it via semantics, preventing user interaction. To accommodate this change in testing, a new waitForCaptureButton utility function was added to ComposeTestRuleExt.kt, which ensures tests wait for the capture button to be both displayed and enabled. This utility function was then integrated across various device tests, including BackgroundDeviceTest, ImageCaptureDeviceTest, NavigationTest, PermissionsTest, and VideoRecordingDeviceTest, replacing previous direct waitUntil calls.
Introduces an
isCameraRunningflag to theCameraStatetoaccurately reflect the underlying camera's status. This state is
derived from the CameraX
CameraStateand is used to disable thecapture button when the camera is not open or available.
This commit also renames
torchEnabledtoisTorchEnabledforclarity and consistency, and ensures the
ZoomStateis beingcorrectly updated in the
CameraState.The capture button's UI has been improved to prevent flickering
during brief transitional states (e.g. switching cameras). This
is achieved by visually debouncing the disabled state.
While the button's semantics and pointer input are disabled
immediately, the visual change to a disabled appearance is
delayed by one second. If the button becomes enabled again
within this period, the distracting flicker is avoided. Color
transitions are animated for a smoother user experience.
Finally, the test suite has been made more robust by updating
the tests to wait for the capture button to be both displayed
and enabled before interaction.