From b41669a9e663c131239009e1558c172b7a75b864 Mon Sep 17 00:00:00 2001 From: wallstop Date: Sat, 22 Aug 2026 05:46:22 +0000 Subject: [PATCH 1/9] Keep wrapped editor rows inside their own box On Unity 2021.3 a wrapping row keeps the height of one line, so the extra lines draw on top of the block beneath it. The Flow Graph details pane and the Message Monitor have twelve such rows. All twelve now go through DxMessagingEditorTheme.ApplyContentSizedWrap. It measures the children and gives the container that height. An editor that already sizes the container is left alone. The Monitor's lane pills and filter tokens now wrap in a row the window owns. Unity sizes a scroll view's own content container to the viewport, so wrapping it clipped the extra lines and left the scroller with nothing to scroll to, on every editor version. Adds the pull-request-writing skill and points the PR template at it. Closes #440. Co-Authored-By: Claude Opus 5 (1M context) --- .agents/skills/pull-request-writing/SKILL.md | 15 ++ .claude/skills/pull-request-writing/SKILL.md | 15 ++ .github/pull_request_template.md | 22 +- .llm/context.md | 7 +- .llm/index.json | 15 +- .llm/index.md | 3 +- .llm/skills/pull-request-writing/SKILL.md | 96 +++++++ .../references/pull-request-examples.md | 104 ++++++++ CHANGELOG.md | 8 + Editor/DxMessagingEditorTheme.cs | 135 ++++++++++ Editor/Windows/DxMessagingFlowGraphWindow.cs | 20 +- .../DxMessagingMessageMonitorWindow.cs | 33 ++- README.md | 2 +- Tests/Editor/DxMessagingEditorThemeTests.cs | 247 ++++++++++++++++++ .../Editor/DxMessagingFlowGraphWindowTests.cs | 131 ++++++++++ .../DxMessagingMessageMonitorWindowTests.cs | 98 +++++++ Tests/Editor/EditorWindowTestUtility.cs | 93 +++++++ llms.txt | 5 +- 18 files changed, 1021 insertions(+), 28 deletions(-) create mode 100644 .agents/skills/pull-request-writing/SKILL.md create mode 100644 .claude/skills/pull-request-writing/SKILL.md create mode 100644 .llm/skills/pull-request-writing/SKILL.md create mode 100644 .llm/skills/pull-request-writing/references/pull-request-examples.md diff --git a/.agents/skills/pull-request-writing/SKILL.md b/.agents/skills/pull-request-writing/SKILL.md new file mode 100644 index 00000000..618d30eb --- /dev/null +++ b/.agents/skills/pull-request-writing/SKILL.md @@ -0,0 +1,15 @@ +--- +name: pull-request-writing +description: "How to title and describe a DxMessaging pull request: short + Simplified Technical English, one idea per sentence, and a body that answers + why the problem mattered, what the change does, and how we know it is correct. + Bans jargon, filler, status narration, and session diaries. Use when opening + or editing a pull request, writing its title, rewriting a description a + reviewer found hard to read, or deciding what evidence belongs in the body." +--- + + + +Canonical instructions: [`.llm/skills/pull-request-writing/SKILL.md`](../../../.llm/skills/pull-request-writing/SKILL.md) + +Read that file and follow it. Supporting detail is in the sibling `references/` directory. diff --git a/.claude/skills/pull-request-writing/SKILL.md b/.claude/skills/pull-request-writing/SKILL.md new file mode 100644 index 00000000..618d30eb --- /dev/null +++ b/.claude/skills/pull-request-writing/SKILL.md @@ -0,0 +1,15 @@ +--- +name: pull-request-writing +description: "How to title and describe a DxMessaging pull request: short + Simplified Technical English, one idea per sentence, and a body that answers + why the problem mattered, what the change does, and how we know it is correct. + Bans jargon, filler, status narration, and session diaries. Use when opening + or editing a pull request, writing its title, rewriting a description a + reviewer found hard to read, or deciding what evidence belongs in the body." +--- + + + +Canonical instructions: [`.llm/skills/pull-request-writing/SKILL.md`](../../../.llm/skills/pull-request-writing/SKILL.md) + +Read that file and follow it. Supporting detail is in the sibling `references/` directory. diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index c5d1edc1..ffa895c9 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -1,12 +1,24 @@ -## Description + - +## Why -## Related Issue + + +## What changed - + + +## How we know + + + +## Related Issue -Fixes # +Closes # ## Type of Change diff --git a/.llm/context.md b/.llm/context.md index a9ffd068..cb086ed5 100644 --- a/.llm/context.md +++ b/.llm/context.md @@ -17,9 +17,9 @@ discover it. `.llm/` is the single source of truth; the mirrors are generated po -31 skills are registered. See [the skill index](./index.md) for the full table. +32 skills are registered. See [the skill index](./index.md) for the full table. -`allocation-and-leak-coverage`, `api-design-patterns`, `benchmark-methodology`, `cache-eviction`, `changelog-management`, `collection-pooling`, `data-driven-tests`, `dispatch-hot-path`, `documentation-code-samples`, `documentation-style`, `github-access`, `github-workflow-consistency`, `il2cpp-build-configuration`, `markdown-authoring`, `memory-reclamation`, `object-pooling`, `package-publishing`, `serializable-dictionary`, `shell-and-powershell`, `singleton-patterns`, `test-code-quality`, `test-coverage-design`, `test-diagnostics`, `test-failure-investigation`, `test-fixtures-and-cleanup`, `unity-editor-ci`, `unity-editor-conventions`, `unity-licensing`, `unity-mcp-test-loop`, `unity-test-execution`, `value-equality-and-hashing` +`allocation-and-leak-coverage`, `api-design-patterns`, `benchmark-methodology`, `cache-eviction`, `changelog-management`, `collection-pooling`, `data-driven-tests`, `dispatch-hot-path`, `documentation-code-samples`, `documentation-style`, `github-access`, `github-workflow-consistency`, `il2cpp-build-configuration`, `markdown-authoring`, `memory-reclamation`, `object-pooling`, `package-publishing`, `pull-request-writing`, `serializable-dictionary`, `shell-and-powershell`, `singleton-patterns`, `test-code-quality`, `test-coverage-design`, `test-diagnostics`, `test-failure-investigation`, `test-fixtures-and-cleanup`, `unity-editor-ci`, `unity-editor-conventions`, `unity-licensing`, `unity-mcp-test-loop`, `unity-test-execution`, `value-equality-and-hashing` @@ -55,6 +55,9 @@ discover it. `.llm/` is the single source of truth; the mirrors are generated po checkout, commit, branch, fetch, rebase, and push operations. Use `gh` only as the final fallback when neither the connector nor `git` covers the required operation. Preserve this order. - Never commit repository settings that auto-approve chat-invoked terminal commands. +- Write pull request titles and descriptions in short Simplified Technical English: say why the + problem mattered, what the change does, and how you know it is correct. No jargon, no filler, no + session diary. Follow [pull-request-writing](./skills/pull-request-writing/SKILL.md). - Ensure fenced markdown examples are closed and do not swallow real sections (for example `## See Also`). - Before committing, run the relevant formatters/linters yourself (`npm run format:check`, `npm run lint:markdown`, `npm run check:spelling`, `npm run validate:all` as applicable); hooks are the fast backstop, not the first signal. diff --git a/.llm/index.json b/.llm/index.json index e6213fa7..d5dcc650 100644 --- a/.llm/index.json +++ b/.llm/index.json @@ -1,5 +1,5 @@ { - "skillCount": 31, + "skillCount": 32, "skills": [ { "name": "allocation-and-leak-coverage", @@ -293,6 +293,19 @@ ".llm/skills/package-publishing/references/unity-analyzer-shipping.md" ] }, + { + "name": "pull-request-writing", + "path": ".llm/skills/pull-request-writing/SKILL.md", + "description": "How to title and describe a DxMessaging pull request: short Simplified Technical English, one idea per sentence, and a body that answers why the problem mattered, what the change does, and how we know it is correct. Bans jargon, filler, status narration, and session diaries. Use when opening or editing a pull request, writing its title, rewriting a description a reviewer found hard to read, or deciding what evidence belongs in the body.", + "metadata": { + "category": "process", + "tags": "pull-request, writing, simplified-technical-english, ste, review, communication" + }, + "lineCount": 96, + "references": [ + ".llm/skills/pull-request-writing/references/pull-request-examples.md" + ] + }, { "name": "serializable-dictionary", "path": ".llm/skills/serializable-dictionary/SKILL.md", diff --git a/.llm/index.md b/.llm/index.md index 845c0608..40070813 100644 --- a/.llm/index.md +++ b/.llm/index.md @@ -2,7 +2,7 @@ -31 skills. Each is an [Agent Skills](https://agentskills.io/specification) +32 skills. Each is an [Agent Skills](https://agentskills.io/specification) directory: `SKILL.md` holds the instructions, `references/` holds supporting detail loaded on demand. | Skill | References | Description | @@ -24,6 +24,7 @@ directory: `SKILL.md` holds the instructions, `references/` holds supporting det | [memory-reclamation](./skills/memory-reclamation/SKILL.md) | 3 | How DxMessaging reclaims empty per-type and per-InstanceId slots through counter-based idle sweeps, IMessageBus.Trim, and DxPools caps, plus the test and documentation duties for any new memory holder. Use when adding a MessageCache field, a dictionary or pool keyed by message type or InstanceId, when slot counts or memory grow over a long session, or when changing DxMessagingRuntimeSettings eviction and buffer settings. | | [object-pooling](./skills/object-pooling/SKILL.md) | 6 | Reuse pooled message objects through Rent/Return instead of allocating a new instance per emit, so dispatch stays allocation-free. Use when adding a message type, seeing GC spikes or a rising allocation rate in the Unity profiler, writing an allocation test, or choosing between a pooled class message and a readonly struct message. | | [package-publishing](./skills/package-publishing/SKILL.md) | 3 | Controlling what ships in the com.wallstop-studios.dxmessaging npm/UPM package: the package.json files allowlist versus .npmignore exclusions, Unity .meta pairing rules for every shipped file and directory, the issue #204 tarball invariants verified against npm pack --json --dry-run, and where RoslynAnalyzer-labeled DLLs must live (Runtime/Analyzers/, never an editor-only asmdef) so the source generator reaches Assembly-CSharp. Use when adding or excluding files from the package, adding a new output directory, seeing CS0315/CS0452 on consumer [Dx*Message] types, or building and verifying the analyzer payload. | +| [pull-request-writing](./skills/pull-request-writing/SKILL.md) | 1 | How to title and describe a DxMessaging pull request: short Simplified Technical English, one idea per sentence, and a body that answers why the problem mattered, what the change does, and how we know it is correct. Bans jargon, filler, status narration, and session diaries. Use when opening or editing a pull request, writing its title, rewriting a description a reviewer found hard to read, or deciding what evidence belongs in the body. | | [serializable-dictionary](./skills/serializable-dictionary/SKILL.md) | 4 | Back a runtime Dictionary with parallel SerializeField lists through ISerializationCallbackReceiver so key/value config is editable in the Unity Inspector and persists in scenes, prefabs, and ScriptableObjects. Use when a SerializeField Dictionary silently stays empty, when parallel key/value lists are drifting out of sync, or when a designer needs to edit a lookup table in the Inspector. | | [shell-and-powershell](./skills/shell-and-powershell/SKILL.md) | 9 | Writing bash and PowerShell that survives CI: set -e error handling for grep/diff/rm, quoting, filename case sensitivity on Linux runners, the PowerShell StrictMode 0/1/many gotcha that requires @()-wrapping every captured result before reading .Count or indexing, here-string quoting, WriteAllText encoding, regex non-greedy versus character-class exclusion, accurate regex comments for the g/i/m/s/u flags, Windows PE-binary stub requirements, cross-drive path containment, and tar archive operands. Use when adding or editing a .sh, .ps1, or Node script, or when a script passes locally and fails on a Linux or Windows runner. | | [singleton-patterns](./skills/singleton-patterns/SKILL.md) | 5 | Implement Unity global managers with RuntimeSingleton for MonoBehaviours that survive scene loads and ScriptableObjectSingleton for Resources-backed config assets, including duplicate handling and quit-time safety. Use when writing a GameManager or AudioManager, when a static Instance field is null after a scene change or on quit, when two copies of a manager exist, or when bootstrapping a manager before the first scene. | diff --git a/.llm/skills/pull-request-writing/SKILL.md b/.llm/skills/pull-request-writing/SKILL.md new file mode 100644 index 00000000..a6fd5046 --- /dev/null +++ b/.llm/skills/pull-request-writing/SKILL.md @@ -0,0 +1,96 @@ +--- +name: pull-request-writing +description: "How to title and describe a DxMessaging pull request: short Simplified Technical English, one idea per sentence, and a body that answers why the problem mattered, what the change does, and how we know it is correct. Bans jargon, filler, status narration, and session diaries. Use when opening or editing a pull request, writing its title, rewriting a description a reviewer found hard to read, or deciding what evidence belongs in the body." +metadata: + category: "process" + tags: "pull-request, writing, simplified-technical-english, ste, review, communication" +--- + +# Pull Request Writing + +A pull request is read by a person who was not in the session. They read the title in a list of +titles, and the body once, to decide whether to trust the change. Write for that reader. + +The whole body is written in Simplified Technical English (STE): short sentences, common words, +one idea per sentence, active voice. + +## When to use + +- Opening a pull request, or editing its title or description. +- A reviewer says a description is long, unclear, or hard to follow. +- Deciding which measurements, tables, or logs belong in the body. +- Writing the pull request that closes several issues at once. + +## Rules + +### Title + +- One line, at most 72 characters, plain sentence case. +- Say what the change does, in the words a user would use. Not the mechanism. +- Start with a verb: `Fix`, `Add`, `Remove`, `Speed up`, `Stop`. +- No conventional-commit prefixes, no issue numbers, no branch names, no "and" chains that + join three unrelated things. If the title needs three clauses, lead with the one a reader + cares about most and let the body carry the rest. + +Good: `Keep wrapped Flow Graph rows inside their own box` +Bad: `fix(editor): DxMessagingFlowGraphWindow flexWrap container height resolution on 2021.3` + +### Body shape + +Three short sections, in this order, and nothing else by default: + +1. **Why** - what went wrong for a user, or what they could not do. One short paragraph. +1. **What changed** - what the code does now. A short list if there is more than one part. +1. **How we know** - the test, the measurement, or the run that proves it. + +Add a `Closes #NNN` line for each issue the change closes. Add a fourth section only when the +change leaves something open that a reader must know about. + +### Length + +- Aim for 200 words. Stop at 400. +- One pull request that closes several issues gets one `Why / What changed / How we know` block + per issue, each still short. It does not get a longer preamble. +- Move deep evidence to the issue or to a `progress/` record and link it. The body carries the + verdict, not the working. + +### Simplified Technical English + +- One idea per sentence. Under 20 words. +- Active voice, present tense: "The row now grows", not "The row will have been grown". +- One word per meaning. Pick `fix` or `repair`, not both, and use it everywhere. +- Use the plainest word that is still exact: `use` not `utilize`, `so` not `hence`, `before` + not `prior to`, `now` not `at this time`. +- Expand an abbreviation on first use unless it names a file, a type, or a workflow. +- No nested clauses, no dashes carrying a second thought, no sentence that needs re-reading. +- ASCII only, exactly as in [documentation-style](../documentation-style/SKILL.md). + +### Banned + +- Filler and marketing: `comprehensive`, `robust`, `seamless`, `leverage`, `delve`, `simply`, + `just`, `note that`, `it is worth noting`. +- Status narration: `As requested`, `Per the previous session`, `This PR does the following`. +- Session diaries: what was tried and rejected, how long something took, which agent did what. + A rejected approach belongs in the issue only when the next person would otherwise retry it. +- Restating the diff. The file list is already on the page. +- Emoji, headings deeper than `###`, and tables with one row. + +## Verification + +Before you open or update the pull request, read the title and body once as a stranger: + +- Does the title alone say what changed? +- Does the body say why it mattered before it says what you did? +- Is there a sentence over 20 words, or a word a new contributor would look up? +- Is there a claim with no evidence behind it? + +Fix what fails. Do not add words to fix it. + +## See Also + +- [Pull Request Examples](./references/pull-request-examples.md) - a rewrite, an issue-closing + body, and the fourth-section case. +- [documentation-style](../documentation-style/SKILL.md) - the ASCII and banned-phrase rules + this skill inherits. +- [changelog-management](../changelog-management/SKILL.md) - what the same change owes + `CHANGELOG.md`, which is written for users, not reviewers. diff --git a/.llm/skills/pull-request-writing/references/pull-request-examples.md b/.llm/skills/pull-request-writing/references/pull-request-examples.md new file mode 100644 index 00000000..7ec288bb --- /dev/null +++ b/.llm/skills/pull-request-writing/references/pull-request-examples.md @@ -0,0 +1,104 @@ +# Pull Request Examples + +Worked examples for [pull-request-writing](../SKILL.md). Each one shows a real failure mode and +the shorter version that replaces it. + +## A rewrite + +### Before + +> ## Summary +> +> This PR provides a comprehensive fix for the long-standing layout issue in the Flow Graph +> details pane, which has been observed to manifest on Unity 2021.3 LTS but not on newer +> editors. As requested in the previous session, I first investigated whether the root cause +> was related to `align-content` resolution semantics, then determined through experimentation +> that the actual issue is that Yoga does not derive the cross-axis extent of a multi-line flex +> container from its flex lines on that version. Note that this was not caught earlier because +> the local MCP verification loop only runs against a single host editor (6000.4.6f1). Ten +> separate container declarations were audited and it was determined that all of them are +> potentially affected, and so a shared helper was leveraged to seamlessly address the entire +> class of problem in one place, rather than redesigning each container individually. + +Everything wrong with it: 130 words with no verdict, three abandoned investigation steps, a +banned word in almost every sentence, and no evidence. + +### After + +> ## Why +> +> On Unity 2021.3, a wrapping row keeps the height of one line. The extra lines draw outside +> the row, on top of the block beneath it. The Flow Graph details pane has ten such rows, so a +> long type name or a deep hierarchy path can hide the next block. +> +> ## What changed +> +> All twelve wrapping containers in the editor now go through +> `DxMessagingEditorTheme.ApplyContentSizedWrap`. It measures the children and gives the +> container that height. An editor that already sizes the container correctly is left alone. +> +> ## How we know +> +> A probe pins a wrapping row to one line's height, which is what 2021.3 does on its own. The +> unfixed probe leaves its children outside the row. The fixed probe does not. A source scan +> fails if any editor file turns wrapping on by hand again. + +95 words. Same change, and now a reviewer can check it. + +## Closing several issues in one pull request + +One `Why / What changed / How we know` block per issue. No shared preamble. + +> Closes #440, #336. +> +> ## Wrapped rows draw outside their box (#440) +> +> **Why.** On Unity 2021.3 a wrapping row keeps one line's height... +> +> **What changed.** ... +> +> **How we know.** ... +> +> ## The MSVC gate passes a compiler that cannot run (#336) +> +> **Why.** ... + +Use `###`-free bold labels when the blocks are short. The reader scans for the issue heading, +then reads three sentences. + +## When a fourth section is right + +Add one only when the reader must act on it, or must not be surprised by it: + +- **What is still open.** Name the part the change does not fix, and the issue that tracks it. +- **Risk.** Name the case you could not test, and why. + +Do not add a fourth section for what you considered and dropped. That is a session diary. + +## Evidence: what goes in the body + +Keep the verdict. Move the working. + +| Evidence | Where it goes | +| ------------------------------------------ | ------------------------------- | +| "793 tests pass, 0 fail" | Body | +| The full test list | Nowhere | +| "EditMode step drops from 113.8s to 36.3s" | Body | +| The per-run table it came from | The issue | +| "Reproduced on run 31764457664" | Body, with the link | +| Console output, stack traces, raw logs | The issue, in a collapsed block | + +The rule behind the table: a number a reviewer would check belongs in the body. The data you +derived it from belongs where it can be re-derived. + +## Titles + +| Bad | Why | Good | +| ---------------------------------------------------------------------------------------- | --------------------------------------------- | ------------------------------------------- | +| `fix: layout` | Says nothing | `Keep wrapped editor rows inside their box` | +| `Refactor DxMessagingFlowGraphWindow flexWrap handling` | Names the mechanism, not the effect | `Keep wrapped editor rows inside their box` | +| `Fix #440` | The reader cannot see the issue from the list | `Keep wrapped editor rows inside their box` | +| `Fix the wrap bug, add MSVC launch probe, close 5 measurement issues, update agent docs` | Four clauses | `Keep wrapped editor rows inside their box` | + +The last one is the common case for a session that closes several issues. Lead with the change +a user would notice. The body lists the rest. diff --git a/CHANGELOG.md b/CHANGELOG.md index 53e432e8..59383c5b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 already did, and the route-kind chips give space back instead of pushing the search field past the edge ([#435](https://github.com/Ambiguous-Interactive/DxMessaging/issues/435)). +- Keep wrapping rows in the Flow Graph and Message Monitor inside their own box. On Unity 2021.3 a + wrapping row keeps the height of one line, so a long type name or a deep hierarchy path pushed + the extra lines on top of the block beneath. Every wrapping container in the editor now measures + its lines and takes that height. Monitor lane pills and active-filter tokens also wrap in a row + the window owns rather than in the scroll view's own content container, which Unity sized to the + viewport and clipped, so tokens past the first lines can now be scrolled to + ([#440](https://github.com/Ambiguous-Interactive/DxMessaging/issues/440)). + ## [3.3.0] ### Added diff --git a/Editor/DxMessagingEditorTheme.cs b/Editor/DxMessagingEditorTheme.cs index 14b5fd20..47886c53 100644 --- a/Editor/DxMessagingEditorTheme.cs +++ b/Editor/DxMessagingEditorTheme.cs @@ -413,6 +413,141 @@ internal static float ApplyResizedHeight( return clamped; } + /// + /// Turns wrapping on for a container and keeps that container at least as tall as the + /// lines its children wrap onto. + /// + /// Unity 2021.3 does not grow a wrapping container to fit the extra lines. The container + /// keeps its single-line height and the wrapped lines draw outside it, on top of whatever + /// the window draws beneath. Newer editors size the container correctly, so the defect is + /// invisible on 6000.x. Issue #435 hit it in the Message Monitor toolbar and issue #440 + /// lists the Flow Graph containers with the same shape. + /// + /// Measuring the children and applying the result as `min-height` supplies the height + /// Unity 2021.3 does not derive. The measurement is only applied when the container is + /// actually too short, so an editor that already sizes the container writes no inline + /// style at all. `align-content: flex-start` packs the lines at the top, so a container + /// that just grew cannot stretch its own lines and ask to grow again. + /// + internal static void ApplyContentSizedWrap(VisualElement container) + { + if (container == null) + { + return; + } + + container.style.flexWrap = Wrap.Wrap; + container.style.alignContent = Align.FlexStart; + WrapHeightFit fit = new(container); + container.RegisterCallback( + static (_, state) => state.Fit(), + fit + ); + container.RegisterCallback( + static (_, state) => state.Release(), + fit + ); + } + + /// + /// The height a wrapping container needs for every line its children occupy, measured in + /// the container's own coordinate space so a panned or zoomed ancestor cannot skew it. + /// Returns 0 when nothing is measurable yet. + /// + internal static float MeasureWrappedContentHeight(VisualElement container) + { + if (container == null) + { + return 0f; + } + + float contentBottom = 0f; + bool measured = false; + foreach (VisualElement child in container.Children()) + { + if (child.resolvedStyle.display == DisplayStyle.None) + { + continue; + } + + Rect childLayout = child.layout; + if (float.IsNaN(childLayout.yMax)) + { + continue; + } + + contentBottom = Mathf.Max( + contentBottom, + childLayout.yMax + child.resolvedStyle.marginBottom + ); + measured = true; + } + + if (!measured) + { + return 0f; + } + + IResolvedStyle containerStyle = container.resolvedStyle; + return contentBottom + containerStyle.paddingBottom + containerStyle.borderBottomWidth; + } + + /// + /// Holds the height one container was given, so a container that already fits is never + /// written to and a container that was grown is only written again when it needs more. + /// + /// The height is only ever raised while the container is in a panel. Unity reports a + /// geometry change to the container when the container's own box changes, and a container + /// held at a height by this class does not change its box when its content shrinks, so + /// there is no second measurement to lower it from. Leaving the panel releases the height + /// instead, which is what a reused or rebuilt container needs. + /// + private sealed class WrapHeightFit + { + private const float Tolerance = 0.5f; + + private readonly VisualElement _container; + + private bool _applied; + private float _appliedHeight; + + internal WrapHeightFit(VisualElement container) + { + _container = container; + } + + internal void Fit() + { + float required = MeasureWrappedContentHeight(_container); + if (required <= 0f) + { + return; + } + + float current = _applied ? _appliedHeight : _container.resolvedStyle.height; + if (required <= current + Tolerance) + { + return; + } + + _applied = true; + _appliedHeight = required; + _container.style.minHeight = required; + } + + internal void Release() + { + if (!_applied) + { + return; + } + + _applied = false; + _appliedHeight = 0f; + _container.style.minHeight = StyleKeyword.Null; + } + } + private static StyleSheet LoadStyleSheet(string path) { return AssetDatabase.LoadAssetAtPath(path); diff --git a/Editor/Windows/DxMessagingFlowGraphWindow.cs b/Editor/Windows/DxMessagingFlowGraphWindow.cs index c7590873..555efadb 100644 --- a/Editor/Windows/DxMessagingFlowGraphWindow.cs +++ b/Editor/Windows/DxMessagingFlowGraphWindow.cs @@ -2463,7 +2463,7 @@ FlowGraphCanvasState canvasState VisualElement legend = new() { name = GraphLegendName }; legend.style.flexDirection = FlexDirection.Row; - legend.style.flexWrap = Wrap.Wrap; + DxMessagingEditorTheme.ApplyContentSizedWrap(legend); legend.style.alignItems = Align.Center; legend.style.marginBottom = 6; @@ -6243,7 +6243,7 @@ Action onSelectionChanged VisualElement header = new(); header.AddToClassList(DxMessagingEditorTheme.DetailHeadClassName); - header.style.flexWrap = Wrap.Wrap; + DxMessagingEditorTheme.ApplyContentSizedWrap(header); VisualElement heading = new(); heading.style.flexGrow = 1; heading.style.flexShrink = 1; @@ -6757,7 +6757,7 @@ Action onSelectionChanged VisualElement summary = new(); summary.style.flexDirection = FlexDirection.Row; - summary.style.flexWrap = Wrap.Wrap; + DxMessagingEditorTheme.ApplyContentSizedWrap(summary); summary.style.alignItems = Align.Center; VisualElement receiver = CreateRelationshipIdentity("RECEIVER", string.Empty); receiver.style.flexGrow = 1; @@ -7109,7 +7109,7 @@ IEnumerable tracePaths row.Add(keyLabel); VisualElement values = new(); values.style.flexDirection = FlexDirection.Row; - values.style.flexWrap = Wrap.Wrap; + DxMessagingEditorTheme.ApplyContentSizedWrap(values); values.style.flexGrow = 1; Label traceIdentity = new( widestTrace.PathCount <= 0 ? "none" : $"Trace #{widestTrace.TraceId}" @@ -7249,7 +7249,7 @@ private static VisualElement CreateRoutePathSection(FlowGraphEdge edge) VisualElement section = CreateDetailsSection("ROUTE"); VisualElement flow = new(); flow.style.flexDirection = FlexDirection.Row; - flow.style.flexWrap = Wrap.Wrap; + DxMessagingEditorTheme.ApplyContentSizedWrap(flow); flow.style.alignItems = Align.Center; string kind = DxMessagingEditorPalette.NormalizeRouteKind(edge.RegistrationTypeName); string context = CreateReadableRouteContext(edge); @@ -7337,7 +7337,7 @@ params GraphNodeMetric[] metrics VisualElement section = CreateDetailsSection(title); VisualElement grid = new(); grid.style.flexDirection = FlexDirection.Row; - grid.style.flexWrap = Wrap.Wrap; + DxMessagingEditorTheme.ApplyContentSizedWrap(grid); foreach (GraphNodeMetric metric in metrics) { VisualElement tile = new(); @@ -7498,7 +7498,7 @@ out string descriptor }; trail.AddToClassList(DetailsHierarchyTrailClassName); trail.style.flexDirection = FlexDirection.Row; - trail.style.flexWrap = Wrap.Wrap; + DxMessagingEditorTheme.ApplyContentSizedWrap(trail); trail.style.alignItems = Align.Center; trail.style.flexGrow = 1; trail.style.flexShrink = 1; @@ -7689,7 +7689,7 @@ private static VisualElement CreateDetailsRelationship( VisualElement header = new(); header.style.flexDirection = FlexDirection.Row; - header.style.flexWrap = Wrap.Wrap; + DxMessagingEditorTheme.ApplyContentSizedWrap(header); header.style.alignItems = Align.Center; Label relationshipLabel = new(label); relationshipLabel.AddToClassList(DxMessagingEditorTheme.CardLabelClassName); @@ -7703,7 +7703,7 @@ private static VisualElement CreateDetailsRelationship( VisualElement flow = new(); flow.style.flexDirection = FlexDirection.Row; - flow.style.flexWrap = Wrap.Wrap; + DxMessagingEditorTheme.ApplyContentSizedWrap(flow); flow.style.alignItems = Align.Center; flow.style.marginTop = 6; flow.Add( @@ -7786,7 +7786,7 @@ string activityLabel { VisualElement activity = new(); activity.style.flexDirection = FlexDirection.Row; - activity.style.flexWrap = Wrap.Wrap; + DxMessagingEditorTheme.ApplyContentSizedWrap(activity); activity.style.marginTop = 5; Label activityCount = new($"{deliveryCount} {activityLabel}"); activityCount.AddToClassList(DxMessagingEditorTheme.DetailFrameClassName); diff --git a/Editor/Windows/DxMessagingMessageMonitorWindow.cs b/Editor/Windows/DxMessagingMessageMonitorWindow.cs index d3e39c51..9ed689fe 100644 --- a/Editor/Windows/DxMessagingMessageMonitorWindow.cs +++ b/Editor/Windows/DxMessagingMessageMonitorWindow.cs @@ -30,8 +30,11 @@ public sealed class DxMessagingMessageMonitorWindow : EditorWindow "dxmessaging-monitor-active-filter-label"; internal const string ActiveFilterTokenScrollViewName = "dxmessaging-monitor-active-filter-token-scroll"; + internal const string ActiveFilterTokenWrapRowName = + "dxmessaging-monitor-active-filter-token-row"; internal const string ActiveFilterTokenClassName = "dxmessaging-monitor-active-filter-token"; + internal const string LanePillWrapRowName = "dxmessaging-monitor-lane-pill-row"; internal const string ActiveFilterClearButtonName = "dxmessaging-monitor-active-filter-clear"; internal const string RefreshButtonName = "dxmessaging-monitor-refresh"; @@ -1369,6 +1372,24 @@ IReadOnlyList filteredEntries return breakdown; } + /// + /// A wrapping row for content inside a . + /// + /// The row belongs to this window rather than being the scroll view's own content + /// container. Unity sizes that container to the viewport, so a wrapping content container + /// keeps only the first screenful of lines: the rest are clipped and the scroller reports + /// nothing to scroll to. An owned row is sized by its own lines, which the scroll view + /// then scrolls (GitHub #440). + /// + private static VisualElement CreateScrollableWrapRow(string name) + { + VisualElement row = new() { name = name }; + row.style.flexDirection = FlexDirection.Row; + row.style.flexShrink = 0; + DxMessagingEditorTheme.ApplyContentSizedWrap(row); + return row; + } + private static VisualElement CreateLanePanel( string panelName, string summaryLabelName, @@ -1406,13 +1427,13 @@ IEnumerable pills laneRows.style.flexShrink = 1; laneRows.style.minHeight = 0; laneRows.style.marginTop = 4; - laneRows.contentContainer.style.flexDirection = FlexDirection.Row; - laneRows.contentContainer.style.flexWrap = Wrap.Wrap; + VisualElement laneRowsContent = CreateScrollableWrapRow(LanePillWrapRowName); + laneRows.Add(laneRowsContent); lanesRoot.Add(laneRows); foreach (VisualElement pill in pills) { - laneRows.Add(pill); + laneRowsContent.Add(pill); } return lanesRoot; @@ -1897,8 +1918,8 @@ out string[] displayTokens tokenScroll.style.flexGrow = 1; tokenScroll.style.flexShrink = 1; tokenScroll.style.maxHeight = 72; - tokenScroll.contentContainer.style.flexDirection = FlexDirection.Row; - tokenScroll.contentContainer.style.flexWrap = Wrap.Wrap; + VisualElement tokenRow = CreateScrollableWrapRow(ActiveFilterTokenWrapRowName); + tokenScroll.Add(tokenRow); summary.Add(tokenScroll); foreach (string token in displayTokens) @@ -1917,7 +1938,7 @@ out string[] displayTokens DxMessagingEditorPalette.Border ); tokenLabel.style.whiteSpace = WhiteSpace.Normal; - tokenScroll.Add(tokenLabel); + tokenRow.Add(tokenLabel); } } diff --git a/README.md b/README.md index 89b40515..2d4563c2 100644 --- a/README.md +++ b/README.md @@ -985,7 +985,7 @@ DxMessaging publishes machine-readable context for AI agents through [llms.txt]( - **[llms.txt](llms.txt)** -- Complete project overview, API reference, and context in a single file - **[Repository Guidelines](.llm/context.md)** -- Coding standards and development workflows -- **[AI Agent Skills](.llm/skills/)** -- 31+ specialized skills covering documentation, testing, GitHub Actions, and more +- **[AI Agent Skills](.llm/skills/)** -- 32+ specialized skills covering documentation, testing, GitHub Actions, and more The `llms.txt` file is automatically updated via CI/CD to stay current with project changes. It includes: diff --git a/Tests/Editor/DxMessagingEditorThemeTests.cs b/Tests/Editor/DxMessagingEditorThemeTests.cs index 6ab61425..3b3115cf 100644 --- a/Tests/Editor/DxMessagingEditorThemeTests.cs +++ b/Tests/Editor/DxMessagingEditorThemeTests.cs @@ -25,6 +25,31 @@ public sealed class DxMessagingEditorThemeTests "white-space: nowrap;", }; + private const float ProbeWindowWidth = 240f; + private const float ProbeWindowHeight = 240f; + private const float ProbeChildWidth = 100f; + private const float ProbeChildHeight = 20f; + private const int ProbeChildCount = 4; + + /// + /// How many probe children fit on one line at . + /// + private const int SingleLineChildCount = 2; + + /// + /// One line of children, which is exactly what Unity 2021.3 leaves a wrapping container at. + /// + private const float PinnedHeight = ProbeChildHeight; + + private readonly List _createdWindows = new(); + + [TearDown] + public void TearDown() + { + EditorWindowTestUtility.SuppressHeadlessWindowRenderErrors(); + EditorWindowTestUtility.CloseTrackedWindows(_createdWindows); + } + [Test] public void ThemeAssetsLoadFromPackagePaths() { @@ -670,6 +695,228 @@ public void ApplyCompleteBorderSupportsUniformCustomWidth() AssertColor(element.style.borderLeftColor.value, DxMessagingEditorPalette.AmberSoft); } + /// + /// Issue #440: on Unity 2021.3 a wrapping container keeps its single-line height, so the + /// lines its children wrap onto draw outside it and over whatever the window paints + /// beneath. Pinning the height reproduces that refusal to grow on every editor, which is + /// what makes this red-green here as well as on the 2021.3 leg. + /// + [Test] + public void AWrappingContainerTooShortForItsLinesGrowsUntilEveryChildIsInsideIt() + { + EditorWindow window = CreateWrapProbeWindow(); + + VisualElement control = AddWrapProbe(window, PinnedHeight, contentSized: false); + VisualElement fixedProbe = AddWrapProbe(window, PinnedHeight, contentSized: true); + EditorWindowTestUtility.SettleLayout(window); + + Assert.That( + DxMessagingEditorTheme.MeasureWrappedContentHeight(control), + Is.GreaterThan( + control.resolvedStyle.height + EditorWindowTestUtility.LayoutTolerance + ), + "The control probe must reproduce the defect, otherwise the fixed probe below " + + "proves nothing: a wrapping container that will not grow leaves its " + + "wrapped lines outside its own box." + ); + + Assert.That( + DxMessagingEditorTheme.MeasureWrappedContentHeight(fixedProbe), + Is.LessThanOrEqualTo( + fixedProbe.resolvedStyle.height + EditorWindowTestUtility.LayoutTolerance + ), + "A container built through ApplyContentSizedWrap must end up tall enough for " + + "every line its children wrap onto." + ); + } + + /// + /// An editor that already sizes a wrapping container must be left alone: no inline + /// `min-height` is written, so the helper cannot lower a height the stylesheet declared. + /// + [Test] + public void AWrappingContainerThatAlreadyFitsIsLeftWithoutAnInlineHeight() + { + EditorWindow window = CreateWrapProbeWindow(); + + // Two children fit on one line, so no editor has to derive a wrapped height and this + // reads the same on Unity 2021.3 as it does on 6000.x. + VisualElement probe = AddWrapProbe( + window, + pinnedHeight: 0f, + contentSized: true, + childCount: SingleLineChildCount + ); + EditorWindowTestUtility.SettleLayout(window); + + Assert.That( + DxMessagingEditorTheme.MeasureWrappedContentHeight(probe), + Is.LessThanOrEqualTo( + probe.resolvedStyle.height + EditorWindowTestUtility.LayoutTolerance + ), + "The probe must already fit, otherwise this asserts the wrong case." + ); + Assert.That( + probe.style.minHeight.keyword, + Is.EqualTo(StyleKeyword.Null), + "A container that already fits its lines must keep the height its stylesheet " + + "gives it, so the helper must not write an inline min-height." + ); + } + + /// + /// A container that leaves the panel is about to be rebuilt or reused, so it must forget + /// the height its last content needed. Otherwise a details pane that once held a deep + /// hierarchy trail keeps that gap for every selection after it. + /// + [Test] + public void AGrownWrappingContainerForgetsItsHeightWhenItLeavesThePanel() + { + EditorWindow window = CreateWrapProbeWindow(); + + VisualElement probe = AddWrapProbe(window, PinnedHeight, contentSized: true); + EditorWindowTestUtility.SettleLayout(window); + Assert.That( + probe.resolvedStyle.height, + Is.GreaterThan(PinnedHeight + EditorWindowTestUtility.LayoutTolerance), + "The probe must have grown first, otherwise there is no height to forget." + ); + + probe.RemoveFromHierarchy(); + EditorWindowTestUtility.SettleLayout(window); + + Assert.That( + probe.style.minHeight.keyword, + Is.EqualTo(StyleKeyword.Null), + "Leaving the panel must release the height, so the container measures its next " + + "content from its own stylesheet height." + ); + + while (probe.childCount > SingleLineChildCount) + { + probe.RemoveAt(probe.childCount - 1); + } + window.rootVisualElement.Add(probe); + EditorWindowTestUtility.SettleLayout(window); + + Assert.That( + probe.resolvedStyle.height, + Is.EqualTo(PinnedHeight).Within(EditorWindowTestUtility.LayoutTolerance), + "Two children need one line, so the re-added container must be one line tall." + ); + Assert.That( + DxMessagingEditorTheme.MeasureWrappedContentHeight(probe), + Is.LessThanOrEqualTo( + probe.resolvedStyle.height + EditorWindowTestUtility.LayoutTolerance + ), + "The re-measured container must still contain its children." + ); + } + + /// + /// Drift guard for the whole class of defect in issue #440: turning wrapping on by hand + /// re-creates it, because nothing then supplies the height Unity 2021.3 does not derive. + /// Every shipped editor source must go through the helper instead. + /// + [Test] + public void EveryEditorWrapContainerGoesThroughTheContentSizedWrapHelper() + { + const string bannedDeclaration = "style.flex" + "Wrap"; + string editorRoot = DxMessagingEditorTheme.PackageRoot + "/Editor"; + string allowedRelativePath = "Editor/DxMessagingEditorTheme.cs"; + + string[] sourcePaths = System.IO.Directory.GetFiles( + editorRoot, + "*.cs", + System.IO.SearchOption.AllDirectories + ); + Assert.That( + sourcePaths, + Is.Not.Empty, + $"Expected editor sources under '{editorRoot}'." + ); + + List offenders = new(); + foreach (string sourcePath in sourcePaths) + { + string normalized = sourcePath.Replace('\\', '/'); + if (normalized.EndsWith(allowedRelativePath, StringComparison.OrdinalIgnoreCase)) + { + continue; + } + + string[] lines = System.IO.File.ReadAllLines(sourcePath); + for (int index = 0; index < lines.Length; index++) + { + if (lines[index].Contains(bannedDeclaration)) + { + offenders.Add($"{normalized}:{index + 1}"); + } + } + } + + Assert.That( + offenders, + Is.Empty, + "These editor sources turn wrapping on directly instead of calling " + + "DxMessagingEditorTheme.ApplyContentSizedWrap, so on Unity 2021.3 their " + + "wrapped lines draw outside the container (GitHub #440):\n" + + string.Join("\n", offenders) + ); + } + + private EditorWindow CreateWrapProbeWindow() + { + EditorWindow window = EditorWindowTestUtility.CreateWindow(); + _createdWindows.Add(window); + window.position = new Rect(0f, 0f, ProbeWindowWidth, ProbeWindowHeight); + EditorWindowTestUtility.ShowWindow(window); + return window; + } + + /// + /// A row of equal children too wide to fit on one line. A non-zero + /// holds the container at one line's worth of height, + /// which is what Unity 2021.3 does to a wrapping container on its own. + /// + private static VisualElement AddWrapProbe( + EditorWindow window, + float pinnedHeight, + bool contentSized, + int childCount = ProbeChildCount + ) + { + VisualElement probe = new(); + probe.style.flexDirection = FlexDirection.Row; + probe.style.width = ProbeWindowWidth; + probe.style.flexShrink = 0f; + if (contentSized) + { + DxMessagingEditorTheme.ApplyContentSizedWrap(probe); + } + else + { + probe.style.flexWrap = Wrap.Wrap; + } + + if (pinnedHeight > 0f) + { + probe.style.height = pinnedHeight; + } + + for (int index = 0; index < childCount; index++) + { + VisualElement child = new(); + child.style.width = ProbeChildWidth; + child.style.height = ProbeChildHeight; + child.style.flexShrink = 0f; + probe.Add(child); + } + + window.rootVisualElement.Add(probe); + return probe; + } + private static void AssertIconLoads(string fileName, int expectedSize) { Texture2D icon = DxMessagingEditorTheme.LoadIcon(fileName); diff --git a/Tests/Editor/DxMessagingFlowGraphWindowTests.cs b/Tests/Editor/DxMessagingFlowGraphWindowTests.cs index 4f648ed4..cd0e499a 100644 --- a/Tests/Editor/DxMessagingFlowGraphWindowTests.cs +++ b/Tests/Editor/DxMessagingFlowGraphWindowTests.cs @@ -105,6 +105,17 @@ public sealed class DxMessagingFlowGraphWindowTests private const string TraceIdLaneDetailsLabelName = "dxmessaging-flow-graph-trace-id-lane-details"; + /// + /// The smallest the Flow Graph window can be made, from its own `minSize`. Containers wrap + /// here first, so this is where issue #440 shows up. + /// + private const float FlowGraphMinimumWidth = 520f; + private const float FlowGraphMinimumHeight = 360f; + + private const string EdgeSelection = "route"; + private const string ComponentSelection = "component"; + private const string MessageSelection = "message"; + private bool _stackTracesBeforeTest; [SetUp] @@ -11472,6 +11483,126 @@ private static FlowGraphSnapshot CreateStableContextRouteSnapshot(string context ); } + /// + /// Issue #440: ten Flow Graph containers wrap. Unity 2021.3 does not grow a wrapping + /// container to fit the lines its children wrap onto, so those lines draw outside the + /// container and over the block beneath it. What makes them wrap is long type names and a + /// deep hierarchy trail - short in a fixture, long in a real project - so this renders + /// both, at the smallest window a reader can make. + /// + [TestCase(EdgeSelection)] + [TestCase(ComponentSelection)] + [TestCase(MessageSelection)] + public void EveryWrappingContainerStaysInsideItsOwnBoxAtTheWindowsMinimumSize( + string selection + ) + { + FlowGraphSnapshot snapshot = CreateLongContentSnapshot(); + string selectedItemKey = selection switch + { + EdgeSelection => DxMessagingFlowGraphWindow.CreateEdgeSelectionKey( + snapshot.Edges[0] + ), + ComponentSelection => DxMessagingFlowGraphWindow.CreateComponentSelectionKey( + snapshot.ComponentNodes[0] + ), + MessageSelection => DxMessagingFlowGraphWindow.CreateMessageSelectionKey( + snapshot.MessageNodes[0] + ), + _ => throw new ArgumentOutOfRangeException(nameof(selection), selection, null), + }; + + EditorWindow window = CreateTrackedEditorWindow(); + window.position = new Rect(0f, 0f, FlowGraphMinimumWidth, FlowGraphMinimumHeight); + EditorWindowTestUtility.ShowWindow(window); + + DxMessagingFlowGraphWindow.BuildGraphUi( + window.rootVisualElement, + snapshot, + new FlowGraphViewState(selectedItemKey: selectedItemKey) + ); + EditorWindowTestUtility.SettleLayout(window); + + Assert.That( + window.rootVisualElement.Q( + DxMessagingFlowGraphWindow.DetailsPaneName + ), + Is.Not.Null, + "The details pane carries most of the wrapping containers, so it has to render." + ); + EditorWindowTestUtility.AssertWrappingContainersContainTheirChildren( + window.rootVisualElement, + $"The Flow Graph at its minimum window size with a {selection} selected" + ); + } + + /// + /// A snapshot whose names are as long as a real project's: a deep hierarchy trail, a + /// namespaced message type, and traces, so the hierarchy trail, metric grid, route flow + /// and widest-trace rows all have enough content to wrap. + /// + private static FlowGraphSnapshot CreateLongContentSnapshot() + { + const string componentId = "component:damage-resolution"; + const string hierarchyPath = + "Root/Systems/Gameplay/Combat/Encounters/EliteWaveDirector/DamageResolutionListener"; + const string messageTypeName = + "Gameplay.Combat.Damage.DamageResolutionRequestedBroadcastMessage"; + + return new FlowGraphSnapshot( + new[] + { + new FlowGraphComponentNode( + componentId, + hierarchyPath, + "PlayerCombatDamageResolutionMessagingComponent", + activeInHierarchy: true, + listenerCount: 3, + registrationCount: 4, + callCount: 128, + localMessageCount: 6 + ), + }, + new[] + { + new FlowGraphMessageNode( + messageTypeName, + registrationCount: 4, + callCount: 128, + recentGlobalEmissionCount: 64, + recentLocalMessageCount: 6, + recentTracedDeliveryCount: 12 + ), + }, + new[] + { + new FlowGraphEdge( + messageTypeName, + componentId, + hierarchyPath, + "Broadcast", + registrationCount: 4, + callCount: 128, + recentTracedDeliveryCount: 12, + context: hierarchyPath + ), + }, + new[] + { + new FlowGraphTracePath( + messageTypeName, + hierarchyPath, + componentId, + hierarchyPath, + "Broadcast", + recentTracedDeliveryCount: 12, + traceIds: new long[] { 8801, 8802, 8803, 8804 } + ), + }, + Array.Empty() + ); + } + private static FlowGraphSnapshot CreateTwoEdgeSnapshot() { return new FlowGraphSnapshot( diff --git a/Tests/Editor/DxMessagingMessageMonitorWindowTests.cs b/Tests/Editor/DxMessagingMessageMonitorWindowTests.cs index 7223f628..509258fd 100644 --- a/Tests/Editor/DxMessagingMessageMonitorWindowTests.cs +++ b/Tests/Editor/DxMessagingMessageMonitorWindowTests.cs @@ -26,6 +26,9 @@ public sealed class DxMessagingMessageMonitorWindowTests "Packages/com.wallstop-studios.dxmessaging/Tests/Editor/Fixtures/EditModeSceneSafety.unity"; private readonly List _createdObjects = new(); private readonly List _createdAssetPaths = new(); + private const float MonitorMinimumWidth = 420f; + private const float MonitorMinimumHeight = 320f; + private readonly List _createdWindows = new(); private const string MessageTypeLanesName = "dxmessaging-monitor-message-type-lanes"; private const string MessageTypeLaneScrollViewName = @@ -3725,6 +3728,101 @@ public void CaptureSnapshotReadsDefaultMessageBusHistory() Assert.That(snapshot.Entries[1].MessageTypeName, Is.EqualTo(nameof(OlderMessage))); } + /// + /// Wrapping the scroll view's own content container clipped the token list to the + /// viewport and left the scroller with nothing to scroll, so every token past the first + /// few lines was unreachable. The tokens wrap in a row this window owns, which the scroll + /// view can then scroll (GitHub #440). + /// + [Test] + public void ActiveFilterTokensStayReachableWhenTheyWrapPastTheScrollViewHeight() + { + MessageMonitorSnapshot snapshot = new( + diagnosticsEnabled: true, + capacity: 8, + entries: new[] { CreateEntry(new OlderMessage(), null) } + ); + string longFilter = string.Join( + " ", + Enumerable.Range(0, 24).Select(index => $"type:Message{index:00}") + ); + EditorWindow window = CreateTrackedEditorWindow(); + window.position = new Rect(0f, 0f, MonitorMinimumWidth, MonitorMinimumHeight); + EditorWindowTestUtility.ShowWindow(window); + + DxMessagingMessageMonitorWindow.BuildMonitorUi( + window.rootVisualElement, + snapshot, + new MessageMonitorViewState(longFilter) + ); + EditorWindowTestUtility.SettleLayout(window); + + ScrollView tokenScroll = window + .rootVisualElement.Q(ActiveFilterSummaryName) + .Q(ActiveFilterTokenScrollViewName); + VisualElement tokenRow = tokenScroll.Q( + DxMessagingMessageMonitorWindow.ActiveFilterTokenWrapRowName + ); + + Assert.That(tokenRow, Is.Not.Null, "The tokens must wrap in a row the window owns."); + Assert.That( + tokenRow.resolvedStyle.height, + Is.GreaterThan(tokenScroll.contentViewport.resolvedStyle.height), + "The probe must overflow the viewport, otherwise there is nothing to scroll to." + ); + Assert.That( + tokenScroll.verticalScroller.highValue, + Is.GreaterThan(0f), + "Every token past the first visible line must be reachable by scrolling." + ); + } + + /// + /// Issue #440: the Monitor's lane rows and active-filter tokens are wrapping containers + /// too. Unity 2021.3 does not grow a wrapping container to fit the lines its children wrap + /// onto, so those lines fall outside their scroll view's content and cannot be scrolled to. + /// A long filter and several message kinds are what fills both at the minimum window size. + /// + [Test] + public void EveryWrappingContainerStaysInsideItsOwnBoxAtTheWindowsMinimumSize() + { + MessageMonitorSnapshot snapshot = new( + diagnosticsEnabled: true, + capacity: 8, + entries: new[] + { + CreateEntry(new OlderMessage(), null), + CreateEntry(new NewerMessage(), new InstanceId(123)), + } + ); + string longFilter = string.Join( + " ", + Enumerable.Range(0, 24).Select(index => $"type:Message{index:00}") + ); + EditorWindow window = CreateTrackedEditorWindow(); + window.position = new Rect(0f, 0f, MonitorMinimumWidth, MonitorMinimumHeight); + EditorWindowTestUtility.ShowWindow(window); + + DxMessagingMessageMonitorWindow.BuildMonitorUi( + window.rootVisualElement, + snapshot, + new MessageMonitorViewState(longFilter) + ); + EditorWindowTestUtility.SettleLayout(window); + + Assert.That( + window + .rootVisualElement.Q(ActiveFilterSummaryName) + .Q(ActiveFilterTokenScrollViewName), + Is.Not.Null, + "The active-filter token scroll view is one of the wrapping containers this covers." + ); + EditorWindowTestUtility.AssertWrappingContainersContainTheirChildren( + window.rootVisualElement, + "The Message Monitor at its minimum window size" + ); + } + private static MessageMonitorEntry CreateEntry(IMessage message, InstanceId? context) { return MessageMonitorEntry.FromEmission(new MessageEmissionData(message, context)); diff --git a/Tests/Editor/EditorWindowTestUtility.cs b/Tests/Editor/EditorWindowTestUtility.cs index 448161a4..bc013699 100644 --- a/Tests/Editor/EditorWindowTestUtility.cs +++ b/Tests/Editor/EditorWindowTestUtility.cs @@ -3,9 +3,12 @@ namespace DxMessaging.Tests.Editor { using System; using System.Collections.Generic; + using DxMessaging.Editor; + using NUnit.Framework; using UnityEditor; using UnityEngine; using UnityEngine.TestTools; + using UnityEngine.UIElements; using Object = UnityEngine.Object; internal sealed class DxMessagingTestHostWindow : EditorWindow @@ -17,6 +20,11 @@ internal static class EditorWindowTestUtility { private static readonly List CreatedWindows = new(); + /// + /// Sub-pixel slack for layout comparisons: Yoga rounds to the panel's pixel grid. + /// + internal const float LayoutTolerance = 0.5f; + internal static DxMessagingTestHostWindow CreateWindow() { DxMessagingTestHostWindow window = @@ -213,6 +221,91 @@ internal static void IgnoreUnityInvalidGcHandleAsserts(Action action) } } + /// + /// Runs the panel's layout to a settled state, so a test reads the geometry a reader sees. + /// The first pass realizes content and can change what elements measure; the second + /// measures the result, including any height a + /// container asked for during + /// the first pass. + /// + internal static void SettleLayout(EditorWindow window) + { + if (window == null) + { + return; + } + + for (int pass = 0; pass < 2; pass++) + { + EditorSurfaceCapture.InvokeInheritedPanelMethod( + window.rootVisualElement.panel, + "ValidateLayout", + Array.Empty() + ); + } + } + + /// + /// Asserts every wrapping container under is tall enough for the + /// lines its children wrap onto. + /// + /// Unity 2021.3 does not grow a wrapping container to fit those lines (issues #435 and + /// #440), so the extra lines draw outside the container, on top of whatever the window + /// paints beneath. This assertion is what reports that on the 2021.3 leg; on newer editors + /// it holds without the fix, because they size the container correctly. + /// + internal static void AssertWrappingContainersContainTheirChildren( + VisualElement root, + string context + ) + { + Assert.That(root, Is.Not.Null, $"{context} must render a root element."); + + List wrapping = new(); + foreach (VisualElement element in root.Query().ToList()) + { + if (element.resolvedStyle.flexWrap != Wrap.Wrap || element.childCount == 0) + { + continue; + } + + wrapping.Add(element); + } + + Assert.That( + wrapping, + Is.Not.Empty, + $"{context} renders no wrapping container, so this assertion would pass " + + "without checking anything. Point it at a surface that wraps." + ); + + foreach (VisualElement element in wrapping) + { + Assert.That( + DxMessagingEditorTheme.MeasureWrappedContentHeight(element), + Is.LessThanOrEqualTo(element.resolvedStyle.height + LayoutTolerance), + $"{context}: wrapping container '{DescribeElement(element)}' is shorter than " + + "the lines its children wrap onto, so those lines draw outside it and " + + "over whatever the window paints beneath." + ); + } + } + + private static string DescribeElement(VisualElement element) + { + if (!string.IsNullOrEmpty(element.name)) + { + return element.name; + } + + foreach (string className in element.GetClasses()) + { + return "." + className; + } + + return element.GetType().Name; + } + private static System.Type FindUnityEditorType(string fullName) { foreach ( diff --git a/llms.txt b/llms.txt index 7fa1a29f..a6234762 100644 --- a/llms.txt +++ b/llms.txt @@ -214,7 +214,7 @@ npm run check:spelling This repository includes comprehensive AI agent guidance in the `.llm/` directory: - **[.llm/context.md](https://github.com/Ambiguous-Interactive/DxMessaging/blob/master/.llm/context.md)** - Repository guidelines, coding standards, testing policies -- **[.llm/skills/](https://github.com/Ambiguous-Interactive/DxMessaging/tree/master/.llm/skills)** - 31+ specialized skills: +- **[.llm/skills/](https://github.com/Ambiguous-Interactive/DxMessaging/tree/master/.llm/skills)** - 32+ specialized skills: - **allocation-and-leak-coverage** - **api-design-patterns** - **benchmark-methodology** @@ -232,6 +232,7 @@ This repository includes comprehensive AI agent guidance in the `.llm/` director - **memory-reclamation** - **object-pooling** - **package-publishing** + - **pull-request-writing** - **serializable-dictionary** - **shell-and-powershell** - **singleton-patterns** @@ -326,5 +327,5 @@ Copyright (c) 2017-2026 Wallstop Studios --- -**Last Updated:** 2026-08-16 +**Last Updated:** 2026-08-22 **Generated by:** scripts/update-llms-txt.js using package.json v3.3.0 and .llm/skills metadata From 73bc304cd7d1dc50232e42bc16c7d46d57e01f72 Mon Sep 17 00:00:00 2001 From: wallstop Date: Sat, 22 Aug 2026 05:49:40 +0000 Subject: [PATCH 2/9] Report an MSVC compiler that is present but cannot start The IL2CPP gate checked that cl.exe exists. A cl.exe that is there but cannot start, from a failed update or a missing sibling DLL, passed the gate. The leg then took a Unity licence seat and the build lock and failed twenty minutes later with a message that named neither the host nor the cause. The gate now also starts the compiler and reports a third verdict, compiler-unusable, that names the symptom and the runner. The launch verdict is advisory, not blocking. This gate runs before the organization build lock, so a probe that is wrong about a healthy host would block every IL2CPP leg on that runner, and no Windows host has yet shown the probe is clean on a healthy toolchain. Enforcing it is one field change once someone demonstrates that. Both cases run off Windows through the injected probe. A pure function reads the launch outcome, so the NTSTATUS exit codes are covered too. Advances #336. Co-Authored-By: Claude Opus 5 (1M context) --- .llm/index.json | 2 +- .llm/skills/shell-and-powershell/SKILL.md | 4 + scripts/__tests__/test-msvc-toolchain.ps1 | 83 +++++++++++- scripts/unity/assert-msvc-toolchain.ps1 | 150 +++++++++++++++++----- 4 files changed, 202 insertions(+), 37 deletions(-) diff --git a/.llm/index.json b/.llm/index.json index d5dcc650..310bfeba 100644 --- a/.llm/index.json +++ b/.llm/index.json @@ -330,7 +330,7 @@ "category": "scripting", "tags": "cross-platform, case-sensitivity, testing, powershell, javascript, ci-cd, linux, windows, macos" }, - "lineCount": 124, + "lineCount": 128, "references": [ ".llm/skills/shell-and-powershell/references/cross-platform-compatibility.md", ".llm/skills/shell-and-powershell/references/powershell-best-practices-part-1.md", diff --git a/.llm/skills/shell-and-powershell/SKILL.md b/.llm/skills/shell-and-powershell/SKILL.md index 04ec139a..dcff5b4b 100644 --- a/.llm/skills/shell-and-powershell/SKILL.md +++ b/.llm/skills/shell-and-powershell/SKILL.md @@ -99,6 +99,10 @@ application for this OS platform`, or a case-mismatch file-not-found. `os.tmpdir()` on `C:`), so `rel.startsWith("..")` mislabels an outside path as inside. Use `isPathInsideDirectory` / `isPathOutsideDirectory` / `isOutsideRelative` from `scripts/lib/path-classifier.js`; if you truly cannot, pair with `path.isAbsolute(rel)`. +- PowerShell parses an unsuffixed hex literal as a SIGNED integer, so `0xC0000000` is + `-1073741824`, not `3221225472`. Comparing a Windows exit code against a bare NTSTATUS + literal therefore matches everything. Suffix it (`0xC0000000L`) or write the decimal. See + `Test-CompilerLaunchOutcome` in `scripts/unity/assert-msvc-toolchain.ps1`. - GNU tar reads an archive operand containing an unqualified colon as a remote spec, so `tar -f C:\Temp\package.tgz` fails. Set the subprocess `cwd` to the archive directory and pass `./`; use `buildLocalTarArchiveSpec()` from `scripts/validate-npm-meta.js` diff --git a/scripts/__tests__/test-msvc-toolchain.ps1 b/scripts/__tests__/test-msvc-toolchain.ps1 index 4efc8c0f..fc04513f 100644 --- a/scripts/__tests__/test-msvc-toolchain.ps1 +++ b/scripts/__tests__/test-msvc-toolchain.ps1 @@ -31,12 +31,28 @@ function Assert-Verdict { [string]$Name, [string]$InstallRoot, [string[]]$ExecutablePaths = @(), + [string[]]$LaunchablePaths, [string]$ExpectedReason, - [bool]$ExpectedOk + [bool]$ExpectedOk, + [bool]$ExpectedAdvisory = $false ) $script:Count++ $probe = { param($path) $ExecutablePaths -contains $path }.GetNewClosure() - $result = Test-MsvcToolchain -InstallRoot $InstallRoot -ProbeExecutable $probe -RunnerName 'TEST-RUNNER' + $arguments = @{ + InstallRoot = $InstallRoot + ProbeExecutable = $probe + RunnerName = 'TEST-RUNNER' + } + if ($PSBoundParameters.ContainsKey('LaunchablePaths')) { + $arguments['ProbeLaunch'] = { param($path) $LaunchablePaths -contains $path }.GetNewClosure() + } + $result = Test-MsvcToolchain @arguments + if ($result.Advisory -ne $ExpectedAdvisory) { + Write-Output ("FAIL $Name`n" + + " expected Advisory=$ExpectedAdvisory, got $($result.Advisory)") + $script:Failures++ + return + } if ($result.Ok -ne $ExpectedOk -or $result.Reason -ne $ExpectedReason) { Write-Output ("FAIL $Name`n" + " expected Ok=$ExpectedOk Reason=$ExpectedReason`n" + @@ -104,6 +120,69 @@ try { } elseif ($VerboseOutput) { Write-Output 'ok newest toolset is chosen by version, not by name' } + # LAUNCH PROBE (#336). Presence alone reports a `cl.exe` that cannot start as + # healthy, and the leg then dies after taking a licence seat and the build + # lock. The probe is injected exactly like the presence probe, so both the + # healthy and the corrupt case run here rather than needing a Windows host. + Assert-Verdict -Name 'a compiler that starts passes the launch probe' ` + -InstallRoot $root -ExecutablePaths @($missing) -LaunchablePaths @($missing) ` + -ExpectedOk $true -ExpectedReason 'ok' + + # A present compiler that will not start is its own verdict, distinct from + # absent, and it is ADVISORY: this gate runs before the organization build + # lock, so it reports rather than blocks until a Windows host demonstrates the + # probe is clean on a healthy toolchain. + Assert-Verdict -Name 'a present compiler that will not start is compiler-unusable' ` + -InstallRoot $root -ExecutablePaths @($missing) -LaunchablePaths @() ` + -ExpectedOk $false -ExpectedReason 'compiler-unusable' -ExpectedAdvisory $true + + # An absent compiler stays `compiler-missing` even with a launch probe wired + # in, so #333's verdict and its blocking failure are unchanged. + Assert-Verdict -Name 'an absent compiler is still compiler-missing with a launch probe' ` + -InstallRoot $root -ExecutablePaths @() -LaunchablePaths @() ` + -ExpectedOk $false -ExpectedReason 'compiler-missing' + + # A newer broken toolset must not hide an older working one, the same way a + # newer absent one already does not. + Assert-Verdict -Name 'falls back past a newer toolset that will not start' ` + -InstallRoot $root -ExecutablePaths @($missing, $older) -LaunchablePaths @($older) ` + -ExpectedOk $true -ExpectedReason 'ok' + + # The three failure messages must stay distinguishable: the operator fix for + # each one differs. + $script:Count++ + $unusable = Test-MsvcToolchain -InstallRoot $root -RunnerName 'TEST-RUNNER' ` + -ProbeExecutable { param($p) $true } -ProbeLaunch { param($p) $false } + if ($unusable.Message -notmatch 'did not start' -or $unusable.Message -notmatch 'TEST-RUNNER') { + Write-Output ("FAIL the compiler-unusable message names the symptom and the runner`n" + + " got $($unusable.Message)") + $script:Failures++ + } + elseif ($VerboseOutput) { Write-Output 'ok the compiler-unusable message names the symptom and the runner' } + + # HOW A LAUNCH OUTCOME IS READ. `cl.exe` with no input files exits non-zero on + # a healthy toolchain, so requiring exit 0 would call a working compiler + # broken. Windows reports a loader failure as an NTSTATUS instead. + $launchCases = @( + @{ Name = 'exit 0 started'; ExitCode = 0; Threw = $false; Expected = $true } + @{ Name = 'exit 2 (no input files) still started'; ExitCode = 2; Threw = $false; Expected = $true } + @{ Name = 'STATUS_DLL_NOT_FOUND did not start'; ExitCode = -1073741515; Threw = $false; Expected = $false } + @{ Name = 'STATUS_INVALID_IMAGE_FORMAT did not start'; ExitCode = -1073741701; Threw = $false; Expected = $false } + @{ Name = 'unsigned NTSTATUS did not start'; ExitCode = 3221225781; Threw = $false; Expected = $false } + @{ Name = 'a throw did not start'; ExitCode = 0; Threw = $true; Expected = $false } + @{ Name = 'no exit code at all did not start'; ExitCode = $null; Threw = $false; Expected = $false } + ) + foreach ($case in $launchCases) { + $script:Count++ + $actual = Test-CompilerLaunchOutcome -ExitCode $case.ExitCode -Threw $case.Threw + if ($actual -ne $case.Expected) { + Write-Output ("FAIL launch outcome: $($case.Name)`n" + + " expected $($case.Expected), got $actual") + $script:Failures++ + } + elseif ($VerboseOutput) { Write-Output "ok launch outcome: $($case.Name)" } + } + # A non-version directory must not crash the comparison. $null = New-Toolset -Version 'not-a-version' Assert-Verdict -Name 'a non-version toolset directory does not break sorting' ` diff --git a/scripts/unity/assert-msvc-toolchain.ps1 b/scripts/unity/assert-msvc-toolchain.ps1 index 920fbcd4..b3b6a479 100644 --- a/scripts/unity/assert-msvc-toolchain.ps1 +++ b/scripts/unity/assert-msvc-toolchain.ps1 @@ -80,15 +80,17 @@ function Test-MsvcToolchain { param( [AllowEmptyString()][string]$InstallRoot, [scriptblock]$ProbeExecutable, + [scriptblock]$ProbeLaunch, [string]$RunnerName = 'unknown' ) if ([string]::IsNullOrWhiteSpace($InstallRoot)) { return [pscustomobject]@{ - Ok = $false - Reason = 'no-visual-studio' - Path = '' - Message = ("No Visual Studio installation with the C++ toolset was found on runner " + + Ok = $false + Advisory = $false + Reason = 'no-visual-studio' + Path = '' + Message = ("No Visual Studio installation with the C++ toolset was found on runner " + "'$RunnerName'. IL2CPP cannot compile a player without it. A runner administrator " + "must install the 'Desktop development with C++' workload.") } @@ -111,33 +113,66 @@ function Test-MsvcToolchain { if ($versions.Count -eq 0) { return [pscustomobject]@{ - Ok = $false - Reason = 'no-toolset' - Path = $toolsRoot - Message = ("Visual Studio is installed on runner '$RunnerName' but carries no MSVC " + + Ok = $false + Advisory = $false + Reason = 'no-toolset' + Path = $toolsRoot + Message = ("Visual Studio is installed on runner '$RunnerName' but carries no MSVC " + "toolset under '$toolsRoot'. IL2CPP cannot compile a player. A runner " + "administrator must add the 'Desktop development with C++' workload.") } } + # A compiler that is present but cannot start is tracked separately from one + # that is absent, because the operator fix differs and because a newer broken + # toolset must not hide an older working one. + $unusable = $null foreach ($version in $versions) { $compiler = Join-Path $version.FullName 'bin\Hostx64\x64\cl.exe' - if (& $ProbeExecutable $compiler) { - return [pscustomobject]@{ - Ok = $true - Reason = 'ok' - Path = $compiler - Message = "MSVC toolset $($version.Name) is usable: $compiler" - } + if (-not (& $ProbeExecutable $compiler)) { continue } + if ($ProbeLaunch -and -not (& $ProbeLaunch $compiler)) { + if ($null -eq $unusable) { $unusable = [pscustomobject]@{ Name = $version.Name; Path = $compiler } } + continue + } + return [pscustomobject]@{ + Ok = $true + Advisory = $false + Reason = 'ok' + Path = $compiler + Message = "MSVC toolset $($version.Name) is usable: $compiler" + } + } + + if ($null -ne $unusable) { + return [pscustomobject]@{ + Ok = $false + # ADVISORY, not blocking. This gate runs before the organization build + # lock, so a false failure would block every IL2CPP leg on the runner. + # No Windows host has yet demonstrated that the launch probe exits + # cleanly on a HEALTHY toolchain invoked by full path outside vcvars + # (#336, step 1), so until it does, a failed launch is reported and the + # leg continues. Flip this to $false in the same change that records + # that demonstration. + Advisory = $true + Reason = 'compiler-unusable' + Path = $unusable.Path + Message = ("MSVC toolset $($unusable.Name) is present on runner '$RunnerName' but " + + "its compiler did not start at '$($unusable.Path)'. A compiler that cannot " + + "start is normally a missing sibling DLL, a truncated file, or security " + + "tooling blocking it, and IL2CPP will fail the player build after taking a " + + "Unity licence seat and the build lock. A runner administrator must repair the " + + "'Desktop development with C++' workload. This is reported, not enforced: see " + + "issue #336.") } } $newest = $versions[0] return [pscustomobject]@{ - Ok = $false - Reason = 'compiler-missing' - Path = (Join-Path $newest.FullName 'bin\Hostx64\x64\cl.exe') - Message = ("MSVC toolset $($newest.Name) is present on runner '$RunnerName' but its " + + Ok = $false + Advisory = $false + Reason = 'compiler-missing' + Path = (Join-Path $newest.FullName 'bin\Hostx64\x64\cl.exe') + Message = ("MSVC toolset $($newest.Name) is present on runner '$RunnerName' but its " + "compiler is not executable at " + "'$(Join-Path $newest.FullName 'bin\Hostx64\x64\cl.exe')'. This is a broken or " + "half-completed Visual Studio update, not a missing install -- Unity will still " + @@ -147,6 +182,40 @@ function Test-MsvcToolchain { } } +function Test-CompilerLaunchOutcome { + <# + .SYNOPSIS + Decide whether a compiler process actually started, from how it ended. + + .DESCRIPTION + Pure, so every branch runs on Linux. The signal is that the process + STARTED, not that it liked its arguments: `cl.exe` invoked with no input + files still exits non-zero on a healthy toolchain, so requiring exit 0 + would call a working compiler broken. + + Windows reports a loader failure -- a missing sibling DLL, a truncated + image, an execution block -- as an NTSTATUS in the exit code rather than + as a compiler diagnostic. `0xC0000135` is STATUS_DLL_NOT_FOUND and + `0xC000007B` is STATUS_INVALID_IMAGE_FORMAT. PowerShell surfaces those as + negative signed integers, so both forms are checked. + #> + [CmdletBinding()] + param( + [AllowNull()][object]$ExitCode, + [bool]$Threw + ) + + if ($Threw) { return $false } + if ($null -eq $ExitCode) { return $false } + + # `0xC0000000L`, with the L. PowerShell parses an unsuffixed hex literal as a + # SIGNED int, so a bare `0xC0000000` is -1073741824 and `0 -ge -1073741824` + # would call every healthy exit code a loader failure. + $code = [int64]$ExitCode + if ($code -lt 0 -or $code -ge 0xC0000000L) { return $false } + return $true +} + # Dot-sourced by the tests, which supply their own inputs. if ($MyInvocation.InvocationName -eq '.') { return } @@ -221,23 +290,36 @@ else { "with C++' workload.") } -# Presence, deliberately -- NOT a launch probe. Running `cl.exe /?` would also -# catch a present-but-corrupt compiler, which this cannot. It was considered and -# rejected for now: `cl.exe` resolves several sibling DLLs (`mspdb*`, `msvcp*`) -# and the toolchain normally runs under `vcvars`, so a bare launch has real ways -# to fail on a HEALTHY host -- and this gate runs before the organization build -# lock, so a false failure blocks every IL2CPP leg on that runner. That is the -# same shape as the `vswhere` bug above, where a discovery failure was reported -# as a missing toolchain. +# Presence AND a launch probe. Presence is what #333 needed and is the blocking +# check. The launch probe catches a `cl.exe` that is there but cannot start -- a +# failed update, a missing sibling DLL, security tooling -- which presence cannot +# see and which today reports the host healthy and fails the leg twenty minutes +# later, after taking a licence seat and the build lock. # -# The asymmetry decides it: a missing `cl.exe` is what #333 actually was, and -# presence catches it. A corrupt-but-present compiler is rarer and still fails -# the build, just later. Adding a launch probe needs a Windows host where both -# the healthy and the corrupt case can be demonstrated, which is #336, not a -# change made blind. (GitHub Copilot raised this.) +# The launch verdict is ADVISORY. This gate runs before the organization build +# lock, so a launch probe that is wrong about a HEALTHY host would block every +# IL2CPP leg on that runner. That risk is real: `cl.exe` resolves sibling DLLs +# from its own directory and the toolchain normally runs under `vcvars`, and no +# Windows host has yet demonstrated the healthy case (#336, step 1). Reporting it +# costs nothing and names the cause up front; enforcing it needs that +# demonstration first. $result = Test-MsvcToolchain -InstallRoot $installRoot -RunnerName $runner -ProbeExecutable { param($path) Test-Path -LiteralPath $path -PathType Leaf +} -ProbeLaunch { + param($path) + $threw = $false + $code = $null + try { + # By full path, with no input files, output discarded: the question is + # whether the image loads, not what it says. + $null = & $path '/?' 2>&1 + $code = $LASTEXITCODE + } + catch { + $threw = $true + } + Test-CompilerLaunchOutcome -ExitCode $code -Threw $threw } if ($result.Ok) { @@ -245,8 +327,8 @@ if ($result.Ok) { exit 0 } -if ($DetectOnly) { - Write-Output "::warning::$($result.Message)" +if ($DetectOnly -or $result.Advisory) { + Write-Output "::warning title=MSVC C++ toolchain ($($result.Reason))::$($result.Message)" exit 0 } From 158ea054a83caee8567d0d408412b2e59248a347 Mon Sep 17 00:00:00 2001 From: wallstop Date: Sat, 22 Aug 2026 05:56:11 +0000 Subject: [PATCH 3/9] Show the suite wall clock in the CI job summary A change added 78 seconds to the EditMode step on every editor leg and stayed green for two days. Nothing in CI reads how long a step takes, so nobody saw it. The suite already measures itself and already warns past a 60s soft budget, but only into the Unity log, which nobody opens on a green run. The CI harness now lifts that one line into the job summary and turns the soft breach into a workflow warning. No new script and no new workflow, and no comparison against history. That keeps the cheapest option in issue #410 and leaves out the one the issue calls most at odds with the tooling philosophy. The suite formats its number with the invariant culture so the harness parse does not depend on the runner locale. A test pins that shared line shape from both sides. Advances #410. Co-Authored-By: Claude Opus 5 (1M context) --- .../Runtime/Core/SuiteWallClockBudgetTest.cs | 12 +- .../run-ci-tests-wall-clock-summary.test.js | 168 ++++++++++++++++++ ...n-ci-tests-wall-clock-summary.test.js.meta | 7 + scripts/unity/run-ci-tests.ps1 | 76 ++++++++ scripts/validate-js-loc-budget.js | 13 +- 5 files changed, 273 insertions(+), 3 deletions(-) create mode 100644 scripts/__tests__/run-ci-tests-wall-clock-summary.test.js create mode 100644 scripts/__tests__/run-ci-tests-wall-clock-summary.test.js.meta diff --git a/Tests/Runtime/Core/SuiteWallClockBudgetTest.cs b/Tests/Runtime/Core/SuiteWallClockBudgetTest.cs index 6f0e0133..8c7dbcba 100644 --- a/Tests/Runtime/Core/SuiteWallClockBudgetTest.cs +++ b/Tests/Runtime/Core/SuiteWallClockBudgetTest.cs @@ -165,9 +165,17 @@ public void EndSuiteTimer() // included because the hard budget is selected per version (the // 2021.x runner gets a wider ceiling); seeing both together makes // a near-budget run easy to triage. + // Invariant culture on purpose: CI lifts this line into the job summary + // (issue #410), so the decimal separator must not follow the runner's + // locale. `scripts/unity/run-ci-tests.ps1` parses exactly this shape. + System.Globalization.CultureInfo invariant = System + .Globalization + .CultureInfo + .InvariantCulture; UnityEngine.Debug.Log( - $"DxMessaging suite wall clock: {elapsed.TotalSeconds:0.00}s " - + $"(soft budget {SoftBudget.TotalSeconds:0.0}s, hard budget {HardBudget.TotalSeconds:0.0}s " + $"DxMessaging suite wall clock: {elapsed.TotalSeconds.ToString("0.00", invariant)}s " + + $"(soft budget {SoftBudget.TotalSeconds.ToString("0.0", invariant)}s, " + + $"hard budget {HardBudget.TotalSeconds.ToString("0.0", invariant)}s " + $"for Unity {UnityEngine.Application.unityVersion})." ); diff --git a/scripts/__tests__/run-ci-tests-wall-clock-summary.test.js b/scripts/__tests__/run-ci-tests-wall-clock-summary.test.js new file mode 100644 index 00000000..c8121bec --- /dev/null +++ b/scripts/__tests__/run-ci-tests-wall-clock-summary.test.js @@ -0,0 +1,168 @@ +"use strict"; + +// Issue #410: a step regression stayed green for two days because nothing in CI +// looks at how long a step takes. `SuiteWallClockBudgetTest` already logs the +// suite's elapsed time and its budgets; `Write-SuiteWallClockSummary` lifts that +// one line into the job summary and warns when it is over the soft budget. +// +// These tests dot-source the function out of run-ci-tests.ps1 and drive it with +// fixture logs, so every branch runs without Unity. + +const { test } = require("node:test"); +const assert = require("node:assert/strict"); +const fs = require("node:fs"); +const os = require("node:os"); +const path = require("node:path"); +const { spawnSync } = require("node:child_process"); + +const RUN_CI_SCRIPT_PATH = path.join(__dirname, "..", "unity", "run-ci-tests.ps1"); + +function commandExists(command) { + // prettier-ignore + const result = spawnSync(command, ["-NoLogo", "-NoProfile", "-Command", "$PSVersionTable.PSVersion"], + { encoding: "utf8", stdio: ["ignore", "pipe", "pipe"] }); + return !result.error && result.status === 0; +} + +const HAS_PWSH = commandExists("pwsh"); + +// The function is defined inside a script that runs a whole Unity leg when +// executed, so the body is extracted by name rather than dot-sourced. +function extractFunction(source, name) { + const start = source.indexOf(`function ${name} {`); + assert.notEqual(start, -1, `${name} must exist in run-ci-tests.ps1`); + const end = source.indexOf("\n}\n", start); + assert.notEqual(end, -1, `${name} must be a complete function`); + return source.slice(start, end + 3); +} + +function runSummary({ logText, label = "editmode" }) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "dxm-wallclock-")); + try { + const logPath = path.join(root, "unity.log"); + fs.writeFileSync(logPath, logText, "utf8"); + const summaryPath = path.join(root, "summary.md"); + const source = fs.readFileSync(RUN_CI_SCRIPT_PATH, "utf8"); + const scriptPath = path.join(root, "probe.ps1"); + fs.writeFileSync( + scriptPath, + [ + "Set-StrictMode -Version Latest", + "$ErrorActionPreference = 'Stop'", + "$script:WroteWallClockSummaryHeader = $false", + extractFunction(source, "Write-SuiteWallClockSummary"), + `Write-SuiteWallClockSummary -LogPath '${logPath}' -Label '${label}'`, + `Write-SuiteWallClockSummary -LogPath '${logPath}' -Label '${label}-second'` + ].join("\n"), + "utf8" + ); + + const result = spawnSync("pwsh", ["-NoLogo", "-NoProfile", "-File", scriptPath], { + encoding: "utf8", + env: { ...process.env, GITHUB_STEP_SUMMARY: summaryPath } + }); + const summary = fs.existsSync(summaryPath) ? fs.readFileSync(summaryPath, "utf8") : ""; + return { + stdout: result.stdout ?? "", + stderr: result.stderr ?? "", + status: result.status, + summary + }; + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } +} + +const UNDER_BUDGET = + "DxMessaging suite wall clock: 36.30s (soft budget 60.0s, hard budget 180.0s for Unity 6000.4.6f1).\n"; +const OVER_BUDGET = + "DxMessaging suite wall clock: 113.80s (soft budget 60.0s, hard budget 180.0s for Unity 6000.4.6f1).\n"; + +test("a run under its soft budget is reported without a warning", { skip: !HAS_PWSH }, () => { + const run = runSummary({ logText: `noise\n${UNDER_BUDGET}more noise\n` }); + + assert.equal(run.status, 0, run.stderr); + assert.match(run.summary, /\| editmode \| 36\.30s \| 60\.0s \| 180\.0s \|/); + assert.doesNotMatch(run.stdout, /::warning::/); +}); + +test("a run over its soft budget warns and names both numbers", { skip: !HAS_PWSH }, () => { + const run = runSummary({ logText: OVER_BUDGET }); + + assert.equal(run.status, 0, run.stderr); + assert.match( + run.stdout, + /::warning::editmode suite wall clock 113\.80s is over its 60\.0s soft budget/ + ); + assert.match(run.summary, /\| editmode \| 113\.80s \| 60\.0s \| 180\.0s \|/); +}); + +test( + "the summary table header is written once per job, not once per leg", + { skip: !HAS_PWSH }, + () => { + const run = runSummary({ logText: UNDER_BUDGET }); + + const headers = run.summary.match(/### Suite wall clock/g) ?? []; + assert.equal(headers.length, 1, run.summary); + assert.match(run.summary, /\| editmode-second \|/); + } +); + +test("a log with no wall-clock line reports nothing", { skip: !HAS_PWSH }, () => { + const run = runSummary({ logText: "Unity started\nUnity finished\n" }); + + assert.equal(run.status, 0, run.stderr); + assert.equal(run.summary, ""); + assert.doesNotMatch(run.stdout, /::warning::/); +}); + +test("a missing log file reports nothing instead of failing the leg", { skip: !HAS_PWSH }, () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "dxm-wallclock-")); + try { + const source = fs.readFileSync(RUN_CI_SCRIPT_PATH, "utf8"); + const scriptPath = path.join(root, "probe.ps1"); + fs.writeFileSync( + scriptPath, + [ + "Set-StrictMode -Version Latest", + "$ErrorActionPreference = 'Stop'", + "$script:WroteWallClockSummaryHeader = $false", + extractFunction(source, "Write-SuiteWallClockSummary"), + `Write-SuiteWallClockSummary -LogPath '${path.join(root, "absent.log")}' -Label 'playmode'` + ].join("\n"), + "utf8" + ); + const result = spawnSync("pwsh", ["-NoLogo", "-NoProfile", "-File", scriptPath], { + encoding: "utf8" + }); + assert.equal(result.status, 0, result.stderr); + assert.doesNotMatch(result.stdout ?? "", /::warning::/); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } +}); + +test("the wall-clock line the harness parses is the one the suite emits", () => { + // The producing side is C#, the consuming side PowerShell. Neither compiles + // the other, so the shared shape is pinned here. + const budgetTest = fs.readFileSync( + path.join(__dirname, "..", "..", "Tests", "Runtime", "Core", "SuiteWallClockBudgetTest.cs"), + "utf8" + ); + assert.match( + budgetTest, + /DxMessaging suite wall clock: \{elapsed\.TotalSeconds\.ToString\("0\.00", invariant\)\}s/ + ); + assert.match( + budgetTest, + /soft budget \{SoftBudget\.TotalSeconds\.ToString\("0\.0", invariant\)\}s/ + ); + assert.match( + budgetTest, + /hard budget \{HardBudget\.TotalSeconds\.ToString\("0\.0", invariant\)\}s/ + ); + + const harness = fs.readFileSync(RUN_CI_SCRIPT_PATH, "utf8"); + assert.match(harness, /DxMessaging suite wall clock:\\s\*\(\[0-9\.\]\+\)s/); +}); diff --git a/scripts/__tests__/run-ci-tests-wall-clock-summary.test.js.meta b/scripts/__tests__/run-ci-tests-wall-clock-summary.test.js.meta new file mode 100644 index 00000000..29d33c17 --- /dev/null +++ b/scripts/__tests__/run-ci-tests-wall-clock-summary.test.js.meta @@ -0,0 +1,7 @@ +fileFormatVersion: 2 +guid: 353475020b1cb4d8693a71e665d6ebda +DefaultImporter: + externalObjects: {} + userData: + assetBundleName: + assetBundleVariant: diff --git a/scripts/unity/run-ci-tests.ps1 b/scripts/unity/run-ci-tests.ps1 index f452d41c..ea2fb3a8 100644 --- a/scripts/unity/run-ci-tests.ps1 +++ b/scripts/unity/run-ci-tests.ps1 @@ -438,6 +438,9 @@ function ConvertTo-SingleLineDiagnostic { # value is used for the opening and closing fence lines. The matching fence in # .github/actions/verify-unity-results/action.yml uses the same scheme. $script:WorkflowCommandStopToken = $null +# StrictMode Latest throws on reading an uninitialized variable, so the +# once-per-job summary header flag is declared here rather than at first use. +$script:WroteWallClockSummaryHeader = $false # Generate a fresh, unpredictable stop-commands fence token. A GUID 'N' form is # 32 hex chars with no separators, so it can never collide with caller text and @@ -498,6 +501,78 @@ function Get-NUnitNodeFullName { # cap). Attribute reads use XmlElement.GetAttribute (returns '' when absent, # never throws) so a results.xml lacking a fullname/name attribute does NOT # degrade the whole enumeration to a generic warning under Set-StrictMode. +function Write-SuiteWallClockSummary { + <# + .SYNOPSIS + Lift the suite's own wall-clock line out of the Unity log and into the job summary. + + .DESCRIPTION + Issue #410: a change added 78 seconds to the EditMode step on every editor + leg and stayed green for two days, because nothing in CI looks at how long + a step takes. `SuiteWallClockBudgetTest` already measures the suite and + already warns past its soft budget, but only into the Unity log, which + nobody reads on a green run. + + Printing the number it already has costs one regex per leg and needs no + new script and no new workflow, which is what issue #410 asks for. It does + not compare against history: that is the option the issue calls most at + odds with the repository's tooling philosophy, and it is not taken here. + #> + [CmdletBinding()] + param( + [string]$LogPath, + [Parameter(Mandatory = $true)][string]$Label + ) + + if (-not $LogPath -or -not (Test-Path -LiteralPath $LogPath -PathType Leaf)) { + return + } + + try { + # The producing side formats with the invariant culture, so the decimal + # separator here is always '.'. + $pattern = 'DxMessaging suite wall clock:\s*([0-9.]+)s\s*\(soft budget\s*([0-9.]+)s,\s*hard budget\s*([0-9.]+)s' + $match = @( + Select-String -LiteralPath $LogPath -Pattern $pattern -ErrorAction SilentlyContinue | + Select-Object -Last 1 + ) + if ($match.Count -lt 1) { + return + } + + $groups = $match[0].Matches[0].Groups + $invariant = [System.Globalization.CultureInfo]::InvariantCulture + $elapsed = 0.0 + $soft = 0.0 + if (-not [double]::TryParse($groups[1].Value, 'Float', $invariant, [ref]$elapsed)) { return } + if (-not [double]::TryParse($groups[2].Value, 'Float', $invariant, [ref]$soft)) { return } + + $summaryPath = $env:GITHUB_STEP_SUMMARY + if ($summaryPath) { + if (-not $script:WroteWallClockSummaryHeader) { + Add-Content -LiteralPath $summaryPath -Value @( + '### Suite wall clock', + '', + '| Leg | Elapsed | Soft budget | Hard budget |', + '| --- | ---: | ---: | ---: |' + ) + $script:WroteWallClockSummaryHeader = $true + } + Add-Content -LiteralPath $summaryPath -Value ("| $Label | $($groups[1].Value)s | " + + "$($groups[2].Value)s | $($groups[3].Value)s |") + } + + if ($elapsed -gt $soft) { + Write-Host ("::warning::${Label} suite wall clock $($groups[1].Value)s is over its " + + "$($groups[2].Value)s soft budget (hard budget $($groups[3].Value)s). A step that " + + "grew without breaching its ceiling is what issue #410 was raised for.") + } + } catch { + # Best-effort reporting must never mask a real result. + Write-Host "::warning::Could not read the suite wall clock for ${Label}: $($_.Exception.Message)" + } +} + function Write-UnityFailedTestAnnotations { [CmdletBinding()] param( @@ -3304,6 +3379,7 @@ function Test-NUnitResults { $skipped = [int]$run.skipped Write-Host "Results: total=$total passed=$passed failed=$failed skipped=$skipped" + Write-SuiteWallClockSummary -LogPath $LogPath -Label $Label if ($total -lt 1) { Write-CiError "0 tests ran for $Label -- check assembly selection and package testables.$exitNote" throw "0 tests ran for $Label." diff --git a/scripts/validate-js-loc-budget.js b/scripts/validate-js-loc-budget.js index 466a9ee5..111fd30c 100644 --- a/scripts/validate-js-loc-budget.js +++ b/scripts/validate-js-loc-budget.js @@ -120,7 +120,18 @@ const path = require("path"); // false red. +29 source and +59 test lines, the tests covering a live // editor, a stale discovery record, a reply carrying no editor state, an // empty reply, and both no-call paths: 18181. -const TOTAL_BUDGET = 18181; +// 081 Make the suite wall clock visible in the job summary (#410). A change +// added 78 seconds to the EditMode step on every editor leg and stayed +// green for two days, because nothing in CI reads how long a step takes. +// `SuiteWallClockBudgetTest` already measures the suite and already warns +// past its soft budget, into a log nobody opens on a green run. The CI +// harness now lifts that one line into the job summary and warns when it is +// over. +168 test lines covering under and over budget, the once-per-job +// table header, a log with no line, a missing log, and the shared line +// shape between the C# producer and the PowerShell consumer. No new script +// and no new workflow, which is the option issue #410 asks for, plus the +// 11 lines this entry itself adds: 18360. +const TOTAL_BUDGET = 18360; const LARGEST_FILE_COUNT = 10; const REPO_ROOT = path.resolve(__dirname, ".."); From 93ebc004ff820f3e04f6d2dc77106fbb045f464c Mon Sep 17 00:00:00 2001 From: wallstop Date: Sat, 22 Aug 2026 06:02:44 +0000 Subject: [PATCH 4/9] Use a CultureInfo import for the wall-clock format Co-Authored-By: Claude Opus 5 (1M context) --- Tests/Runtime/Core/SuiteWallClockBudgetTest.cs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/Tests/Runtime/Core/SuiteWallClockBudgetTest.cs b/Tests/Runtime/Core/SuiteWallClockBudgetTest.cs index 8c7dbcba..a894f83e 100644 --- a/Tests/Runtime/Core/SuiteWallClockBudgetTest.cs +++ b/Tests/Runtime/Core/SuiteWallClockBudgetTest.cs @@ -5,6 +5,7 @@ namespace DxMessaging.Tests.Runtime { using System; using System.Diagnostics; + using System.Globalization; using NUnit.Framework; using NUnit.Framework.Interfaces; @@ -168,10 +169,7 @@ public void EndSuiteTimer() // Invariant culture on purpose: CI lifts this line into the job summary // (issue #410), so the decimal separator must not follow the runner's // locale. `scripts/unity/run-ci-tests.ps1` parses exactly this shape. - System.Globalization.CultureInfo invariant = System - .Globalization - .CultureInfo - .InvariantCulture; + CultureInfo invariant = CultureInfo.InvariantCulture; UnityEngine.Debug.Log( $"DxMessaging suite wall clock: {elapsed.TotalSeconds.ToString("0.00", invariant)}s " + $"(soft budget {SoftBudget.TotalSeconds.ToString("0.0", invariant)}s, " From 98728f50c97e53e4256bec7513fdc679a1b4f0e4 Mon Sep 17 00:00:00 2001 From: wallstop Date: Sat, 22 Aug 2026 06:18:10 +0000 Subject: [PATCH 5/9] Harden the wrap helper, the launch probe, and the summary header Three findings from reviewing this branch: The wrap helper measured absolutely positioned children. Those are out of flow and create no wrapped line, so one would have dragged the container's height with it. They are skipped now. The MSVC launch probe read its outcome outside its own try block. This verdict is advisory, so an unexpected throw there would have failed the leg the gate exists to protect. Everything is inside the try now. The CI job summary header used an in-process flag, but the workflow runs the harness once per test mode, so each leg is its own process and the header printed three times per job. It is looked up in the summary file now, and the test spawns one process per leg to prove it. The wrap drift guard also scans the stylesheets. A stylesheet can turn wrapping on just as well as a source file can. Co-Authored-By: Claude Opus 5 (1M context) --- Editor/DxMessagingEditorTheme.cs | 7 +++++ Tests/Editor/DxMessagingEditorThemeTests.cs | 28 ++++++++++++++++--- .../run-ci-tests-wall-clock-summary.test.js | 26 +++++++++-------- scripts/unity/assert-msvc-toolchain.ps1 | 11 ++++---- scripts/unity/run-ci-tests.ps1 | 16 +++++++---- scripts/validate-js-loc-budget.js | 7 +++-- 6 files changed, 67 insertions(+), 28 deletions(-) diff --git a/Editor/DxMessagingEditorTheme.cs b/Editor/DxMessagingEditorTheme.cs index 47886c53..44d55a2e 100644 --- a/Editor/DxMessagingEditorTheme.cs +++ b/Editor/DxMessagingEditorTheme.cs @@ -470,6 +470,13 @@ internal static float MeasureWrappedContentHeight(VisualElement container) continue; } + // An absolutely positioned child is out of flow, so it creates no wrapped line + // and must not drag the container's height with it. + if (child.resolvedStyle.position == Position.Absolute) + { + continue; + } + Rect childLayout = child.layout; if (float.IsNaN(childLayout.yMax)) { diff --git a/Tests/Editor/DxMessagingEditorThemeTests.cs b/Tests/Editor/DxMessagingEditorThemeTests.cs index 3b3115cf..36b8e748 100644 --- a/Tests/Editor/DxMessagingEditorThemeTests.cs +++ b/Tests/Editor/DxMessagingEditorThemeTests.cs @@ -814,9 +814,9 @@ public void AGrownWrappingContainerForgetsItsHeightWhenItLeavesThePanel() } /// - /// Drift guard for the whole class of defect in issue #440: turning wrapping on by hand - /// re-creates it, because nothing then supplies the height Unity 2021.3 does not derive. - /// Every shipped editor source must go through the helper instead. + /// Drift guard for the whole class of defect in issue #440: turning wrapping on by hand, + /// in a source file or in a stylesheet, re-creates it, because nothing then supplies the + /// height Unity 2021.3 does not derive. Everything shipped must go through the helper. /// [Test] public void EveryEditorWrapContainerGoesThroughTheContentSizedWrapHelper() @@ -855,10 +855,30 @@ public void EveryEditorWrapContainerGoesThroughTheContentSizedWrapHelper() } } + // A stylesheet can turn wrapping on just as well as a source file can, and + // nothing would then supply the height either. + foreach ( + string ussPath in new[] + { + DxMessagingEditorTheme.TokensUssPath, + DxMessagingEditorTheme.ThemeUssPath, + } + ) + { + string[] ussLines = System.IO.File.ReadAllLines(ussPath); + for (int index = 0; index < ussLines.Length; index++) + { + if (ussLines[index].Contains("flex-" + "wrap:")) + { + offenders.Add($"{ussPath}:{index + 1}"); + } + } + } + Assert.That( offenders, Is.Empty, - "These editor sources turn wrapping on directly instead of calling " + "These editor sources or stylesheets turn wrapping on directly instead of calling " + "DxMessagingEditorTheme.ApplyContentSizedWrap, so on Unity 2021.3 their " + "wrapped lines draw outside the container (GitHub #440):\n" + string.Join("\n", offenders) diff --git a/scripts/__tests__/run-ci-tests-wall-clock-summary.test.js b/scripts/__tests__/run-ci-tests-wall-clock-summary.test.js index c8121bec..e70d965b 100644 --- a/scripts/__tests__/run-ci-tests-wall-clock-summary.test.js +++ b/scripts/__tests__/run-ci-tests-wall-clock-summary.test.js @@ -47,25 +47,30 @@ function runSummary({ logText, label = "editmode" }) { fs.writeFileSync( scriptPath, [ + "param([string]$Label)", "Set-StrictMode -Version Latest", "$ErrorActionPreference = 'Stop'", - "$script:WroteWallClockSummaryHeader = $false", extractFunction(source, "Write-SuiteWallClockSummary"), - `Write-SuiteWallClockSummary -LogPath '${logPath}' -Label '${label}'`, - `Write-SuiteWallClockSummary -LogPath '${logPath}' -Label '${label}-second'` + `Write-SuiteWallClockSummary -LogPath '${logPath}' -Label $Label` ].join("\n"), "utf8" ); - const result = spawnSync("pwsh", ["-NoLogo", "-NoProfile", "-File", scriptPath], { - encoding: "utf8", - env: { ...process.env, GITHUB_STEP_SUMMARY: summaryPath } - }); + // The workflow runs this script once per test mode, so each leg is its own + // process. Driving it the same way is what proves the header is written once + // per job rather than once per leg. + const legs = [label, `${label}-second`]; + const runs = legs.map((leg) => + spawnSync("pwsh", ["-NoLogo", "-NoProfile", "-File", scriptPath, "-Label", leg], { + encoding: "utf8", + env: { ...process.env, GITHUB_STEP_SUMMARY: summaryPath } + }) + ); const summary = fs.existsSync(summaryPath) ? fs.readFileSync(summaryPath, "utf8") : ""; return { - stdout: result.stdout ?? "", - stderr: result.stderr ?? "", - status: result.status, + stdout: runs.map((r) => r.stdout ?? "").join(""), + stderr: runs.map((r) => r.stderr ?? "").join(""), + status: runs.find((r) => r.status !== 0)?.status ?? 0, summary }; } finally { @@ -127,7 +132,6 @@ test("a missing log file reports nothing instead of failing the leg", { skip: !H [ "Set-StrictMode -Version Latest", "$ErrorActionPreference = 'Stop'", - "$script:WroteWallClockSummaryHeader = $false", extractFunction(source, "Write-SuiteWallClockSummary"), `Write-SuiteWallClockSummary -LogPath '${path.join(root, "absent.log")}' -Label 'playmode'` ].join("\n"), diff --git a/scripts/unity/assert-msvc-toolchain.ps1 b/scripts/unity/assert-msvc-toolchain.ps1 index b3b6a479..d1f7fe83 100644 --- a/scripts/unity/assert-msvc-toolchain.ps1 +++ b/scripts/unity/assert-msvc-toolchain.ps1 @@ -308,18 +308,19 @@ $result = Test-MsvcToolchain -InstallRoot $installRoot -RunnerName $runner -Prob Test-Path -LiteralPath $path -PathType Leaf } -ProbeLaunch { param($path) - $threw = $false - $code = $null + # EVERYTHING is inside the try, including reading the outcome. This verdict is + # advisory, so an unexpected throw here must become "did not start" and a + # warning, never a terminating error that fails the leg this gate exists to + # protect. try { # By full path, with no input files, output discarded: the question is # whether the image loads, not what it says. $null = & $path '/?' 2>&1 - $code = $LASTEXITCODE + return (Test-CompilerLaunchOutcome -ExitCode $LASTEXITCODE -Threw $false) } catch { - $threw = $true + return (Test-CompilerLaunchOutcome -ExitCode $null -Threw $true) } - Test-CompilerLaunchOutcome -ExitCode $code -Threw $threw } if ($result.Ok) { diff --git a/scripts/unity/run-ci-tests.ps1 b/scripts/unity/run-ci-tests.ps1 index ea2fb3a8..3496adb8 100644 --- a/scripts/unity/run-ci-tests.ps1 +++ b/scripts/unity/run-ci-tests.ps1 @@ -438,9 +438,6 @@ function ConvertTo-SingleLineDiagnostic { # value is used for the opening and closing fence lines. The matching fence in # .github/actions/verify-unity-results/action.yml uses the same scheme. $script:WorkflowCommandStopToken = $null -# StrictMode Latest throws on reading an uninitialized variable, so the -# once-per-job summary header flag is declared here rather than at first use. -$script:WroteWallClockSummaryHeader = $false # Generate a fresh, unpredictable stop-commands fence token. A GUID 'N' form is # 32 hex chars with no separators, so it can never collide with caller text and @@ -549,14 +546,21 @@ function Write-SuiteWallClockSummary { $summaryPath = $env:GITHUB_STEP_SUMMARY if ($summaryPath) { - if (-not $script:WroteWallClockSummaryHeader) { + # The header is looked up in the FILE, not held in a variable: the + # workflow runs this script once per test mode, so each leg is its own + # pwsh process and an in-process flag would print the header three + # times per job. + $header = '### Suite wall clock' + $alreadyOpen = (Test-Path -LiteralPath $summaryPath -PathType Leaf) -and + @(Select-String -LiteralPath $summaryPath -Pattern ([regex]::Escape($header)) ` + -SimpleMatch:$false -ErrorAction SilentlyContinue).Count -gt 0 + if (-not $alreadyOpen) { Add-Content -LiteralPath $summaryPath -Value @( - '### Suite wall clock', + $header, '', '| Leg | Elapsed | Soft budget | Hard budget |', '| --- | ---: | ---: | ---: |' ) - $script:WroteWallClockSummaryHeader = $true } Add-Content -LiteralPath $summaryPath -Value ("| $Label | $($groups[1].Value)s | " + "$($groups[2].Value)s | $($groups[3].Value)s |") diff --git a/scripts/validate-js-loc-budget.js b/scripts/validate-js-loc-budget.js index 111fd30c..99234182 100644 --- a/scripts/validate-js-loc-budget.js +++ b/scripts/validate-js-loc-budget.js @@ -130,8 +130,11 @@ const path = require("path"); // table header, a log with no line, a missing log, and the shared line // shape between the C# producer and the PowerShell consumer. No new script // and no new workflow, which is the option issue #410 asks for, plus the -// 11 lines this entry itself adds: 18360. -const TOTAL_BUDGET = 18360; +// 11 lines this entry itself adds. Review then moved the once-per-job header +// check into the summary FILE, because the workflow runs the harness once +// per test mode, so an in-process flag printed the header three times per +// job; the test now spawns one process per leg: 18367. +const TOTAL_BUDGET = 18367; const LARGEST_FILE_COUNT = 10; const REPO_ROOT = path.resolve(__dirname, ".."); From 3636639b54af36c54f38ebf8aeff29bb6d84c53b Mon Sep 17 00:00:00 2001 From: wallstop Date: Sat, 22 Aug 2026 06:21:20 +0000 Subject: [PATCH 6/9] Say "no type suffix" instead of a word the dictionary rejects Co-Authored-By: Claude Opus 5 (1M context) --- .llm/skills/shell-and-powershell/SKILL.md | 2 +- scripts/unity/assert-msvc-toolchain.ps1 | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.llm/skills/shell-and-powershell/SKILL.md b/.llm/skills/shell-and-powershell/SKILL.md index dcff5b4b..07ae5af6 100644 --- a/.llm/skills/shell-and-powershell/SKILL.md +++ b/.llm/skills/shell-and-powershell/SKILL.md @@ -99,7 +99,7 @@ application for this OS platform`, or a case-mismatch file-not-found. `os.tmpdir()` on `C:`), so `rel.startsWith("..")` mislabels an outside path as inside. Use `isPathInsideDirectory` / `isPathOutsideDirectory` / `isOutsideRelative` from `scripts/lib/path-classifier.js`; if you truly cannot, pair with `path.isAbsolute(rel)`. -- PowerShell parses an unsuffixed hex literal as a SIGNED integer, so `0xC0000000` is +- PowerShell parses a hex literal with no type suffix as a SIGNED integer, so `0xC0000000` is `-1073741824`, not `3221225472`. Comparing a Windows exit code against a bare NTSTATUS literal therefore matches everything. Suffix it (`0xC0000000L`) or write the decimal. See `Test-CompilerLaunchOutcome` in `scripts/unity/assert-msvc-toolchain.ps1`. diff --git a/scripts/unity/assert-msvc-toolchain.ps1 b/scripts/unity/assert-msvc-toolchain.ps1 index d1f7fe83..a775f9a6 100644 --- a/scripts/unity/assert-msvc-toolchain.ps1 +++ b/scripts/unity/assert-msvc-toolchain.ps1 @@ -208,7 +208,7 @@ function Test-CompilerLaunchOutcome { if ($Threw) { return $false } if ($null -eq $ExitCode) { return $false } - # `0xC0000000L`, with the L. PowerShell parses an unsuffixed hex literal as a + # `0xC0000000L`, with the L. PowerShell parses a hex literal with no type suffix as a # SIGNED int, so a bare `0xC0000000` is -1073741824 and `0 -ge -1073741824` # would call every healthy exit code a loader failure. $code = [int64]$ExitCode From 38f7712ae41e958bfb9bbef8afdd4049272982b4 Mon Sep 17 00:00:00 2001 From: wallstop Date: Sat, 22 Aug 2026 06:24:16 +0000 Subject: [PATCH 7/9] Give each lane scroll view its own wrap row name The lane panel is built twice, once for message-type lanes and once for context lanes, so a shared constant put the same element name in the tree twice. The name is derived from the scroll view's own name now. Co-Authored-By: Claude Opus 5 (1M context) --- Editor/Windows/DxMessagingMessageMonitorWindow.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Editor/Windows/DxMessagingMessageMonitorWindow.cs b/Editor/Windows/DxMessagingMessageMonitorWindow.cs index 9ed689fe..bd826d45 100644 --- a/Editor/Windows/DxMessagingMessageMonitorWindow.cs +++ b/Editor/Windows/DxMessagingMessageMonitorWindow.cs @@ -34,7 +34,6 @@ public sealed class DxMessagingMessageMonitorWindow : EditorWindow "dxmessaging-monitor-active-filter-token-row"; internal const string ActiveFilterTokenClassName = "dxmessaging-monitor-active-filter-token"; - internal const string LanePillWrapRowName = "dxmessaging-monitor-lane-pill-row"; internal const string ActiveFilterClearButtonName = "dxmessaging-monitor-active-filter-clear"; internal const string RefreshButtonName = "dxmessaging-monitor-refresh"; @@ -1427,7 +1426,10 @@ IEnumerable pills laneRows.style.flexShrink = 1; laneRows.style.minHeight = 0; laneRows.style.marginTop = 4; - VisualElement laneRowsContent = CreateScrollableWrapRow(LanePillWrapRowName); + // Derived from the scroll view's own name: this panel is built twice, once for + // message-type lanes and once for context lanes, so a shared constant would put the + // same element name in the tree twice. + VisualElement laneRowsContent = CreateScrollableWrapRow(scrollViewName + "-row"); laneRows.Add(laneRowsContent); lanesRoot.Add(laneRows); From b294240169017c9bac10a09077c3f27f6a80a2d0 Mon Sep 17 00:00:00 2001 From: wallstop Date: Sat, 22 Aug 2026 06:25:10 +0000 Subject: [PATCH 8/9] Say the PR length ceiling is per block, not per page Found by applying the skill to this branch's own pull request, which closes several issues and so runs past 400 words in total. Co-Authored-By: Claude Opus 5 (1M context) --- .llm/index.json | 2 +- .llm/skills/pull-request-writing/SKILL.md | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/.llm/index.json b/.llm/index.json index 310bfeba..cef4c252 100644 --- a/.llm/index.json +++ b/.llm/index.json @@ -301,7 +301,7 @@ "category": "process", "tags": "pull-request, writing, simplified-technical-english, ste, review, communication" }, - "lineCount": 96, + "lineCount": 97, "references": [ ".llm/skills/pull-request-writing/references/pull-request-examples.md" ] diff --git a/.llm/skills/pull-request-writing/SKILL.md b/.llm/skills/pull-request-writing/SKILL.md index a6fd5046..49bacef8 100644 --- a/.llm/skills/pull-request-writing/SKILL.md +++ b/.llm/skills/pull-request-writing/SKILL.md @@ -48,9 +48,10 @@ change leaves something open that a reader must know about. ### Length -- Aim for 200 words. Stop at 400. -- One pull request that closes several issues gets one `Why / What changed / How we know` block - per issue, each still short. It does not get a longer preamble. +- Aim for 200 words per block. Stop at 400. +- A pull request closing several issues has one `Why / What changed / How we know` block per + issue, so it is longer overall. The ceiling is per block, not per page. It still does not get a + preamble. - Move deep evidence to the issue or to a `progress/` record and link it. The body carries the verdict, not the working. From 11c825165868378445b008a3e5bf166fa1a8211e Mon Sep 17 00:00:00 2001 From: wallstop Date: Sat, 22 Aug 2026 07:11:20 +0000 Subject: [PATCH 9/9] Stop the wrap helper writing an unbounded height, and let it hear its children CI on Unity 2021.3, 2022.3 and 6000.3 found two real defects that the host editor does not show. A child laid out with no bound reports Unity's largest length, 8388608. The helper treated that as a measurement and asked for a box eight million pixels tall, which is where the 8388608 and 41943040 numbers in those legs came from. Such a measurement is now ignored. The helper also could not hear its own content grow. Unity reports a geometry change to an element only when that element's own box changes, and never reports a child's change to the parent, in either propagation phase. So a container already held at a height never learned that its text had finished measuring and needed more room. That left a Flow Graph details header ten pixels short on 6000.3. Each child is now watched directly. Both are covered by tests that fail without the fix. Test-side, the layout settle now runs until the tree stops changing instead of a fixed two passes, and the unset-inline-style assertion accepts both keywords Unity uses for it: Null on 6000.4, Undefined on 2021.3 and 2022.3. Co-Authored-By: Claude Opus 5 (1M context) --- Editor/DxMessagingEditorTheme.cs | 58 +++++++++++++++--- Tests/Editor/DxMessagingEditorThemeTests.cs | 68 ++++++++++++++++++++- Tests/Editor/EditorWindowTestUtility.cs | 68 +++++++++++++++++++-- 3 files changed, 179 insertions(+), 15 deletions(-) diff --git a/Editor/DxMessagingEditorTheme.cs b/Editor/DxMessagingEditorTheme.cs index 44d55a2e..bd8df672 100644 --- a/Editor/DxMessagingEditorTheme.cs +++ b/Editor/DxMessagingEditorTheme.cs @@ -2,6 +2,7 @@ namespace DxMessaging.Editor { using System; + using System.Collections.Generic; using UnityEditor; using UnityEngine; using UnityEngine.UIElements; @@ -420,7 +421,7 @@ internal static float ApplyResizedHeight( /// Unity 2021.3 does not grow a wrapping container to fit the extra lines. The container /// keeps its single-line height and the wrapped lines draw outside it, on top of whatever /// the window draws beneath. Newer editors size the container correctly, so the defect is - /// invisible on 6000.x. Issue #435 hit it in the Message Monitor toolbar and issue #440 + /// invisible on 6000.4. Issue #435 hit it in the Message Monitor toolbar and issue #440 /// lists the Flow Graph containers with the same shape. /// /// Measuring the children and applying the result as `min-height` supplies the height @@ -428,6 +429,12 @@ internal static float ApplyResizedHeight( /// actually too short, so an editor that already sizes the container writes no inline /// style at all. `align-content: flex-start` packs the lines at the top, so a container /// that just grew cannot stretch its own lines and ask to grow again. + /// + /// The children are watched as well as the container. Unity reports a geometry change to + /// an element only when that element's own box changes, and it does not report a child's + /// change to the parent in either propagation phase. A container already held at a height + /// therefore never hears that its text finished measuring and needs more room, which is + /// how a details header ended up ten pixels short on Unity 6000.3. /// internal static void ApplyContentSizedWrap(VisualElement container) { @@ -452,7 +459,10 @@ internal static void ApplyContentSizedWrap(VisualElement container) /// /// The height a wrapping container needs for every line its children occupy, measured in /// the container's own coordinate space so a panned or zoomed ancestor cannot skew it. - /// Returns 0 when nothing is measurable yet. + /// + /// Returns 0 when nothing is measurable yet, which includes a child laid out with an + /// unbounded size. Unity reports that as `Length`'s maximum, 8388608, and treating it as a + /// real height would ask for a box eight million pixels tall. /// internal static float MeasureWrappedContentHeight(VisualElement container) { @@ -483,6 +493,11 @@ internal static float MeasureWrappedContentHeight(VisualElement container) continue; } + if (childLayout.yMax >= UnboundedLayoutSize) + { + return 0f; + } + contentBottom = Mathf.Max( contentBottom, childLayout.yMax + child.resolvedStyle.marginBottom @@ -499,21 +514,25 @@ internal static float MeasureWrappedContentHeight(VisualElement container) return contentBottom + containerStyle.paddingBottom + containerStyle.borderBottomWidth; } + /// + /// Unity's largest representable length (`Length`'s own maximum). A layout that reaches it + /// was measured with no bound rather than measured, so it is not a height to apply. + /// + private const float UnboundedLayoutSize = 8388608f; + /// /// Holds the height one container was given, so a container that already fits is never /// written to and a container that was grown is only written again when it needs more. /// - /// The height is only ever raised while the container is in a panel. Unity reports a - /// geometry change to the container when the container's own box changes, and a container - /// held at a height by this class does not change its box when its content shrinks, so - /// there is no second measurement to lower it from. Leaving the panel releases the height - /// instead, which is what a reused or rebuilt container needs. + /// The height is only ever raised while the container is in a panel. Leaving the panel + /// releases it, which is what a reused or rebuilt container needs. /// private sealed class WrapHeightFit { private const float Tolerance = 0.5f; private readonly VisualElement _container; + private readonly HashSet _watchedChildren = new(); private bool _applied; private float _appliedHeight; @@ -525,8 +544,10 @@ internal WrapHeightFit(VisualElement container) internal void Fit() { + WatchChildren(); + float required = MeasureWrappedContentHeight(_container); - if (required <= 0f) + if (required <= 0f || required >= UnboundedLayoutSize) { return; } @@ -544,6 +565,7 @@ internal void Fit() internal void Release() { + _watchedChildren.Clear(); if (!_applied) { return; @@ -553,6 +575,26 @@ internal void Release() _appliedHeight = 0f; _container.style.minHeight = StyleKeyword.Null; } + + /// + /// A child that resizes in place changes what the container needs, and Unity does not + /// report a child's geometry change to its parent. Each child is watched directly. + /// + private void WatchChildren() + { + foreach (VisualElement child in _container.Children()) + { + if (!_watchedChildren.Add(child)) + { + continue; + } + + child.RegisterCallback( + static (_, state) => state.Fit(), + this + ); + } + } } private static StyleSheet LoadStyleSheet(string path) diff --git a/Tests/Editor/DxMessagingEditorThemeTests.cs b/Tests/Editor/DxMessagingEditorThemeTests.cs index 36b8e748..eee82030 100644 --- a/Tests/Editor/DxMessagingEditorThemeTests.cs +++ b/Tests/Editor/DxMessagingEditorThemeTests.cs @@ -36,6 +36,11 @@ public sealed class DxMessagingEditorThemeTests /// private const int SingleLineChildCount = 2; + /// + /// Unity's largest representable length, which an element laid out with no bound reports. + /// + private const float UnboundedLayoutSize = 8388608f; + /// /// One line of children, which is exactly what Unity 2021.3 leaves a wrapping container at. /// @@ -756,9 +761,11 @@ public void AWrappingContainerThatAlreadyFitsIsLeftWithoutAnInlineHeight() ), "The probe must already fit, otherwise this asserts the wrong case." ); + // Unity reports an unset inline style as `Null` on 6000.4 and as `Undefined` on + // 2021.3 and 2022.3. Both mean the same thing: no inline value was written. Assert.That( probe.style.minHeight.keyword, - Is.EqualTo(StyleKeyword.Null), + Is.EqualTo(StyleKeyword.Null).Or.EqualTo(StyleKeyword.Undefined), "A container that already fits its lines must keep the height its stylesheet " + "gives it, so the helper must not write an inline min-height." ); @@ -813,6 +820,65 @@ public void AGrownWrappingContainerForgetsItsHeightWhenItLeavesThePanel() ); } + /// + /// Unity reports a geometry change to an element only when that element's own box changes, + /// and never reports a child's change to the parent. So a container already held at a + /// height hears nothing when its content grows in place, which is how a Flow Graph details + /// header ended up ten pixels short on Unity 6000.3 with the fix applied. + /// + [Test] + public void AWrappingContainerGrowsAgainWhenAChildGrowsInPlace() + { + EditorWindow window = CreateWrapProbeWindow(); + + VisualElement probe = AddWrapProbe(window, PinnedHeight, contentSized: true); + EditorWindowTestUtility.SettleLayout(window); + float firstHeight = probe.resolvedStyle.height; + + probe[probe.childCount - 1].style.height = ProbeChildHeight * 3f; + EditorWindowTestUtility.SettleLayout(window); + + Assert.That( + probe.resolvedStyle.height, + Is.GreaterThan(firstHeight + EditorWindowTestUtility.LayoutTolerance), + "The container must grow again when a child grows, not stay at the height its " + + "first content needed." + ); + Assert.That( + DxMessagingEditorTheme.MeasureWrappedContentHeight(probe), + Is.LessThanOrEqualTo( + probe.resolvedStyle.height + EditorWindowTestUtility.LayoutTolerance + ), + "The grown child must end up inside the container." + ); + } + + /// + /// A child laid out with no bound reports Unity's largest length instead of a measurement. + /// Treating it as a height asks for a box eight million pixels tall, which is what the + /// Unity 2021.3, 2022.3 and 6000.3 legs reported before this guard existed. + /// + [Test] + public void AnUnboundedChildIsNotMistakenForAHeightToApply() + { + EditorWindow window = CreateWrapProbeWindow(); + + VisualElement probe = AddWrapProbe(window, pinnedHeight: 0f, contentSized: true); + probe[0].style.height = UnboundedLayoutSize; + EditorWindowTestUtility.SettleLayout(window); + + Assert.That( + DxMessagingEditorTheme.MeasureWrappedContentHeight(probe), + Is.EqualTo(0f), + "An unbounded child is not a measurement, so there is nothing to report." + ); + Assert.That( + probe.style.minHeight.keyword, + Is.EqualTo(StyleKeyword.Null).Or.EqualTo(StyleKeyword.Undefined), + "The helper must not write an eight-million-pixel min-height." + ); + } + /// /// Drift guard for the whole class of defect in issue #440: turning wrapping on by hand, /// in a source file or in a stylesheet, re-creates it, because nothing then supplies the diff --git a/Tests/Editor/EditorWindowTestUtility.cs b/Tests/Editor/EditorWindowTestUtility.cs index bc013699..372add13 100644 --- a/Tests/Editor/EditorWindowTestUtility.cs +++ b/Tests/Editor/EditorWindowTestUtility.cs @@ -3,6 +3,7 @@ namespace DxMessaging.Tests.Editor { using System; using System.Collections.Generic; + using System.Text; using DxMessaging.Editor; using NUnit.Framework; using UnityEditor; @@ -25,6 +26,18 @@ internal static class EditorWindowTestUtility /// internal const float LayoutTolerance = 0.5f; + /// + /// Unity's largest representable length. An element reporting it was laid out with no + /// bound rather than measured. + /// + private const float UnboundedLayoutSize = 8388608f; + + /// + /// How many layout passes will run before giving up on the + /// tree settling. Real windows settle in two or three. + /// + private const int MaxLayoutPasses = 8; + internal static DxMessagingTestHostWindow CreateWindow() { DxMessagingTestHostWindow window = @@ -222,11 +235,14 @@ internal static void IgnoreUnityInvalidGcHandleAsserts(Action action) } /// - /// Runs the panel's layout to a settled state, so a test reads the geometry a reader sees. - /// The first pass realizes content and can change what elements measure; the second - /// measures the result, including any height a - /// container asked for during - /// the first pass. + /// Runs the panel's layout until it stops changing, so a test reads the geometry a reader + /// settles on rather than an intermediate frame. + /// + /// One pass is never enough. Realizing content changes what elements measure, text height + /// is only final once its width is, and a + /// container asks for its height + /// during a pass and receives it in the next one. Unity settles this over frames; a test + /// has to ask for the frames. /// internal static void SettleLayout(EditorWindow window) { @@ -235,14 +251,43 @@ internal static void SettleLayout(EditorWindow window) return; } - for (int pass = 0; pass < 2; pass++) + string previous = null; + for (int pass = 0; pass < MaxLayoutPasses; pass++) { EditorSurfaceCapture.InvokeInheritedPanelMethod( window.rootVisualElement.panel, "ValidateLayout", Array.Empty() ); + + string current = DescribeLayout(window.rootVisualElement); + if (current == previous) + { + return; + } + + previous = current; + } + } + + private static string DescribeLayout(VisualElement root) + { + StringBuilder description = new(); + foreach (VisualElement element in root.Query().ToList()) + { + Rect layout = element.layout; + description + .Append(layout.x) + .Append(',') + .Append(layout.y) + .Append(',') + .Append(layout.width) + .Append(',') + .Append(layout.height) + .Append(';'); } + + return description.ToString(); } /// @@ -269,6 +314,17 @@ string context continue; } + // An element laid out with no bound reports Unity's largest length rather than a + // measurement. A box eight million pixels tall is not painting over anything a + // reader can see, and asserting on it compares two sentinels. + if ( + float.IsNaN(element.resolvedStyle.height) + || element.resolvedStyle.height >= UnboundedLayoutSize + ) + { + continue; + } + wrapping.Add(element); }