Refactor Quick Settings UI#530
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.
![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 UI to use a row-based layout (SettingRow) for settings like Capture Mode, HDR, Aspect Ratio, and Flash Mode, while removing the individual setting expansion logic. It also updates FlashModeUiStateAdapter to handle disabled states for unsupported flash modes on the current lens. The review feedback highlights a critical accessibility issue where mergeDescendants = true on SettingRow blocks screen readers from accessing individual buttons. Additionally, suggestions were made to make the Kotlin code in FlashModeUiStateAdapter more idiomatic, and several style guide violations were identified, including missing KDoc comments on internal composables and an incorrectly formatted test tag string.
| Row( | ||
| modifier = modifier | ||
| .testTag(QUICK_SETTINGS_SCROLL_CONTAINER) | ||
| .fillMaxWidth(), | ||
| horizontalArrangement = Arrangement.spacedBy(8.dp, Alignment.CenterHorizontally), | ||
| contentPadding = PaddingValues(horizontal = 16.dp) | ||
| .fillMaxWidth() | ||
| .padding(vertical = 12.dp, horizontal = 16.dp) | ||
| .semantics(mergeDescendants = true) {}, |
There was a problem hiding this comment.
Setting mergeDescendants = true on the outer Row of SettingRow will merge the entire row, including all interactive buttons, into a single accessibility node. This makes it impossible for screen reader users to focus and click the individual setting buttons (like Off, On, Auto). Please remove mergeDescendants = true from the outer Row so that the interactive buttons remain individually accessible.
Row(
modifier = modifier
.fillMaxWidth()
.padding(vertical = 12.dp, horizontal = 16.dp),| val allDeviceSupportedFlashModes = buildSet<FlashMode> { | ||
| systemConstraints.perLensConstraints.let { | ||
| it.keys.forEach { key -> | ||
| it[key]?.supportedFlashModes?.let { flashModes -> addAll(flashModes) } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic to collect all supported flash modes across all lenses can be simplified and made more idiomatic by using Kotlin's standard library functions like flatMap and toSet() on the map's values.
val allDeviceSupportedFlashModes = systemConstraints.perLensConstraints.values
.flatMap { it.supportedFlashModes }
.toSet()| val onlyOffSelectable = displayableModes.size == 1 && | ||
| displayableModes[0].value == FlashMode.OFF && | ||
| displayableModes[0] is SingleSelectableUiState.SelectableUi |
There was a problem hiding this comment.
Instead of checking the size and accessing the first element by index, we can use singleOrNull() which is safer and more idiomatic in Kotlin.
| val onlyOffSelectable = displayableModes.size == 1 && | |
| displayableModes[0].value == FlashMode.OFF && | |
| displayableModes[0] is SingleSelectableUiState.SelectableUi | |
| val onlyOffSelectable = displayableModes.singleOrNull()?.let { | |
| it.value == FlashMode.OFF && it is SingleSelectableUiState.SelectableUi | |
| } ?: false |
| if (newUiState is Unavailable) { | ||
| newUiState // Switch to Unavailable | ||
| } else { | ||
| if (cameraAppSettings.flashMode == FlashMode.LOW_LIGHT_BOOST) { | ||
| val strength = when (val llbState = cameraState.lowLightBoostState) { | ||
| is LowLightBoostState.Active -> llbState.strength | ||
| else -> LowLightBoostState.MINIMUM_STRENGTH | ||
| } | ||
| copy( | ||
| isLowLightBoostActive = strength > 0.5 | ||
| ) | ||
| newUiState as Available // Cast to Available | ||
|
|
||
| // Check if the list of modes or their enabled/disabled states have changed. | ||
| // Data class list comparison works well here. | ||
| if (this.availableFlashModes != newUiState.availableFlashModes) { | ||
| newUiState | ||
| } else if (this.selectedFlashMode != cameraAppSettings.flashMode) { | ||
| // Only the selection changed | ||
| copy(selectedFlashMode = cameraAppSettings.flashMode) | ||
| } else { | ||
| // Nothing has changed | ||
| this | ||
| // Check for Low Light Boost state changes if it's the selected mode | ||
| if (cameraAppSettings.flashMode == FlashMode.LOW_LIGHT_BOOST) { | ||
| val strength = when (val llbState = cameraState.lowLightBoostState) { | ||
| is LowLightBoostState.Active -> llbState.strength | ||
| else -> LowLightBoostState.MINIMUM_STRENGTH | ||
| } | ||
| val newIsLowLightBoostActive = strength > 0.5 | ||
| if (this.isLowLightBoostActive != newIsLowLightBoostActive) { | ||
| copy(isLowLightBoostActive = newIsLowLightBoostActive) | ||
| } else { | ||
| this // Nothing changed | ||
| } | ||
| } else { | ||
| this // Nothing changed | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Using when to handle newUiState allows us to leverage Kotlin's smart casting and avoid the explicit cast newUiState as Available.
when (newUiState) {
is Unavailable -> newUiState
is Available -> {
// Check if the list of modes or their enabled/disabled states have changed.
// Data class list comparison works well here.
if (this.availableFlashModes != newUiState.availableFlashModes) {
newUiState
} else if (this.selectedFlashMode != cameraAppSettings.flashMode) {
// Only the selection changed
copy(selectedFlashMode = cameraAppSettings.flashMode)
} else {
// Check for Low Light Boost state changes if it's the selected mode
if (cameraAppSettings.flashMode == FlashMode.LOW_LIGHT_BOOST) {
val strength = when (val llbState = cameraState.lowLightBoostState) {
is LowLightBoostState.Active -> llbState.strength
else -> LowLightBoostState.MINIMUM_STRENGTH
}
val newIsLowLightBoostActive = strength > 0.5
if (this.isLowLightBoostActive != newIsLowLightBoostActive) {
copy(isLowLightBoostActive = newIsLowLightBoostActive)
} else {
this // Nothing changed
}
} else {
this // Nothing changed
}
}
}
}| @Composable | ||
| fun focusedRatioButtons( | ||
| onUnFocus: () -> Unit, | ||
| onSetAspectRatio: (AspectRatio) -> Unit, | ||
| aspectRatioUiState: AspectRatioUiState | ||
| ): List<@Composable () -> Unit> = listOf( | ||
| { | ||
| CloseExpandedSettingsButton(onUnFocus) | ||
| }, | ||
| { | ||
| QuickSetRatio( | ||
| modifier = Modifier.testTag(QUICK_SETTINGS_RATIO_3_4_BUTTON), | ||
| onClick = { onSetAspectRatio(AspectRatio.THREE_FOUR) }, | ||
| assignedRatio = AspectRatio.THREE_FOUR, | ||
| aspectRatioUiState = aspectRatioUiState, | ||
| isHighlightEnabled = true | ||
| ) | ||
| }, | ||
| { | ||
| QuickSetRatio( | ||
| modifier = Modifier.testTag(QUICK_SETTINGS_RATIO_9_16_BUTTON), | ||
| onClick = { onSetAspectRatio(AspectRatio.NINE_SIXTEEN) }, | ||
| assignedRatio = AspectRatio.NINE_SIXTEEN, | ||
| aspectRatioUiState = aspectRatioUiState, | ||
| isHighlightEnabled = true | ||
| ) | ||
| }, | ||
| { | ||
| QuickSetRatio( | ||
| modifier = Modifier.testTag(QUICK_SETTINGS_RATIO_1_1_BUTTON), | ||
| onClick = { onSetAspectRatio(AspectRatio.ONE_ONE) }, | ||
| assignedRatio = AspectRatio.ONE_ONE, | ||
| aspectRatioUiState = aspectRatioUiState, | ||
| isHighlightEnabled = true | ||
| internal fun CaptureModeRow( |
There was a problem hiding this comment.
According to the repository style guide (Section 11), all non-private members (including internal composables) must have KDoc documentation. Please add a descriptive KDoc for CaptureModeRow explaining its behavior, features, and parameters.
References
- All non-private classes, functions, and composables must have KDoc documentation. (link)
| @Composable | ||
| fun focusedCaptureModeButtons( | ||
| onUnFocus: () -> Unit, | ||
| onSetCaptureMode: (CaptureMode) -> Unit, | ||
| captureModeUiState: CaptureModeUiState | ||
| ): List<@Composable () -> Unit> = listOf( | ||
| { | ||
| CloseExpandedSettingsButton(onUnFocus) | ||
| }, | ||
| { | ||
| QuickSetCaptureMode( | ||
| modifier = Modifier | ||
| .testTag(BTN_QUICK_SETTINGS_FOCUSED_CAPTURE_MODE_OPTION_STANDARD), | ||
| onClick = { onSetCaptureMode(CaptureMode.STANDARD) }, | ||
| assignedCaptureMode = CaptureMode.STANDARD, | ||
| captureModeUiState = captureModeUiState, | ||
| isHighlightEnabled = true | ||
| ) | ||
| }, | ||
| { | ||
| QuickSetCaptureMode( | ||
| modifier = Modifier | ||
| .testTag(BTN_QUICK_SETTINGS_FOCUSED_CAPTURE_MODE_IMAGE_ONLY), | ||
| onClick = { onSetCaptureMode(CaptureMode.IMAGE_ONLY) }, | ||
| assignedCaptureMode = CaptureMode.IMAGE_ONLY, | ||
| captureModeUiState = captureModeUiState, | ||
| isHighlightEnabled = true | ||
| internal fun HdrRow( |
There was a problem hiding this comment.
According to the repository style guide (Section 11), all non-private members (including internal composables) must have KDoc documentation. Please add a descriptive KDoc for HdrRow explaining its behavior, features, and parameters.
References
- All non-private classes, functions, and composables must have KDoc documentation. (link)
| @Composable | ||
| internal fun AspectRatioRow( |
There was a problem hiding this comment.
According to the repository style guide (Section 11), all non-private members (including internal composables) must have KDoc documentation. Please add a descriptive KDoc for AspectRatioRow explaining its behavior, features, and parameters.
References
- All non-private classes, functions, and composables must have KDoc documentation. (link)
| @Composable | ||
| private fun CloseExpandedSettingsButton(onUnFocus: () -> Unit, modifier: Modifier = Modifier) { | ||
| FilledIconButton( | ||
| modifier = modifier.testTag(QUICK_SETTINGS_CLOSE_EXPANDED_BUTTON), | ||
| onClick = onUnFocus | ||
| ) { | ||
| Icon( | ||
| painter = painterResource(R.drawable.ic_close), | ||
| contentDescription = stringResource( | ||
| R.string.quick_settings_btn_close_expanded_settings_description | ||
| ) | ||
| internal fun FlashRow( |
There was a problem hiding this comment.
According to the repository style guide (Section 11), all non-private members (including internal composables) must have KDoc documentation. Please add a descriptive KDoc for FlashRow explaining its behavior, features, and parameters.
References
- All non-private classes, functions, and composables must have KDoc documentation. (link)
| const val HDR_VIDEO_UNSUPPORTED_ON_DEVICE_TAG = "HdrVideoUnsupportedOnDeviceTag" | ||
| const val HDR_VIDEO_UNSUPPORTED_ON_LENS_TAG = "HdrVideoUnsupportedOnDeviceTag" | ||
| const val HDR_SIMULTANEOUS_IMAGE_VIDEO_UNSUPPORTED_TAG = "HdrSimultaneousImageVideoUnsupportedTag" | ||
| const val FLASH_UNSUPPORTED_ON_LENS_TAG = "FlashUnsupportedOnLensTag" |
There was a problem hiding this comment.
According to the repository style guide (Section 10), when creating a new test tag, the string value must be a lower_snake_case string following the schema element_purpose_value. Please update the string value of FLASH_UNSUPPORTED_ON_LENS_TAG to follow this convention (e.g., "toast_flash_unsupported_on_lens").
References
- When creating a new test tag, the constant name must be in UPPER_SNAKE_CASE, and the string value must be a lower_snake_case string following a specific schema: element_purpose_value. (link)
…r/quickSettings/button-rows and implement HDR toggle
1 participant
No description provided.