diff --git a/docs/research/2026-08-11-op5-recheck.md b/docs/research/2026-08-11-op5-recheck.md new file mode 100644 index 00000000..aae4a5e9 --- /dev/null +++ b/docs/research/2026-08-11-op5-recheck.md @@ -0,0 +1,332 @@ +# Campaign OP slice OP5 — fix-round re-check + +**Commit re-checked:** `6d0b0f92` "fix(ui): OP5 review fixes — thumb sync, batched +opacity writes, cull register row, tests" +**Against:** `docs/research/2026-08-11-op5-review.md` (M1 MUST-FIX, S1/S2/S3 +SHOULD-FIX, N11) +**Mode:** read-only (no build, no test run, no launch); git reads only +**Verdict: CLOSED** — all six findings genuinely closed. 0 MUST-FIX, 2 SHOULD-FIX +and 5 NOTE residuals, all on the NEW surface the fix introduced rather than on +the findings themselves. + +--- + +## Per-finding closure + +### M1 — CLOSED + +`src/AcDream.App/UI/Layout/ChatOptionsPageController.cs:361-382` + +Both apply closures now push their own slider first, from the post-link truth: + +```csharp +bindings.SetDefaultOpacity(value); +slider1.SetScalarPosition(bindings.CurrentDefaultOpacity()); // M1: own slider first +activeRow!.RefreshFromLink(bindings.CurrentActiveOpacity()); +``` + +symmetric at `:376-378` for `slider2`. This is the sibling pattern the review +cited (`CharacterOptionsPageController.cs:379-385`, and this file's own filter +block). + +**Exact scenario walked** (drag Default only → Reset → value AND thumb revert): + +1. Seed default 0.3 / active 0.9. `slider1.ScalarPosition = 0.3`, + `defaultRow._saved = 0.3`. +2. Drag Default down to 0.1. `UiScrollbar.ChangeScalarPosition` + (`UiScrollbar.cs:566-570`) → `ScalarChanged` → `FloatOptionRow.SetCurrentValue` + (`OptionPageModel.cs:234-239`) → `apply`. `ChatOpacityLink.SetDefault(0.9, 0.1)` + leaves active at 0.9 (`ChatOpacityLink.cs:31-37`), so `activeRow.Changed` + stays false and only `defaultRow` is changed — the review's exact + single-row case. +3. Click Reset → `OptionPage.Reset()` (`OptionPageModel.cs:628-633`) restores + only `defaultRow` → `RestoreSavedValue` (`OptionPageModel.cs:269-273`) calls + `_apply` and never `_refresh` — unchanged. The revert now reaches the thumb + because `_apply` itself writes `slider1.SetScalarPosition(0.3)`. + +**Do the tests assert a reachable thumb state?** Yes. +`SetScalarPosition` writes `ScalarPosition` (`UiScrollbar.cs:85-86`), which is +the exact value the draw path multiplies into the thumb origin +(`UiScrollbar.cs:228-230`, `float x = travel * ScalarPosition`). Asserting +`ScalarPosition` IS asserting thumb position; there is no separate cached +geometry. +`Reset_AfterOnlyDefaultSliderChanged_RevertsTheThumbToo` and its Active twin +(`tests/AcDream.App.Tests/UI/Layout/ChatOptionsPageControllerTests.cs:472-535`) +assert it, and both also assert `Assert.False(otherRow.Changed)` so the +self-healing two-row case cannot be mistaken for the single-row one. + +**Does drag-the-other survive (AP-190 never-clamp)?** Yes, untouched. The link +returns the value being set verbatim — `SetDefault` returns `newDefault` +(`ChatOpacityLink.cs:36`) and `SetActive` returns `newActive` (`:50`); only the +OTHER value moves. `RetailWindowOpacityController.SetDefaultOpacity/SetActiveOpacity` +(`RetailWindowOpacityController.cs:89-105`) delegate straight to it. So +`bindings.CurrentDefaultOpacity()` inside `defaultRow`'s own apply is always +exactly the dragged value (clamped to [0,1] only), and the new own-slider push +is a no-op mid-drag and load-bearing only on Reset/Defaults. It cannot fight a +drag or re-introduce clamping. + +### S1 — CLOSED + +**Fires exactly once per real drag, all three paths.** The identical +`bool wasDragging` guard is applied at all three `MouseUp` sites — vertical +model `UiScrollbar.cs:443-450`, horizontal model `:500-507`, horizontal scalar +`:543-550`. `_draggingThumb` is set only inside a `MouseDown` branch and +cleared only at `MouseUp`, so within one real gesture exactly one +`DragCompleted` is emitted. Covered by four tests +(`tests/AcDream.App.Tests/UI/UiScrollbarTests.cs:106-201`), including +"10 MouseMoves fire zero completions". + +**The `IsDragging` discriminator.** The claim "a slider drag and a button click +can never be concurrent" holds under this input model, for a stronger reason +than the phrasing suggests: the pressed slider holds pointer capture +(`UiScrollbar()` sets `CapturesPointerDrag = true`, `UiScrollbar.cs:152`; +`UiRoot.OnMouseDown` → `SetCapture(target)`, `UiRoot.cs:492`, then the +`CapturesPointerDrag || HandlesClick` branch at `:549-557`), and Reset/Defaults +are `UiButton` clicks that require their own left-button MouseDown → left-up +over the same element (`UiRoot.cs:649-661`). One left button cannot be down on +both. Also note the discriminator is correctly per-slider: `defaultRow`'s apply +can only be reached from `slider1`'s own `ScalarChanged` or from Reset/Defaults — +dragging `slider2` reaches `defaultRow` via `RefreshFromLink`, which never +calls apply (`OptionPageModel.cs:250-255`). + +**Live opacity still applies per tick.** `SetDefaultOpacity`/`SetActiveOpacity` +still run `ReapplyAll()` on every call (`RetailWindowOpacityController.cs:93,104`); +only the disk write moved. Pinned by +`DraggingDefaultSlider_DefersTheSettingsWriteUntilDragEnd`'s +`Assert.NotEqual(0.2f, fakeBindings.DefaultOpacity)` +(`ChatOptionsPageControllerTests.cs:625` region). + +**Reset/Defaults flush immediately.** Neither is mid-drag, so +`if (!sliderN.IsDragging) bindings.FlushOpacity()` fires inline — same +single-write-per-discrete-edit shape as pre-fix. Pinned by +`ResetClick_FlushesImmediately_NotMidDrag` (2 flushes: the edit, then the +Reset). `Defaults()` restores both rows unconditionally +(`OptionPageModel.cs:638-643`) → 2 flushes, same as pre-fix. + +**No save regression on other paths.** `SaveChatOpacity` had exactly two callers +before this commit (`git grep SaveChatOpacity 6d0b0f92^` → `RetailUiRuntime.cs:2126,2131`, +the two setter lambdas); both are replaced by the single `FlushOpacity: +SaveChatOpacity` wiring (`RetailUiRuntime.cs:2136`). Nothing else called it. +The other opacity consumer, `RuntimeSettingsTargets.cs:223`, goes through +`RetailWindowOpacityController.SetOpacity` and owns its own persistence — never +touched `SaveChatOpacity` and is unaffected. + +**Page hide.** Covered, indirectly but correctly. `OnHidden() => Reset()` +(`OptionPageModel.cs:672`) restores every changed row through `apply`, which +flushes (not mid-drag). There is no state where a hide can strand an unflushed +value: the only value the disk lags is a mid-drag one, and a mid-drag hide +reverts live opacity to the saved value the disk already holds. See +SHOULD-FIX R1 below for what a mid-drag hide *does* break. + +### S2 — CLOSED, and the correction is honest + +- **AP-201** is filed and its text is accurate: it states the cull predates OP5 + ("Filed 2026-08-11 at the OP5 review-fix round (S2), predates OP5 but was made + user-visible by it"), quotes the cull expression verbatim, and names the + 260 px blocks as the reason it is newly visible. +- **#371** (`docs/ISSUES.md:27-47`) matches, cites + `src/AcDream.App/UI/UiScrollablePanel.cs:69`, states the root-cause fix + (a real per-row clip rect) and explicitly records that it was NOT attempted. +- The cited line is still exactly what the register quotes + (`UiScrollablePanel.cs:69`: `child.Visible = top >= -0.5f && top + child.Height <= Height + 0.5f;`), + and the class doc at `:8-12` still says "clips whole rows because the UI + renderer does not have a scissor stack yet". +- **Gate-script correction states the expected vanish-reappear honestly** + (`docs/research/2026-08-11-campaign-op-test-script.md`, step 2 and the + "report as a bug" list): "can vanish ENTIRELY at that exact offset rather than + showing a partially-clipped view. Keep scrolling a little further and the block + reappears whole." The old "any block whose height looks clipped" bullet was + rewritten rather than deleted — it still catches the genuine self-sizing + regression ("STILL missing rows, or leaves a large empty gap … once fully + scrolled into view"). Nothing was hidden from the gate. + +### S3 — the review-correction claim is TRUE; the new test is partly real + +`git grep ChatWindowMainFilter e71e5a96 -- tests/.../SettingsStoreTests.cs` +returns `:340` (default assertion) and `:347-352` (an explicit +`original`→`SaveChat`→`LoadChat`→`Assert.Equal` round trip with +`ChatWindowMainFilter = 0x1ul`). The round-trip test **did** exist at +`e71e5a96`; the review's S3 first half was wrong and the commit's correction is +verified. + +The replacement coverage (`tests/AcDream.App.Tests/UI/ChatMainWindowFilterSeedTests.cs`) +pins something real, but less than its own doc-comment implies — see NOTE R5. + +### N11 — CLOSED as asked; residual is inherent + +`ChatOptionsPageControllerTests.cs:770-793` now resolves both the ListBox and +the scrollbar through `UiElement.FindDescendant(chatSlot, …)` from the page slot +`0x1000050C`, matching `ConfigOptionsPageControllerTests.ScrollbarLinkage_ModelPointsAtTheConfigListBoxScroll` +(`:499-505`). That is exactly the shape N11 requested, and the production +lookup it mirrors is unchanged and correct +(`ChatOptionsPageController.cs:236-246`). The "asserts on the wrong object" +half of N11 is closed. The "would not catch a de-scoping regression" half is +only partly closed — see NOTE R4. + +--- + +## Blast radius of the NEW surface + +**`UiScrollbar.IsDragging` / `DragCompleted` — no effect on other consumers.** +`DragCompleted` defaults to `null` and has exactly two writers, both in +`ChatOptionsPageController.cs:388-389`; `IsDragging` is a read-only projection +of the existing `_draggingThumb` field (`UiScrollbar.cs:44`) with two +non-production readers besides the two apply closures (tests). The three +`MouseUp` bodies are otherwise byte-equivalent to before: both fields are still +cleared and the handler still returns `true`. Every other scrollbar in the +codebase (chat transcript, item lists, favorite-spell bar, combat power meter) +is behaviorally identical. + +**`Bindings` record growth (`FlushOpacity` inserted mid-list) — all sites +updated.** `ChatOptionsPageController.Bindings` is a positional record, but all +three construction sites use named arguments, so a positional shift is +impossible: +`src/AcDream.App/UI/RetailUiRuntime.cs:2129-2144` (production), +`tests/AcDream.App.Tests/UI/Layout/ChatOptionsPageControllerTests.cs:169-186` +(`FakeBindings.ToBindings`), and +`tests/AcDream.App.Tests/UI/Layout/ConfigOptionsPageControllerTests.cs:558-570` +(the OP6 test file's local Chat fake, updated in this commit). Grep for +`ChatOptionsPageController.Bindings(` finds no fourth site. + +**`RetailUiRuntime` save wiring — no path lost its save.** Covered under S1 +above: two pre-change callers, both accounted for; no other caller existed. + +--- + +## Residuals + +### SHOULD-FIX + +**R1 — a capture loss mid-drag strands `_draggingThumb = true`, and the new +discriminator turns that stale flag into a silent persistence failure.** + +`_draggingThumb` is cleared in exactly one place per path — the `MouseUp` case +(`UiScrollbar.cs:446`, `:503`, `:546`) — plus the `IsModelDisabled` early-out +(`:362-368`, unreachable for a scalar slider because `ScalarChanged` is +non-null). But `UiRoot` can drop pointer capture WITHOUT delivering a `MouseUp`: + +- `UiRoot.ClearSubtreeOwnership` → `ReleaseCapture()` (`UiRoot.cs:263-271`), + reached from `OnElementVisibilityChanging` (`UiRoot.cs:253-258`), which the + `UiElement.Visible` setter calls on every hide (`UiElement.cs:124-137`). + Reachable by keyboard while the button is held: `ToggleOptionsPanel()` → + `ToggleWindow(WindowNames.Options)` (`src/AcDream.App/Input/GameplayInputCommandController.cs:40-41`). +- A second `MouseDown` (right/middle button) on a different element re-targets + capture unconditionally (`UiRoot.cs:492`), so the subsequent left-up is + delivered to the new target and never reaches the slider. + +After either, `IsDragging` reads `true` forever for that slider, so +`if (!slider1.IsDragging) bindings.FlushOpacity()` (`ChatOptionsPageController.cs:366,379`) +never fires again for Reset/Defaults/discrete edits on that row: the value +applies live but is never persisted, with no log. Drags still flush (the next +real `MouseUp` sees `wasDragging == true`), which is what makes it silent. + +Pre-fix, the same stale flag was benign (a stuck `_draggingThumb` with no +capture receives no `MouseMove`); this commit is what gives it a consequence. +The seam for the root-cause fix already exists: `UiRoot.PointerCaptureChanged` +(`UiRoot.cs:213`, raised at `:790` and `:801`) — or `IsDragging` could be +defined as "dragging AND still the captured element". + +**R2 — the `DragCompleted` doc claim is false on the scalar path, and the +"never a drag" test does not cover the case that breaks it.** + +`UiScrollbar.cs:47-50` promises the callback fires "never on a `MouseUp` that +was not preceded by an actual drag (a bare click that only page-scrolled or +**jumped**…)". True for the two model paths (`_draggingThumb` is set only inside +the thumb-hit branch, `:412-417` and `:473-477`), but false for the scalar path +the opacity sliders use: `OnScalarEvent`'s `MouseDown` sets +`_draggingThumb = true` **unconditionally** at `UiScrollbar.cs:531`, after the +jump branch at `:527-530`. So a bare click on the opacity track fires +`DragCompleted`. + +Second-order: that same click produces **two** flushes, because +`ChangeScalarPosition` at `:529` runs while `_draggingThumb` is still `false` +(the flag is set on the next line), so the apply closure flushes inline, and +then the `MouseUp` flushes again. Harmless (idempotent whole-file write, 2 not +dozens), but it contradicts "exactly one per gesture" in the same doc block. + +`HorizontalScalar_DragCompleted_DoesNotFireOnAMouseUpThatWasNeverADrag` +(`UiScrollbarTests.cs:139-155`) only covers a `MouseUp` with no prior +`MouseDown` at all, so nothing catches this. Either correct the doc (and add a +track-jump-click test asserting the real behavior) or gate `_draggingThumb` on +the thumb-hit branch in `OnScalarEvent` the way the other two paths do. + +### NOTE + +**R3 — the M1 tests do not drive the real drag pipeline, so the assertion that +genuinely discriminates is the intermediate one, not the Reset one.** +`Reset_AfterOnlyDefaultSliderChanged_RevertsTheThumbToo` moves the value with +`defaultRow.SetCurrentValue(0.1f)` rather than through `UiScrollbar` events. On +pre-fix code `slider1.ScalarPosition` would have stayed at its 0.3 seed, so the +post-Reset assertion (`Assert.Equal(0.3f, slider1.ScalarPosition)`) would pass +by accident; the test fails pre-fix at the earlier +`Assert.Equal(0.1f, slider1.ScalarPosition, 3)` instead. It does discriminate, +just not at the line that reads like the point of the test. The S1 test +(`DraggingDefaultSlider_…`) does drive the real `UiScrollbar` event pipeline; +folding a Reset assertion onto the end of that one would pin the review's +literal scenario. + +**R4 — N11's "would not catch a de-scoping regression" is still open for the +Chat side, for the reason N11 itself identified.** `ImportedLayout`'s flat map +is last-write-wins and the Chat slot is built last, so +`layout.FindElement(0x10000201)` and the scoped lookup resolve the SAME +instance today. A Chat-side regression to a flat lookup therefore still passes +both the reworked test and OP6's +`SharedScrollbarId_ChatAndConfigBoundTogether_EachOwnsItsOwnScrollbar` +(`ConfigOptionsPageControllerTests.cs:508-554`) — that test discriminates the +**Config** side only (Config binds second; a flat Config lookup would clobber +Chat's `Model` and trip `Assert.Same(chatListBox.Scroll, chatScrollbar.Model)`). +The reworked assertion is still the right shape — it starts failing the moment +build order changes — but no test can fail on a Chat de-scoping while the +coincidence holds. Not fixable without changing build order; worth one line in +the test's own comment so a future reader does not over-trust it. + +**R5 — the new seed tests re-implement the production statement instead of +invoking it.** `ChatMainWindowFilterSeedTests.cs:47` and `:58` copy the two +lines from `RetailUiRuntime.MountChat` (`RetailUiRuntime.cs:947-949`) verbatim +into the test body, so neither test can fail if that seed is deleted or +re-pointed at the wrong window id — the thing S3 asked to pin. What they DO pin +that nothing else did: the cross-type default agreement +(`ChatSettings.Default.ChatWindowMainFilter` = `0xFBFFFFFF`, +`ChatSettings.cs:71`, equals `ChatWindowState.MainWindowDefaultFilter`), i.e. +the seed is a faithful no-op on a fresh install. Test 1 is largely redundant +with the pre-existing `SettingsStoreTests` round trip. The commit message is +honest about the limitation; the test's own doc comment ("pins the two-line +seed") reads stronger than what it does. + +**R6 — doc wording: "the link may have clamped it".** +`ChatOptionsPageController.cs:304-305` explains the post-link read as "not the +raw dragged value — the link may have clamped it". The link never clamps the +value being set — that is precisely AP-190 / `ChatOpacityLink`'s stated +invariant ("enforced by dragging the OTHER value, never by clamping the one +being set", `ChatOpacityLink.cs:4-6`); the only clamp is the [0,1] range clamp. +The comment as written invites a future reader to believe the opposite of the +invariant. "the link normalizes it to [0,1]" would be accurate. + +**R7 — the Chat tab's Apply button does not flush opacity.** `AfterApply` for +all three option pages publishes `SaveCharacterOptionsRuntimeCmd` +(`RetailUiRuntime.cs:2044`, `OptionsPanelController.cs:150-154`), which has +nothing to do with the local `settings.json` opacity blob. Harmless — Apply +changes no value, it only commits baselines, and every value-changing path +flushes — but it means the flush set really is exactly {drag end, discrete +edit, Reset, Defaults}, as the S1 doc says. Recorded so a future reader does +not assume Apply is a backstop. + +--- + +## Summary table + +| ID | Class | One-line | +|---|---|---| +| M1 | CLOSED | Both apply closures push their own slider from the post-link truth; drag-Default-only → Reset now reverts the thumb; never-clamp untouched | +| S1 | CLOSED | One flush per gesture across all three drag paths; Reset/Defaults/hide flush inline; live apply still per tick; no other save path affected | +| S2 | CLOSED | AP-201 + #371 accurately scope the cull as pre-existing/made-visible; gate script names the vanish-reappear honestly and keeps the real regression bullet | +| S3 | CLOSED | Round-trip test verified present at `e71e5a96` — the review was wrong; replacement pins the default agreement (see R5) | +| N11 | CLOSED | Scoped page-slot assertion landed, matching OP6's pattern (residual R4) | +| — | Blast radius | Clean: `DragCompleted` null-default with 2 writers, `MouseUp` otherwise unchanged; 3 `Bindings` sites all named-arg; 2 pre-change `SaveChatOpacity` callers both accounted for | +| R1 | SHOULD-FIX | Capture loss without `MouseUp` strands `_draggingThumb`; stale `IsDragging` silently disables the Reset/Defaults flush for the session | +| R2 | SHOULD-FIX | `DragCompleted` doc contradicts `OnScalarEvent` (a bare track-jump click IS a "drag"); that click double-flushes; untested | +| R3 | NOTE | M1 tests bypass the real drag pipeline; the discriminating assertion is the intermediate one | +| R4 | NOTE | Chat-side de-scoping still undetectable (flat winner == Chat instance); only the Config side is discriminated | +| R5 | NOTE | Seed tests copy the production statement rather than invoking it; what they really pin is the default agreement | +| R6 | NOTE | "the link may have clamped it" contradicts the never-clamp invariant it cites | +| R7 | NOTE | Chat-tab Apply publishes character options only — not an opacity flush backstop |