erik/acdream
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 3

fix(ui): OP8 rework — activation/scope preservation, camera-row de-alias, conflict-confirm dialog

Browse source 
Fixes the three MUST-FIX findings from the 2026-08-11 combined dual-lens
review of commit b4edee97 (docs/research/2026-08-11-op8-review.md).

M1 — SetForAction destroyed ActivationType/InputScope on every write,
collapsing walk-mode's Hold, the three combat-scoped bindings, and
CameraInstantMouseLook's mouse chord the instant a row (including
Defaults, which touches all ~140 mapped rows at once) wrote back.
Widened the Bindings seam to carry the full Binding (chord + activation
+ scope), not a bare chord: KeyboardConfigController captures each
row's live Activation/Scope ONCE at build time (every multi-chord
action in KeyBindings.RetailDefaults() shares one pair across all its
bindings) and reapplies it on every write — rebind, Cancel/Revert, and
Defaults (which restores DAT-sourced KEYS only, never touches the
pair). New tests pin this across both Defaults and Cancel for a
Hold+MeleeCombat-scoped action.

M2 — InputMap 0x5 (CameraControls) and 0x6 (CameraAlternateControls)
aliased one InputAction each: both rows read/wrote the same live target,
so they showed identical stale chords, a rebind of one silently wiped
the other, and a row could conflict with its own twin. Building real
per-scheme dual-binding storage (or new InputAction members plus the
camera-dispatch code to consume them) is a feature, not a one-line fix.
Chose the third option: only ctx 0x5 — the scheme RetailDefaults()
actually has live support for — maps to InputAction; ctx 0x6 falls
through to the existing unmapped/store-only path (AP-203), fully
rendered, bindable, and persisted, honestly carrying no live effect.
This also retired 10 stale allowlist entries in the DAT-vs-
RetailDefaults() round-trip test: with the alias gone, ctx 0x5 alone
matches RetailDefaults() exactly for all twelve Camera actions.

M3 — the auto-reassign-on-conflict path was wired silent in production
(NotifyReassigned: _ => "") though the contract asked for a prompt and
retail confirms before overwriting (OpenOverwriteBindingDialog). Wired
a real confirm dialog through RetailDialogFactory.MakeConfirmation —
the same seam GameplayConfirmationController already uses — read
lazily since DialogFactory mounts after MountKeyboardConfig in
Initialize()'s order. Only reassigns on accept; decline leaves every
row untouched. AP-204 (which recorded the narrowing) is RETIRED; the
still-true OK/Cancel left-click-vs-right-click-release note moves to a
code comment (zero observable difference, doesn't warrant a register
row). Reverted the gate script's step 9 from documenting the silent
shape back to the real confirm-prompt behavior.

