Implement animation disable mechanism for instrumentation tests#519
Implement animation disable mechanism for instrumentation tests#519temcguir wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to disable animations within the application, primarily for testing purposes. It adds a LocalDisableAnimations CompositionLocal, updates MainActivity to read a disable_animations flag from TestStorage or an intent extra, and modifies various UI components to bypass animations when this flag is active. The review feedback focuses on making the implementation more idiomatic to Jetpack Compose by using conditional transitions within AnimatedVisibility and animateFloatAsState instead of manual if-else blocks, which reduces code duplication and nesting.
| if (disableAnimations) { | ||
| if (isVisible) { | ||
| ElapsedTimeText( | ||
| modifier = Modifier.testTag(ELAPSED_TIME_TAG), | ||
| elapsedTimeUiState = captureUiState.elapsedTimeUiState | ||
| ) | ||
| } | ||
| } else { | ||
| AnimatedVisibility( | ||
| visible = isVisible, | ||
| enter = fadeIn(), | ||
| exit = fadeOut(animationSpec = tween(delayMillis = 1_500)) | ||
| ) { | ||
| ElapsedTimeText( | ||
| modifier = Modifier.testTag(ELAPSED_TIME_TAG), | ||
| elapsedTimeUiState = captureUiState.elapsedTimeUiState | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
The pattern of manually bypassing AnimatedVisibility with an if (disableAnimations) block is repeated multiple times in this PR. This can be simplified by providing conditional EnterTransition and ExitTransition to AnimatedVisibility. This approach adheres to the DRY principle and improves code readability while still achieving the goal of disabling animations in tests.
See Repository Style Guide Rule 23 (Simplify Complex Logic) and Rule 27 (Promote Reusability).
AnimatedVisibility(
visible = isVisible,
enter = if (disableAnimations) EnterTransition.None else fadeIn(),
exit = if (disableAnimations) ExitTransition.None else fadeOut(animationSpec = tween(delayMillis = 1_500))
) {
ElapsedTimeText(
modifier = Modifier.testTag(ELAPSED_TIME_TAG),
elapsedTimeUiState = captureUiState.elapsedTimeUiState
)
}References
- Simplify Complex Logic: Look for needlessly complex code. If a multi-line block of logic can be condensed into a more concise and readable idiomatic expression, suggest the simplification. (link)
- Promote Reusability (DRY Principle): Identify duplicated or highly similar blocks of code. If a pattern of logic is repeated—even with minor variations—suggest extracting it into a reusable function, composable, or helper class. (link)
| if (disableAnimations) { | ||
| if (isQuickSettingsVisible) { | ||
| quickSettingsController?.let { qsc -> | ||
| ToggleQuickSettingsButton( | ||
| modifier = modifier, | ||
| isOpen = ( | ||
| captureUiState.quickSettingsUiState | ||
| as? QuickSettingsUiState.Available | ||
| )?.quickSettingsIsOpen == true, | ||
| quickSettingsController = qsc | ||
| ) | ||
| } | ||
| } | ||
| } else { | ||
| AnimatedVisibility( | ||
| visible = isQuickSettingsVisible, | ||
| enter = fadeIn(), | ||
| exit = fadeOut(animationSpec = tween(delayMillis = 1_500)) | ||
| ) { | ||
| quickSettingsController?.let { qsc -> | ||
| ToggleQuickSettingsButton( | ||
| modifier = modifier, | ||
| isOpen = ( | ||
| captureUiState.quickSettingsUiState | ||
| as? QuickSettingsUiState.Available | ||
| )?.quickSettingsIsOpen == true, | ||
| quickSettingsController = qsc | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Similar to the elapsedTimeDisplay, this block can be simplified by using conditional transitions within AnimatedVisibility instead of a manual if check.
AnimatedVisibility(
visible = isQuickSettingsVisible,
enter = if (disableAnimations) EnterTransition.None else fadeIn(),
exit = if (disableAnimations) ExitTransition.None else fadeOut(animationSpec = tween(delayMillis = 1_500))
) {
quickSettingsController?.let { qsc ->
ToggleQuickSettingsButton(
modifier = modifier,
isOpen = (
captureUiState.quickSettingsUiState
as? QuickSettingsUiState.Available
)?.quickSettingsIsOpen == true,
quickSettingsController = qsc
)
}
}References
- Simplify Complex Logic: Look for needlessly complex code. If a multi-line block of logic can be condensed into a more concise and readable idiomatic expression, suggest the simplification. (link)
| val animatedAudioAlpha = if (disableAnimations) { | ||
| 1f | ||
| } else { | ||
| val alpha by animateFloatAsState( | ||
| targetValue = EaseOutExpo.transform( | ||
| (currentUiState.value.amplitude.toFloat()).coerceIn( | ||
| 0f, | ||
| 1f | ||
| ) | ||
| ), | ||
| label = "AudioAnimation" | ||
| ) | ||
| alpha | ||
| } |
There was a problem hiding this comment.
Instead of conditionally calling animateFloatAsState, you can call it unconditionally and provide a conditional targetValue and animationSpec. This is more idiomatic in Compose and reduces nesting.
val animatedAudioAlpha by animateFloatAsState(
targetValue = if (disableAnimations) 1f else EaseOutExpo.transform(
(currentUiState.value.amplitude.toFloat()).coerceIn(0f, 1f)
),
animationSpec = if (disableAnimations) snap() else tween(),
label = "AudioAnimation"
)References
- Simplify Complex Logic: Look for needlessly complex code. If a multi-line block of logic can be condensed into a more concise and readable idiomatic expression, suggest the simplification. (link)
| if (disableAnimations) { | ||
| if (isVisible) { | ||
| Box( | ||
| Modifier | ||
| .testTag(FOCUS_METERING_INDICATOR_TAG) | ||
| .alpha( | ||
| if (focusMeteringUiState.status == FocusMeteringUiState.Status.SUCCESS) { | ||
| 1f | ||
| } else { | ||
| alpha | ||
| } | ||
| ) | ||
| .border( | ||
| 1.dp, | ||
| Color.White, | ||
| CircleShape | ||
| ) | ||
| .size(TAP_TO_FOCUS_INDICATOR_SIZE) | ||
| .offset { tapCoords.round() } | ||
| .offset(-TAP_TO_FOCUS_INDICATOR_SIZE / 2, -TAP_TO_FOCUS_INDICATOR_SIZE / 2) | ||
| ) | ||
| } | ||
| } else { | ||
| AnimatedVisibility( | ||
| visible = isVisible, | ||
| enter = fadeIn() + scaleIn(initialScale = 1.5f), | ||
| exit = fadeOut() + when (focusMeteringUiState.status) { | ||
| FocusMeteringUiState.Status.SUCCESS -> scaleOut(targetScale = 0.5f) | ||
| FocusMeteringUiState.Status.FAILURE -> scaleOut(targetScale = 1.5f) | ||
| else -> fadeOut() | ||
| }, | ||
| modifier = Modifier | ||
| .offset { tapCoords.round() } | ||
| // Offset the indicator to be centered on the tap coordinates | ||
| .offset(-TAP_TO_FOCUS_INDICATOR_SIZE / 2, -TAP_TO_FOCUS_INDICATOR_SIZE / 2) | ||
| ) { | ||
| Box( | ||
| Modifier | ||
| .testTag(FOCUS_METERING_INDICATOR_TAG) | ||
| .alpha( | ||
| if (focusMeteringUiState.status == FocusMeteringUiState.Status.SUCCESS) { | ||
| 1f | ||
| } else { | ||
| alpha | ||
| } | ||
| ) | ||
| .border( | ||
| 1.dp, | ||
| Color.White, | ||
| CircleShape | ||
| ) | ||
| .size(TAP_TO_FOCUS_INDICATOR_SIZE) | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
This is another instance where AnimatedVisibility is manually bypassed. Using conditional transitions would significantly simplify this block and maintain consistency across the codebase.
AnimatedVisibility(
visible = isVisible,
enter = if (disableAnimations) EnterTransition.None else fadeIn() + scaleIn(initialScale = 1.5f),
exit = if (disableAnimations) ExitTransition.None else fadeOut() + when (focusMeteringUiState.status) {
FocusMeteringUiState.Status.SUCCESS -> scaleOut(targetScale = 0.5f)
FocusMeteringUiState.Status.FAILURE -> scaleOut(targetScale = 1.5f)
else -> fadeOut()
},
modifier = Modifier
.offset { tapCoords.round() }
// Offset the indicator to be centered on the tap coordinates
.offset(-TAP_TO_FOCUS_INDICATOR_SIZE / 2, -TAP_TO_FOCUS_INDICATOR_SIZE / 2)
) {
Box(
Modifier
.testTag(FOCUS_METERING_INDICATOR_TAG)
.alpha(
if (focusMeteringUiState.status == FocusMeteringUiState.Status.SUCCESS) {
1f
} else {
alpha
}
)
.border(
1.dp,
Color.White,
CircleShape
)
.size(TAP_TO_FOCUS_INDICATOR_SIZE)
)
}References
- Simplify Complex Logic: Look for needlessly complex code. If a multi-line block of logic can be condensed into a more concise and readable idiomatic expression, suggest the simplification. (link)
Problem
Continuous animations (like screen flash, focus pulse, audio amplitude) keep the Compose runtime busy, causing Espresso's idle checks to fail or time out on slow emulators.
Solution
Implemented a
LocalDisableAnimationsCompositionLocalto provide a flag to disable or snap animations.MainActivitybased on an intent extra.UiTestUtilviaTestStoragearguments passed from the build configuration.Impact
Drastically reduces CPU load and Espresso timeouts by stopping background animations during tests.