What's New: build the changelog lazily, as iOS already does - #1014
Merged
Conversation
The sheet composed EVERY release card up front on each open. There are 267 entries, each a surface plus a version/date row plus one row per bullet, so roughly a thousand text nodes measured in the frame the sheet appears - and one more release every time we ship. Not a new idea: the Swift twin already uses LazyVStack and says why in a PERF comment. This is the Android half of that fix, which was missed. Swept for the same shape elsewhere first - WorkoutsScreen is the other list that grows without bound, and it was already solved by pagination (#797), so this is the only remaining instance. contentPadding, NOT Modifier.padding: the old padding was applied AFTER verticalScroll, so it scrolled with the content. contentPadding preserves that exactly; a padding modifier would inset the viewport instead and move where content clips at the edges. That is the easy thing to get wrong in a Column-to-LazyColumn swap. Keyed by release.version - unique and stable - mirroring the Swift ForEach id. Arrangement.spacedBy carries over unchanged, so spacing is untouched. Visually identical; only construction becomes on demand. No measurement: there is no Compose runtime here, so the magnitude is a structural estimate rather than a number.
LazyColumn THROWS IllegalArgumentException on a duplicate key. The forEachIndexed this replaces rendered duplicates without complaint, so keying by release.version would have handed a generated file the power to crash the What's New screen at runtime. All 266 entries are unique today, checked. But appchangelog-gen.py only guards the NEWEST entry against duplication, so a re-released version or a hand-edit would be enough, and the failure would land on a user-facing screen rather than in CI. Keys exist so recomposition can track identity when a list inserts, removes or reorders. AppChangelog.releases is a compile-time constant that cannot change during a session, so there is no identity to track and the key was pure cost. The Swift twin's `id:` is not a precedent for keeping one: SwiftUI's ForEach requires an identifier, LazyColumn does not.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Finishes a fix the project already made on iOS.
What was wrong
WhatsNewSheetusedColumn+verticalScroll, which composes every release card up front on each open. There are 267 entries, each a surface plus a version/date row plus one row per bullet — on the order of a thousand text nodes measured in the frame the sheet appears. And one more release every time we ship.Not a new idea
The Swift twin already does this and left the reasoning in the file:
Someone judged it worth fixing on the platform where they could watch it happen, and the Android half was missed. This is that half.
Swept for the same shape before fixing the one instance
Other
verticalScroll+forEachscreens iterate bounded collections — settings rows, wizard steps, a day's tiles. The only other list that grows without bound isWorkoutsScreen, and it was already solved by pagination in #797 ("This card lives inside ONE LazyColumn item"). So the project has now handled this pattern in all three places it occurs.The detail that is easy to get wrong
contentPadding, notModifier.padding. The old padding was applied afterverticalScroll, so it scrolled with the content.contentPaddingpreserves that exactly; a padding modifier would inset the viewport instead and change where content clips at the edges.Arrangement.spacedBy(Metrics.sectionGap)carries over unchanged, so spacing is untouched.Parity
iOS needs nothing —
WhatsNewViewalready usesLazyVStack. This PR brings Android level with it rather than introducing a divergence.No stored value or analytic changes, so the byte-identical contract is not involved.
Verification
src/main/javacompile against amainworktree — 2517 errors both sides, 5 in this file both sides, error sets byte-identical. No new errors.Tools/doc_comment_lint.pyclean;Tools/i18n_audit.py --ciclean.rememberScrollState,verticalScroll) had no other use in the file.What I did not do
Measure it. There is no Compose runtime here, so the magnitude is a structural estimate — ~267 cards composed eagerly versus ~4 visible plus prefetch — not a number. It would be worth a frame-time look on a mid-range device if anyone has one to hand, but the direction is not in question and iOS already committed to it.
The change is visually identical: header, hairlines, footer, card styling and the "newest is the headline" rule are all untouched. Only construction becomes on demand.
Re-review: the item keys were a crash waiting to happen
My first version keyed the items by
release.version.LazyColumnthrowsIllegalArgumentExceptionon a duplicate key, where theforEachIndexedit replaces rendered duplicates without complaint — so that would have handed a generated file the power to crash the What's New screen at runtime.All 266 entries are unique today; I checked. But
appchangelog-gen.pyonly guards the newest entry against duplication, so a re-released version or a hand-edit is all it would take, and the failure would land on a user-facing screen rather than in CI.Keys are now dropped entirely. They exist so recomposition can track identity when a list inserts, removes or reorders —
AppChangelog.releasesis a compile-time constant that cannot change during a session, so there was no identity to track and the key was pure cost.Worth naming the bad reasoning that put it there: I copied the Swift
ForEach(… id: \.element.id)as though it were evidence that a key was wanted. It is not — SwiftUI'sForEachrequires an identifier,LazyColumndoes not. Mirroring a twin is only a good argument when the constraint is the same on both sides.