docs: OP8 combined review — REJECT (3 MUST): activation-loss, camera aliasing, silent reassign
M1: SetForAction rebuilds bindings as new Binding(chord, action), defaulting Press/InputScope.Game and dropping the read chord's ActivationType/InputScope — MovementWalkMode (Hold), the three melee heights (Hold+MeleeCombat), Missile/Magic-scoped rows, and CameraInstantMouseLook (Hold, Device 1) all collapse; RestoreDefaultValue invokes _apply so ONE Defaults click flattens ~140 actions and OK persists it (gate step 12 tells the user to click Defaults). M2: the ten camera 0x5/0x6 rows alias one InputAction each — 20 rows over 10 targets, twins seed identically, clobber each other, self-conflict; the round-trip test justifies the dual map on independence the mechanism makes impossible. M3: silent auto-reassign (NotifyReassigned -> empty, DisplaySystemMessage skips empty) destroys a binding with zero feedback while the doc claims it reports; retail confirms first (OpenOverwriteBindingDialog), the plan gate says 'conflict prompt', the tested SettingsVM prompt precedent exists, AP-204 concedes RetailDialogFactory is wired, and a fake asserts a message production contradicts. SHOULD: non-bindable check ordered after first-match conflict (inverts retail's any-non-bindable-refuses); ActionMap. ConflictingMaps never read (false conflicts on legitimately-shared combat keys); Save lacks the writer's try/catch; sparse-row Mapping-3 lands on Mapping-1; ~330 uncached layout imports at startup. Clean: the 0x26000000 DID (better-anchored than cited — .actionmap MasterDBMap range, 0x27 is the type tag); no-Clear-button confirmed against the fixture; six-box grouping matches lane D; identity byte-comparison is a permanent test; modal capture real; AP-202/203/204 accurate; OP3 INERT retired. Brief-premise correction:ff577641is the CAMPAIGN tip (claude/latest-commits-cb0c8f), not main's (852cdda7); OP8 additive, only OptionPageModel.cs is a triple-append merge watch-point. Rework round 1 to the isolated op8-keyboard worktree. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
ff5776415b
commit
1e36d4a7a1
1 changed files with 448 additions and 0 deletions
448
docs/research/2026-08-11-op8-review.md
Normal file
448
docs/research/2026-08-11-op8-review.md
Normal file
|
|
@ -0,0 +1,448 @@
|
|||
# Campaign OP slice OP8 (Configure Keyboard) — combined dual-lens review
|
||||
|
||||
**Reviewed:** worktree `C:\Users\erikn\source\repos\acdream\.claude\worktrees\op8-keyboard`,
|
||||
branch `op8-keyboard`, commit `b4edee97`, based on campaign tip `ff577641`.
|
||||
**Contract:** `docs/plans/2026-08-10-options-panel-campaign.md` §4 OP8 + §2 D4.
|
||||
**Research:** `docs/research/2026-08-10-keyboard-config-and-gameplay-tab.md` §5–§7.
|
||||
**Mode:** read-only. No build, no test, no launch. Findings are source- and
|
||||
DAT-fixture-derived, plus independent decomp corroboration.
|
||||
|
||||
## Verdict: **REJECT**
|
||||
|
||||
Three MUST-FIX findings. Two are silent data-destruction bugs in the live
|
||||
`KeyBindings` write path that the slice's own test seam cannot see by
|
||||
construction (M1, M2); one is a contract deviation where the shipped behaviour
|
||||
is strictly worse than the register row describing it, and the connected-gate
|
||||
script was rewritten to accept it (M3).
|
||||
|
||||
The DAT-reading half of this slice is genuinely good work — the `0x26000000`
|
||||
resolution is correct and better-anchored than its own citation claims, the
|
||||
"no Clear button" finding is verified against the committed fixture, the
|
||||
six-page scoping avoids the reused-id trap correctly, and the identity table's
|
||||
DAT-vs-`RetailDefaults()` agreement is pinned by a permanent test. The defects
|
||||
are all downstream, in the seam between a resolved row and the live dispatcher.
|
||||
|
||||
---
|
||||
|
||||
## MUST-FIX
|
||||
|
||||
### M1 — every write through `SetForAction` destroys `ActivationType` and `InputScope`
|
||||
|
||||
`src/AcDream.App/UI/RetailUiRuntime.cs:2337-2344`:
|
||||
|
||||
```csharp
|
||||
SetForAction: (action, chords) =>
|
||||
{
|
||||
KeyBindings updated = CloneWithout(dispatcher.Bindings, action);
|
||||
foreach (KeyChord chord in chords)
|
||||
updated.Add(new Binding(chord, action));
|
||||
dispatcher.SetBindings(updated);
|
||||
},
|
||||
```
|
||||
|
||||
`Binding` (`src/AcDream.UI.Abstractions/Input/Binding.cs:14-17`) defaults
|
||||
`Activation = ActivationType.Press` and `Scope = InputScope.Game`. The seam
|
||||
type (`Action<InputAction, IReadOnlyList<KeyChord>>`,
|
||||
`KeyboardConfigController.cs:158`) carries chords only, and the read side
|
||||
(`RetailUiRuntime.cs:2335-2336`) does `.Select(b => b.Chord)` — so activation
|
||||
and scope are dropped on read and re-invented as `Press`/`Game` on write.
|
||||
|
||||
That is not hypothetical. `KeyBindings.RetailDefaults()` binds, and
|
||||
`RetailActionIdentityTable` maps, all of the following:
|
||||
|
||||
| Action | RetailDefaults | Identity-table row |
|
||||
|---|---|---|
|
||||
| `MovementWalkMode` | `ActivationType.Hold` (`KeyBindings.cs:173-174`) | `(0x4, 0x32)` |
|
||||
| `CombatLowAttack` / `MediumAttack` / `HighAttack` | `Hold` + `InputScope.MeleeCombat` (`KeyBindings.cs:262-264`) | `(0x10000003, 0x1000005D-5F)` |
|
||||
| `CombatDecrease/IncreaseAttackPower` | `InputScope.MeleeCombat` (`:257-258`) | `(0x10000003, 0x1000005B-5C)` |
|
||||
| every Missile row | `InputScope.MissileCombat` (`:269-273`) | `(0x10000004, 0x100000EF-F3)` |
|
||||
| every Magic row | `InputScope.MagicCombat` (`:274-287`) | `(0x10000005, …)` |
|
||||
| `CameraInstantMouseLook` | `Hold`, mouse `Device: 1` (`:303-306`) | `(0x5, 0x3D)` |
|
||||
|
||||
Blast radius is much wider than "a user rebinds one key", because
|
||||
`ActionKeyMapOptionRow` invokes `_apply` from `SetCurrentValue`,
|
||||
`RestoreSavedValue` **and** `RestoreDefaultValue`
|
||||
(`OptionPageModel.cs:566-587`):
|
||||
|
||||
- **Cancel / Revert** run `Page.Reset()` → `RestoreSavedValue()` → `_apply`,
|
||||
so cancelling a rebind does *not* restore the original `Hold`/scope — it
|
||||
writes the saved chords back as `Press`/`Game`.
|
||||
- **Defaults** runs `Page.Defaults()` → `RestoreDefaultValue()` on **every**
|
||||
row unconditionally (`OptionPageModel.cs:712-717`), so a single click of
|
||||
Defaults collapses the activation and scope of the whole mapped table —
|
||||
~140 actions — in one shot. `KeyboardConfigController.WireScreenButtons`
|
||||
(`:476-484`) wires exactly that.
|
||||
- **OK** then persists the collapsed table:
|
||||
`dispatcher.Bindings.SaveToFile(...)` (`RetailUiRuntime.cs:2349`), and
|
||||
`KeyBindings.SaveToFile` only emits activation/scope when non-default
|
||||
(`KeyBindings.cs:514-516`), so the loss is permanent across relaunch.
|
||||
|
||||
Observable effects: walk-mode never unlatches (its `Hold` becomes `Press`);
|
||||
the three melee attack-height holds stop repeating; every combat-scoped
|
||||
binding moves into `InputScope.Game`, changing scope precedence for every
|
||||
other binding sharing those chords (Insert/Delete/End/PageDown/PageUp are
|
||||
shared across Melee/Missile/Magic by design); and `CameraInstantMouseLook`'s
|
||||
`Device: 1` MMB-hold is replaced by a keyboard `Press` chord.
|
||||
|
||||
Note the connected-gate script step 12 (`docs/research/2026-08-11-campaign-op-test-script.md`,
|
||||
§OP8 "Erase, then Reset to Defaults") instructs the user to click Defaults —
|
||||
i.e. the gate as written triggers this bug.
|
||||
|
||||
**Fix shape:** widen the seam to carry `Binding`, not `KeyChord` — read
|
||||
`dispatcher.Bindings.ForAction(action)` whole, and on write preserve each
|
||||
surviving binding's `Activation`/`Scope`, defaulting new chords from the
|
||||
action's existing bindings (or from the DAT row's `RetailKeyChord.Activation`,
|
||||
which `RetailActionMapReader` already reads and
|
||||
`KeyboardConfigController.DatDefaultsToChords` (`:356-366`) currently discards).
|
||||
|
||||
### M2 — the Camera 0x5/0x6 rows alias one `InputAction`; two independent row models, one shared live target
|
||||
|
||||
`RetailActionIdentityTable.cs:92-104` maps ten camera actions under **both**
|
||||
InputMap `0x5` and `0x6` to the same `InputAction`:
|
||||
|
||||
```csharp
|
||||
foreach (uint ctx in new uint[] { 0x5, 0x6 })
|
||||
{
|
||||
M(ctx, 0x33, InputAction.CameraMoveToward);
|
||||
…
|
||||
}
|
||||
```
|
||||
|
||||
`KeyboardConfigController.Bind` builds one row per DAT row, so this produces
|
||||
20 rows over 10 actions. Each row seeds from
|
||||
`bindings.CurrentForAction(action)` (`KeyboardConfigController.cs:328-330`) —
|
||||
the *same* list for both — and each writes through `SetForAction`, which
|
||||
replaces the action's entire binding set (`CloneWithout` + re-add). Three
|
||||
consequences:
|
||||
|
||||
1. **Both rows display identical chords**, not the numpad scheme in one and
|
||||
the arrow scheme in the other. The two `ActionKeyMapOptionRow` instances
|
||||
are independent, so after rebinding one, the other's model is stale and the
|
||||
screen shows a chord that is no longer bound.
|
||||
2. **A rebind of one row silently wipes the other's**, because `SetForAction`
|
||||
is whole-action replacement.
|
||||
3. **`FindConflict` (`:446-464`) excludes only the row being edited**, so
|
||||
setting a camera row to the chord its own twin displays is reported as a
|
||||
cross-row conflict and triggers the auto-reassign path against itself.
|
||||
|
||||
This directly contradicts the justification the slice files for the mapping.
|
||||
`RetailActionIdentityRoundTripTests.cs:36-47` says the dual mapping is correct
|
||||
because "a user can rebind either scheme's row independently" — the mechanism
|
||||
makes that impossible. The register (AP-203) does not cover it either; it
|
||||
covers *unmapped* rows, not aliased ones.
|
||||
|
||||
**Fix shape:** either give `InputAction` a distinct member per scheme (the
|
||||
honest retail shape — retail keeps them as separate `(InputMap, Action)` rows
|
||||
precisely because they are separately bindable), or key row storage on
|
||||
`(InputMapId, ActionId)` throughout and let the mapped case resolve to a
|
||||
per-context slice of the action's bindings rather than the whole set. The
|
||||
current "roughly half the rows are mapped, the rest use
|
||||
`RetailUnmappedKeyBindings`" split already has the per-row keyed store needed
|
||||
for the second option.
|
||||
|
||||
### M3 — the auto-reassign is silent in production, the contract asked for a prompt, and the test asserts a message production never emits
|
||||
|
||||
Three separate problems that compound:
|
||||
|
||||
**(a) Production wires the notification to nothing.**
|
||||
`RetailUiRuntime.cs:2359-2366` supplies `NotifyReassigned: _ => string.Empty`,
|
||||
and `DisplaySystemMessage` at `:2352-2355` skips empty strings. So
|
||||
`BeginSlotCapture`'s reassign branch
|
||||
(`KeyboardConfigController.cs:391-399`) erases another row's binding and says
|
||||
nothing at all. The controller's own class doc (`:96-98`) claims it "reports
|
||||
the outcome via `Bindings.NotifyReassigned`" — it does not.
|
||||
|
||||
**(b) The contract asked for a prompt.** Plan §4 OP8's gate line reads
|
||||
"conflict prompt on a taken chord", and the contract body says "N-way
|
||||
cross-map conflicts + the non-user-bindable refusal per lane D §5". Retail
|
||||
confirms *before* overwriting (research §5.4:
|
||||
`OpenOverwriteBindingDialog(&conflicts)` — "confirm, may be many"). acdream
|
||||
already has a conflict-prompt precedent: `SettingsVM.PendingConflict`
|
||||
("'X' is already bound to Y. Reassign it to Z?", research §6.1), which D1
|
||||
explicitly says "feeds OP8". AP-204 itself notes `RetailDialogFactory`'s
|
||||
catalog is wired. So the prompt is neither unprecedented nor unbuildable here.
|
||||
|
||||
A register row documents a deviation; it does not authorise deviating from the
|
||||
slice's own stated gate criterion. Per the review brief's own test — *if retail
|
||||
confirms before overwriting, silent reassign is a MUST-FIX-class fidelity gap,
|
||||
register row or not* — retail does confirm, and this is that gap.
|
||||
|
||||
**(c) The gate script was rewritten to accept the shipped behaviour.**
|
||||
`docs/research/2026-08-11-campaign-op-test-script.md` §OP8 step 9 now reads
|
||||
"'Move Forward' silently loses its W slot … this port applies it immediately
|
||||
without asking first". Changing the acceptance criterion to match the
|
||||
implementation is a coordinator decision, not an implementer one.
|
||||
|
||||
**(d) The test pins a behaviour production defeats.**
|
||||
`KeyboardConfigControllerTests.cs:88` supplies
|
||||
`NotifyReassigned: label => $"reassigned from {label}"` and `:240` asserts
|
||||
`fake.Messages` contains "reassigned". That test passes while the shipped
|
||||
screen is silent — false confidence exactly where the risk is.
|
||||
|
||||
**Minimum fix:** either build the confirm dialog (contract-faithful), or — if
|
||||
the coordinator accepts the narrowing — supply a real notification string so
|
||||
the destruction is at least visible, fix the controller doc, and have the
|
||||
coordinator (not the implementer) amend the plan's gate line and the script
|
||||
together.
|
||||
|
||||
---
|
||||
|
||||
## SHOULD-FIX
|
||||
|
||||
### S1 — the conflict scan is first-match, and the non-bindable check is ordered after it
|
||||
|
||||
`FindConflict` (`KeyboardConfigController.cs:446-464`) returns on the **first**
|
||||
matching row, and only reaches the acdream-only ("non-bindable") scan when no
|
||||
row matched. Retail does the opposite (research §5.4): it collects *all*
|
||||
conflicting `(map, control, action)` triples, and **if any one of them is
|
||||
non-user-bindable it refuses outright**. So a chord that conflicts with both a
|
||||
DAT row and an acdream-only action gets auto-reassigned here where retail
|
||||
would refuse; and when a chord is held by three rows, only one is cleared,
|
||||
leaving the other two still holding it. The class doc at `:84-94` describes
|
||||
this as retail's N-way model; it is a first-match model.
|
||||
|
||||
### S2 — the DAT's own `ActionMap.ConflictingMaps` table is never read
|
||||
|
||||
`RetailActionMapReader.Read` (`src/AcDream.Core/Input/RetailActionMap.cs:197-221`)
|
||||
iterates `actionMap.InputMaps` only. The `ActionMap` DBObj also exposes
|
||||
`ConflictingMaps` (`Dictionary<uint, InputsConflictsValue>` — see the test's
|
||||
own construction at `RetailActionMapReaderTests.cs:47`), which is
|
||||
near-certainly what retail's `ICIDM::FindConflictingInputMaps` consults. The
|
||||
port instead treats all 306 rows as one flat conflict universe, so chords that
|
||||
retail considers legitimately shared across non-conflicting contexts are
|
||||
reported as conflicts. `RetailDefaults()` proves this is a live case:
|
||||
Insert / PageUp / Delete / End / PageDown are each bound in all three of
|
||||
`MeleeCombat`, `MissileCombat` and `MagicCombat` (`KeyBindings.cs:257-282`),
|
||||
and those are distinct DAT rows under contexts `0x10000003/4/5`. Rebinding any
|
||||
of them will trip a spurious conflict and (per M3) silently erase a sibling.
|
||||
|
||||
### S3 — the OK button's save has no error handling, unlike the established precedent
|
||||
|
||||
`RetailUiRuntime.cs:2346-2351`:
|
||||
|
||||
```csharp
|
||||
Save: () =>
|
||||
{
|
||||
dispatcher.Bindings.SaveToFile(keyboard.KeyBindingsFilePath);
|
||||
unmapped.SaveToFile(unmappedPath);
|
||||
},
|
||||
```
|
||||
|
||||
The codebase's existing writer for the same file wraps this in try/catch and
|
||||
logs (`src/AcDream.App/Settings/RuntimeKeyBindingTarget.cs:34-44`, whose
|
||||
comment explicitly reasons about not rolling back the live binding on
|
||||
persistence failure). Here an IO failure throws out of `UiButton.OnClick`,
|
||||
through `UiRoot.BubbleEvent`, into the input/render loop. Match the existing
|
||||
pattern.
|
||||
|
||||
### S4 — assigning a high slot collapses onto a low one
|
||||
|
||||
`BeginSlotCapture` (`:408-411`) pads with `default(KeyChord)` up to the target
|
||||
slot, then `ReplaceSlotValue` (`:425-426`) strips every `default` before
|
||||
storing. So on a row with no bindings, clicking "Mapping 3" and pressing a key
|
||||
puts the label on **Mapping 1**; on a row with one binding, clicking
|
||||
"Mapping 3" lands on Mapping 2. Retail's `SetBinding(qc, slot)` writes the
|
||||
slot the user clicked. (The erase path's shift via `RemoveAt` is
|
||||
retail-consistent — `m_qclCurrent` is a list — so this is specifically the
|
||||
assign path.)
|
||||
|
||||
### S5 — ~330 DAT layout imports at startup for a screen most sessions never open
|
||||
|
||||
`MountKeyboardConfig` is unconditional in the mount chain
|
||||
(`RetailUiRuntime.cs:401`) and `KeyboardConfigController.Bind` builds every
|
||||
header and every one of the 306 rows eagerly. Each row is one
|
||||
`AddItemFromTemplateList` → `TemplateResolver` call
|
||||
(`UiTemplateListBox.cs:172-183`), and the production resolver
|
||||
(`RetailUiRuntime.cs:2312-2329`) does a fresh
|
||||
`LayoutImporter.ImportInfos` + `LayoutImporter.Build` under
|
||||
`_bindings.Assets.DatLock` every time — `ImportInfos`
|
||||
(`LayoutImporter.cs:243-254`) has no cache. That is ~330 lock cycles, layout
|
||||
resolves and widget-tree builds (each row = 3 buttons + a synthesized
|
||||
`UiText`) on every launch. The existing Options tabs are 50 and 27 rows; this
|
||||
is roughly 6× the largest. Build the rows on first `Toggle` instead.
|
||||
|
||||
### S6 — `UiButton`'s right-click addition does change existing behaviour for disabled buttons
|
||||
|
||||
`src/AcDream.App/UI/UiButton.cs:530-533`:
|
||||
|
||||
```csharp
|
||||
case UiEventType.RightClick:
|
||||
if (!Enabled) return true;
|
||||
OnRightClick?.Invoke();
|
||||
return OnRightClick is not null;
|
||||
```
|
||||
|
||||
Before this commit `UiEventType.RightClick` fell through to `default: return
|
||||
false` (`:547-548`), i.e. unhandled → `UiRoot.BubbleEvent` continued to the
|
||||
parent. Now a **disabled** `UiButton` returns `true` and swallows the
|
||||
right-click. The enabled-with-no-handler case is unchanged (`false`), so the
|
||||
blast is narrow, and the new shape is arguably *more* consistent with the
|
||||
`Click` case above it (`:521`), which already swallows when disabled. But the
|
||||
XML doc's claim at `:52-54` — "every pre-existing `UiButton` is unaffected …
|
||||
it does not change any existing click/drag behavior" — is not accurate.
|
||||
Either return `false` when `OnRightClick` is null regardless of `Enabled`, or
|
||||
correct the doc and name the disabled-swallow as intentional.
|
||||
|
||||
---
|
||||
|
||||
## NOTE
|
||||
|
||||
**N1 — the `0x26000000` / `0x27` resolution is correct, and better-anchored
|
||||
than its own citation.** `RetailActionMap.cs:14-16` calls it "empirically"
|
||||
resolved. It is in fact hard-anchored in the decomp:
|
||||
`docs/research/named-retail/acclient_2013_pseudo_c.txt:34743` shows the
|
||||
`MasterDBMap::sm_DBTypeDefHash` registration `var_38 = 0x26000000;
|
||||
var_34_39 = 0x2600ffff;` with extension `".actionmap"`, and `:30960`
|
||||
(`0x0041c6a0`) shows the DID→type dispatch returning `0x27` for
|
||||
`arg2 >= 0x26000000 && arg2 <= 0x2600ffff`. So "`0x27` is the type tag, not
|
||||
the DID high byte; the object lives in `0x26000000..0x2600FFFF`" is verified,
|
||||
not merely observed. Add the citation per the project's retail-anchor rule.
|
||||
The narrower "single instance" claim remains probe-only.
|
||||
|
||||
**N2 — the union-merge order-independence claim is asserted, not pinned.**
|
||||
`Read_UnionMergesBothMasterMapsWithoutOrderDependency`
|
||||
(`RetailActionMapReaderTests.cs:64-139`) builds a **synthetic** disjoint shape
|
||||
and asserts the union picks up both sides. It does not verify the actual
|
||||
premise — that the two shipped master maps' action sets under their one shared
|
||||
context (`0x5`) are disjoint. That premise is load-bearing: retail runs
|
||||
`CMasterInputMap::Merge(&m_InputMap, o, 1)` whose overlap semantics (union vs
|
||||
overwrite) were never traced (research §5.6), and union == overwrite only
|
||||
while the sets are disjoint. `Read_AgainstInstalledDats_MatchesPinnedShape`
|
||||
(`:209-247`) pins row counts and one spot-label but not disjointness. One
|
||||
extra live-DAT assertion would close it.
|
||||
|
||||
**N3 — the six-box grouping matches lane D's retail six exactly, and the
|
||||
reused-id trap is handled correctly.** `KeyboardConfigController.cs:131-139`
|
||||
lists `0x1000049D` Movement, `0x1000049F` Camera, `0x100004A1` Combat,
|
||||
`0x100004A3` UI, `0x10000211` CharacterSettings, `0x100004A5` Emote — the
|
||||
same six containers and the same six `ActionClass` keys as research §5.1.
|
||||
I verified against the committed fixture that ListBox `0x10000025` and
|
||||
scrollbar `0x10000026` each appear **6 times**, so the per-page
|
||||
`UiElement.FindDescendant` scoping (`:214`, `:222`, `:230`) is both correct
|
||||
and necessary; a flat `layout.FindElement` would have collapsed all six pages
|
||||
onto one. The empirical `ActionClass` derivation (1/2/3/4/5/7, no 6) is pinned
|
||||
by per-class row counts in the live-DAT test (`:232-237`).
|
||||
|
||||
**N4 — the identity byte-comparison is a permanent TEST, not a one-time
|
||||
check, and the three surfaced `RetailDefaults()` gaps are correctly left
|
||||
alone.** `RetailActionIdentityRoundTripTests.MappedActions_DatUnionDefaultBindings_MatchRetailDefaults`
|
||||
compares the DAT default union against `RetailDefaults()` for every mapped
|
||||
action, with an explicit citation-bearing allowlist
|
||||
(`:67-102`) for the three known disagreements — the walk-mode Shift echo, the
|
||||
ten `CameraAlternateControls` arrows, and the Quickslot Ctrl+N ambiguity. All
|
||||
three are documented in that test's class doc (`:20-60`) with reasoning, and
|
||||
none patches the foundational `KeyBindings.cs`. That is the right call. (The
|
||||
camera entry's *reasoning* is nonetheless undermined by M2.)
|
||||
|
||||
**N5 — the "no Clear button" finding is verified.** I confirmed it directly
|
||||
against the committed fixture: row template `0x1000002F`
|
||||
(`keyboard_config_21000009.json:30050`) has `Type = 268435508 = 0x10000034`
|
||||
(`UIOption_ActionKeyMap`) and exactly three direct element children —
|
||||
`0x10000030`, `0x10000031`, `0x10000032`. No fourth Clear child. So the
|
||||
controller's claim at `:52-59` — that `m_buttonClear` exists in the C++ class
|
||||
but the shipped 2013 layout authors no affordance for it — holds against the
|
||||
data.
|
||||
|
||||
**N6 — modal capture goes through the real `InputDispatcher`, and is
|
||||
keyboard-only.** `RetailUiRuntime.cs:2344-2345` calls
|
||||
`dispatcher.BeginCapture` directly, and `InputDispatcher.OnKeyDown`
|
||||
(`:470-488`) suppresses the action, ignores modifier-only keys, and returns
|
||||
`default(KeyChord)` on Escape — which the controller maps to null and treats
|
||||
as a silent cancel (`KeyboardConfigController.cs:385`). That is the real
|
||||
mechanism, correctly used. But capture always constructs `Device: 0`
|
||||
(`InputDispatcher.cs:483`), so a mouse chord can never be captured — meaning
|
||||
`CameraInstantMouseLook`'s `Device: 1` MMB-hold default can be *destroyed*
|
||||
through this screen (M1) but never restored through it.
|
||||
|
||||
**N7 — the refusal text degrades to complete silence.** If
|
||||
`ID_KeyMapCantOverwriteReadOnlyKeymap_Label` fails to resolve,
|
||||
`refusalText` is null → `NonBindableRefusalText: string.Empty`
|
||||
(`RetailUiRuntime.cs:2357`) → `DisplaySystemMessage` skips it (`:2352-2355`).
|
||||
Preferring silence over invented English is the right instinct and matches the
|
||||
header path (`KeyboardConfigController.cs:273-277`), but combined with M3 it
|
||||
means *both* conflict outcomes can be invisible. Worth a diagnostic line at
|
||||
mount when the string does not resolve. I could not independently byte-verify
|
||||
the resolved value `"Could not overwrite "` without running the DAT reader;
|
||||
the string *key* and its `.rdata` VA `0x007BEA8C` are confirmed in research
|
||||
§5.4.
|
||||
|
||||
**N8 — test coverage misses the risky seams.** Nineteen new tests, and the
|
||||
happy paths are well covered. Not covered: (a) activation/scope preservation —
|
||||
invisible by construction, since `FakeBindings.SetForAction`
|
||||
(`KeyboardConfigControllerTests.cs:73-77`) takes chords only, exactly like the
|
||||
production seam; (b) the camera dual-row alias — every test uses at most one
|
||||
camera row (`:102`); (c) the production capture lambda in `RetailUiRuntime`
|
||||
(`chord == default ? null : chord`), which is only exercised through the fake;
|
||||
(d) `RetailUnmappedKeyBindings.LoadOrEmpty`/`SaveToFile` round-trip — grep
|
||||
shows **no** test file references that type at all; (e) `RetailScanCodeMap` is
|
||||
exercised only transitively by the skip-if-no-DAT round-trip test. Both
|
||||
live-DAT tests `return` silently rather than using a real skip
|
||||
(`RetailActionMapReaderTests.cs:213`,
|
||||
`RetailActionIdentityRoundTripTests.cs:108`) — consistent with the project's
|
||||
`ConformanceDats` convention, but it means CI proves neither the `0x26000000`
|
||||
DID nor the 306-row shape.
|
||||
|
||||
**N9 — `OnShown`/`OnHidden` are never wired.** `KeyboardConfigController` does
|
||||
not implement `IRetainedPanelController`, so its `OptionPage` never gets
|
||||
retail's visibility-driven re-seed/revert (`OptionPageModel.cs:719-746`).
|
||||
Rows are seeded once at mount and never re-read from live bindings. Within a
|
||||
session the state stays self-consistent, and `Chrome = Imported` means the
|
||||
window has no close affordance other than OK/Cancel, so there is no silent
|
||||
"close without committing" path — but it does diverge from the OnShown
|
||||
discipline OP4's MUST-FIX 1 established for the sibling pages.
|
||||
|
||||
**N10 — `UiTemplateListBox`'s class doc is now stale.** It states "The Options
|
||||
panel's three ListBoxes (Character/Config/Chat, Campaign OP slice OP4+) are
|
||||
the only elements a controller will ever call `AddItemFromTemplateList`
|
||||
against" (`UiTemplateListBox.cs:33-35`). OP8 adds six more.
|
||||
|
||||
**N11 — register rows AP-202/203/204 are accurate; the OP3 INERT contract is
|
||||
properly retired.** AP-202 (`.keymap` interchange, Load/Save-As inert),
|
||||
AP-203 (unmapped rows), AP-204 (silent auto-reassign + OK/Cancel left-click)
|
||||
each describe what the code actually does, including AP-204 stating plainly
|
||||
that retail shows a modal confirm first. `OptionsPanelController.cs:194-199`
|
||||
replaces the INERT comment with a real `BindButton`, `OpenConfigureKeyboard`
|
||||
is threaded from `RetailUiRuntime.cs:2062`, and the test script's §OP8 opens
|
||||
by stating the OP3 INERT line no longer applies. The bookkeeping is clean —
|
||||
the problem is M3(b)/(c), that AP-204 records a narrowing the contract did not
|
||||
authorise and the script was edited to match.
|
||||
|
||||
**N12 — Escape while this screen is open quits the client.**
|
||||
`GameplayInputCommandController.HandleEscape` (`:260-269`) falls through to
|
||||
`_window.Close()`. During capture the dispatcher consumes Escape first, so the
|
||||
exposure is only between captures — pre-existing behaviour, not an OP8
|
||||
regression, but an 800×600 screen that looks modal and whose Escape exits the
|
||||
game is worth the user knowing before the connected gate.
|
||||
|
||||
**N13 — merge-readiness: the review brief's premise about `ff577641` is
|
||||
wrong, but nothing blocks the merge.** `ff577641` is **not** the current
|
||||
main-tree tip and is not an ancestor of `main`. It is the tip of
|
||||
`claude/latest-commits-cb0c8f` (worktree `eloquent-hugle-42119e`), the campaign
|
||||
branch. `main` is at `852cdda7`; the merge-base is `6bb4cfa7`, so the campaign
|
||||
branch is 41 commits ahead and `main` has drifted **2** commits since:
|
||||
`c6bc2bf7` (`tests/.../Streaming/LandblockBuildOriginTests.cs`, +16) and
|
||||
`852cdda7` (`docs/ISSUES.md`). Neither file is touched by OP8 — zero overlap.
|
||||
Within the campaign, OP8's shared-file edits are all additive and should merge
|
||||
cleanly: `OptionPageModel.cs` appends `ActionKeyMapOptionRow` after OP5/OP6's
|
||||
leaves; `RetailUiRuntime.cs` adds a record, a mount call and a mount method;
|
||||
`OptionsPanelController.cs` is one parameter plus one `BindButton`;
|
||||
`UiButton.cs` / `InputDispatcher.cs` / `WindowNames.cs` are one member each.
|
||||
The one thing the coordinator should watch is `OptionPageModel.cs`, which
|
||||
OP5, OP6 and OP8 all append to.
|
||||
|
||||
---
|
||||
|
||||
## Summary table
|
||||
|
||||
| ID | Class | One line |
|
||||
|---|---|---|
|
||||
| M1 | MUST-FIX | `SetForAction` drops `ActivationType`/`InputScope`; Defaults collapses ~140 actions and OK persists it |
|
||||
| M2 | MUST-FIX | Camera ctx `0x5`/`0x6` alias one `InputAction` → 20 rows / 10 targets, stale models, mutual clobber |
|
||||
| M3 | MUST-FIX | Auto-reassign wired silent in production; contract asked for a prompt; gate script rewritten to accept it; test asserts a message production never emits |
|
||||
| S1 | SHOULD-FIX | Conflict scan is first-match; non-bindable check ordered after row match, inverting retail |
|
||||
| S2 | SHOULD-FIX | `ActionMap.ConflictingMaps` never read → flat conflict universe, false cross-context conflicts |
|
||||
| S3 | SHOULD-FIX | `Save` lacks the try/catch the existing `RuntimeKeyBindingTarget` writer uses |
|
||||
| S4 | SHOULD-FIX | Assigning "Mapping 3" on a sparse row lands on Mapping 1 |
|
||||
| S5 | SHOULD-FIX | ~330 uncached DAT layout imports at startup for a rarely-opened screen |
|
||||
| S6 | SHOULD-FIX | Disabled `UiButton` now swallows right-click; the "unaffected" doc claim is inaccurate |
|
||||
| N1–N13 | NOTE | See above — N1/N3/N4/N5/N6 are verifications that **passed**; N2/N7–N12 are gaps; N13 is merge state |
|
||||
Loading…
Add table
Add a link
Reference in a new issue