From 67b0815c79dfae91d10c11088a2653ca48ceb3e8 Mon Sep 17 00:00:00 2001 From: Erik Date: Tue, 11 Aug 2026 08:13:24 +0200 Subject: [PATCH] =?UTF-8?q?fix(ui):=20OP5=20re-check=20residuals=20R1/R2?= =?UTF-8?q?=20(coordinator=20pass)=20=E2=80=94=20OP5=20CLOSED?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit R1: UiRoot now delivers WM_CAPTURECHANGED (0x215 — retail's own Win32 event-id space) to the element losing pointer capture on BOTH release and re-target; UiScrollbar terminates a mid-drag gesture there, completing it (one DragCompleted flush persisting the user's last-seen value) and unlatching IsDragging — a panel-close keybind mid-drag or a second-button re-target can no longer latch the drag flag forever and silently suppress every later settings flush. Normal MouseUp paths no-op (the latch is already clear when capture releases). R2: the scalar latch arms BEFORE the track-click jump applies, so the jump's own ScalarChanged tick defers its flush to the MouseUp's single DragCompleted — one flush per press gesture, never the inline-then-completed double; the DragCompleted doc now states the real contract (fires once per value-capable gesture incl. capture loss) instead of the refuted never-on-jump claim. Tests: capture-loss mid-drag (ends + completes once + stray-MouseUp no-double), no-drag capture-change no-op, bare-track-click single-completion with the latch observed armed during the jump tick. Also reconciles the research doc's U4 row to its closure (the six caption pairs, the BN zero-fold post-mortem) per the OP6 rework's flag. Full Release suite: 13,128 passed / 4 skipped / 0 failed (one documented #250-class allocation flake on first run, green in isolation and on full-suite rerun). Co-Authored-By: Claude Fable 5 --- .../2026-08-10-options-panel-structure.md | 2 +- src/AcDream.App/UI/UiEvent.cs | 7 ++ src/AcDream.App/UI/UiRoot.cs | 18 ++++ src/AcDream.App/UI/UiScrollbar.cs | 42 ++++++++-- .../AcDream.App.Tests/UI/UiScrollbarTests.cs | 83 +++++++++++++++++++ 5 files changed, 142 insertions(+), 10 deletions(-) diff --git a/docs/research/2026-08-10-options-panel-structure.md b/docs/research/2026-08-10-options-panel-structure.md index d6afe46d..002dec78 100644 --- a/docs/research/2026-08-10-options-panel-structure.md +++ b/docs/research/2026-08-10-options-panel-structure.md @@ -910,7 +910,7 @@ wire**. The user's own DAT dir currently holds `acclient.keymap` and `test.keyma | U1 | **What `m_default` is for the Character tab's 49 checkboxes.** `InitOptions` never calls `SetDefaultValue`, so "Defaults" behaviour on the biggest tab is unestablished. | Read `UIOption_Checkbox`'s ctor and `UIOption::InqDefaultGameplayOptionProperty` / `InqGameplayOptionNameAndTooltip`. **Blocking for the Defaults button.** | | U2 | **Which options are "auto-save"** (immediate `0x0005`) vs batched into the `0x01A1` blob. | `CPlayerModule::IsAutoSaveOption` — enumerate it. This is exactly handoff Q4's discriminator. | | U3 | Whether acdream ships the **50th Character row** ("Listen to PK death messages", `ID_PlayerOption_HearPKDeaths`, string `0x0D16E9A3` present in the DAT, absent from the 2013 code). | Design call. The string and ACE support exist; only the 2013 wiring is missing. | -| U4 | The exact **`SetSliderLabel` operand pairs** for the Config tab's six labelled sliders (which of `Dark/Bright`, `Speed/Detail`, `Close/Far`, `Narrow/Wide`, `Slow/Fast`, `Soft/Hard` goes with which). | Same push-imm decode technique as `AddHeader`, applied at the six `SetSliderLabel` sites. Cosmetic; not blocking. | +| U4 | ~~The exact **`SetSliderLabel` operand pairs** for the Config tab's six labelled sliders.~~ **CLOSED 2026-08-11 (OP6 review + rework).** An intermediate OP6 claim that retail ships ZERO captions was the SAME BN zero-fold artifact as `AddHeader` (the review byte-decoded the `mov ecx/edx,[disp32]` string-id loads at `0x0049E4C6` etc.; the rework independently re-read 2 of 6 sites + the PDB global sequence `0x0083E768`–`0x0083E794`). The six pairs, in declaration order: Stiffness `Soft/Hard`, Adjustment Speed `Slow/Fast`, FOV `Narrow/Wide`, Screen Brightness `Dark/Bright`, Graphics Performance `Speed/Detail`, Degrade Distance `Close/Far`. Implemented in the OP6 rework (`472525b9`). | Evidence: `docs/research/2026-08-11-op6-review.md` M1; the rework commit's own byte log. | | U5 | Semantics of layout property **`0x57`** (page/window registration) and **`0x58`** (`enum 1` on every options root). | Find the `GetAttribute_Enum(this, 0x57 …)` read. Not blocking — the toggle action can be driven from `0x12` on the buttons. | | U6 | Consumer of input actions **`0x10000027`** ("Exit Game") and **`0x1000001F`** ("Configure Keyboard"). | Neither has a default keybind. `0x1000001F` is well corroborated by the keyboard screen's OK/Cancel; `0x10000027` rests on the label alone. | | U7 | The two **`support.turbine.com` URLs** behind Urgent Assistance / Report Abuse. | Dead endpoints. Recommend a **register row**: acdream shows a "not available" notice rather than launching a browser at a dead Turbine host. Extract the exact strings only if the register row needs to quote them. | diff --git a/src/AcDream.App/UI/UiEvent.cs b/src/AcDream.App/UI/UiEvent.cs index 044026b6..13b7a16e 100644 --- a/src/AcDream.App/UI/UiEvent.cs +++ b/src/AcDream.App/UI/UiEvent.cs @@ -68,6 +68,13 @@ public static class UiEventType public const int MouseMove = 0x200; public const int MouseDown = 0x201; // left button down public const int MouseUp = 0x202; // left button up + /// WM_CAPTURECHANGED (0x215) — delivered by + /// to the element LOSING pointer capture when capture is released or + /// re-targeted WITHOUT a MouseUp reaching that element (panel hidden by a + /// keybind mid-drag; a second button press re-targeting capture). A widget + /// holding gesture state keyed to capture (the scrollbar's drag latch) + /// must terminate the gesture here (OP5 re-check R1, 2026-08-11). + public const int CaptureChanged = 0x215; public const int DoubleClickLeft = 0x203; public const int RightDown = 0x204; public const int RightUp = 0x205; diff --git a/src/AcDream.App/UI/UiRoot.cs b/src/AcDream.App/UI/UiRoot.cs index 0e05a8c6..b948dd32 100644 --- a/src/AcDream.App/UI/UiRoot.cs +++ b/src/AcDream.App/UI/UiRoot.cs @@ -787,6 +787,7 @@ public sealed class UiRoot : UiElement if (ReferenceEquals(Captured, e)) return; UiElement? previous = Captured; Captured = e; + NotifyCaptureLost(previous); PointerCaptureChanged?.Invoke(previous, e); } @@ -797,10 +798,27 @@ public sealed class UiRoot : UiElement // Retail restarts the tooltip idle deadline when capture is released. _hoverStartedMs = _nowMs; _tooltipFired = false; + NotifyCaptureLost(previous); if (previous is not null) PointerCaptureChanged?.Invoke(previous, null); } + /// OP5 re-check R1 (2026-08-11): WM_CAPTURECHANGED to the element + /// losing capture — a capture drop WITHOUT a MouseUp (panel hidden by a + /// keybind mid-drag; a second button re-targeting capture) must let the + /// element terminate any capture-keyed gesture (the scrollbar's drag + /// latch, which otherwise reads IsDragging=true forever and silently + /// suppresses every later settings flush). A normal MouseUp path is + /// unaffected: the gesture state is already cleared by the time capture + /// releases, so the handler no-ops. + private static void NotifyCaptureLost(UiElement? previous) + { + if (previous is null) return; + var lost = new UiEvent( + previous.EventId, previous, UiEventType.CaptureChanged); + previous.OnEvent(in lost); + } + // ── Window manager (named top-level windows: Show / Hide / Toggle) ─── // Registry state lives in RetailWindowManager; methods below are compatibility forwarders. diff --git a/src/AcDream.App/UI/UiScrollbar.cs b/src/AcDream.App/UI/UiScrollbar.cs index 7d207562..24b17201 100644 --- a/src/AcDream.App/UI/UiScrollbar.cs +++ b/src/AcDream.App/UI/UiScrollbar.cs @@ -44,14 +44,19 @@ public sealed class UiScrollbar : UiElement public bool IsDragging => _draggingThumb; /// - /// Fires once, at the MouseUp that ends a thumb drag — never on a - /// MouseUp that was not preceded by an actual drag (a bare click that - /// only page-scrolled or jumped, or a stray MouseUp with no prior - /// MouseDown). OP5 review fix S1: the drag-end seam neither - /// (fires on every tick) nor - /// scrolling provided — the Chat tab's opacity sliders use this to flush a - /// batched settings write exactly once per drag gesture instead of once per - /// MouseMove. + /// Fires once at the end of a press gesture that could have changed the + /// value: the MouseUp ending a MODEL-mode thumb drag, the + /// MouseUp ending ANY scalar-mode press (thumb drag OR bare + /// track-click jump — OP5 re-check R2: the scalar latch arms on + /// MouseDown before the jump applies, so the jump's flush defers + /// here rather than double-flushing), or a WM_CAPTURECHANGED + /// capture loss mid-drag (OP5 re-check R1 — the gesture completes with + /// the user's last-seen value). Never fires on a stray MouseUp + /// with no prior press, nor on model-mode button/page clicks. OP5 review + /// fix S1: the drag-end seam neither (fires + /// per tick) nor scrolling provided — the Chat tab's + /// opacity sliders flush a batched settings write exactly once per + /// gesture instead of once per MouseMove. /// public Action? DragCompleted { get; set; } @@ -372,6 +377,20 @@ public sealed class UiScrollbar : UiElement public override bool OnEvent(in UiEvent e) { + // OP5 re-check R1: a capture drop without a MouseUp (panel hidden by + // a keybind mid-drag; a second button re-targeting capture) ends the + // drag HERE — completing the gesture (flush via DragCompleted) so the + // user's last-seen value persists and IsDragging cannot latch true + // forever. A normal MouseUp already cleared the latch, so this no-ops. + if (e.Type == UiEventType.CaptureChanged) + { + bool wasDragging = _draggingThumb; + _draggingThumb = false; + _pressedButton = EndButton.None; + if (wasDragging) DragCompleted?.Invoke(); + return false; // informational — never consumes + } + if (IsModelDisabled) { _draggingThumb = false; @@ -532,6 +551,12 @@ public sealed class UiScrollbar : UiElement float travel = MathF.Max(1f, Width - thumbWidth); float thumbX = travel * ScalarPosition; float x = e.Data1; + // OP5 re-check R2: the latch is set BEFORE the track-click + // jump below, so the jump's own ScalarChanged tick reads + // IsDragging=true and DEFERS its flush to the MouseUp's + // DragCompleted — one flush per press gesture, never the + // inline-then-DragCompleted double the previous order caused. + _draggingThumb = true; if (x >= thumbX && x <= thumbX + thumbWidth) { _dragOffsetX = x - thumbX; @@ -541,7 +566,6 @@ public sealed class UiScrollbar : UiElement _dragOffsetX = thumbWidth * 0.5f; ChangeScalarPosition((x - _dragOffsetX) / travel); } - _draggingThumb = true; return true; } diff --git a/tests/AcDream.App.Tests/UI/UiScrollbarTests.cs b/tests/AcDream.App.Tests/UI/UiScrollbarTests.cs index 7a9308e5..c59fa3d8 100644 --- a/tests/AcDream.App.Tests/UI/UiScrollbarTests.cs +++ b/tests/AcDream.App.Tests/UI/UiScrollbarTests.cs @@ -178,6 +178,89 @@ public class UiScrollbarTests Assert.Equal(1, completedCount); } + // ── OP5 re-check R1/R2 (2026-08-11, coordinator pass) ─────────────────── + + [Fact] + public void CaptureLossMidDrag_EndsTheGesture_CompletesOnce_AndUnlatchesIsDragging() + { + // R1: UiRoot can drop capture WITHOUT a MouseUp (panel hidden by a + // keybind mid-drag; a second button re-targeting capture). The + // WM_CAPTURECHANGED delivery must end the drag, fire ONE completion + // (persisting the user's last-seen value), and unlatch IsDragging — + // otherwise every later Reset/Defaults flush is silently suppressed. + int completedCount = 0; + var bar = new UiScrollbar + { + Width = 90f, + Height = 14f, + Horizontal = true, + ScalarChanged = _ => { }, + DragCompleted = () => completedCount++, + }; + bar.SetScalarPosition(0f); + + Assert.True(bar.OnEvent(new UiEvent(0u, bar, UiEventType.MouseDown, Data1: 5))); + Assert.True(bar.OnEvent(new UiEvent(0u, bar, UiEventType.MouseMove, Data1: 30))); + Assert.True(bar.IsDragging); + + bar.OnEvent(new UiEvent(0u, bar, UiEventType.CaptureChanged)); + + Assert.False(bar.IsDragging); + Assert.Equal(1, completedCount); + + // A later stray MouseUp (capture already gone) must not double-fire. + bar.OnEvent(new UiEvent(0u, bar, UiEventType.MouseUp, Data1: 30)); + Assert.Equal(1, completedCount); + } + + [Fact] + public void CaptureChange_WithNoActiveDrag_IsANoOp() + { + int completedCount = 0; + var bar = new UiScrollbar + { + Width = 90f, + Height = 14f, + Horizontal = true, + DragCompleted = () => completedCount++, + }; + + bar.OnEvent(new UiEvent(0u, bar, UiEventType.CaptureChanged)); + + Assert.False(bar.IsDragging); + Assert.Equal(0, completedCount); + } + + [Fact] + public void HorizontalScalar_BareTrackClickJump_DefersItsTickAndCompletesExactlyOnce() + { + // R2: a bare track click (outside the thumb) jumps the scalar. The + // latch now arms BEFORE the jump applies, so the jump's own + // ScalarChanged tick observes IsDragging=true (a consumer defers its + // flush) and the MouseUp's single DragCompleted carries the gesture's + // one flush — never the inline-then-completed double. + int completedCount = 0; + bool draggingDuringTick = false; + UiScrollbar bar = null!; + bar = new UiScrollbar + { + Width = 90f, + Height = 14f, + Horizontal = true, + ScalarChanged = _ => draggingDuringTick = bar.IsDragging, + DragCompleted = () => completedCount++, + }; + bar.SetScalarPosition(0f); // thumb spans [0, 16] + + // Click far outside the thumb — the jump branch. + Assert.True(bar.OnEvent(new UiEvent(0u, bar, UiEventType.MouseDown, Data1: 70))); + Assert.True(draggingDuringTick); // the jump tick saw the latch armed + Assert.Equal(0, completedCount); + + Assert.True(bar.OnEvent(new UiEvent(0u, bar, UiEventType.MouseUp, Data1: 70))); + Assert.Equal(1, completedCount); // one gesture, one completion + } + [Fact] public void HorizontalModel_DragCompleted_FiresOnceAtMouseUp() {