docs: OP8 re-review — REOPEN (narrow): M1/M2/M3 closed, doc+display residuals
All three MUST verified mechanically: the one-pair-per-action capture
claim PROVEN against RetailDefaults() (9 multi-binding actions, no
counterexample); the identity map now injective (all 122 entries
checked); the confirm dialog real, N-way, accept-only, with the lazy
read's null unreachable in production. The OK/Cancel gesture divergence
is honestly retired (no distinguishing retail affordance = no Risk
symptom to write).
Coordinator third-round residuals: R1 — S2's deferral is commit-message-
only and M2 worsened it (0x6's DAT defaults are the arrow keys, so
Defaults + the flat conflict universe makes 'Move Forward -> Up' trip a
false N-way confirm; the gate script carries no warning). R2 — AP-203's
row body doesn't cover the ten same-verb-sibling-live rows. SHOULD: the
ten Camera Alternate rows render BLANK (retail shows the arrow keys);
identity-map injectivity is load-bearing for M1+M2 with nothing
enforcing it. MERGE NOTE: textually clean vs the campaign tip, but
OP8's six ListBoxes sit on the #372 viewport path — blank on the branch,
auto-healed by merging onto 057d8cd7+; the OP8 gate runs POST-merge.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
075ade1a6c
commit
7133aadc65
1 changed files with 414 additions and 0 deletions
414
docs/research/2026-08-11-op8-rereview.md
Normal file
414
docs/research/2026-08-11-op8-rereview.md
Normal file
|
|
@ -0,0 +1,414 @@
|
||||||
|
# Campaign OP slice OP8 — focused re-review of the rework
|
||||||
|
|
||||||
|
**Reviewed:** worktree `C:\Users\erikn\source\repos\acdream\.claude\worktrees\op8-keyboard`,
|
||||||
|
branch `op8-keyboard`, rework commit `b1968ce9`, parent `b4edee97`, campaign
|
||||||
|
base `ff577641`.
|
||||||
|
**Against:** `docs/research/2026-08-11-op8-review.md` (REJECT, 3 MUST-FIX + 6 SHOULD-FIX).
|
||||||
|
**Mode:** read-only. No build, no test, no launch. Every finding is
|
||||||
|
source-derived with `file:line` evidence.
|
||||||
|
|
||||||
|
## Verdict: **REOPEN (narrow — documentation-scoped)**
|
||||||
|
|
||||||
|
**All three original MUST-FIX are genuinely CLOSED**, and I verified each by
|
||||||
|
walking the mechanism, not by reading the commit message. All four addressed
|
||||||
|
SHOULD-FIXes (S1/S3/S4/S6) are closed too. The rework is good work.
|
||||||
|
|
||||||
|
Reopening is on **two residuals the M2 choice itself created** plus **the
|
||||||
|
deferral bookkeeping**, all of which are doc-scale and none of which require
|
||||||
|
re-touching the mechanism:
|
||||||
|
|
||||||
|
- The M2 carve-out is a **new** retail deviation. The register's *section
|
||||||
|
header* names it; the **AP-203 row body does not**, and that row's "Risk"
|
||||||
|
column describes a materially different symptom. The project's register rule
|
||||||
|
("any commit that introduces a deviation adds its register row IN THE SAME
|
||||||
|
COMMIT") is binding.
|
||||||
|
- The M2 carve-out **widens S2's deferred false-conflict surface onto the four
|
||||||
|
arrow keys** — the most commonly rebound keys on the screen — and the gate
|
||||||
|
script still carries **no S2 warning at all**. The connected gate will
|
||||||
|
produce a false defect report.
|
||||||
|
|
||||||
|
Fixing R1+R2 is two documentation edits. R3 is one judgement call.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Closure check — the three MUST-FIX
|
||||||
|
|
||||||
|
### M1 — activation/scope preservation: **CLOSED**
|
||||||
|
|
||||||
|
The seam now carries `Binding`, not `KeyChord`
|
||||||
|
(`KeyboardConfigController.cs:181-195`; production
|
||||||
|
`RetailUiRuntime.cs:2336-2344`). Each row captures the pair once at build from
|
||||||
|
the action's first live binding, falling back to `Press`/`Game` only when the
|
||||||
|
action starts wholly unbound (`KeyboardConfigController.cs:362-367`), and
|
||||||
|
reapplies it at the single write boundary
|
||||||
|
(`KeyboardConfigController.cs:374-386`).
|
||||||
|
|
||||||
|
**I walked the original M1 scenario end to end for a `Hold` + `MeleeCombat`
|
||||||
|
action.** `template` is a local captured by the `apply` closure, and every
|
||||||
|
write path funnels through that one closure:
|
||||||
|
|
||||||
|
| Path | Route | Activation/Scope |
|
||||||
|
|---|---|---|
|
||||||
|
| Rebind | `ApplySlot` → `ReplaceSlotValue` → `SetCurrentValue` → `_apply` (`OptionPageModel.cs:566-571`) | preserved |
|
||||||
|
| Erase | `EraseSlot` → same | preserved |
|
||||||
|
| Defaults | `WireScreenButtons` (`:557-565`) → `Page.Defaults()` (`OptionPageModel.cs:712-717`) → `RestoreDefaultValue` → `_apply` (`:583-587`) | preserved |
|
||||||
|
| Revert / Cancel | `Page.Reset()` → `RestoreSavedValue` → `_apply` (`:577-581`) | preserved |
|
||||||
|
| OK | `Page.Apply()` → `SaveCurrentValue` (no `_apply`) then `Save()` | nothing written — nothing to lose |
|
||||||
|
|
||||||
|
**Spot-verification of the one-pair-per-action justification against
|
||||||
|
`KeyBindings.RetailDefaults()` itself (`KeyBindings.cs:147-392`): the claim
|
||||||
|
holds.** I enumerated every action bound more than once and every action
|
||||||
|
carrying a non-default `Activation`/`Scope`:
|
||||||
|
|
||||||
|
- Multi-binding actions: `MovementForward` (W/Up), `MovementBackup` (X/Down),
|
||||||
|
`MovementTurnLeft` (A/Left), `MovementTurnRight` (D/Right),
|
||||||
|
`MovementStrafeLeft` (Z/Alt+A/Alt+Left), `MovementStrafeRight`
|
||||||
|
(C/Alt+D/Alt+Right), `MovementWalkMode` (ShiftLeft/ShiftRight, **both**
|
||||||
|
`Hold`, `:173-174`), `CreateShortcut` (`0`/`Ctrl+0`, `:247-248`),
|
||||||
|
`CameraActivateAlternateMode` (KeypadDivide/F2, `:298-299`). Every one of
|
||||||
|
these shares a single pair across all its bindings.
|
||||||
|
- Non-default-pair actions are **all single-binding**: the 5 melee
|
||||||
|
(`:257-264`), 5 missile (`:269-273`), 13 magic + 9 `UseSpellSlot_*`
|
||||||
|
(`:274-288`), `CameraInstantMouseLook` (`:303-306`), `SelectRight`
|
||||||
|
(`:326-329`), the three `SelectDbl*` (`:333-341`), `AcdreamRmbOrbitHold`
|
||||||
|
(`:386-389`).
|
||||||
|
|
||||||
|
**No action in `RetailDefaults()` has per-binding differing activation or
|
||||||
|
scope.** The capture-once model is sound against the production default set.
|
||||||
|
See N9 for the one shape that *can* express it (a hand-edited `keybinds.json`)
|
||||||
|
and R4 for what makes the model fragile going forward.
|
||||||
|
|
||||||
|
The two new tests pin the two paths that mattered
|
||||||
|
(`KeyboardConfigControllerTests.cs`: `DefaultsButton_PreservesActivationAnd
|
||||||
|
Scope_ForAHoldScopedAction`, `CancelButton_PreservesActivationAndScope`), and
|
||||||
|
they assert the `Binding` fields, not just the chords — the exact gap N8 called
|
||||||
|
out as invisible-by-construction.
|
||||||
|
|
||||||
|
### M2 — camera aliasing: **CLOSED mechanically**, with two carried consequences (R1, R2, R3)
|
||||||
|
|
||||||
|
Only ctx `0x5` maps now (`RetailActionIdentityTable.cs:91-121`); the `foreach
|
||||||
|
(uint ctx in new uint[] { 0x5, 0x6 })` loop is gone.
|
||||||
|
|
||||||
|
- **No dual rows over one action.** I checked the *whole* map for remaining
|
||||||
|
many-to-one entries, not just camera: 122 `M(...)` calls, every
|
||||||
|
`InputAction.*` value appearing exactly once. The map is now injective, so
|
||||||
|
the three consequences the original M2 listed (identical stale chords, mutual
|
||||||
|
clobber, self-conflict) are all structurally gone.
|
||||||
|
- **The `0x6` rows genuinely take the store-only path end to end.**
|
||||||
|
`TryResolve` misses → `mapped == false` →
|
||||||
|
`initial = CurrentForUnmapped((0x6, actionId))` and writes go to
|
||||||
|
`SetForUnmapped` (`KeyboardConfigController.cs:354-386`), which lands in
|
||||||
|
`RetailUnmappedKeyBindings` and the sibling `keybinds-unmapped.json`. The new
|
||||||
|
test asserts exactly this both ways
|
||||||
|
(`CameraContext5And6_AreIndependentRows_NotAliased`).
|
||||||
|
- **The retired allowlist entries are correct.** Ten camera entries removed
|
||||||
|
from `KnownRetailDefaultsDisagreements`
|
||||||
|
(`RetailActionIdentityRoundTripTests.cs:62-72`), with the class doc rewritten
|
||||||
|
from "three disagreements" to "two". With ctx `0x6` no longer in the union,
|
||||||
|
ctx `0x5`'s twelve camera actions match `RetailDefaults()` exactly — that
|
||||||
|
follows directly from the `RetailDefaults()` camera block (`:298-317`), which
|
||||||
|
is purely the numpad scheme.
|
||||||
|
|
||||||
|
**On the honesty question the brief asks.** Store-only-but-bindable is a
|
||||||
|
*weaker* deception here than for the other AP-203 rows, and a *stronger* one:
|
||||||
|
|
||||||
|
- Weaker, because the two schemes land under **different headers** —
|
||||||
|
`ID_InputMap_CameraControls` and `ID_InputMap_CameraAlternateControls`
|
||||||
|
(`RetailActionMap.cs:144-145`) — so the user is not looking at two
|
||||||
|
identically-framed rows.
|
||||||
|
- Stronger, because unlike the emote and CharacterSettings rows AP-203's Risk
|
||||||
|
column describes, **a live consumer for these verbs exists and works** —
|
||||||
|
the sibling ctx-`0x5` row on the same page drives it. "Rotate Camera Left"
|
||||||
|
works in one group and is inert in the other. That is a different symptom
|
||||||
|
from "this row has no consumer anywhere."
|
||||||
|
|
||||||
|
So: **AP-203's wording does not cover these ten.** See R2.
|
||||||
|
|
||||||
|
### M3 — conflict dialog: **CLOSED**
|
||||||
|
|
||||||
|
`ConfirmOverwrite` is wired to the real `RetailDialogFactory.MakeConfirmation`
|
||||||
|
seam (`RetailUiRuntime.cs:2374-2388`), the same one
|
||||||
|
`GameplayConfirmationController` uses (`:2531-2533`). Reassign happens **only**
|
||||||
|
on accept, and the N-way collection is applied whole
|
||||||
|
(`KeyboardConfigController.cs:450-468`). Decline returns without touching a
|
||||||
|
single row — pinned by
|
||||||
|
`Capture_ConflictWithAnotherRow_DeclineLeavesBothRowsUnchanged`. AP-204 is
|
||||||
|
struck through and given a retirement note (register `:189`); gate script step 9
|
||||||
|
is reverted to the confirm-prompt behaviour and now also exercises the decline
|
||||||
|
path (`2026-08-11-campaign-op-test-script.md:913-921`).
|
||||||
|
|
||||||
|
**The lazy read cannot NPE.** `RetailUiRuntime.cs:2383` guards
|
||||||
|
`if (DialogFactory is null) { onResult(false); return; }` before the call.
|
||||||
|
|
||||||
|
**Is a null read reachable?** Two cases, both handled:
|
||||||
|
|
||||||
|
1. *Ordering.* `MountKeyboardConfig()` at `Initialize():401` runs before
|
||||||
|
`MountDialogFactory()` at `:404`. But the closure is only invoked from
|
||||||
|
`BeginSlotCapture`'s capture callback, which needs a user keypress on a
|
||||||
|
window mounted `Visible = false` (`:2408`) and opened via a toolbar/Options
|
||||||
|
path that requires `Initialize()` to have returned. Not reachable.
|
||||||
|
2. *Degradation.* If `RetailDataIdResolver.Resolve(dats, 2, 5)` returns 0,
|
||||||
|
`MountDialogFactory` logs and returns with `DialogFactory` left null
|
||||||
|
(`:2506-2510`). Then every conflict silently declines forever. Not a crash,
|
||||||
|
but see N3 — that state has no diagnostic at the capture site.
|
||||||
|
|
||||||
|
**On the OK/Cancel left-click vs right-click-release retirement claim.** The
|
||||||
|
rework moved it from a register row to a code comment
|
||||||
|
(`KeyboardConfigController.cs:575-579`) on the argument that it is
|
||||||
|
zero-observable. **I agree, and the register's own convention supports it.**
|
||||||
|
Retail's `idMessage 0x19` right-click-release on this button pair carries no
|
||||||
|
distinguishing visual affordance — there is no cursor change, no hover cue, no
|
||||||
|
label difference that would let a user *see* which gesture the button wants.
|
||||||
|
The only observable would be "right-click does nothing here" — and after S6,
|
||||||
|
right-click on a handler-less `UiButton` bubbles to the parent exactly as it
|
||||||
|
did before OP8, so nothing surprising happens either. The register's Risk
|
||||||
|
column is defined as "the symptom you'd observe"; there is no symptom to write.
|
||||||
|
This one does not need the row the original review implied.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## SHOULD-FIX verification
|
||||||
|
|
||||||
|
| ID | Status | Evidence |
|
||||||
|
|---|---|---|
|
||||||
|
| **S1** | **CLOSED** | `FindConflicts` (`KeyboardConfigController.cs:525-545`) runs the non-user-bindable scan first and returns immediately (`:527-535`), then collects **every** matching row into a list (`:537-543`) instead of returning first-match. `BeginSlotCapture`'s switch (`:437-473`) honours the ordering, and the N-way set is both named in the dialog message (`:454-457`) and cleared in full on accept (`:461-465`). Two tests pin it, including the both-conflicts-exist ordering case (`Capture_ConflictWithBothARowAndANonBindableAction_NonBindableWins`). |
|
||||||
|
| **S3** | **CLOSED** | `RetailUiRuntime.cs:2351-2367` wraps both writes in try/catch + `Console.WriteLine`, matching `RuntimeKeyBindingTarget.Apply`'s discipline (and its reasoning about not rolling back the live binding). The IO throw no longer escapes `UiButton.OnClick` into `UiRoot.BubbleEvent`. |
|
||||||
|
| **S4** | **CLOSED (in-session)** | `ReplaceSlotValue` (`:496-509`) now trims only *trailing* empties via a `lastReal` scan, so `ApplySlot`'s padding survives and "Mapping 3" on an empty row lands on display index 2. `EraseSlot` (`:486-494`) early-returns on an already-empty slot instead of `RemoveAt`-shifting later bindings. `apply` filters interior `default(KeyChord)` at the write boundary (`:379`), so no padding leaks into a persisted `Binding`. Both pinned by tests. See N5 for the one thing S4 cannot fix. |
|
||||||
|
| **S6** | **CLOSED** | `UiButton.cs:530-543` now returns `false` (unhandled → bubbles) when `OnRightClick` is null regardless of `Enabled`, so the disabled-swallow the original review found is gone and the property's own doc claim (`:52-58`, "every pre-existing `UiButton` is unaffected … it does not change any existing click/drag behavior") is now **accurate**. OP8's key buttons always set the handler, so the erase path is unaffected. |
|
||||||
|
| **S2** | **DEFERRED — not honestly recorded** | See R1. `ConflictingMaps` appears nowhere in the branch except `RetailActionMapReaderTests.cs` (as a test-fixture construction detail) and the commit message. No register row, no `docs/ISSUES.md` entry, no plan-ledger note, no gate-script warning. |
|
||||||
|
| **S5** | **DEFERRED — not recorded** | See R5. `MountKeyboardConfig` is still unconditional at `RetailUiRuntime.cs:401`; nothing durable records the deferral. |
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Residuals
|
||||||
|
|
||||||
|
### MUST-FIX
|
||||||
|
|
||||||
|
#### R1 — S2's deferral is recorded only in a commit message, and the M2 carve-out just made its blast radius worse. The gate script does not warn.
|
||||||
|
|
||||||
|
The conflict scan is a flat universe over all 306 rows
|
||||||
|
(`KeyboardConfigController.cs:537-543` iterates `_rows` with no InputMap
|
||||||
|
filter). Two live consequences at the OP8 connected gate:
|
||||||
|
|
||||||
|
1. **Shared combat keys (pre-existing, called out by the original S2).**
|
||||||
|
Insert / PageUp / Delete / End / PageDown are each bound in all three of
|
||||||
|
`MeleeCombat`, `MissileCombat` and `MagicCombat` by design
|
||||||
|
(`KeyBindings.cs:257-282`), under distinct DAT contexts
|
||||||
|
`0x10000003/4/5`. Rebinding any of them now opens a confirm naming two
|
||||||
|
sibling rows retail would never consider conflicting — and **accepting
|
||||||
|
erases both**, where before M3 it silently erased one. M3 makes it visible
|
||||||
|
and declinable, which is strictly better, but it is still a false conflict.
|
||||||
|
2. **Arrow keys (NEW — introduced by the M2 carve-out).** ctx `0x6`'s DAT
|
||||||
|
defaults are the arrow cluster (the rework's own test seeds
|
||||||
|
`RetailKeyChord(0xCB, …)` = `DIK_LEFT` for `(0x6, 0x35)`;
|
||||||
|
`KeyboardConfigControllerTests.cs`). Before M2 those rows displayed the
|
||||||
|
*mapped* action's numpad chords, so they never claimed an arrow. After M2
|
||||||
|
they are unmapped, and **clicking Defaults writes the arrows into the
|
||||||
|
unmapped store** (`RestoreDefaultValue` → `apply` →
|
||||||
|
`SetForUnmapped`). From that point Left/Right/Up/Down are held by both the
|
||||||
|
Movement rows (`KeyBindings.cs:153-165`) *and* ten Camera Alternate rows —
|
||||||
|
so rebinding "Move Forward" to Up-arrow trips an N-way confirm naming a
|
||||||
|
camera row, and accepting erases it. Gate script step 12 explicitly
|
||||||
|
instructs the user to click Defaults
|
||||||
|
(`2026-08-11-campaign-op-test-script.md:987-990`).
|
||||||
|
|
||||||
|
The gate script's "What to report" section (`:983-1001`) has no S2 line. The
|
||||||
|
user will file this as a defect.
|
||||||
|
|
||||||
|
**Fix (docs only):** (a) a register row for the flat-conflict-universe
|
||||||
|
divergence — retail consults `ActionMap.ConflictingMaps` /
|
||||||
|
`ICIDM::FindConflictingInputMaps`, we do not — or a `docs/ISSUES.md` entry
|
||||||
|
referenced from the plan ledger; and (b) an explicit warning in the gate
|
||||||
|
script's §OP8 "What to report", naming the combat cluster and the arrow keys as
|
||||||
|
*known* false conflicts so the user reports the *wording*, not the *existence*.
|
||||||
|
|
||||||
|
#### R2 — the M2 carve-out is a new deviation whose register row does not name it
|
||||||
|
|
||||||
|
`retail-divergence-register.md:178` (the AP section header) was amended to say
|
||||||
|
"all 10 CameraAlternateControls rows per the M2 de-alias fix". **The AP-203 row
|
||||||
|
itself (`:188`) was not.** Its body enumerates "82 of 87 Emote rows", "all 48
|
||||||
|
CharacterSettings hotkey rows", "Spell Slot 10-12, Quickslot 10-13", and
|
||||||
|
"roughly twenty UI-panel-toggle rows" — no camera. Its Risk column reads:
|
||||||
|
|
||||||
|
> A user rebinds e.g. an emote or a CharacterSettings hotkey … and the binding
|
||||||
|
> persists but has no observable in-game effect
|
||||||
|
|
||||||
|
That is not this symptom. For the camera ten, the verb **does** have a live
|
||||||
|
consumer that **does** work — through the sibling row in the group directly
|
||||||
|
above it on the same page. "Two rows, same page, same verb, one live one inert"
|
||||||
|
is a distinct observable and belongs in the Risk column, per the register's own
|
||||||
|
rule that the column is written as the symptom you would observe.
|
||||||
|
|
||||||
|
**Fix (docs only):** extend AP-203's row body + Risk column to name the ten
|
||||||
|
`(0x6, …)` rows and their distinct symptom, or file a dedicated AP row for the
|
||||||
|
carve-out. The section header alone does not satisfy the same-commit rule.
|
||||||
|
|
||||||
|
### SHOULD-FIX
|
||||||
|
|
||||||
|
#### R3 — the ten Camera Alternate rows render **blank**; retail shows the arrow keys
|
||||||
|
|
||||||
|
`BuildActionRow` seeds an unmapped row's display from the store only
|
||||||
|
(`KeyboardConfigController.cs:369-371`), and `RetailUnmappedKeyBindings.Get`
|
||||||
|
returns `Array.Empty<KeyChord>()` for an absent key
|
||||||
|
(`RetailUnmappedKeyBindings.cs:23-26`) — nothing anywhere seeds the store from
|
||||||
|
`row.DefaultBindings`. On a fresh install the "Camera Alternate Controls" group
|
||||||
|
shows ten rows with all three Mapping buttons blank. The rework's own test pins
|
||||||
|
this as intended (`Assert.Empty(ctx6.Model.Current)` and
|
||||||
|
`Assert.Empty(fake.Unmapped)`), and `RetailActionMapReaderTests.cs:233`
|
||||||
|
(`Assert.Equal(22, byClass[RetailActionClass.Camera])`) confirms all 22 rows do
|
||||||
|
render.
|
||||||
|
|
||||||
|
This is a *new* visual regression: before M2 those rows displayed chords (the
|
||||||
|
wrong ones, but chords).
|
||||||
|
|
||||||
|
**Watch the fix.** Naively seeding the display from DAT defaults creates an
|
||||||
|
erase-idempotency trap: erase → store empty → next open re-seeds from DAT →
|
||||||
|
the erase silently un-does itself. Any seeding needs a written-through
|
||||||
|
"materialised defaults" store entry, or an explicit empty marker. If the
|
||||||
|
coordinator judges that out of scope, this needs to be R1's third gate-script
|
||||||
|
warning line, because ten blank rows on the Camera tab will be reported.
|
||||||
|
|
||||||
|
#### R4 — the identity map's injectivity is now load-bearing and nothing enforces it
|
||||||
|
|
||||||
|
M1's capture-once model and M2's de-alias both assume **exactly one DAT row per
|
||||||
|
`InputAction`**. Break that assumption and the failure is silent: two rows
|
||||||
|
would again share one live target, each with its own captured
|
||||||
|
`(Activation, Scope)`, and the last writer wins. I verified injectivity by hand
|
||||||
|
over all 122 entries in `RetailActionIdentityTable.cs:70-253` — it holds today —
|
||||||
|
but there is no `Debug.Assert`, no `Bind`-time duplicate log, and no test.
|
||||||
|
`RetailActionIdentityRoundTripTests` checks DAT-default agreement, not
|
||||||
|
uniqueness of values.
|
||||||
|
|
||||||
|
**Fix:** one test asserting `Map.Values.Distinct().Count() == Map.Count`, or a
|
||||||
|
`Bind`-time warning when two `RowView`s resolve to the same `MappedAction`.
|
||||||
|
|
||||||
|
#### R5 — S5's deferral has no durable record either
|
||||||
|
|
||||||
|
`MountKeyboardConfig` is still unconditional (`RetailUiRuntime.cs:401`) and
|
||||||
|
`Bind` still builds all 306 rows + 19 headers eagerly, each through the
|
||||||
|
uncached `LayoutImporter.ImportInfos` path under `DatLock`
|
||||||
|
(`:2316-2330`). The deferral lives only in the commit message. Same fix shape
|
||||||
|
as R1(a): an `ISSUES.md` entry or a plan-ledger line.
|
||||||
|
|
||||||
|
#### R6 — the plan ledger's OP8 row is stale
|
||||||
|
|
||||||
|
`docs/plans/2026-08-10-options-panel-campaign.md:427` reads
|
||||||
|
`| OP8 | CODE-COMPLETE, gate READY | (this branch's commit) | not yet reviewed | …`.
|
||||||
|
Every sibling row carries real SHAs and a review chain (see OP5/OP6/OP7 at
|
||||||
|
`:424-426`). This is the coordinator's row to fill, but it is currently the
|
||||||
|
only durable place the S2/S5 deferrals could have been recorded and was not.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## NOTE
|
||||||
|
|
||||||
|
**N1 — merge overlap: textually clean, but OP8 has a hard *semantic* dependency
|
||||||
|
on `057d8cd7`.** The campaign branch gained three commits since `ff577641`:
|
||||||
|
`1e36d4a7` (the OP8 review doc), `c3ed32fb` and `057d8cd7` (#372). Changed
|
||||||
|
files: `docs/ISSUES.md`, `docs/research/2026-08-11-op8-review.md`,
|
||||||
|
`src/AcDream.App/UI/UiTemplateListBox.cs`,
|
||||||
|
`tests/…/OptionsPanelLiveMountProbeTests.cs`,
|
||||||
|
`tests/…/UiTemplateListBoxViewportTests.cs`. **OP8 touches none of them** — a
|
||||||
|
read-only `git merge-tree` over the merge-base reports zero conflicts.
|
||||||
|
|
||||||
|
But `057d8cd7` is the #372 fix: `UiTemplateListBox`'s lazily-created viewport
|
||||||
|
was constructed 0×0 with fill-anchors, which captured a degenerate anchor
|
||||||
|
baseline and made `UiScrollablePanel.LayoutScrollableChildren` cull every row —
|
||||||
|
so every ListBox-backed Options tab rendered **blank**. OP8 adds **six more**
|
||||||
|
`UiTemplateListBox` instances driven through the identical lazy-viewport path
|
||||||
|
(`KeyboardConfigController.cs:251-260`). **On `b1968ce9` as it stands, all six
|
||||||
|
Configure Keyboard pages will render blank.** The branch must be merged onto
|
||||||
|
(or rebased over) a campaign tip at or after `057d8cd7` before the OP8
|
||||||
|
connected gate, or the gate fails for a reason that has nothing to do with OP8.
|
||||||
|
|
||||||
|
**N2 — `ShowConfirmation` already existed with the exact shape the M3 wiring
|
||||||
|
needed.** `RetailUiRuntime.cs:588-595` is
|
||||||
|
`ShowConfirmation(string, Action<bool>)` doing precisely
|
||||||
|
`DialogFactory?.MakeConfirmation(msg, data => completed(data.GetBoolean(
|
||||||
|
RetailDialogProperty.ConfirmationResult)))`. The rework inlined a near-duplicate
|
||||||
|
at `:2380-2387`. The only behavioural difference is the null-factory case (the
|
||||||
|
inline version calls `onResult(false)`; `ShowConfirmation` returns `0u` and
|
||||||
|
never calls back — same net effect, no reassign). Worth collapsing.
|
||||||
|
|
||||||
|
**N3 — the degraded-DialogFactory state is silent at the capture site.** If the
|
||||||
|
dialog catalog fails to resolve, `MountDialogFactory` logs once at startup
|
||||||
|
(`:2506-2510`) and every subsequent conflict declines with no user-visible
|
||||||
|
feedback. Combined with the original N7 (the refusal string degrading to
|
||||||
|
silence via `NonBindableRefusalText: refusalText ?? string.Empty` at `:2373`),
|
||||||
|
**both** conflict outcomes can be invisible in that state. One diagnostic line
|
||||||
|
in the `DialogFactory is null` branch would close it.
|
||||||
|
|
||||||
|
**N4 — the confirm goes on the default dialog queue, and it is modal.**
|
||||||
|
`MakeConfirmation` uses `DefaultQueueKey` (`RetailDialogFactory.cs:125-136`),
|
||||||
|
so a gameplay confirmation already open queues the rebind confirm behind it.
|
||||||
|
That is fine because `CreateDialog` sets `_host.Modal = view.Root`
|
||||||
|
(`RetailDialogFactory.cs:279`) — which closes the reentrancy hole I went
|
||||||
|
looking for: the user cannot click OK/Cancel on the keyboard screen, or start a
|
||||||
|
second capture, while a rebind confirm is pending, so the deferred
|
||||||
|
`ApplySlot` can never land after the screen was cancelled or saved.
|
||||||
|
|
||||||
|
**N5 — S4's slot position is session-only.** `KeyBindings` has no slot concept,
|
||||||
|
so a chord placed on "Mapping 3" of an empty row persists as a single binding
|
||||||
|
and re-displays on "Mapping 1" after OK + relaunch (`BuildActionRow`'s
|
||||||
|
`initial` is a dense `Select(b => b.Chord)`, `:369-370`). Retail's own
|
||||||
|
`m_qclCurrent` is also a list, so this may match retail; either way it is
|
||||||
|
beyond what S4 asked for and worth knowing before the gate.
|
||||||
|
|
||||||
|
**N6 — an action with retail defaults cannot be persistently unbound, and OP8
|
||||||
|
is the first UI that exposes erase.** `KeyBindings.LoadOrDefault`'s
|
||||||
|
merge-over-defaults (`KeyBindings.cs:469-477`) re-adds `RetailDefaults()`
|
||||||
|
bindings for any action the loaded file has none for. So: right-click-erase all
|
||||||
|
three slots of "Move Forward", click OK, relaunch — W and Up are back. That is
|
||||||
|
pre-existing `KeyBindings` behaviour, not an OP8 defect, but gate script steps
|
||||||
|
11 and 14-16 walk the user right into it.
|
||||||
|
|
||||||
|
**N7 — `FindConflicts`' non-bindable scan is quadratic per capture.**
|
||||||
|
`KeyboardConfigController.cs:529-534` iterates every `InputAction`, runs
|
||||||
|
`RetailActionIdentityTable.Map.Values.Contains(candidate)` (a linear scan over
|
||||||
|
122 values) for each, and calls `CurrentForAction` — which in production is
|
||||||
|
`dispatcher.Bindings.ForAction(action).ToArray()`, allocating an array per
|
||||||
|
action. That is ~250 array allocations and ~30k comparisons per keypress in a
|
||||||
|
config screen: harmless, but a `HashSet` built once would be free.
|
||||||
|
|
||||||
|
**N8 — chords held by real-but-unmapped retail actions are hard-refused.** With
|
||||||
|
S1's reordering, any chord bound to an `InputAction` absent from the identity
|
||||||
|
table now refuses outright before the row scan. Two of those are *not*
|
||||||
|
acdream-only debug keys: `SelectionPickUp` (**F**) and
|
||||||
|
`ScrollUp`/`ScrollDown` (**Ctrl+Up** / **Ctrl+Down**) — all bound in
|
||||||
|
`RetailDefaults()` (`:184`, `:345-346`) but absent from the identity map's
|
||||||
|
`0x10000007` and `0x10000009` blocks. So the user cannot bind anything to F and
|
||||||
|
gets "Could not overwrite " with no explanation. Pre-existing (`b4edee97`), not
|
||||||
|
a rework regression, and arguably the honest behaviour — but it is a plausible
|
||||||
|
gate report.
|
||||||
|
|
||||||
|
**N9 — the capture-once model's one expressible counterexample.**
|
||||||
|
`keybinds.json` carries `activation` and `scope` **per binding**
|
||||||
|
(`KeyBindings.cs:433-459`), so a hand-edited file *can* give one action two
|
||||||
|
bindings with differing pairs; this row model would collapse them to the first
|
||||||
|
binding's pair on the next write. No acdream code path produces that shape, and
|
||||||
|
`SaveToFile` (`:499-519`) round-trips whatever it is given, so the exposure is
|
||||||
|
limited to manual editing. Worth one sentence in the class doc's M1 paragraph
|
||||||
|
(`KeyboardConfigController.cs:84-105`), which currently states the invariant as
|
||||||
|
a property of `RetailDefaults()` without noting the file format is wider.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Summary table
|
||||||
|
|
||||||
|
| ID | Class | One line |
|
||||||
|
|---|---|---|
|
||||||
|
| M1 | **CLOSED** | Full `Binding` seam; pair captured once and reapplied on rebind/Defaults/Cancel/Revert; one-pair-per-action verified against `RetailDefaults()` — no counterexample |
|
||||||
|
| M2 | **CLOSED** | ctx `0x6` de-mapped; map now injective (122/122 verified); `0x6` rows take the store-only path end to end; ten allowlist entries correctly retired |
|
||||||
|
| M3 | **CLOSED** | Real `MakeConfirmation` confirm before reassign, N-way, accept-only; lazy read cannot NPE and is unreachable-null in production; the OK/Cancel gesture note genuinely does not warrant a register row |
|
||||||
|
| S1 | **CLOSED** | Non-bindable checked first; all conflicting rows collected |
|
||||||
|
| S3 | **CLOSED** | `Save` try/catch matching `RuntimeKeyBindingTarget` |
|
||||||
|
| S4 | **CLOSED** | Trailing-only trim; empty-slot right-click is a no-op |
|
||||||
|
| S6 | **CLOSED** | `false` when no handler regardless of `Enabled`; doc claim now accurate |
|
||||||
|
| R1 | MUST-FIX | S2 deferral recorded only in a commit message; M2 added the arrow keys to the false-conflict surface; gate script has no warning → false defect at the gate |
|
||||||
|
| R2 | MUST-FIX | M2's carve-out is a new deviation; AP-203's *row* does not name it and its Risk column describes a different symptom (section header alone ≠ the same-commit rule) |
|
||||||
|
| R3 | SHOULD-FIX | Ten Camera Alternate rows render blank vs retail's arrow keys; no DAT-default seeding for unmapped rows (watch the erase-idempotency trap in any fix) |
|
||||||
|
| R4 | SHOULD-FIX | Identity-map injectivity is now load-bearing for M1 *and* M2 but unenforced — no assert, no test |
|
||||||
|
| R5 | SHOULD-FIX | S5 deferral has no durable record either |
|
||||||
|
| R6 | SHOULD-FIX | Plan ledger's OP8 row still says "not yet reviewed" / "(this branch's commit)" |
|
||||||
|
| N1 | NOTE | Merge is textually clean (zero overlap, `merge-tree` conflict-free) but OP8 **depends on** `057d8cd7` (#372) — without it all six keyboard pages render blank |
|
||||||
|
| N2–N9 | NOTE | Duplicate `ShowConfirmation`; silent degraded DialogFactory; queued-but-modal confirm (no reentrancy hole); session-only slot position; erase can't persist over `LoadOrDefault`'s merge; quadratic non-bindable scan; F / Ctrl+Arrow hard-refused; per-binding pairs expressible in JSON |
|
||||||
Loading…
Add table
Add a link
Reference in a new issue