docs: OP4 re-review — REOPEN (narrow): timestamp seam regressed, 9/10 closed
MF-1/M1 CLOSED with the blast scenario walked (Reset now restores live
values; UiLocked converges on every seed). MF-2 CLOSED with a
load-bearing fixture check (all nine verb buttons author Ghosted and
lack DAT 0x0B — TrySetRetailState would otherwise short-circuit). M2
store retirement verified to zero remaining references; M3 walked; S2's
.spv hygiene verified by recomputing the compiler hash scheme.
R1 MUST-FIX: the timestamp now prefixes ChatEntry.Text (the BODY), so
six of ten chat kinds render 'Alice says, "13:05:09 hi"' — retail
composes the line FIRST and prepends the timestamp as a separate string
at display time (AddTextToScroll @0x00563c50 receives composed lines;
fprintf("%ls%ls" ts, text) @0x00563e5b). Fix at ChatVM's display
composition; the two new tests pin the defect and must be rewritten.
R2: the seed-event re-read for OPEN panels (Combat vs Character can
disagree until re-shown; a panel open across reconnect reopens the
wrong-direction Reset for that window) — wire the existing
OnCharacterOptionsChanged hook like LockUI or register it. R3: no test
drives the refresh widget push. R4: headless AutoRepeatAttack default
flip false->true unnamed in AP-196.
Coordinator fixes directly (second round) once OP5 frees the tree.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
bc43fb1d1d
commit
4798302226
1 changed files with 425 additions and 0 deletions
425
docs/research/2026-08-11-op4-rereview.md
Normal file
425
docs/research/2026-08-11-op4-rereview.md
Normal file
|
|
@ -0,0 +1,425 @@
|
|||
# Campaign OP slice OP4 — fix-round RE-REVIEW
|
||||
|
||||
**Commit under review:** `bc43fb1d` "fix(ui,runtime): OP4 review fixes — live
|
||||
re-seed, enable-gating, Combat panel re-point, universal timestamps"
|
||||
**Claim under test:** closure of every consolidated finding from
|
||||
`docs/research/2026-08-11-op4-review-mechanism.md` (MF-1, MF-2, SF-1, SF-2,
|
||||
SF-3, N-1..N-5) and `docs/research/2026-08-11-op4-review-blast.md`
|
||||
(M1, M2, M3, S1–S6, N1–N8).
|
||||
**Mode:** read-only. No build, no test run, no client launch. The only file
|
||||
written is this one.
|
||||
|
||||
---
|
||||
|
||||
## Verdict
|
||||
|
||||
**REOPEN — narrow.** Nine of the ten consolidated findings are genuinely
|
||||
closed, several of them better than the reviews asked for. **One fix
|
||||
(S1/SHOULD-FIX, universal chat timestamps) introduced a NEW defect that is
|
||||
worse than the gap it closed**: the prefix is applied to
|
||||
`ChatEntry.Text` — the raw message body — instead of to the composed display
|
||||
line, so every sender-formatted chat kind now renders the timestamp *inside
|
||||
the quoted speech*:
|
||||
|
||||
```
|
||||
Alice says, "13:05:09 hello" ← acdream after bc43fb1d
|
||||
13:05:09 Alice says, "hello" ← retail
|
||||
```
|
||||
|
||||
Six of the ten `ChatKind`s are affected (LocalSpeech, RangedSpeech, Channel,
|
||||
Tell, Emote, SoulEmote); the four that happen to be correct (System, Combat,
|
||||
Popup, and the AddText callers) are exactly the ones that were already
|
||||
covered before this commit. **The new tests pin the wrong invariant**, so the
|
||||
suite is green on a broken line format. Gate-script step 13 will fail on the
|
||||
"inbound tell/say" half it was written for.
|
||||
|
||||
This is one localized fix (move the prefix to the composed-line seam) plus a
|
||||
test rewrite. Nothing else in the round needs to change.
|
||||
|
||||
| Class | Count | Items |
|
||||
|---|---|---|
|
||||
| MUST-FIX | 1 | R1 — timestamp prefixes the message body, not the composed line |
|
||||
| SHOULD-FIX | 3 | R2 — N-5 (server-echo refresh) neither wired nor registered, and this commit widened its blast; R3 — the `refresh` widget push has no test; R4 — headless `AutoRepeatAttack` default flips `false → true`, unnamed in AP-196 |
|
||||
| NOTE | 3 | R5 — `refresh`-from-`SaveCurrentValue` is an adaptation, not retail's shape (comment reads otherwise); R6 — enable-gate tests cover the Character page only; R7 — `HeadlessGameplayOperations` combat options can now throw where they were constants |
|
||||
|
||||
---
|
||||
|
||||
## Per-finding closure check
|
||||
|
||||
### MF-1 / blast M1 — stale pre-login row seed — **CLOSED**
|
||||
|
||||
Traced end to end.
|
||||
|
||||
**The re-seed mechanism.** `BoolOptionRow` gained `read`/`refresh` delegates
|
||||
(`src/AcDream.App/UI/Layout/OptionPageModel.cs:86-99`) and
|
||||
`SaveCurrentValue` is now
|
||||
(`src/AcDream.App/UI/Layout/OptionPageModel.cs:148-156`):
|
||||
|
||||
```csharp
|
||||
if (_read is not null) { _current = _read(); _refresh?.Invoke(_current); }
|
||||
_saved = _current;
|
||||
```
|
||||
|
||||
Retail's `UIOption_Checkbox::SaveCurrentValue @0x004868e0` is verbatim
|
||||
`m_current = vtable->GetValue(); m_saved = m_current;` (pseudo-C `:146936`),
|
||||
and `UIOption_Checkbox::GetValue @0x00486f60` (`:147401`) resolves to
|
||||
`PlayerModule::GetOption(m_playerOption)` at `00486ff8` — the live
|
||||
server-synced word, exactly as the mechanism review claimed. The wiring
|
||||
supplies the live reader and a widget push that deliberately bypasses `apply`
|
||||
(`CharacterOptionsPageController.cs:383-385`: `read: () =>
|
||||
bindings.CurrentValue(spec.Id)`, `refresh: value => checkbox.Selected =
|
||||
value`), so a re-seed cannot echo back onto the wire. `UiButton.Selected`'s
|
||||
setter only calls `UpdateVisualState()` (`UiButton.cs:162-171`) — no `Click`,
|
||||
no `OnClick` — so the push is inert on the wire by construction, not just by
|
||||
convention.
|
||||
|
||||
**The show path.** `RetailWindowFrame.Mount(... Visible = false ...)`
|
||||
(`RetailUiRuntime.cs:2064`) means no `OnShown` fires at composition, so the
|
||||
pre-login mount no longer matters. Opening the panel sets the frame visible →
|
||||
`UiRoot.ElementVisibilityChanged` → `RetailWindowManager.OnElementVisibility
|
||||
Changed` → `handle.NotifyVisibility(true)` (`RetailWindowManager.cs:276`) →
|
||||
`Controller.OnShown()` (`RetailWindowHandle.cs:101`) →
|
||||
`OptionsPanelController.OnShown()` (`:307-311`) → active `OptionPage.OnShown()`
|
||||
→ `Apply()` → `SaveCurrentValue()` on all 50 rows. Tab-switch-in takes the
|
||||
same path through `OnActivePageChanged` (`:283-295`). This matches retail's
|
||||
`PlayerOptionPage::OnVisibilityChanged(true) → SaveCurrentValues` (pseudo-C
|
||||
`:245374`, `:245757`).
|
||||
|
||||
**The exact blast-M1 scenario now behaves correctly.** Server ON, client
|
||||
constructor default OFF, panel shown, row clicked (now a real toggle to OFF,
|
||||
not a silent no-op), tab switched away → `OnHidden → Reset` → the row is
|
||||
`Changed` → `RestoreSavedValue()` restores `_saved`, which the show re-seeded
|
||||
to **true**, and `apply(true)` re-asserts the server's own value. The
|
||||
wrong-direction mutation is gone. Pinned by
|
||||
`Reset_AfterReseed_RestoresTheLiveValue_NotTheStaleConstructionDefault`
|
||||
(`tests/AcDream.App.Tests/UI/Layout/CharacterOptionsPageControllerTests.cs`),
|
||||
plus a 50-row convergence test and a "no `SetOption` during re-read" assertion.
|
||||
|
||||
**The read closure stays valid across sessions.** `CurrentCharacterOption` is
|
||||
`id => d.Character.Options.GetOptionBit(id)`
|
||||
(`InteractionRetainedUiComposition.cs:860`); `RuntimeCharacterState.Options` is
|
||||
a get-only property assigned once (`RuntimeCharacterState.cs:105,118`), and
|
||||
reconnect calls `Options.ResetSession` on that same instance (`:442`) rather
|
||||
than replacing it — so the retained UI's captured delegate never goes stale.
|
||||
|
||||
**LockUI convergence.** `OnCharacterOptionsChanged` now also pushes the live
|
||||
bit through `SetUiLocked` (`LiveSessionRuntimeFactory.cs:336-341`). It fires
|
||||
from `LiveSessionEventRouter.cs:210-226` on **every complete
|
||||
PlayerDescription** — login and reconnect — and deliberately not on a
|
||||
trailer-truncated parse (OP7 MF-2, correct). Threading is safe: the router runs
|
||||
under `WorldSession.Tick()`, the per-frame pump (`WorldSession.cs:1117-1123`),
|
||||
i.e. the same thread the retained UI runs on, so the new
|
||||
`UiRoot.UiLocked` write and its `UiLockChanged` fan-out
|
||||
(`RetailWindowManager.cs:316-320`, which enumerates the window dictionary) are
|
||||
not a cross-thread hazard.
|
||||
|
||||
### MF-2 — Apply/Reset enable-gating — **CLOSED**
|
||||
|
||||
`OptionsPanelController.Bind` now captures both buttons and installs the gate
|
||||
per page (`:220-243`), running it once at bind time for retail's `PostInit →
|
||||
OnOptionChanged(0)` tail; Defaults is wired but never gated. Verified against
|
||||
the committed fixture that this actually works on all three pages: each of the
|
||||
nine `0x100001FC/FD/FE` instances authors a **`Ghosted` state** and **does not**
|
||||
carry DAT property `0x0B` (`ToggleBehavior`). That second fact is load-bearing
|
||||
and easy to miss — `UiButton.TrySetRetailState` short-circuits `Normal`/
|
||||
`Highlight` into a `Selected` flip for toggle buttons *without re-enabling*
|
||||
(`UiButton.cs:212-216`), which would have left Apply/Reset permanently ghosted
|
||||
after the first Apply. They are push buttons, so the `Ghosted → Enabled=false`
|
||||
/ `!Enabled → Enabled=true` path (`:217-223`) is the one taken.
|
||||
`UiElement.Pick` skips `!Enabled` subtrees (`UiElement.cs:586`) and
|
||||
`UiButton.OnEvent`'s `Click` case returns early when `!Enabled` (`:494`), so a
|
||||
ghosted button is genuinely unclickable. Five tests pin start-disabled,
|
||||
click-enables, Apply-disables, Defaults-leaves-enabled, Defaults-never-gated.
|
||||
|
||||
### M2 — two writable copies (Combat panel) — **CLOSED**
|
||||
|
||||
The retirement is complete, not gestured. `grep` across `src/` and `tests/`
|
||||
finds **zero** readers or writers of `GameplaySettings.{AutoTarget,
|
||||
AutoRepeatAttack, ViewCombatTarget}`, `RuntimeSettingsController.{AutoTarget,
|
||||
AutoRepeatAttack, ViewCombatTarget, SetCombatGameplay}`, or
|
||||
`GameplaySettingsState` — all deleted, including the settings.json read/write
|
||||
keys (`SettingsStore.cs`) and the dead ImGui `SettingsPanel` checkboxes. The
|
||||
`ICombatGameplaySettingsSource` interface survives with
|
||||
`CharacterOptionCombatSettingsSource` as its only implementation.
|
||||
|
||||
Convergence trace: the Combat LED click →
|
||||
`CombatUiController.Bindings.SetOption` (`CombatUiController.cs:127-135`) →
|
||||
`SetSingleCharacterOptionRuntimeCmd` (`RetailUiRuntime.cs:1203-1206`) →
|
||||
`LiveSessionCommandRouter` → `SendSingleCharacterOption` →
|
||||
`RuntimeCharacterOptionsState.TrySetOption` (`RuntimeCharacterState.cs:806`).
|
||||
The Character tab's row publishes the *identical* command. Reads are
|
||||
`GetOptionBit` on both sides (`CombatUiController.SyncControls` `:209-211`;
|
||||
`BoolOptionRow._read`). One store, one writer path — the divergence is closed.
|
||||
They converge on the next refresh of whichever surface was not clicked
|
||||
(`SyncControls` runs on `OnShown`/combat-mode/attack-state changes), which the
|
||||
gate script's new step 20 states honestly ("toggling one and
|
||||
reopening/refreshing the other"). See **R2** for the residual.
|
||||
|
||||
`HeadlessGameplayOperations` now reads the live bits
|
||||
(`src/AcDream.Headless/Hosting/HeadlessGameplayOperations.cs:135-138`),
|
||||
closing blast S5. AP-196 (`docs/architecture/retail-divergence-register.md:189`)
|
||||
records the default-source change, the `ViewCombatTarget true→false` flip, the
|
||||
three-field deletion, and the "other five stay as write-behind mirrors"
|
||||
distinction truthfully; the AP header count moved 136 → 138 (`:178`).
|
||||
|
||||
### M3 — `SetUiLocked` convergence guard — **CLOSED**
|
||||
|
||||
`_lastAppliedUiLocked` (`RuntimeSettingsController.cs:188`) is a `bool?`
|
||||
starting `null`, compared instead of `Gameplay.LockUI` (`:361`) and written
|
||||
only after the runtime target actually accepted the push (`:366-367`), with the
|
||||
same bookkeeping added to `SaveGameplay`'s own push (`:525-526`).
|
||||
|
||||
Walking the blast-M3 scenario — persisted `"lockUI": true`, fresh login, one
|
||||
toggle press: login's `OnCharacterOptionsChanged` calls `SetUiLocked(false)`
|
||||
(server bit clear); `null != false` → applies, `_lastAppliedUiLocked = false`.
|
||||
The toggle then computes `locked = !GetOptionBit(LockUI) = true`
|
||||
(`LiveSessionRuntimeFactory.cs:413-418`); `false != true` → **`ApplyUiLock`
|
||||
runs**. The old guard would have early-returned on the first press. Pinned by
|
||||
`SetUiLocked_AppliesEvenWhenGameplayLockUIAlreadyMatches_IfNeverActuallyApplied`,
|
||||
which also asserts the second identical call correctly no-ops.
|
||||
|
||||
### S1 — DisplayTimeStamps coverage — **NOT CLOSED (regressed): see R1**
|
||||
|
||||
Coverage *is* fixed — `ChatLog.Append` is a true chokepoint (one
|
||||
`_buffer.Enqueue`, ten producers, all funnelling through it), and the three
|
||||
bypass paths spot-checked (heard speech via `OnLocalSpeech`, Turbine channels
|
||||
via `OnChannelBroadcast`, combat text via `OnCombatLine`) now all get a prefix.
|
||||
The SpewBox stays exempt for free (`RuntimeCommunicationState.AddText:228-232`
|
||||
returns before touching `Chat`), and moving the prefix behind
|
||||
`OnSystemMessage`'s dedupe comparison actually *retires* blast N5's dedupe
|
||||
concern. But the placement is wrong — full detail in **R1**.
|
||||
|
||||
### S2 — sky fog gate — **CLOSED**
|
||||
|
||||
`sky.frag:68-69` now reads `int fogMode = int(uFogParams.w); if (uApplyFog >
|
||||
0.5 && fogMode != 0)`, matching `mesh_modern.frag`/`terrain_modern.frag`'s
|
||||
`mode == 0` convention; `uFogParams` is already in the frag shader's UBO
|
||||
(`sky.frag:50`).
|
||||
|
||||
**.spv hygiene claim verified.** `git show --stat` for
|
||||
`src/AcDream.App/Rendering/` lists exactly `sky.frag`, `sky.frag.spv`, one
|
||||
manifest line, and `GameWindow.cs`'s comment edit — no collateral shader
|
||||
churn. And the manifest hash is genuinely correct: recomputing the compiler's
|
||||
own scheme (`tools/ShaderCompiler/Program.cs:214-220` — UTF-8 of the source
|
||||
with `\r\n → \n`) over `sky.frag` yields
|
||||
`2ddf210d69b0c4a3c0870eecfb0ccba2097d93b365729739402bfe91e3b120d0`, exactly
|
||||
the new manifest value (cross-checked against `mesh_modern.frag`'s untouched
|
||||
entry to confirm the scheme).
|
||||
|
||||
### S4 / SF-1 — culture-invariant format, doc downgrade, AP-197 — **CLOSED**
|
||||
|
||||
`FormatTimestampPrefix` (`ChatLog.cs:468-469`) is
|
||||
`DateTime.Now.ToString(@"H\:mm\:ss ", CultureInfo.InvariantCulture)` — colons
|
||||
escaped so they are literals rather than `DateTimeFormatInfo.TimeSeparator`,
|
||||
matching the CRT `wcsftime(u"%#H:%M:%S ")` retail actually calls
|
||||
(pseudo-C `00563e24`). A `fi-FI` test exists at both layers. The
|
||||
"byte-verified" overclaim is downgraded to "BN-sourced … NOT byte-verified —
|
||||
wire doc U6" in both doc comments, and AP-197
|
||||
(`retail-divergence-register.md:190`) records the ignored per-character
|
||||
`GenericQualitiesData` key-1 format override with an accurate risk column.
|
||||
N-4's AP-194 anchor-column typo is fixed (`:186`, `UIOption_Checkbox::
|
||||
SetPlayerOption @0x00486e80`, with the correction annotated).
|
||||
N-7's `RunAsDefaultMovement` prose now cites `ToggleRun_PlayerOption` at all
|
||||
three sites.
|
||||
|
||||
### SF-2 / SF-3 / S6 — tests and gate script — **CLOSED** (with R3, R6)
|
||||
|
||||
- Six header keys hash-pinned by `[Theory]`, each also asserted present in
|
||||
`Groups` (so a rename can't quietly orphan the pin).
|
||||
- A real `MouseDown/MouseUp/Click` sequence on the built `0x10000219`
|
||||
checkbox, asserting both `Selected` and the published `SetOption` — this
|
||||
pins the `ToggleBehavior` dependency that all 50 LEDs rest on.
|
||||
- The `option × modifier` truth table (4 rows) plus `AutoRunActive` with the
|
||||
option OFF, the latter honestly labelled a current-behavior pin rather than
|
||||
a retail-correctness claim (N6 stays open as a question, correctly).
|
||||
- Gate script step 8 now states the logout-flush mechanism and both outcomes;
|
||||
new steps 19 (enable-gating) and 20 (Combat/Character cross-check) exist and
|
||||
are worded to match what the code actually does.
|
||||
|
||||
Not verifiable read-only: the commit's "13,044 passed / 4 skipped" claim.
|
||||
|
||||
---
|
||||
|
||||
## Residuals
|
||||
|
||||
### R1 — MUST-FIX — the timestamp prefixes the message body, not the composed line
|
||||
|
||||
`ChatLog.Append` (`src/AcDream.Core/Chat/ChatLog.cs:436-444`) rewrites
|
||||
`entry.Text`:
|
||||
|
||||
```csharp
|
||||
if (DisplayTimestampsSource?.Invoke() == true)
|
||||
entry = entry with { Text = FormatTimestampPrefix() + entry.Text };
|
||||
```
|
||||
|
||||
But `ChatEntry.Text` is the *message body*, not the display line. The display
|
||||
line is composed later, from `Sender` + `Text`, in
|
||||
`ChatVM.FormatEntry` (`src/AcDream.UI.Abstractions/Panels/Chat/ChatVM.cs:251-300`)
|
||||
— the same method both retained chat windows render through
|
||||
(`ChatWindowController.cs:681` and `FloatingChatWindowController.cs:272`, via
|
||||
`RecentLinesDetailed()` at `ChatVM.cs:334-351`). Result, per kind:
|
||||
|
||||
| Kind | Rendered after `bc43fb1d` | Retail |
|
||||
|---|---|---|
|
||||
| LocalSpeech / RangedSpeech | `Alice says, "13:05:09 hi"` | `13:05:09 Alice says, "hi"` |
|
||||
| Channel | `[Trade] Bob says, "13:05:09 wts"` | `13:05:09 [Trade] Bob says, "wts"` |
|
||||
| Tell | `Regal tells you, "13:05:09 psst"` | `13:05:09 Regal tells you, "psst"` |
|
||||
| Emote / SoulEmote | `* Caith 13:05:09 waves` | `13:05:09 * Caith waves` |
|
||||
| System / Combat / Popup | correct (Text *is* the line) | correct |
|
||||
|
||||
**Retail is unambiguous here, in two independent places.**
|
||||
`ClientSystem::AddTextToScroll @0x00563c50` (pseudo-C `:368347`) receives an
|
||||
already-composed line in `arg2`, formats the timestamp into a **separate**
|
||||
string (`00563e24 wcsftime(... u"%#H:%M:%S " ...)` → `s_NullBuffer_4`, which is
|
||||
initialized to the empty buffer at `00563dcd`), and passes the two as distinct
|
||||
`StringInfo`s to `ECM_UI::SendNotice_DisplayFinalStringInfo(arg3, &text,
|
||||
×tamp, arg5)` at `00563f2b`; the log-file branch writes
|
||||
`fprintf(..., "%ls%ls\n", timestamp, text)` at `00563e5b` — timestamp first,
|
||||
whole line second. And the caller side confirms the line is composed *before*
|
||||
the call: `ClientCommunicationSystem::Handle_Communication__HearSpeech`
|
||||
builds `sprintf("%s %s\n", speakerPhrase, text)` at `00571533` and only then
|
||||
calls `AddTextToScroll` at `0057154d` (pseudo-C `:382204`ff).
|
||||
|
||||
**The new tests encode the defect rather than catching it.**
|
||||
`ChatLogTests.DisplayTimestampsSource_AppliesToEveryProducer` (`:381`) and
|
||||
`RuntimeCommunicationStateTests.DisplayTimestampsSource_ForwardsToChat_
|
||||
TimestampingEveryProducer_NotJustAddText` (`:314`) both assert
|
||||
`^\d{1,2}:\d{2}:\d{2} hi$` on the **raw `ChatEntry.Text`** of a `LocalSpeech`
|
||||
entry — the exact string that will render inside the quotes. No test asserts a
|
||||
rendered line, so the suite cannot go red on this.
|
||||
|
||||
**Consequence for the gate.** Script step 13 asks the user to "send/receive a
|
||||
new chat line (e.g. `/who` or any inbound tell/say)" and expects a prefix. The
|
||||
`/who` half (System) passes; the tell/say half shows the timestamp inside the
|
||||
quotation marks. This burns a gate round on a known-wrong line format.
|
||||
|
||||
**Fix shape.** Prefix at the composed-line seam, not the entry: apply it in
|
||||
`ChatVM.FormatEntry`/`RecentLinesDetailed` (retail's own model is a separate
|
||||
leading field, so a render-time prefix is the faithful analog and also keeps
|
||||
the stored transcript clean for the runtime event stream). Retarget the tests
|
||||
onto `FormatEntry` output for at least one sender-composed kind per shape
|
||||
(speech, tell, channel, emote) plus one bare kind (System), and keep the
|
||||
culture test at whichever layer owns the formatting.
|
||||
|
||||
### R2 — SHOULD-FIX — N-5 (no refresh on the server's own option echo) is still neither wired nor registered, and this commit widened its blast radius
|
||||
|
||||
Mechanism review N-5 said: wire `OnCharacterOptionsChanged` to refresh the
|
||||
visible page, **or** file a register row. Neither happened. `grep` finds no
|
||||
register row mentioning `SendNotice_RefreshOptionsPanel` /
|
||||
`SendNotice_PlayerOptionChanged` / a deferred options refresh.
|
||||
|
||||
This is no longer purely cosmetic after `bc43fb1d`, because the commit added a
|
||||
second surface reading the same store:
|
||||
|
||||
1. **Cross-panel disagreement.** With the Combat window open, clicking
|
||||
"Automatically Repeat Attacks" on the Character tab changes the bit but the
|
||||
Combat LED keeps its old art until the next `SyncControls`
|
||||
(`CombatUiController.cs:228` `OnShown`, combat-mode change, or attack-state
|
||||
change) — and vice versa. Both now read one store, so this is a stale
|
||||
*repaint*, not a stale *value*; it is nonetheless a new user-visible
|
||||
inconsistency introduced by this commit.
|
||||
2. **Panel open across the seed.** MF-1 re-seeds on show/tab-switch only, not
|
||||
on the `PlayerDescription` seed itself — the very hook this commit wired for
|
||||
`LockUI` (`LiveSessionRuntimeFactory.cs:336-341`). A Character tab that is
|
||||
already open when the seed lands keeps constructor-default rows *and*
|
||||
constructor-default `_saved` baselines, which reopens the M1 "Reset writes
|
||||
the wrong direction" failure for that window. Same for a reconnect
|
||||
(`Options.ResetSession` → fresh `PlayerDescription`) with the panel left
|
||||
open.
|
||||
|
||||
Cheapest closure: have `OnCharacterOptionsChanged` also call
|
||||
`OptionsPanelController.OnShown()`-equivalent re-seed on the visible page (the
|
||||
hook is already wired and already proven safe on this thread), and call
|
||||
`CombatUiController.SyncControls()` from the same place. Otherwise, file the
|
||||
register row N-5 asked for and say which slice owns it.
|
||||
|
||||
### R3 — SHOULD-FIX — the `refresh` widget push has no test
|
||||
|
||||
MF-1's stated fix instruction was to assert "the row **and the checkbox** both
|
||||
converge on the next `OnShown()`". The three new re-seed tests assert
|
||||
`row.Current` / `row.Saved` only; no test reads `checkbox.Selected` after a
|
||||
re-seed (the only `checkbox.Selected` assertions in the file are in the new
|
||||
click test, `CharacterOptionsPageControllerTests.cs:630,636`). The `refresh`
|
||||
delegate — the half that makes the LED art match the re-read value, and the
|
||||
half a future refactor is most likely to drop — is production-only. One
|
||||
assertion on the first row's checkbox inside
|
||||
`OnShown_ReSeedsRow_FromLiveBindingValue_ChangedBehindItsBack` closes it.
|
||||
|
||||
### R4 — SHOULD-FIX — headless `AutoRepeatAttack`'s observable default flips `false → true`, unnamed in AP-196
|
||||
|
||||
`HeadlessGameplayOperations.AutoRepeatAttack` was the literal `false`; it now
|
||||
reads the live bit (`:135-136`), whose pre-seed value is the retail
|
||||
constructor default **ON** (`CharacterOptionTable.cs:111`,
|
||||
`Add(AutoRepeatAttack, true, 0x00000002u, true, true)` — auto-save, client
|
||||
default `true`). Every existing headless bot that never configured
|
||||
`characterOptions` therefore starts auto-repeating attacks where it previously
|
||||
did not. That is retail-correct and desirable, but it is an observable-default
|
||||
change of exactly the class AP-196 was filed to record, and AP-196 names only
|
||||
`ViewCombatTarget`'s flip. Add the sentence (or a Slice-K-facing note) so a
|
||||
future "why is my bot chain-attacking?" report lands on the row instead of on
|
||||
an investigation.
|
||||
|
||||
### R5 — NOTE — `refresh`-from-`SaveCurrentValue` is an acdream adaptation, and the doc comment reads as if it were retail's shape
|
||||
|
||||
Retail's `SaveCurrentValue @0x004868e0` does **not** call `Refresh()` — only
|
||||
`RestoreSavedValue @0x00486900` and `RestoreDefaultValue` do (pseudo-C
|
||||
`:146936`, `:146950`). Retail does not need it: its options word is seeded
|
||||
before the panel's rows exist, so `m_current` can never drift from the art.
|
||||
acdream's re-read *can* change `_current` with no other write, so the push is
|
||||
required — a justified adaptation. The `refresh` param doc
|
||||
(`OptionPageModel.cs:79-85`) opens "Retail's own `Refresh()` push of the
|
||||
re-read value onto the widget … invoked ONLY from `SaveCurrentValue`'s re-read
|
||||
path", which a future reader grepping `0x004868E0` will find contradicted by
|
||||
the decomp. One clause ("acdream-only — retail's `SaveCurrentValue` needs no
|
||||
push because its rows are built after the seed") removes the trap.
|
||||
|
||||
### R6 — NOTE — the enable-gate tests cover the Character page only
|
||||
|
||||
`GetCharacterPageButtons` scopes to `0x10000211`; the Chat (`0x1000050C`) and
|
||||
Config (`0x10000213`) pages get the same gate installed by the same loop but
|
||||
are unpinned. The fixture confirms all nine buttons have the required shape
|
||||
(no `0x0B`, `Ghosted` present), so this is coverage, not a defect — but a
|
||||
one-line `[Theory]` over the three page ids would also pin the per-page
|
||||
scoping the slice's own commit message calls out.
|
||||
|
||||
### R7 — NOTE — `HeadlessGameplayOperations`'s combat options can now throw
|
||||
|
||||
`AutoRepeatAttack`/`AutoTarget` call `RequireRuntime()`, which throws
|
||||
`InvalidOperationException` when unbound (`:272-275`); they were constants and
|
||||
could never throw. `_runtime` is assigned once in `Bind` and never cleared
|
||||
(`:46,65`), and both properties are only read from runtime-attached combat
|
||||
flows, so this is not reachable today. Recorded so a future teardown-ordering
|
||||
change doesn't rediscover it as a crash.
|
||||
|
||||
---
|
||||
|
||||
## Blast radius of the fixes themselves (checked, clean except R1/R2/R4)
|
||||
|
||||
- **`ChatLog.DisplayTimestampsSource`** is a new public property on a Core
|
||||
type; the only binder is `GameWindow.cs:662` (graphical host). Headless never
|
||||
binds it — `HeadlessGameplayOperations`'s only chat write
|
||||
(`:252 CommunicationOwner.Chat.OnSystemMessage`) goes through the same
|
||||
`Append` but with the source null, so bot event streams and the 67 Headless
|
||||
tests are untouched. No plugin-abstraction surface references `ChatLog` or
|
||||
`ChatEntry`. `RuntimeCommunicationState.DisplayTimestampsSource` became a
|
||||
forwarding property, so every existing caller compiles unchanged.
|
||||
- **`CombatUiController.Bindings` rewiring** — exactly one production
|
||||
construction site (`RetailUiRuntime.cs:1199-1206`) and one
|
||||
`CombatRuntimeBindings` site (`InteractionRetainedUiComposition.cs:644-646`),
|
||||
both updated; `OptionsRuntimeBindings` likewise has a single construction
|
||||
site, so `MountCombat`'s new `_bindings.Options` dependency cannot be null.
|
||||
Tests construct through a fake with the same shape.
|
||||
- **Deleted `RuntimeSettingsController` members** — zero remaining references
|
||||
anywhere in `src/` or `tests/`. `GameplaySettings`'s three positional
|
||||
parameters were removed, which is a compile-time forcing function (AP-196
|
||||
says so explicitly); stale `autoTarget`/`autoRepeatAttack`/`viewCombatTarget`
|
||||
keys in an existing `settings.json` are simply ignored on read and dropped on
|
||||
write.
|
||||
- **`SetUiLocked` on every seed** now persists a server-derived `lockUI` into
|
||||
`settings.json` and updates the Settings VM draft from the session route.
|
||||
That is one save per session start (the guard no-ops afterwards), on the same
|
||||
thread as the existing `SyncChatFromServerOptions` call beside it, and it
|
||||
makes the persisted value track the server — consistent with AP-196's
|
||||
"write-behind mirror" framing. No re-entrancy: `ApplyUiLock` terminates at
|
||||
`UiRoot.UiLocked = locked` (`RuntimeSettingsTargets.cs:192`) and never
|
||||
publishes a wire command.
|
||||
Loading…
Add table
Add a link
Reference in a new issue