From 4fada238e69fdfef5eb21e3a898536168843bbb4 Mon Sep 17 00:00:00 2001 From: Erik Date: Sun, 6 Sep 2026 13:51:25 +0200 Subject: [PATCH] fix(plugin-ui): Slice A review fixes - real grip/toggle children, dialog z-order, persisted collapse/hide intent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent Opus reviews of 01b98ca30 (Slice A - movable, collapsible plugin shelf) found blocking design and behavior issues plus several should-fix gaps. All addressed in this commit: 1. Grip/toggle are now REAL children instead of a drawn band + a cursor-position-dependent HandlesClick override + an OnEvent toggle hack: ShelfGripPanel (WindowMoveHandle=true) spans the top band minus the toggle width; a UiSimpleButton toggle sits beside it (HandlesClick already wins at UiRoot.OnMouseDown before the Draggable-window fallback). The shelf's own Draggable is now FALSE - verified against UiRoot.FindDragHandleWindow, which never reads a window's own Draggable flag at all (it walks for a WindowMoveHandle ancestor-or-self then climbs to the nearest child of UiRoot), so Draggable=true was never required for the grip to work and only armed the whole-window- drag fallback for clicks on the shelf's own padding - exactly the behavior the review asked NOT to have. The two Assert.Single(shelf. Children) test sites now filter by the (now internal) PluginShelfButton type instead of asserting child count. 2. Deleted the per-tick "always highest ZOrder" raise in OnTick. It fought RetailDialogFactory.Tick's own per-frame dialog re-raise, so a dialog opened while the shelf was visible could never end up on top of it. Registration's ordinary press-to-raise (a grip press calls BringToFront before the drag starts) remains. 3. Collapse and hide/show intent are now persisted through their own channel. RetainedWindowState gained a nullable RequestedVisible; RetailWindowHandle gained an internal StateChanged event that RetailWindowLayoutPersistence subscribes to (alongside Moved/Resized/ Shown/Hidden) and that the shelf raises after a collapse toggle or Show/Hide. Capture() now persists state.RequestedVisible (the panel's own intent) instead of the derived IsVisible, so an availability hide (last plugin window unregistered) is never mistaken for a user hide. WindowNames.PluginShelf is now one of RetailUiRuntime's state-managed visibility windows, so Apply() restores the intent through RestoreWindowState directly rather than via Show()/Hide(). 4. RetailWindowLayoutPersistence now subscribes to RetailWindowManager.WindowRegistered/WindowUnregistered so a window (a plugin window loaded after startup, or the shelf on any path that constructs persistence first) attaches even when it registers after persistence already exists. 5. Reflow()'s default height argument is now nullable and falls back to the last height OnTick actually measured (or unbounded if none yet), instead of always forcing a single-column layout - every call site OTHER than OnTick's own row-wrap (Add, unregister, Show/Hide, the collapse toggle, RestoreWindowState) used to collapse a wrapped multi-column layout to one column for a frame. 6. _userPositioned is now flipped only when the handle's position differs from the recorded dock placement, not on every RetailWindowHandle. Moved (which fires unconditionally on any completed window-drag release, including a zero-movement grip click, and on any ClampAllToScreen reachable-clamp). 7. New tests cover: a press+drag starting on an entry button does not move the shelf; the removed per-tick raise (a sibling with higher ZOrder keeps it after a tick); TogglePluginManager's hidden-and- collapsed -> shown-and-expanded / visible -> hidden transitions at the shelf API (no RetailUiRuntime construction harness exists in this test suite to exercise the action-routing switch itself - the "no plugin windows registered" message branch is therefore not covered here). 8. The hide branch of TogglePluginManager now displays "Plugin shelf hidden. Press Shift+Ctrl+F1 to show it again."; the show branch stays silent. 9. The collapse toggle now draws ASCII '<'/'>' instead of the DAT-font- dependent '«'/'»' glyphs (the only use of those code points in the App UI, silently dropped by UiDatFont when absent), and gets the same DatFont + bitmap fallback the shelf's entry buttons already have through UiSimpleButton. A new installed-DAT test pins that the default font actually carries both ASCII glyphs. 10. ResizeX/ResizeY are false on the shelf so a restored layout's saved dimensions can never stomp the derived Width/Height via ResizeTo. 11. WindowNames.PluginShelf replaces the "plugin-shelf" literal at every site (RetailUiRuntime, docs comments, tests). 12. The grip dims to half opacity while RetailWindowManager.IsLocked, the same visual cue every other retail window gets (the shelf's grip has no DatElementId, so RetailWindowLockPresentationController's authored- chrome dimming does not reach it on its own). Every new test was verified to fail against the pre-fix source: reverting src/AcDream.App/UI/PluginSidePanel.cs, IRetainedWindowStateController.cs, RetailWindowHandle.cs, WindowNames.cs, and RetailUiRuntime.cs to their 01b98ca30 state makes the whole PluginSidePanelTests.cs file fail to even compile (missing WindowNames.PluginShelf, the now-internal PluginShelfButton type, and RetainedWindowState.RequestedVisible); reverting RetailWindowLayoutPersistence.cs alone (fixed source elsewhere) makes WindowRegisteredAfterConstruction_StillRoundTrips fail at runtime with a null saved layout, confirming finding 4 in isolation. Verified: dotnet build src/AcDream.App (Release) green; dotnet test tests/AcDream.App.Tests (Release) targeted filter (PluginSidePanel|RetailWindowLayout|Markup|UiRootInput) 105/105 green; full suite 7303 passed / 97 skipped / 36 failed (identical failure set to the pre-fix-round baseline - installed-DAT live-mount probes, Linux-only pacing/credential tests, and alpha-flush COUNT-only conformance divergences, none touching plugin UI; +9 tests, all passing, over the prior 7294/97/36 baseline). Co-Authored-By: Claude Fable 5.1 --- .../UI/IRetainedWindowStateController.cs | 16 +- src/AcDream.App/UI/PluginSidePanel.cs | 356 +++++++++++------- src/AcDream.App/UI/RetailUiRuntime.cs | 24 +- src/AcDream.App/UI/RetailWindowHandle.cs | 15 + .../UI/RetailWindowLayoutPersistence.cs | 60 ++- src/AcDream.App/UI/WindowNames.cs | 5 + .../UI/PluginSidePanelTests.cs | 351 ++++++++++++++++- .../UI/RetailWindowLayoutPersistenceTests.cs | 22 ++ .../UI/UiDatFontBorderPixelTests.cs | 37 ++ 9 files changed, 725 insertions(+), 161 deletions(-) diff --git a/src/AcDream.App/UI/IRetainedWindowStateController.cs b/src/AcDream.App/UI/IRetainedWindowStateController.cs index cdb129bcd..095a22942 100644 --- a/src/AcDream.App/UI/IRetainedWindowStateController.cs +++ b/src/AcDream.App/UI/IRetainedWindowStateController.cs @@ -1,11 +1,25 @@ namespace AcDream.App.UI; /// Panel state that is not completely described by outer-frame bounds. +/// +/// Review fix round (finding 3, docs/plans/2026-09-06-plugin-shelf-and-dat-icons.md +/// Slice A): the panel's own persisted show/hide INTENT, independent of the +/// outer frame's derived (which for a +/// controller like PluginSidePanel also folds in an availability gate — +/// see that class's ApplyVisibility). Null means "no override, fall back +/// to " — every existing state +/// controller that never had an intent distinct from derived visibility keeps +/// working unchanged. 's +/// Capture/Apply read and write this instead of the outer frame's +/// raw visibility so an availability-driven hide is never mistaken for (and +/// then persisted as) a user hide. +/// public readonly record struct RetainedWindowState( bool Collapsed = false, bool Maximized = false, float? PersistedTop = null, - float? PersistedHeight = null); + float? PersistedHeight = null, + bool? RequestedVisible = null); /// /// Optional state seam used by retained-window persistence. Bounds are restored diff --git a/src/AcDream.App/UI/PluginSidePanel.cs b/src/AcDream.App/UI/PluginSidePanel.cs index fa1defb76..2d0cfb912 100644 --- a/src/AcDream.App/UI/PluginSidePanel.cs +++ b/src/AcDream.App/UI/PluginSidePanel.cs @@ -10,11 +10,12 @@ namespace AcDream.App.UI; /// /// Slice A (2026-09-06, docs/plans/2026-09-06-plugin-shelf-and-dat-icons.md): /// the shelf is itself a retained window ( -/// registers it as "plugin-shelf") so it gets drag, the UI lock, and -/// position/visibility/collapsed -/// persistence for free, exactly like every other retained window. It implements -/// for the collapse flag and -/// to separate "has entries to show" +/// registers it as ) so it gets drag, the UI +/// lock, and position/visibility/ +/// collapsed persistence for free, exactly like every other retained window. It +/// implements for the collapse flag +/// (and now the persisted show/hide intent — see ) +/// and to separate "has entries to show" /// (availability) from the user's own show/hide choice — the same pattern /// PluginWindowVisibilityController uses for individual plugin windows, /// just applied synchronously (see ) so unit tests @@ -24,19 +25,47 @@ namespace AcDream.App.UI; /// /// /// -/// The grip/collapse-toggle band across the top is drawn, not a child element: -/// paints it and / -/// handle its input by inspecting the live pointer position against the shelf's -/// OWN bounds. This is deliberate — a real child element for the toggle would -/// have to sit outside any grip subtree -/// (nesting it inside would let the drag-handle promotion swallow its press -/// before the button ever sees a click; see UiRoot.OnMouseDown's -/// handleWindow is not null priority over HandlesClick), and a -/// grip element as a plain sibling would change 's -/// count/shape, which the existing single-button shelf tests assert directly. -/// Keeping the whole shelf and excluding just -/// the toggle's pixel rect from gets the same -/// grip-drags/buttons-and-toggle-don't behavior without adding any child. +/// Review fix round (2026-09-06): the grip and collapse toggle are now REAL +/// child elements — ( +/// ) and a plain toggle beside +/// it — exactly the mechanism every other retail window with a non-title-bar move +/// strip already uses (ChatLayoutConformanceTests.MountedChatWindow_TopStrip_IsAMoveHandleNotAGrip +/// pins the same pattern on the imported chat window's top strip). The original +/// Slice A implementation drew the grip/toggle and computed a live, cursor-position- +/// dependent override instead — a real design +/// smell (input handling required reading the CURRENT mouse position out of +/// from inside a hit-test-adjacent property getter) kept only +/// because a real toggle CHILD would have had to sit outside the grip's own +/// subtree — which is exactly where it +/// belongs anyway: only walks INTO a +/// ancestor chain looking for the flag: a +/// SIBLING toggle button never enters that walk, so nesting was never required. +/// +/// +/// +/// is on the shelf +/// itself (verified against , which does +/// not read a window's own at all — it walks +/// from the pressed element for a +/// ancestor-or-self, then climbs to the nearest child of . A +/// press on therefore resolves handleWindow == this and +/// drags regardless of ). Leaving the shelf +/// would ALSO satisfy the grip drag, but would +/// additionally arm UiRoot.OnMouseDown's whole-window-drag fallback +/// (window is {{ Draggable: true }}) for any press that lands on the +/// shelf's own padding — between entry buttons, or in the margin around them — +/// since neither the padding nor the shelf itself declares +/// . That is exactly the behavior Slice A's +/// review round asked NOT to have ("the GRIP drags, the padding between buttons +/// does NOT"), so stays false: only +/// 's subtree can +/// start a move. The one cost is that a press on padding or an entry/toggle +/// button no longer raises the shelf to front via UiRoot's +/// FindWindow(target) fallback (which requires +/// or on an ancestor-or-self) — only a grip +/// press does. This is an acceptable trade: a click can only ever land on the +/// shelf's exposed region in the first place (whatever already won the Z-order +/// hit-test), so there is nothing behind it left to reveal. /// /// public sealed class PluginSidePanel : UiPanel, IDisposable, IRetainedWindowStateController, IRetainedPanelController @@ -50,8 +79,8 @@ public sealed class PluginSidePanel : UiPanel, IDisposable, IRetainedWindowState /// total height (only the grip remains when collapsed). private const float GripHeight = 12f; - /// Width of the collapse-toggle glyph's clickable rect, anchored - /// to the grip's right end. + /// Width of the collapse-toggle button, anchored to the grip's + /// right end. private const float ToggleWidth = 16f; /// Collapsed shelf width — just enough for the toggle glyph. @@ -63,6 +92,8 @@ public sealed class PluginSidePanel : UiPanel, IDisposable, IRetainedWindowState private readonly Func _resolve; private readonly UiDatFont? _font; private readonly Dictionary _entries = []; + private readonly ShelfGripPanel _grip; + private readonly UiSimpleButton _toggle; private bool _disposed; private float _lastLayoutHeight = -1f; @@ -74,17 +105,20 @@ public sealed class PluginSidePanel : UiPanel, IDisposable, IRetainedWindowState /// The user's/persisted show-hide intent, independent of whether /// there are currently any entries to show (see ). /// Shift+Ctrl+F1 (InputAction.TogglePluginManager) flips this via - /// /. + /// /. Persisted directly (review fix + /// round finding 3): see / + /// and . private bool _requestedVisible = true; - /// True once the shelf has been dragged, or a persisted layout - /// with a differing position has been restored onto it. While false, the - /// shelf is still in its default right-edge dock and - /// preserves the DOCKED (top-right) corner on growth; once true, it - /// preserves the top-left corner instead (see the class doc + Slice A - /// plan item 2). Tracked via on the - /// shelf's own registration, so unregistered (legacy unit-test) use never - /// sets this and behaves like the shelf has always been docked. + /// True once the shelf has been dragged away from its current + /// docked position (a real grip drag, or a persisted layout with a + /// differing position restored onto it — see ). + /// While false, the shelf is still docked and preserves + /// the DOCKED (top-right) corner on growth; once true, it preserves the + /// top-left corner instead (see the class doc + Slice A plan item 2). + /// Tracked via on the shelf's own + /// registration, so unregistered (legacy unit-test) use never sets this and + /// behaves like the shelf has always been docked. private bool _userPositioned; /// True once the one-time initial dock placement (right edge, top @@ -93,10 +127,27 @@ public sealed class PluginSidePanel : UiPanel, IDisposable, IRetainedWindowState /// position to preserve a corner of). private bool _initialDockApplied; + /// + /// The last position or the one-time dock placement + /// left the shelf at while it was still docked ( + /// is false). Review fix round finding 6: + /// fires unconditionally on EVERY window-drag release — including a + /// zero-movement grip click, and every + /// reachability re-clamp — not only a genuine drag. Comparing the handle's + /// CURRENT position against this recorded dock placement (rather than + /// treating any as "the user moved + /// it") is what lets a same-position click, or an unrelated clamp that + /// happens to leave the shelf exactly where the dock formula already had + /// it, pass through without flipping . + /// + private float _dockLeft; + private float _dockTop; + /// The shelf's own retained-window handle, captured the moment - /// RetailUiRuntime.MountPlugins registers it as "plugin-shelf" - /// (via ). Null when the shelf is used - /// unregistered, e.g. by tests that predate Slice A. + /// RetailUiRuntime.MountPlugins registers it as + /// (via ). + /// Null when the shelf is used unregistered, e.g. by tests that predate + /// Slice A. private RetailWindowHandle? _handle; public PluginSidePanel( @@ -112,14 +163,41 @@ public sealed class PluginSidePanel : UiPanel, IDisposable, IRetainedWindowState Height = GripHeight + OuterPadding * 2f; Top = DefaultTop; Anchors = AnchorEdges.None; - Draggable = true; + // See the class doc for why this stays false: FindDragHandleWindow does + // not require it for the grip to drag, and leaving it true would also + // arm the whole-window-drag fallback for padding clicks. + Draggable = false; ConstrainDragToParent = true; Resizable = false; + // Nit 10: the shelf's Width/Height are entirely derived (Reflow), so a + // restored layout's saved dimensions must never stomp them via ResizeTo. + ResizeX = false; + ResizeY = false; BackgroundColor = new Vector4(0f, 0f, 0f, 0.88f); BorderColor = new Vector4(0.62f, 0.48f, 0.16f, 1f); BorderThickness = 1f; Visible = false; + _grip = new ShelfGripPanel + { + WindowMoveHandle = true, + BackgroundColor = Vector4.Zero, + BorderColor = Vector4.Zero, + }; + _toggle = new UiSimpleButton + { + BackgroundColor = Vector4.Zero, + BorderColor = Vector4.Zero, + TextColor = ToggleGlyphColor, + DatFont = _font, + Outline = true, + TextSource = () => _collapsed ? "<" : ">", + }; + _toggle.Click += ToggleCollapsed; + AddChild(_grip); + AddChild(_toggle); + LayoutChrome(); + _windows.WindowUnregistered += OnWindowUnregistered; _windows.WindowRegistered += OnWindowRegistered; } @@ -197,6 +275,7 @@ public sealed class PluginSidePanel : UiPanel, IDisposable, IRetainedWindowState if (_collapsed) _collapsed = false; Reflow(); + _handle?.NotifyStateChanged(); } /// Hide the shelf. Preserves entries/positions; never disables a @@ -205,12 +284,20 @@ public sealed class PluginSidePanel : UiPanel, IDisposable, IRetainedWindowState { _requestedVisible = false; Reflow(); + _handle?.NotifyStateChanged(); } protected override void OnTick(double deltaSeconds) { base.OnTick(deltaSeconds); + // Nit 12: dim the grip under the global UI lock, the same visual cue + // every retail window gets from RetailWindowLockPresentationController — + // that controller only recognizes AUTHORED dat chrome ids, and the grip + // is a runtime-created element with no DatElementId, so it needs its + // own lock-driven dim here. + _grip.Opacity = _windows.IsLocked ? 0.5f : 1f; + if (Parent is { } parent) { // Row-wrap: re-flow into however many columns fit the live parent @@ -235,108 +322,33 @@ public sealed class PluginSidePanel : UiPanel, IDisposable, IRetainedWindowState { Left = MathF.Max(0f, parent.Width - Width - OuterPadding); _initialDockApplied = true; - } - - // Keep the shelf itself reachable across a display resize — the - // same reachability guarantee KeepWindowReachable below gives the - // individual plugin windows. A direct clamp (not handle.MoveTo) - // so it never reads as a user move. - if (parent.Width > 0f && parent.Height > 0f) - { - float clampedLeft = Math.Clamp(Left, 0f, MathF.Max(0f, parent.Width - Width)); - float clampedTop = Math.Clamp(Top, 0f, MathF.Max(0f, parent.Height - Height)); - if (clampedLeft != Left || clampedTop != Top) - { - Left = clampedLeft; - Top = clampedTop; - } + _dockLeft = Left; + _dockTop = Top; } } foreach (RetailWindowHandle handle in _entries.Keys) KeepWindowReachable(handle); - // The shelf remains reachable even after ordinary windows are raised. - if (Parent is { } root) - { - int highest = 0; - foreach (UiElement sibling in root.Children) - { - if (!ReferenceEquals(sibling, this)) - highest = Math.Max(highest, sibling.ZOrder); - } - if (ZOrder <= highest) - ZOrder = highest == int.MaxValue ? highest : highest + 1; - } + // Review fix round finding 2: the per-tick "always highest ZOrder" raise + // that used to live here is DELETED. Registration already gives the + // shelf press-to-raise (a grip press resolves as this window's own + // move-drag and calls UiRoot.BringToFront before the drag starts); + // forcing it back to the top of EVERY sibling on every tick fought + // RetailDialogFactory.Tick's own re-raise of open dialogs (it + // re-asserts each dialog's Z-order every tick specifically so the + // screen's opaque backdrop cannot bury it — see that method's own doc + // comment), so a dialog opened while the shelf was visible could never + // actually end up on top of it. Retail dialogs must outrank the shelf. } - /// - /// A press within the collapse-toggle's rect (top-right of the grip band) - /// is handled by this element instead of promoting to a whole-window drag - /// — see UiRoot.OnMouseDown's target.HandlesClick branch, - /// checked BEFORE its window is {{ Draggable: true }} fallback. - /// Computed from the live pointer position (not a cached hit-test side - /// effect) because this getter has exactly one caller in the whole - /// codebase (OnMouseDown), which sets UiRoot.MouseX/MouseY - /// to the press coordinates immediately before reading it. - /// - public override bool HandlesClick + private void ToggleCollapsed() { - get - { - if (FindRoot() is not { } root) return false; - Vector2 sp = ScreenPosition; - return IsWithinToggleRect(root.MouseX - sp.X, root.MouseY - sp.Y); - } + _collapsed = !_collapsed; + Reflow(); + _handle?.NotifyStateChanged(); } - public override bool OnEvent(in UiEvent e) - { - if (e.Type == UiEventType.Click && Enabled && IsWithinToggleRect(e.Data1, e.Data2)) - { - _collapsed = !_collapsed; - Reflow(); - return true; - } - return false; - } - - protected override void OnDraw(UiRenderContext ctx) - { - base.OnDraw(ctx); - - // Grip strip: three short dashes centered in the band (excluding the - // toggle's own rect) — a subtle drag affordance in the existing - // border color. - const float dashWidth = 5f; - const float dashGap = 4f; - float totalDashWidth = dashWidth * 3f + dashGap * 2f; - float dashX = MathF.Max(2f, (Width - ToggleWidth - totalDashWidth) * 0.5f); - float dashY = GripHeight * 0.5f - 1f; - for (int i = 0; i < 3; i++) - ctx.DrawFill(dashX + i * (dashWidth + dashGap), dashY, dashWidth, 2f, BorderColor); - - string glyph = _collapsed ? "«" : "»"; // « / » - if (_font is { } dat) - { - float glyphWidth = dat.MeasureWidth(glyph); - ctx.DrawStringDat( - dat, - glyph, - Width - ToggleWidth + (ToggleWidth - glyphWidth) * 0.5f, - (GripHeight - dat.LineHeight) * 0.5f, - ToggleGlyphColor, - outline: true); - } - } - - private static bool IsWithinToggleRect(float localX, float localY, float width) - => localY >= 0f && localY < GripHeight - && localX >= width - ToggleWidth && localX < width; - - private bool IsWithinToggleRect(float localX, float localY) - => IsWithinToggleRect(localX, localY, Width); - private void OnWindowRegistered(RetailWindowHandle handle) { if (!ReferenceEquals(handle.OuterFrame, this)) return; @@ -345,7 +357,11 @@ public sealed class PluginSidePanel : UiPanel, IDisposable, IRetainedWindowState _windows.WindowRegistered -= OnWindowRegistered; } - private void OnHandleMoved(RetailWindowHandle _) => _userPositioned = true; + private void OnHandleMoved(RetailWindowHandle _) + { + if (Left != _dockLeft || Top != _dockTop) + _userPositioned = true; + } // ── IRetainedPanelController: separates "has entries" (availability) from // the user's own show/hide request — see ApplyVisibility. ──────────────── @@ -360,13 +376,29 @@ public sealed class PluginSidePanel : UiPanel, IDisposable, IRetainedWindowState _requestedVisible = false; } - // ── IRetainedWindowStateController: collapse persistence ──────────────── + // ── IRetainedWindowStateController: collapse + requested-visible intent ── - public RetainedWindowState CaptureWindowState() => new(Collapsed: _collapsed); + public RetainedWindowState CaptureWindowState() => + new(Collapsed: _collapsed, RequestedVisible: _requestedVisible); + /// + /// Review fix round finding 3: restores BOTH the collapse flag and the + /// persisted show/hide INTENT directly onto , + /// then re-derives through the same synchronous + /// path every other state change uses — never + /// through /, so a login-time restore + /// never fires an extra BringToFront or a redundant state-changed + /// save. registers + /// as one of 's state-managed + /// visibility windows, so RetailWindowLayoutPersistence.Apply calls + /// this instead of / + /// for visibility. + /// public void RestoreWindowState(RetainedWindowState state) { _collapsed = state.Collapsed; + if (state.RequestedVisible is { } requestedVisible) + _requestedVisible = requestedVisible; Reflow(); } @@ -403,6 +435,23 @@ public sealed class PluginSidePanel : UiPanel, IDisposable, IRetainedWindowState handle.MoveTo(left, top); } + /// Repositions the grip/toggle children to span the current + /// Width — called whenever changes it (entries added/ + /// removed, collapse toggled, row-wrap) so the toggle always sits flush + /// against the shelf's right edge and the grip fills the rest of the band. + private void LayoutChrome() + { + _grip.Left = 0f; + _grip.Top = 0f; + _grip.Width = MathF.Max(0f, Width - ToggleWidth); + _grip.Height = GripHeight; + + _toggle.Left = Width - ToggleWidth; + _toggle.Top = 0f; + _toggle.Width = ToggleWidth; + _toggle.Height = GripHeight; + } + /// /// Recomputes button positions and the shelf's own Width/Height for the /// current entries + collapsed state, then (once the shelf has been @@ -411,17 +460,32 @@ public sealed class PluginSidePanel : UiPanel, IDisposable, IRetainedWindowState /// false), top-left once the user has moved it. Always reflows the full /// button set (even while collapsed, with Visible=false) so /// expanding is instant, per Slice A plan item 4. + /// + /// Review fix round finding 5: the parameterless default no longer + /// forces a single-column layout. Every OTHER call site (, + /// unregister, /, the collapse toggle, + /// ) calls this with no argument — only + /// 's row-wrap logic ever passes an explicit height. + /// Defaulting to "unbounded" on every OTHER call site collapsed a + /// multi-column wrapped layout back to one column for a single frame + /// (until the next row-wrap pass corrected it), + /// visible as a one-frame reflow flash on every collapse/expand. Reusing + /// (the last height + /// actually measured) when no explicit height is given keeps the same + /// column count these calls would already be using. /// - private void Reflow(float maximumHeight = float.PositiveInfinity) + private void Reflow(float? maximumHeight = null) { + float effectiveHeight = maximumHeight + ?? (_lastLayoutHeight >= 0f ? _lastLayoutHeight : float.PositiveInfinity); float oldWidth = Width; - int maximumRows = float.IsPositiveInfinity(maximumHeight) + int maximumRows = float.IsPositiveInfinity(effectiveHeight) ? Math.Max(1, _entries.Count) : Math.Max( 1, (int)MathF.Floor( - (maximumHeight - OuterPadding * 2f + ButtonGap) + (effectiveHeight - OuterPadding * 2f + ButtonGap) / (ButtonExtent + ButtonGap))); int index = 0; foreach (ShelfEntry entry in _entries.Values) @@ -458,7 +522,14 @@ public sealed class PluginSidePanel : UiPanel, IDisposable, IRetainedWindowState if (_initialDockApplied && !_userPositioned) Left += oldWidth - Width; + LayoutChrome(); ApplyVisibility(); + + if (_initialDockApplied && !_userPositioned) + { + _dockLeft = Left; + _dockTop = Top; + } } /// Availability (has entries) AND the user's requested-visible @@ -495,7 +566,30 @@ public sealed class PluginSidePanel : UiPanel, IDisposable, IRetainedWindowState PluginShelfButton Button, PluginMinimizeButton Minimize); - private sealed class PluginShelfButton : UiSimpleButton + /// The top drag-grip band — a real + /// child (review fix round finding 1), drawn as three short dashes centered + /// in its own bounds (which already exclude the toggle's rect — see + /// ). is dimmed by + /// under the global UI lock (nit 12). + private sealed class ShelfGripPanel : UiPanel + { + private static readonly Vector4 DashColor = new(0.62f, 0.48f, 0.16f, 1f); + + protected override void OnDraw(UiRenderContext ctx) + { + base.OnDraw(ctx); + + const float dashWidth = 5f; + const float dashGap = 4f; + float totalDashWidth = dashWidth * 3f + dashGap * 2f; + float dashX = MathF.Max(2f, (Width - totalDashWidth) * 0.5f); + float dashY = Height * 0.5f - 1f; + for (int i = 0; i < 3; i++) + ctx.DrawFill(dashX + i * (dashWidth + dashGap), dashY, dashWidth, 2f, DashColor); + } + } + + internal sealed class PluginShelfButton : UiSimpleButton { private static readonly Vector4 HiddenBackground = new(0.025f, 0.025f, 0.02f, 0.96f); diff --git a/src/AcDream.App/UI/RetailUiRuntime.cs b/src/AcDream.App/UI/RetailUiRuntime.cs index d765f1204..fc031471d 100644 --- a/src/AcDream.App/UI/RetailUiRuntime.cs +++ b/src/AcDream.App/UI/RetailUiRuntime.cs @@ -678,6 +678,13 @@ public sealed class RetailUiRuntime : IDisposable // session that ended mid-trade was restoring an empty // open window at every launch). WindowNames.SecureTrade, + // Review fix round finding 3 (2026-09-06): the shelf folds + // an availability gate (EntryCount > 0) on top of the + // user's own show/hide request — PluginSidePanel's + // IRetainedWindowStateController restores the saved INTENT + // itself (RestoreWindowState), so this layer must not also + // drive Show/Hide from the derived layout.Visible. + WindowNames.PluginShelf, ]); } @@ -1057,9 +1064,19 @@ public sealed class RetailUiRuntime : IDisposable return true; } if (shelf.Visible) + { shelf.Hide(); + // Finding 8: hiding has no other affordance to bring it + // back (no menu entry, no button) — tell the user the + // exact chord that reopens it. The show branch stays + // silent; showing something is its own feedback. + _bindings.Options.DisplaySystemMessage( + "Plugin shelf hidden. Press Shift+Ctrl+F1 to show it again."); + } else + { shelf.Show(); + } return true; case AcDream.UI.Abstractions.Input.InputAction.ToggleAbuseReportingPanel: _bindings.Options.DisplaySystemMessage(OptionsPanelText.ReportAbuseUnavailable); @@ -4709,13 +4726,14 @@ public sealed class RetailUiRuntime : IDisposable _bindings.Assets.DefaultFont); Host.Root.AddChild(_pluginSidePanel); // Slice A: the shelf is itself a retained window (stable - // key "plugin-shelf") so drag, the UI lock, and + // key WindowNames.PluginShelf) so drag, the UI lock, and // RetailWindowLayoutPersistence apply for free — the // panel implements both IRetainedPanelController (the // availability-vs-requested-visible split) and - // IRetainedWindowStateController (collapse) itself. + // IRetainedWindowStateController (collapse + persisted + // show/hide intent) itself. Host.WindowManager.Register( - "plugin-shelf", + WindowNames.PluginShelf, _pluginSidePanel, _pluginSidePanel, controller: _pluginSidePanel); diff --git a/src/AcDream.App/UI/RetailWindowHandle.cs b/src/AcDream.App/UI/RetailWindowHandle.cs index fd7d12e68..ee8328f27 100644 --- a/src/AcDream.App/UI/RetailWindowHandle.cs +++ b/src/AcDream.App/UI/RetailWindowHandle.cs @@ -57,6 +57,18 @@ public sealed class RetailWindowHandle public event Action? Moved; public event Action? Resized; public event Action? Closed; + + /// + /// Review fix round (finding 3, docs/plans/2026-09-06-plugin-shelf-and-dat-icons.md + /// Slice A): raised by a controller-owned state change that Moved/Resized/ + /// Shown/Hidden do not cover — e.g. PluginSidePanel's collapse toggle + /// or its own /, which route through + /// instead of the outer frame's + /// Visible property. + /// subscribes here (alongside Moved/Resized/Shown/Hidden) to trigger a save. + /// Internal: no consumer outside this assembly needs it today. + /// + internal event Action? StateChanged; public event Action? LockChanged; public event Action? DescendantFocusChanged; public event Action? DescendantCaptureChanged; @@ -111,6 +123,9 @@ public sealed class RetailWindowHandle internal void NotifyMoved() => Moved?.Invoke(this); internal void NotifyResized() => Resized?.Invoke(this); + /// Raises . See that event's own doc. + internal void NotifyStateChanged() => StateChanged?.Invoke(this); + internal void NotifyClosed() { if (_closedSinceShown) return; diff --git a/src/AcDream.App/UI/RetailWindowLayoutPersistence.cs b/src/AcDream.App/UI/RetailWindowLayoutPersistence.cs index de2ae0614..9a0353dd5 100644 --- a/src/AcDream.App/UI/RetailWindowLayoutPersistence.cs +++ b/src/AcDream.App/UI/RetailWindowLayoutPersistence.cs @@ -36,10 +36,22 @@ public sealed class RetailWindowLayoutPersistence : IDisposable ? new HashSet(StringComparer.Ordinal) : new HashSet(stateManagedVisibilityWindows, StringComparer.Ordinal); + // Review fix round finding 4: attach late registrations too. Before this + // fix, only windows already registered at CONSTRUCTION time ever got a + // save subscription — a plugin window (or the plugin shelf, on a path + // that somehow constructs persistence first) registered afterward was + // silently never persisted. WindowUnregistered detaches the mirror image + // so a stale handle is not held (and re-notified) forever. + _manager.WindowRegistered += OnWindowRegistered; + _manager.WindowUnregistered += OnWindowUnregistered; foreach (RetailWindowHandle handle in manager.Windows) Attach(handle); } + private void OnWindowRegistered(RetailWindowHandle handle) => Attach(handle); + + private void OnWindowUnregistered(RetailWindowHandle handle) => Detach(handle); + /// Restore all registered windows after character and screen are /// known. (#390): the login-time restore keeps /// its lazy schema-migration save; the LIVE display-change reload passes @@ -206,9 +218,12 @@ public sealed class RetailWindowLayoutPersistence : IDisposable private void Attach(RetailWindowHandle handle) { + if (_attached.Contains(handle)) + return; _attached.Add(handle); handle.Moved += OnChanged; handle.Resized += OnChanged; + handle.StateChanged += OnChanged; if (!_stateManagedVisibilityWindows.Contains(handle.Name)) { handle.Shown += OnChanged; @@ -216,6 +231,20 @@ public sealed class RetailWindowLayoutPersistence : IDisposable } } + private void Detach(RetailWindowHandle handle) + { + if (!_attached.Remove(handle)) + return; + handle.Moved -= OnChanged; + handle.Resized -= OnChanged; + handle.StateChanged -= OnChanged; + if (!_stateManagedVisibilityWindows.Contains(handle.Name)) + { + handle.Shown -= OnChanged; + handle.Hidden -= OnChanged; + } + } + private void OnChanged(RetailWindowHandle handle) { if (_restoring || _disposed) return; @@ -245,7 +274,12 @@ public sealed class RetailWindowLayoutPersistence : IDisposable state.PersistedTop ?? handle.Top, handle.Width, state.PersistedHeight ?? handle.Height, - handle.IsVisible, + // Review fix round finding 3: persist the controller's own show/hide + // INTENT when it reports one, never the outer frame's derived + // IsVisible — a controller (e.g. PluginSidePanel) may fold in an + // availability gate on top of the user's actual request, and an + // availability-driven hide must never be captured as a user hide. + state.RequestedVisible ?? handle.IsVisible, state.Collapsed, state.Maximized, handle.AuthoredGeometryRevision); @@ -292,9 +326,17 @@ public sealed class RetailWindowLayoutPersistence : IDisposable float y = Math.Clamp(FiniteOr(layout.Y, handle.Top), 0f, maxY); handle.MoveTo(x, y); + // Review fix round finding 3: always hand the saved intent to the + // controller (not only when restoreVisibility is true) — a + // state-managed window (restoreVisibility false) restores its own + // show/hide intent THIS way instead of through Show/Hide below, so + // login never fires an extra BringToFront or routes through the + // ordinary Shown/Hidden notification for a window whose visibility + // this persistence layer does not otherwise touch. handle.StateController?.RestoreWindowState(new RetainedWindowState( Collapsed: layout.Collapsed, - Maximized: layout.Maximized)); + Maximized: layout.Maximized, + RequestedVisible: layout.Visible)); if (restoreVisibility) { @@ -335,16 +377,10 @@ public sealed class RetailWindowLayoutPersistence : IDisposable { if (_disposed) return; _disposed = true; - foreach (RetailWindowHandle handle in _attached) - { - handle.Moved -= OnChanged; - handle.Resized -= OnChanged; - if (!_stateManagedVisibilityWindows.Contains(handle.Name)) - { - handle.Shown -= OnChanged; - handle.Hidden -= OnChanged; - } - } + _manager.WindowRegistered -= OnWindowRegistered; + _manager.WindowUnregistered -= OnWindowUnregistered; + foreach (RetailWindowHandle handle in _attached.ToArray()) + Detach(handle); _attached.Clear(); } } diff --git a/src/AcDream.App/UI/WindowNames.cs b/src/AcDream.App/UI/WindowNames.cs index f641c7afc..716e0d60b 100644 --- a/src/AcDream.App/UI/WindowNames.cs +++ b/src/AcDream.App/UI/WindowNames.cs @@ -48,4 +48,9 @@ public static class WindowNames /// Campaign QT slice QT5: the three-tab Contracts/Journal/Page /// List panel (). public const string Journal = "journal"; + + /// Slice A (2026-09-06, + /// docs/plans/2026-09-06-plugin-shelf-and-dat-icons.md): the movable, + /// collapsible plugin shelf (). + public const string PluginShelf = "plugin-shelf"; } diff --git a/tests/AcDream.App.Tests/UI/PluginSidePanelTests.cs b/tests/AcDream.App.Tests/UI/PluginSidePanelTests.cs index 61852ac2c..ba931e580 100644 --- a/tests/AcDream.App.Tests/UI/PluginSidePanelTests.cs +++ b/tests/AcDream.App.Tests/UI/PluginSidePanelTests.cs @@ -1,3 +1,4 @@ +using System.Numerics; using AcDream.App.UI; using AcDream.Plugin.Abstractions; using AcDream.UI.Abstractions.Panels.Settings; @@ -70,8 +71,12 @@ public sealed class PluginSidePanelTests Assert.True(shelf.Visible); Assert.Equal(1, shelf.EntryCount); - UiSimpleButton shelfButton = Assert.IsAssignableFrom( - Assert.Single(shelf.Children)); + // Review fix round finding 1: the grip and collapse toggle are now + // real sibling children too, so shelf.Children is no longer just the + // one entry button — filter by the entry-button TYPE instead of + // asserting a raw child count. + PluginSidePanel.PluginShelfButton shelfButton = Assert.Single( + shelf.Children.OfType()); shelfButton.OnEvent(new UiEvent { Type = UiEventType.Click }); Assert.False(handle.IsVisible); @@ -141,7 +146,7 @@ public sealed class PluginSidePanelTests using var shelf = new PluginSidePanel( root.WindowManager, _ => (0u, 0, 0), font: null); root.AddChild(shelf); - root.WindowManager.Register("plugin-shelf", shelf, shelf, controller: shelf); + root.WindowManager.Register(WindowNames.PluginShelf, shelf, shelf, controller: shelf); var frame = new UiPanel { Width = 200f, Height = 100f }; root.AddChild(frame); @@ -170,7 +175,7 @@ public sealed class PluginSidePanelTests using var shelf = new PluginSidePanel( root.WindowManager, _ => (0u, 0, 0), font: null); root.AddChild(shelf); - root.WindowManager.Register("plugin-shelf", shelf, shelf, controller: shelf); + root.WindowManager.Register(WindowNames.PluginShelf, shelf, shelf, controller: shelf); for (int i = 0; i < 6; i++) { @@ -202,7 +207,7 @@ public sealed class PluginSidePanelTests using var shelf = new PluginSidePanel( root.WindowManager, _ => (0u, 0, 0), font: null); root.AddChild(shelf); - root.WindowManager.Register("plugin-shelf", shelf, shelf, controller: shelf); + root.WindowManager.Register(WindowNames.PluginShelf, shelf, shelf, controller: shelf); var frame = new UiPanel { Width = 200f, Height = 100f }; root.AddChild(frame); @@ -245,7 +250,7 @@ public sealed class PluginSidePanelTests using var shelf = new PluginSidePanel( root.WindowManager, _ => (0u, 0, 0), font: null); root.AddChild(shelf); - root.WindowManager.Register("plugin-shelf", shelf, shelf, controller: shelf); + root.WindowManager.Register(WindowNames.PluginShelf, shelf, shelf, controller: shelf); var frame = new UiPanel { Width = 200f, Height = 100f }; root.AddChild(frame); @@ -278,7 +283,7 @@ public sealed class PluginSidePanelTests using var shelf = new PluginSidePanel( root.WindowManager, _ => (0u, 0, 0), font: null); root.AddChild(shelf); - root.WindowManager.Register("plugin-shelf", shelf, shelf, controller: shelf); + root.WindowManager.Register(WindowNames.PluginShelf, shelf, shelf, controller: shelf); var frame = new UiPanel { Width = 200f, Height = 100f }; root.AddChild(frame); @@ -292,8 +297,8 @@ public sealed class PluginSidePanelTests float expandedWidth = shelf.Width; float leftBeforeCollapse = shelf.Left; - UiSimpleButton button = Assert.IsAssignableFrom( - Assert.Single(shelf.Children)); + PluginSidePanel.PluginShelfButton button = Assert.Single( + shelf.Children.OfType()); Assert.True(button.Visible); // Click the collapse toggle at the grip's right end through the real @@ -329,7 +334,7 @@ public sealed class PluginSidePanelTests using var shelf = new PluginSidePanel( root.WindowManager, _ => (0u, 0, 0), font: null); root.AddChild(shelf); - root.WindowManager.Register("plugin-shelf", shelf, shelf, controller: shelf); + root.WindowManager.Register(WindowNames.PluginShelf, shelf, shelf, controller: shelf); var frame1 = new UiPanel { Width = 200f, Height = 100f }; root.AddChild(frame1); @@ -377,7 +382,7 @@ public sealed class PluginSidePanelTests using var shelf = new PluginSidePanel( root.WindowManager, _ => (0u, 0, 0), font: null); root.AddChild(shelf); - root.WindowManager.Register("plugin-shelf", shelf, shelf, controller: shelf); + root.WindowManager.Register(WindowNames.PluginShelf, shelf, shelf, controller: shelf); var frame = new UiPanel { Width = 200f, Height = 100f }; root.AddChild(frame); @@ -391,12 +396,12 @@ public sealed class PluginSidePanelTests using var persistence = new RetailWindowLayoutPersistence( root.WindowManager, store, () => "Alice", () => (800, 600)); - root.WindowManager.MoveTo("plugin-shelf", 120f, 88f); + root.WindowManager.MoveTo(WindowNames.PluginShelf, 120f, 88f); shelf.RestoreWindowState(new RetainedWindowState(Collapsed: true)); shelf.Hide(); UiWindowLayout saved = Assert.IsType( - store.LoadWindowLayout("Alice", "800x600", "plugin-shelf", default)); + store.LoadWindowLayout("Alice", "800x600", WindowNames.PluginShelf, default)); Assert.Equal((120f, 88f), (saved.X, saved.Y)); Assert.False(saved.Visible); Assert.True(saved.Collapsed); @@ -406,7 +411,7 @@ public sealed class PluginSidePanelTests using var shelf2 = new PluginSidePanel( root2.WindowManager, _ => (0u, 0, 0), font: null); root2.AddChild(shelf2); - root2.WindowManager.Register("plugin-shelf", shelf2, shelf2, controller: shelf2); + root2.WindowManager.Register(WindowNames.PluginShelf, shelf2, shelf2, controller: shelf2); var frame2 = new UiPanel { Width = 200f, Height = 100f }; root2.AddChild(frame2); RetailWindowHandle pluginHandle2 = root2.WindowManager.Register( @@ -430,4 +435,322 @@ public sealed class PluginSidePanelTests Directory.Delete(directory, recursive: true); } } + + // ── Review fix round (2026-09-06): grip/toggle real children, dialog + // z-order, persisted collapse/hide intent ────────────────────────────── + + [Fact] + public void Drag_StartingOnAnEntryButton_DoesNotMoveTheShelf() + { + // Finding 1 / finding 7a: a press that lands on a real entry button + // (UiSimpleButton.HandlesClick => true) must be handled by the button, + // never promoted to a whole-shelf drag. + var root = new UiRoot { Width = 800f, Height = 600f }; + using var shelf = new PluginSidePanel( + root.WindowManager, _ => (0u, 0, 0), font: null); + root.AddChild(shelf); + root.WindowManager.Register(WindowNames.PluginShelf, shelf, shelf, controller: shelf); + + var frame = new UiPanel { Width = 200f, Height = 100f }; + root.AddChild(frame); + RetailWindowHandle pluginHandle = root.WindowManager.Register( + "plugin:acdream.test:main", frame); + shelf.Add( + new PluginUiOwner("acdream.test", "Test Plugin"), + new PluginPanelDescriptor("main", "Test Plugin"), + pluginHandle); + root.Tick(0.016d, 16L); + + PluginSidePanel.PluginShelfButton button = Assert.Single( + shelf.Children.OfType()); + Vector2 buttonScreen = button.ScreenPosition; + int pressX = (int)buttonScreen.X + 5; + int pressY = (int)buttonScreen.Y + 5; + float leftBefore = shelf.Left; + float topBefore = shelf.Top; + + root.OnMouseDown(UiMouseButton.Left, pressX, pressY); + root.OnMouseMove(pressX + 100, pressY + 100); + root.OnMouseUp(UiMouseButton.Left, pressX + 100, pressY + 100); + + Assert.Equal(leftBefore, shelf.Left); + Assert.Equal(topBefore, shelf.Top); + } + + [Fact] + public void PerTickZOrderRaise_Removed_ADialogAddedAfterKeepsItsHigherZOrder() + { + // Finding 2: the OnTick block that used to bump the shelf's ZOrder + // above every sibling, every frame, is gone. Registration's own + // press-to-raise remains (a grip drag calls UiRoot.BringToFront before + // the drag starts) but ticking alone must never re-assert dominance — + // RetailDialogFactory.Tick re-raises open dialogs every frame the same + // way, and the two must not fight. + var root = new UiRoot { Width = 800f, Height = 600f }; + using var shelf = new PluginSidePanel( + root.WindowManager, _ => (0u, 0, 0), font: null); + root.AddChild(shelf); + root.WindowManager.Register(WindowNames.PluginShelf, shelf, shelf, controller: shelf); + + var frame = new UiPanel { Width = 200f, Height = 100f }; + root.AddChild(frame); + RetailWindowHandle pluginHandle = root.WindowManager.Register( + "plugin:acdream.test:main", frame); + shelf.Add( + new PluginUiOwner("acdream.test", "Test Plugin"), + new PluginPanelDescriptor("main", "Test Plugin"), + pluginHandle); + root.Tick(0.016d, 16L); + + int shelfZBefore = shelf.ZOrder; + var dialog = new UiPanel { Width = 100f, Height = 60f, ZOrder = shelfZBefore + 50 }; + root.AddChild(dialog); + + root.Tick(0.016d, 16L); + root.Tick(0.016d, 16L); + + Assert.Equal(shelfZBefore, shelf.ZOrder); + Assert.True(dialog.ZOrder > shelf.ZOrder); + } + + [Fact] + public void ToggleClick_SavesCollapsedOnly_WithNoSubsequentHideOrMove() + { + // Finding 3a. + string directory = Path.Combine( + Path.GetTempPath(), "acdream-plugin-shelf-tests-" + Guid.NewGuid().ToString("N")); + string path = Path.Combine(directory, "settings.json"); + try + { + var store = new SettingsStore(path); + var root = new UiRoot { Width = 800f, Height = 600f }; + using var shelf = new PluginSidePanel( + root.WindowManager, _ => (0u, 0, 0), font: null); + root.AddChild(shelf); + root.WindowManager.Register(WindowNames.PluginShelf, shelf, shelf, controller: shelf); + + var frame = new UiPanel { Width = 200f, Height = 100f }; + root.AddChild(frame); + RetailWindowHandle pluginHandle = root.WindowManager.Register( + "plugin:acdream.test:main", frame); + shelf.Add( + new PluginUiOwner("acdream.test", "Test Plugin"), + new PluginPanelDescriptor("main", "Test Plugin"), + pluginHandle); + root.Tick(0.016d, 16L); + + using var persistence = new RetailWindowLayoutPersistence( + root.WindowManager, store, () => "Alice", () => (800, 600), + stateManagedVisibilityWindows: [WindowNames.PluginShelf]); + + float topBefore = shelf.Top; + float rightEdgeBefore = shelf.Left + shelf.Width; + + int toggleX = (int)shelf.Left + (int)shelf.Width - 8; + int toggleY = (int)shelf.Top + 4; + root.OnMouseDown(UiMouseButton.Left, toggleX, toggleY); + root.OnMouseUp(UiMouseButton.Left, toggleX, toggleY); + + UiWindowLayout saved = Assert.IsType( + store.LoadWindowLayout("Alice", "800x600", WindowNames.PluginShelf, default)); + Assert.True(saved.Collapsed); + Assert.True(saved.Visible); // no Hide happened + Assert.Equal(topBefore, saved.Y); // no vertical Move happened + // Collapsing shrinks Width; the shelf is still DOCKED, so Reflow's + // own anchor math shifts Left to keep the right edge fixed — that + // is not a "Move" (it never goes through handle.MoveTo/NotifyMoved), + // just the collapse's own geometry, captured in the same save. + Assert.Equal(shelf.Left, saved.X); + Assert.Equal(rightEdgeBefore, shelf.Left + shelf.Width, precision: 3); + } + finally + { + if (Directory.Exists(directory)) + Directory.Delete(directory, recursive: true); + } + } + + [Fact] + public void UnregisteringTheLastPlugin_DoesNotPersistTheAvailabilityHideAsAUserHide() + { + // Finding 3b. + string directory = Path.Combine( + Path.GetTempPath(), "acdream-plugin-shelf-tests-" + Guid.NewGuid().ToString("N")); + string path = Path.Combine(directory, "settings.json"); + try + { + var store = new SettingsStore(path); + var root = new UiRoot { Width = 800f, Height = 600f }; + using var shelf = new PluginSidePanel( + root.WindowManager, _ => (0u, 0, 0), font: null); + root.AddChild(shelf); + root.WindowManager.Register(WindowNames.PluginShelf, shelf, shelf, controller: shelf); + + var frame = new UiPanel { Width = 200f, Height = 100f }; + root.AddChild(frame); + RetailWindowHandle pluginHandle = root.WindowManager.Register( + "plugin:acdream.test:main", frame); + shelf.Add( + new PluginUiOwner("acdream.test", "Test Plugin"), + new PluginPanelDescriptor("main", "Test Plugin"), + pluginHandle); + root.Tick(0.016d, 16L); + + using var persistence = new RetailWindowLayoutPersistence( + root.WindowManager, store, () => "Alice", () => (800, 600), + stateManagedVisibilityWindows: [WindowNames.PluginShelf]); + + // Establish a genuine "the user wants this visible" save. + shelf.Show(); + Assert.True(shelf.Visible); + + root.WindowManager.Unregister(pluginHandle.Name); + Assert.Equal(0, shelf.EntryCount); + Assert.False(shelf.Visible); // derived Visible DOES flip (no entries)... + + // ...but the persisted INTENT must still read visible=true: the + // availability-hide is not subscribed for a state-managed window. + UiWindowLayout saved = Assert.IsType( + store.LoadWindowLayout("Alice", "800x600", WindowNames.PluginShelf, default)); + Assert.True(saved.Visible); + } + finally + { + if (Directory.Exists(directory)) + Directory.Delete(directory, recursive: true); + } + } + + [Fact] + public void RestoreWindowState_HiddenAndCollapsedIntent_AppliesDirectly_ThenShowExpandsAndReveals() + { + // Finding 3c: restoring Collapsed=true/RequestedVisible=false onto a + // fresh shelf that already has entries must hide AND collapse it — + // without going through Show/Hide (RestoreWindowState sets the intent + // and re-derives Visible through the same synchronous ApplyVisibility + // path every other state change uses). Show() must then reveal AND + // expand it. + var root = new UiRoot { Width = 800f, Height = 600f }; + using var shelf = new PluginSidePanel( + root.WindowManager, _ => (0u, 0, 0), font: null); + root.AddChild(shelf); + root.WindowManager.Register(WindowNames.PluginShelf, shelf, shelf, controller: shelf); + + var frame = new UiPanel { Width = 200f, Height = 100f }; + root.AddChild(frame); + RetailWindowHandle pluginHandle = root.WindowManager.Register( + "plugin:acdream.test:main", frame); + shelf.Add( + new PluginUiOwner("acdream.test", "Test Plugin"), + new PluginPanelDescriptor("main", "Test Plugin"), + pluginHandle); + root.Tick(0.016d, 16L); + Assert.True(shelf.Visible); + + shelf.RestoreWindowState(new RetainedWindowState(Collapsed: true, RequestedVisible: false)); + + Assert.False(shelf.Visible); + Assert.True(shelf.CaptureWindowState().Collapsed); + + shelf.Show(); + + Assert.True(shelf.Visible); + Assert.False(shelf.CaptureWindowState().Collapsed); + } + + [Fact] + public void CollapseExpandViaTheToggle_DoesNotCollapseTheColumnWrapToOneColumn() + { + // Finding 5: every Reflow() call site OTHER than OnTick's row-wrap + // used to default to an unbounded height, forcing a single-column + // layout for one frame after Add/remove/collapse/expand/restore — + // reusing _lastLayoutHeight keeps the column count OnTick already + // established. 12 entries at 800x260 forces a real multi-column wrap. + var root = new UiRoot { Width = 800f, Height = 260f }; + using var shelf = new PluginSidePanel( + root.WindowManager, _ => (0u, 0, 0), font: null); + root.AddChild(shelf); + root.WindowManager.Register(WindowNames.PluginShelf, shelf, shelf, controller: shelf); + + for (int i = 0; i < 12; i++) + { + var frame = new UiPanel { Width = 200f, Height = 100f }; + root.AddChild(frame); + RetailWindowHandle handle = root.WindowManager.Register($"plugin:test:{i}", frame); + shelf.Add( + new PluginUiOwner($"test.{i}", $"Plugin {i}"), + new PluginPanelDescriptor("main", $"Plugin {i}"), + handle); + } + root.Tick(0.016d, 16L); + + // Collapse via the real toggle click. + int toggleX = (int)shelf.Left + (int)shelf.Width - 8; + int toggleY = (int)shelf.Top + 4; + root.OnMouseDown(UiMouseButton.Left, toggleX, toggleY); + root.OnMouseUp(UiMouseButton.Left, toggleX, toggleY); + + // Expand via the real toggle click (coordinates recomputed: the + // shelf's collapsed Width moved the toggle). + toggleX = (int)shelf.Left + (int)shelf.Width - 8; + toggleY = (int)shelf.Top + 4; + root.OnMouseDown(UiMouseButton.Left, toggleX, toggleY); + root.OnMouseUp(UiMouseButton.Left, toggleX, toggleY); + + root.Tick(0.016d, 16L); + + Assert.True(shelf.Top + shelf.Height <= root.Height); + Assert.All( + shelf.Children.OfType(), + button => Assert.True(button.Top + button.Height <= shelf.Height)); + } + + [Fact] + public void ZeroMovementGripReleaseDoesNotFlipAnchoring_ButARealDragDoes() + { + // Finding 6: RetailWindowHandle.Moved fires unconditionally on every + // completed window-drag gesture — including a press+release on the + // grip with NO intervening mouse move. (RetailWindowManager.MoveTo + // itself short-circuits an already-equal position before ever firing + // Moved, so the meaningful equivalent of "a MoveTo to the identical + // dock position" is this zero-pixel drag-release, not a same-value + // MoveTo call.) That must not be mistaken for a user drag; a REAL + // drag still must. + var root = new UiRoot { Width = 800f, Height = 600f }; + using var shelf = new PluginSidePanel( + root.WindowManager, _ => (0u, 0, 0), font: null); + root.AddChild(shelf); + root.WindowManager.Register(WindowNames.PluginShelf, shelf, shelf, controller: shelf); + + var frame = new UiPanel { Width = 200f, Height = 100f }; + root.AddChild(frame); + RetailWindowHandle pluginHandle = root.WindowManager.Register( + "plugin:acdream.test:main", frame); + shelf.Add( + new PluginUiOwner("acdream.test", "Test Plugin"), + new PluginPanelDescriptor("main", "Test Plugin"), + pluginHandle); + root.Tick(0.016d, 16L); // establishes the initial right-edge dock + + int pressX = (int)shelf.Left + 10; + int pressY = (int)shelf.Top + 5; + root.OnMouseDown(UiMouseButton.Left, pressX, pressY); + root.OnMouseUp(UiMouseButton.Left, pressX, pressY); // zero movement + + float rightEdgeBeforeCollapse = shelf.Left + shelf.Width; + shelf.RestoreWindowState(new RetainedWindowState(Collapsed: true)); + // Still docked: the RIGHT edge survived the width change. + Assert.Equal(rightEdgeBeforeCollapse, shelf.Left + shelf.Width, precision: 3); + shelf.RestoreWindowState(new RetainedWindowState(Collapsed: false)); + + // Now a REAL drag via the grip. + root.OnMouseDown(UiMouseButton.Left, pressX, pressY); + root.OnMouseMove(pressX + 40, pressY + 40); + root.OnMouseUp(UiMouseButton.Left, pressX + 40, pressY + 40); + + float leftAfterDrag = shelf.Left; + shelf.RestoreWindowState(new RetainedWindowState(Collapsed: true)); + // User-positioned now: the LEFT edge survives instead. + Assert.Equal(leftAfterDrag, shelf.Left); + } } diff --git a/tests/AcDream.App.Tests/UI/RetailWindowLayoutPersistenceTests.cs b/tests/AcDream.App.Tests/UI/RetailWindowLayoutPersistenceTests.cs index 4a81e5cec..848f12eba 100644 --- a/tests/AcDream.App.Tests/UI/RetailWindowLayoutPersistenceTests.cs +++ b/tests/AcDream.App.Tests/UI/RetailWindowLayoutPersistenceTests.cs @@ -402,6 +402,28 @@ public sealed class RetailWindowLayoutPersistenceTests : IDisposable Assert.True(state.Restored.Maximized); } + // ── Review fix round finding 4 (plugin-shelf campaign, 2026-09-06): + // attach late registrations, not only windows present at construction ── + + [Fact] + public void WindowRegisteredAfterConstruction_StillRoundTrips() + { + var store = new SettingsStore(PathName); + var root = new UiRoot { Width = 800, Height = 600 }; + using var persistence = new RetailWindowLayoutPersistence( + root.WindowManager, store, () => "Alice", () => (800, 600)); + + // The window is registered AFTER persistence already exists — before + // this fix only windows present in RetailWindowManager.Windows at + // construction time ever got a save subscription. + RetailWindowHandle handle = Mount(root, "late-window"); + handle.MoveTo(77f, 88f); + + UiWindowLayout saved = Assert.IsType( + store.LoadWindowLayout("Alice", "800x600", "late-window", default)); + Assert.Equal((77f, 88f), (saved.X, saved.Y)); + } + private static RetailWindowHandle Mount( UiRoot root, string name, diff --git a/tests/AcDream.App.Tests/UI/UiDatFontBorderPixelTests.cs b/tests/AcDream.App.Tests/UI/UiDatFontBorderPixelTests.cs index 2614f17df..c9cc1a516 100644 --- a/tests/AcDream.App.Tests/UI/UiDatFontBorderPixelTests.cs +++ b/tests/AcDream.App.Tests/UI/UiDatFontBorderPixelTests.cs @@ -1,7 +1,10 @@ using System; using System.Collections.Generic; using System.IO; +using AcDream.App.Rendering; +using AcDream.App.Tests.Rendering.Gpu; using AcDream.App.UI; +using AcDream.Content; using DatReaderWriter; using DatReaderWriter.DBObjs; using DatReaderWriter.Options; @@ -80,6 +83,40 @@ public sealed class UiDatFontBorderPixelTests Assert.True(checkedCount > 10, "expected the documented font sweep to find multiple populated fonts"); } + /// + /// Review fix round finding 9 (plugin-shelf campaign, 2026-09-06): the + /// plugin shelf's collapse toggle now draws ASCII </> + /// instead of the «/» glyphs, which were the only use of + /// those code points anywhere in the App UI and which + /// silently skips when absent ( — + /// callers just don't draw a missing glyph, no error). Pins that the + /// default retail font (0x40000000, the same font + /// PluginSidePanel's buttons already resolve through + /// AcDream.App.RuntimeOptions-style DefaultFont wiring) actually + /// carries both ASCII code points, using the same + /// -backed + /// idiom TextureCacheLinearTwinTests uses to load a real dat font + /// with no live GPU. + /// + [Fact] + [Trait("Lane", "InstalledDat")] + public void RealDatFont_HasLessThanAndGreaterThanGlyphs() + { + string? datDir = ResolveDatDir(); + if (datDir is null) + Assert.Fail("Lane=InstalledDat requires an installed retail DAT directory; see docs/release-gate.md."); + + using var dats = new DatCollection(datDir, DatAccessType.Read); + using var adapter = new DatCollectionAdapter(dats); + var device = new RecordingGpuDevice(); + var cache = new TextureCache(device, adapter); + + UiDatFont? font = UiDatFont.Load(adapter, cache); + Assert.NotNull(font); + Assert.True(font!.TryGetGlyph('<', out _), "default font is missing '<' (toggle glyph)"); + Assert.True(font.TryGetGlyph('>', out _), "default font is missing '>' (toggle glyph)"); + } + /// /// Pure plumbing check — no dat, no GL: the ctor /// stores / verbatim onto