SHOULD-FIX addressed as one-liners in files already touched:
- S1: non-user-bindable conflicts are now checked BEFORE any row
  conflict (retail's own order), and ALL conflicting rows are collected
  (N-way), not just the first match.
- S3: Save wraps the file-write pair in the same try/catch
  RuntimeKeyBindingTarget.Apply already uses for keybinds.json.
- S4: assigning "Mapping 3" on a row with no existing bindings now
  lands on display index 2, not index 0 — ReplaceSlotValue trims only
  TRAILING empty slots instead of stripping every default(KeyChord).
  Right-click on an already-empty slot is now a no-op instead of
  shifting later bindings.
- S6: UiButton.OnRightClick returns false (unhandled, bubbles to
  parent) when no handler is set, disabled or not — matching the
  pre-existing behavior the class doc already claimed.

Left for a future pass (not one-liners): S2 (ActionMap.ConflictingMaps
is still unread — the conflict scan treats all 306 rows as one flat
universe instead of respecting the DAT's own legitimately-shared-key
table) and S5 (the ~330 DAT layout imports still run eagerly at mount
instead of lazily on first open).

19 KeyboardConfigControllerTests (was 12): +2 activation/scope
preservation (Defaults, Cancel), +1 camera de-alias, +2 confirm-dialog
accept/decline, +1 non-bindable-takes-priority-over-row-conflict, +1
sparse-row third-slot placement. Full solution suite 13,154 passed / 4
skipped / 0 failed (this round's baseline 13,147/4/0, zero regressions).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik 2026-08-11 09:53:10 +02:00
parent b4edee970f
commit b1968ce980
8 changed files with 521 additions and 163 deletions
Download patch file Download diff file Expand all files Collapse all files

224
src/AcDream.App/UI/Layout/KeyboardConfigController.cs
View file

@ -81,21 +81,44 @@ namespace AcDream.App.UI.Layout;
/// </para>
///
/// <para>
/// <b>Conflicts (research doc §5.4).</b> Retail's conflict model is N-way and
/// cross-input-map, with a DISTINCT refusal for a chord already bound to a
/// non-user-bindable action. This port scans every OTHER row on this screen
/// (the full user-bindable universe, since every DAT-sourced row is inherently
/// user-bindable — <see cref="RetailActionMapReader"/> already filtered out the
/// non-bindable ones) PLUS the live <see cref="KeyBindings"/> table for chords
/// bound to an acdream-only action with no <see cref="RetailActionIdentityTable"/>
/// row at all (Ctrl+M mute, the debug F-keys, ...) — those are this port's
/// "non-user-bindable" analogue (there is no retail row to reassign them from) and
/// refuse via <see cref="Bindings.NonBindableRefusalText"/> exactly like retail's
/// distinct <c>OpenCantOverwriteBindingDialog</c>. A genuine cross-row conflict
/// (register row — narrowed from retail's modal confirm-before-reassign) auto-
/// reassigns (erases the losing row's slot, applies the new one) and reports the
/// outcome via <see cref="Bindings.NotifyReassigned"/> rather than blocking on a
/// confirm dialog this slice does not build.
/// <b>Activation/Scope preservation (M1, 2026-08-11 review).</b> A mapped row's
/// <see cref="Bindings.CurrentForAction"/> read returns the FULL live
/// <see cref="Binding"/> list, not bare chords — a single acdream
/// <see cref="InputAction"/> consistently carries one <see cref="ActivationType"/>/
/// <see cref="InputScope"/> pair across every one of its bindings (verified
/// against every multi-chord action in <c>KeyBindings.RetailDefaults()</c>:
/// walk-mode's Hold, the three melee/missile/magic combat scopes, ...), so this
/// row captures that pair ONCE at build time (from the first live binding, or
/// <see cref="ActivationType.Press"/>/<see cref="InputScope.Game"/> if the action
/// starts wholly unbound) and reapplies it to every chord this row ever writes —
/// on a live rebind, on Cancel/Revert (<c>RestoreSavedValue</c>), and on Defaults
/// (<c>RestoreDefaultValue</c>, which restores DAT-sourced KEYS only; Activation/
/// Scope are retail-side properties of the ACTION, not of which physical key
/// triggers it, so Defaults must never touch them). Before this fix,
/// <c>SetForAction</c> reconstructed every <see cref="Binding"/> with the
/// constructor's bare defaults (<see cref="ActivationType.Press"/>/
/// <see cref="InputScope.Game"/>), so a single click of Defaults collapsed the
/// Hold/scope of every one of the ~140 mapped actions in one shot — walk-mode
/// stopped unlatching, melee/missile/magic combat holds stopped repeating, and
/// scope precedence broke for every chord shared across those three scopes by
/// design (Insert/Delete/End/PageUp/PageDown).
/// </para>
///
/// <para>
/// <b>Conflicts (research doc §5.4, reworked per M3/S1, 2026-08-11 review).</b>
/// Retail's conflict model is N-way and cross-input-map, with a DISTINCT refusal
/// for a chord already bound to a non-user-bindable action, checked BEFORE any
/// user-bindable conflict is even considered (retail refuses outright the instant
/// ANY conflicting target is non-user-bindable). This port's non-user-bindable
/// analogue is a chord already bound to an acdream-only action with no
/// <see cref="RetailActionIdentityTable"/> row at all (Ctrl+M mute, the debug
/// F-keys, ...) — refused via <see cref="Bindings.NonBindableRefusalText"/>
/// exactly like retail's distinct <c>OpenCantOverwriteBindingDialog</c>, with no
/// dialog (a hard stop, matching the DAT-verified refusal string). A genuine
/// cross-row conflict collects EVERY conflicting row (not just the first) and
/// opens a real confirm dialog through <see cref="Bindings.ConfirmOverwrite"/> —
/// retail's <c>OpenOverwriteBindingDialog(&amp;conflicts)</c> — BEFORE reassigning;
/// only on accept are the losing rows' slots erased and the new chord applied.
/// </para>
/// </summary>
public sealed class KeyboardConfigController
@ -152,10 +175,12 @@ public sealed class KeyboardConfigController
/// <summary>The live read/write/capture seam this screen writes bindings
/// through — mirrors every other Campaign OP page controller's
/// <c>Bindings</c> shape (a plain delegate record, no DAT/InputDispatcher
/// dependency baked into the controller itself).</summary>
/// dependency baked into the controller itself). <see cref="CurrentForAction"/>/
/// <see cref="SetForAction"/> carry the FULL <see cref="Binding"/> (chord +
/// activation + scope), not a bare chord — M1's fix (see class doc).</summary>
public sealed record Bindings(
Func<InputAction, IReadOnlyList<KeyChord>> CurrentForAction,
Action<InputAction, IReadOnlyList<KeyChord>> SetForAction,
Func<InputAction, IReadOnlyList<Binding>> CurrentForAction,
Action<InputAction, IReadOnlyList<Binding>> SetForAction,
Func<(uint InputMapId, uint ActionId), IReadOnlyList<KeyChord>> CurrentForUnmapped,
Action<(uint InputMapId, uint ActionId), IReadOnlyList<KeyChord>> SetForUnmapped,
Action<Action<KeyChord?>> BeginCapture,
@ -163,7 +188,11 @@ public sealed class KeyboardConfigController
Action Toggle,
Action<string> DisplaySystemMessage,
string NonBindableRefusalText,
Func<string, string> NotifyReassigned);
// M3 (2026-08-11 review): retail's OpenOverwriteBindingDialog — confirm
// BEFORE reassigning a chord already bound to another row on this screen.
// message is pre-composed (real row labels, no invented retail text);
// the callback receives the user's Yes(true)/No(false) choice.
Action<string, Action<bool>> ConfirmOverwrite);
public OptionPage Page { get; } = new();
public IReadOnlyList<RowView> Rows => _rows;
@ -325,17 +354,35 @@ public sealed class KeyboardConfigController
bool mapped = RetailActionIdentityTable.TryResolve(row.InputMapId, row.ActionId, out InputAction action);
InputAction? mappedAction = mapped ? action : null;
IReadOnlyList<KeyChord> initial = mapped
// M1: capture this row's live Activation/Scope ONCE, from the first
// existing binding for the action (every multi-chord action in
// KeyBindings.RetailDefaults() shares one Activation/Scope pair across
// all its bindings — see class doc). Falls back to the Binding record's
// own defaults (Press/Game) only when the action starts wholly unbound.
IReadOnlyList<Binding> liveBindings = mapped
? bindings.CurrentForAction(action)
: Array.Empty<Binding>();
(ActivationType Activation, InputScope Scope) template = liveBindings.Count > 0
? (liveBindings[0].Activation, liveBindings[0].Scope)
: (ActivationType.Press, InputScope.Game);
IReadOnlyList<KeyChord> initial = mapped
? liveBindings.Select(b => b.Chord).ToArray()
: bindings.CurrentForUnmapped((row.InputMapId, row.ActionId));
IReadOnlyList<KeyChord> defaults = DatDefaultsToChords(row.DefaultBindings);
var model = new ActionKeyMapOptionRow(initial, defaults, apply: value =>
{
// Interior/padding default(KeyChord) entries (S4 — sparse-slot
// display, see ReplaceSlotValue) are never real bindings; filter
// them out at the write boundary, not at storage time.
IReadOnlyList<KeyChord> real = value.Where(c => c != default).ToArray();
if (mapped)
bindings.SetForAction(action, value);
bindings.SetForAction(
action,
real.Select(c => new Binding(c, action, template.Activation, template.Scope)).ToArray());
else
bindings.SetForUnmapped((row.InputMapId, row.ActionId), value);
bindings.SetForUnmapped((row.InputMapId, row.ActionId), real);
});
Page.Register(model);
@ -369,7 +416,10 @@ public sealed class KeyboardConfigController
{
IReadOnlyList<KeyChord> current = view.Model.Current;
for (int i = 0; i < view.KeyButtons.Count; i++)
view.KeyButtons[i].Label = i < current.Count ? DescribeChord(current[i]) : null;
{
bool bound = i < current.Count && current[i] != default;
view.KeyButtons[i].Label = bound ? DescribeChord(current[i]) : null;
}
}
private static string DescribeChord(KeyChord chord)
@ -384,83 +434,114 @@ public sealed class KeyboardConfigController
{
if (captured is not { } chord) return; // Escape — retail cancels silently.
switch (FindConflict(chord, exclude: view))
(ConflictOutcome outcome, List<RowView> conflictRows) = FindConflicts(chord, exclude: view);
switch (outcome)
{
case ConflictKind.None:
break;
case ConflictKind.Row:
// A real cross-row conflict — auto-reassign (register row: retail
// confirms first via OpenOverwriteBindingDialog; this port narrows
// to reassign-then-notify rather than a blocking modal).
RowView conflictRow = _lastConflictRow!;
ReplaceSlotValue(conflictRow, RemoveChord(conflictRow.Model.Current, chord));
RefreshRowButtons(conflictRow);
bindings.DisplaySystemMessage(bindings.NotifyReassigned(conflictRow.Label ?? "?"));
break;
case ConflictKind.NonBindable:
// Bound to an acdream-only action with no DAT row at all (Ctrl+M
// mute, the debug F-keys, ...) — this port's analogue of retail's
// distinct "can't overwrite" refusal (OpenCantOverwriteBindingDialog).
case ConflictOutcome.NonBindable:
// S1 / retail order: checked BEFORE any row conflict is even
// considered — retail refuses outright the instant ANY
// conflicting target is non-user-bindable. This port's
// analogue: a chord already bound to an acdream-only action
// with no DAT row at all (Ctrl+M mute, the debug F-keys, ...) —
// OpenCantOverwriteBindingDialog's ported refusal, no dialog.
bindings.DisplaySystemMessage(bindings.NonBindableRefusalText);
return;
}
List<KeyChord> updated = new(view.Model.Current);
while (updated.Count <= slot) updated.Add(default);
updated[slot] = chord;
ReplaceSlotValue(view, updated);
RefreshRowButtons(view);
case ConflictOutcome.Rows:
// M3: retail's OpenOverwriteBindingDialog — confirm BEFORE
// reassigning (N-way: every conflicting row is named, not just
// the first). Only on accept do the losing rows lose the slot.
string names = string.Join(", ", conflictRows.Select(r => r.Label ?? "?"));
string message =
$"'{DescribeChord(chord)}' is already bound to {names}. "
+ $"Reassign it to '{view.Label}'?";
bindings.ConfirmOverwrite(message, accepted =>
{
if (!accepted) return;
foreach (RowView conflictRow in conflictRows)
{
ReplaceSlotValue(conflictRow, RemoveChord(conflictRow.Model.Current, chord));
RefreshRowButtons(conflictRow);
}
ApplySlot(view, slot, chord);
});
return;
case ConflictOutcome.None:
ApplySlot(view, slot, chord);
return;
}
});
}
private static void ApplySlot(RowView view, int slot, KeyChord chord)
{
List<KeyChord> updated = new(view.Model.Current);
while (updated.Count <= slot) updated.Add(default);
updated[slot] = chord;
ReplaceSlotValue(view, updated);
RefreshRowButtons(view);
}
private void EraseSlot(RowView view, int slot)
{
if (slot >= view.Model.Current.Count) return;
if (view.Model.Current[slot] == default) return; // nothing bound in this display slot
var updated = new List<KeyChord>(view.Model.Current);
updated.RemoveAt(slot);
ReplaceSlotValue(view, updated);
RefreshRowButtons(view);
}
private static void ReplaceSlotValue(RowView view, IReadOnlyList<KeyChord> value) =>
view.Model.SetCurrentValue(value.Where(c => c != default).ToArray());
private static void ReplaceSlotValue(RowView view, IReadOnlyList<KeyChord> value)
{
// S4 (2026-08-11 review): only trim TRAILING empty slots. Retail's
// SetBinding(qc, slot) writes the SPECIFIC slot the user clicked — a row
// with no bindings whose "Mapping 3" button is set must keep the chord at
// display index 2, not collapse it onto index 0. Interior default(KeyChord)
// entries only ever come from ApplySlot's own padding, so trimming just the
// tail keeps RefreshRowButtons' positional read correct without inventing a
// nullable-chord storage type.
int lastReal = -1;
for (int i = 0; i < value.Count; i++)
if (value[i] != default) lastReal = i;
view.Model.SetCurrentValue(lastReal < 0 ? Array.Empty<KeyChord>() : value.Take(lastReal + 1).ToArray());
}
private static IReadOnlyList<KeyChord> RemoveChord(IReadOnlyList<KeyChord> from, KeyChord chord) =>
from.Where(c => c != chord).ToArray();
private enum ConflictKind { None, Row, NonBindable }
// Set by FindConflict just before returning ConflictKind.Row — avoids a
// second lookup pass at the call site. Single-threaded (UI thread only).
private RowView? _lastConflictRow;
private enum ConflictOutcome { None, NonBindable, Rows }
/// <summary>
/// Retail's N-way, cross-input-map conflict scan (research doc §5.4:
/// <c>ICIDM::FindConflictingInputMaps</c>/<c>FindConflictingControls</c>),
/// scoped to this screen's own universe: every OTHER row's current chord set
/// scoped to this screen's own universe: the non-user-bindable check runs
/// FIRST (S1 — retail's own order), then EVERY OTHER row's current chord set
/// (covers BOTH mapped and unmapped rows — a chord already claimed by an
/// unmapped row is just as real a conflict as one claimed by a mapped one),
/// then the live <see cref="KeyBindings"/> table for an acdream-only action
/// this screen has no row for at all.
/// unmapped row is just as real a conflict as one claimed by a mapped one) is
/// collected in full, not just the first match.
/// </summary>
private ConflictKind FindConflict(KeyChord chord, RowView exclude)
private (ConflictOutcome Outcome, List<RowView> Rows) FindConflicts(KeyChord chord, RowView exclude)
{
_lastConflictRow = null;
if (_bindings is not null)
{
foreach (InputAction candidate in Enum.GetValues<InputAction>())
{
if (RetailActionIdentityTable.Map.Values.Contains(candidate)) continue;
if (_bindings.CurrentForAction(candidate).Any(b => b.Chord == chord))
return (ConflictOutcome.NonBindable, new List<RowView>());
}
}
var rows = new List<RowView>();
foreach (RowView other in _rows)
{
if (ReferenceEquals(other, exclude)) continue;
if (!other.Model.Current.Contains(chord)) continue;
_lastConflictRow = other;
return ConflictKind.Row;
if (other.Model.Current.Contains(chord))
rows.Add(other);
}
if (_bindings is null) return ConflictKind.None;
foreach (InputAction candidate in Enum.GetValues<InputAction>())
{
if (RetailActionIdentityTable.Map.Values.Contains(candidate)) continue;
if (_bindings.CurrentForAction(candidate).Contains(chord))
return ConflictKind.NonBindable;
}
return ConflictKind.None;
return rows.Count > 0 ? (ConflictOutcome.Rows, rows) : (ConflictOutcome.None, rows);
}
private static void WireScreenButtons(
@ -493,8 +574,9 @@ public sealed class KeyboardConfigController
// OK — right-click release in retail (idMessage 0x19); ported as a plain
// left-click here, matching every other Campaign OP button (the asymmetry
// is authored-input-only, not a behavior a user would notice — register
// row if reviewed otherwise).
// is authored-input-only — no user-visible affordance differs, since
// retail's own right-click-release on just this pair of buttons carries
// no distinguishing visual cue either).
if (layout.FindElement(OkButtonId) is UiButton okButton)
okButton.OnClick = () =>
{

51
src/AcDream.App/UI/RetailUiRuntime.cs
View file

@ -2331,13 +2331,17 @@ public sealed class RetailUiRuntime : IDisposable
},
resolveString: (tableId, stringId) => strings.Resolve(tableId, stringId),
new Layout.KeyboardConfigController.Bindings(
CurrentForAction: action => dispatcher.Bindings.ForAction(action)
.Select(b => b.Chord).ToArray(),
SetForAction: (action, chords) =>
// M1 (2026-08-11 review): read/write the FULL live Binding
// (chord + activation + scope), not a bare chord — SetForAction
// used to reconstruct every Binding with the constructor's bare
// defaults (Press/Game), collapsing walk-mode's Hold and every
// combat-scoped binding's scope the instant a row wrote back.
CurrentForAction: action => dispatcher.Bindings.ForAction(action).ToArray(),
SetForAction: (action, newBindings) =>
{
KeyBindings updated = CloneWithout(dispatcher.Bindings, action);
foreach (KeyChord chord in chords)
updated.Add(new Binding(chord, action));
foreach (Binding b in newBindings)
updated.Add(b);
dispatcher.SetBindings(updated);
},
CurrentForUnmapped: key => unmapped.Get(key.InputMapId, key.ActionId),
@ -2346,8 +2350,20 @@ public sealed class RetailUiRuntime : IDisposable
chord => onResult(chord == default ? null : chord)),
Save: () =>
{
dispatcher.Bindings.SaveToFile(keyboard.KeyBindingsFilePath);
unmapped.SaveToFile(unmappedPath);
// S3 (2026-08-11 review): match the existing keybinds.json
// writer's own discipline (RuntimeKeyBindingTarget.Apply) —
// an IO failure is reported, not thrown out of UiButton.OnClick
// into the input/render loop, and does not roll back the
// already-accepted live binding.
try
{
dispatcher.Bindings.SaveToFile(keyboard.KeyBindingsFilePath);
unmapped.SaveToFile(unmappedPath);
}
catch (Exception failure)
{
Console.WriteLine($"keyboard config: save failed: {failure.Message}");
}
},
Toggle: () => ToggleWindow(WindowNames.KeyboardConfig),
DisplaySystemMessage: text =>
@ -2355,13 +2371,20 @@ public sealed class RetailUiRuntime : IDisposable
if (!string.IsNullOrEmpty(text)) _bindings.Options.DisplaySystemMessage(text);
},
NonBindableRefusalText: refusalText ?? string.Empty,
// No retail string exists for "binding reassigned" — retail's
// own flow only shows the confirm-before-reassign dialog
// (research doc §5.4's OpenOverwriteBindingDialog), never a
// post-reassign notice. This port's auto-reassign (register
// row) stays silent rather than inventing English for a
// message retail never had.
NotifyReassigned: _ => string.Empty));
// M3 (2026-08-11 review): retail's OpenOverwriteBindingDialog —
// confirm through the SAME RetailDialogFactory/MakeConfirmation
// seam GameplayConfirmationController already uses, before
// reassigning. DialogFactory is mounted AFTER MountKeyboardConfig
// in Initialize()'s order, so this reads the property lazily
// (Initialize() has always finished by the time a user can
// actually open this screen and trigger a capture).
ConfirmOverwrite: (message, onResult) =>
{
if (DialogFactory is null) { onResult(false); return; }
DialogFactory.MakeConfirmation(
message,
data => onResult(data.GetBoolean(RetailDialogProperty.ConfirmationResult)));
}));
if (controller is null)
{

14
src/AcDream.App/UI/UiButton.cs
View file

@ -528,9 +528,19 @@ public sealed class UiButton : UiElement, IUiGlobalTimeListener, IUiDatStateful
OnClickAt?.Invoke(e.Data1, e.Data2);
return OnClick is not null || OnClickAt is not null;
case UiEventType.RightClick:
// S6 (2026-08-11 review): unlike Click (whose swallow-when-
// disabled is pre-existing, harmless-by-construction behavior
// every button already had), RightClick was UNHANDLED before
// this class grew OnRightClick — it fell through to `default:
// return false` and bubbled to the parent. Preserve that for
// every button with no handler, disabled or not, so this
// addition is genuinely a no-op for every pre-existing button
// (matching this property's own doc comment) and only changes
// behavior for the ones that opt in.
if (OnRightClick is null) return false;
if (!Enabled) return true;
OnRightClick?.Invoke();
return OnRightClick is not null;
OnRightClick.Invoke();
return true;
case UiEventType.DragEnter:
_itemDragAcceptance = e.Payload is ItemDragPayload payload
? OnItemDragOver?.Invoke(payload) ?? ItemDragAcceptance.None