UI TestEngine: precise modal-blocked detection (fix #5961 regression)#5968
Merged
Conversation
The rootLevelBlocked() heuristic introduced in #5961 flagged every root-level entry as blocked whenever any popup was open, regardless of whether the entry itself lived *inside* the modal. This broke every test_ui scenario that pressed a button inside a modal (e.g. `Don't Save` in the `New scene` confirmation dialog) — pressButton threw `disabled: blocked by modal '...'`, Python propagated, MeshInspector hit std::terminate and exited with code 6. Caught by the ubuntu22-arm64 CI run on MeshInspectorCode#7242: RuntimeError: pressButton Don't Save: disabled: blocked by modal 'New scene##new scene' terminate called without an active exception Root cause: the TestEngine tree doesn't mirror ImGui's window stack, so a global "any popup open" check can't distinguish "button behind the modal" from "button inside the modal." Fix: do the check at widget registration time (where ImGui's GetCurrentWindow() is valid) and fold the result into the existing disabledReason string on the internal entry. A widget whose current window is not in the topmost blocking modal's ancestor chain gets disabledReason = "blocked by modal '<name>'" for this frame; widgets inside the modal (or with no modal open) get empty reason. - MRUITestEngine.cpp: effectiveDisabledReason now returns std::string and includes a modal-fallback branch via ImGui::GetTopMostPopupModal() + ParentWindow-chain walk. - MRUITestEngineControl.cpp: rootLevelBlocked() and topBlockingModalName() removed; composeStatus collapses to a single string_view input; listEntries / pressButton / writeValue drop the blockingModal branch and the path.size()==1 special case. No header changes. No API surface change — status strings still have the same shape: "available" | "disabled: <reason>" | "disabled: blocked by modal '<name>'". Also switches from IsPopupOpen(AnyPopupId|AnyPopupLevel) to GetTopMostPopupModal() so non-modal popups (tooltips, BeginPopup) no longer count as blockers. Verified live via MCP against the build: behind-modal root entries report the modal-blocked status and pressButton errors cleanly, while entries inside the modal (Toolbar Customize's `Reset to default`, `##About`, etc.) show "available" and press successfully. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
oitel
approved these changes
Apr 23, 2026
Comment on lines
+55
to
+63
| bool insideTopModal = false; | ||
| for ( ImGuiWindow* w = ImGui::GetCurrentWindow(); w; w = w->ParentWindow ) | ||
| { | ||
| if ( w == topModal ) | ||
| { | ||
| insideTopModal = true; | ||
| break; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Why is this not in a separate function like imGuiContextSaysDisabled()?
Contributor
Author
There was a problem hiding this comment.
Done in 49e16c9. imGuiBlockingModalName() is now a peer of imGuiContextSaysDisabled(); effectiveDisabledReason becomes a three-line if-chain. Live-verified the status output is unchanged.
Addresses @oitel's review on #5968: the inline `GetTopMostPopupModal() + ParentWindow walk` was a dense block inside `effectiveDisabledReason`. Extracted into a peer of `imGuiContextSaysDisabled()` — returns the blocking modal's name (or empty) so the caller can treat it as a single value. The reason-composer now reads as a three-line if-chain. No behavior change — live-verified that `status` and typed errors produce identical output before/after the extraction. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fedr
approved these changes
Apr 23, 2026
Grantim
added a commit
that referenced
this pull request
Apr 23, 2026
Master's #5968 landed while this PR was in review and refactored `composeStatus` to a single-argument form (blocking-modal formatting moved into `effectiveDisabledReason` upstream, and `rootLevelBlocked`/ `topBlockingModalName` were removed from this TU). Update `walkAll` and `listAllEntries` accordingly: - drop the `rootBlockingModal` and `isRootLevel` parameters from `walkAll` — every entry just calls `composeStatus(entryDisabledReason)` with the new 1-arg form; - drop the `blockingModal` local in `listAllEntries`. Side benefit: descendants now also surface `"disabled: blocked by modal '<name>'"` when applicable (previously the old 2-arg path special-cased only root-level entries; with the upstream refactor that special case is obsolete). Fixes the emscripten + macOS arm64 Release CI failures on this PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Grantim
added a commit
that referenced
this pull request
Apr 23, 2026
* UI TestEngine: add listAllEntries for one-shot flat tree dump
Walking the UI tree via `ui.listEntries` costs 4-5 sequential round-
trips to reach a known widget. The new `ui.listAllEntries` MCP tool
(plus Python mirror `uiListAllEntries`) returns every entry in the
subtree at `path` (default empty = whole tree) as a flat depth-first
list of `(path, TypedEntry)` pairs — groups appear in the list with
their descendants on subsequent rows, so the tree structure is
recoverable from the flat output. `TypedEntry` and `ui.listEntries`
are untouched — purely additive.
- `PathedEntry = pair<vector<string>, TypedEntry>` alias in
MRUITestEngineControl.h keeps the signature readable.
- `typeOf(Entry)` factored out so `listEntries` and the new walker
share the variant switch; `mcpTypeStr(EntryType)` factored out so
the old and new MCP handlers share the enum-to-string.
- pybind11 auto-converts `pair` to tuple and `vector<pair<…>>` to
list of tuples — no new binding class needed.
Output schema: `Array(Object{path, name, type, status})`. Declared
recursive `children`-tree schemas in `Mcp::Schema` aren't supported
(no `\$ref`), and the flat-with-path form sidesteps that entirely
while remaining trivially serializable.
Live-verified via MCP on a default-state viewer:
* 84 entries returned, 9857 bytes serialized (under the 10 KB budget).
* Identical set of `(path, type)` tuples to a recursive
`ui.listEntries` walk (0 missing, 0 extra, 0 status drift).
* Subtree rooting at `["QuickAccess"]` returns 9 entries all with the
correct prefix; first-level matches `ui.listEntries` at that path.
* Bogus path returns the same "No such entry" error as `listEntries`.
* Omitting `path` defaults to empty (whole tree).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* UI TestEngine: adapt walkAll to 1-arg composeStatus after #5968 merge
Master's #5968 landed while this PR was in review and refactored
`composeStatus` to a single-argument form (blocking-modal formatting
moved into `effectiveDisabledReason` upstream, and `rootLevelBlocked`/
`topBlockingModalName` were removed from this TU).
Update `walkAll` and `listAllEntries` accordingly:
- drop the `rootBlockingModal` and `isRootLevel` parameters from
`walkAll` — every entry just calls `composeStatus(entryDisabledReason)`
with the new 1-arg form;
- drop the `blockingModal` local in `listAllEntries`.
Side benefit: descendants now also surface `"disabled: blocked by
modal '<name>'"` when applicable (previously the old 2-arg path
special-cased only root-level entries; with the upstream refactor
that special case is obsolete).
Fixes the emscripten + macOS arm64 Release CI failures on this PR.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Signed-off-by: Grant Karapetyan <grant.karapetyan@meshinspector.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
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.
Summary
Replaces the overbroad
rootLevelBlocked()heuristic inUI::TestEngine::Controlwith a per-widget check done at registration time. Fixes the test_ui regressionubuntu22-arm64 build-teston MeshInspectorCode#7242.The regression
PR #5961 added
statusreporting and typed errors frompressButton/writeValuefor disabled-or-blocked widgets. The "blocked by modal" branch usedImGui::IsPopupOpen("", AnyPopupId | AnyPopupLevel)— true whenever any popup is open anywhere. It flagged every root-level entry, including buttons that actually lived inside the modal. Test scenarios pressing modal-own buttons likeDon't Saveon theNew sceneconfirmation dialog hit a typed error instead of being simulated, Python propagated, MeshInspector terminated with exit 6:test_all_scens.py::test_all_scenarios[main_set/features/select/qase_regression]all fail as a result.Why the heuristic was wrong
The
UI::TestEnginetree (pushTree/popTree) is independent of ImGui's window/popup structure. MeshInspector'sNew sceneconfirmation dialog registers itsDon't Savebutton viacreateButton(...)without a surroundingpushTree, so the button sits at the TestEngine root. A global "any popup open" test can't distinguish "at root behind the modal" from "at root inside the modal."The fix
Do the check at widget registration time (where
ImGui::GetCurrentWindow()is valid) and fold the result into the existingdisabledReasonstring on the internal entry. A widget whose current window is not in the topmost blocking modal's ancestor chain getsdisabledReason = "blocked by modal '<name>'"for that frame; widgets inside the modal (or with no modal open) get an empty reason.listEntries,pressButton,writeValue, andcomposeStatusalready consumedisabledReason— no per-caller changes needed.Concretely:
MRUITestEngine.cpp::effectiveDisabledReason— return type bumped fromstd::string_viewtostd::string; after the caller-reason andBeginDisabledauto-detect branches, falls back to checkingImGui::GetTopMostPopupModal()and walking the current window'sParentWindowchain. If the widget is outside the modal, format"blocked by modal '<name>'".MRUITestEngineControl.cpp—rootLevelBlocked()andtopBlockingModalName()deleted;composeStatuscollapses to a singlestd::string_view disabledReasoninput;listEntries/pressButton/writeValuedrop theblockingModalbranch and thepath.size() == 1special case (the per-entry reason now carries all needed info).ButtonEntry::disabledReasonandValueEntry::disabledReasonalready storestd::string. MCPstatusand PythonUiEntry.statusproduce the same three shapes as before:"available"|"disabled: <reason>"|"disabled: blocked by modal '<name>'".Secondary improvement: switches from
IsPopupOpen(AnyPopupId|AnyPopupLevel)toGetTopMostPopupModal(), so non-modal popups (tooltips,BeginPopupwithout_Modal) no longer count as blockers.Test plan
Verified live via MCP against Debug build:
"available".RibbonSceneButtons/*with empty scene —"disabled: <ribbon requirement>"(unchanged — the requirement-based path isn't touched).##Cube,Create Object,Watch Video,Information,Show on Startup) report"disabled: blocked by modal 'Toolbar Customize'".Toolbar Customize/##About,Toolbar Customize/Reset to default, etc.) report"available".pressButton(["Toolbar Customize", "Reset to default"])(inside modal) returns{}— no typed error.pressButton(["Toolbar", "##ToolbarCustomizeBtn"])(behind modal) returns"pressButton .../...: disabled: blocked by modal 'Toolbar Customize'"— correct typed error on a widget NOT in the modal, at any path depth.The CI-regression case (
Don't SaveinsideNew scene##new scene) is the symmetric "inside-modal at root" path: the widget's current window == the top modal →disabledReasonstays empty →status: "available"→pressButtonsucceeds.🤖 Generated with Claude Code