From 26b119354de004bb9858037c72ac2f6742e88b48 Mon Sep 17 00:00:00 2001 From: Erik Date: Tue, 11 Aug 2026 00:29:08 +0200 Subject: [PATCH] =?UTF-8?q?docs:=20OP2=20dual-lens=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20double=20REJECT=20(re-classing=20blast=20radius)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Blast lens: the unguarded Type-8 arm re-classes five pre-existing elements in four shipped panels (vendor backdrop sprite stops drawing; character sheet + spellbook swallow clicks; IUiDatStateful propagation severed), the Type-5 guard's premise is factually wrong (ten shipped elements author non-empty 0x64), and the 27 stale fixtures make the harness structurally blind to both — the green suite was true but meaningless. Merge semantics, fixture provenance, hermeticity, and the children-attached hook all verified clean. Mechanism lens: readers decode correctly through the canonical effective-state path; Type 8's real retail class is UIElement_Panel (Update @0x0046BD00 — switching behavior confirmed faithful); both claimed structural identities (UIOption_Slider = horizontal scrollbar, UIOption_Menu = UiMenu shape) CONFIRMED from fixture fingerprints; U10 closed (0x10000521 is the bitfield row template, consumed by CreateChildren @0x00485DF0 via AddItemFromTemplateList(0)); TS-72 is backwards — the toggle math is decomp-confirmed right, IsSet's all-bits predicate is confirmed wrong (retail Refresh checks ANY mask bit). Missing: GetChildRecursive resolution, no-0x32 means NO default switch, SetupTabPageHash's malformed-entry filters. Rework round 1 follows: UiTabControl/UiTemplateListBox become UiDatElement subclasses with dormant behavior (controller-activated), bitfield rows build from the authored template, IsSet goes any-bit, all 32 fixtures regenerate as the acceptance gate. Co-Authored-By: Claude Fable 5 --- docs/research/2026-08-11-op2-review-blast.md | 376 ++++++++++++ .../2026-08-11-op2-review-mechanism.md | 580 ++++++++++++++++++ 2 files changed, 956 insertions(+) create mode 100644 docs/research/2026-08-11-op2-review-blast.md create mode 100644 docs/research/2026-08-11-op2-review-mechanism.md diff --git a/docs/research/2026-08-11-op2-review-blast.md b/docs/research/2026-08-11-op2-review-blast.md new file mode 100644 index 00000000..253ced0a --- /dev/null +++ b/docs/research/2026-08-11-op2-review-blast.md @@ -0,0 +1,376 @@ +# OP2 review — regression / blast-radius lens + +**Commit under review:** `df9c7a35` "feat(ui): Campaign OP slice OP2 — tab control, +template ListBox, UIOption widget mappings" +**Reviewer lens:** what OP2 breaks OUTSIDE its own scope (Options panel). +**Date:** 2026-08-11 +**Verdict: REJECT** — two independent MUST-FIX regressions on shipped panels, and a +test harness structurally incapable of observing either. + +--- + +## 0. Executive summary + +OP2's own acceptance criterion is written into the code it added: + +> `src/AcDream.App/UI/Layout/DatWidgetFactory.cs:103` — +> "*(OP2 acceptance: no behavior change to any existing widget)*" + +That criterion is **falsified on two independent axes**, and the justification for one +of them is a factual claim about the codebase that is demonstrably wrong: + +> `DatWidgetFactory.cs:99-101` — "*an ORDINARY Type-5 ListBox that authors no template +> array (**none currently reach this factory** …) keeps falling to the generic +> UiDatElement fallback unchanged*" + +**Ten** Type-5 elements reach that factory today, in five shipped panels, and **every +one of them authors a non-empty template array**. Separately, **five** Type-8 elements +in four shipped panels change widget class. + +Total blast radius: **15 elements across 7 shipped, user-gated panels** — character +sheet, combat, spellbook/component book, vendor, examination, effects (positive + +negative), mini-game — silently change widget class in production. + +The 12,770-test green suite is a **structural false negative**: the fixture path never +calls the code that populates the new fields, so every fixture-driven test takes the +pre-OP2 branch while production takes the new one. + +This is the failure mode `docs/research/2026-08-05-c4-closeout-handoff.md` names as +binding: *a contract asserting a mechanism that does not exist*, plus *inferring a fact +you can observe*. + +The new Options-panel work itself (fixtures, tab table read, template list read, the +four `UIOption_*` mappings, the 25 tests) is sound. The problem is entirely in the +un-gated widening of two `switch` arms onto pre-existing authored data. + +--- + +## 1. Fixture-wide enumeration (the requested sweep) + +Method: parsed all **32** committed fixtures under +`tests/AcDream.App.Tests/UI/Layout/fixtures/` (27 pre-existing + 5 new), walked every +`ElementInfo` node, and recorded (a) the node `Type`, (b) whether any state authors dat +property `0x2E` / `0x64` / `0x72`, and (c) whether the JSON carries the new serialized +fields. + +### 1.1 Type-8 (`UIElement_TabControl`) — 6 occurrences, 5 of them pre-existing + +| Fixture | Element | Children | Authors `0x2E`? | Tab entries | Pre-OP2 widget | Post-OP2 widget | +|---|---|---|---|---|---|---| +| `character_2100002E.json` | `0x10000227` (**panel ROOT**) | 7 | yes | **3** | `UiDatElement` | `UiTabControl` | +| `combat_21000073.json` | `0x100000A2` | 16 | yes | **8** | `UiDatElement` | `UiTabControl` | +| `spellbook_21000034.json` | `0x100002A8` (**panel ROOT**) | 5 | yes | **2** | `UiDatElement` | `UiTabControl` | +| `vendor_21000012_100000B7.json` | `0x100000B8` | 6 | yes | **3** | `UiDatElement` | `UiTabControl` | +| `vendor_21000012_100000B7.json` | `0x1000008D` | 0 | **no** | 0 | `UiDatElement` | `UiTabControl` | +| `options_2100002B.json` | `0x10000208` | 10 | yes | 4 | *(new)* | `UiTabControl` ✅ | + +Authored tab tables actually present in the pre-existing panels (verbatim from the +fixtures, `{0x30 button, 0x31 page, 0x32 isDefault}`): + +- character `0x10000227`: `(0x10000228→0x1000022B, default)`, `(0x10000229→0x1000022C)`, + `(0x100003F8→0x100003F9)` +- combat `0x100000A2`: 8 rows, `(0x10000063→0x1000006A, default)` … `(0x10000382→0x10000383)` +- spellbook `0x100002A8`: `(0x100002A9→0x100002AC, default)`, `(0x100002AA→0x100002AD)` +- vendor `0x100000B8`: `(0x100000B9→0x1000007C, default)`, `(0x100000BA→0x10000084)`, + `(0x100000BB→0x1000008D)` + +### 1.2 Type-5 (`UIElement_ListBox`) — 16 occurrences, 10 of them pre-existing + +**Every pre-existing Type-5 element authors a NON-EMPTY property `0x64`.** + +| Fixture | Element | `0x64` rows | `0x72` | Controller constant | Post-OP2 (production) | +|---|---|---|---|---|---| +| `character_2100002E.json` | `0x1000023D` ×2 | **5** | `0x1000023E` | `CharacterStatController.ListBoxId` | `UiTemplateListBox` | +| `character_2100002E.json` | `0x10000532` | **1** | `0x10000533` | — | `UiTemplateListBox` | +| `effects_positive_2100001B.json` | `0x10000123` | **1** | `0x10000124` | `EffectsUiController.ListId` | `UiTemplateListBox` | +| `effects_negative_2100001B.json` | `0x10000123` | **1** | `0x10000124` | `EffectsUiController.ListId` | `UiTemplateListBox` | +| `examine_2100006B_100005F2.json` | `0x10000149` | **1** | — | `AppraisalUiController.CreatureStatsListId` | `UiTemplateListBox` | +| `examine_2100006B_100005F2.json` | `0x1000032D` | **1** | — | `AppraisalUiController.SpellFormulaListId` | `UiTemplateListBox` | +| `examine_2100006B_100005F2.json` | `0x10000335` | **1** | — | `AppraisalUiController.CreatureExtraListId` | `UiTemplateListBox` | +| `mini_game_2100001E.json` | `0x10000174` | **1** | — | — | `UiTemplateListBox` | +| `spellbook_21000034.json` | `0x10000464` | **2** | `0x10000465` | `SpellbookWindowController.ComponentListId` | `UiTemplateListBox` | +| `options_*` (3 new) | `0x100001FA` / `0x10000200` / `0x1000050D` | 3 / 8 / 9 | yes | *(new)* | `UiTemplateListBox` ✅ | + +### 1.3 The four `0x100000xx` UIOption class ids — **zero** pre-existing occurrences + +| Class id | Occurrences | Files | +|---|---|---| +| `0x10000036` CheckboxSlider | 2 | `options_2100002B.json` only | +| `0x10000037` Slider | 4 | `options_2100002B.json` only | +| `0x10000038` Menu | 1 | `options_2100002B.json` only | +| `0x10000044` CheckboxBitfield64 | 1 | `options_2100002B.json` only | + +**Clean.** These four arms are the only genuinely additive part of the switch change. +No pre-existing layout carries any of them. + +### 1.4 New-field presence per fixture (fixture-provenance check) + +`System.Text.Json` with `IncludeFields = true` serializes *every* public field, so a +fixture generated by the current generator necessarily contains `Outline`, +`OutlineColor`, `TabTable`, `TemplateList`, and `ScrollbarElementId` on every node. + +| Group | Files | `Outline`/`OutlineColor` | `TabTable`/`TemplateList`/`ScrollbarElementId` | +|---|---|---|---| +| The 5 new Options fixtures | `options_2100002B`, `options_gameplay_2100002A`, `options_character_21000028`, `options_chat_2100005C`, `options_config_21000029` | **present** | **present** | +| `chat_2100006f.json` | regenerated at CH6a | present | absent | +| The other 26 pre-existing | all remaining | **absent** | **absent** | + +--- + +## 2. MUST-FIX findings + +### MUST-FIX 1 — Type-8 arm silently re-classes 5 elements in 4 shipped panels + +**Site:** `src/AcDream.App/UI/Layout/DatWidgetFactory.cs:110` +```csharp +8 => new UiTabControl(info.TabTable), +``` +Unlike the Type-5 arm, this one carries **no guard at all** — every Type-8 element in +every layout now becomes a `UiTabControl`. + +`UiTabControl` (`src/AcDream.App/UI/UiTabControl.cs:42`) derives from bare `UiElement`. +`UiDatElement` (`src/AcDream.App/UI/Layout/UiDatElement.cs:31`) is +`UiElement, IUiDatStateful` and its constructor sets `ClickThrough = true` +(`UiDatElement.cs:111`). Four independent consequences: + +**(a) Lost background art — a straight rendering regression.** +Vendor `0x1000008D` is 800×20 and authors a DirectState sprite: +`StateMedia[""] = (100687632, DrawMode 1)`. `RetailUiRuntime.cs:2287` documents it by +name: *"backdrop element `0x1000008D` provides its own fill"*. It has **zero children** +and **no `0x2E` property** — it is pure chrome that happens to carry retail Type 8. +`UiTabControl` has no media plumbing whatsoever, so **the vendor panel's backdrop fill +stops drawing.** This is the clearest single defect in the commit. + +**(b) `ClickThrough` inverts `true → false` on all five.** +`UiElement.ClickThrough` is an auto-property defaulting to `false` +(`UiElement.cs:159`); `UiElement.HitTest` gates on it at `UiElement.cs:582-583` +(`if (ClickThrough) return null; return OnHitTest(...) ? this : null;`), and +`OnHitTest` defaults to a plain bounds test (`UiElement.cs:421-422`). Two of the five +are **panel roots** (character `0x10000227`, spellbook `0x100002A8`), so the entire +character sheet and the entire spellbook now claim every pointer hit inside their +bounds instead of passing unclaimed clicks through to the window shell behind them. + +**(c) Lost `IUiDatStateful` state propagation.** +`LayoutImporter.cs:168-169` runs `stateful.TrySetRetailState(...)` only for +`IUiDatStateful` widgets; `UiDatElement.TrySetRetailState` recurses into children +(`UiDatElement.cs:98-99`). `UiTabControl` does not implement the interface, so the +authored default-state propagation into the 7 / 16 / 5 / 6 children of these four tab +hosts silently stops. This is the exact mechanism the comment at +`LayoutImporter.cs:164-167` says the hook exists to preserve. + +**(d) Import-time visibility + click rebinding on controller-owned tabs.** +`UiTabControl.OnChildrenAttached` (`UiTabControl.cs:65-81`) runs *during* +`LayoutImporter.Build`, before any controller wiring, and calls +`RetailTabBinding.SetClick` (which also sets `ClickThrough = false` on each tab button, +`RetailTabBinding.cs:16`) plus `SwitchTo(default)` → `page.Visible = active` and +`RetailTabBinding.SetOpen`. All four panels already own these tabs +(`CharacterStatController.cs:428-432`, `SpellbookWindowController.cs:138-139`, +`VendorUiController.cs:587-589`). + +Mitigating detail, and it is *luck*, not design: on 3 of the 4 panels the tab table's +ids do **not** match the resolved child ids, so `FindDirectChild` misses and `SwitchTo` +is inert — vendor's table names pages `0x1000007C/0x10000084/0x1000008D` while the +controller and the actual children use `0x100000BC/0x100000C4/0x100000CD`; combat's +table names `0x1000006A..0x10000383` while the children are `0x100000AA..0x100005C3`. +Only spellbook and character resolve, and both controllers happen to re-assert +visibility afterwards. **Nothing in the commit establishes or tests this ordering.** + +Worth flagging on its own: `CharacterStatController.cs:439-449` carries an explicit +comment — *"WHY this cannot be done in the importer … The controller is the correct and +only place for initial tab-page selection"* — that OP2 now contradicts for four panels +without amending it. + +### MUST-FIX 2 — Type-5 guard rests on a false premise; 10 elements flip in production + +**Site:** `src/AcDream.App/UI/Layout/DatWidgetFactory.cs:104-105` +```csharp +5 when info.TemplateList.Count > 0 => new UiTemplateListBox( + info.TemplateList, info.ScrollbarElementId), +``` +The guard is correct *in the fixture path* and useless *in production*, because +`info.TemplateList` is populated by different code on the two paths: + +- **Fixture path:** `FixtureLoader.LoadInfos` (`FixtureLoader.cs:253-265`) deserializes + straight into `ElementInfo` and hands it to `LayoutImporter.Build`. + `ApplyCanonicalLegacyProjection` is **never called**. Stale fixtures carry no + `TemplateList` key → the field initializer's empty list survives → guard is `false` + → old `UiDatElement` behavior. Tests are green. +- **Production path:** `LayoutImporter.ToInfo` calls + `ElementReader.ApplyCanonicalLegacyProjection(info)` (`LayoutImporter.cs:465`), which + runs `info.TemplateList = ReadTemplateList(info)` from property `0x64`. Guard is + `true` for all ten → `UiTemplateListBox`. + +Section 1.2 shows all ten pre-existing Type-5 elements author 1–5 valid +`{0x63 layoutDid, 0x62 elementId}` rows. Six of them are named controller constants +(`AppraisalUiController.CreatureStatsListId` / `CreatureExtraListId` / +`SpellFormulaListId`, `CharacterStatController.ListBoxId`, `EffectsUiController.ListId`, +`SpellbookWindowController.ComponentListId`). + +Consequences: same `IUiDatStateful` loss and same media loss as MUST-FIX 1, plus a +**spurious child injection** — `UiTemplateListBox`'s constructor unconditionally does +`base.AddChild(_viewport)` (`UiTemplateListBox.cs:94`), so every one of these ten list +boxes gains a full-size anchored `UiScrollablePanel` as its first child. Since +`UiPanel` defaults `ClickThrough = false` (confirmed by the comment at +`UiPanel.cs:157-159`), that transparent viewport participates in hit-testing across the +whole list-box rect, competing with the `UiItemList` each controller adds afterwards on +an untested Z-order tie-break. + +Mitigating: the consumers all use `FindElement(...)` returning `UiElement?` with an +`is UiItemList` test plus an else-branch (`EffectsUiController.cs:90-94`, +`SpellbookWindowController.cs:196-199`, `AppraisalUiController.cs:169`, +`:241-243`, `:313-315`), so no controller *fails to mount*. The damage is confined to +rendering, hit-testing, and state propagation — but it is unreviewed and unmeasured. + +### MUST-FIX 3 — the harness cannot observe either change (the root enabler) + +`ApplyCanonicalLegacyProjection` has exactly two call sites, both DAT-only: +`LayoutImporter.cs:465` (`ToInfo`) and `ElementReader.cs:410` (`Merge`). The fixture +path reaches neither. Therefore: + +- Every fixture-driven test exercises the **pre-OP2** branch for all 15 affected + elements while production exercises the **post-OP2** branch. The "12,770 passed / + 0 failed, zero regressions" claim in the commit message is true and meaningless for + this change. +- The one existing test that touches an affected element, + `RetailFixtureConformanceTests.cs:100` + (`{ FixtureLoader.LoadCharacterInfos, 0x10000227u, 8u, 300f, 600f, 60 }`), asserts the + **`ElementInfo.Type`**, not the built widget class — it already knew `0x10000227` is + Type 8 and still could not catch this. +- `RetailLayoutFixtureGenerator.cs` now regenerates **all 32** layouts. The moment the + coordinator runs `ACDREAM_REGENERATE_UI_FIXTURES=1`, the 27 stale fixtures gain + populated `TabTable`/`TemplateList`, and the suite flips to production shapes for all + 15 elements in one commit, with no test written to explain the diff. + +Answering the task's forward-looking question directly: **no committed test depends on +fields present only in new-shape fixtures.** The exposure is the inverse and worse — +tests currently depend on those fields being *absent*. + +**Minimum fix shape (recommendation, not a mandate):** narrow both arms so they cannot +reach a pre-existing element — e.g. gate on the Options LayoutDesc ids, or on the +authored-tab-table ids actually resolving to direct children, or (cleanest) require +`info.TabTable.Count > 0 && every ButtonElementId/PageElementId resolves` before +choosing `UiTabControl`, and make `UiTabControl`/`UiTemplateListBox` carry the same +media + `IUiDatStateful` behavior `UiDatElement` provides. Whatever the shape, land a +test that builds the four affected pre-existing layouts **through the DAT-path +projection** (call `ApplyCanonicalLegacyProjection` over the fixture tree before +`Build`) and asserts the resolved widget class for all 15 ids. + +--- + +## 3. SHOULD-FIX + +### SHOULD-FIX 1 — `UiTemplateListBox` injects a hit-testable viewport unconditionally + +`UiTemplateListBox.cs:94` (`base.AddChild(_viewport)`) runs in the constructor, before +`LayoutImporter.BuildWidget` appends the element's own dat children. The widget does not +set `ConsumesDatChildren`, so authored children become **siblings of the viewport rather +than rows inside it** — silently correct today only because all three Options ListBoxes +author zero children, and a latent trap for OP3+ the moment one doesn't. Combined with +`UiPanel`'s `ClickThrough = false` default this is the mechanism behind MUST-FIX 2's +hit-testing exposure. + +### SHOULD-FIX 2 — the tab table's ids are base-layout ids on inherited layouts + +`UiTabControl.FindDirectChild` (`UiTabControl.cs:105-111`) matches +`child.DatElementId` against the raw `0x2E` values. On three of the four pre-existing +panels those values do not correspond to any child in the *incorporated* tree (§2, +MUST-FIX 1(d)) — strongly suggesting property `0x2E` carries **base-layout** ids that +`LayoutImporter.IncorporateChildren` remaps. The Options panel happens to author its +tab table with post-incorporation ids, so OP2's own tests pass. This will bite OP3+ +directly, because the Options pages mount cross-layout via `BaseLayoutId`/`BaseElement` +— exactly the case where the remap applies. Worth resolving now rather than +rediscovering it as an OP4 mystery. + +### SHOULD-FIX 3 — silent no-resolve is indistinguishable from correct behavior + +`OnChildrenAttached` and `SwitchTo` both treat a `FindDirectChild` miss as a no-op +(`UiTabControl.cs:70-72`, `:95-99`). A tab control whose entire table fails to resolve +(combat, today) is behaviorally identical to one with no table at all. Given +SHOULD-FIX 2, a diagnostic line on unresolved entries would have surfaced this during +implementation. + +--- + +## 4. NOTEs (verified clean, or informational) + +**NOTE 1 — `ElementReader.Merge` handles the three new fields correctly.** Verified. +`Merge` (`ElementReader.cs:344-412`) does *not* add scalar merge rules for `TabTable`, +`TemplateList`, or `ScrollbarElementId`, and does not need to: it merges the **state +property tables** first (`:400-408`, `UiStateInfo.Merge` giving derived-wins per +property) and then calls `ApplyCanonicalLegacyProjection(m)` at `:410`, which recomputes +all three from the merged effective state. Base-only `0x64` survives; derived `0x64` +replaces base wholesale (correct — arrays are whole-value properties, not concatenated); +neither is doubled. **No existing `Merge` behavior changed** — the new field reads are +appended after the existing `Outline`/`OutlineColor` block in +`ApplyCanonicalLegacyProjection` and touch no prior line. This addresses focus point 2 +in full: nothing here can drop or double a template list for the OP3+ cross-layout +mounts. + +**NOTE 2 — fixture-drift handling is exactly as described.** `git show --stat df9c7a35` +lists 5 fixture files, all `A` (added), zero `M`. The 27 pre-existing fixtures are +byte-untouched by this commit. And per §1.4 the 5 new fixtures **do** carry +`Outline`/`OutlineColor` (plus the three new fields), proving they came from the current +generator rather than being hand-built or stale-sourced. This focus-3 MUST-FIX condition +is **satisfied**. + +**NOTE 3 — the 25 new tests are hermetic.** +`OptionsPanelLayoutConformanceTests.cs` contains exactly 25 `[Fact]`s (matching the +commit message) and zero `IDatReaderWriter` / `DatCollection` references; every test +loads a committed fixture through `FixtureLoader`. The generator is env-gated +(`RetailLayoutFixtureGenerator.cs:42-50`, early-returns unless +`ACDREAM_REGENERATE_UI_FIXTURES == "1"`), so it is inert on a machine without DATs. The +csproj copies fixtures by wildcard (`AcDream.App.Tests.csproj:28`, +`UI\Layout\fixtures\*.json`), so the five new files need no registration. **The suite +remains green without the machine DAT.** Focus point 5: clean. + +**NOTE 4 — `IUiChildrenAttachedListener` cannot double-fire.** The hook is invoked +exactly once per widget instance, at `LayoutImporter.cs:174-175`, immediately after that +widget's own child loop and before `BuildWidget` returns. Depth-first ordering means a +nested listener fires before its ancestor, which is the correct order for +cross-reference resolution. Nested `LayoutImporter.Build` calls from +`UiTemplateListBox.TemplateResolver` create fresh widget instances, so no re-entry +hazard. Focus point 4's "fire twice / wrong subtree" concern: **not present.** + +**NOTE 5 — wheel / drag / clipping forwarding is structurally fine.** Because +`_viewport` is anchored `Left|Top|Right|Bottom` and `UiElement.HitTest` walks children +before gating on the parent (`UiElement.cs:571-583`), pointer and wheel events land on +the `UiScrollablePanel` directly — no forwarding shim is needed, and the scroll model, +per-row clipping, and `LineHeight` quantum are the same `UiScrollablePanel` code the +character sheet and CH6 already exercise. The anchor-capture behavior for a viewport +constructed at 0×0 inside a sized host is *not* covered by any of the 25 tests; low risk, +but unmeasured. + +**NOTE 6 — `UiOptionToggleSlider` first-match binding.** +`UiOptionToggleSlider.OnChildrenAttached` (`UiOptionToggleSlider.cs:33-42`) binds the +first `UiButton` and first `UiScrollbar` child. The `else if` is harmless (a child cannot +be both types). Zero pre-existing layouts carry class `0x10000036`, so no blast radius — +noted only because the pattern silently picks the first match rather than the authored +`0x10000219` / `0x1000021C` ids the class doc cites. + +**NOTE 7 — per-element allocation in the hot projection path.** +`ApplyCanonicalLegacyProjection` now allocates two fresh `List<>` instances +(`ReadTabTable`, `ReadTemplateList`, `ElementReader.cs:490-497`) for **every element on +every call**, including the ~99% that author neither property, and `Merge` calls it once +per inheritance level. Import is startup-only and not on a frame path, so this is +cosmetic — but an early `TryGetEffectiveProperty` bail before allocating would be free. + +**NOTE 8 — TS-72 is correctly filed.** The register hunk +(`docs/architecture/retail-divergence-register.md`) adds TS-72 for +`UiCheckboxBitfield64`'s click-toggle bit math and bumps the count 40 → 41. Correct per +the register rules; the widget is genuinely unreachable today. No comment on the bit +math itself — outside this lens. + +--- + +## 5. Verdict + +**REJECT.** + +The additive half of OP2 (five fixtures, the three `ElementReader` property readers, the +`Merge` interaction, the `IUiChildrenAttachedListener` hook, the four `UIOption_*` class +ids, the 25 hermetic tests) is well-built and clean under this lens. But two `switch` +arms were widened onto authored data the commit believed was absent, changing the widget +class of 15 elements across 7 shipped, user-gated panels — including a documented +rendering backdrop that now draws nothing — and the harness that reported "zero +regressions" is structurally incapable of seeing any of it. The re-review gate should be +the DAT-path projection test described at the end of §2. diff --git a/docs/research/2026-08-11-op2-review-mechanism.md b/docs/research/2026-08-11-op2-review-mechanism.md new file mode 100644 index 00000000..49d9625f --- /dev/null +++ b/docs/research/2026-08-11-op2-review-mechanism.md @@ -0,0 +1,580 @@ +# Campaign OP slice OP2 — MECHANISM-FAITHFULNESS review + +**Date:** 2026-08-11 +**Commit under review:** `df9c7a35` — "feat(ui): Campaign OP slice OP2 — tab control, +template ListBox, UIOption widget mappings" +**Lens:** retail-mechanism fidelity + contract fidelity against +`docs/plans/2026-08-10-options-panel-campaign.md` §4 OP2 and +`docs/research/2026-08-10-options-panel-structure.md`. +**Mode:** READ-ONLY. No build, no test run, no launch. Every claim below is either a +quote from committed source, a decode of a committed fixture, or an extract from +`docs/research/named-retail/acclient_2013_pseudo_c.txt`. + +## VERDICT: **REJECT** + +Two independent MUST-FIX classes, each with hard evidence: + +1. **The unconditional `Type 8` mapping (and the `Type 5` mapping) silently rewires four + already-shipped windows and nine already-shipped list boxes at runtime.** The committed + test suite cannot see it because the 27 pre-existing fixtures were deliberately NOT + regenerated, so they deserialize with `TabTable`/`TemplateList` empty. One + already-committed test (`EffectsUiControllerTests.cs:135-137`) encodes the pre-OP2 + contract and would fail against real DAT data. The slice's own acceptance criterion + ("no change to any existing widget's behaviour") is false as landed. +2. **The retail mechanism for both new widgets is fully present in the named decomp and + was not consulted.** `UIElement_Panel::Update/OpenTab/SetupTabPageHash` (Type 8 is + `UIElement_Panel`, **not** a class called `UIElement_TabControl` — that class does not + exist in the PDB) and `UIOption_CheckboxBitfield64::CreateChildren/ListenToElementMessage/ + Refresh` are all decompiled. Reading them confirms most of the port, refutes + `UiCheckboxBitfield64`'s checked-state predicate, refutes its "the dat authors nothing" + premise, and makes TS-72's stated unknown a non-unknown. + +The parts the prompt asked me to independently verify — the two structural identities, the +tab-table decode, the template-list decode, the effective-state discipline, the conformance +pins — are **all correct**. This is a REJECT on blast radius and on two mechanism defects, +not on the Options-panel data work, which is clean. + +--- + +## 1. Property readers (0x2E / 0x64 / 0x72) + +### 1.1 Tab table `0x2E` — decode CORRECT, entry filtering DIVERGES + +`ElementReader.ReadTabTable` (`src/AcDream.App/UI/Layout/ElementReader.cs:508-527`) decodes +`{0x30 button, 0x31 page, 0x32 isDefault}` and preserves authored array order. + +Independently decoded from the committed fixture +`tests/AcDream.App.Tests/UI/Layout/fixtures/options_2100002B.json`, element `0x10000208` +DirectState: + +``` +P0x58 = Enum(0x1) +P0x2E = Array[4] + Struct{0x30=Enum(0x1000020D), 0x31=Enum(0x10000212), 0x32=Bool(True)} + Struct{0x30=Enum(0x1000020E), 0x31=Enum(0x10000211), 0x32=Bool(False)} + Struct{0x30=Enum(0x1000050B), 0x31=Enum(0x1000050C), 0x32=Bool(False)} + Struct{0x30=Enum(0x1000020F), 0x31=Enum(0x10000213), 0x32=Bool(False)} +P0x57 = Enum(0x1000001A) +``` + +This matches research §1.3 / §10.1 exactly (Gameplay default, entry 0; Chat third at +`0x1000050B`/`0x1000050C`). The `Enum`/`DataId`/`Integer` coercion in `ReadStructMemberId` +(`:554-564`) is the right tolerance — the authored kind here is `Enum`. + +Retail's own reader is `UIElement_Panel::SetupTabPageHash @0x0046C2E0`. Three filters +acdream does not implement: + +| Retail | acdream | Impact | +|---|---|---| +| `BaseProperty::GetPropertyName(entry) != 0x2F` → **skip entry** (`0x0046C3A6`) | any `Struct` accepted | NOTE — every authored entry is `0x2F` | +| `InqProperty(0x30)` or `InqProperty(0x31)` missing → **skip entry** (`0x0046C3E5`, `0x0046C3FC`) | entry added with `0` for the missing member | **SHOULD-FIX** — a partial entry yields `UiTabTableEntry(0, pageId, …)` that still participates in `SwitchTo` and can hide a page | +| `tab != page` required (`0x0046C46B`) | not checked | NOTE | + +### 1.2 Template list `0x64` — decode EXACT + +`ReadTemplateList` (`:529-545`) decodes `{0x63 layout DID, 0x62 element id}`, in that order. +Retail: `UIElement_ListBox::AddItem(BaseProperty*) @0x0046F170` reads +`InqStructElement(0x63)` then `InqStructElement(0x62)` and calls +`AddItem(did, elementId, parent)`. **Byte-for-byte the same pair, same order.** ✔ + +Decoded from the committed fixtures (all three arrays, verbatim): + +``` +0x100001FA (Character) P0x64 = 3 × {0x2100002B, 0x10000216 / 0x10000217 / 0x10000218} +0x10000200 (Config) P0x64 = 8 × {0x2100002B, 216,217,218,21A,222,220,21D,221} +0x1000050D (Chat) P0x64 = 9 × {…same 8…, 0x10000520} +``` + +Identical to research §1.5's table. ✔ + +### 1.3 Scrollbar `0x72` — correct value, imprecise semantics, missing sibling + +`ReadReferencedElementId(info, 0x72)` (`:569-579`) is correct: the authored kind is `Enum` +and retail reads it with `InqEnum`. + +Exact retail semantics, from `UIElement_Scrollable::OnSetAttribute @0x00474630` (not cited +anywhere in the commit): + +``` +0x71 -> m_eHorizonalScrollbarID (+ UpdateScrollbarSize_(1)) +0x72 -> m_eVerticalScrollbarID (+ UpdateScrollbarSize_(0)) +``` +resolution is `UIElement::FindRelative(this, id)` validated by `DynamicCast(0xB)` and a +mutual-relative check (`GetScrollbarPointer_ @0x00473EC0`). + +- **NOTE:** `ElementInfo.ScrollbarElementId` / `UiTemplateListBox.ScrollbarElementId` should + be named *vertical*; `0x71` (horizontal) is unread. `0x71` IS authored in already-committed + fixtures (`vendor_…` `0x100000BD`/`0x100000C5`/`0x100000CE`, `combat_…` `0x100000B6`) — not + on the Options ListBoxes, so no OP2-observable gap, but the asymmetry deserves a comment. + +### 1.4 The round-5 N1 lesson — CORRECTLY OBSERVED + +All three readers resolve through `ElementInfo.TryGetEffectiveProperty` (`:215-235`), which +is DirectState-as-base then effective-default override — **not** a per-state first-wins scan. +`ApplyCanonicalLegacyProjection` is invoked from both entry points +(`LayoutImporter.ToInfo` and `ElementReader.Merge:410`), and the three new fields are +*recomputed* from the merged state rather than scalar-merged, which is strictly correct. +No N1 reintroduction. ✔ + +- **SHOULD-FIX (coverage):** the new conformance tests read `TabTable`/`TemplateList`/ + `ScrollbarElementId` off the **serialized** fixture fields — `LayoutImporter.Build` does not + re-run the projection at load. So `ReadTabTable`/`ReadTemplateList` are exercised only at + fixture-generation time and a future regression in either reader would not fail a single + test. One synthetic `ElementInfo` + `States[Direct].Properties[0x2E]` unit test per reader + closes this for ~20 lines. + +--- + +## 2. `UiTabControl` — right behaviour, wrong class, wrong lookup, invented fallback + +**The core behaviour is confirmed against retail.** `UIElement_Panel::Update @0x0046BD00`: + +``` +if (arg2 != m_OpenPageToken && arg3 != m_OpenTabToken) { + m_OpenTabToken = arg3; m_OpenPageToken = arg2; + foreach (tabId -> pageId) in m_TabPageHash { + visible = 0; state = 0xB; // Closed + if (m_OpenTabToken == tabId && m_OpenPageToken == pageId) { visible = 1; state = 0xC; } // Open + page = GetChildRecursive(this, pageId); if (page) page->SetVisible(visible); + tab = GetChildRecursive(this, tabId); if (tab) tab->SetState(state); + } + UIElement::BroadcastElementMessage(this, 0x2C, 0, 0); +} +``` + +`UiTabControl.SwitchTo` (`src/AcDream.App/UI/UiTabControl.cs:88-103`) does exactly this: +exactly one page `Visible`, every tab button driven to `Open`/`Closed` via +`RetailTabBinding.SetOpen` → `RetailUiStateIds.Open = 12` / `Closed = 11` +(`src/AcDream.App/UI/IUiDatStateful.cs:15-16`) = retail's `0xC`/`0xB`. ✔ The authored data +corroborates independently: the four tab buttons are `Type 0xC` text elements with +`DefaultStateId = 11 (Closed)` and authored state-11 grey `ARGB(255,127,127,127)` vs state-12 +`ARGB(255,204,204,204)` font colours. + +**The authored default IS honoured on mount** — `OnChildrenAttached` (`:65-81`) picks the +`IsDefault` entry and `SwitchTo`s it, and the fixture proves entry 0 (Gameplay, +`0x1000020D`/`0x10000212`) is the sole default. Retail takes the last `0x32==true` entry; +acdream's loop does too. ✔ + +Four defects: + +- **MUST-FIX (citation).** The class is documented as retail `UIElement_TabControl` + (`UiTabControl.cs:7`, `DatWidgetFactory.cs:108-109`). **No such class exists** — + `grep UIElement_TabControl docs/research/named-retail/symbols.json` returns nothing. Type 8 + is `UIElement_Panel` (`UIElement::RegisterElementClass(8, UIElement_Panel::Create)` at + `0x0046C6B7`). CLAUDE.md's phase checklist requires a named symbol + address; citing a + fictional class is what let items 2–4 below through. Correct anchors: + `UIElement_Panel::SetupTabPageHash @0x0046C2E0`, `::Update @0x0046BD00`, + `::OpenTab @0x0046BE20`, `::InqTabFromPage @0x0046BEB0`, + `::ListenToElementMessage @0x0046BF90`. +- **SHOULD-FIX.** `FindDirectChild` (`:105-111`) scans immediate children only; retail uses + `GetChildRecursive` for **both** the tab and the page. Fine for `0x2100002B` (all direct), + wrong for the Configure Keyboard tab host `0x1000049B` (research §8) this widget is + explicitly meant to also serve. +- **SHOULD-FIX.** `defaultEntry ??= _tabs[0]` (`:78`) is invented. Retail leaves + `m_OpenPageToken/m_OpenTabToken` at 0 when no entry authors `0x32`, so `Update(0,0)` hits the + guard and **no switch happens at all**. Either match retail or file a register row. +- **NOTE.** Retail's `BroadcastElementMessage(this, 0x2C, …)` after each switch, its + `SetMouseVisible(1)` per tab button during setup (`0x0046C4FB`), and its + `ListenToElementMessage` handling of `idMessage 0x18` (a page becoming visible re-opens its + tab; the panel follows its open page's visibility) are unported. Retail also drives + switching from a child element message (`0x19`/`0x29` → `OpenTab`) rather than a per-button + click delegate — acdream's `RetailTabBinding.SetClick` adaptation is consistent with the + existing character/vendor/spellbook tabs, but it is an adaptation and OP3 will need the + `0x2C` notification for the `OnVisibilityChanged` apply/revert semantics (research §3.6). + +### 2.1 **MUST-FIX — the Type-8 arm is unconditional and rewires four shipped windows** + +`DatWidgetFactory.cs:110` maps **every** Type-8 element to `UiTabControl`. Type 8 = +`UIElement_Panel`; a Panel without a `0x2E` array is just a panel. Two consequences, both +regressions relative to the previous `_ => new UiDatElement(info, resolve)` fallback: + +* `UiTabControl : UiElement` has **no `OnDraw`** — the authored DirectState media is not drawn + (`UiDatElement.OnDraw`, `src/AcDream.App/UI/Layout/UiDatElement.cs:193-216`). +* `UiElement.ClickThrough` defaults **false**; `UiDatElement` sets it **true** + (`UiDatElement.cs:111`). Every converted panel now swallows clicks that miss a child. + +I decoded the raw `States` bag of every committed fixture (the raw property survives even +though the projected field does not). Pre-existing Type-8 elements: + +| Fixture | Element | Size | Authored `P0x2E` | Media | Consequence | +|---|---|---|---|---|---| +| `vendor_21000012_100000B7.json` | `0x1000008D` | 800×20 | **none** | **yes** (`0x06004…`, DirectState) | loses its fill; `RetailUiRuntime.cs:2287` comments that this element "provides its own fill" | +| `vendor_…` | `0x100000B8` | 800×110 | 3 entries (`B9/BC`, `BA/C4`, `BB/CD`) | no | live tab control fights `VendorUiController` | +| `character_2100002E.json` | `0x10000227` (window root) | 300×600 | 3 entries (`228/22B`, `229/22C`, `538/539`) | no | live tab control; entry 3 is the **Titles** tab `CharacterStatController.cs:432` deliberately keeps inert | +| `spellbook_21000034.json` | `0x100002A8` (window root) | 300×600 | 2 entries | no | live tab control vs `SpellbookWindowController` | +| `combat_21000073.json` | `0x100000A2` | 800×60 | **8 entries** (`A3/AA` … `5C2/5C3`) | no | **no controller rebinds these** — `UiTabControl` takes ownership outright and hides 7 of the 8 stance pages | + +`VendorUiController`/`SpellbookWindowController`/`CharacterStatController` re-bind their tabs +after import, so their click hijack is overwritten; but `UiTabControl.OnChildrenAttached` +still runs first and mutates `Visible`/state, and **combat has no re-binder at all**. + +The minimum correct fix is (a) gate the arm exactly like the Type-5 arm +(`8 when info.TabTable.Count > 0`) so media-bearing plain panels such as `0x1000008D` keep +`UiDatElement`, (b) give `UiTabControl` `UiDatElement`'s media draw + `ClickThrough` +semantics, and (c) explicitly reconcile the four pre-existing hosts — either `UiTabControl` +owns their switching and the duplicated controller code goes, or Type 8 is not mapped +outside `0x2100002B` in this slice. + +### 2.2 **MUST-FIX — the same class of regression on the Type-5 arm** + +`DatWidgetFactory.cs:99-105` justifies the gate with "none currently reach this factory". That +is refuted by the same raw-property decode. Pre-existing Type-5 elements that **do** author +`P0x64`, and therefore flip from `UiDatElement` to `UiTemplateListBox` at runtime: + +| Fixture | Element | Media | Referenced by | +|---|---|---|---| +| `effects_positive/negative_2100001B.json` | `0x10000123` | no | `EffectsUiController.ListId` | +| `examine_2100006B_100005F2.json` | `0x10000149`, `0x10000335`, `0x1000032D` | no | `AppraisalUiController.CreatureStatsListId` / `CreatureExtraListId` / `SpellFormulaListId` | +| `character_2100002E.json` | `0x1000023D` | no | `CharacterStatController.ListBoxId` | +| `character_2100002E.json` | `0x10000532` | **yes** | Titles page list | +| `spellbook_21000034.json` | `0x10000464` | no | `SpellbookWindowController.ComponentListId` | +| `mini_game_2100001E.json` | `0x10000174` | **yes** | mini-game board | + +`UiTemplateListBox : UiPanel` zeroes `BackgroundColor`/`BorderColor` (`UiTemplateListBox.cs:92-93`) +and draws no authored media, is not `ClickThrough`, and inserts an anchored `UiScrollablePanel` +as child 0. + +**Smoking gun:** `tests/AcDream.App.Tests/UI/Layout/EffectsUiControllerTests.cs:135-137` +already asserts + +```csharp +UiElement listHost = Assert.IsType(layout.FindElement(EffectsUiController.ListId)); +UiItemList list = Assert.IsType(Assert.Single(listHost.Children)); +``` + +Against real DAT data both assertions now fail (wrong type; two children). It passes only +because `effects_positive_2100001B.json` predates the `TemplateList` field. + +**This is also why the commit message's fixture-drift paragraph is wrong.** It states the +regenerated drift was "an Outline/OutlineColor field pair added by an earlier commit, +bcc34ee3". The pre-existing fixtures contain **neither** `Outline`/`OutlineColor` **nor** +`TabTable`/`TemplateList`/`ScrollbarElementId` (grep count 0 for all five in +`character_2100002E`, `vendor_…`, `spellbook_…`, `combat_…`). Regeneration would have +populated the three new fields too — i.e. the decision to revert the drift is exactly what +suppressed the evidence of §2.1/§2.2. Regenerating those 27 fixtures is the acceptance gate +for any fix here. + +--- + +## 3. `UiTemplateListBox` — mechanism faithful + +`AddItemFromTemplateList(index)` (`src/AcDream.App/UI/UiTemplateListBox.cs:104-118`) +resolves `Templates[index]` through `TemplateResolver(layoutId, elementId)` and appends the +built subtree. Retail (`UIElement_ListBox::AddItemFromTemplateList @0x0046F2B0` → +`AddItem(BaseProperty*) @0x0046F170` → `AddItem(did, elementId, parent) @0x0046EDC0` → +`UIElementManager::CreateChildElementByDid(this, did, elementId)`) does the same thing: build +the named element out of the named LayoutDesc as a child of the list box, then append. +Instantiating through `LayoutImporter.ImportInfos(dats, layoutId, elementId)` + `Build` is the +correct acdream analogue of `CreateChildElementByDid`, and it is the same overload +`RetailDialogFactory` already uses. ✔ Out-of-range and no-resolver both return null, matching +retail's null returns. ✔ + +Scrollbar linkage is carried as data (`ScrollbarElementId`) for a page controller to bind to +`Scroll`; that mirrors `ChatWindowController`'s existing pattern. Acceptable for a +"mechanism-only" slice — see §1.3 for the naming/`0x71` notes. + +- **NOTE.** Row stacking uses `_viewport.ContentHeight` as the next row's `Top` + (`:114-116`). Retail sizes the box from `CalculatePaperSize` and the ListBox owns row + placement; the acdream model is equivalent for uniform top-anchored rows and is the same + model `CharacterStatController.RebuildActiveList` already uses. Fine. +- **NOTE.** Authored dat children of a `UiTemplateListBox` land as siblings of the internal + viewport (the class does not override `AddChild`), while template rows go inside it. Benign + for the Options ListBoxes (no authored children) but relevant to §2.2's converted lists. + +--- + +## 4. The two structural identities — **BOTH CONFIRMED** (independent check) + +I decoded the fixture myself rather than taking the commit message's word. + +### 4.1 `UIOption_Slider 0x10000037` ≡ horizontal `UIElement_Scrollbar` — **CONFIRMED** + +`options_2100002B.json` element `0x1000021C`: + +``` +0x1000021C T=0x10000037 (146,4) 120x12 DirectState image 0x060012C5 + P0x7B=Bool(True) P0x86=Float(0.5) P0x7C=Bool(True) P0x7E=Bool(False) + P0x82=Bool(False) P0x83=Bool(True) [+ P0x47/P0x48/P0x4B, see below] + child id=1 T=0x1 7x12 DirectState image 0x060012C6 +``` + +Compare the real `Type 0xB` scalar slider in `combat_21000073.json`, element `0x1000004F` +(gmCombatUI desired-power): + +``` +0x1000004F T=0xB 707x14 + P0x7B=Bool(True) P0x86=Float(0.5) P0x7C=Bool(True) P0x7E=Bool(False) + P0x82=Bool(False) P0x83=Bool(False) + child id=1 T=0x1 12x14 child id=4 T=0x3 child T=0x7 (meter) +``` + +**Identical property fingerprint, identical `child id = 1` thumb convention, `W > H`.** +`BuildScrollbar`'s horizontal branch (`DatWidgetFactory.cs:223-279`) reads exactly +`DefaultImage(info)` for the track and `child id 1` for the thumb. The identity holds; the +mapping is correct and the conformance test (`OptionsPanelFixture_SliderControl_BuildsAs…`) +pins `Horizontal == true` and a non-zero thumb sprite. ✔ + +### 4.2 `UIOption_Menu 0x10000038` ≡ `UiMenu`'s shape — **CONFIRMED** + +``` +options_2100002B 0x10000224 T=0x10000038 120x18 + P0x2 P0x6 P0x7=DID(0x21000043) P0x8 P0x9 P0xA=DID(0x21000043) P0x13 [+0x47/0x48/0x4B] + children: 0x10000355 T=0xC 100x18 (label) 0x10000356 T=0x3 17x19 (arrow cap) + +vendor_… 0x100000BF T=0x6 (UIElement_Menu) 117x18 + P0x2 P0x6 P0x7=DID(0x21000043) P0x8 P0x9 P0xA=DID(0x21000043) P0x13 + children: 0x1000034D T=0xC 100x18 (label) 0x1000034E T=0x3 17x19 (arrow cap) +``` + +Same seven properties, the same popup-template DID `0x21000043`, the same 100×18 label + +17×19 arrow-cap child pair. `new UiMenu()` is right, and it matches how the existing Type-6 +arm is built. ✔ + +- **NOTE (both identities).** The `UIOption_*` family carries an extra authored triple + `P0x47=Enum(0x10000397)`, `P0x48=DID(0x21000041)`, `P0x4B=Bool(True)` that the plain + Type-6/Type-11 counterparts do not. It is on the slider, the menu, **and** the already-mapped + `UIOption_Checkbox 0x10000219`, so it is a pre-existing uniform gap rather than something + OP2 introduced — but somebody should establish what `0x21000041/0x10000397` is (it looks like + a shared tooltip/option-chrome base pair) before OP4–OP6 rely on those rows rendering + completely. + +--- + +## 5. `UiOptionToggleSlider` / `UiCheckboxBitfield64` and TS-72 + +### 5.1 `UiOptionToggleSlider` — consistent with the authored templates ✔ + +Fixture-verified: `0x10000220` (272×20) and `0x10000221` (272×36) are both +`T=0x10000036` and both contain exactly one `0x10000219` (`T=0x10000035`, checkbox) and one +`0x1000021C` (`T=0x10000037`, slider); `0x10000221` additionally carries the two range-label +`Type 0xC` children `0x1000021E`/`0x1000021F`. Composing rather than consuming children is +right, and `OnChildrenAttached` (`src/AcDream.App/UI/UiOptionToggleSlider.cs:33-42`) picks the +first `UiButton` and first `UiScrollbar`, which is unambiguous for both templates. The +"labelled slider row `0x1000021D` is a plain `Type 3`, not `0x10000036`" test is a genuinely +good data-driven discriminator. ✔ + +### 5.2 `UiCheckboxBitfield64` — **MUST-FIX ×2** + +**(a) The checked-state predicate is wrong.** `IsSet` (`src/AcDream.App/UI/UiCheckboxBitfield64.cs:137-138`) +requires **all** mask bits set. Retail `UIOption_CheckboxBitfield64::Refresh @0x004859C0`: + +``` +lowHit = m_current.low & mask.low; +highHit = m_current.high & mask.high; +allSet = (lowHit == mask.low && highHit == mask.high); // computed… +ebx = ((lowHit | highHit) != 0) ? 0 : 1; // …but the VISIBLE flag is ANY-set +row = UIElement_ListBox::GetItem(this, i); +cb = UIElement::GetChild(row, 0x10000219); +UIElement::SetAttribute_Bool(cb, 0xE, ebx == 0); // checked <=> ANY mask bit set +``` + +Retail checks the box when **any** bit of the mask is set; acdream only when **all** are. This +matters immediately: 8 of the 13 Chat filter masks are multi-bit +(Gameplay `0x83912021`, Combat `0x00600040`, Magic `0x00020080`, AreaSpeech `0x00001004`, +Tells `0x00000018`, Allegiance `0x00040C00`, …). The click direction is derived from the +displayed state on both sides, so a wrong predicate inverts the next click too. + +**(b) The toggle math is NOT unknown — TS-72's premise is false.** +`UIOption_CheckboxBitfield64::ListenToElementMessage @0x00485AE0`: + +``` +if (idMessage == 1 + && GetAttribute_Int (pElement, 0x10000084, &index) // row index stamped at build time + && GetAttribute_Bool(pElement, 0xE, &onOff) // the row checkbox's new state + && index < childCount) +{ + BitUtils::SetBitsOnOrOff(&m_current, m_rgChildValues[index].mask, onOff); + vtable+0x2D4 (1); // Apply(1) +} +return UIElement_ListBox::ListenToElementMessage(this, edi); +``` + +`SetBitsOnOrOff(value, mask, on)` **is** "on ⇒ OR mask, off ⇒ AND-NOT mask" — so +`ToggleRow` (`:140-155`) is *correct*, and TS-72's claim that "the decompiled excerpt this +campaign pulled covers `Apply`'s WRITE side, not its own click-handler's bit math" describes +what the campaign grepped, not what exists. Every method is in +`acclient_2013_pseudo_c.txt` and `symbols.json`: `CreateChildren @0x00485DF0`, +`AddChild @0x004862A0`, `Refresh @0x004859C0`, `ListenToElementMessage @0x00485AE0`, +`GetValue @0x00486150`, `Changed/SaveCurrentValue/RestoreSavedValue/RestoreDefaultValue` +`@0x004856A0…0x00485780`. **TS-72 is therefore not an accurate statement of what is and is +not verified.** It should be rewritten (or retired) to say: the OR/AND-NOT half is +byte-confirmed correct; the checked-state predicate is byte-confirmed **wrong**. + +**(c) "The template authors zero children" is refuted by the committed fixture.** +`BuildCheckboxBitfield64` (`DatWidgetFactory.cs:810-815`) reads nothing from `ElementInfo`, +and the class doc (`UiCheckboxBitfield64.cs:11-14`) plus the commit message assert the +template "carries NO children and NO media". The actual authored element: + +``` +0x10000520 T=0x10000044 272x100 + P0x64 = Array[1] -> Struct{0x63=DID(0x2100002B), 0x62=Enum(0x10000521)} + P0x10000082 = DID(0x06004D17) + P0x10000083 = DID(0x06004D19) +``` + +It authors **its own row-template list** — the very `0x64` property this commit taught the +reader to decode — naming `0x2100002B/0x10000521`, plus two RenderSurface DIDs (which are the +LED sprite's `Highlight`/`Ghosted` images, `0x06004D17`/`0x06004D19`). `0x10000521` is a +272×20 container holding `0x10000219` (`Type 1`, base `0x21000044/0x10000329`) with the 13×13 +five-state LED child `0x10000328` — i.e. exactly the row art. Research §1.5 flagged +`0x10000521` as "UNKNOWN consumer"; **this fixture closes that unknown** (research U10 too), +and the decomp confirms it: + +``` +UIOption_CheckboxBitfield64::CreateChildren @0x00485DF0 + for i in 0 .. m_rgChildValues.m_num-1: + row = UIElement_ListBox::AddItemFromTemplateList(this, 0, nullptr); // template idx 0 + cb = GetChildRecursive(row, 0x10000219)->DynamicCast(1); + SetAttribute_Int(cb, 0x10000084, i); + UIElement_Text::SetStringInfo(cb, childValues[i].siLabel); + UIElement::SetTooltip (cb, childValues[i].siTooltip); + ResizeTo(GetWidth(), CalculatePaperSize(0, -1)); + Refresh(); +``` + +`UIOption_CheckboxBitfield64` **is a `UIElement_ListBox`** — it calls +`UIElement_ListBox::AddItemFromTemplateList`/`GetItem`/`CalculatePaperSize`/ +`ListenToElementMessage` on itself. So the faithful acdream port is +`UiCheckboxBitfield64` composing the `UiTemplateListBox` mechanism **this same commit +shipped**, not a hand-rolled `UiPanel` of synthesized `UiButton`s. As written, `AddChild` +(`:108-135`) fabricates an `ElementInfo{Type=1, Height=RowHeight}` with a hand-injected +`0x0B` property and an invented `RowHeight = 14f` whose comment claims "No authored row +height exists for this block (it is unauthored in the dat)" — the dat authors 272×20 with a +260×14 checkbox inset at (6,2). Consequences as landed: no LED art, no authored font +(`0x40000002`), no authored colour (`0x1B`), no authored label insets (`0x23`/`0x25`), no +justification (`0x14=Left`), no tooltip plumbing, and no outline seeding. + +`AddChild(uint64 mask, uint32 labelId, uint32 tooltipId) @0x004862A0` is also confirmed to be +pure bookkeeping (append a `ChildInfo{mask, StringInfo(label, tableEnum **0x10000006**), +StringInfo(tooltip, 0x10000006)}`) — note the table enum is `0x10000006`, not the `0x10000003` +used for headers/labels elsewhere. Worth pinning for OP5. + +--- + +## 6. Outline seams (`aa6635ae` / AP-192) — **PRESERVED** + +I scanned all five new fixtures for authored `0x21`/`0x22`. Every occurrence (32 total) is +`0x21 = Bool(True)` **in state 3 only**, on `Type 1` buttons: the three +Apply/Reset/Defaults buttons on each of Character/Chat/Config, and the seven Gameplay-tab +buttons. Those build through the untouched `BuildButton`, whose effective-default resolution +correctly yields outline-off — precisely the AP-192 deferral, unchanged. No authored `0x22` +anywhere; no authored `0x21` on any element reaching a *new* build path. ✔ + +- **NOTE.** `new UiMenu()` for `0x10000038` does not seed `Outline`/`OutlineColor` — but + neither does the pre-existing Type-6 arm, and `UiMenu`'s own doc records "No authored menu + element carries it today", which the Options menu `0x10000224` confirms (no `0x21`/`0x22` + on it or its label child). Unchanged, not regressed. +- **NOTE (folds into §5.2c).** `UiCheckboxBitfield64`'s synthesized rows have no outline + seeding either — but that is a symptom of bypassing the authored template, not a separate + defect. Building the rows through `AddItemFromTemplateList` gets the seam for free via + `BuildCheckbox`. + +--- + +## 7. Conformance tests vs research §10.1 — **ACCURATE**, coverage thin in two places + +I spot-checked every pinned constant against the research doc and the fixtures. All correct: + +| Test | Pins | Research §10.1 / §1.3 / §1.5 | Verdict | +|---|---|---|---| +| `…TabControl_HasFourEntriesInAuthoredOrder_GameplayDefault` | `020D/0212 default`, `020E/0211`, `050B/050C`, `020F/0213`, `Assert.Single(IsDefault)` | identical | ✔ | +| `CharacterListBoxFixture_TemplateList_…` | 3 × `{0x2100002B, 216/217/218}` | identical | ✔ | +| `ConfigListBoxFixture_TemplateList_…` | 8, order `216,217,218,21A,222,220,21D,221` | identical | ✔ | +| `ChatListBoxFixture_TemplateList_…` | 9, + `0x10000520` | identical | ✔ | +| scrollbar linkage ×3 | `0x100001FB`, `0x10000201`, `0x10000201` | identical | ✔ | +| `CharacterListBox_TemplateMechanism_ReachesSixHeadersAndFortyNineToggles` | `{3,15,6,11,7,7}` = 49 under 6 headers; `6*22 + 49*20` content height | §7 counts; template heights 292×22 / 272×20 confirmed in fixture | ✔ | +| `UiTabControl_*` ×5 | default mount, exactly-one-visible, Open/Closed flip, click-through, no-op | matches `UIElement_Panel::Update` | ✔ | + +Two gaps: + +- **SHOULD-FIX.** No test pins the reader itself (see §1.4) — every "reader" test actually + reads a serialized field. +- **SHOULD-FIX.** `UiCheckboxBitfield64_AddChild_TracksMaskAndTogglesOnClick` deliberately + picks the single-bit `Error` mask (`0x04000000`) because it "round-trips cleanly" — which is + exactly the case that cannot distinguish acdream's all-set predicate from retail's any-set + predicate. A multi-bit mask (Combat `0x00600040`) against a partially-set current value is + the test that would have caught §5.2a. +- **NOTE.** The plan's OP2 contract cites "tab-button ↔ page-slot switching per lane A §5". + §5 is the Chat-tab row map; the tab control is §1.3 and its host is §1.4. The implementer + cited the right sections; the plan's cross-reference should be corrected so OP8 does not + chase §5. + +--- + +## Summary of classifications + +**MUST-FIX** +1. `DatWidgetFactory.cs:110` — unconditional `Type 8 → UiTabControl` converts four shipped + Type-8 hosts (vendor `0x100000B8`, character `0x10000227`, spellbook `0x100002A8`, combat + `0x100000A2` — the last with **no** controller re-binder) and strips `UiDatElement` media + drawing + `ClickThrough` from the media-bearing vendor backdrop `0x1000008D`. +2. `DatWidgetFactory.cs:104` — the Type-5 gate's "none currently reach this factory" is false; + nine shipped ListBoxes author `P0x64` (effects, examine ×3, character ×2, spellbook, + mini-game). `EffectsUiControllerTests.cs:135-137` already encodes the pre-OP2 contract and + fails against real DAT data. Fixture staleness — not correctness — is what keeps the suite + green; regenerating the 27 fixtures is the acceptance gate for the fix. +3. `UiCheckboxBitfield64.cs:137-138` — checked-state predicate is all-bits-set; retail + (`Refresh @0x004859C0`) is any-bit-set. +4. `UiCheckboxBitfield64` / `DatWidgetFactory.cs:810` — the template `0x10000520` **does** + author content (`P0x64 → 0x2100002B/0x10000521`, plus `P0x10000082`/`P0x10000083`); retail + `CreateChildren @0x00485DF0` builds every row through `AddItemFromTemplateList(0)`. The + synthesized `ElementInfo` + invented `RowHeight = 14f` must be replaced by the authored + template path, and the false "unauthored in the dat" comments removed. +5. `UiTabControl.cs:7` / `DatWidgetFactory.cs:108` — cite `UIElement_Panel` + (`SetupTabPageHash @0x0046C2E0`, `Update @0x0046BD00`, `OpenTab @0x0046BE20`); the named + class `UIElement_TabControl` does not exist in the PDB. +6. `docs/architecture/retail-divergence-register.md` TS-72 — inaccurate as written; the click + math is in the decomp, the OR/AND-NOT half is confirmed right and the predicate half is + confirmed wrong. Also missing rows for the invented bitfield row geometry and the dropped + `0x64`/`0x10000082`/`0x10000083` authored data. + +**SHOULD-FIX** +7. `ReadTabTable` must skip entries missing `0x30`/`0x31` (retail `0x0046C3E5`/`0x0046C3FC`). +8. `UiTabControl.FindDirectChild` → recursive descendant lookup (retail `GetChildRecursive`). +9. `defaultEntry ??= _tabs[0]` is invented; retail performs no switch when nothing authors + `0x32`. +10. Add reader-level unit tests for `0x2E`/`0x64`/`0x72` (fixture tests only pin serialized + values). +11. Add a multi-bit-mask `UiCheckboxBitfield64` test. + +**NOTE** +12. `0x2F` MasterPropertyId and `tab != page` filters unimplemented (harmless today). +13. `0x72` is specifically the **vertical** scrollbar (`UIElement_Scrollable::OnSetAttribute + @0x00474630`); `0x71` (horizontal) is unread though authored in vendor/combat layouts. +14. `BroadcastElementMessage(0x2C)`, `SetMouseVisible(1)`, and the `idMessage 0x18` + visibility-follow path are unported — OP3 needs the first for §3.6 semantics. +15. The `UIOption_*` `P0x47/P0x48/P0x4B` triple (`0x21000041/0x10000397`) is unread across the + whole family, including the pre-existing checkbox mapping. +16. `UIOption_CheckboxBitfield64::AddChild` uses string table enum `0x10000006`, not + `0x10000003` — pin for OP5. +17. Research §1.5's "`0x10000521` … UNKNOWN consumer" and §10.5 U10 are now closed by the + committed fixture; the research doc should be updated. +18. The plan's OP2 contract cites "lane A §5" for the tab host; the correct sections are §1.3 + (tab table) and §1.4 (host). +19. The commit message's fixture-drift paragraph understates the drift (three new fields, not + one pair). + +--- + +## Appendix — retail extracts used (all from `docs/research/named-retail/`) + +| Symbol | Address | Used for | +|---|---|---| +| `UIElement::RegisterElementClass(8, UIElement_Panel::Create)` | `0x0046C6B7` | Type 8 identity | +| `UIElement_Panel::SetupTabPageHash` | `0x0046C2E0` | `0x2E` entry filters, default selection, `Update(page, tab)` tail | +| `UIElement_Panel::Update` | `0x0046BD00` | one-visible-page + `SetState(0xB/0xC)` + `Broadcast(0x2C)` | +| `UIElement_Panel::OpenTab` / `InqTabFromPage` / `ListenToElementMessage` | `0x0046BE20` / `0x0046BEB0` / `0x0046BF90` | switching entry points, `0x18`/`0x19`/`0x29` messages | +| `UIElement_ListBox::AddItemFromTemplateList` | `0x0046F2B0` | `0x64` array indexing | +| `UIElement_ListBox::AddItem(BaseProperty*)` | `0x0046F170` | `0x63`/`0x62` struct member order | +| `UIElement_ListBox::AddItem(did, elementId, parent)` | `0x0046EDC0` | `CreateChildElementByDid` seam | +| `UIElement_Scrollable::OnSetAttribute` | `0x00474630` | `0x71` horizontal / `0x72` vertical | +| `UIElement_Scrollable::GetScrollbarPointer_` | `0x00473EC0` | `FindRelative` + `DynamicCast(0xB)` validation | +| `UIOption_CheckboxBitfield64::CreateChildren` | `0x00485DF0` | rows built from template index 0 | +| `UIOption_CheckboxBitfield64::ListenToElementMessage` | `0x00485AE0` | `SetBitsOnOrOff` click math | +| `UIOption_CheckboxBitfield64::Refresh` | `0x004859C0` | any-bit-set checked predicate | +| `UIOption_CheckboxBitfield64::AddChild` | `0x004862A0` | `ChildInfo{mask,label,tooltip}`, table enum `0x10000006` | +| `UIElement::RegisterElementClass(0x10000037/38/36/44, …)` | `0x0048522A` / `0x0048436A` / `0x00484A3A` / `0x00485EDA` | the four UIOption class ids | + +Fixture decodes were performed with a throwaway JSON reader over the committed +`tests/AcDream.App.Tests/UI/Layout/fixtures/*.json` (scratchpad only, not committed). Nothing +in the repository was modified except this document.