Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the CaptureButton animations to utilize Material 3 Expressive Motion schemes, replacing hardcoded durations and transitions. It also introduces new tests to verify the behavior of the button ring border and scaling animations. The review feedback suggests optimizing performance by deferring state reading of animatedColor and animatedBorderWidth using lambda providers to prevent excessive recompositions of the CaptureButton. Additionally, it is recommended to update the corresponding tests for these lambda parameters and to import CompositionLocalProvider directly in the test file instead of using its fully qualified name.
| captureButtonSize = captureButtonSize, | ||
| color = animatedColor | ||
| color = animatedColor, | ||
| borderWidth = animatedBorderWidth.value |
There was a problem hiding this comment.
Reading animating states like animatedColor and animatedBorderWidth directly in the body of CaptureButton causes the entire CaptureButton composable to recompose on every single frame of the animations. Pass them as lambda providers to defer state reading and optimize performance.
Note: You will also need to update the CaptureButtonRing definition below, as well as any previews and tests that call it, to accept and pass these lambdas.
| captureButtonSize = captureButtonSize, | |
| color = animatedColor | |
| color = animatedColor, | |
| borderWidth = animatedBorderWidth.value | |
| captureButtonSize = captureButtonSize, | |
| color = { animatedColor }, | |
| borderWidth = { animatedBorderWidth } |
References
- Performance and Efficiency: Scan for inefficient operations, especially within Composable functions (e.g., expensive calculations, improper state management leading to excessive recompositions). (link)
| fun captureButtonRing_borderWidthZero_doesNotComposeBorder() { | ||
| composeTestRule.setContent { | ||
| CaptureButtonRing( | ||
| captureButtonSize = 86f, | ||
| color = Color.White, | ||
| borderWidth = 0f | ||
| ) | ||
| } | ||
| composeTestRule.onNodeWithTag(CAPTURE_BUTTON_RING_BORDER).assertDoesNotExist() | ||
| } | ||
|
|
||
| @Test | ||
| fun captureButtonRing_borderWidthGreaterThanZero_composesBorder() { | ||
| composeTestRule.setContent { | ||
| CaptureButtonRing( | ||
| captureButtonSize = 86f, | ||
| color = Color.White, | ||
| borderWidth = 3f | ||
| ) | ||
| } | ||
| composeTestRule.onNodeWithTag(CAPTURE_BUTTON_RING_BORDER).assertExists() | ||
| } | ||
| } |
There was a problem hiding this comment.
Update the CaptureButtonRing tests to match the new signature that accepts lambda providers for color and borderWidth.
@Test
fun captureButtonRing_borderWidthZero_doesNotComposeBorder() {
composeTestRule.setContent {
CaptureButtonRing(
captureButtonSize = 86f,
color = { Color.White },
borderWidth = { 0.dp }
)
}
composeTestRule.onNodeWithTag(CAPTURE_BUTTON_RING_BORDER).assertDoesNotExist()
}
@Test
fun captureButtonRing_borderWidthGreaterThanZero_composesBorder() {
composeTestRule.setContent {
CaptureButtonRing(
captureButtonSize = 86f,
color = { Color.White },
borderWidth = { 3.dp }
)
}
composeTestRule.onNodeWithTag(CAPTURE_BUTTON_RING_BORDER).assertExists()
}| androidx.compose.runtime.CompositionLocalProvider( | ||
| LocalDisableAnimations provides true | ||
| ) { |
There was a problem hiding this comment.
Avoid using fully qualified names for common Compose APIs like CompositionLocalProvider. Instead, import androidx.compose.runtime.CompositionLocalProvider at the top of the file and use it directly to improve code readability and maintainability.
| androidx.compose.runtime.CompositionLocalProvider( | |
| LocalDisableAnimations provides true | |
| ) { | |
| CompositionLocalProvider( | |
| LocalDisableAnimations provides true | |
| ) { |
Extracts the recording red color (Color(0xFFED0000)) into an internal CaptureTokens object in :ui:components:capture. Updates CaptureButtonNucleus and ElapsedTimeText to use the shared token instead of duplicating the literal color value, and adds a unit test verifying the token value.
…creenshot goldens
This PR updates
CaptureButton,CaptureButtonRing, andCaptureButtonNucleusto use Material 3 Expressive motion specifications and refined sizing and geometry.Key Changes
Button and Ring Sizing:
76.dpto86.dp.3.dp<->0.dp) usingMotionScheme.expressive().fastSpatialSpec()so the outer border smoothly transitions when entering or leavingCaptureMode.STANDARD.CaptureButtonRingand keyedisCaptureButtonPressedwithremember(initialPressed)to properly supportLocalInitialPressedState.Spatial and Effects Motion Specs:
CaptureButtonNucleussize and corner radius animations to useMotionScheme.expressive().fastSpatialSpec(), synchronizing the scale transition and corner radius morph (8.dpwhen locked).MotionScheme.expressive().fastEffectsSpec(), updated recording indicator color to#ED0000, and updated pressed opacity inCaptureMode.IMAGE_ONLYfrom0.5fto0.8f.Standard (Hybrid) Mode Transitions:
0.80f) and pressed capture scale (0.86f) with a responsive spring spec (stiffness = 1800f,dampingRatio = 0.65f), instant color materialization, and a50msvisual press hold for quick taps so taps reliably materialize and bounce within the outer ring border.0.dp.Testing: