A queued animation lost its updateState(), so a deferred add or remove never happened (issue #5606) - #5870
Merged
Conversation
…e never happened (issue #5606) AnimationManager.updateAnimations() steps the head of its queue only while isInProgress() is true and otherwise completes it without an update. That is right for an animation that already ran, and wrong for one whose isInProgress() is false from the moment it is queued: that animation was never stepped, so its entire payload is still sitting in updateState(). Core queues three of those, and all three were dead: - Container.insertComponentAt - the deferred insertion taken while another animation is in flight. cmp.setParent(this) is set optimistically and the component never enters components, so cnt.add(x) during an animation was a silent no-op. - Container.removeComponentImpl - the matching deferred removal. The component is detached from the layout and its parent nulled, but never taken out of components, so it kept painting. - RefreshThemeCallback, which Container.wrapInLayeredPane() uses to re-root the content pane. This one was the worst of the three: unlike the other two it does not override flush(), so there was no path on which it ran at all. The wrapper was left holding the content pane while nothing held the wrapper, so Form.getActualPane() returned a container the form never painted and every InteractionDialog - the lightweight Picker popup among them - opened into a layered pane that could not appear on screen. So adding or removing a component, or asking for a layered pane, while any animation was running did not do what it said. completeAnimation() now gives a never-stepped animation exactly one updateState() before completing, tracked by a `stepped` flag rather than by `completed`, which completeIfNeeded() sets for both cases. CompoundAnimation cascades instead of taking one aggregate update. A single updateState() cannot stand in for the children: isInProgress() walks the sequence cursor past the end as it finds every child finished, so one update would land on the last child and apply nothing else - a never-started sequentialAnimation() would apply only its last child. Cascading also covers the partially stepped case, where the compound was stepped but a child that was never in progress got skipped as the cursor walked over it. The sequential branch of updateState() now marks the child it steps, so a child stepped through that path is not applied a second time on completion. This is a regression from 9ed10e0, which changed the one line in updateAnimations() and added AnimationManagerTest.testAlreadyFinishedAnimation- RunsCompletionWithoutUpdateState to pin it. That test builds an animation whose isInProgress() is false from the start - structurally the same thing Container queues - and asserts it must not be updated, so it asserted the defect. It is replaced here by one that asserts the payload is applied exactly once. Its sibling, testFinishedAnimationDoesNotUpdateStateAgainBeforeRemoval, covers the concern that commit was really about - not stepping a finished animation past its end - and is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Developer Guide build artifacts are available for download from this workflow run:
Developer Guide quality checks: |
Contributor
Cloudflare Preview
|
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
#5606) Form.getFormLayeredPane() builds the overlay pane with a painter that draws the form underneath itself as its own background: protected void paintBackground(Graphics g) { if (getComponentCount() > 0) { if (super.isVisible()) { super.setVisible(false); Form.this.paint(g); super.setVisible(true); } } } That is right when the pane is painted on its own - a repaint targeting just the pane would otherwise composite its children over stale pixels - and wrong during the form's own paint pass, where the form has already drawn everything beneath the pane, because the pane is one of its children. Drawing it again there paints the whole tree a second time into one frame. Opaque fills are idempotent, which is why this was invisible in everything that is filled: the title bar, a field border, the popup itself. Every translucent pixel composites twice and comes out darker - antialiased glyphs and the toolbar's drop shadow. The guard is why nobody has hit it. The pane has no children until something adds itself to it, and the thing that does - InteractionDialog, which is what the lightweight Picker popup is - arrives through Container's deferred insertion, which was dropped entirely by the defect fixed in the previous commit. A pane that never had children never ran this painter, so fixing the deferred insertion opened this path for the first time and the Android ValidatorLightweightPicker screenshot came back with its text and the toolbar shadow composited twice. inInternalPaint marks the form's own pass and is the same discriminator Form.paint() already uses to keep from drawing its background twice, so the guard becomes `getComponentCount() > 0 && !inInternalPaint`. Measured on an API 36 emulator, with everything except ComponentAnimation.java and Form.java held byte-identical between arms: master 0 double paints deferred-insert fix only 165 double paints, screenshot differs from baseline both fixes 0 double paints, screenshot byte-identical to master The second test covers the case the painter exists for, so that deleting the painter outright cannot pass. It paints the pane itself rather than the sub-pane getFormLayeredPane() hands out, because painting a child of the pane also runs every ancestor painter on the way down and that count is not the property under test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
Collaborator
Author
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
…uble paint Both of these put their content in the form level layered pane with no animation in flight, so the insertion took the immediate path and the pane already had children. The guard in its background painter was therefore already open on master, and the form was already being painted twice into every frame of these two screenshots. Unlike the Picker popup - which never got its content into the pane at all, because the deferred insertion was dropped - this was not latent. The baselines are what a double composite looks like. The new captures differ from them only by not compositing twice. Every single changed pixel is lighter, on both platforms and in both files: android AppReviewDialog 3063 px changed, 3063 lighter, 0 darker android Sheet 3327 px changed, 3327 lighter, 0 darker macos AppReviewDialog 1756 px changed, 1756 lighter, 0 darker macos Sheet 1615 px changed, 1615 lighter, 0 darker That is the exact inverse of the ValidatorLightweightPicker diff this branch started from, which was 100% darker, and it is confined to antialiased glyph edges - the layout, the sheet, the stars and the title are identical. Only the android and macos legs run on a pull request that touches CodenameOne/src, so only those two are reseeded from a measured capture here. The same two files exist for ios, linux, windows, mac-catalyst and javascript and have to be checked on their own legs rather than guessed at. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…changes Both legs compare stored screenshots, and both listed only their own port, the VM and the sample app. Neither listed the core renderer, and neither listed its native theme - the two things that most directly decide what those screenshots look like. A pull request that changed core rendering therefore never ran them, and the first thing to notice was the nightly, on master, after the change had already landed. That is not hypothetical. The Sheet and AppReviewDialog baselines were sitting on a double composite (issue #5606): the form level layered pane repainted the whole form as its own background during the form's own paint pass, and the captures recorded it. The android and macos legs caught it on the pull request because they do list CodenameOne/src; linux and windows would only have found it after the merge. Also added is each leg's own screenshot directory. A commit that reseeds a baseline should be checked by the leg that produced it, and neither scripts/windows nor the push half of scripts/linux was listed. The macos leg already lists exactly this set - core, port, native theme, its own scripts directory - so this brings the two stragglers in line rather than inventing a rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21ffd0b08c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+141
to
142
| applyPendingState(); | ||
| completeIfNeeded(); |
There was a problem hiding this comment.
When an animation reports isInProgress() == false from enqueue time, this new call means it still has pending work, but AnimationManager.isAnimating() returns false when it is the sole queue entry and addAnimationAndBlock() also stops waiting solely on that predicate. Consequently flushAnimation() can run its callback—or addAnimationAndBlock() can return—before updateAnimations() reaches this line and performs the deferred mutation, so callers can observe or manipulate the old component tree even though the requested animation has not completed. The manager needs to treat the new unstepped state as pending work for its blocking and flush decisions.
Useful? React with 👍 / 👎.
Comment on lines
+272
to
+275
| /// progress got skipped as the cursor walked over it. | ||
| @Override | ||
| void applyPendingState() { | ||
| for (ComponentAnimation a : anims) { |
There was a problem hiding this comment.
For a sequential compound whose first child reports false immediately and whose next child is active, isInProgress() advances the cursor past the first child, so the later child runs and the compound's completion callback fires before this loop is reached on the removal tick. This loop then applies the skipped first child after the later animation and after completion, reversing the promised sequence order; immediate children need to be applied as the sequence cursor advances rather than swept only at the end.
Useful? React with 👍 / 👎.
Collaborator
Author
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 166 screenshots: 166 matched. |
Collaborator
Author
|
Compared 166 screenshots: 166 matched. |
Collaborator
Author
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 160 screenshots: 160 matched. Benchmark Results
Detailed Performance Metrics
|
The same two captures, recording the same double paint the android and macos baselines did, and corrected the same way: every changed pixel is lighter and none is darker. mac-catalyst AppReviewDialog 887 px changed, 887 lighter, 0 darker mac-catalyst Sheet 1331 px changed, 1331 lighter, 0 darker Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AppReviewDialog is the same correction the android, macos and mac-catalyst baselines took: 3389 px changed, every one of them lighter, none darker - the form no longer painted twice into one frame. Sheet carries that same lighter band and one thing more, a 1px full width rule under the sheet's header that the old baseline does not have. That rule is not a side effect of the paint fix. Measured on an iOS Metal simulator, with the port, the plugin and core all rebuilt from this branch and the two arms differing by exactly one file: both fixes rule present deferred-insert fix only rule present master (the stored baseline) rule absent The two local arms differ by 4944 px, all of them lighter and none darker, which is the paint fix and nothing else - so reverting it does not take the rule away. What puts the rule there is the deferred insertion being repaired. SheetScreenshotTest shows a child sheet while the parent sheet's slide up animation is still running, so the child's attachment takes Container's deferred branch - the one whose updateState() was dropped. On master part of that never happened and the header rendered without its rule. The rule appearing is the defect being fixed, which is why the baseline has to move rather than the code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…the synchronous one Issue #4912 ("form remains shaded after closing the Toolbar side menu") was fixed once and came back, and the customer report behind #5606 is the same symptom again: navigate through an on-top side menu and the form you come back to is dimmed for good. Both regression tests written for that symptom -- #4912's closeLeftSideMenuClearsShadedBackdropAfterAnimation and #4979's sideMenuCommandFiresAfterLayeredPaneDetach -- disable the dispose animation by reflection so detachToolbarLayeredPane runs synchronously inside closeLeftSideMenu. That is the one path that never had the bug, so neither test could see it return. On the animated path the detach runs from the dispose animation's completion callback, so its cnt.remove() is a mutation made while the animation manager is running. With anything else still in the queue at that moment Container takes it as a deferred removal, whose whole payload lives in updateState() -- and updateAnimations() only steps the head while isInProgress() is true, which a deferred mutation never is. The dim pane was therefore completed without ever being applied and stayed on the form: a black fill at ~31% over the whole form, and a non-empty form level layered pane, which makes that pane paint the entire form a second time inside the form's own pass and composite every translucent pixel twice. The new test drives the queue through Form.repaintAnimations() the way Display's EDT loop does rather than through AnimationManager.flush() -- flush() is only reached on form deinitialize and applies each animation directly, which is precisely why draining with it hides this. Red on master, green here. Verified that reverting 9ed10e0's single line in AnimationManager.updateAnimations() also turns it green, which is what identifies that commit as the regression. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
shai-almog
added a commit
that referenced
this pull request
Sep 20, 2026
Two binary conflicts, both hellocodenameone macOS goldens, and both sides had changed them for a real reason: this branch reseeded the whole macOS set for the Aqua theme flip (c957f8c, run 35395172908), and master's #5870 changed the dialog title from bold to regular weight while fixing a queued animation that lost its updateState(). So NEITHER side's bytes are right for the merged tree, which has the Aqua theme AND that fix. Resolved to ours because the theme flip rewrites 75-78% of each image while master's delta is 0.3% in the title, so taking master's would leave two tiles inconsistent with the other ~158 in the set. These two therefore still encode the pre-#5870 title weight and have to be re-captured from a macOS run on this merged head. That is not left to memory: scripts-macos.yml compares the runner's render against these goldens, so if the title weight does change under Aqua the job fails and names them. Verified across the merge rather than assumed: no conflict markers, no file deleted relative to the branch tip, this branch's Form.java desktop work (escapeConsumedOnPress, desktopKeyReleased, isReachableForTraversal, DesktopTabIteratorFilter) intact beside master's layered-pane paint change, and port_status.json lost no id and no screenshot mapping from either side while gaining master's foldable-posture entries. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
shai-almog
added a commit
that referenced
this pull request
Sep 20, 2026
From linux-build-run run 35498811062, the first run on the merge commit, both arches. Master fixed a queued animation that lost its updateState() and, in the same change, stopped FormLayeredPane drawing the form beneath it during the form's own paint pass -- which had been painting the whole tree a second time into the same frame. Opaque fills survive that unchanged. ANTI-ALIASED TEXT DOES NOT, and that is the whole of this diff: identical layout, identical glyph positions, 1444 and 1751 pixels differing by at most 36, all of them text edges plus one border row, with the capture LIGHTER than the golden in every case (Sheet 138.6 against 127.4, AppReviewDialog 142.9 against 132.8, byte-identical numbers on x64 and arm64). Heavier text is the double paint; the capture is the single one. So these four goldens encoded the bug. Master reseeded exactly the same two tests for android, mac-catalyst and macos in that commit; its Linux and Windows sets did not move, and this branch's Linux set does because it renders under Adwaita. Windows cross-build passed unchanged on the same merge commit. Not a tolerance question and no sidecar was added: the old bytes are wrong, not noisy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
shai-almog
added a commit
that referenced
this pull request
Sep 20, 2026
From scripts-macos run 35502304966. These are the two files the merge with master conflicted on, and the resolution took OURS deliberately -- the Aqua flip had rewritten 75-78% of each image and master's delta was 0.3%, so taking master's would have left two tiles rendered in the old theme while the other ~158 were Aqua. The commit said they still encoded the pre-#5870 title weight and would have to be re-captured, and that the macOS job would name them rather than the point being left to memory. It named them. Same signature as the Linux pair, measured the same way: 940 and 1409 pixels differing by at most 42, all of them one line of text, with the capture LIGHTER than the golden (83.1 against 58.4, and 83.7 against 60.0). Side by side the golden's "Tap sheet to dismiss" and "Rating sheet" are visibly heavier. Which also settles what the merge commit could only describe: master's change did not restyle the dialog title from bold to regular. The title was never bold. It was anti-aliased text painted twice into one frame, and #5870 stopped the second pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Fixes #5606.
The defect
AnimationManager.updateAnimations()steps the head of its queue only whileisInProgress()is true, and otherwise completes it without an update:That is right for an animation that already ran. It is wrong for one whose
isInProgress()is false from the moment it is queued: that animation was never stepped, so its entire payload is still sitting inupdateState(). Core queues three of those, and all three were dead:Container.insertComponentAt- the deferred insertion taken while another animation is in flight.cmp.setParent(this)is set optimistically and the component never enterscomponents, socnt.add(x)during an animation was a silent no-op.Container.removeComponentImpl- the matching deferred removal. The component is detached from the layout and its parent nulled but never taken out ofcomponents, so it kept painting.RefreshThemeCallback, whichContainer.wrapInLayeredPane()uses to re-root the content pane.The third was the worst of the three, and is not just a dropped mutation. Unlike the other two it does not override
flush(), so there was no path on which it ran at all.wrapInLayeredPane()puts the content pane into the new wrapper eagerly and defers only the half that puts the wrapper into the form, so the tree was left like this:Form.getActualPane()returnslayeredPane.getParent(), so it handed back a container the form never painted. EveryInteractionDialog- the lightweightPickerpopup among them - opened into a layered pane that could not appear on screen.The fix
completeAnimation()now gives a never-stepped animation exactly oneupdateState()before completing, tracked by a newsteppedflag. The existingcompletedflag cannot serve:completeIfNeeded()sets it for both cases.CompoundAnimationcascades to its children instead of taking one aggregate update. A singleupdateState()cannot stand in for them -isInProgress()walks the sequence cursor past the end as it finds every child finished, so one update would land on the last child and apply nothing else, and a never-startedsequentialAnimation()would apply only its last child. Cascading also covers the partially stepped case, where the compound was stepped but a child that was never in progress got skipped as the cursor walked over it. The sequential branch ofupdateState()marks the child it steps so that child is not applied a second time on completion.This is a regression, and a test was pinning it
9ed10e0 changed the one line in
updateAnimations()fromupdateAnimationState()tocompleteAnimation(), and addedAnimationManagerTest.testAlreadyFinishedAnimationRunsCompletionWithoutUpdateStatein the same commit. That test builds an animation whoseisInProgress()is false from the start - structurally the same thingContainerqueues - and asserts it must not be updated. It asserted the defect, so it is replaced here by one that asserts the payload is applied exactly once.Its sibling
testFinishedAnimationDoesNotUpdateStateAgainBeforeRemovalcovers what that commit was really about - not stepping a finished animation past its end - and is unchanged and still passing. Separating the two cases is the whole point of thesteppedflag.Verification
DeferredMutationTestis new: deferred remove, deferred add, the layered pane re-root, and a never-started sequence. All four fail on master, along with the rewrittenAnimationManagerTestcase - 5 failing assertions - and all pass here.updateState(), so a one-directional check passes on a component no ancestor will ever paint.core-unittests: 7255 tests, 0 failures.core-unittests: 0 findings (report regenerated for this run).check-cast-semantics.sh,check-control-characters.pyandcheck-copyright-headers.sh --baseall clean.The open question from the issue
#5606 reports that with this fix the Android
ValidatorLightweightPickerscreenshot fails with the content pane composited twice, and proposes a CI bisect as the next step. Two things are worth knowing before reading this branch's Android leg:newParent.initComponentImpl()conditional onisInitialized()where it had been unconditional. On the old code the deferred re-root re-initialized the whole live content pane the moment it fired. The measurement predates that, so it needs re-taking rather than trusting.picker.setUseLightweightPopup(true)opens anInteractionDialog, which lives in the layered pane and callsanimateLayout()on it. On master that popup's pane is the orphaned wrapper.I could not reproduce a double composite off-device - a paint-counting probe over the re-root scenario, with and without elevation, reports exactly one paint each way, and the full unit suite is clean. The Android screenshot leg on this PR is the measurement that settles it.