Repository navigation
Conversation
![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 camera capture screen layout by decomposing the monolithic CaptureLayout.kt into modular, posture-aware layouts: CompactPortraitLayout, CompactShortLayout, and SideRailCaptureLayouts (covering foldables and tablets), along with comprehensive unit tests. The feedback focuses on restricting the visibility of LocalActiveCaptureLayout to internal, adding missing KDoc documentation for several new non-private components, and optimizing the layout tree by removing redundant wrapping Box nodes. Additionally, there are recommendations to address potential layout lag from dynamic rail measurements and to improve accessibility by ensuring consistent screen reader traversal indices and correctly consuming accessibility clicks on the Quick Settings card.
| val LocalActiveCaptureLayout: ProvidableCompositionLocal<String> = | ||
| compositionLocalOf { "CompactPortraitLayout" } |
There was a problem hiding this comment.
The LocalActiveCaptureLayout CompositionLocal is currently public. To adhere to the repository style guide, it should use the most restrictive visibility modifier possible (internal in this case) since it is only used within the capture UI module.
| val LocalActiveCaptureLayout: ProvidableCompositionLocal<String> = | |
| compositionLocalOf { "CompactPortraitLayout" } | |
| internal val LocalActiveCaptureLayout: ProvidableCompositionLocal<String> = | |
| compositionLocalOf { "CompactPortraitLayout" } |
References
- 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)
| val LocalActiveCaptureLayout: ProvidableCompositionLocal<String> = | ||
| compositionLocalOf { "CompactPortraitLayout" } | ||
|
|
||
| internal object CompactPortraitTokens { |
There was a problem hiding this comment.
Please add KDoc documentation for the CompactPortraitTokens object to adhere to the repository style guide, which requires all non-private members to be documented.
References
- All non-private classes, functions, and composables must have KDoc documentation. (link)
| internal const val QUICK_SETTINGS_OVERLAY_CARD_PLACEHOLDER_TAG = "card_quick_settings_overlay" | ||
|
|
||
| @Immutable | ||
| internal data class SideRailPostureConfig( |
There was a problem hiding this comment.
Please add KDoc documentation for the SideRailPostureConfig data class to adhere to the repository style guide, which requires all non-private members to be documented.
References
- All non-private classes, functions, and composables must have KDoc documentation. (link)
| val clearRailsForQuickSettings: Boolean = true | ||
| ) | ||
|
|
||
| internal object SideRailTokens { |
There was a problem hiding this comment.
Please add KDoc documentation for the SideRailTokens object to adhere to the repository style guide, which requires all non-private members to be documented.
References
- All non-private classes, functions, and composables must have KDoc documentation. (link)
| var startRailWidthPx by remember(seededRailWidthPx) { mutableIntStateOf(seededRailWidthPx) } | ||
| var endRailWidthPx by remember(seededRailWidthPx) { mutableIntStateOf(seededRailWidthPx) } |
There was a problem hiding this comment.
Using onSizeChanged to dynamically measure the rail widths and feed them back into state (startRailWidthPx and endRailWidthPx) causes a 1-frame layout lag and potential visual jitter/flicker when the screen is first displayed or rotated. Since the rail widths are predictable (MinSideRailWidth + config.railHorizontalPadding * 2), consider calculating the padding directly in Dp during composition, or implement a custom single-pass Layout for the scaffold to measure and position the rails and Quick Settings card in one go.
| Box( | ||
| modifier = Modifier | ||
| .align(Alignment.CenterStart) | ||
| .fillMaxHeight() | ||
| .onSizeChanged { startRailWidthPx = it.width } | ||
| .suppressBackgroundForModal(isModalOpen) | ||
| ) { | ||
| CompositionLocalProvider( | ||
| LocalLayoutDirection provides localeDirection | ||
| ) { | ||
| StartSideRail( | ||
| slots = slots, | ||
| modifier = Modifier.fillMaxHeight(), | ||
| isCompactHeight = config.isCompactHeight, | ||
| topPadding = config.railTopPadding, | ||
| bottomPadding = config.railBottomPadding, | ||
| horizontalPadding = config.railHorizontalPadding, | ||
| minSectionGap = config.railMinSectionGap, | ||
| centerVerticalFraction = config.centerVerticalFraction | ||
| ) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The wrapping Box layout node is redundant here. We can remove it and apply the alignment, size, and modal suppression modifiers directly to StartSideRail to optimize the layout tree and reduce nesting overhead.
CompositionLocalProvider(
LocalLayoutDirection provides localeDirection
) {
StartSideRail(
slots = slots,
modifier = Modifier
.align(Alignment.CenterStart)
.fillMaxHeight()
.onSizeChanged { startRailWidthPx = it.width }
.suppressBackgroundForModal(isModalOpen),
isCompactHeight = config.isCompactHeight,
topPadding = config.railTopPadding,
bottomPadding = config.railBottomPadding,
horizontalPadding = config.railHorizontalPadding,
minSectionGap = config.railMinSectionGap,
centerVerticalFraction = config.centerVerticalFraction
)
}| modifier = Modifier | ||
| .align(Alignment.CenterEnd) | ||
| .fillMaxHeight() | ||
| .onSizeChanged { endRailWidthPx = it.width } | ||
| .suppressBackgroundForModal(isModalOpen) | ||
| ) { | ||
| CompositionLocalProvider( | ||
| LocalLayoutDirection provides localeDirection | ||
| ) { | ||
| EndSideRail( | ||
| slots = slots, | ||
| modifier = Modifier.fillMaxHeight(), | ||
| isCompactHeight = config.isCompactHeight, | ||
| showElapsedTimeInRail = false, | ||
| topPadding = config.railTopPadding, | ||
| bottomPadding = config.railBottomPadding, | ||
| horizontalPadding = config.railHorizontalPadding, | ||
| minSectionGap = config.railMinSectionGap, | ||
| centerVerticalFraction = config.centerVerticalFraction | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The wrapping Box layout node is redundant here. We can remove it and apply the alignment, size, and modal suppression modifiers directly to EndSideRail to optimize the layout tree and reduce nesting overhead.
CompositionLocalProvider(
LocalLayoutDirection provides localeDirection
) {
EndSideRail(
slots = slots,
modifier = Modifier
.align(Alignment.CenterEnd)
.fillMaxHeight()
.onSizeChanged { endRailWidthPx = it.width }
.suppressBackgroundForModal(isModalOpen),
isCompactHeight = config.isCompactHeight,
showElapsedTimeInRail = false,
topPadding = config.railTopPadding,
bottomPadding = config.railBottomPadding,
horizontalPadding = config.railHorizontalPadding,
minSectionGap = config.railMinSectionGap,
centerVerticalFraction = config.centerVerticalFraction
)
}| modifier = Modifier | ||
| .fillMaxSize() | ||
| .background(SideRailTokens.QuickSettingsScrimColor) | ||
| .testTag(QUICK_SETTINGS_SCRIM) |
There was a problem hiding this comment.
To ensure consistent screen reader traversal precedence across all posture layouts, the Quick Settings scrim in SideRailCaptureScaffold should have traversalIndex = 1f set in its semantics block, matching the implementation in CompactPortraitLayout.
.semantics {
contentDescription = dismissLabel
traversalIndex = 1f
}| Surface( | ||
| modifier = Modifier | ||
| .testTag(QUICK_SETTINGS_OVERLAY_CARD_PLACEHOLDER_TAG) | ||
| .widthIn(max = maxCardWidth) | ||
| .then(heightModifier) | ||
| .pointerInput(Unit) { | ||
| detectTapGestures { /* Consume taps on card surface */ } | ||
| }, | ||
| shape = RoundedCornerShape(SideRailTokens.QuickSettingsCardCornerRadius), | ||
| color = MaterialTheme.colorScheme.surfaceContainerLow, | ||
| tonalElevation = SideRailTokens.QuickSettingsCardTonalElevation |
There was a problem hiding this comment.
Using pointerInput with detectTapGestures consumes touch events but does not stop accessibility click propagation. If a screen reader user double-taps the card background, the click action will propagate up to the clickable scrim and dismiss the Quick Settings card. Replacing this with .clickable with an empty lambda correctly consumes both touch and accessibility click actions.
| Surface( | |
| modifier = Modifier | |
| .testTag(QUICK_SETTINGS_OVERLAY_CARD_PLACEHOLDER_TAG) | |
| .widthIn(max = maxCardWidth) | |
| .then(heightModifier) | |
| .pointerInput(Unit) { | |
| detectTapGestures { /* Consume taps on card surface */ } | |
| }, | |
| shape = RoundedCornerShape(SideRailTokens.QuickSettingsCardCornerRadius), | |
| color = MaterialTheme.colorScheme.surfaceContainerLow, | |
| tonalElevation = SideRailTokens.QuickSettingsCardTonalElevation | |
| Surface( | |
| modifier = Modifier | |
| .testTag(QUICK_SETTINGS_OVERLAY_CARD_PLACEHOLDER_TAG) | |
| .widthIn(max = maxCardWidth) | |
| .then(heightModifier) | |
| .clickable( | |
| interactionSource = remember { MutableInteractionSource() }, | |
| indication = null, | |
| onClick = {} // Consumes taps and accessibility clicks to prevent dismissal | |
| ), |
Summary
Extracts the monolithic capture layout in
CaptureLayout.ktintoCompactPortraitLayoutand introduces four side-rail posture layouts—FoldableLayout,TabletPortraitLayout,TabletLandscapeLayout, andCompactShortLayout—sharing mode-agnosticCaptureScaffoldState, in-hierarchy Quick Settings surfaces, and scopedCaptureRtlPolicyhandling. Factoring out these five layout primitives and their geometry/inset/accessibility invariants ahead ofkim/adaptive/layout-orchestratorlets reviewers verify each posture's layout math and tests independently while keepingCaptureScaffoldLayoutbehaviorally unchanged via delegation toCompactPortraitLayout.Key Changes
CaptureLayout.kt&TestTags.kt:CaptureScaffoldLayoutto a ~110-line compatibility adapter packing its slot lambdas intoCaptureSlotsand delegating toCompactPortraitLayout(..., includeStatusBarsInTopBar = false).COMPACT_CONTROLS_TAG = "container_capture_controls_compact".CompactPortraitLayout.kt(new):CompactPortraitLayout,AdaptivePortraitCaptureLayout,VerticalMaterialControls,CompactTopBarBox,CompactCaptureRow, andCompactBottomToolbar, plusLocalActiveCaptureLayoutandModifier.suppressBackgroundForModal.BottomSheetScaffoldstate to non-deprecatedrememberBottomSheetState, addsincludeStatusBarsInTopBarmulti-window top-bar clearance, and scopesCaptureRtlPolicy.mirrorCompactBottomBarstrictly to the horizontal capture and bottom-toolbar rows.SideRailCaptureLayouts.kt(new):FoldableLayout(0.50frail center,380.dpmax Quick Settings card width),TabletLandscapeLayout(0.50frail center,20.dprail horizontal padding,412.dpmax card width), andTabletPortraitLayout(0.70flower thumb-zone rail center,48.dpbottom inset,412.dpmax card width) backed bySideRailCaptureScaffold.QuickSettingsOverlayCardPlaceholder(TODO(#648)): an in-hierarchy floating M3 card (surfaceContainerLow,28.dpcorners,verticalScroll,BackHandler,0.32fscrim) dynamically clearing both measured side rails by12.dp.CompactShortLayout.kt(new):CompactShortLayoutfor< 480dp x < 480dpmulti-window / cover-display panes (360x360dp,320x340dp), wrappingCaptureSlotswith proportionalModifier.sizeInand hardLayoutConstraintscaps (withCondensedSizeCaps/ConstrainedSlot) overSideRailCaptureScaffold(isCompactHeight = true,8.dpcondensed chrome padding,<= 320.dpcard width).SideRailCaptureLayoutsTest.kt&CompactShortLayoutTest.kt(new):statusBarsinsets, Quick Settings bounds, and accessibility modal pruning.Technical Notes
SideRailCaptureScaffoldseeds rail widths fromMinSideRailWidth + railHorizontalPadding * 2and updates them viaonSizeChanged, offsettingtopStartContent/viewfinderTopEndContentbymax(startRailWidth, endRailWidth) + 12.dpand anchoring the floating Quick Settings card atBottomStartunderrailContainerDirection(start = safeStart + startRailWidth + 12.dp,end = safeEnd + endRailWidth + 12.dp) so it always opens besideStartSideRailwithout overlap in both unmirrored and mirrored RTL.CompactShortLayout): After subtracting liveWindowInsets.systemBars.union(WindowInsets.displayCutout)and16.dprail vertical padding (innerRailHeight), start-rail slots cap at0.20 + 0.45 + 0.16 = 0.81 * innerRailHeightand end-rail slots cap at0.15 + 0.22 + 0.15 + 0.15 = 0.67 * innerRailHeight, guaranteeing collision-free, unclipped placement even at320x340dpor360x360dpwith a32.dpvisible status bar.BottomSheetScaffoldnorQuickSettingsOverlayCardPlaceholderspawns aPopuporDialogwindow (screenFlashOverlayremains drawn last edge-to-edge above all chrome). Whenever Quick Settings or anerrorDialog(tracked via bothisErrorDialogShowingandonSizeChangednon-zero bounds) is active,Modifier.suppressBackgroundForModalappliesfocusProperties { canFocus = false }andsemantics { hideFromAccessibility() }to background chrome.mirrorSideRails,mirrorCompactBottomBar) is provided only to the immediate horizontal positioning container, whileLocalLayoutDirectionis restored tolocaleDirectioninside every child slot wrapper.Pre-Existing Bugs Fixed
CaptureLayout.ktpreviously left background camera controls focusable and exposed to accessibility traversal whileBottomSheetScaffold(Quick Settings) or in-hierarchyCameraErrorDialogwas open. Fixed viaModifier.suppressBackgroundForModaland automaticerrorDialogplacement detection.QUICK_SETTINGS_SCRIM& drag handle semantics/precedence: GuardedCompactPortraitLayout's scrim with!isErrorModalActiveso it no longer overlays an activeCameraErrorDialog, addedRole.Button,contentDescription, andtraversalIndex = 1fto the scrim, added.minimumInteractiveComponentSize()toQUICK_SETTINGS_DRAG_HANDLE, and replaced deprecatedrememberStandardBottomSheetStatewithrememberBottomSheetState.Not Included Yet
kim/adaptive/layout-orchestrator): Adds the 7-modeCameraLayoutModehysteresis classifier, preserves the viewfinder viamovableContentOf, and wires liveCaptureScaffoldcontrols across all five layouts.#648): The floating card is a private placeholder (QuickSettingsOverlayCardPlaceholder) until#648lands.#635,#649,#622,#644): VerticalToggleSwitch,VerticalIndicatorPill,Tooltip, andZoomBarRTL polish land in their dedicated Wave 1 leaf PRs.kim/adaptive/layout-rotation,kim/adaptive/viewfinder-rotation,kim/adaptive/content-gravity): Handled in the post-join branches.Testing
SideRailCaptureLayoutsTest: Verifies full-bleed viewfinder centering (LocalCameraLayoutSolution == null),FoldableLayout(852x883dp)0.50frails,TabletPortraitLayout(800x1280dp)0.70flower thumb zone,TabletLandscapeLayout(1280x800dp) &CompactPortraitLayout(412x915dp,360x500dp) unmirrored/mirroredCaptureRtlPolicybehavior, rail-clearedQuickSettingsOverlayCardPlaceholdernon-overlap and!isPopup()/!isDialog()hierarchy, synthetic visible/hiddenstatusBarsinsets, error modal precedence, andHideFromAccessibilitybackground pruning.CompactShortLayoutTest: Verifies zero clipping and strict vertical rail ordering at360x360dp,320x340dp(COMPACT_SHORT_SLIM),470x400dp(both RTL policies),360x360dpwith a32dpvisible multi-window status bar, and condensed<= 320dpQuick Settings card bounds/dismissal.