From 07f2b3f72e8838250a0c23c03a8913126a2d9899 Mon Sep 17 00:00:00 2001 From: Erik Date: Tue, 11 Aug 2026 13:42:39 +0200 Subject: [PATCH] =?UTF-8?q?fix(settings):=20OP9=20review=20residuals=20?= =?UTF-8?q?=E2=80=94=20restore=20SaveAudio=20live-apply=20pin,=20delete=20?= =?UTF-8?q?dead=20residues?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the OP9 combined review's findings (docs/research/2026-08-11-op9-review.md, APPROVE-WITH-FIXES): - MUST-FIX 1: SaveAudio -> ApplyAudio (OP6's Config-tab live-apply) lost its ONLY assertion when the retired SettingsVM save-order test was deleted. Restored directly on the now-public seam: SaveAudioPersistsThenPushesLiveApplyAudioWithTheSavedSnapshot pins persist-then-push order + the pushed snapshot; SaveAudioSkipsTheLivePushWhenPersistenceFails pins the failure ordering (a failed persist pushes nothing and commits nothing). Also closes SF-5: the OP6 effective-volume comment's 'target-audio assertion above' reference is real again and now names the restored test. - SF-3: dead residues deleted — RuntimeSettingsController's private SaveCharacter (zero callers post-371197a3), ISettingsStorage.SaveCharacter + its JsonRuntimeSettingsStorage/FakeStorage implementations (the deleted private method was the only caller), and IngressShutdownRoots.Settings (zero readers since the view-model shutdown stage died). SettingsStore's PUBLIC SaveCharacter stays: it is the tested storage-API seam, and per-toon entries in existing settings.json files still load through the live LoadCharacter path. - SF-2: code-structure.md's presentation-seam list no longer routes the settings preview through 'optional SettingsVM'. - NIT 6: AP-196's retirement note now attributes LockUI (/lockui + PlayerDescription SetUiLocked convergence) and UseMouseTurning (Gameplay-tab macro + Config-tab row) to their real channels instead of folding all 13 members into the Character tab. Full Release suite: 13,077 passed / 4 skipped / 0 failed (13,075 + the two restored tests). One unnamed App-assembly failure appeared on the first post-fix full run and did not reproduce on the isolated assembly rerun nor a second full run — consistent with the known #250-class parallel-load flake, recorded here for honesty rather than silently rerun. Co-Authored-By: Claude Fable 5 --- docs/architecture/code-structure.md | 8 ++- .../retail-divergence-register.md | 2 +- src/AcDream.App/Rendering/GameWindow.cs | 1 - .../Rendering/GameWindowLifetime.cs | 1 - .../Settings/RuntimeSettingsController.cs | 26 -------- .../RuntimeSettingsControllerTests.cs | 66 ++++++++++++++----- 6 files changed, 57 insertions(+), 47 deletions(-) diff --git a/docs/architecture/code-structure.md b/docs/architecture/code-structure.md index ba15e6a9..a56b00b8 100644 --- a/docs/architecture/code-structure.md +++ b/docs/architecture/code-structure.md @@ -208,8 +208,12 @@ documentation, not an exhaustive allowlist): - `DevToolsFramePresenter -> DevToolsPanelSet -> panel/ViewModel bindings` for the optional developer UI. - `WorldRenderFrameBuilder -> RuntimeWorldFrameSettingsPreview -> - IRuntimeSettingsPreviewSource -> RuntimeSettingsController -> optional - SettingsVM` for the live settings draft preview applied before world drawing. + IRuntimeSettingsPreviewSource -> RuntimeSettingsController` for the settings + snapshot read before world drawing. (The `SettingsVM` draft-preview tail of + this seam was retired at Campaign OP slice OP9 — the preview source now + mirrors the committed Display/Audio snapshot directly; the retail Options + panel applies its edits live through `SaveDisplay`/`SaveAudio` instead of a + draft layer.) - `LocalPlayerPortalViewport -> LocalPlayerTeleportController -> GameplayInputFrameController -> InputDispatcher.Fired -> GameWindow` for the canonical portal/input lifetime and the host's input-action subscription. diff --git a/docs/architecture/retail-divergence-register.md b/docs/architecture/retail-divergence-register.md index c82d7408..a0cd46ad 100644 --- a/docs/architecture/retail-divergence-register.md +++ b/docs/architecture/retail-divergence-register.md @@ -189,7 +189,7 @@ AP-94..AP-112 for the confirmed retail-UI completion gaps. | ~~AP-204~~ | **RETIRED 2026-08-11 at the OP8 rework (M3, combined review).** Originally filed for two narrowings: (1) silent auto-reassign on a cross-row conflict instead of retail's modal `OpenOverwriteBindingDialog`, and (2) OK/Cancel wired as left-click instead of retail's right-click-release gesture. (1) is FIXED — `KeyboardConfigController.BeginSlotCapture` now opens a real confirm dialog through `RetailDialogFactory.MakeConfirmation` (the SAME seam `GameplayConfirmationController` uses) BEFORE reassigning, listing every conflicting row (N-way), and only applies on accept; decline leaves every row untouched. (2) is NOT fixed and does not warrant its own row: it is authored-input-only with zero observable difference to a user (retail's own right-click-release on just this pair of buttons carries no distinguishing visual cue either, and every other Campaign OP button already uses left-click) — noted as a code comment at the OK/Cancel wiring site instead of a register row, matching this register's convention of reserving rows for divergences that could produce an observable symptom. | `src/AcDream.App/UI/Layout/KeyboardConfigController.cs` (`FindConflicts`/`BeginSlotCapture`; `WireScreenButtons`'s OK/Cancel `OnClick` comment); `src/AcDream.App/UI/RetailUiRuntime.cs` (`MountKeyboardConfig`'s `ConfirmOverwrite` wiring) | — | — | `docs/research/2026-08-10-keyboard-config-and-gameplay-tab.md` §5.4 (`UIOption_ActionKeyMap::KeyHitHandler @0x00489570`, `OpenOverwriteBindingDialog @0x00488BF0`, `OpenCantOverwriteBindingDialog @0x00489300`) and §5.5 (OK/Cancel `idMessage 0x19` gesture) | | AP-194 | `CharacterOptionTable`'s `ClientDefault` column (what the Character tab's Defaults button restores) disagrees with the raw constructor default word for three ids: `ConfirmVolatileRareUse` (`0x2D`), `ShowHelm` (`0x2F`), and `ShowCloak` (`0x32`) are all ON in retail's constructor default `CharacterOptions2 = 0x00948700` (`PlayerModule::PlayerModule @0x005D51F0`, byte-verified literal write) but report default-OFF via `PlayerModule::GetDefaultOptionValue @0x005D2A30`, whose own per-option table stops at id `0x2A` and returns `false` for everything past it. This is retail's OWN behavior, reproduced deliberately — the Defaults button does not reproduce a fresh `PlayerModule`. **CONFIRMED 2026-08-11 at Campaign OP slice OP4**: `CharacterOptionsPageController` seeds every `BoolOptionRow`'s default directly from this column (`EveryRow_DefaultValue_MatchesCharacterOptionTableClientDefault`, `tests/AcDream.App.Tests/UI/Layout/CharacterOptionsPageControllerTests.cs`); the directive below was followed, not re-litigated. OP4 also independently traced retail's OWN mechanism for the Character tab specifically — `UIOption_Checkbox::SetPlayerOption @0x00486e80` (pseudo-C line 147375) sets `m_default` directly from `GetDefaultOptionValue`, confirming this column (not the separate `DBPropertyCollection`/`InqDefaultGameplayOptionProperty` mechanism that governs the Chat/Config tabs' `m_propName`-bound rows) is the correct and ONLY source for this tab. | `src/AcDream.Runtime/Gameplay/CharacterOptionTable.cs` (`ClientDefault` column; see the type's XML doc); `src/AcDream.App/UI/Layout/CharacterOptionsPageController.cs` | Byte-verified at both addresses (wire research §2.5 for the constructor literals, §8.2 for `GetDefaultOptionValue`'s own table and bounds check) — this is not a guess, it is retail's documented quirk. "Fixing" it to match the constructor default would make acdream's Defaults button MORE correct than retail's own, which is the opposite of this project's goal. | A future OP-campaign slice (OP4, the Character tab's Defaults button) must consult THIS column, not the constructor default word, or a future reader may "fix" this back and silently diverge from retail. | `PlayerModule::GetDefaultOptionValue @0x005D2A30`; `UIOption_Checkbox::SetPlayerOption @0x00486e80` (N-4 anchor-column correction, OP4 review-fix round 2026-08-11 — was mislabeled `PlayerModule::SetPlayerOption`, same address, wrong class); `PlayerModule::PlayerModule @0x005D51F0`; `docs/research/2026-08-10-set-character-options-wire.md` §8.2 | | AP-193 | Character option id `0x34` (`ListenToPKDeathMessages` / "Listen to PK death messages") is mapped to `CharacterOptions2` bit `0x02000000` and modeled as a batched (non-auto-save) option purely on ACE's own enum — the id does not exist in the 2013 EoR PDB (`PlayerOption` there terminates at `TotalNumberOfPlayerOptions_PlayerOption = 0x34`), so neither the mask nor its `IsAutoSaveOption`/`GetDefaultOptionValue` classification is byte-verifiable against our binary. | `src/AcDream.Runtime/Gameplay/CharacterOptionTable.cs` (`HearPkDeathMessages` row) | The user's retail memory (and ACE's own `CharacterOption` enum) both carry this option; shipping wire+store coverage for it is strictly better than omitting the row the Character tab's screenshots show, and ACE never actually reads the bit server-side (`PlayerFactory.cs:659-660` — "possibly was added to Defaults post PDB we have"), so a wrong id/mask/auto-save guess here has zero server-observable consequence either way. | If the final EoR client's real id/mask/auto-save classification ever surfaces (a later PDB, or a byte-level trace against a 2015+ binary), this row's values may be wrong and need correcting — until then treat them as ACE-sourced, not retail-verified. | ACE `PlayerFactory.cs:659-660`, `CharacterOptions2.cs` (`ListenToPKDeathMessages = 0x02000000`); `named-retail/acclient.h:4162-4218` (2013 `PlayerOption` terminates at `0x34`); `docs/research/2026-08-10-set-character-options-wire.md` §8.1 | -| ~~AP-196~~ | **RETIRED 2026-08-11 at Campaign OP slice OP9.** Filed at the OP4 review-fix round (MUST-FIX 3/blast M2) recording that OP4's Group-C re-point deleted only three of the eight re-pointed `GameplaySettings` fields (`AutoTarget`/`AutoRepeatAttack`/`ViewCombatTarget`), leaving `VividTargetingIndicator`/`CoordinatesOnRadar`/`LockUI`/`AcceptLootPermits`/`ToggleRun` behind as WRITE-BEHIND `settings.json` persistence/draft mirrors of the now-authoritative server bit (plus a "two writable copies" default-source change, ADDENDUM historical only). OP9 verified all remaining `GameplaySettings` members — those five plus `ShowTooltips`/`SideBySideVitals`/`SpellDuration`/`AllowGive`/`ShowHelm`/`ShowCloak`/`AdvancedCombatUI`/`UseMouseTurning`, 13 total — were ALREADY bound through `CharacterOptionTable`/`CharacterOptionsPageController`'s server-bit seam (the OP4 Character tab) and deleted the `GameplaySettings` record outright — the type, the `SettingsStore.LoadGameplay`/`SaveGameplay` plumbing, and `RuntimeSettingsController`'s `Gameplay` property/`SetAcceptLootPermits` write-behind method — closing the "two writable copies" gap for good: there is no longer a second store to diverge from server truth. | `src/AcDream.App/Settings/RuntimeSettingsController.cs`; `src/AcDream.App/UI/Layout/CharacterOptionsPageController.cs`; `src/AcDream.Runtime/Gameplay/CharacterOptionTable.cs` | — | — | `docs/research/2026-08-10-character-options-map.md` §7.1/§7.2 (Group C re-point directive); `CharacterOptionTable.cs` | +| ~~AP-196~~ | **RETIRED 2026-08-11 at Campaign OP slice OP9.** Filed at the OP4 review-fix round (MUST-FIX 3/blast M2) recording that OP4's Group-C re-point deleted only three of the eight re-pointed `GameplaySettings` fields (`AutoTarget`/`AutoRepeatAttack`/`ViewCombatTarget`), leaving `VividTargetingIndicator`/`CoordinatesOnRadar`/`LockUI`/`AcceptLootPermits`/`ToggleRun` behind as WRITE-BEHIND `settings.json` persistence/draft mirrors of the now-authoritative server bit (plus a "two writable copies" default-source change, ADDENDUM historical only). OP9 verified all remaining `GameplaySettings` members — those five plus `ShowTooltips`/`SideBySideVitals`/`SpellDuration`/`AllowGive`/`ShowHelm`/`ShowCloak`/`AdvancedCombatUI`/`UseMouseTurning`, 13 total — already had a live server-bit home in `RuntimeCharacterOptionsState` (11 as OP4 Character-tab rows through `CharacterOptionTable`/`CharacterOptionsPageController`; `LockUI` through `/lockui` + the PlayerDescription `SetUiLocked` convergence, deliberately not a Character-tab row; `UseMouseTurning` through the Gameplay-tab mouse-macro button + the Config tab's Use-Mouse-Turning row — OP9 review NIT 6's channel-attribution correction) and deleted the `GameplaySettings` record outright — the type, the `SettingsStore.LoadGameplay`/`SaveGameplay` plumbing, and `RuntimeSettingsController`'s `Gameplay` property/`SetAcceptLootPermits` write-behind method — closing the "two writable copies" gap for good: there is no longer a second store to diverge from server truth. | `src/AcDream.App/Settings/RuntimeSettingsController.cs`; `src/AcDream.App/UI/Layout/CharacterOptionsPageController.cs`; `src/AcDream.Runtime/Gameplay/CharacterOptionTable.cs` | — | — | `docs/research/2026-08-10-character-options-map.md` §7.1/§7.2 (Group C re-point directive); `CharacterOptionTable.cs` | | AP-197 | **Filed 2026-08-11 at the OP4 review-fix round (SF-1/S4).** "Display Timestamps" hardcodes retail's `PlayerModule` constructor-default format string `"%#H:%M:%S "` rather than reading the PER-CHARACTER override `GenericQualitiesData::InqString(m_pPlayerOptionsData, 1, &m_TimeStampFormat)` carries when the wire's `GenericQualitiesData` string-key `1` is populated — acdream's `PlayerDescription` parser reads and discards that field (wire research doc: "timestamp string (`0x80`) \| read, discarded \| ❌ \| never sent"). | `src/AcDream.Core/Chat/ChatLog.cs` (`FormatTimestampPrefix`); parser site cited at `docs/research/2026-08-10-set-character-options-wire.md:647` | The 2013 client's own constructor default is the only format any fresh/default character would ever show — retail ships no options-panel control that authors a custom one — so hardcoding the one value every real player sees is a safe, honest approximation until a consumer needs the per-character override. | A character whose account somehow carries a non-default persisted timestamp format (a modded/legacy server, or a hypothetical later retail patch exposing a UI for it) sees acdream render the DEFAULT format instead of their stored one — cosmetic only (still a valid H:MM:SS-shaped timestamp), never a wire or data-loss risk. | `PlayerModule::PlayerModule @0x005D51F0` (ctor default literal); `GenericQualitiesData::InqString` call site (wire doc §3.3); `docs/research/2026-08-10-set-character-options-wire.md` U6 | | AP-198 | **Filed 2026-08-11 at Campaign OP slice OP6; CORRECTED at the OP6 rework round (2026-08-11, review N1/S2) — the row count was ALWAYS ten (this row's own enumeration always listed ten items); the commit message that said "nine" was the error, now reconciled, and `Render_ScreenBrightness` no longer overloads `Gamma`.** The Config tab's "Graphics Options" + "Rendering Quality Options" sections author ten rows with no acdream renderer consumer: `Render_ScreenBrightness` (its OWN `DisplaySettings.ScreenBrightness` field, range [-1,1] default 0 — NOT the pre-existing `Gamma` multiplier, which has a different unit system and its own live legacy Settings-panel consumer; no gamma-correction pass exists for either), `Render_AutomaticDegrades`, `Render_GraphicsPerformance`, `Render_DegradeDistance`, `Render_LandscapeTextureDetail`, `Render_EnvironmentTextureDetail`, `Render_TextureFiltering`, `Render_LandscapeDrawDistance`, `Render_BuildingDetailTextures`, `Render_MultiPassAlpha`. acdream's world renderer is Vulkan driven by ONE aggregate `QualitySettings`/`QualityPreset` (near/far streaming radii, anisotropic level, alpha-to-coverage, completion budget) — there is no per-feature texture-detail/degrade-distance knob for any of these ten rows to drive. Each round-trips faithfully through `DisplaySettings`/`SettingsStore` and shows retail's own row/label/range (where applicable), with zero observable render effect. **Sub-note, `Render_LandscapeDrawDistance` specifically:** its retail default (`gmConfigUI::InitOptions @0x0049E70D`, `SetDefaultValue(8)`) does not index its own 6-entry `UIPreferences::SetEnumChoices` array (`ID_Graphics_Value_VeryLow`..`Extreme`, `gmClient::InitUIPreferences @0x004041b7`) — reproduced faithfully as an opaque `int` (`DisplaySettings.LandscapeDrawDistance`), not guessed into a clamped index; the Config-tab menu simply shows no highlighted selection at the default. | `src/AcDream.UI.Abstractions/Panels/Settings/DisplaySettings.cs`; `src/AcDream.App/UI/Layout/ConfigOptionsPageController.cs` (`BindGraphicsSection`/`BindRenderingQualitySection`) | Building ten dead per-feature render knobs into a Vulkan renderer that has no analogous per-feature toggles would be pure UI theater with no correctness payoff; persisting them faithfully keeps the panel honest (every row is clickable, nothing crashes, nothing silently discards a user's choice) while the register makes the "no effect" fact auditable rather than a silent gap a future report would have to re-discover. | A user who changes any of these ten Config-tab controls sees no visual change and, for `LandscapeDrawDistance` specifically, may see no highlighted menu item even after Defaults — both are the CONTRACTED behaviour for this row, not a bug. | `gmConfigUI::InitOptions @0x0049E400`; `gmClient::InitUIPreferences @0x004035b0` (`UIPreferences::AttachPreference`/`SetEnumChoices` calls); `src/AcDream.App/Settings/RuntimeSettingsController.cs` (`QualitySettings`/`ReapplyQualityPreset`) | | AP-199 | **Filed 2026-08-11 at Campaign OP slice OP6; CORRECTED at the OP6 rework round (2026-08-11, review M2) — the field names and the "gating to zero when disabled" wording were describing an INVERTED, muted-by-default bug, not the shipped behaviour.** The Config tab's "Sound Options" section authors three rows with no acdream consumer: `Sound_SoundFeatures` (Stereo/Mono menu — acdream's OpenAL backend has no channel-count toggle), the Interface Sound toggle+slider trio (`Sound_InterfaceSoundDisabled`/`Sound_InterfaceSoundVolume` — AP-174 already documents this as retail's OWN dead knob, "registered and then never read... interface sounds are scaled by the EFFECT knob"; acdream matches that exact behaviour rather than building a working Interface bus), and `Sound_PlaySoundOnlyWhenActive` (no window-focus-based audio mute subsystem exists). All three round-trip faithfully through the new `AudioSettings.SoundFeatures`/`InterfaceEnabled`/`InterfaceVolume`/`PlaySoundOnlyWhenActive` fields. The Sound and Ambient trios' own toggle+slider pairs are NOT covered by this row — `SfxEnabled`/`AmbientEnabled`/`Sfx`/`Ambient` are LIVE (`RuntimeSettingsController.SaveAudio` now pushes into `OpenAlAudioEngine` on every change; the effective volume is zero only when the corresponding `*Enabled` flag is false — retail's own `SoundManager::effect_sounds_enabled`/`ambient_sounds_enabled` statics default to enabled, so a fresh profile is audible, not muted). | `src/AcDream.UI.Abstractions/Panels/Settings/AudioSettings.cs`; `src/AcDream.App/UI/Layout/ConfigOptionsPageController.cs` (`BindSoundSection`) | Matches the SAME reasoning AP-174 already established for the Interface knob specifically; Sound Features and Play-Only-When-Active are honest new store-only rows with no existing or planned acdream subsystem to bind (stereo/mono output selection and window-focus audio gating are both out of this campaign's scope). | A user who changes any of these three Config-tab controls sees/hears no change — the CONTRACTED behaviour, matching retail's own Interface-knob precedent for two of the three. | `gmClient::InitUIPreferences @0x004035b0` (`AttachPreference(&Sound_SoundFeatures, ...)`/`&Sound_InterfaceSoundDisabled`/`&Sound_InterfaceSoundVolume`/`&Sound_PlaySoundOnlyWhenActive`); AP-174 (Interface-knob precedent); `SoundManager::InitPrefs @0x005503F0` (`UserPreferences::RegisterPreference` binding the enabled-sense statics) | diff --git a/src/AcDream.App/Rendering/GameWindow.cs b/src/AcDream.App/Rendering/GameWindow.cs index fb466b14..5baab269 100644 --- a/src/AcDream.App/Rendering/GameWindow.cs +++ b/src/AcDream.App/Rendering/GameWindow.cs @@ -1667,7 +1667,6 @@ public sealed class GameWindow : _retailUiLease, _uiHost, _runtime, - _runtimeSettings, _movementInput, _cameraInput, _windowCallbacks), diff --git a/src/AcDream.App/Rendering/GameWindowLifetime.cs b/src/AcDream.App/Rendering/GameWindowLifetime.cs index 53fd7b46..72605e26 100644 --- a/src/AcDream.App/Rendering/GameWindowLifetime.cs +++ b/src/AcDream.App/Rendering/GameWindowLifetime.cs @@ -72,7 +72,6 @@ internal sealed record IngressShutdownRoots( // Keeps failed physical UI bindings alive through native-window release. UiHost? RetainedUiHost, GameRuntime Runtime, - RuntimeSettingsController Settings, DispatcherMovementInputSource MovementInput, DispatcherCameraInputSource CameraInput, SilkWindowCallbackBinding? WindowCallbacks); diff --git a/src/AcDream.App/Settings/RuntimeSettingsController.cs b/src/AcDream.App/Settings/RuntimeSettingsController.cs index 91c1e088..2c804526 100644 --- a/src/AcDream.App/Settings/RuntimeSettingsController.cs +++ b/src/AcDream.App/Settings/RuntimeSettingsController.cs @@ -28,8 +28,6 @@ internal interface IRuntimeSettingsStorage void SaveChat(ChatSettings chat); - void SaveCharacter(string toonKey, CharacterSettings character); - void SaveCameraTurning(CameraTurningSettings cameraTurning); } @@ -65,9 +63,6 @@ internal sealed class JsonRuntimeSettingsStorage : IRuntimeSettingsStorage public void SaveChat(ChatSettings chat) => _store.SaveChat(chat); - public void SaveCharacter(string toonKey, CharacterSettings character) => - _store.SaveCharacter(toonKey, character); - public void SaveCameraTurning(CameraTurningSettings cameraTurning) => _store.SaveCameraTurning(cameraTurning); } @@ -544,27 +539,6 @@ internal sealed class RuntimeSettingsController : /// public void NotifyServerOptionsSeeded() => ServerOptionsSeeded?.Invoke(); - private void SaveCharacter(CharacterSettings character) - { - try - { - _storage.SaveCharacter(ActiveToonKey, character); - Character = character; - if (string.Equals( - ActiveToonKey, - DefaultToonKey, - StringComparison.OrdinalIgnoreCase)) - { - _defaultCharacter = character; - } - _log($"settings: character[{ActiveToonKey}] saved to {_storage.Location}"); - } - catch (Exception ex) - { - _log($"settings: character save failed: {ex.Message}"); - } - } - private static QualitySettings ResolveQuality(QualityPreset preset) => QualitySettings.WithEnvOverrides(QualitySettings.From(preset)); } diff --git a/tests/AcDream.App.Tests/Settings/RuntimeSettingsControllerTests.cs b/tests/AcDream.App.Tests/Settings/RuntimeSettingsControllerTests.cs index 358f0e2b..de00dfbb 100644 --- a/tests/AcDream.App.Tests/Settings/RuntimeSettingsControllerTests.cs +++ b/tests/AcDream.App.Tests/Settings/RuntimeSettingsControllerTests.cs @@ -233,12 +233,58 @@ public sealed class RuntimeSettingsControllerTests Assert.False(cmd.Value); } + [Fact] + public void SaveAudioPersistsThenPushesLiveApplyAudioWithTheSavedSnapshot() + { + // OP9 review MUST-FIX 1: SaveAudio's live push into + // IRuntimeSettingsTargets.ApplyAudio (OP6's Config-tab live-apply — + // what makes a Sound-slider drag audible immediately) lost its only + // assertion when the retired SettingsVM save-order test was deleted. + // Re-pinned directly on the now-public SaveAudio seam: persist + // FIRST, then exactly one live push carrying the saved snapshot. + var events = new List(); + var controller = CreateController(events: events); + var targets = new FakeRuntimeTargets(events); + controller.BindRuntimeTargets(targets); + events.Clear(); + + AudioSettings updated = AudioSettings.Default with { Sfx = 0.35f }; + controller.SaveAudio(updated); + + Assert.Equal(["save-audio", "target-audio"], events); + Assert.Equal(updated, Assert.Single(targets.AudioCalls)); + Assert.Equal(updated, controller.Audio); + } + + [Fact] + public void SaveAudioSkipsTheLivePushWhenPersistenceFails() + { + // Companion ordering pin: a failed persist must not push a snapshot + // the store never accepted (SaveAudio's try body runs storage → + // committed property → live target, so the throw stops all three). + var events = new List(); + var storage = new FakeStorage(events) { ThrowOnAudioSave = true }; + var controller = CreateController(storage); + var targets = new FakeRuntimeTargets(events); + controller.BindRuntimeTargets(targets); + events.Clear(); + + AudioSettings before = controller.Audio; + controller.SaveAudio(AudioSettings.Default with { Sfx = 0.35f }); + + Assert.Equal(["save-audio"], events); + Assert.Empty(targets.AudioCalls); + Assert.Equal(before, controller.Audio); + } + // OP6 rework (2026-08-11, review S5 / M2): pins the EFFECTIVE volume // ApplyAudio actually computes, not just that some target was called. - // The FakeRuntimeTargets-based "target-audio" assertion above (and its - // predecessor before this rework) only ever checked that ApplyAudio - // fired, never what it fired WITH — the exact gap that let M2's - // Enabled/Disabled inversion mute every default profile unnoticed. + // The FakeRuntimeTargets-based "target-audio" assertion above + // (SaveAudioPersistsThenPushesLiveApplyAudioWithTheSavedSnapshot — + // restored at the OP9 review round after the retired SettingsVM + // save-order test took the original with it) only ever checked that + // ApplyAudio fired, never what it fired WITH — the exact gap that let + // M2's Enabled/Disabled inversion mute every default profile unnoticed. [Theory] [InlineData(true, true, 0.6f, 0.9f, 0.6f, 0.9f)] // enabled: slider value passes through [InlineData(false, false, 0.6f, 0.9f, 0f, 0f)] // disabled: forced to zero regardless of slider @@ -1002,8 +1048,6 @@ public sealed class RuntimeSettingsControllerTests public bool ThrowOnChatSave { get; init; } - public bool ThrowOnCharacterSave { get; init; } - public DisplaySettings LoadDisplay() { DisplayLoads++; @@ -1057,16 +1101,6 @@ public sealed class RuntimeSettingsControllerTests ChatValue = chat; } - public void SaveCharacter(string toonKey, CharacterSettings character) - { - _events.Add($"save-character:{toonKey}"); - if (ThrowOnCharacterSave) - throw new IOException("character persistence failed"); - Characters[toonKey] = character; - if (string.Equals(toonKey, "default", StringComparison.OrdinalIgnoreCase)) - DefaultCharacterValue = character; - } - public CameraTurningSettings LoadCameraTurning() => CameraTurningValue; public void SaveCameraTurning(CameraTurningSettings cameraTurning)