fix(ui): OP5 review fixes — thumb sync, batched opacity writes, cull register row, tests

Fixes the OP5 (Chat tab) dual-lens review findings against e71e5a96:

- M1 (MUST-FIX): each opacity row's own apply closure now pushes its OWN
  slider's thumb from the post-link truth (bindings.Current*Opacity()),
  mirroring the OP4 binding pattern. Before this, a single-slider drag
  followed by Reset reverted the live value/link but left that slider's
  own thumb stuck at the dragged position.

- S1 (SHOULD-FIX): the Chat tab's two opacity sliders no longer round-trip
  the whole settings.json on every drag MouseMove tick. UiScrollbar gains
  IsDragging + a DragCompleted callback (fires once, at the MouseUp that
  ends an actual thumb drag); the opacity apply closures flush immediately
  when not mid-drag (Reset/Defaults/discrete edits, same as before) and
  defer to DragCompleted otherwise, collapsing dozens of per-tick writes
  into exactly one per drag gesture. Live opacity still applies every tick.

- S2 (SHOULD-FIX): filed register row AP-201 and issue #371 for the
  UiScrollablePanel whole-row-cull-vs-clip divergence the review found
  (predates OP5, made user-visible by OP5's 240-260px filter blocks). Not
  fixed in this round (a renderer-level scissor stack is out of scope
  here) — corrected the OP5 connected-gate script instead so a straddling
  block's disappear-then-reappear-whole is no longer reported as a
  self-sizing regression.

- S3 (SHOULD-FIX): the chatWindowMainFilter round-trip test already
  existed in e71e5a96 (the review missed it scrolling past line 330);
  added the genuinely missing coverage instead — a composed test pinning
  RetailUiRuntime.MountChat's window-0 SettingsStore -> ChatWindowState
  seed (MountChat itself needs live DAT access and isn't unit-testable
  directly).

- N11: ScrollbarLinkage_ModelPointsAtTheChatListBoxScroll now asserts
  through the scoped page-slot lookup (UiElement.FindDescendant) instead
  of the flat layout.FindElement, which passed for the wrong reason given
  the shared scrollbar id 0x10000201 — matches OP6's own scrollbar-linkage
  test pattern.

Also updated ConfigOptionsPageControllerTests' local ChatOptionsPageController
Bindings fake for the new FlushOpacity parameter.

Full Release suite: 13,117 passed / 4 skipped / 0 failed (baseline 13,107/4/0
post-OP6 — 10 tests added, zero skips added, zero failures).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik 2026-08-11 07:36:54 +02:00
parent e318e8628d
commit 6d0b0f9285
10 changed files with 501 additions and 20 deletions

View file

@ -0,0 +1,66 @@
using System;
using System.IO;
using AcDream.Core.Chat;
using AcDream.UI.Abstractions.Panels.Settings;
namespace AcDream.App.Tests.UI;
/// <summary>
/// OP5 review fix S3 (2026-08-11): pins the two-line seed
/// <c>RetailUiRuntime.MountChat</c> runs for the MAIN chat window's filter
/// (<c>RetailUiRuntime.cs:946-948</c>) — <c>ChatSettings.ChatWindowMainFilter</c>
/// loaded from the local <see cref="SettingsStore"/> feeds
/// <see cref="ChatWindowState.SetFilter"/> for <see cref="ChatWindowState.MainWindowId"/>,
/// the SAME "local-only persistence" leg <c>MountFloatingChatWindows</c> already
/// runs for windows 1-4 (research doc §4.4/§6.1). <c>MountChat</c> itself needs
/// live DAT access and is not unit-testable directly; this pins the seed
/// COMPOSITION (store round-trip + <see cref="ChatWindowState"/> write) instead,
/// exactly the two statements the production method runs.
/// </summary>
public sealed class ChatMainWindowFilterSeedTests : IDisposable
{
private readonly string _tempPath;
public ChatMainWindowFilterSeedTests()
{
_tempPath = Path.Combine(
Path.GetTempPath(),
$"acdream-chat-seed-test-{Guid.NewGuid():N}.json");
}
public void Dispose()
{
if (File.Exists(_tempPath)) File.Delete(_tempPath);
}
[Fact]
public void MountChatSeed_ReadsStoredMainWindowFilter_IntoChatWindowState()
{
var store = new SettingsStore(_tempPath);
store.SaveChat(ChatSettings.Default with { ChatWindowMainFilter = 0x1ul });
var windows = new ChatWindowState();
// RetailUiRuntime.MountChat's own seed, verbatim:
// _bindings.Chat.Windows.SetFilter(
// ChatWindowState.MainWindowId, chatStore.LoadChat().ChatWindowMainFilter);
windows.SetFilter(ChatWindowState.MainWindowId, store.LoadChat().ChatWindowMainFilter);
Assert.Equal(0x1ul, windows.GetFilter(ChatWindowState.MainWindowId));
}
[Fact]
public void MountChatSeed_WithNoStoredFile_SeedsTheRetailPostInitDefault()
{
var store = new SettingsStore(_tempPath); // file does not exist yet
var windows = new ChatWindowState();
windows.SetFilter(ChatWindowState.MainWindowId, store.LoadChat().ChatWindowMainFilter);
// ChatWindowState's own constructor default for window 0 IS the retail
// PostInit default (0xFBFFFFFF) already — this proves the seed is a
// faithful no-op on a fresh install, not just that it doesn't crash.
Assert.Equal(ChatWindowState.MainWindowDefaultFilter, windows.GetFilter(ChatWindowState.MainWindowId));
Assert.Equal(0xFBFFFFFFul, windows.GetFilter(ChatWindowState.MainWindowId));
}
}

View file

@ -154,6 +154,7 @@ public sealed class ChatOptionsPageControllerTests
public float ActiveOpacityDatDefault = 1.0f;
public List<float> DefaultOpacitySets { get; } = new();
public List<float> ActiveOpacitySets { get; } = new();
public int OpacityFlushes { get; private set; }
public Dictionary<int, ulong> Filters { get; } = new()
{
@ -178,6 +179,7 @@ public sealed class ChatOptionsPageControllerTests
(DefaultOpacity, ActiveOpacity) = ChatOpacityLinkFor(DefaultOpacity, value, isDefault: false);
ActiveOpacitySets.Add(value);
},
FlushOpacity: () => OpacityFlushes++,
DefaultOpacityDatDefault: DefaultOpacityDatDefault,
ActiveOpacityDatDefault: ActiveOpacityDatDefault,
CurrentFilter: windowId => Filters[windowId],
@ -338,6 +340,27 @@ public sealed class ChatOptionsPageControllerTests
}
}
/// <summary>The two opacity-slider widgets, in build order (slider1 =
/// Default, slider2 = Active) — both are <see cref="UiScrollbar"/>s built
/// with <c>Horizontal = true</c> (import-time from the authored template's
/// own wide-vs-tall extent, <c>DatWidgetFactory.cs:214</c>) driven through
/// <see cref="UiScrollbar.ScalarChanged"/> rather than <c>Model</c>. Scoped
/// to the ListBox's own subtree, so the page-level vertical list scrollbar
/// (a DIFFERENT, non-horizontal, sibling-not-descendant element) is never
/// collected.</summary>
private static List<UiScrollbar> CollectScalarSliders(UiElement listBoxRoot)
{
var found = new List<UiScrollbar>();
Walk(listBoxRoot, found);
return found;
static void Walk(UiElement node, List<UiScrollbar> acc)
{
if (node is UiScrollbar { Horizontal: true } bar) acc.Add(bar);
foreach (UiElement child in node.Children) Walk(child, acc);
}
}
[Fact]
public void Bind_SeedsSlidersFromCurrentOpacity_NotTheDatDefault()
{
@ -446,6 +469,170 @@ public sealed class ChatOptionsPageControllerTests
Assert.Equal(1.0f, bindings.ActiveOpacity);
}
// ── OP5 review fix M1: single-row Reset must revert that row's OWN thumb ─
[Fact]
public void Reset_AfterOnlyDefaultSliderChanged_RevertsTheThumbToo()
{
// MUST-FIX M1 (OP5 review, 2026-08-11): defaultRow's own apply closure
// must push slider1's OWN thumb (mirroring the OP4 binding pattern),
// not just the LINKED slider2 via RefreshFromLink. Drag Default DOWN
// only, well below Active, so the link never raises Active and
// activeRow.Changed stays false — the exact single-row-changed
// scenario the review names as reachable and previously uncovered.
var fakeBindings = new FakeBindings { DefaultOpacity = 0.3f, ActiveOpacity = 0.9f };
ImportedLayout layout = FixtureLoader.LoadOptionsPanelHost();
OptionsPanelController controller = OptionsPanelController.Bind(
layout,
new OptionsPanelController.Callbacks(
Toggle: () => { },
RequestExitToCharacterSelection: () => { },
ExitGame: () => { },
UseMouseTurningSettings: () => { },
DisplaySystemMessage: _ => { }))!;
bool bound = ChatOptionsPageController.Bind(
layout, controller.ChatPage, MakeTemplateResolver(), (_, _) => null,
fakeBindings.ToBindings());
Assert.True(bound);
var listBox = Assert.IsType<UiTemplateListBox>(
layout.FindElement(ChatOptionsPageController.ListBoxElementId));
List<UiScrollbar> sliders = CollectScalarSliders(listBox);
Assert.Equal(2, sliders.Count);
UiScrollbar slider1 = sliders[0];
var defaultRow = Assert.IsType<FloatOptionRow>(controller.ChatPage.Rows[0]);
var activeRow = Assert.IsType<FloatOptionRow>(controller.ChatPage.Rows[1]);
defaultRow.SetCurrentValue(0.1f);
Assert.Equal(0.1f, slider1.ScalarPosition, 3);
Assert.True(defaultRow.Changed);
Assert.False(activeRow.Changed); // link never touched Active
controller.ChatPage.Reset();
Assert.Equal(0.3f, defaultRow.Current);
Assert.Equal(0.3f, fakeBindings.DefaultOpacity);
Assert.Equal(0.3f, slider1.ScalarPosition, 3); // the thumb reverted too, not just the value
}
[Fact]
public void Reset_AfterOnlyActiveSliderChanged_RevertsTheThumbToo()
{
// Symmetric case the review also names: drag Active UP only, Reset.
var fakeBindings = new FakeBindings { DefaultOpacity = 0.3f, ActiveOpacity = 0.6f };
ImportedLayout layout = FixtureLoader.LoadOptionsPanelHost();
OptionsPanelController controller = OptionsPanelController.Bind(
layout,
new OptionsPanelController.Callbacks(
Toggle: () => { },
RequestExitToCharacterSelection: () => { },
ExitGame: () => { },
UseMouseTurningSettings: () => { },
DisplaySystemMessage: _ => { }))!;
bool bound = ChatOptionsPageController.Bind(
layout, controller.ChatPage, MakeTemplateResolver(), (_, _) => null,
fakeBindings.ToBindings());
Assert.True(bound);
var listBox = Assert.IsType<UiTemplateListBox>(
layout.FindElement(ChatOptionsPageController.ListBoxElementId));
List<UiScrollbar> sliders = CollectScalarSliders(listBox);
Assert.Equal(2, sliders.Count);
UiScrollbar slider2 = sliders[1];
var defaultRow = Assert.IsType<FloatOptionRow>(controller.ChatPage.Rows[0]);
var activeRow = Assert.IsType<FloatOptionRow>(controller.ChatPage.Rows[1]);
activeRow.SetCurrentValue(0.95f);
Assert.Equal(0.95f, slider2.ScalarPosition, 3);
Assert.True(activeRow.Changed);
Assert.False(defaultRow.Changed); // link never touched Default (0.6 < 0.95)
controller.ChatPage.Reset();
Assert.Equal(0.6f, activeRow.Current);
Assert.Equal(0.6f, fakeBindings.ActiveOpacity);
Assert.Equal(0.6f, slider2.ScalarPosition, 3); // the thumb reverted too, not just the value
}
// ── OP5 review fix S1: settings write batches to drag-end, not per tick ─
[Fact]
public void DraggingDefaultSlider_DefersTheSettingsWriteUntilDragEnd()
{
// SHOULD-FIX S1 (OP5 review, 2026-08-11): N MouseMove ticks inside one
// thumb-drag gesture must flush the settings write ZERO times (only
// the LIVE opacity applies per tick); the matching MouseUp flushes
// exactly once. Drives the REAL UiScrollbar event pipeline (not
// FloatOptionRow.SetCurrentValue directly) so UiScrollbar.IsDragging
// is genuinely true for the duration, exactly like a real mouse drag.
var fakeBindings = new FakeBindings { DefaultOpacity = 0.2f, ActiveOpacity = 1.0f };
ImportedLayout layout = FixtureLoader.LoadOptionsPanelHost();
OptionsPanelController controller = OptionsPanelController.Bind(
layout,
new OptionsPanelController.Callbacks(
Toggle: () => { },
RequestExitToCharacterSelection: () => { },
ExitGame: () => { },
UseMouseTurningSettings: () => { },
DisplaySystemMessage: _ => { }))!;
bool bound = ChatOptionsPageController.Bind(
layout, controller.ChatPage, MakeTemplateResolver(), (_, _) => null,
fakeBindings.ToBindings());
Assert.True(bound);
var listBox = Assert.IsType<UiTemplateListBox>(
layout.FindElement(ChatOptionsPageController.ListBoxElementId));
List<UiScrollbar> sliders = CollectScalarSliders(listBox);
UiScrollbar slider1 = sliders[0];
Assert.True(slider1.Width > 16f, $"fixture slider1.Width={slider1.Width} too narrow for this test's thumb math");
// Click INSIDE the current thumb (no "jump to click position" branch)
// so this is a clean drag start with no incidental extra flush.
float thumbWidth = MathF.Min(16f, slider1.Width);
float travel = MathF.Max(1f, slider1.Width - thumbWidth);
float thumbX = travel * slider1.ScalarPosition;
int clickX = (int)(thumbX + thumbWidth * 0.5f);
Assert.True(slider1.OnEvent(new UiEvent(0u, slider1, UiEventType.MouseDown, Data1: clickX)));
Assert.True(slider1.IsDragging);
Assert.Equal(0, fakeBindings.OpacityFlushes);
for (int i = 1; i <= 10; i++)
{
Assert.True(slider1.OnEvent(new UiEvent(0u, slider1, UiEventType.MouseMove, Data1: clickX + i)));
Assert.Equal(0, fakeBindings.OpacityFlushes); // N drag ticks = 0 saves
}
// Live opacity DID apply on every tick even though nothing flushed.
Assert.NotEqual(0.2f, fakeBindings.DefaultOpacity);
Assert.True(slider1.OnEvent(new UiEvent(0u, slider1, UiEventType.MouseUp, Data1: clickX + 10)));
Assert.False(slider1.IsDragging);
Assert.Equal(1, fakeBindings.OpacityFlushes); // drag end = 1 save
}
[Fact]
public void ResetClick_FlushesImmediately_NotMidDrag()
{
// A discrete Reset click (never mid-drag) keeps writing immediately —
// the SAME single-write-per-discrete-edit shape the pre-fix code had
// for every call, matching the review's "the filter checkboxes are
// discrete clicks and are fine as written" characterization applied
// to Reset/Defaults on the opacity rows too.
var fakeBindings = new FakeBindings { DefaultOpacity = 0.3f, ActiveOpacity = 0.9f };
(OptionsPanelController controller, FakeBindings bindings, bool bound) = BindRealWith(fakeBindings);
Assert.True(bound);
var defaultRow = Assert.IsType<FloatOptionRow>(controller.ChatPage.Rows[0]);
defaultRow.SetCurrentValue(0.1f); // not mid-drag either — flushes immediately
Assert.Equal(1, bindings.OpacityFlushes);
controller.ChatPage.Reset();
Assert.Equal(2, bindings.OpacityFlushes);
}
[Fact]
public void CheckingAFilterRow_PublishesSetFilter_ForItsOwnCompactWindowId()
{
@ -562,9 +749,28 @@ public sealed class ChatOptionsPageControllerTests
Assert.Empty(page.Rows);
}
// The tab host's own private per-page SLOT id (OptionsPanelController's
// ChatPageId) — the id that actually survives base-merge in the
// host-mounted tree (see ChatOptionsPageController.PageSlotElementId's
// own doc for why the standalone layout's root id does not survive).
// OptionsPanelController keeps its own copy private; this test-local
// literal mirrors it for a scoped lookup exactly the way the controller
// itself scopes its scrollbar linkage (ConfigOptionsPageControllerTests
// uses the identical pattern for its own scrollbar-linkage tests).
private const uint ChatPageSlotId = 0x1000050Cu;
[Fact]
public void ScrollbarLinkage_ModelPointsAtTheChatListBoxScroll()
{
// OP5 review fix N11 (2026-08-11): 0x10000201 is authored in BOTH the
// Chat and Config page slots (research doc §10.1); ImportedLayout's
// flat FindElement is last-write-wins across the whole tree, so a
// flat lookup here would pass even if the controller's OWN scoped
// lookup (ChatOptionsPageController.Bind) regressed to a flat one —
// it happens to resolve the same instance today only because Chat is
// built last. Assert through the SAME scoped path the controller
// itself uses (UiElement.FindDescendant from the page's own slot),
// matching OP6's ScrollbarLinkage_ModelPointsAtTheConfigListBoxScroll.
ImportedLayout layout = FixtureLoader.LoadOptionsPanelHost();
OptionsPanelController controller = OptionsPanelController.Bind(
layout,
@ -579,10 +785,11 @@ public sealed class ChatOptionsPageControllerTests
layout, controller.ChatPage, MakeTemplateResolver(), (_, _) => null,
fakeBindings.ToBindings());
var chatSlot = UiElement.FindDescendant(controller.TabPanel, ChatPageSlotId)!;
var listBox = Assert.IsType<UiTemplateListBox>(
layout.FindElement(ChatOptionsPageController.ListBoxElementId));
UiElement.FindDescendant(chatSlot, ChatOptionsPageController.ListBoxElementId));
var scrollbar = Assert.IsType<UiScrollbar>(
layout.FindElement(ChatOptionsPageController.ScrollbarElementId));
UiElement.FindDescendant(chatSlot, ChatOptionsPageController.ScrollbarElementId));
Assert.Same(listBox.Scroll, scrollbar.Model);
}

