docs: OP9 review — APPROVE-WITH-FIXES
Combined mechanism-faithfulness x regression/blast-radius pass over
371197a3 (OP9 retirement of the dead F11 settings surface +
GameplaySettings), per the OP5/OP7 single-reviewer precedent for
closeout-shaped slices.
Retirement verified correct: zero production readers of GameplaySettings
or SettingsVM existed pre-commit (checked against the pre-commit tree,
not the diff), SetUiLocked first-call/repeat-call behavior is provably
unchanged by deleting _uiLockConverged, the wire paths
(SetAcceptLootPermits 0x0005, ToggleUiLock, PlayerDescription
convergence) are untouched, the settings.json unknown-key carry-forward
is real, and the AP-196 register edit reconciles (143 -> 142 active,
29 -> 30 retired, total unchanged).
MUST-FIX 1: SaveAudio -> ApplyAudio (OP6 live-apply, live consumer in
ConfigOptionsPageController) lost its only assertion when
SettingsViewModelSavePreservesSectionAndTargetOrder was deleted.
SHOULD-FIX 2-5: stale architecture-doc seam naming SettingsVM; three
dead residues (uncallable private SaveCharacter, orphaned
IngressShutdownRoots.Settings, writerless CharacterSettings path);
test delta enumerates to -84, not the claimed -80 "exactly"; dangling
comment referencing the deleted assertion. Two nits.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
371197a345
commit
289bf5bc6e
1 changed files with 391 additions and 0 deletions
391
docs/research/2026-08-11-op9-review.md
Normal file
391
docs/research/2026-08-11-op9-review.md
Normal file
|
|
@ -0,0 +1,391 @@
|
|||
# Campaign OP slice OP9 — combined-lens review (mechanism × blast radius)
|
||||
|
||||
**Target:** `371197a3` — *refactor(settings): OP9 — retire the dead F11
|
||||
settings surface + fully-superseded GameplaySettings*
|
||||
**Reviewer:** combined mechanism-faithfulness × regression/blast-radius pass
|
||||
(one reviewer, OP5/OP7 precedent for closeout-shaped slices).
|
||||
**Base:** `1a57f96e` (campaign tip after the OP8 merge). Worktree clean at
|
||||
review time; HEAD == the target commit.
|
||||
|
||||
## VERDICT: APPROVE-WITH-FIXES
|
||||
|
||||
The retirement itself is correct. Every claim in the commit message's
|
||||
per-file disposition list that bears on **live behavior** verifies against
|
||||
the post-commit source: no production code path read `GameplaySettings`, no
|
||||
production code path constructed `SettingsVM`, `SetUiLocked`'s observable
|
||||
contract is unchanged, the wire behavior of `SetAcceptLootPermits` /
|
||||
`ToggleUiLock` / the PlayerDescription convergence is untouched, and the
|
||||
settings.json unknown-key carry-forward mechanism is real. The register
|
||||
edit is arithmetically and semantically correct.
|
||||
|
||||
Five findings, none of them a behavior regression: one lost regression
|
||||
guard on a live mechanism (MUST-FIX 1), one stale **architecture** doc
|
||||
seam (SF-2), three dead residues the retirement created and left (SF-3),
|
||||
a test-delta claim that does not reconcile (SF-4), and a dangling
|
||||
comment (SF-5). Plus two nits.
|
||||
|
||||
---
|
||||
|
||||
## MUST-FIX 1 — `SaveAudio` → `ApplyAudio` (OP6's live-apply) lost its only test
|
||||
|
||||
`RuntimeSettingsController.SaveAudio` pushes the saved snapshot into the
|
||||
live engine — `src/AcDream.App/Settings/RuntimeSettingsController.cs:420`
|
||||
(`_runtimeTargets?.ApplyAudio(audio);`), documented at `:407-413` as the
|
||||
Campaign OP slice OP6 change that made Audio-tab edits take effect
|
||||
without a relaunch. Its live production consumer is
|
||||
`ConfigOptionsPageController`'s Sound rows (`ConfigOptionsPageController.cs:103`).
|
||||
|
||||
Pre-commit, exactly one test asserted that push: the now-deleted
|
||||
`SettingsViewModelSavePreservesSectionAndTargetOrder`, whose ordered event
|
||||
list contained `"target-audio"` with an explicit OP6 comment
|
||||
(`371197a3~1:tests/AcDream.App.Tests/Settings/RuntimeSettingsControllerTests.cs:296-312`).
|
||||
|
||||
Post-commit, `grep -rn ApplyAudio src tests` shows **no assertion anywhere**
|
||||
that `SaveAudio` invokes `IRuntimeSettingsTargets.ApplyAudio`. The fake
|
||||
still records the event —
|
||||
`tests/AcDream.App.Tests/Settings/RuntimeSettingsControllerTests.cs:817-820`
|
||||
(`events.Add("target-audio");`) — and nothing ever reads it. The two
|
||||
surviving audio tests (`ComputeEffectiveCategoryVolumes_*`, `:241-278`)
|
||||
test the static volume helper, not the controller push. `ApplyStartup`'s
|
||||
own audio call (`RuntimeSettingsController.cs:226`) is still covered by
|
||||
`StartupApplyIsOrderedExactlyOnceAndRuntimeBindingDoesNotReplay` (`:54`),
|
||||
so this gap is specific to the **save** path.
|
||||
|
||||
This is the class the slice contract cannot absorb: the commit asserts
|
||||
"no behavior change to anything live, verified", and one live mechanism is
|
||||
now unverified. Note the contrast — the *other* three target pushes were
|
||||
kept covered, deliberately or not:
|
||||
|
||||
| Push | Post-commit coverage |
|
||||
|---|---|
|
||||
| `SaveDisplay` → `ApplyDisplayWindowState` | implicit but real — `DisplayTargetFailurePreservesEstablishedStoreThenPublishBoundary` (`:566`) sets `ThrowOnDisplay` and asserts the resulting state boundary, which only happens if the target was called |
|
||||
| `ReapplyQualityPreset` → `ApplyQuality` | positive (`:633`, `:653`) |
|
||||
| `SetUiLocked` → `ApplyUiLock` | positive (`:672-679`, `:694`, `:702`) |
|
||||
| `SaveChat` → `SetChatOpacity` | positive — `SaveChat_PushesOpacityToRuntimeTargets_LiveApply_NoRestart` (`:338`) |
|
||||
| **`SaveAudio` → `ApplyAudio`** | **none** |
|
||||
|
||||
**Fix (minimal):** in `RuntimeSettingsControllerTests`, add the missing
|
||||
positive assertion in the same shape as the chat-opacity test — bind a
|
||||
`FakeRuntimeTargets`, call `controller.SaveAudio(controller.Audio with
|
||||
{ Master = 0.25f })`, assert the recorded events contain `"target-audio"`
|
||||
(ideally assert the *value* forwarded, per the `:236-240` comment's own
|
||||
lesson about M2's inversion).
|
||||
|
||||
---
|
||||
|
||||
## SHOULD-FIX 2 — the architecture doc still names `SettingsVM` as a live seam
|
||||
|
||||
`docs/architecture/code-structure.md:209-212`:
|
||||
|
||||
> `WorldRenderFrameBuilder -> RuntimeWorldFrameSettingsPreview ->
|
||||
> IRuntimeSettingsPreviewSource -> RuntimeSettingsController -> optional
|
||||
> SettingsVM` for the live settings draft preview applied before world drawing.
|
||||
|
||||
This sits inside a present-tense list of current presentation seams
|
||||
("These are presentation state/command seams…", `:217`), not a historical
|
||||
narrative. It is the one surviving reference that would send a future
|
||||
reader looking for a type that no longer exists, and CLAUDE.md's
|
||||
architecture rule ("when the architecture doc and reality diverge, update
|
||||
one or the other") makes it in-scope for the same commit.
|
||||
|
||||
Secondary, lower severity, same file: `:738-739` — "The controller also
|
||||
preserves unrelated unsaved Gameplay draft fields when combat preferences
|
||||
change" is now false. It is inside the Slice-8 checkpoint-G/H ledger
|
||||
narrative, so it reads as dated history; a one-clause "(retired at
|
||||
Campaign OP slice OP9)" is enough.
|
||||
|
||||
Everything else the repo-wide sweep turned up is genuinely historical and
|
||||
correctly left alone: `docs/plans/2026-07-22-gamewindow-slice-8-*.md`,
|
||||
`memory/project_gamewindow_decomposition.md:81/85/181`,
|
||||
`docs/research/2026-08-1*-op{3,4,6,8}-*.md`,
|
||||
`docs/research/2026-08-10-keyboard-config-and-gameplay-tab.md`,
|
||||
`docs/research/2026-08-10-character-options-map.md`,
|
||||
`docs/ISSUES.md:15859`, and the `docs/research/2026-05-21-a6-captures/*.log`
|
||||
run logs.
|
||||
|
||||
---
|
||||
|
||||
## SHOULD-FIX 3 — three dead residues left behind by the retirement
|
||||
|
||||
The slice deleted the *callers* without sweeping what became unreachable:
|
||||
|
||||
1. **`RuntimeSettingsController.SaveCharacter(CharacterSettings)`** —
|
||||
`src/AcDream.App/Settings/RuntimeSettingsController.cs:547-566`. Private,
|
||||
**zero callers** post-commit (its only caller was the `saveCharacter`
|
||||
callback handed to the deleted `CreateViewModel`). C# emits no warning
|
||||
for an unused private method, so the build stays green while the method,
|
||||
its `_defaultCharacter` promotion branch, and the whole write half of
|
||||
the `CharacterSettings` store are unreachable:
|
||||
`IRuntimeSettingsStorage.SaveCharacter` (`:31`),
|
||||
`JsonRuntimeSettingsStorage.SaveCharacter` (`:68`), and
|
||||
`SettingsStore.SaveCharacter` now have no production writer.
|
||||
(`ActiveToonKey` is genuinely live — `InteractionRetainedUiComposition.cs:507`
|
||||
and `:612` key per-character window layouts off it — so only the
|
||||
`Character`/`SaveCharacter` half is dead. `RuntimeSettingsController.Character`
|
||||
likewise has no production reader.)
|
||||
The commit also deleted that method's only test
|
||||
(`CharacterSaveUsesActiveToonAndDefaultSaveBecomesResetContext`) without
|
||||
re-targeting it — consistent with the code being dead, but it means the
|
||||
code is now both uncallable and untested rather than removed.
|
||||
2. **`IngressShutdownRoots.Settings`** —
|
||||
`src/AcDream.App/Rendering/GameWindowLifetime.cs:75`, captured at
|
||||
`src/AcDream.App/Rendering/GameWindow.cs:1670`. Its **only** reader was
|
||||
the `Soft("settings view model", …UnbindViewModel())` stage this commit
|
||||
deleted; `grep` finds no other `ingress.Settings` use. Harmless (a
|
||||
reference held until shutdown) but it is now a shutdown-roots member
|
||||
that roots nothing.
|
||||
|
||||
Either delete these in the OP9 follow-up, or — if `CharacterSettings`
|
||||
persistence is meant to come back on a later Options-panel tab — say so in
|
||||
a one-line comment so the next reader does not re-derive that it is dead.
|
||||
The commit message's "CharacterSettings … UNTOUCHED per the campaign
|
||||
contract" covers keeping the *record and store*; it does not explain the
|
||||
now-uncallable controller method.
|
||||
|
||||
---
|
||||
|
||||
## SHOULD-FIX 4 — the −80 test delta does not reconcile; static enumeration gives −84
|
||||
|
||||
The commit states the delta "is exactly the deleted SettingsVM/
|
||||
SettingsPanel/GameplaySettings test surface". Enumerating the touched
|
||||
files at both commits:
|
||||
|
||||
| File | Removed | Added |
|
||||
|---|---:|---:|
|
||||
| `tests/.../Panels/Settings/GameplaySettingsTests.cs` (deleted) | 3 | — |
|
||||
| `tests/.../Panels/Settings/SettingsVMTests.cs` (deleted) | 43 | — |
|
||||
| `tests/.../Panels/Settings/SettingsPanelTests.cs` (deleted) | 29 | — |
|
||||
| `tests/AcDream.App.Tests/Settings/RuntimeSettingsControllerTests.cs` | 13 | 6 |
|
||||
| `tests/.../Panels/Settings/SettingsStoreTests.cs` | 4 | 2 |
|
||||
| `tests/AcDream.App.Tests/Input/GameplayInputCommandControllerTests.cs` | 2 (InlineData) | 2 (InlineData) |
|
||||
| `tests/.../Rendering/GameWindowSlice8BoundaryTests.cs`, `tests/.../Input/InputDispatcherCaptureTests.cs` | 0 | 0 |
|
||||
| **Net** | **94** | **10** |
|
||||
|
||||
**−84**, not −80. All three deleted files declare `public sealed class`
|
||||
(discoverable), none carries `[Theory]`/`[MemberData]`/`Skip`, and the
|
||||
skip count is 4 on both sides — so no discovery artifact explains the gap.
|
||||
The baseline is quoted twice in the immediately preceding commits
|
||||
(`f1d50207`, `1a57f96e`: 13,155 passed / 4 skipped), so either that
|
||||
baseline is off by 4 or the post number is. Nothing in the diff adds four
|
||||
tests elsewhere.
|
||||
|
||||
This is an evidence-accounting defect, not a behavior defect — **no live
|
||||
behavior test is unaccounted for by it** (the one real coverage loss is
|
||||
MUST-FIX 1, which is inside the 13-removed RuntimeSettingsControllerTests
|
||||
bucket). Re-measure and restate, or drop the word "exactly".
|
||||
|
||||
For the record, the 13 removed `RuntimeSettingsControllerTests` cases
|
||||
disposition cleanly: 6 were re-targeted onto the surviving API
|
||||
(`CharacterContext*`, `DraftPreview*`, `RuntimeTargetLoans*`,
|
||||
`SetUiLocked_*`, `SyncChatFromServerOptions*`, `UnboundRuntimeTargets*`),
|
||||
5 were SettingsVM-draft-only, 1 (`UiLockPersistenceFailureCanRetry…`)
|
||||
pinned a retry against a save that no longer exists — and its
|
||||
target-failure sibling `UiLockTargetFailureCanRetryTheSameRequestedValue`
|
||||
(`:683`) survives and still pins the real invariant — 1 covered the now-dead
|
||||
`SaveCharacter` (SF-3), and 1 (`SettingsViewModelSavePreservesSectionAndTargetOrder`)
|
||||
carried the `"target-audio"` assertion (MUST-FIX 1).
|
||||
|
||||
---
|
||||
|
||||
## SHOULD-FIX 5 — dangling comment referencing a deleted assertion
|
||||
|
||||
`tests/AcDream.App.Tests/Settings/RuntimeSettingsControllerTests.cs:236-240`:
|
||||
|
||||
> "The FakeRuntimeTargets-based `"target-audio"` assertion **above** (and its
|
||||
> predecessor before this rework) only ever checked that ApplyAudio fired…"
|
||||
|
||||
There is no longer any `"target-audio"` assertion above (or anywhere); what
|
||||
is now immediately above is
|
||||
`ConcreteRuntimeTargetPublishesSetSingleCharacterOptionOntoTheBus`. Reword
|
||||
when fixing MUST-FIX 1 — the two are the same edit.
|
||||
|
||||
---
|
||||
|
||||
## NIT 6 — "bound through … `CharacterOptionsPageController`" is imprecise for 2 of the 13
|
||||
|
||||
Commit §4 and the AP-196 retirement note both say all 13 members "were
|
||||
ALREADY bound through `CharacterOptionTable`/`CharacterOptionsPageController`'s
|
||||
server-bit seam (the OP4 Character tab)". All 13 are in the table —
|
||||
verified one by one at `src/AcDream.Runtime/Gameplay/CharacterOptionTable.cs`
|
||||
`:117,119,121,123,125,127,130,131,132,158,160,161,162` — but **two are
|
||||
deliberately not Character-tab rows** (`CharacterOptionsPageControllerTests.cs:26`
|
||||
names the exclusion set: `AppearOffline`, `UseMouseTurning`, `LockUI`, per
|
||||
research §2.7):
|
||||
|
||||
- `LockUI` — bound through `/lockui` + the radar control:
|
||||
`LiveSessionRuntimeFactory.cs:419-421` (server-bit read, `0x0005` send,
|
||||
`SetUiLocked` visual push), `InteractionRetainedUiComposition.cs:492`
|
||||
(mount-time seed), `:647` (radar binding).
|
||||
- `UseMouseTurning` — bound through the Gameplay-tab mouse macro:
|
||||
`InteractionRetainedUiComposition.cs:843-845` (`IsUseMouseTurningEnabled`
|
||||
live server-bit read) and `RetailUiRuntime.cs:2009-2019` (wire send).
|
||||
Note this is the *server* bit, correctly distinct from the Config tab's
|
||||
own client-local `CameraTurningSettings.UseMouseTurning` (TS-74,
|
||||
`ConfigOptionsPageController.cs:605-612`).
|
||||
|
||||
The **substance holds** — all 13 have a live server-bit home and none was
|
||||
orphaned — only the sentence's channel attribution is loose. Worth
|
||||
correcting in the register row, which is the durable artifact.
|
||||
|
||||
## NIT 7 — OP9's own bookkeeping is still outstanding
|
||||
|
||||
`docs/plans/2026-08-10-options-panel-campaign.md:428` still reads
|
||||
`| OP9 | — | | | |`, and `:9` still says "ACTIVE — planned 2026-08-10. No
|
||||
slices landed yet" (stale for the whole campaign, pre-existing). §OP9's
|
||||
bullet list (`:352-370`) also owes the ISSUES sweep (#358 retest), the
|
||||
CLAUDE.md Current-state paragraph, and the memory digest update. None of
|
||||
that is a defect in *this* commit — it is the closeout half of the slice —
|
||||
but it should not be forgotten when OP9 is declared complete.
|
||||
|
||||
---
|
||||
|
||||
## Verified clean (mechanism lens)
|
||||
|
||||
1. **`RuntimeSettingsController` live core** — all present and unaltered:
|
||||
storage load/save for Display/Audio/Chat/Character (`:170-181`),
|
||||
`ResolvedQuality`/`ReapplyQualityPreset` (`:369-383`), `ApplyStartup`
|
||||
(`:213-235`), `SaveAudio`'s OP6 live-apply (`:414-427`), public
|
||||
`SaveDisplay` (`:391-405`, still the `ConfigOptionsPageController` seam),
|
||||
`IRuntimeSettingsTargets.SetSingleCharacterOption` (`:115`) and
|
||||
`SetChatOpacity` (`:124`), `ServerOptionsSeeded` /
|
||||
`NotifyServerOptionsSeeded` (`:542-545`), `SetUiLocked` (`:292-299`).
|
||||
The only removals are `Gameplay`, `SetAcceptLootPermits`, the private
|
||||
`SaveGameplay`, and the four `_viewModel?` notification calls.
|
||||
|
||||
**`SetUiLocked` first-call / repeat-call equivalence — verified.**
|
||||
The old guard was `if (_lastAppliedUiLocked == locked && _uiLockConverged)
|
||||
return;` with `_uiLockConverged` initialised **true** and
|
||||
`_lastAppliedUiLocked` initialised **null**. On the first call
|
||||
`(bool?)null == locked` is `false`, so the conjunct never mattered: the
|
||||
first `ApplyUiLock` push happened then and happens now. Deleting
|
||||
`_uiLockConverged` therefore cannot have changed whether the first push
|
||||
fires. Repeat same-value calls returned early then (after a successful
|
||||
`SaveGameplay` set `_uiLockConverged = true`) and return early now.
|
||||
Failure ordering is also preserved: `_lastAppliedUiLocked` is still
|
||||
assigned *after* `ApplyUiLock`, so a throwing target leaves the guard
|
||||
un-advanced and the same value can be retried — pinned by
|
||||
`UiLockTargetFailureCanRetryTheSameRequestedValue` (`:683-704`). The one
|
||||
behavioral difference is that a *storage* failure no longer forces a
|
||||
redundant re-push on the next same-value call; there is no storage call
|
||||
left to fail, and `ApplyUiLock` is idempotent. All three call sites
|
||||
traced and unchanged: PlayerDescription convergence
|
||||
(`LiveSessionRuntimeFactory.cs:339-340`), `ToggleUiLock` (`:419-421`),
|
||||
radar/Character-tab path (`InteractionRetainedUiComposition.cs:647`).
|
||||
|
||||
2. **`IRuntimeSettingsPreviewSource`** — the "already always false in
|
||||
production" claim is **true**. Searching the *pre*-commit tree
|
||||
(`git grep … 371197a3~1`) for `CreateViewModel` / `CreateViewModelBinding`
|
||||
/ `new SettingsVM` / `new SettingsPanel` returns **only** test files plus
|
||||
the controller's own definitions. `_viewModel` was therefore always
|
||||
`null` in production, so `HasDraftPreview` was always `false` and
|
||||
`DisplayPreview`/`AudioPreview` always fell through to
|
||||
`Display`/`Audio` — exactly what they now return unconditionally. Both
|
||||
consumers see identical values: `WorldRenderFrameBuilder.cs:370-380`
|
||||
(the whole draft block was already dead in production) and
|
||||
`LiveObjectFrameController.cs:59-62` (`SettingsParticleRangeSource`,
|
||||
which reads `DisplayPreview` unconditionally — unchanged). Third
|
||||
consumer `InteractionRetainedUiComposition.cs:685`
|
||||
(`DisplayPreview.ShowFps`) likewise unchanged.
|
||||
|
||||
3. **`GameplayInputCommandController`** — diff confirms exactly three cases
|
||||
touched. `AcdreamToggleDebugPanel` (`:190-197`) and `ToggleChatEntry`
|
||||
(`:208-213`) keep `return true`, matching the prior no-op's handled
|
||||
contract; `ToggleOptionsPanel` (`:214-224`) still calls
|
||||
`_retained.ToggleOptionsPanel()` with only the comment reworded. No
|
||||
other `InputAction` case changed, and the removed constructor parameter
|
||||
is the only signature change (`SessionPlayerComposition.cs:1194`
|
||||
updated). Worth recording: the return value is inert in production —
|
||||
`GameplayInputActionRouter.HandleCommand` (`:170-171`) discards it and
|
||||
the command controller is the **last** stage of the priority graph — so
|
||||
the `true`-vs-`false` question could not have been observable either
|
||||
way. The new `RetiredDevToolsCommand_IsConsumedWithoutClaimingATypedOwner`
|
||||
theory (`GameplayInputCommandControllerTests.cs:43-56`) re-targets the two
|
||||
deleted mapping rows rather than dropping them. No `ToggleSettingsPanel`
|
||||
`InputAction` exists (`grep` over `AcDream.UI.Abstractions` is empty), so
|
||||
`KeyBindings.RetailDefaults()` and the keymap fixtures needed no edit —
|
||||
claim confirmed.
|
||||
|
||||
4. **`LiveSessionRuntimeFactory`** — `SetAcceptLootPermits` still sends
|
||||
`0x0005` via `SendSingleCharacterOption` (`:494-496`); only the
|
||||
`_interaction.Settings.SetAcceptLootPermits(value)` write-behind line was
|
||||
removed, and it had no reader. `AcceptLootPermits` still reads the server
|
||||
bit (`:491-493`). `ToggleUiLock` (`:415-422`) and the PlayerDescription
|
||||
`OnCharacterOptionsChanged` convergence (`:337-345`) are byte-identical
|
||||
apart from comments.
|
||||
|
||||
5. **settings.json migration** — mechanism verified at
|
||||
`SettingsStore.SaveSection` (`src/AcDream.UI.Abstractions/Panels/Settings/SettingsStore.cs:643-687`):
|
||||
every top-level key other than the target section and `"version"` is
|
||||
carried forward by `prop.Value.GetRawText()`, so an orphaned
|
||||
`"gameplay"` block survives *every* subsequent save, and no `Load*`
|
||||
method reads it. The two new tests
|
||||
(`SettingsStoreTests.cs:219-260`) pin exactly that, not something weaker:
|
||||
the first proves a leftover block does not disturb Display/Audio/Chat
|
||||
loads; the second writes an unrelated section and asserts the raw
|
||||
`"gameplay"` / `"lockUI": true` / `"showHelm": false` text is still in the
|
||||
file afterwards. The re-targeted coexistence and window-position/layout
|
||||
tests correctly substitute a Display assertion for the deleted Gameplay
|
||||
one, preserving each test's actual point.
|
||||
|
||||
## Verified clean (blast lens)
|
||||
|
||||
- **Forward references.** Repo-wide sweep (`src`, `tests`, `tools`,
|
||||
`scripts`, `docs`, `.claude`) for `SettingsVM`, `SettingsPanel`,
|
||||
`RuntimeSettingsViewModelBinding`, `CreateViewModel`,
|
||||
`IDevToolsGameplayCommands`, `DevToolsGameplayCommands`,
|
||||
`GameplaySettings`, `LoadGameplay`, `SaveGameplay`,
|
||||
`SetAcceptLootPermits` (the controller method), `ToggleSettingsPanel`,
|
||||
`UnbindViewModel`: the only *code* hits are OP9's own explanatory
|
||||
comments, plus `ICombatGameplaySettingsSource` substring matches
|
||||
(`RuntimeCharacterState.cs:850`) and an unrelated
|
||||
`DebugVM CreateViewModel()` local helper in
|
||||
`tests/AcDream.App.Tests/Combat/CombatFeedbackSlotTests.cs:29`. One
|
||||
vacuous-but-harmless residue: `GameWindowSlice8BoundaryTests.cs:319`
|
||||
still asserts `DoesNotContain(".LoadGameplay()")` (and `:328/:330`
|
||||
`_persistedGameplay`/`_settingsVm`) — negative source-text guards that
|
||||
can no longer fail wrongly; leaving them is fine.
|
||||
- **No string/reflection lookup** of the deleted panel: `acdream.settings`
|
||||
(the old `SettingsPanel.Id`) appears nowhere in the repo.
|
||||
- **`IPanel`/`IPanelRenderer` survive with real implementers** —
|
||||
`ChatPanel.cs:34`, `DebugPanel.cs:22`, `VitalsPanel.cs:16`. Claim
|
||||
confirmed.
|
||||
- **Headless / UI Studio / plugin surfaces** (the three the implementer's
|
||||
15-file list did not cover): `src/AcDream.Headless`,
|
||||
`src/AcDream.Plugin.Abstractions`, `src/AcDream.Runtime`,
|
||||
`src/AcDream.Core*`, `tools/`, `scripts/`, `.claude/` — **zero** hits for
|
||||
any deleted type. `src/AcDream.UI.ImGui/` is an empty leftover directory,
|
||||
not in `AcDream.slnx`.
|
||||
- **`SettingsDevToolsComposition` survives correctly.** Despite its name it
|
||||
is a live composition phase (`ApplyStartup`,
|
||||
`SettingsDevToolsComposition.cs:53`); only its doc comment needed the
|
||||
update it got. The plan's "retire … `SettingsDevToolsComposition` wiring"
|
||||
bullet referred to the ImGui wiring already removed at V11.
|
||||
- **Register (§7 of the brief).** `AP-196` is struck through with a
|
||||
substantive retirement note (`retail-divergence-register.md:192`), the
|
||||
section-3 header moves 143 → 142 (`:178`), and the counts verify by
|
||||
enumeration: active `| AP-` rows 143 → 142, retired `| ~~AP-` rows
|
||||
29 → 30, total row count unchanged. No other row cites a deleted type —
|
||||
the only other `GameplaySettings` hit is `TS-73` (`:367`), a substring
|
||||
match inside `ICombatGameplaySettingsSource`.
|
||||
- **Gate script (§9 of the brief).**
|
||||
`docs/research/2026-08-11-campaign-op-test-script.md` is clean: every
|
||||
"F11" and "Gameplay tab" reference is to the retail Options panel
|
||||
(post-OP3), not to the deleted ImGui surface. No step exercises anything
|
||||
this commit removed.
|
||||
|
||||
---
|
||||
|
||||
## Fix list (ordered, for the fixer)
|
||||
|
||||
1. Add the `SaveAudio` → `ApplyAudio` assertion to
|
||||
`RuntimeSettingsControllerTests`; reword the `:236-240` comment in the
|
||||
same edit (MUST-FIX 1 + SF-5).
|
||||
2. Update `docs/architecture/code-structure.md:209-212` (drop the
|
||||
`-> optional SettingsVM` link) and tag `:738-739` as OP9-retired (SF-2).
|
||||
3. Delete `RuntimeSettingsController.SaveCharacter` (`:547-566`) and
|
||||
`IngressShutdownRoots.Settings` (`GameWindowLifetime.cs:75`,
|
||||
`GameWindow.cs:1670`), or document why they stay (SF-3).
|
||||
4. Re-measure and restate the test delta, or drop "exactly" (SF-4).
|
||||
5. Correct the LockUI/UseMouseTurning channel wording in the AP-196
|
||||
retirement note (NIT 6); fill the OP9 plan-ledger row at closeout
|
||||
(NIT 7).
|
||||
Loading…
Add table
Add a link
Reference in a new issue