From b1b9a0e5f52827f25afcecf6220ac518c0da151a Mon Sep 17 00:00:00 2001 From: Zach Klippenstein Date: Mon, 27 Nov 2023 10:01:49 -0800 Subject: [PATCH] Use pure coroutines to animate cursor instead of Compose animation APIs. Fixes for both BTF1 and BTF2. This change also manually observes window focus to stop the cursor animation when the window loses focus or the screen is turned off. This wasn't needed before because the animation framework automatically paused animations when the window lost focus. Bug: b/249422803 Relnote: "Cursor animation no longer requests frames between on and off states." Test: CursorAnimationStateTest Test: TextFieldCursorTest Change-Id: Ia22537827b1735a0c2f6f5c732bebf79aaa3b773 --- .../internal/CursorAnimationStateTest.kt | 166 ++++++++++++++++++ .../foundation/text/TextFieldCursor.kt | 23 +-- .../input/internal/CursorAnimationState.kt | 96 ++++++++++ .../input/internal/TextFieldCoreModifier.kt | 55 ++---- 4 files changed, 288 insertions(+), 52 deletions(-) create mode 100644 compose/foundation/foundation/src/androidUnitTest/kotlin/androidx/compose/foundation/text2/input/internal/CursorAnimationStateTest.kt create mode 100644 compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/text2/input/internal/CursorAnimationState.kt diff --git a/compose/foundation/foundation/src/androidUnitTest/kotlin/androidx/compose/foundation/text2/input/internal/CursorAnimationStateTest.kt b/compose/foundation/foundation/src/androidUnitTest/kotlin/androidx/compose/foundation/text2/input/internal/CursorAnimationStateTest.kt new file mode 100644 index 0000000000000..58920e69a7ab7 --- /dev/null +++ b/compose/foundation/foundation/src/androidUnitTest/kotlin/androidx/compose/foundation/text2/input/internal/CursorAnimationStateTest.kt @@ -0,0 +1,166 @@ +/* + * Copyright 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package androidx.compose.foundation.text.input.internal + +import com.google.common.truth.Truth.assertThat +import kotlin.test.Test +import kotlinx.coroutines.CoroutineStart +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runCurrent +import kotlinx.coroutines.test.runTest +import org.junit.runner.RunWith +import org.junit.runners.JUnit4 + +@OptIn(ExperimentalCoroutinesApi::class) +@RunWith(JUnit4::class) +class CursorAnimationStateTest { + + private val animationState = CursorAnimationState() + + @Test + fun alphaNotAnimatingInitially() = runTest { + assertNotAnimating() + } + + @Test + fun snapToVisibleAndAnimate_animatesAlpha() = runTest { + val job = launch { + animationState.snapToVisibleAndAnimate() + } + + // Should start immediately. + assertThat(animationState.cursorAlpha).isEqualTo(0f) + runCurrent() + + // Then let's verify a few blinks… + assertThat(animationState.cursorAlpha).isEqualTo(1f) + testScheduler.advanceTimeBy(500) + assertThat(animationState.cursorAlpha).isEqualTo(1f) + testScheduler.advanceTimeBy(500) + assertThat(animationState.cursorAlpha).isEqualTo(0f) + testScheduler.advanceTimeBy(500) + assertThat(animationState.cursorAlpha).isEqualTo(1f) + + job.cancel() + } + + @Test + fun snapToVisibleAndAnimate_suspendsWhileAnimating() = runTest { + val job = launch(start = CoroutineStart.UNDISPATCHED) { + animationState.snapToVisibleAndAnimate() + } + + // Advance a few blinks. + repeat(10) { + testScheduler.advanceTimeBy(500) + assertThat(job.isActive).isTrue() + } + + job.cancel() + } + + @Test + fun snapToVisibleAndAnimate_stopsAnimating_whenCancelledImmediately() = runTest { + val job = launch(start = CoroutineStart.UNDISPATCHED) { + animationState.snapToVisibleAndAnimate() + } + job.cancel() + + assertNotAnimating() + assertThat(job.isActive).isFalse() + } + + @Test + fun snapToVisibleAndAnimate_stopsAnimating_whenCancelledAsync() = runTest { + val job = launch { + animationState.snapToVisibleAndAnimate() + } + job.cancel() + + assertNotAnimating() + assertThat(job.isActive).isFalse() + } + + @Test + fun snapToVisibleAndAnimate_stopsAnimating_whenCancelledAfterAWhile() = runTest { + val job = launch(start = CoroutineStart.UNDISPATCHED) { + animationState.snapToVisibleAndAnimate() + } + + // Advance a few blinks… + repeat(10) { + testScheduler.advanceTimeBy(500) + } + job.cancel() + + assertNotAnimating() + } + + @Test + fun cancelAndHide_stopsAnimating_immediately() = runTest { + val job = launch(start = CoroutineStart.UNDISPATCHED) { + animationState.snapToVisibleAndAnimate() + } + animationState.cancelAndHide() + + assertNotAnimating() + assertThat(job.isActive).isFalse() + } + + @Test + fun cancelAndHide_beforeStart_doesntBlockAnimation() = runTest { + animationState.cancelAndHide() + val job = launch { + animationState.snapToVisibleAndAnimate() + } + + runCurrent() + assertThat(animationState.cursorAlpha).isEqualTo(1f) + + job.cancel() + } + + @Test + fun cancelAndHide_stopsAnimating_afterAWhile() = runTest { + val job = launch(start = CoroutineStart.UNDISPATCHED) { + animationState.snapToVisibleAndAnimate() + } + + // Advance a few blinks… + repeat(10) { + testScheduler.advanceTimeBy(500) + } + animationState.cancelAndHide() + + assertNotAnimating() + assertThat(job.isActive).isFalse() + } + + private fun TestScope.assertNotAnimating() { + // Allow the cancellation to process. + advanceUntilIdle() + + // Verify a few blinks. + repeat(10) { + assertThat(animationState.cursorAlpha).isEqualTo(0f) + testScheduler.advanceTimeBy(490) + } + } +} diff --git a/compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/text/TextFieldCursor.kt b/compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/text/TextFieldCursor.kt index 4cadd14b04470..80f9cafdeb2e5 100644 --- a/compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/text/TextFieldCursor.kt +++ b/compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/text/TextFieldCursor.kt @@ -16,10 +16,10 @@ package androidx.compose.foundation.text -import androidx.compose.animation.core.Animatable import androidx.compose.animation.core.AnimationSpec import androidx.compose.animation.core.infiniteRepeatable import androidx.compose.animation.core.keyframes +import androidx.compose.foundation.text2.input.internal.CursorAnimationState import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.remember import androidx.compose.ui.Modifier @@ -36,11 +36,8 @@ import androidx.compose.ui.graphics.isUnspecified import androidx.compose.ui.platform.LocalWindowInfo import androidx.compose.ui.text.input.OffsetMapping import androidx.compose.ui.text.input.TextFieldValue -import androidx.compose.ui.unit.Density import androidx.compose.ui.unit.Dp -import androidx.compose.ui.unit.dp import kotlin.math.floor -import kotlinx.coroutines.withContext internal fun Modifier.cursor( state: TextFieldState, @@ -49,22 +46,20 @@ internal fun Modifier.cursor( cursorBrush: Brush, enabled: Boolean ) = if (enabled) composed { - val cursorAlpha = remember { Animatable(1f) } + val cursorAnimation = remember { CursorAnimationState() } + // Don't bother animating the cursor if it wouldn't draw any pixels. val isBrushSpecified = !(cursorBrush is SolidColor && cursorBrush.value.isUnspecified) + // Only animate the cursor when its window is actually focused. This also disables the cursor + // animation when the screen is off. + // TODO confirm screen-off behavior. val isWindowFocused = LocalWindowInfo.current.isWindowFocused - if (state.hasFocus && value.selection.collapsed && isBrushSpecified && isWindowFocused) { + if (isWindowFocused && state.hasFocus && value.selection.collapsed && isBrushSpecified) { LaunchedEffect(value.annotatedString, value.selection) { - // Animate the cursor even when animations are disabled by the system. - withContext(FixedMotionDurationScale) { - // ensure that the value is always 1f _this_ frame by calling snapTo - cursorAlpha.snapTo(1f) - // then start the cursor blinking on animation clock (500ms on to start) - cursorAlpha.animateTo(0f, cursorAnimationSpec) - } + cursorAnimation.snapToVisibleAndAnimate() } drawWithContent { this.drawContent() - val cursorAlphaValue = cursorAlpha.value.coerceIn(0f, 1f) + val cursorAlphaValue = cursorAnimation.cursorAlpha if (cursorAlphaValue != 0f) { val transformedOffset = offsetMapping .originalToTransformed(value.selection.start) diff --git a/compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/text2/input/internal/CursorAnimationState.kt b/compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/text2/input/internal/CursorAnimationState.kt new file mode 100644 index 0000000000000..69faf49d07f08 --- /dev/null +++ b/compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/text2/input/internal/CursorAnimationState.kt @@ -0,0 +1,96 @@ +/* + * Copyright 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package androidx.compose.foundation.text2.input.internal + +import androidx.compose.foundation.AtomicReference +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableFloatStateOf +import androidx.compose.runtime.setValue +import kotlinx.coroutines.Job +import kotlinx.coroutines.cancelAndJoin +import kotlinx.coroutines.coroutineScope +import kotlinx.coroutines.delay +import kotlinx.coroutines.launch + +/** + * Holds the state of the animation that blinks the cursor. + * + * We can't use the Compose Animation APIs because they busy-loop on delays, and this animation + * spends most of its time delayed so that's a ton of wasted frames. Pure coroutine delays, however, + * will not cause any work to be done until the delay is over. + */ +internal class CursorAnimationState { + + private var animationJob = AtomicReference(null) + + /** + * The alpha value that should be used to draw the cursor. + * Will always be in the range [0, 1]. + */ + var cursorAlpha by mutableFloatStateOf(0f) + private set + + /** + * Immediately shows the cursor (sets [cursorAlpha] to 1f) and starts blinking it on and off + * every 500ms. If a previous animation was running, it will be cancelled before the new one + * starts. + * + * Won't return until the animation cancelled via [cancelAndHide] or this coroutine's [Job] is + * cancelled. In both cases, the cursor will always end up hidden. + */ + suspend fun snapToVisibleAndAnimate() { + coroutineScope { + // Can't do a single atomic update because we need to get the old value before launching + // the new coroutine. So we set to null first, and then launch only if still null (i.e. + // no other caller beat us to starting a new animation). + val oldJob = animationJob.getAndSet(null) + + // Even though we're launching a new coroutine, because of structured concurrency, the + // restart function won't return until the animation is finished, and cancelling the + // calling coroutine will cancel the animation. + animationJob.compareAndSet(null, launch { + // Join the old job after cancelling to ensure it finishes its finally block before + // we start changing the cursor alpha, so we don't end up interleaving alpha + // updates. + oldJob?.cancelAndJoin() + + // Start the new animation and run until cancelled. + try { + while (true) { + cursorAlpha = 1f + // Ignore MotionDurationScale – the cursor should blink even when animations + // are disabled by the system. + delay(500) + cursorAlpha = 0f + delay(500) + } + } finally { + // Hide cursor when the animation is cancelled. + cursorAlpha = 0f + } + }) + } + } + + /** + * Immediately cancels the cursor animation and hides the cursor (sets [cursorAlpha] to 0f). + */ + fun cancelAndHide() { + val job = animationJob.getAndSet(null) + job?.cancel() + } +} diff --git a/compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/text2/input/internal/TextFieldCoreModifier.kt b/compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/text2/input/internal/TextFieldCoreModifier.kt index 87c5432182b74..220559b385371 100644 --- a/compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/text2/input/internal/TextFieldCoreModifier.kt +++ b/compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/text2/input/internal/TextFieldCoreModifier.kt @@ -16,10 +16,6 @@ package androidx.compose.foundation.text2.input.internal -import androidx.compose.animation.core.Animatable -import androidx.compose.animation.core.AnimationSpec -import androidx.compose.animation.core.infiniteRepeatable -import androidx.compose.animation.core.keyframes import androidx.compose.foundation.ExperimentalFoundationApi import androidx.compose.foundation.ScrollState import androidx.compose.foundation.gestures.Orientation @@ -29,7 +25,6 @@ import androidx.compose.foundation.text2.BasicTextField2 import androidx.compose.foundation.text2.input.internal.selection.TextFieldSelectionState import androidx.compose.foundation.text2.input.internal.selection.textFieldMagnifierNode import androidx.compose.runtime.snapshotFlow -import androidx.compose.ui.MotionDurationScale import androidx.compose.ui.geometry.Rect import androidx.compose.ui.graphics.Brush import androidx.compose.ui.graphics.Color @@ -52,6 +47,7 @@ import androidx.compose.ui.node.SemanticsModifierNode import androidx.compose.ui.node.currentValueOf import androidx.compose.ui.node.invalidateMeasurement import androidx.compose.ui.platform.InspectorInfo +import androidx.compose.ui.platform.LocalWindowInfo import androidx.compose.ui.semantics.SemanticsPropertyReceiver import androidx.compose.ui.text.TextLayoutResult import androidx.compose.ui.text.TextPainter @@ -68,7 +64,6 @@ import kotlinx.coroutines.CoroutineStart import kotlinx.coroutines.Job import kotlinx.coroutines.flow.collectLatest import kotlinx.coroutines.launch -import kotlinx.coroutines.withContext /** * Modifier element for the core functionality of [BasicTextField2] that is passed as inner @@ -141,7 +136,7 @@ internal class TextFieldCoreModifierNode( * another half a second when TextField is focused and editable. Initial value should be 0f * so that when cursor needs to be drawn for the first time, change to 1f invalidates draw. */ - private val cursorAlpha = Animatable(0f) + private val cursorAnimation = CursorAnimationState() /** * Whether to show cursor at all when TextField has focus. This depends on enabled, read only, @@ -214,7 +209,7 @@ internal class TextFieldCoreModifierNode( if (!showCursor) { changeObserverJob?.cancel() changeObserverJob = null - coroutineScope.launch { cursorAlpha.snapTo(0f) } + cursorAnimation.cancelAndHide() } else if (!wasFocused || previousTextFieldState != textFieldState || !previousShowCursor @@ -222,15 +217,17 @@ internal class TextFieldCoreModifierNode( // this node is writeable, focused and gained that focus just now. // start the state value observation changeObserverJob = coroutineScope.launch { - // Animate the cursor even when animations are disabled by the system. - withContext(FixedMotionDurationScale) { - snapshotFlow { textFieldState.text } - .collectLatest { - // ensure that the value is always 1f _this_ frame by calling snapTo - cursorAlpha.snapTo(1f) - // then start the cursor blinking on animation clock (500ms on to start) - cursorAlpha.animateTo(0f, cursorAnimationSpec) - } + snapshotFlow { + // Read the text state, so the animation restarts when the text or cursor + // position change. + textFieldState.text + // Only animate the cursor when its window is actually focused. This also + // disables the cursor animation when the screen is off. + currentValueOf(LocalWindowInfo).isWindowFocused + }.collectLatest { isWindowFocused -> + if (isWindowFocused) { + cursorAnimation.snapToVisibleAndAnimate() + } } } } @@ -488,12 +485,9 @@ internal class TextFieldCoreModifierNode( private fun DrawScope.drawCursor() { // Only draw cursor if it can be shown and its alpha is higher than 0f // Alpha is checked before showCursor purposefully to make sure that we read - // cursorAlpha.value in draw phase. So, when the alpha value changes, draw phase - // invalidates. - if (cursorAlpha.value <= 0f || !showCursor) return - - val cursorAlphaValue = cursorAlpha.value.coerceIn(0f, 1f) - if (cursorAlphaValue == 0f) return + // cursorAlpha in draw phase. So, when the alpha value changes, draw phase invalidates. + val cursorAlphaValue = cursorAnimation.cursorAlpha + if (cursorAlphaValue == 0f || !showCursor) return val cursorRect = textFieldSelectionState.cursorRect @@ -516,16 +510,6 @@ internal class TextFieldCoreModifierNode( } } -private val cursorAnimationSpec: AnimationSpec = infiniteRepeatable( - animation = keyframes { - durationMillis = 1000 - 1f at 0 - 1f at 499 - 0f at 500 - 0f at 999 - } -) - private val DefaultCursorThickness = 2.dp /** @@ -534,11 +518,6 @@ private val DefaultCursorThickness = 2.dp private val Brush.isSpecified: Boolean get() = !(this is SolidColor && this.value.isUnspecified) -private object FixedMotionDurationScale : MotionDurationScale { - override val scaleFactor: Float - get() = 1f -} - /** * Finds the rectangle area that corresponds to the visible cursor. *