Fix #1363: copy calcPreferredSize() result instead of retaining the reference#5109
Merged
Merged
Conversation
…1363) Component.preferredSizeImpl() retained the Dimension instance returned from calcPreferredSize() as its cached preferredSize. When a subclass's calcPreferredSize() returns a shared Dimension — for example ContainerList.Entry.calcPreferredSize() returning the single renderer component's own preferredSize field — every "cached" preferredSize ends up pointing at the same instance. Re-measuring the renderer for the next entry then silently mutates every previously-measured entry, so all entries collapse to the most recently computed size. Switch to copying the width/height into a Component-owned Dimension: - allocate a new Dimension on first measure - update width/height in place on later re-measures (preserves the cached reference, so callers that already hold getPreferredSize() see the new values without a second lookup) Closes #1363. Adds maven/core-unittests/.../ComponentPreferredSizeIsolationTest.java with two regression cases: - two components whose calcPreferredSize() returns the same Dimension end up with independent preferred-size instances - mutating the source Dimension after measurement does not alter the cached preferred size Full layout/component test sweep (334 tests across Component/Container/InputComponent + Box/Border/Flow/Grid/Layered/ Coordinate/Group/Mig/Table layouts + ContainerList and the SpanLabel+LayeredLayout preferred-size regression at #3000) is green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Cloudflare Preview
|
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Collaborator
Author
|
Compared 122 screenshots: 122 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 122 screenshots: 122 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 122 screenshots: 122 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
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
1 participant
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.
Summary
Closes #1363 — the next-oldest open issue (filed Feb 2015, 9 comments).
Component.preferredSizeImpl()cached theDimensioninstance returned fromcalcPreferredSize()directly. When a subclass'scalcPreferredSize()returns a sharedDimension— for exampleContainerList.Entry.calcPreferredSize()returning the single renderer's ownpreferredSizefield — every entry's "cached" preferred size ends up pointing at the same instance. The next time the renderer is re-measured (for the next entry), every previously-measured entry's cached size silently changes too, and all entries collapse to the most recently computed size. That's the failure mode the 2015 reporter pinpointed in comment #3.The fix is what the reporter proposed in their first comment: copy the width/height into the component's own
Dimensioninstead of swapping the reference. To keep callers that have already retainedgetPreferredSize()working (so they observe the updated values rather than a stale snapshot), we update the existingDimensionin place on re-measure and only allocate on the first measure.Test plan
ComponentPreferredSizeIsolationTestcovers two cases:calcPreferredSize()returns the sameDimensionend up with independent preferred-size instances (assertNotSame).Dimensionafter the firstgetPreferredSize()call does not alter the cached value.expected: <40> but was: <999>andexpected: not same but was: <width = 40 height = 20>).Component,Container,InputComponent, the full layout suite (Box/Border/Flow/Grid/Layered/Coordinate/Group/Mig/Table),ContainerListand the existing SpanLabel Height incorrect setAllowEnableLayoutOnPaint Regression #3000 SpanLabel/LayeredLayout preferred-size regression.Behaviour change for consumers
calcPreferredSize()returns a uniqueDimensionevery call (the vast majority —Label.calcPreferredSize()for example usesgetUIManager().getLookAndFeel().getLabelPreferredSize(this)which returns a freshDimension) see no observable change.calcPreferredSize()returns a sharedDimension(the rare case that triggered com.codename1.ui.Component.preferredSizeImpl() is unsafe as it sets the preferredSize from calcPreferredSize() which may be overridden #1363) now correctly get their own cached values that don't mutate from under them.getPreferredSize()continue to observe live values because we update the cachedDimensionin place rather than swapping the reference on re-measure.🤖 Generated with Claude Code