Decouple HDR quick setting controls from capture modes#528
Conversation
- Decoupled dynamic range (video HDR) from image format (image HDR) settings across UI, controller, and CameraX configuration layers.
- Removed dynamic range constraints from the createImageUseCase configuration in CameraSession.kt, enabling independent Ultra HDR
image capture.
- Updated QuickSettings bottom sheet click handlers to mutate only the HDR setting relevant to the active capture mode.
- Enforced specialized Low Light Boost vs Ultra HDR conflicts in CameraXCameraSystem.kt, prioritizing Low Light Boost.
- Created HdrUiStateAdapterTest.kt covering all HDR availability states and flash conflicts.
- Refactored CameraXCameraSystemTest.kt to run parameterized HDR decoupling tests on both front and rear lenses.
![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 refactors HDR and image format constraint handling when switching capture modes, ensuring HDR settings are preserved or disabled appropriately. It also adds comprehensive instrumented and unit tests to verify these behaviors. The review feedback suggests simplifying a when block in QuickSettingsScreen.kt, enforcing concurrent camera mode constraints when changing capture modes in CameraXCameraSystem.kt, and restricting the visibility of the new test class HdrUiStateAdapterTest to internal.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| when (captureMode) { | ||
| CaptureMode.IMAGE_ONLY -> { | ||
| quickSettingsController.setImageFormat(i) | ||
| } | ||
| CaptureMode.VIDEO_ONLY -> { | ||
| quickSettingsController.setDynamicRange(d) | ||
| } | ||
| CaptureMode.STANDARD -> { | ||
| quickSettingsController.setDynamicRange(d) | ||
| quickSettingsController.setImageFormat(i) | ||
| } | ||
| null -> { | ||
| quickSettingsController.setDynamicRange(d) | ||
| quickSettingsController.setImageFormat(i) | ||
| } | ||
| } |
There was a problem hiding this comment.
The when block can be simplified by combining the redundant branches for CaptureMode.STANDARD and null into an else branch, and using single-line expressions for the other branches. This improves readability and aligns with the repository style guide's directive to simplify complex logic.
when (captureMode) {
CaptureMode.IMAGE_ONLY -> quickSettingsController.setImageFormat(i)
CaptureMode.VIDEO_ONLY -> quickSettingsController.setDynamicRange(d)
else -> {
quickSettingsController.setDynamicRange(d)
quickSettingsController.setImageFormat(i)
}
}References
- Simplify Complex Logic: Look for needlessly complex code. If a multi-line block of logic can be condensed into a more concise and readable idiomatic expression (e.g., using Kotlin standard library functions), suggest the simplification. (link)
| override suspend fun setCaptureMode(captureMode: CaptureMode) { | ||
| currentSettings.update { old -> | ||
| old?.copy(captureMode = captureMode) | ||
| ?.tryApplyDynamicRangeConstraints() | ||
| ?.tryApplyImageFormatConstraints() | ||
| } | ||
| } |
There was a problem hiding this comment.
When switching the capture mode to STANDARD or IMAGE_ONLY, if concurrentCameraMode is currently DUAL, it should be reset to OFF because concurrent camera mode is only supported in VIDEO_ONLY mode. Calling tryApplyConcurrentCameraModeConstraints() here ensures that this constraint is properly enforced when the capture mode changes.
Note: You should also update tryApplyConcurrentCameraModeConstraints() to check captureMode == CaptureMode.VIDEO_ONLY to fully resolve this constraint.
| override suspend fun setCaptureMode(captureMode: CaptureMode) { | |
| currentSettings.update { old -> | |
| old?.copy(captureMode = captureMode) | |
| ?.tryApplyDynamicRangeConstraints() | |
| ?.tryApplyImageFormatConstraints() | |
| } | |
| } | |
| override suspend fun setCaptureMode(captureMode: CaptureMode) { | |
| currentSettings.update { old -> | |
| old?.copy(captureMode = captureMode) | |
| ?.tryApplyDynamicRangeConstraints() | |
| ?.tryApplyImageFormatConstraints() | |
| ?.tryApplyConcurrentCameraModeConstraints() | |
| } | |
| } |
References
- Architectural Consistency: Does the new code align with the existing MVVM architecture? Are ViewModels, Repositories, and UI components used correctly? Does it introduce any anti-patterns or deviate from established conventions in the codebase? (link)
| import org.junit.runners.JUnit4 | ||
|
|
||
| @RunWith(JUnit4::class) | ||
| class HdrUiStateAdapterTest { |
There was a problem hiding this comment.
According to the repository style guide, all new classes should use the most restrictive visibility modifier possible. Since HdrUiStateAdapterTest is a test class and does not need to be accessed from outside its module, it should be declared as internal.
| class HdrUiStateAdapterTest { | |
| internal class HdrUiStateAdapterTest { |
References
- Proper Visibility Modifiers: Ensure all new functions, properties, and classes use the most restrictive visibility modifier possible (e.g., private, internal) while still allowing necessary access. Avoid public visibility unless explicitly required for external API exposure. (link)
1 participant
This PR decouples HDR quick settings (HDR Video / HLG10 and HDR Image / Ultra HDR) from automatic capture mode transitions. Instead of forcing capture mode changes when toggling HDR, HDR availability is now constrained by the active capture mode:
HDRVideo (HLG10) is restricted toVIDEO_ONLYmode.HDRImage (Ultra HDR) is restricted toIMAGE_ONLYmode.STANDARD(hybrid) Mode (both image and video bound) is restricted to SDR/JPEGThis PR does not modify the dedicated settings screen
Benign Behavioral Changes
HDRinSTANDARDMode: HDR is now completely disabled in STANDARD mode (forces SDR/JPEG)binding and improve stability.