fix(ui): OP3 review fixes — byte-verified Magic chat lines, Gameplay/OptionPage shape, mid-air tri-state, shared geometry
Consolidated fix round for the two OP3 dual-lens reviews
(docs/research/2026-08-11-op3-review-{mechanism,blast}.md), both
APPROVE-WITH-FIXES.
MUST-FIX:
- The six "Use Mouse Turning Settings" chat lines were typed
RetailLogTextType.ClientLocal (0x1A); retail types them 0x07 (Magic).
BYTE-VERIFIED against the PDB-paired binary at all six
gmConfigUI::SetMouseTurningDefaults call sites (0x0049E972/E9E2/EA52/
EAA4/EAF6/EB48): every site pushes `6a 07` (type=7) immediately before
the text-pointer push and the AddTextToScroll call. Added a dedicated
OptionsRuntimeBindings.DisplayMouseTurningMacroLine seam routed at
Magic (scrolling chat transcript, light blue, timestamped) instead of
the 4-slot SpewBox ClientLocal uses; the mid-air refusal and UA/RA
keep ClientLocal (both independently confirmed correct).
- Filed AD-77: the client-wide floating-only gmPanelUI host divergence
(retail also exposes a docked 0x21000017 host) the plan §5 delegated
to this review, scoped to every main panel, not just Options.
SHOULD-FIX:
- gmGameplayOptionsUI is not an OptionPage in retail (acclient.h:55857,
UIElement_Field). OptionsPanelController now constructs the Gameplay
slot's OptionPage with AfterApply deliberately null, so entering/
leaving that tab never publishes SaveCharacterOptionsRuntimeCmd.
Corrected OptionPageModel's doc comment and rewrote the two tests
that pinned the wrong (Gameplay-flushes) shape.
- Added the OptionPage.OnOptionChanged seam (PlayerOptionPage::
OnOptionChanged @0x004F27D0) — fires as the last step of Apply/
Reset/Defaults, plus once per live LED edit via a new
IOptionRow.AttachPageNotify hook (BoolOptionRow wires it into
SetCurrentValue only, matching retail's Apply(1)-only
HandleDialogAndNotices path). OP4-6 will bind Apply/Reset enable
state to this.
- Exit to Character Selection's mid-air refusal is now tri-state
(Func<bool?> IsGrounded): retail's UseTime only reaches the airborne
test inside `else if (smartbox->player)`, so outside player mode (or
with no live controller) the button is a SILENT no-op, not a
refusal. Fixed the inverted comment at both call sites.
- Options panel geometry now matches its nine gmPanelUI siblings
sharing RetailPanelUiController's one main-panel rectangle
(ResizeX=false, bottom-edge-only resize, no invented Min/MaxWidth/
Height) instead of being the only all-four-edge/horizontal-resize
outlier whose width silently reverted whenever a sibling was shown.
- Added the three missing test pins: Options/Character mutual
exclusion through a REAL RetailPanelUiController registration,
RetailDialogFactory.MakeConfirmation's omitted-queueKey overload
sharing DefaultQueueKey, and UiTabPanel.ActivePageChanged never
firing on a dormant (non-activated) host.
- TS-74's What/Where now names the five store-only CameraTurning
preferences explicitly instead of only mentioning them in Risk.
- Test script gains the toolbar-button ghosted->enabled+highlight
check, UseMouseTurning-survives-relogin and the five prefs-survive-
relaunch steps, a UA/RA legibility eye-item, and the corrected
bottom-edge-only geometry description for step 5.
One-liners fixed in files already touched: symmetric close-button
resolve-failure logging in OptionsPanelController.Bind (blast NOTE 8).
Full Release suite: 12,947 passed / 4 skipped / 0 failed (baseline
12,935/4/0 post-OP7 — 12 net new tests; the two OptionPageModelTests
"wrong-shape" tests were renamed/rewritten in place, not removed).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
64898f1301
commit
386076af0f
11 changed files with 505 additions and 72 deletions
|
|
@ -31,6 +31,21 @@ public interface IOptionRow
|
|||
/// <summary><c>RestoreDefaultValue</c>: <c>m_current = m_default</c>,
|
||||
/// then applies the default live.</summary>
|
||||
void RestoreDefaultValue();
|
||||
|
||||
/// <summary>
|
||||
/// Wires this row's owning-page notify hook — retail's
|
||||
/// <c>UIOption::m_pOCH</c> (option-change-handler) pointer, invoked by
|
||||
/// <c>UIOption::HandleDialogAndNotices @0x004EFB90</c>'s
|
||||
/// <c>m_pOCH->OnOptionChanged(this)</c> call after a LIVE user edit
|
||||
/// (a widget's own <c>SetCurrentValue</c>/<c>Apply(1)</c> path ONLY —
|
||||
/// <c>RestoreSavedValue</c>/<c>RestoreDefaultValue</c> use retail's
|
||||
/// <c>Apply(0)</c>, which skips this per-row notify because the OWNING
|
||||
/// VERB (<see cref="OptionPage.Reset"/>/<see cref="OptionPage.Defaults"/>)
|
||||
/// already calls <see cref="OptionPage.OnOptionChanged"/> once itself,
|
||||
/// at its own tail). Called once by <see cref="OptionPage.Register"/>;
|
||||
/// mechanism review S2, 2026-08-11 fix round.
|
||||
/// </summary>
|
||||
void AttachPageNotify(Action notify);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
|
|
@ -44,6 +59,7 @@ public interface IOptionRow
|
|||
public sealed class BoolOptionRow : IOptionRow
|
||||
{
|
||||
private readonly Action<bool>? _apply;
|
||||
private Action? _notifyPageOptionChanged;
|
||||
private bool _current;
|
||||
private bool _saved;
|
||||
private bool _default;
|
||||
|
|
@ -77,14 +93,19 @@ public sealed class BoolOptionRow : IOptionRow
|
|||
/// <summary>
|
||||
/// Retail <c>SetCurrentValue @0x00486970</c> — the LED-click entry point.
|
||||
/// Writes <c>m_current</c> and applies it live immediately; does NOT
|
||||
/// touch <see cref="Saved"/> (Apply is the only verb that commits).
|
||||
/// touch <see cref="Saved"/> (Apply is the only verb that commits). Also
|
||||
/// notifies the owning page (<see cref="AttachPageNotify"/>) — retail's
|
||||
/// <c>Apply(1)</c>-only <c>HandleDialogAndNotices</c> path.
|
||||
/// </summary>
|
||||
public void SetCurrentValue(bool value)
|
||||
{
|
||||
_current = value;
|
||||
_apply?.Invoke(value);
|
||||
_notifyPageOptionChanged?.Invoke();
|
||||
}
|
||||
|
||||
public void AttachPageNotify(Action notify) => _notifyPageOptionChanged = notify;
|
||||
|
||||
public void SaveCurrentValue() => _saved = _current;
|
||||
|
||||
public void RestoreSavedValue()
|
||||
|
|
@ -134,13 +155,20 @@ public sealed class BoolOptionRow : IOptionRow
|
|||
/// </list>
|
||||
///
|
||||
/// <para>
|
||||
/// An empty page (zero registered rows — the Gameplay tab's own model, which
|
||||
/// has no <c>UIOption</c> rows at all per research doc §6) makes every verb a
|
||||
/// no-op and <see cref="Changed"/> permanently false; <see cref="AfterApply"/>
|
||||
/// still fires on <see cref="Apply"/>/<see cref="OnShown"/> (retail's
|
||||
/// <c>SaveCurrentValues</c> flushes the blob regardless of whether THIS
|
||||
/// page's own rows changed anything — the module's dirty flag is global, not
|
||||
/// per-page).
|
||||
/// An empty page (zero registered rows) makes every verb a no-op and
|
||||
/// <see cref="Changed"/> permanently false; when <see cref="AfterApply"/> IS
|
||||
/// wired, it still fires on <see cref="Apply"/>/<see cref="OnShown"/> even
|
||||
/// with zero rows (retail's <c>SaveCurrentValues</c> flushes the blob
|
||||
/// regardless of whether THIS page's own rows changed anything — the
|
||||
/// module's dirty flag is global, not per-page). <b>This is a property of
|
||||
/// the generic empty-page shape, not a description of the Gameplay tab.</b>
|
||||
/// <c>gmGameplayOptionsUI</c> (<c>acclient.h:55857</c>) derives from
|
||||
/// <c>UIElement_Field</c>, not <c>OptionPage</c>/<c>PlayerOptionPage</c> at
|
||||
/// all, so retail never calls <c>SaveCurrentValues</c> for it in the first
|
||||
/// place — <see cref="OptionsPanelController"/> deliberately
|
||||
/// constructs the Gameplay slot's <see cref="OptionPage"/> instance with
|
||||
/// <see cref="AfterApply"/> left <c>null</c> so entering/leaving that tab
|
||||
/// never flushes (mechanism review S1, 2026-08-11 fix round).
|
||||
/// </para>
|
||||
/// </summary>
|
||||
public sealed class OptionPage
|
||||
|
|
@ -154,17 +182,33 @@ public sealed class OptionPage
|
|||
/// <see cref="OnShown"/>) — the seam a controller wires to the batched
|
||||
/// <c>SaveOptions</c>/blob-flush command. Never invoked by
|
||||
/// <see cref="Reset"/> or <see cref="Defaults"/> (retail's Reset/Defaults
|
||||
/// call <c>Apply(0)</c> per-row and re-run <c>OnOptionChanged(0)</c>
|
||||
/// call <c>Apply(0)</c> per-row and reach <see cref="OnOptionChanged"/>
|
||||
/// directly — they never reach <c>PlayerOptionPage::SaveCurrentValues</c>,
|
||||
/// so they never flush).
|
||||
/// </summary>
|
||||
public Action? AfterApply { get; set; }
|
||||
|
||||
/// <summary>
|
||||
/// Retail <c>OptionPage::OnOptionChanged(0)</c> — the ONLY thing that
|
||||
/// enable-gates Apply/Reset (<c>PlayerOptionPage::OnOptionChanged
|
||||
/// @0x004F27D0</c>: disabled when <see cref="Changed"/> is false, enabled
|
||||
/// otherwise; Defaults is NEVER gated — retail's override never fetches
|
||||
/// its child id at all). Retail runs this as the LAST statement of all
|
||||
/// three verbs (<c>0x004F2C95</c> Apply, <c>0x004F2CE5</c> Defaults,
|
||||
/// <c>0x004F2D4A</c> Reset); a live user edit
|
||||
/// (<see cref="IOptionRow.AttachPageNotify"/>'s
|
||||
/// <c>SetCurrentValue</c>-only path) also reaches it directly via
|
||||
/// <c>UIOption::HandleDialogAndNotices @0x004EFB90</c>. OP4-6 bind
|
||||
/// buttons to this seam; mechanism review S2, 2026-08-11 fix round.
|
||||
/// </summary>
|
||||
public Action? OnOptionChanged { get; set; }
|
||||
|
||||
/// <summary>Registers one row. Retail's <c>OptionPage::RegisterOption
|
||||
/// @0x004F2E90</c>, called from each page's <c>InitOptions</c>.</summary>
|
||||
public void Register(IOptionRow row)
|
||||
{
|
||||
ArgumentNullException.ThrowIfNull(row);
|
||||
row.AttachPageNotify(() => OnOptionChanged?.Invoke());
|
||||
_rows.Add(row);
|
||||
}
|
||||
|
||||
|
|
@ -173,17 +217,19 @@ public sealed class OptionPage
|
|||
public bool Changed => _rows.Any(static row => row.Changed);
|
||||
|
||||
/// <summary><c>OptionPage::SaveCurrentValues @0x004F2C60</c> — Apply:
|
||||
/// commits every row's baseline unconditionally, then flushes via
|
||||
/// <see cref="AfterApply"/>.</summary>
|
||||
/// commits every row's baseline unconditionally, flushes via
|
||||
/// <see cref="AfterApply"/>, then re-evaluates <see cref="OnOptionChanged"/>.</summary>
|
||||
public void Apply()
|
||||
{
|
||||
foreach (IOptionRow row in _rows)
|
||||
row.SaveCurrentValue();
|
||||
AfterApply?.Invoke();
|
||||
OnOptionChanged?.Invoke();
|
||||
}
|
||||
|
||||
/// <summary><c>OptionPage::RestoreSavedValues @0x004F2D00</c> — Reset:
|
||||
/// reverts only the rows that are currently <see cref="IOptionRow.Changed"/>.
|
||||
/// reverts only the rows that are currently <see cref="IOptionRow.Changed"/>,
|
||||
/// then re-evaluates <see cref="OnOptionChanged"/>.
|
||||
/// Snapshotted before iterating so a row's own revert (which flips
|
||||
/// <see cref="IOptionRow.Changed"/> back to false) cannot skip a later
|
||||
/// row.</summary>
|
||||
|
|
@ -191,15 +237,17 @@ public sealed class OptionPage
|
|||
{
|
||||
foreach (IOptionRow row in _rows.Where(static row => row.Changed).ToArray())
|
||||
row.RestoreSavedValue();
|
||||
OnOptionChanged?.Invoke();
|
||||
}
|
||||
|
||||
/// <summary><c>OptionPage::RestoreDefaultValues @0x004F2CB0</c> —
|
||||
/// Defaults: restores every row unconditionally, live, without
|
||||
/// committing.</summary>
|
||||
/// committing, then re-evaluates <see cref="OnOptionChanged"/>.</summary>
|
||||
public void Defaults()
|
||||
{
|
||||
foreach (IOptionRow row in _rows)
|
||||
row.RestoreDefaultValue();
|
||||
OnOptionChanged?.Invoke();
|
||||
}
|
||||
|
||||
/// <summary><c>PlayerOptionPage::OnVisibilityChanged(true)</c> — the page
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue