Animations: Remove "phrases" and sync markdown UI elements with text #42
Conversation
|
|
||
| Layout(content = { | ||
| val alpha = rememberMarkdownFade(richTextRenderOptions, markdownAnimationState) | ||
| Layout(modifier = Modifier.alpha(alpha.value), content = { |
There was a problem hiding this comment.
Using .graphicsLayer { } block to defer animated value reads to the draw phase would improve perf
| mutableFloatStateOf(if (richTextRenderOptions.animate) 0f else 1f) | ||
| } | ||
| LaunchedEffect(Unit) { | ||
| targetAlpha.value = 1f |
There was a problem hiding this comment.
I see this code was just moved, but we would skip a recomposition step by using a simple animate call to update the state value instead of animateFloatAsState, and also prevents us from creating something like an Animatable in cases we don't need to animate.
| } | ||
| // Remove animation right away, in case it had split at an inappropriate unicode point. | ||
| animations.remove(phraseIndex) | ||
| fun updateText(newText: AnnotatedString) { |
There was a problem hiding this comment.
Only called in the launched effect? Maybe we should inline it?
| animations.remove(phraseIndex) | ||
| fun updateText(newText: AnnotatedString) { | ||
| val currentText = lastText.value | ||
| if (newText.text == currentText.text) return |
There was a problem hiding this comment.
What do you think about letting the launched effect key on the text value instead?
| val startIndex = commonPrefixLength(currentText.text, newText.text) | ||
| lastText.value = newText | ||
| textToRender.value = newText | ||
| animations.clear() |
There was a problem hiding this comment.
Why do we clear the other animations? Maybe they are still running, and they won't draw their updates anymore.
| fun updateText(newText: AnnotatedString) { | ||
| val currentText = lastText.value | ||
| if (newText.text == currentText.text) return | ||
| val startIndex = commonPrefixLength(currentText.text, newText.text) |
There was a problem hiding this comment.
I guess we might kind of lose our on the granular animation now, but it would feel snappier in general since text would come in faster after delays?
There was a problem hiding this comment.
yeah i don't like these changes once i tried them in prod, backing them out
| ) | ||
| ) | ||
| LaunchedEffect(Unit) { | ||
| markdownAnimationState.addAnimation(richTextRenderOptions) |
There was a problem hiding this comment.
addAnimation only ticks the delay counter up, we don't need to unregister it globally
|
going through these comments and moving them to #43 |
We don't use the "phrases" concept anymore and we'd like to sync markdown decorations. This generalizes well and removes some stale code. hopefully fixes some bugs along the way.