From 527a3a40569ceabf66fb5bc499948f0e98f8a4d9 Mon Sep 17 00:00:00 2001 From: Erik Date: Tue, 11 Aug 2026 06:54:33 +0200 Subject: [PATCH] =?UTF-8?q?docs:=20OP5=20combined-lens=20review=20?= =?UTF-8?q?=E2=80=94=20APPROVE-WITH-FIXES=20(port=20verified,=204=20fixes)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The retail port held under independent re-derivation: AddChild takes ONE 64-bit mask (@0x004862A0, the Society-row split-direction confirmation), both spot-checked filter blocks byte-exact (12/13 rows, Gameplay fall-through cases), the (0x16,2) enum-map order proven from GetDIDFromEnum's body, AP-195's LED swap lands on state 6 of the exact 13x13 face child, self-sizing real, fixtures strictly additive (33 files, zero deletions). M1: the opacity apply closures never refresh their OWN slider — drag Default only, Reset: the value reverts (windows change) but the thumb stays; Defaults self-heals only by both-rows luck. S1: SaveChatOpacity does a full load+WriteAllText per drag TICK (dozens-to-hundreds of synchronous JSON round-trips per drag; retail batches via the dirty timer) — mark-dirty + flush on drag-end/Apply/hide. S2: the whole-row-culling viewport makes a straddling 240/260px block VANISH (no scissor stack), and the gate script asks the user to report exactly that as a regression — false-failure generator + missing register row. S3: no test for chatWindowMainFilter round-trip or the window-0 seed. N11 (forward, OP6): 0x10000201 is authored under BOTH the Config and Chat slots and the flat FindElement returns Chat's (last-write-wins); OP5's linkage test passes through the flat lookup for the wrong reason. Fix round queues behind the OP6 builder. Co-Authored-By: Claude Fable 5 --- docs/research/2026-08-11-op5-review.md | 335 +++++++++++++++++++++++++ 1 file changed, 335 insertions(+) create mode 100644 docs/research/2026-08-11-op5-review.md diff --git a/docs/research/2026-08-11-op5-review.md b/docs/research/2026-08-11-op5-review.md new file mode 100644 index 00000000..c309d2c2 --- /dev/null +++ b/docs/research/2026-08-11-op5-review.md @@ -0,0 +1,335 @@ +# Campaign OP slice OP5 (the Chat tab) — combined dual-lens review + +**Commit under review:** `e71e5a96` "feat(ui): Campaign OP slice OP5 — the Chat tab" +**Reviewer:** combined mechanism + blast-radius lens, 2026-08-11 +**Mode:** read-only (no build, no test run, no launch) +**Verdict: APPROVE-WITH-FIXES** — 1 MUST-FIX, 3 SHOULD-FIX, 15 NOTES. + +The retail port itself is correct. Every byte-level claim the slice makes was +re-derived independently from the pseudo-C and the committed fixtures and every +one held, including the research-ambiguity call the implementer settled. The +MUST-FIX is a binding-pattern slip (one of the three apply closures does not push +its own widget), not a fidelity error. + +--- + +## MUST-FIX + +### M1 — the two opacity rows' `apply` closures never refresh their OWN slider; Reset leaves the thumb desynced from the value + +`src/AcDream.App/UI/Layout/ChatOptionsPageController.cs:328-348` + +```csharp +defaultRow = new FloatOptionRow( + initialDefault, bindings.DefaultOpacityDatDefault, + apply: value => + { + bindings.SetDefaultOpacity(value); + activeRow!.RefreshFromLink(bindings.CurrentActiveOpacity()); // pushes the OTHER slider + }, // never slider1 + read: bindings.CurrentDefaultOpacity, + refresh: value => slider1.SetScalarPosition(value)); +``` + +This breaks the OP4 binding pattern the slice claims to follow. Both other +implementations push their own widget FIRST inside `apply`: + +- `src/AcDream.App/UI/Layout/CharacterOptionsPageController.cs:379-385` — + `apply: value => { checkbox.Selected = value; bindings.SetOption(...); }` +- this same file, `ChatOptionsPageController.cs:451-456` (the filter block) — + `apply: value => { block.SetCurrentValue(low, high); bindings.SetFilter(...); }` + +`FloatOptionRow.RestoreSavedValue`/`RestoreDefaultValue` +(`OptionPageModel.cs:~236-247`) only call `_apply`, never `_refresh` — exactly +like `BoolOptionRow`, which is safe *because* its `apply` does the widget push. + +**Reachable failure.** Drag the Default slider DOWN only. The link +(`ChatOpacityLink.SetDefault`) leaves Active untouched, so `activeRow.Changed` +is false and `defaultRow.Changed` is true. Click **Reset** → +`OptionPage.Reset()` (`OptionPageModel.cs:~465`) restores only `defaultRow` → +`_apply(saved)` → the live opacity reverts (windows visibly change) and +`slider2` gets refreshed — but `slider1`'s thumb stays where the user dragged +it. Symmetric case: drag Active UP only, then Reset. The same desync occurs on +the tab-switch-away path (`OnHidden() => Reset()`), and is only healed later by +`OnShown() => Apply() → SaveCurrentValue() → _refresh`. + +`Defaults()` happens to self-heal because it restores *every* row in sequence and +each row's apply refreshes the other one — pure luck, not design. + +**Not covered by anything.** No test asserts slider thumb position after +Reset/Defaults (`ChatOptionsPageControllerTests.cs:428-447` checks +`row.Current` and the bindings, not the widget), and the connected gate script +step 11 (`docs/research/2026-08-11-campaign-op-test-script.md`) drags *both* +sliders, which is precisely the case that self-heals. + +**Fix shape:** push the row's own slider inside each apply closure (mirroring +OP4), ideally from the post-link truth (`bindings.CurrentDefaultOpacity()`) +rather than the raw `value`, so a link-adjusted value can never leave the thumb +lying. Add one test: single-slider drag → `Reset()` → assert +`slider.ScalarPosition`. + +--- + +## SHOULD-FIX + +### S1 — full `settings.json` read+rewrite on every slider drag tick + +`src/AcDream.App/UI/RetailUiRuntime.cs:2112-2121` wires both opacity setters to +`SaveChatOpacity()` (`RetailUiRuntime.cs:696-707`), which runs +`store.LoadChat()` (open + `JsonDocument.Parse` of the whole file) followed by +`store.SaveChat(...)` → `SaveSection` → `File.WriteAllText` +(`SettingsStore.cs:538`). + +That closure runs once per **mouse-move event** while a thumb is held: +`UiScrollbar.cs:507-511` (`MouseMove when _draggingThumb`) → +`ChangeScalarPosition` → `ScalarChanged` → `FloatOptionRow.SetCurrentValue` → +`apply`. A drag across the track is therefore dozens-to-hundreds of synchronous +whole-file JSON round trips on the UI thread. + +Retail does not do this: the structure research doc §3.5 documents the +character-options blob behind a **dirty timer**, i.e. retail batches. The +retail-faithful shape here is mark-dirty + flush on drag end / Apply / page +hide / panel close. (The filter checkboxes are discrete clicks and are fine as +written.) + +### S2 — 240–260 px filter blocks inside a viewport that culls whole rows + +`src/AcDream.App/UI/UiScrollablePanel.cs:69` + +```csharp +child.Visible = top >= -0.5f && top + child.Height <= Height + 0.5f; +``` + +with the class's own doc (`UiScrollablePanel.cs:8-12`): *"clips whole rows +because the UI renderer does not have a scissor stack yet."* + +Until OP5 every row in this viewport was 8–36 px, so the all-or-nothing cull +read as ordinary row-granular scrolling. OP5 inserts five self-sized blocks of +12×20 = 240 px and 13×20 = 260 px into a 560 px viewport whose total content is +roughly 1.5 k px (6 headers @22 + 2 sliders @20/36 + 6 separators @8 + five +blocks). At most scroll offsets a whole block straddles the viewport edge and +therefore **vanishes entirely** rather than clipping — a 260 px pop per +scroll-step, on a page that must be scrolled to reach section 6. + +The connected gate script asks the user to verify exactly this +("each block is exactly tall enough to show all of its own rows with **no +clipping** and no dead space", and reports "any block whose height looks +clipped"), so this is very likely to come back as a gate failure attributed to +the self-sizing work rather than to the viewport. + +There is no register row for the whole-row cull today (searched +`retail-divergence-register.md` for scissor/whole-row — AD-17/AP-117 are the 3-D +clip rows, unrelated). Either clip this viewport properly or file the row + +issue before the gate. + +### S3 — the new persistence key has no round-trip test, and neither does the new seed + +`SettingsStore.cs:200` (load) / `:587` (save) add `chatWindowMainFilter`, and +`RetailUiRuntime.cs:926-928` adds the `MountChat` seed for window 0. Neither is +pinned: +`tests/AcDream.UI.Abstractions.Tests/Panels/Settings/SettingsStoreTests.cs:311+` +covers `ChatWindow1Filter`..`ChatWindow4Filter` only, and +`SaveChat_then_LoadChat_round_trips_all_fields` (`:279`) predates the field. +Two-line addition; the floaty test is the template. + +--- + +## NOTES (verified claims + forward hazards) + +**N1 — mechanism item 2 (ONE 64-bit mask, not a 128-bit high:low pair): +CONFIRMED. The implementer's reading is right.** +Declaration: `UIOption_CheckboxBitfield64::AddChild(class UIOption_CheckboxBitfield64* this, uint64_t arg2, uint32_t arg3, uint32_t arg4)` +@ `0x004862A0` (`acclient_2013_pseudo_c.txt:146466`); its body stores +`var_128 = arg2` (low) and `var_124 = *(uint32_t*)((char*)arg2)[4]` (high) into +one `ChildInfo` — a single 64-bit stack argument split by BN's usual +push-pair artifact. `SetDefaultValue` is likewise `uint64_t` @ `0x00485780`. +Independent confirmation of the split *direction*: the Society row is emitted as +`var_20_11 = 1; AddChild(..., 0, 0, 0)` @ `0x0049FEFB`/`0x0049FF01` — high dword +1, low dword 0 → `0x0000000100000000`, byte-identical to the committed literal +(`ChatOptionsPageController.cs:120`). Every other row is preceded by +`var_20_N = 0`. `SplitMask`/`CombineMask` +(`ChatOptionsPageController.cs:472-477`) are the correct inverse pair. **A wrong +reading here would have mis-masked every row; it did not.** + +**N2 — the five blocks vs §5.2's byte-decoded inventory: exact, two blocks +spot-checked in full.** +Main window inline block `0x0049FDD1`–`0x0049FF23`: `SetDefaultValue(0xfbffffff, 0)`, +`SetUserData(8)`, then **12** `AddChild` calls — `0x600040, 0x20080, 0x1004, +0x18, 0x40c00, 0x80000, 0x8000000, 0x10000000, 0x20000000, 0x40000000, +[hi=1/lo=0], 0x4000000` — no Gameplay row. Floaty-4 inline block +`0x004A0000`–`0x004A0176`: `SetUserData(5)`, default `(0x78000000, 0)`, **13** +calls, the same 12 preceded by `0x83912021`. Helper +`AddCheckboxBitfield64Option @0x0049EDA0`: case 2 → `0x101c`, case 3 → +`0x40c00`, case 4 → `0x80000`, case 5 → `0x78000000`, all falling through +`label_49EE4F` into the Gameplay `AddChild`; case 8 sets `0xfbffffff` and breaks +past it. All of that matches `FilterRows`/`FilterBlocks` +(`ChatOptionsPageController.cs:107-151`) and `ChatWindowState`'s new named +constants (`ChatWindowState.cs:82-86`) exactly. Template indices also confirmed +against the committed fixture: `options_panel_2100006E_1000018D.json` ListBox +`0x1000050D` templates 0..8 = `0x10000216 / 0x10000217 / 0x10000218 / +0x1000021A / 0x10000222 / 0x10000220 / 0x1000021D / 0x10000221 / 0x10000520`, so +header 0, separator 1, unlabelled slider 3, labelled slider 6, bitfield 8 are all +right (research §1.5's table). + +**N3 — `GetDIDFromEnumStatic(0x16, 2)` argument order: CONFIRMED.** +`GetDIDFromEnumStatic(__return, arg2, arg3)` @ `0x00413910` forwards to +`GetDIDFromEnum(this, __return, arg3 := arg2, arg4 := arg3)`; the body +(`0x00413940`) runs `EnumIDMap::EnumToDID(masterMap, arg4)` **first** +(`0x004139A1`) and `EnumToDID(subMap, arg3)` second (`0x004139E6`). With the call +being `(0x16, 2)`: master[2] → submap, submap[0x16] → DID. That is exactly +`RetailDataIdResolver.Resolve(dats, enumValue: 0x16, enumCategory: 2)` +(`src/AcDream.Content/RetailDataIdResolver.cs:12-27`) as +`ChatOptionsDatDefaults.cs:40-43,73` calls it. Defaults 0.5/1.0 are pinned +against the live installed DAT (`ChatOptionsDatDefaultsTests.cs:32-34`, skips +gracefully without DATs) and degrade to the documented `ChatInterface` +constructor fallback, never to an invented value. + +**N4 — AP-195's LED swap: semantics AND target element are right.** +`Refresh @0x004859C0` computes any-set (`ebx == 0`) and all-set (`var_d_1`), +writes attribute `0x0E` = any-set on checkbox `0x10000219`, and **only while +any-set** calls `SetMediaImageForState(GetFirstChildElement(checkbox), +0x10000082-if-all-else-0x10000083, 1, 6)`. `SetMediaImageForState @0x00463BE0` +writes exactly **one** state — id `arg4` = 6 — not a 1..6 range. The committed +fixture shows why that is equivalent to our port: LED child `0x10000328` state 6 +is `Highlight` and its authored image already IS `0x06004D17` +(= property `0x10000082`), while `Ghosted` is `0x06004D19` (= `0x10000083`); a +checked button resolves to `Highlight` via +`UiButton.UpdateVisualState`/`UiButtonStateMachine`. Our `FaceFileOverride` is +set on the checkbox `UiButton`, and `DatWidgetFactory.BuildButton:718-724,765-771` +makes that button's *face* be the 13×13 LED child (checkbox `StateMedia` is +empty, exactly one stateful face child), with `FaceLeft/Top/Width/Height` = +`0/1/13/13`, and `_faceSegments` empty so `UiButton.cs:347`'s override branch is +the one taken. All-set / partial / unset render identically to retail. + +**N5 — the self-sizing half is real.** `CreateChildren @0x00485DF0` ends +`ResizeTo(GetWidth(), CalculatePaperSize(&vtable, 0, 0xffffffff))` then +tail-jumps `Refresh`. `UiCheckboxBitfield64.cs:237` grows `Height` only and +leaves `Width` at the authored 272 — the right shape. Minor inexactness: +`CalculatePaperSize` is retail's own paper metric (list insets included); +we use the exact sum of row heights. Non-blocking, worth a line in the class doc. + +**N6 — retirement note truthful, AP-187 broadening accurate.** The AP-195 row is +deleted in this same commit, the §3 header count goes 138 → 137, and AP-187 is +rewritten (not duplicated) to cover the main window's filter plus the new live +write path, citing `2026-08-09-chat-retail-window-shell.md` §4.1/§4.4/§6.1 and +the CH6f plan row rather than restating them — exactly the "cite, don't +duplicate" instruction in the plan's OP5 section. Register bookkeeping for this +slice is clean. + +**N7 — fixture regeneration is strictly additive (the OP2 lesson holds).** +`git diff --numstat` over `tests/AcDream.App.Tests/UI/Layout/fixtures/`: 33 files, +**0 deleted lines**. Every added line is one of exactly two shapes +(`"LedCheckedSprite": N,` / `"LedUncheckedSprite": N,`), and only ONE element +across all 33 files carries non-zero values — `100683031`/`100683033` +(`0x06004D17`/`0x06004D19`) on `0x10000520` in `options_2100002B.json`, precisely +as AP-195 documented. No pre-existing value changed anywhere, so no conformance +suite can shift meaning. (Cosmetic: the commit message says "all 19 committed +layout fixtures"; it is 33.) + +**N8 — `UiButton.FaceFileOverride` blast radius: none.** Only two writers, both +in `UiCheckboxBitfield64.ApplyRowVisualsForMask` (`UiCheckboxBitfield64.cs:324,328`); +default `null` everywhere else; consulted at exactly one draw site +(`UiButton.cs:347`) in the single-face branch. Every other `UiButton` instance in +the codebase is bit-identical to before. One silent-failure caveat: if a future +bitfield row template ever authors **more than one** stateful face child, +`_faceSegments` becomes non-empty and the override is ignored with no log +(`UiButton.cs:340-346`). Not reachable with the shipped `0x10000521`. + +**N9 — `ElementInfo` threading is safe.** The two reads sit in the same +"recompute from the current effective state every call" block as +`TabTable`/`TemplateList`/`ScrollbarElementId` (`ElementReader.cs:509-531`), so +base+derived merge is picked up automatically; `ReadReferencedElementId` +(`ElementReader.cs:602-612`) accepts `Enum`/`DataId`/`Integer`, which covers the +Kind-2 DataId properties. Only `UiCheckboxBitfield64` consumes the fields +(`DatWidgetFactory.cs:165-166`), so an unrelated element that happened to author +`0x10000082` would populate a field nobody reads. + +**N10 — `AddPrebuiltRow` changes nothing for existing template lists.** It is a +literal extraction of `AddItemFromTemplateList`'s tail (`UiTemplateListBox.cs:179-217`); +the caller now delegates. Only new caller is `ChatOptionsPageController.cs:442`. +Other `AddItemFromTemplateList` consumers are the Character tab +(`CharacterOptionsPageController.cs:285,308,322`) and OP5's own header/separator/ +slider rows; the component/effect/examine row factories use their own helpers. +Two new tests pin both the stacking and the "a row added AFTER a prebuilt block +stacks below the block's FINAL height" property. + +**N11 — the shared scrollbar id: OP5 is right, and OP6 MUST repeat it (with a +sharper test).** `0x10000201` is authored in **two** page slots of the host tree: +under the Config slot `0x10000213` (ListBox `0x10000200`, 8 templates) and under +the Chat slot `0x1000050C` (ListBox `0x1000050D`, 9 templates). `ImportedLayout` +is a flat `Dictionary` written as `byId[info.Id] = w` during a +depth-first build (`LayoutImporter.cs:97,120`), i.e. **last-write-wins**, and the +Chat slot is the LAST top-level child (index 9) while Config is index 7 — so a +flat `FindElement(0x10000201)` returns the **Chat** instance today. OP5's scoped +lookup from `PageSlotElementId` (`ChatOptionsPageController.cs:224-227`) is +correct and future-proof, but currently returns the same object the flat lookup +would. Consequences: +1. **OP6 must scope from `0x10000213`.** A plain `layout.FindElement(0x10000201)` + in the Config controller will bind Config's scroll model onto **Chat's** + scrollbar, and nothing will fail loudly. +2. `ScrollbarLinkage_ModelPointsAtTheChatListBoxScroll` + (`ChatOptionsPageControllerTests.cs:566-588`) asserts through the FLAT lookup, + so it passes for the wrong reason and would not catch a de-scoping + regression. Assert through `UiElement.FindDescendant(chatPageSlot, ...)`. +The same shared-id shape applies to `0x100001FC/FD/FE` (Apply/Reset/Defaults), +present in all three page slots and already scoped per page by +`OptionsPanelController.cs:214-222` — including `ChatPageId`, so OP5 inherits +correct per-page ghosting for free. + +**N12 — plan cross-reference is stale.** The plan's OP5 section +(`docs/plans/2026-08-10-options-panel-campaign.md:287`) cites "lane A §8's +byte-decoded masks", but §8 of `2026-08-10-options-panel-structure.md` is +*Configure Keyboard*; the masks are §5/§5.2. The code cites §5.2 correctly +(`ChatOptionsPageController.cs:13,104,423`). Fix the plan pointer. + +**N13 — naming reads backwards.** `UiCheckboxBitfield64.UncheckedLedSprite` / +`ElementInfo.LedUncheckedSprite` mean "checked but only PARTIALLY set"; a genuinely +unchecked row gets **no** override at all. The doc comments say so, but the names +mislead at the call site. `PartialLedSprite` would match retail's own semantics. + +**N14 — no invented user-visible text.** Every string is resolved by hashing +retail's own symbol name through `DatStringResolver.ComputeHash`, and all of them +exist in the pseudo-C globals: the six `ID_ChatOption_*_Section` headers, the 13 +`ID_ChatOption_TextFilter_*` labels (including retail's own "Allegience" +misspelling, correctly used as the hash key with the correctly-spelled table +value), their `_Desc` tooltip counterparts, and +`ID_UI_Value_Transparent`/`ID_UI_Value_Opaque`. Every resolution failure logs and +renders empty rather than falling back to English +(`ChatOptionsPageController.cs:262-268,364-368,429-433`). + +**N15 — OP4-pattern compliance otherwise holds.** `OnShown() → Apply() → +SaveCurrentValue()` re-reads the live source and pushes it to the widget for both +new row types (`OptionPageModel.cs` `FloatOptionRow.SaveCurrentValue`, +`BitfieldOptionRow.SaveCurrentValue`), with tests +(`ChatOptionsPageControllerTests.cs:407-425,505-521`). Apply/Reset ghosting is +wired per page including `ChatPageId` (`OptionsPanelController.cs:214-235`). +`UiCheckboxBitfield64.SetCurrentValue` deliberately does not fire `ValueChanged`, +so a re-seed cannot loop back as a user edit (pinned at +`OptionsPanelLayoutConformanceTests.cs:~468-479`). Retail's slider labelling is +matched too: `InitOptions` calls `SetSliderLabel` for the **labelled** slider only +(`0x0049FD37`), and the port sets range captions on row 2 only +(`ChatOptionsPageController.cs:311-315`) and no row caption on either — retail +sets none either. End-to-end live routing verified: checkbox click → +`ToggleRow` → `ValueChanged` → `BitfieldOptionRow.SetCurrentValue` → apply → +`ChatWindowState.SetFilter` (a real write for window 0 since CH6a/b, +`ChatWindowState.cs:158-167`, bumping `Revision`) + `SaveChatWindowFilters` → +`ChatWindowController.GetTranscriptLines` re-lays out because its cache keys on +the filter value itself (`ChatWindowController.cs:669-676`). Mount ordering is +safe: `MountChat` (`:366`) → `MountFloatingChatWindows` (`:367`) → +`MountOptionsPanel` (`:374`), so `SaveChatWindowFilters`' all-five write can +never persist default floaty filters over stored ones. `ChatSettings`' new +parameter is inserted mid-list in a positional record, but both construction +sites use named arguments (`SettingsStore.cs:186`, `SettingsStoreTests.cs:282`), +so no silent argument shift. + +--- + +## Summary table + +| ID | Class | One-line | +|---|---|---| +| M1 | MUST-FIX | Opacity `apply` closures don't push their own slider; single-row Reset desyncs the thumb from the value | +| S1 | SHOULD-FIX | Whole `settings.json` load+rewrite per slider drag tick; retail batches behind a dirty timer | +| S2 | SHOULD-FIX | 240–260 px blocks in a viewport that culls whole rows — blocks blink out mid-scroll; no register row | +| S3 | SHOULD-FIX | No round-trip test for `chatWindowMainFilter`, none for the new `MountChat` seed | +| N1–N15 | NOTE | See above — masks/defaults/argument-order/AP-195/fixtures all verified; OP6 scrollbar-scoping hazard in N11 |