docs: OP5 combined-lens review — APPROVE-WITH-FIXES (port verified, 4 fixes)
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 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
6097d2d136
commit
527a3a4056
1 changed files with 335 additions and 0 deletions
335
docs/research/2026-08-11-op5-review.md
Normal file
335
docs/research/2026-08-11-op5-review.md
Normal file
|
|
@ -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<uint, UiElement>` 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 |
|
||||
Loading…
Add table
Add a link
Reference in a new issue