Skip to content

[CameraX • Compose] Update CameraXBasic to match compose best practices #300

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
madebymozart wants to merge 1 commit into from
Closed

[CameraX • Compose] Update CameraXBasic to match compose best practices #300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 30 additions & 20 deletions samples/camera/camerax/src/main/java/com/example/platform/camerax/basic/CameraXBasic.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import androidx.compose.foundation.layout.height
import androidx.compose.foundation.layout.offset
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.size
import androidx.compose.foundation.layout.wrapContentSize
import androidx.compose.foundation.shape.CircleShape
import androidx.compose.material.icons.Icons
import androidx.compose.material.icons.automirrored.filled.ArrowBack
Expand Down Expand Up @@ -63,6 +64,7 @@ import androidx.compose.ui.unit.round
import androidx.lifecycle.LifecycleOwner
import androidx.lifecycle.compose.LocalLifecycleOwner
import androidx.lifecycle.compose.collectAsStateWithLifecycle
import androidx.lifecycle.viewmodel.compose.viewModel
import coil.compose.rememberAsyncImagePainter
import com.google.accompanist.permissions.ExperimentalPermissionsApi
import com.google.accompanist.permissions.PermissionState
Expand All @@ -84,22 +86,21 @@ fun CameraXBasic(modifier: Modifier = Modifier) {
var showCapturedImage by remember { mutableStateOf<Uri?>(null) }
val cameraPermissionState = rememberPermissionState(Manifest.permission.CAMERA)
val imageCaptureCallbackExecutor: ExecutorService = remember { Executors.newSingleThreadExecutor() }
val viewModel = remember { CameraXBasicViewModel() }
val viewModel: CameraXBasicViewModel = viewModel()

DisposableEffect(Unit) {
onDispose {
imageCaptureCallbackExecutor.shutdown()
}
}

Box(modifier = Modifier.fillMaxSize()) {
Box(modifier = modifier.fillMaxSize()) {
ContentWithPermissionHandling(
cameraPermissionState = cameraPermissionState,
showCapturedImage = showCapturedImage,
onShowCapturedImageChange = { showCapturedImage = it },
viewModel = viewModel,
imageCaptureCallbackExecutor = imageCaptureCallbackExecutor,
modifier = modifier,
)
}
}
Expand Down Expand Up @@ -131,7 +132,10 @@ private fun ContentWithPermissionHandling(
when (cameraPermissionState.status) {
is PermissionStatus.Granted -> {
if (showCapturedImage != null) {
CapturedImageView(uri = showCapturedImage) {
CapturedImageView(
uri = showCapturedImage,
modifier = modifier,
) {
onShowCapturedImageChange(null)
}
} else {
Expand All @@ -154,6 +158,7 @@ private fun ContentWithPermissionHandling(
is PermissionStatus.Denied -> CameraPermissionDeniedView(
cameraPermissionState.status,
cameraPermissionState,
modifier = modifier,
)
}
}
Expand All @@ -171,9 +176,10 @@ private fun ContentWithPermissionHandling(
private fun CameraPermissionDeniedView(
status: PermissionStatus,
cameraPermissionState: PermissionState,
modifier: Modifier,
) {
Column(
modifier = Modifier
modifier = modifier
.fillMaxSize()
.padding(16.dp),
horizontalAlignment = Alignment.CenterHorizontally,
Expand All @@ -185,7 +191,7 @@ private fun CameraPermissionDeniedView(
"Camera permission is required to use this feature."
}
Text(text = textToShow)
Spacer(modifier = Modifier.height(8.dp))
Spacer(modifier = modifier.height(8.dp))
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

It seems the modifier parameter of CameraPermissionDeniedView is being applied to this Spacer. Typically, the modifier parameter of a composable is intended for its root element (the Column in this case). Child elements like this Spacer should define their own modifiers.

Reusing the parent's modifier here could lead to unexpected behavior. For example, if CameraPermissionDeniedView were called with modifier = Modifier.fillMaxWidth(), this Spacer would also attempt to fillMaxWidth, which is likely not the desired outcome for a simple spacer.

Could this be changed to use Modifier.height(8.dp) directly?

Suggested change
Spacer(modifier = modifier.height(8.dp))
Spacer(modifier = Modifier.height(8.dp))

Button(onClick = { cameraPermissionState.launchPermissionRequest() }) {
Text("Request Permission")
}
Expand Down Expand Up @@ -232,11 +238,11 @@ private fun CameraPreviewContent(

surfaceRequest?.let { request ->
val coordinateTransformer = remember { MutableCoordinateTransformer() }
Box(modifier = Modifier.fillMaxSize()) {
Box(modifier = modifier.fillMaxSize()) {
CameraXViewfinder(
surfaceRequest = request,
coordinateTransformer = coordinateTransformer,
modifier = Modifier
modifier = modifier
.fillMaxSize() // Ensure CameraXViewfinder fills the Box
.pointerInput(viewModel, coordinateTransformer) {
detectTapGestures { tapCoords ->
Expand All @@ -253,7 +259,7 @@ private fun CameraPreviewContent(
visible = showAutofocusIndicator,
enter = fadeIn(),
exit = fadeOut(),
modifier = Modifier
modifier = modifier
.offset { autofocusLocation.takeOrElse { Offset.Zero }.round() }
.offset((-24).dp, (-24).dp), // Adjust offset to center the indicator
Comment on lines +262 to 264
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Similar to the CameraXViewfinder, the modifier parameter of CameraPreviewContent is being reused for this AnimatedVisibility. This can lead to unexpected behavior if CameraPreviewContent's modifier has properties not intended for this specific animated element (e.g., background color, specific sizing that conflicts with offset).

To ensure predictable behavior and proper encapsulation, should this AnimatedVisibility define its own Modifier chain starting with Modifier.offset(...)?

Suggested change
modifier = modifier
.offset { autofocusLocation.takeOrElse { Offset.Zero }.round() }
.offset((-24).dp, (-24).dp), // Adjust offset to center the indicator
modifier = Modifier
.offset { autofocusLocation.takeOrElse { Offset.Zero }.round() }
.offset((-24).dp, (-24).dp) // Adjust offset to center the indicator

) {
Expand All @@ -263,14 +269,14 @@ private fun CameraPreviewContent(
.size(48.dp),
)
}
Column(
modifier = Modifier
.align(Alignment.BottomCenter)

Button(
modifier = modifier
.fillMaxSize()
.wrapContentSize(align = Alignment.BottomCenter)
.padding(16.dp),
Comment on lines +274 to 277
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The modifier parameter of CameraPreviewContent is also being used as the base for this Button's modifier. This is the third direct child/element within CameraPreviewContent (after the root Box itself and CameraXViewfinder) that is reusing this modifier.

For clarity and to avoid unintended side effects from the passed-in modifier, would it be better for the Button to construct its modifier chain starting from a fresh Modifier instance, like Modifier.fillMaxSize().wrapContentSize(align = Alignment.BottomCenter).padding(16.dp)?

modifier = Modifier
    .fillMaxSize()
    .wrapContentSize(align = Alignment.BottomCenter)
    .padding(16.dp)

horizontalAlignment = Alignment.CenterHorizontally,
) {
Button(onClick = onTakePhotoClick) { Text("Take Photo") }
}
onClick = onTakePhotoClick,
) { Text("Take Photo") }
}
}
}
Expand All @@ -283,19 +289,23 @@ private fun CameraPreviewContent(
* (e.g., clicks the back button).
*/
@Composable
fun CapturedImageView(uri: Uri, onDismiss: () -> Unit) {
fun CapturedImageView(
uri: Uri,
modifier: Modifier,
onDismiss: () -> Unit,
) {
Box(
modifier = Modifier.fillMaxSize(),
modifier = modifier.fillMaxSize(),
) {
Image(
painter = rememberAsyncImagePainter(uri),
contentDescription = "Captured Image",
modifier = Modifier.fillMaxSize(),
modifier = modifier.fillMaxSize(),
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The modifier parameter of CapturedImageView (which is correctly applied to its root Box on line 298, chained with .fillMaxSize()) is being reused here for the Image composable. If the passed-in modifier contains, for example, padding, this padding would be applied to the Box and then again effectively to the Image within that Box.

To avoid such compounded effects, should the Image simply use Modifier.fillMaxSize() to fill its parent Box?

modifier = Modifier.fillMaxSize()

contentScale = ContentScale.Fit,
)
IconButton(
onClick = onDismiss,
modifier = Modifier
modifier = modifier
.align(Alignment.TopStart)
.padding(16.dp),
Comment on lines +308 to 310
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Similar to the Image composable, the modifier parameter of CapturedImageView is being reused for this IconButton. The IconButton should define its own modifiers for alignment and padding relative to its parent Box.

Would it be clearer and safer to use Modifier.align(Alignment.TopStart).padding(16.dp) here?

modifier = Modifier
    .align(Alignment.TopStart)
    .padding(16.dp)

) {
Expand Down
5 changes: 3 additions & 2 deletions .../camera/camerax/src/main/java/com/example/platform/camerax/basic/CameraXBasicViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import androidx.camera.lifecycle.awaitInstance
import androidx.compose.ui.geometry.Offset
import androidx.core.content.ContextCompat
import androidx.lifecycle.LifecycleOwner
import androidx.lifecycle.ViewModel
import kotlinx.coroutines.awaitCancellation
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.StateFlow
Expand All @@ -52,7 +53,7 @@ import java.util.concurrent.ExecutorService
* functionality using CameraX. It exposes a [StateFlow] for the camera preview [SurfaceRequest]
* to be used in a composable.
*/
class CameraXBasicViewModel {
class CameraXBasicViewModel : ViewModel() {
private val _surfaceRequest = MutableStateFlow<SurfaceRequest?>(null)
val surfaceRequest: StateFlow<SurfaceRequest?> = _surfaceRequest
private var surfaceMeteringPointFactory: SurfaceOrientedMeteringPointFactory? = null
Expand Down Expand Up @@ -169,7 +170,7 @@ class CameraXBasicViewModel {
*
* @param context The application context.
* @param onImageCaptured Callback invoked when the image is successfully captured and saved.
* @param onError Callback invoked if an error occurs during image capture.
* @param onErrorA Callback invoked if an error occurs during image capture.
*/
private class ImageSavedCallback(
private val context: Context,
Expand Down
Loading