InteractionDialog: host on the right form + don't lose queued sibling dialogs (#5193)#5214
Open
shai-almog wants to merge 5 commits into
Open
InteractionDialog: host on the right form + don't lose queued sibling dialogs (#5193)#5214shai-almog wants to merge 5 commits into
shai-almog wants to merge 5 commits into
Conversation
… dialogs Fixes the "dialog never appears / appears later on another screen" symptoms of #5193, which the stackable layer fix (#5195) didn't cover: - show() and showPopupDialog(Rectangle) anchored the dialog to Display.getCurrent(), which still returns the outgoing form while a form transition is queued or in flight (Form.show() is asynchronous). The dialog would attach to the form leaving the screen, stay invisible, and only materialize when that form's animation queue was flushed on re-show or minimize/restore. They now resolve the host via the newly public Display.getCurrentUpcoming(), which returns the transition's destination form. - showPopupDialog(Component) now hosts the popup on the target component's own form instead of Display.getCurrent(), and pins it so the show() call at the end of the positioning flow targets the same form used for the positioning math. - In stackable mode, cleanupLayer() decided to detach the shared layer using getComponentCount() == 0, which doesn't see inserts that Container.insertComponentAt() deferred to the animation queue (e.g. a sibling dialog shown while this dialog's dispose animation ran). The pending insert would then flush into the detached container and the sibling dialog was lost forever. Use getChildrenAsList(true), which reflects queued changes. - resize() now uses the form the dialog is actually on rather than Display.getCurrent(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
Compared 128 screenshots: 128 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Contributor
Cloudflare Preview
|
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Collaborator
Author
|
Compared 128 screenshots: 128 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 63 screenshots: 63 matched. Benchmark Results
Build and Run Timing
|
Collaborator
Author
|
Compared 52 screenshots: 52 matched. Benchmark Results
Build and Run Timing
|
…ansitions Review feedback: getCurrentUpcoming is too niche/confusing to expose. Display.java is reverted to master. Instead, show(int,int,int,int) and showPopupDialogImpl gate on the existing public Display.isInTransition() and defer themselves with callSerially: the EDT doesn't process serial calls while a form transition is animating (edtLoopImpl returns right after paintTransitionAnimation), so the deferred show runs once the destination form has become the current form and Display.getCurrent() is correct. A pendingShow flag keeps isShowing()/showDialog() truthful through the deferral window and lets dispose() abandon a deferred show. The component-form pinning in showPopupDialog(Component), the stackable cleanupLayer fix and the resize() fix are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Five new tests in InteractionDialogTest, all verified to fail against the pre-fix InteractionDialog and pass with the fix: - showDuringFormTransitionAttachesToDestinationForm: show() during an in-flight form transition defers (isShowing() true, no form attach) and lands on the transition's destination form, not the outgoing one. - showPopupDialogPinsTargetComponentFormDuringTransition: the component popup attaches immediately to the target component's own form even mid-transition. - showPopupDialogRectDefersDuringTransition: the rect popup defers like show() and ends up on the destination form. - disposeDuringTransitionAbandonsDeferredShow: a dialog disposed during the deferral window never materializes. - stackableDisposeKeepsSharedLayerWhenSiblingInsertQueued: reproduces the queued-sibling-insert state deterministically (manual gate animation + single updateAnimations pop) and asserts dispose keeps the shared layer so the sibling materializes when the queue drains. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The mac-native screenshot job intermittently stops mid-suite (observed on master runs 27287581172/27208374291/27183747605 and PR #5214's first attempt, always inside the dual-appearance theme phase at a varying test) and the CI log shows nothing between the last capture and the 1500s DeviceRunner timeout, because the app's CN1SS suite markers only ever landed in artifact files. - Stream the per-test CN1SS:INFO/ERR:suite markers (the #5213 stage logging) into the CI console live from whichever log channel (unified-log stream or `open` stdout capture) produces them first, so a wedged run shows exactly which test it died on. - On STAGE:TIMEOUT, dump the last 25 suite markers from both channels and capture a 5s `sample` of the stuck process into the uploaded artifacts (ParparVM emits readable com_codename1_* symbols, so the sample shows where the EDT is parked), echoing the CN1 frames to the console. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Same tracing as the mac-native harness: build-ios wedged twice on PR #5214 (33/124 then 52/124 covered, both times stopping silently inside the graphics test block) and the console showed nothing between launch and the 1500s DeviceRunner timeout because the per-test CN1SS suite markers only existed in the device-log artifact. Stream the markers live into the CI console, and on STAGE:TIMEOUT dump the trailing markers plus a 5s `sample` of the simulator app process (it runs as a host macOS process; ParparVM's com_codename1_* symbols make the EDT stack readable) into the uploaded artifacts. Co-Authored-By: Claude Fable 5 <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.
Summary
Follow-up to #5195 for #5193. The remaining symptoms ThomasH99 reported — the first dialog never appearing, dialogs popping up later on a different screen, and background/foreground making them appear — are not the layer-sharing collision; they come from two other mechanisms:
Wrong-form attach.
show(int,int,int,int)andshowPopupDialogImplanchored the dialog toDisplay.getCurrent(), which still returns the outgoing form while a form transition is queued or in flight (Form.show()is asynchronous). The dialog attached to the form leaving the screen, stayed invisible, and only materialized when that form's animation queue was flushed — i.e. when navigating back to it or on minimize/restore (Form.deinitializeImpl()→AnimationManager.flush()). That is exactly the "shows up at another screen, like if the adding to the layer was delayed" report.Stackable cleanup could eat a pending sibling. In stackable mode
cleanupLayer()usedgetComponentCount() == 0to decide to detach the shared layer. That count doesn't see inserts deferred byContainer.insertComponentAt()while an animation is in progress (e.g. dialog B shown during dialog A's blocking dispose animation, sinceinvokeAndBlockkeeps serving EDT callbacks). The layer was detached with B's insert still queued, B then flushed into the detached container and was lost permanently — which is whysetStackable(true)didn't help the reporter.Changes
No new public API.
show(...)andshowPopupDialog(Rectangle)gate on the existingDisplay.isInTransition()and defer themselves withcallSeriallywhen a form transition is in flight. The EDT doesn't process serial calls while a transition is animating (edtLoopImplreturns right afterpaintTransitionAnimation), so the deferred show runs exactly when the destination form has become the current form andDisplay.getCurrent()is correct again. ApendingShowflag keepsisShowing()/showDialog()truthful through the deferral window and letsdispose()abandon a deferred show.showPopupDialog(Component)hosts the popup on the target component's own form (it already computed it for the formMode check) and pins it so theshow()call at the end of the positioning flow targets the same form used for the positioning math — no deferral needed on this path.cleanupLayer()usesgetChildrenAsList(true).isEmpty()so a layer with a queued insert is not detached (same trapForm.getLayeredPanealready guards against).resize()uses the form the dialog is actually on (getComponentForm()).Test coverage
Five new regression tests in
InteractionDialogTest(core-unittests), all verified to fail against the pre-fixInteractionDialogand pass with the fix:showDuringFormTransitionAttachesToDestinationForm— show() mid-transition defers and attaches to the destination form, not the outgoing one.showPopupDialogPinsTargetComponentFormDuringTransition— component popups attach immediately to the target component's form.showPopupDialogRectDefersDuringTransition— rect popups defer like show().disposeDuringTransitionAbandonsDeferredShow— a dialog disposed during the deferral window never materializes.stackableDisposeKeepsSharedLayerWhenSiblingInsertQueued— deterministic reproduction of the queued-sibling-insert state; dispose must not detach the shared layer.Mac-native suite tracing (separate commit)
The mac-native screenshot job intermittently stops mid-suite in the dual-appearance theme phase (observed on master runs 27287581172 / 27208374291 / 27183747605 before this PR, and on this PR's first attempt) with nothing in the CI log between the last capture and the 1500s timeout. The harness now:
CN1SS:INFO/ERR:suitemarkers (the Screenshot suite: retry once + stage logging for silent capture timeouts #5213 stage logging) into the CI console live, so a wedged run shows exactly which test it died on;STAGE:TIMEOUT, dumps the last suite markers from both log channels and captures a 5ssampleof the stuck process into the uploaded artifacts (ParparVM emits readablecom_codename1_*symbols, so the sample shows where the EDT is parked).Fixes #5193
🤖 Generated with Claude Code