View file

@ -563,6 +563,7 @@ public sealed class ConfigOptionsPageControllerTests
CurrentActiveOpacity: () => ActiveOpacity,
SetDefaultOpacity: value => DefaultOpacity = value,
SetActiveOpacity: value => ActiveOpacity = value,
FlushOpacity: () => { },
DefaultOpacityDatDefault: 0.5f,
ActiveOpacityDatDefault: 1.0f,
CurrentFilter: _ => 0xFBFFFFFFul,

View file

@ -103,6 +103,104 @@ public class UiScrollbarTests
Assert.True(bar.OnEvent(new UiEvent(0u, bar, UiEventType.MouseUp, Data1: 45)));
}
// ── OP5 review fix S1: the drag-end seam (IsDragging / DragCompleted) ────
[Fact]
public void HorizontalScalar_DragCompleted_FiresOnceAtMouseUp_NotOnEachMove()
{
int completedCount = 0;
var bar = new UiScrollbar
{
Width = 90f,
Height = 14f,
Horizontal = true,
ScalarChanged = _ => { },
DragCompleted = () => completedCount++,
};
bar.SetScalarPosition(0f); // thumb spans [0, 16]
// Click INSIDE the thumb — no "jump to click" branch, a clean drag start.
Assert.True(bar.OnEvent(new UiEvent(0u, bar, UiEventType.MouseDown, Data1: 5)));
Assert.True(bar.IsDragging);
Assert.Equal(0, completedCount);
for (int i = 0; i < 10; i++)
{
Assert.True(bar.OnEvent(new UiEvent(0u, bar, UiEventType.MouseMove, Data1: 10 + i)));
Assert.Equal(0, completedCount); // N drag ticks fire zero completions
}
Assert.True(bar.OnEvent(new UiEvent(0u, bar, UiEventType.MouseUp, Data1: 50)));
Assert.False(bar.IsDragging);
Assert.Equal(1, completedCount); // drag end fires exactly one
}
[Fact]
public void HorizontalScalar_DragCompleted_DoesNotFireOnAMouseUpThatWasNeverADrag()
{
int completedCount = 0;
var bar = new UiScrollbar
{
Width = 90f,
Height = 14f,
Horizontal = true,
ScalarChanged = _ => { },
DragCompleted = () => completedCount++,
};
// A bare MouseUp with no prior MouseDown/drag must not fire the callback.
bar.OnEvent(new UiEvent(0u, bar, UiEventType.MouseUp, Data1: 10));
Assert.Equal(0, completedCount);
}
[Fact]
public void VerticalModel_DragCompleted_FiresOnlyForAnActualThumbDrag_NotAButtonClick()
{
// Height=200, default 16px decrement/increment buttons -> trackTop=16,
// trackLen=168. content=400/view=100 -> ThumbRatio=0.25 -> thumbH=42,
// travel=126. At PositionRatio=0 the thumb spans local Y [16, 58].
var model = new UiScrollable { ContentHeight = 400, ViewHeight = 100 };
int completedCount = 0;
var bar = new UiScrollbar { Width = 16f, Height = 200f, Model = model, DragCompleted = () => completedCount++ };
// A click on the decrement (up-arrow) button is never a drag.
Assert.True(bar.OnEvent(new UiEvent(0u, bar, UiEventType.MouseDown, Data1: 0, Data2: 5)));
Assert.False(bar.IsDragging);
Assert.True(bar.OnEvent(new UiEvent(0u, bar, UiEventType.MouseUp, Data1: 0, Data2: 5)));
Assert.Equal(0, completedCount);
// A click INSIDE the thumb (local Y 30, within [16, 58]) starts a real drag.
Assert.True(bar.OnEvent(new UiEvent(0u, bar, UiEventType.MouseDown, Data1: 0, Data2: 30)));
Assert.True(bar.IsDragging);
Assert.True(bar.OnEvent(new UiEvent(0u, bar, UiEventType.MouseMove, Data1: 0, Data2: 40)));
Assert.Equal(0, completedCount);
Assert.True(bar.OnEvent(new UiEvent(0u, bar, UiEventType.MouseUp, Data1: 0, Data2: 40)));
Assert.Equal(1, completedCount);
}
[Fact]
public void HorizontalModel_DragCompleted_FiresOnceAtMouseUp()
{
var model = new UiScrollable { ContentHeight = 320, ViewHeight = 80, LineHeight = 32 };
int completedCount = 0;
var bar = new UiScrollbar
{
Width = 160f,
Height = 16f,
Horizontal = true,
Model = model,
DragCompleted = () => completedCount++,
};
Assert.True(bar.OnEvent(new UiEvent(0u, bar, UiEventType.MouseDown, Data1: 20)));
Assert.True(bar.IsDragging);
Assert.True(bar.OnEvent(new UiEvent(0u, bar, UiEventType.MouseMove, Data1: 144)));
Assert.Equal(0, completedCount);
Assert.True(bar.OnEvent(new UiEvent(0u, bar, UiEventType.MouseUp, Data1: 144)));
Assert.False(bar.IsDragging);
Assert.Equal(1, completedCount);
}
[Fact]
public void HorizontalModel_ButtonsTrackAndThumbDriveSharedScroll()
{