Optimize ElapsedTimeText recompositions and enable tabular figures#515
Optimize ElapsedTimeText recompositions and enable tabular figures#515temcguir wants to merge 14 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the elapsed time display logic to use a lambda provider and introduces tabular figures for the timer text to prevent layout jitter. While the changes aim to improve performance, the reviewer identified that reading the state value at the top level of the PreviewScreen composable still triggers unnecessary recompositions, defeating the intended optimization. Feedback also highlights a hardcoded format string that should be moved to resources, outdated KDoc documentation, and a suggestion to use LocalTextStyle to avoid overriding theme defaults. Finally, a minor cleanup is requested to replace a fully qualified name with an import.
temcguir
force-pushed
the
temcguir/elapsed_time_opt
branch
from
4b07289 to
9a8a270
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the PreviewScreen to use a provider pattern for CaptureUiState.Ready, which optimizes how state is passed to child components. It also updates the ElapsedTimeText component to use a provider for formatted time and applies tabular numbers for consistent character width. Review feedback suggests using a safe cast within the captureUiStateProvider to prevent potential crashes during state transitions and extracting the hardcoded time format string to strings.xml to support localization and adhere to the project's style guide.
temcguir
changed the title
temcguir
force-pushed
the
temcguir/elapsed_time_opt
branch
from
d2c801f to
999349e
Compare
…sedTimeUiState directly.
temcguir
force-pushed
the
temcguir/elapsed_time_opt
branch
from
999349e to
1894f13
Compare
temcguir
force-pushed
the
temcguir/elapsed_time_opt
branch
from
1894f13 to
43c5368
Compare
Kimblebee
left a comment
Kimblebee
left a comment
There was a problem hiding this comment.
I think we could consider the tradeoffs of passing everything into a lambda vs decoupling high-frequency values (like elapsedTimeNanos) from the UI state and exposing it as a separate flow?
There was a problem hiding this comment.
As something adjacent to this PR's changes, could this be optimized as well? Wouldnt the updates to elapsed time cause this LaunchedEffect to repeatedly trigger while recording?
| textAlign = TextAlign.Center | ||
| text = formattedTime, | ||
| textAlign = TextAlign.Center, | ||
| style = androidx.compose.material3.LocalTextStyle.current.copy(fontFeatureSettings = "tnum") |
There was a problem hiding this comment.
nit: can we clean this up and import LocalTextStyle at the top of the class like the others?
| * @param formattedTimeProvider a provider for the formatted time string. | ||
| */ | ||
| @Composable | ||
| fun ElapsedTimeText(modifier: Modifier = Modifier, elapsedTimeUiState: ElapsedTimeUiState) { |
There was a problem hiding this comment.
The updated param Kdoc is incongruent with the funciton signature
# Conflicts: # .gemini/styleguide.md
…ean up imports and update KDoc/signature for ElapsedTimeText
2 participants
Problem
Reading the elapsed time state at the root of
PreviewScreencaused the entire screen to recompose on every timer tick. This added unnecessary load to the Compose runtime.Solution
ContentScreenandLayoutWrapperto pass a lambda provider() -> CaptureUiState.Readyinstead of the raw state.derivedStateOfinPreviewScreento lazily evaluate the state.ElapsedTimeTextwithin its slot, deferring the state read to the component level.Impact
Isolated recompositions to just the components that need the state, improving performance during recording.