bottom sheet refactor - #569
Conversation
- remove flip camera, stream config, and concurrent camera from quick settings - all quick settings menu items adopt the button row ux - WIP unique selection of settings depending on current capture mode
auxiliary function for settings subtitles
… state - Re-added the 'More Settings' button to the Quick Settings bottom sheet. - Introduced a 'showMoreSettingsButton' boolean parameter to control its visibility, defaulting to true. - Removed unused 'focusedQuickSetting' state from 'QuickSettingsUiState' and 'TrackedCaptureUiState'. - Cleaned up 'FlashModeUiStateAdapter.kt' by removing a debug 'println' and adding a 'todo(kc)' for 'visibleFlashModes'.
Remove unused drawables and their corresponding enum classes in
QuickSettingsEnums that are no longer referenced after the quick
settings refactoring.
- 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.
…oogle/jetpack-camera-app into kim/hdr/decouple-capture-modes
…tings/button-rows
… state adapter tests - Refactored the Quick Settings bottom sheet UI to a flat layout, replacing nested navigation and scrollable containers with direct option rows. - Updated Instrumented tests (BackgroundDeviceTest, CaptureModeSettingsTest, ConcurrentCameraTest, NavigationTest, SwitchCameraTest) and helper functions in ComposeTestRuleExt.kt to interact with the flat layout. - Added LocalDisableAnimations composition local support in PreviewScreen.kt and QuickSettingsModalBottomSheet to disable animations for faster, more reliable testing. - Refactored FlashModeUiState.Unavailable and HdrUiState.Unavailable from classes to objects and updated tests in FlashModeUiStateAdapterTest.kt and HdrUiStateAdapterTest.kt. - Dynamically update FlipCameraButton's content description based on current lens facing.
…lBottomSheet Reverted QuickSettingsModalBottomSheet in QuickSettingsComponents.kt to directly use Material3's ModalBottomSheet, removing the custom non-gestural Box/Column layout previously used with LocalDisableAnimations. Cleaned up ComposeTestRuleExt.kt visitQuickSettings cleanup to rely on standard swipe-to-dismiss behavior and verification.
…tings/button-rows
…tings/button-rows
- Remove obsolete toast and disabled rationale strings from ui:components:capture. - Delete unused strings.xml in ui:controller:impl. - Retain ui:uistateadapter:capture as the single source of truth for toast and rationale strings.
…tings/button-rows
…tings/button-rows
…tings/button-rows
…ttingsBottomSheet
…tings/button-rows
# Conflicts: # app/src/androidTest/java/com/google/jetpackcamera/utils/ComposeTestRuleExt.kt
- Remove isQuickSettingsOpen and quickSettingsIsOpen from TrackedCaptureUiState, QuickSettingsUiState, and UI state adapters. - Remove toggleQuickSettings() from QuickSettingsController and its implementations. - Manage quick settings drawer visibility as local UI state in PreviewScreen via rememberSaveable. - Clean up unused quickSettingsIsOpen and toggleQuickSettings tests.
…osable - Make QuickSettingsContent internal and container-independent with modifier and showMoreSettingsButton parameters. - Flatten internal layout structure by removing intermediate QuickSettingsLayout helper. - Add additional top padding above the 'More settings' navigation button.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
6b1fec0 to
a4bf36f
Compare
Integrate Material 3 BottomSheetScaffold into CaptureLayout to render Quick Settings in-hierarchy and avoid window-spawning modal overlays. Make the drag handle pill clickable for dismiss, and update test helpers to close the sheet deterministically.
a4bf36f to
8544a5b
Compare
|
/gemini review |
![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 the Quick Settings panel to use BottomSheetScaffold instead of a custom overlay, moving the open/close state from the global UI state to local Compose state in PreviewScreen. Key feedback includes: ensuring the test helper checks if the drag handle is displayed before clicking it to prevent test failures; optimizing gesture synchronization by observing targetValue instead of isVisible; adding zoomStateManager to the captureButtonLambda remember keys to avoid stale references; remembering the onDismissQuickSettings lambda to prevent unnecessary recompositions; and extracting the hardcoded onClickLabel string to strings.xml to comply with localization guidelines.
| val dragHandleNodes = onAllNodesWithTag(QUICK_SETTINGS_DRAG_HANDLE).fetchSemanticsNodes() | ||
| if (dragHandleNodes.isNotEmpty()) { | ||
| onNodeWithTag(QUICK_SETTINGS_DRAG_HANDLE).performClick() | ||
| } else { | ||
| val openToggle = | ||
| onNodeWithContentDescription(CaptureR.string.quick_settings_toggle_open_description) | ||
| if (openToggle.isDisplayed()) { | ||
| openToggle.performClick() | ||
| } | ||
| } |
There was a problem hiding this comment.
Since BottomSheetScaffold keeps the bottom sheet in the composition tree even when hidden, onAllNodesWithTag(QUICK_SETTINGS_DRAG_HANDLE) will return a non-empty list even when the sheet is closed. Attempting to click the hidden drag handle will result in an AssertionError because the node is not displayed. We should check if the drag handle is actually displayed before attempting to click it.
| val dragHandleNodes = onAllNodesWithTag(QUICK_SETTINGS_DRAG_HANDLE).fetchSemanticsNodes() | |
| if (dragHandleNodes.isNotEmpty()) { | |
| onNodeWithTag(QUICK_SETTINGS_DRAG_HANDLE).performClick() | |
| } else { | |
| val openToggle = | |
| onNodeWithContentDescription(CaptureR.string.quick_settings_toggle_open_description) | |
| if (openToggle.isDisplayed()) { | |
| openToggle.performClick() | |
| } | |
| } | |
| val dragHandleNodes = onAllNodesWithTag(QUICK_SETTINGS_DRAG_HANDLE).fetchSemanticsNodes() | |
| if (dragHandleNodes.isNotEmpty() && onNodeWithTag(QUICK_SETTINGS_DRAG_HANDLE).isDisplayed()) { | |
| onNodeWithTag(QUICK_SETTINGS_DRAG_HANDLE).performClick() | |
| } else { | |
| val openToggle = | |
| onNodeWithContentDescription(CaptureR.string.quick_settings_toggle_open_description) | |
| if (openToggle.isDisplayed()) { | |
| openToggle.performClick() | |
| } | |
| } |
| LaunchedEffect(scaffoldState.bottomSheetState.isVisible) { | ||
| if (!scaffoldState.bottomSheetState.isVisible && isQuickSettingsOpen) { | ||
| isQuickSettingsOpen = false | ||
| } | ||
| } |
There was a problem hiding this comment.
Observing isVisible for gesture synchronization means isQuickSettingsOpen will only update to false after the collapse animation fully completes. This can make the UI (such as the quick settings toggle button) feel laggy or out of sync. Observing targetValue == SheetValue.Hidden allows us to immediately update the state as soon as the swipe-down gesture starts, making the UI much more responsive.
| LaunchedEffect(scaffoldState.bottomSheetState.isVisible) { | |
| if (!scaffoldState.bottomSheetState.isVisible && isQuickSettingsOpen) { | |
| isQuickSettingsOpen = false | |
| } | |
| } | |
| LaunchedEffect(scaffoldState.bottomSheetState.targetValue) { | |
| if (scaffoldState.bottomSheetState.targetValue == SheetValue.Hidden && isQuickSettingsOpen) { | |
| isQuickSettingsOpen = false | |
| } | |
| } |
| val captureButtonLambda = remember( | ||
| captureButtonState, | ||
| quickSettingsState, | ||
| quickSettingsController, | ||
| captureController | ||
| ) { |
There was a problem hiding this comment.
The captureButtonLambda captures zoomStateManager but does not include it in its remember keys. If zoomStateManager is recreated (e.g., when the lens changes or during configuration changes), the lambda will hold a stale reference to the old manager. Adding zoomStateManager to the keys ensures correctness.
val captureButtonLambda = remember(
captureButtonState,
captureController,
zoomStateManager
) {| LayoutWrapper( | ||
| modifier = modifier, | ||
| scaffoldState = scaffoldState, | ||
| onDismissQuickSettings = { isQuickSettingsOpen = false }, |
There was a problem hiding this comment.
The onDismissQuickSettings lambda is currently recreated on every recomposition of ContentScreen, which can cause LayoutWrapper and PreviewLayout to unnecessarily recompose. Wrapping it in a remember block ensures a stable reference and improves performance.
| LayoutWrapper( | |
| modifier = modifier, | |
| scaffoldState = scaffoldState, | |
| onDismissQuickSettings = { isQuickSettingsOpen = false }, | |
| val onDismissQuickSettings = remember { { isQuickSettingsOpen = false } } | |
| LayoutWrapper( | |
| modifier = modifier, | |
| scaffoldState = scaffoldState, | |
| onDismissQuickSettings = onDismissQuickSettings, |
| .clickable( | ||
| onClickLabel = "Close quick settings", | ||
| onClick = onDismissQuickSettings | ||
| ) |
There was a problem hiding this comment.
The accessibility onClickLabel is a user-facing string and should not be hardcoded to "Close quick settings". According to the repository style guide, all user-facing strings must be extracted to strings.xml to support localization.
| .clickable( | |
| onClickLabel = "Close quick settings", | |
| onClick = onDismissQuickSettings | |
| ) | |
| .clickable( | |
| onClickLabel = stringResource(R.string.quick_settings_close_description), | |
| onClick = onDismissQuickSettings | |
| ) |
References
- Forbid hardcoded user-facing strings in composables. All text should be extracted into strings.xml to support localization and make updates easier. (link)
2 participants
Summary
Refactors Quick Settings to use Material 3's in-hierarchy
BottomSheetScaffoldinstead ofModalBottomSheet.Why the Transition from
ModalBottomSheettoBottomSheetScaffold?BottomSheetScaffoldrenders the sheet within the primary Compose layout tree, maintaining a single window lifecycle and preserving seamless viewfinder rendering.Key Changes
ScaffoldwithBottomSheetScaffoldinCaptureLayout.kt(sheetPeekHeight = 0.dp), unifyingSnackbarHostand removing nested sheet wrappers.BottomSheetDefaults.DragHandlewithonDismissQuickSettingsclick action andQUICK_SETTINGS_DRAG_HANDLEtest tag.isQuickSettingsOpeninPreviewScreen.ktwithLaunchedEffectsync andBackHandler(enabled = isQuickSettingsOpen).closeQuickSettings()and updatedvisitQuickSettingsinComposeTestRuleExt.ktto click the drag handle pill, eliminating flaky synthetic gesture timeouts.Lifecycle Behavior & Trade-offs
ModalBottomSheet(which dynamically mounts/unmounts from composition upon opening/closing),BottomSheetScaffoldis a persistent layout container. When hidden (sheetPeekHeight = 0.dp), the sheet container remains in the composition tree and is translated off-screen rather than decomposed. Automated tests therefore check visibility (.isNotDisplayed()) rather than complete semantics node absence.onDismissRequestmodal API for bidirectionalLaunchedEffectsynchronization between the boolean UI state (isQuickSettingsOpen) andSheetState..clickablemodifier was explicitly attached to provide a deterministic tap-to-dismiss target for accessibility and test automation.