Scope dialog dismissal to how the dialog was shown - #7
Conversation
Closes the gap wurst-castle-fight worked around in RoundStatsBoard, which carries the comment: "Not UIDialog.withModal(): that wires an outside-click handler calling the dialog's own global hide(), which would close the board for EVERY player when one of them clicks the dim." That was accurate. withModal()'s dismiss handler, the confirm dialog's Yes/No buttons and UIDialog.closeButton() all called the global hide(), so one player dismissing a dialog closed it for everyone - which made the per-player show(p) overload added in #6 unusable with a modal. Dismissal now follows how the dialog was shown, rather than picking one scope for both cases. setVisible(player, bool) marks the dialog owner-scoped, and dismissFor(p) then hides for that player alone; a dialog only ever shown globally still dismisses globally. So a shared prompt keeps resolving once for all - the right behaviour when the dialog represents a single decision - while a private prompt cannot be closed out from under the other players. ModalDismissCallback already carries the acting player from #6, so no new plumbing was needed. Backward compatible: the only behaviour that changes is for dialogs shown per player, which was not expressible before #6. Neither consuming map uses withModal() or UIDialog.closeButton() today (zombie-defense uses the free closeButton(frame, size), untouched), so this is inert for them until they adopt it. TableLayoutTest gains a "Private" button opening the confirm dialog for the clicking player only. Panel fit re-checked headlessly before committing: 0.368 required of 0.380 available, no overflow.
![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: 50c1bac2a4
ℹ️ 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".
| if ownerScoped and p != null | ||
| hide(p) | ||
| else | ||
| hide() |
There was a problem hiding this comment.
ownerScoped when changing dialog visibility
When either dialog is opened with show(p), ownerScoped is never assigned anywhere and therefore remains false; consequently every Yes/No, modal-backdrop, or close-button click reaches the global hide() branch and still closes the dialog for all players. Update the scope state in both the global and player-scoped visibility paths so it reflects the most recent way the dialog was shown.
AGENTS.md reference: AGENTS.md:L20-L23
Useful? React with 👍 / 👎.
Valid review finding, and the previous commit's feature did nothing without it. ownerScoped was declared and read by dismissFor() but never assigned, so it stayed false and every Yes/No, modal-backdrop and close-button click took the global hide() branch - exactly the behaviour the commit set out to fix. The intended assignment was lost to a silent no-op find/replace and the grep that would have caught it was not read closely enough. Both classes now update the flag in both directions, so it reflects the most recent way the dialog was shown: setVisible(player, bool) sets it, and a global show() clears it again. A dialog shown per player, then later shown to everyone, correctly goes back to dismissing for everyone. Verified by inspection this time rather than assumed: each class has exactly one declaration, one read in dismissFor, one set-true in the player-scoped path and one set-false in the global show, and all five dismissal paths (modal outside-click x2, Yes, No, closeButton) route through dismissFor.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5dbbe6cfec
ℹ️ 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".
| function setVisible(player p, bool flag) returns thistype | ||
| // Records that this dialog is being driven per player, so dismissFor() closes it for the | ||
| // acting player instead of everyone. A later global show() clears it again. | ||
| ownerScoped = true |
There was a problem hiding this comment.
When a dialog opened with show() is subsequently hidden from one player via hide(p) or setVisible(p, false), this unconditional assignment changes its dismissal mode to owner-scoped even though the dialog was shown globally. A remaining player's Yes/No/outside click then hides only that player, leaving the dialog visible to others and allowing repeated result callbacks, contrary to the documented rule that globally shown dialogs dismiss for everyone; the mirrored assignment in UIDialog.setVisible has the same problem. Only a player-scoped show should switch the mode, while a scoped hide should preserve the mode of the active presentation.
AGENTS.md reference: AGENTS.md:L20-L23
Useful? React with 👍 / 👎.
Valid review finding. setVisible(player, bool) set ownerScoped unconditionally, so a scoped HIDE flipped the mode too. Hiding a globally shown dialog from one player - an observer, say - turned it into a per-player prompt, and a remaining player's Yes/No/outside click then closed only their own copy. The dialog stayed up for everyone else, who could answer again, firing the result callback repeatedly. That contradicts the documented rule that a globally shown dialog dismisses for everyone. A scoped show is what establishes a per-player presentation, so only that sets the mode now; a scoped hide preserves the mode of the active presentation. Global show() still clears it. Combinations checked: show() then hide(p) keeps global dismissal; show(p) dismisses for p alone; show() after show(p) reverts to global; hide(p) on a never-shown dialog leaves the mode untouched.
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. 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". |
1 participant
Follow-up to #6. Closes the gap
wurst-castle-fighthad already worked around inRoundStatsBoard.wurst:That comment was accurate.
withModal()'s dismiss handler, the confirm dialog's Yes/No buttons andUIDialog.closeButton()all called the globalhide(), so one player dismissing closed the dialog for everyone — which made theshow(p)overload added in #6 unusable together with a modal.Approach
Rather than pick one scope for both cases, dismissal now follows how the dialog was shown:
show()→ dismissing closes it for everyone (unchanged);show(p)→ dismissing closes it for that player alone.setVisible(player, bool)sets anownerScopedflag anddismissFor(p)branches on it. A shared prompt therefore keeps resolving once for all — correct when the dialog represents a single decision — while a private prompt cannot be closed out from under the other players.ModalDismissCallbackalready carries the acting player from #6, so no new plumbing was needed.Applies to all three dismissal paths: the modal outside-click,
UIConfirmDialog's Yes/No, andUIDialog.closeButton().Compatibility
The only behaviour that changes is for dialogs shown per player, which was not expressible before #6, so nothing existing can depend on it. Verified against both consuming maps: neither uses
withModal()orUIDialog.closeButton()— zombie-defense uses the freecloseButton(frame, size)fromTableUiButtons, which is untouched. Inert for them until they adopt it.Validation
Stage 1 (this repo's gate):
grill typecheck --quiet— passes.grill test— full suite green, zero compiler warnings.git diff --check— clean.inspect()model before committing: 0.368 required of 0.380 available, empty overflow summary. Scratch file removed.Stage 2 (engine evidence) — not run, and not runnable here. This repo has no e2e harness of its own; per
AGENTS.mdthat gate belongs to a consuming map.TableLayoutTest.wurstgains a "Private" button that opens the confirm dialog for the clicking player only, so the scenario is ready to click, but the behaviour this PR changes is per-player dismissal — it cannot be observed with one client. The real check needs two clients: P1 opens via "Private", P2 must not see it; P1 clicks the dim, and the dialog must close for P1 only. The pre-existing "Confirm" button covers the global path in the same run.Treat this as unverified against the engine until that run happens.