Repository navigation
Conversation
Before capture sub-modes, the settings screen disabled Low Light Boost, stream configuration, stabilization, and frame rate while dual camera was enabled. Those checks read a persisted setting, so they can't detect a session-scoped sub-mode. - Add a cameraEffect field to CameraFeaturePolicy, PolicyOverrides, and policy-default persistence, so a sub-mode policy can lock the camera effect like the other settings. - Expose the active capture sub-mode from ConstraintsRepository. - In SettingsViewModel, disable settings that the active sub-mode's policy hides and options that it excludes, with a rationale naming the sub-mode. Options already disabled for another reason keep that reason. - Add tests for policy compatibility and intersection of stabilization, frame rate, and camera effect, and for restoring frame rate and camera effect when a sub-mode ends.
- When the user changes a policy-controlled setting while a sub-mode is active, PolicyOverrides now records the new value as the one to restore, so leaving the sub-mode keeps the user's latest choice instead of the value from before the sub-mode. This applies whether or not the policy permits the choice. - Check frame rate and stabilization against the lens only when the active policy sets them, or when they are restored, so a sub-mode that doesn't restrict them handles them the same way as outside a sub-mode. - Move the active capture sub-mode out of ConstraintsRepository into a new ActiveCaptureSubModeRepository, so existing ConstraintsRepository implementations in other apps keep compiling. Share it eagerly, seeded from the current settings, so its value is never a stale default. - Fix the flash setting's disabled description, which formatted the new sub-mode rationale with too few arguments. - Only disable the camera effect setting for a sub-mode when the sub-mode removed an effect that would otherwise be offered. A sub-mode policy that hides a whole setting shows the sub-mode as the reason even if the setting is also disabled for another reason. Only options excluded individually keep their existing reason.
Covers changing settings in one sub-mode, switching directly to another, and returning to the default sub-mode: permitted choices apply immediately, blocked choices apply once a sub-mode that allows them is active, and the latest choices apply after the last sub-mode ends.
setImageFormatAndDynamicRange_inCaptureSubMode_keepsPolicyValues changes the target frame rate to 60 while the sub-mode is active. Choices made during a sub-mode now replace the values restored when it ends, so the test now expects 60 fps (if the lens supports it) after leaving the sub-mode, and stabilization ON only if the lens supports it at that frame rate.
A value requested while a sub-mode was active was lost when the policy had kept the entry value and then replaced the request with its default (for example, an Only policy that permits the entry value): on exit, the setting no longer matched the value recorded on entry, so the policy's default was kept. PolicyOverrides now marks requested values and always restores them. - Derive ActiveCaptureSubModeRepository.activeCaptureSubModeId from the current settings on each collection instead of sharing it eagerly in a scope that outlives the repository. The property is now a Flow; each collector receives the current value first. - Document on CameraSystem.setCaptureSubMode that values requested during a sub-mode are restored when it ends, and update the related KDoc. - Add tests for the Only-policy case, for setFlashMode during a sub-mode, and for the flow emitting changes to a collector.
Java callers construct CameraFeaturePolicy with the original five parameters. Generate overloads so they keep compiling now that the policy has stabilization, frame rate, and camera effect parameters.
…e-policy-video-settings
…e-policy-video-settings # Conflicts: # feature/settings/src/main/java/com/google/jetpackcamera/settings/SettingsViewModel.kt # feature/settings/src/test/java/com/google/jetpackcamera/settings/CameraAppSettingsViewModelTest.kt
![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 introduces support for enforcing and restoring camera feature policies—specifically stabilization mode, target frame rate, and camera effects—when entering and exiting capture sub-modes. It adds the ActiveCaptureSubModeRepository to track the active sub-mode, updates the settings data sources to handle policy defaults and user preferences, and refactors the SettingsViewModel to restrict UI options (FPS, stabilization, flash, and effects) with descriptive rationales when a sub-mode is active. Comprehensive unit and instrumented tests have been added to verify these behaviors. The review feedback suggests simplifying an if-else block in SettingsMappers.kt using Kotlin's idiomatic takeIf and safe call operators to improve readability and align with codebase patterns.
…e-policy-video-settings
Covers a sub-mode that locks the frame rate and turns off the camera effect staying in force when the lens is flipped, with the earlier values restored on exit. Also covers a restored image capture effect taking precedence over Ultra HDR turned on during the sub-mode.
…e-policy-video-settings
…e-policy-video-settings
…tings - Add videoQuality to CameraFeaturePolicy and PolicyOverrides, so a capture sub-mode can lock it and the previous value is restored on exit. - Enforce the active sub-mode policy when the video quality is changed. - Settings disables video quality and aspect ratio options that the active sub-mode policy does not permit, with the sub-mode as the reason. - Disabled single-choice options show the reason they are disabled, so it is also read by accessibility services.
…e-policy-video-settings # Conflicts: # core/camera/src/androidTest/java/com/google/jetpackcamera/core/camera/CameraXCameraSystemTest.kt # core/camera/src/main/java/com/google/jetpackcamera/core/camera/CameraXCameraSystem.kt
…r' into temcguir/feature-policy-video-settings
Remove tests whose behavior is covered by other tests, merge per-field model tests that share the same setup, and use one view model helper for sub-mode policy tests. Each original assertion is kept.
Add tests for the stabilization mode, image format, and dynamic range setters, for every setting recorded by PolicyOverrides.withRequestedChange, and for the effect setting when a sub-mode permits none of the supported effects.
…e-policy-video-settings
…e-policy-video-settings # Conflicts: # feature/settings/src/main/res/values/strings.xml
…e-policy-video-settings # Conflicts: # core/settings/datastore-prefs/src/androidTest/java/com/google/jetpackcamera/core/settings/datastoreprefs/PrefsDataStoreSettingsDataSourceInstrumentedTest.kt # core/settings/datastore-prefs/src/main/java/com/google/jetpackcamera/core/settings/datastoreprefs/PrefsDataStoreSettingsDataSource.kt # feature/settings/src/test/java/com/google/jetpackcamera/settings/CameraAppSettingsViewModelTest.kt
…e-policy-video-settings
| override fun setFlashMode(flashMode: FlashMode) { | ||
| currentSettings.update { old -> | ||
| old?.copy(flashMode = flashMode) | ||
| old?.withUserChange { copy(flashMode = flashMode) } | ||
| ?.tryApplyDynamicRangeConstraints() | ||
| ?.tryApplyImageFormatConstraints() | ||
| ?.tryApplyConcurrentCameraModeConstraints() | ||
| ?.tryApplyCaptureSubModeConstraints() | ||
| } | ||
| } |
There was a problem hiding this comment.
In both setFlashMode and setCameraEffect (line 1424), cross-setting constraints (tryApplyDynamicRangeConstraints, tryApplyImageFormatConstraints, tryApplyCaptureModeConstraints) run before tryApplyCaptureSubModeConstraints().
If an active sub-mode locks flashMode = OFF or cameraEffect = NONE_EFFECT_ID while allowing HLG10 or JPEG_ULTRA_HDR, a call to setFlashMode(LOW_LIGHT_BOOST) or setCameraEffect(...) will downgrade HDR to SDR/JPEG (or flip captureMode and exit the sub-mode) before tryApplyCaptureSubModeConstraints() clamps the blocked change—and because current != enforced afterward, PolicyOverrides.restore() won't restore HDR on exit either. Running tryApplyCaptureSubModeConstraints() first prevents blocked changes from mutating dependent settings.
| val enforced = policy.enforceRestrictions(settings) | ||
| .copy(activeCaptureSubModeId = resolvedSubModeId) | ||
| .tryApplyDynamicRangeConstraints() | ||
| .tryApplyImageFormatConstraints() | ||
| .tryApplyFlashModeConstraints() | ||
| .tryApplyVideoQualityConstraints() | ||
| // Only re-check frame rate and stabilization if the policy restricts them. | ||
| .let { if (policy.targetFrameRate != null) it.tryApplyFrameRateConstraints() else it } | ||
| .let { | ||
| if (policy.targetFrameRate != null || policy.stabilizationMode != null) { | ||
| it.tryApplyStabilizationConstraints() | ||
| } else { | ||
| it | ||
| } | ||
| } |
There was a problem hiding this comment.
Two ordering issues when resolving sub-mode constraints:
-
Direct
SubModeA -> SubModeBtransitions: When switching directly between two sub-modes (isContinuingSameProvider == false),restoreCaptureSubModeOverrides()(line 1258) restores pre-SubModeAvalues and immediately runsDEFAULT-mode constraints beforeSubModeB's policy is enforced. For example, if both sub-modes locktargetFrameRate = 30(overriding60fps) and the user enablesStabilizationMode.ONinSubModeA, switching toSubModeBtemporarily restores60fps and resets stabilization toAUTObeforeSubModeBclamps FPS back to30(same forselectedCameraEffectvsJPEG_ULTRA_HDR). We should do rawoverrides.restore(...)forbaseSettings, only call fullrestoreCaptureSubModeOverrides()whenresolvedProvider == null, and inenforcedbelow also re-check FPS/stabilization if restoration changed them (it.targetFrameRate != targetFrameRate). -
tryApplyFlashModeConstraints()order: Both here (lines 1292–1294) and inrestoreCaptureSubModeOverrides()(lines 1325–1327),tryApplyFlashModeConstraints()should run beforetryApplyDynamicRangeConstraints()andtryApplyImageFormatConstraints()so an unsupportedLOW_LIGHT_BOOSTon the target lens is reset toOFFbefore downgrading HDR.
| is CameraEffectUiState.Enabled -> { | ||
| val permittedEffects = supportedEffects.filterTo(mutableSetOf()) { | ||
| config.permits(it) | ||
| } | ||
| if (permittedEffects.isEmpty() && supportedEffects.isNotEmpty()) { | ||
| CameraEffectUiState.Disabled(rationale) | ||
| } else { | ||
| copy(supportedEffects = permittedEffects) | ||
| } | ||
| } |
There was a problem hiding this comment.
lower priority: supportedEffects only contains registered effect IDs (not NONE_EFFECT_ID), whereas JcaEffectsSetting renders "Multi-Stream Capture" (NONE_EFFECT_ID) plus supportedEffects. If a policy uses OptionVisibility.Only that excludes NONE_EFFECT_ID and permits just 1 effect, only 1 total option is valid, so this should return CameraEffectUiState.Disabled(rationale):
| is CameraEffectUiState.Enabled -> { | |
| val permittedEffects = supportedEffects.filterTo(mutableSetOf()) { | |
| config.permits(it) | |
| } | |
| if (permittedEffects.isEmpty() && supportedEffects.isNotEmpty()) { | |
| CameraEffectUiState.Disabled(rationale) | |
| } else { | |
| copy(supportedEffects = permittedEffects) | |
| } | |
| } | |
| is CameraEffectUiState.Enabled -> { | |
| val permittedEffects = supportedEffects.filterTo(mutableSetOf()) { | |
| config.permits(it) | |
| } | |
| val nonePermitted = | |
| config.permits(com.google.jetpackcamera.model.NONE_EFFECT_ID) | |
| val totalPermittedOptions = | |
| permittedEffects.size + if (nonePermitted) 1 else 0 | |
| if (totalPermittedOptions <= 1 && supportedEffects.isNotEmpty()) { | |
| CameraEffectUiState.Disabled(rationale) | |
| } else { | |
| copy(supportedEffects = permittedEffects) | |
| } | |
| } |
…e-policy-video-settings
…e-policy-video-settings
…e-policy-video-settings # Conflicts: # core/camera/src/androidTest/java/com/google/jetpackcamera/core/camera/CameraXCameraSystemTest.kt # core/settings/datastore-proto/src/test/java/com/google/jetpackcamera/settings/ProtoDataStoreSettingsDataSourceTest.kt # core/settings/settings-proto/src/main/java/com/google/jetpackcamera/settings/CameraAppSettingsProtoDefaults.kt # core/settings/settings-proto/src/main/java/com/google/jetpackcamera/settings/SettingsMappers.kt # core/settings/src/main/java/com/google/jetpackcamera/settings/model/CameraFeaturePolicy.kt # core/settings/src/main/java/com/google/jetpackcamera/settings/model/PolicyOverrides.kt # core/settings/src/test/java/com/google/jetpackcamera/settings/model/CameraFeaturePolicyTest.kt # core/settings/src/test/java/com/google/jetpackcamera/settings/model/PolicyOverridesTest.kt # feature/settings/src/main/java/com/google/jetpackcamera/settings/SettingsViewModel.kt # feature/settings/src/test/java/com/google/jetpackcamera/settings/CameraAppSettingsViewModelTest.kt
…nt settings - While a capture sub-mode is active, apply a user change that its policy doesn't permit only to the values restored when the sub-mode ends, so that it can't reset dependent settings (for example, a blocked camera effect or Low Light Boost turning off Ultra HDR). - When switching directly from one capture sub-mode to another, check the restored values against the current lens only after the next sub-mode's policy applies, so that values both sub-modes permit are kept. - Check the flash mode before the dynamic range and image format, which depend on it, when a sub-mode starts or ends. - Disable the camera effect setting when a sub-mode's policy leaves only one selectable option, counting the "no effect" option.
Part of the capture sub-mode stack: #598 ← #599 ← this PR ← #608 ← #609.
Architecture
Amber hexagon: changed in this PR. Blue box: unchanged. Purple: the
CaptureSubModeProviderinterface. Dashed arrow: implements. A module labeled with a PR number is added in that PR.Summary
Extends
CameraFeaturePolicyso a capture sub-mode can control stabilization, target frame rate, the camera effect, and video quality, and keeps the user's choices across sub-modes. The Settings screen disables settings that the active sub-mode restricts.Changes
CameraFeaturePolicygainsstabilizationMode,targetFrameRate,cameraEffect, andvideoQuality. They are covered bytoCameraAppSettings,enforceRestrictions,isCompatibleWith, andintersect.PolicyOverridesrecords and restores the four new fields.ActiveCaptureSubModeRepository(in:data:settings). It exposes the live sub-mode as aFlow<CaptureSubModeId>and is implemented byCameraXCameraSystemRepository.ConstraintsRepositoryis unchanged.SettingsViewModeldisables or filters settings that the active sub-mode restricts, with the rationale "%1$s is unsupported in %2$s mode":Onlypolicy disables the excluded options.FlashModeSettingformats its disabled rationale with all arguments.CameraFeaturePolicyuses@JvmOverloads, so Java callers that pass the original five parameters keep compiling.Persistence change (upgrade impact)
camera_app_settings.protonow declarestarget_frame_rateasoptional, so the data source can tell "unset" apart from "auto" (0).Testing
CameraFeaturePolicyTest,PolicyOverridesTest(requested values, including anOnlypolicy that replaces a permitted entry value),CameraAppSettingsViewModelTest(restriction overlays including video quality and aspect ratio, rationale precedence, Compose rationales for a disabled setting and a disabled option),CameraXCameraSystemRepositoryTest,ProtoDataStoreSettingsDataSourceTest,LocalSettingsRepositoryTest.CameraXCameraSystemTest: restore of effect and frame rate; requested aspect ratio, flash, and frame rate; switching sub-mode to sub-mode with user changes; a lens flip while a sub-mode locks frame rate and effect; a restored image capture effect taking precedence over Ultra HDR turned on during the sub-mode; a video quality chosen during the sub-mode restored on exit.PrefsDataStoreSettingsDataSourceInstrumentedTest.