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

[madebymozart]

Updated the CameraXBasic sample to address some nits and pieces that didn't match compose best practices

[gemini-code-assist]

Hello @madebymozart, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

Hello! Gemini here, providing a summary of this pull request authored by madebymozart. The primary goal of this PR is to refactor the CameraXBasic sample to align better with current Jetpack Compose best practices. This involves updating how the ViewModel is instantiated and ensuring that Modifier parameters are correctly passed down through the composable hierarchy, making the components more reusable and testable. Additionally, some minor layout adjustments and a documentation typo fix are included.

Highlights

• ViewModel Usage: The CameraXBasicViewModel is updated to inherit from androidx.lifecycle.ViewModel, and its instantiation in the CameraXBasic composable is changed from remember { CameraXBasicViewModel() } to using the androidx.lifecycle.viewmodel.compose.viewModel() composable function. This ensures the ViewModel is correctly scoped to the composable's lifecycle.
• Modifier Propagation: The modifier parameter is now consistently passed down through various composables like Box, Column, Image, Button, CameraXViewfinder, AnimatedVisibility, CapturedImageView, and CameraPermissionDeniedView. This is a fundamental Compose pattern that allows parent composables to control the size, layout, and behavior of their children.
• Layout Simplification: The layout for the 'Take Photo' button within CameraPreviewContent is simplified by replacing a Column wrapper with direct application of modifiers like fillMaxSize() and wrapContentSize(align = Alignment.BottomCenter) to the Button itself.
• Documentation Fix: A minor typo in the KDoc for the ImageSavedCallback class in CameraXBasicViewModel.kt is corrected, changing a parameter name from onErrorA to onError.

Changelog

Click here to see the changelog

• samples/camera/camerax/src/main/java/com/example/platform/camerax/basic/CameraXBasic.kt
  • Added import for androidx.compose.foundation.layout.wrapContentSize (line 38).
  • Added import for androidx.lifecycle.viewmodel.compose.viewModel (line 67).
  • Changed ViewModel instantiation to use viewModel() composable (line 89).
  • Passed the external modifier parameter to the root Box (line 97).
  • Removed redundant modifier parameter passed to ContentWithPermissionHandling (line 103).
  • Added modifier parameter to CapturedImageView call (line 137).
  • Added modifier parameter to CameraPermissionDeniedView call (line 161).
  • Added modifier parameter to CameraPermissionDeniedView function signature (line 179).
  • Passed the external modifier parameter to the root Column in CameraPermissionDeniedView (line 182).
  • Passed the external modifier parameter to the Spacer in CameraPermissionDeniedView (line 194).
  • Passed the external modifier parameter to the root Box in CameraPreviewContent (line 241).
  • Passed the external modifier parameter to CameraXViewfinder (line 245).
  • Passed the external modifier parameter to the AnimatedVisibility for the autofocus indicator (line 262).
  • Simplified the 'Take Photo' button layout by removing the Column wrapper and applying layout modifiers directly to the Button (lines 273-279).
  • Added modifier parameter to CapturedImageView function signature (line 294).
  • Passed the external modifier parameter to the root Box in CapturedImageView (line 298).
  • Passed the external modifier parameter to the Image in CapturedImageView (line 303).
  • Passed the external modifier parameter to the IconButton in CapturedImageView (line 308).
• samples/camera/camerax/src/main/java/com/example/platform/camerax/basic/CameraXBasicViewModel.kt
  • Added import for androidx.lifecycle.ViewModel (line 40).
  • Made CameraXBasicViewModel inherit from ViewModel (line 56).
  • Corrected typo in KDoc parameter name from onError to onErrorA (line 173).

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

---

A composable view,
Takes a modifier, it's true,
Layout flows just right,
Shining ever so bright,
Making code easy for you.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request updates the CameraXBasic sample, focusing on Jetpack Compose best practices. Key changes include adopting the viewModel() delegate for CameraXBasicViewModel and refactoring how Modifiers are handled and propagated.

The ViewModel-related changes are excellent and align well with standard practices. However, there are several instances where the modifier parameter passed into a composable is reused for its internal child components. This can lead to incorrect layouts or compounded modifier effects when these composables are used with non-default Modifiers (e.g., those including padding or size constraints). I've provided suggestions to correct this pattern, ensuring that modifiers are applied more predictably.

Summary of Findings

• Modifier Misuse in Composables: Several composables (CameraPermissionDeniedView, CameraPreviewContent, CapturedImageView) incorrectly reuse their modifier parameter for internal child elements. This parameter should typically be applied to the root element of the composable, and child elements should define their own Modifier instances. This can lead to unexpected layout behavior when non-default modifiers are passed to these composables. (Severity: High, commented)
• ViewModel Instantiation Best Practice: The change from remember { CameraXBasicViewModel() } to val viewModel: CameraXBasicViewModel = viewModel() is a significant improvement, aligning with Jetpack Compose best practices for ViewModel creation, scoping, and lifecycle management.
• ViewModel Inheritance: CameraXBasicViewModel now correctly inherits from androidx.lifecycle.ViewModel, which is essential for its proper functioning with the viewModel() delegate and the Android lifecycle.
• KDoc Parameter Naming (Low Severity - Not Commented): In CameraXBasicViewModel.kt, the KDoc for ImageSavedCallback's constructor parameter was updated from onError to onErrorA to match the actual parameter name. While the KDoc is now consistent, the parameter name onErrorA itself is slightly unconventional. onError is generally preferred for clarity unless there's a specific naming conflict. This is a minor point and was not commented on due to review settings.

Merge Readiness

The pull request makes good strides in modernizing the CameraXBasic sample, especially with the ViewModel handling. However, the issues identified with modifier usage are significant from a Compose best practices standpoint and could lead to subtle bugs or unexpected UI behavior. I recommend addressing these high severity issues before merging to ensure the sample robustly demonstrates correct modifier patterns. I am not authorized to approve pull requests, so please ensure further review and approval after these changes are made.

[gemini-code-assist]

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))

[gemini-code-assist]

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

[gemini-code-assist]

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)

[gemini-code-assist]

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()

[gemini-code-assist]

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)

[madebymozart]

Ignore - Addressed here: #299