fix(chat): CH2 re-review nits — resize centering, top-aligned flow, sweep wording

Applies the seven NITs from the CH2 re-review (verdict APPROVE-WITH-FIXES,
following the REJECT->rework at e0e78883):

1. SpewBoxController's centered Left was captured once via
   AnchorEdges.Top and replayed forever on resize (UiElement.ApplyAnchor's
   Left/Right-both-false branch pins a fixed margin). Anchors is now
   AnchorEdges.None and Tick recomputes Left every frame against the
   current root width.

2. OneLine=false was defaulting to UiText's bottom-pinned transcript flow
   (VerticalJustify honored only via ConfigureDatState, which this
   synthesized element never calls). Added UiText.HonorVerticalJustification
   so a non-DAT controller can opt the scrollable path into
   VerticalJustify without a full LayoutDesc binding; SpewBoxController
   sets VerticalJustify=Top so lines flow from the top of the 450x72 box,
   matching newest-at-top insert semantics. Noted as invented-pending-
   measurement in AP-178's row (no new row).

3. Documented the deliberate inversion of UiText.LinesProvider's
   oldest-first contract in SpewBoxController.Tick (SpewBoxVM.Lines feeds
   newest-first, which is correct specifically because the box is now
   top-aligned) and added a test pinning the rendered order (newer message
   is the topmost line), driving root.Tick.

4. Fixed the stale "retail's code default, 1" comment in
   SpewBoxControllerTests — MaxConcurrentItems is the shipped LayoutDesc's
   AUTHORED value, 4.

5. Added the matching unmapped-id diagnostics line to
   LiveSessionRuntimeFactory's ShowWeenieError sink, matching the pattern
   GameEventWiring's WeenieError/WeenieErrorWithString handlers already
   use.

6. Corrected the "EXHAUSTIVE Portal sweep found ZERO" overclaim in
   SpewBoxLayoutDumpDiagnostic: the loop's id source was DatCollection's
   top-level aggregate GetAllIdsOfType<LayoutDesc>(), not dats.Portal's
   own (which reports a count of ZERO for this type), so querying those
   ids against dats.Portal.TryGet established nothing about Portal either
   way. Corrected the same overclaim echoed in SpewBoxState's
   MaxConcurrentItems doc comment and in AP-178's register text (both the
   table row and the section-header history line). What's actually
   established: dats.Local hosts the SpewBox layout at 0x21000011; whether
   Portal also carries a copy remains unestablished.

7. Added a test exercising the full ShowWeenieError -> AddText -> SpewBox
   path for id 0x0561 (the 50-friends-cap refusal) in
   LiveSessionCommandRouterTests, mirroring LiveSessionRuntimeFactory's
   ShowWeenieError closure exactly since every other LiveSessionRuntimeFactory
   test in this tree is a source-text conformance grep, not an
   instantiation.

Ledger: CH2 ledger row's review column now reads REJECT -> reworked
e0e78883 -> re-review APPROVE-WITH-FIXES -> nits (this commit); Status
header flips CH2 to code-complete/closed pending the user gate, CH3 next.

Build green; touched-project tests green (19/19 new/changed,
4351/3354 App.Tests unaffected pass); full Release suite 11,916 passed /
4 skipped / 0 failed (baseline 11,914/4/0 plus the two new tests this
commit adds).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Erik 2026-08-09 18:48:13 +02:00
parent 235820b4f6
commit 233c30d13f
9 changed files with 216 additions and 41 deletions

File diff suppressed because one or more lines are too long

View file

@ -1,10 +1,12 @@
# Campaign CH — chat & interface-text retail parity
**Status:** ACTIVE 2026-08-09 — CH1 complete (implementation `172c6f9a` +
review fixes `34d8a3c0`); CH2 code-complete (`77c8296e`), REJECT-reviewed
(`docs/research/2026-08-09-ch2-review-findings.md`) and reworked
(`e0e78883`), pending the in-client user gate (jump-in-air / jump-loaded
refusals showing on-screen, not in chat).
review fixes `34d8a3c0`); CH2 CODE-COMPLETE and CLOSED pending the user
gate (`77c8296e`, REJECT-reviewed at
`docs/research/2026-08-09-ch2-review-findings.md`, reworked `e0e78883`,
re-reviewed APPROVE-WITH-FIXES with nits applied this commit) — the sole
outstanding item is the in-client user gate (jump-in-air / jump-loaded
refusals showing on-screen, not in chat). CH3 (side channels) next.
**Why now:** first track of the alpha-release program (chat is the most
visible daily surface for the friend-alpha). User-directed 2026-08-09.
@ -103,7 +105,7 @@ implementer per slice against a pinned contract (per
|---|---|---|---|---|
| R1R4 research | `see docs/research/2026-08-09-chat-retail-*` | — | — | — |
| CH1 colors | `172c6f9a` | 11,835 passed / 4 skipped / 0 failed | APPROVE-WITH-FIXES; fixed `34d8a3c0` | pending |
| CH2 interface text | `77c8296e`, reworked `e0e78883` | 11,914 passed / 4 skipped / 0 failed | REJECT; reworked `e0e78883` (`docs/research/2026-08-09-ch2-review-findings.md`) | pending |
| CH2 interface text | `77c8296e`, reworked `e0e78883` | 11,916 passed / 4 skipped / 0 failed | REJECT → reworked `e0e78883` → re-review APPROVE-WITH-FIXES → nits (this commit) | pending |
| CH3 side channels | — | — | — | — |
| CH4 commands | — | — | — | — |
| CH5 closeout | — | — | — | — |

View file

@ -359,6 +359,15 @@ internal sealed class LiveSessionRuntimeFactory
(string? text, RetailLogTextType type) = WeenieErrorMessages.Resolve(code, null);
if (text is not null)
_domain.Communication.AddText(text, type);
else
// CH2 re-review nit 5 (docs/plans/2026-08-09-chat-parity-campaign.md):
// matches GameEventWiring's WeenieError/
// WeenieErrorWithString handlers, which already log
// this same case. An unmapped id resolves to a null
// Text — retail's switch has no default case, so it
// produces no player-facing text — but the raw id
// stays visible to us instead of silently vanishing.
Console.WriteLine($"[weenie-error] unmapped code=0x{code:X4}");
},
PlayerPublicWeenieBitfield: () =>
_domain.EntityObjects.Objects.Get(_player.Identity.ServerGuid)?

View file

@ -1,4 +1,5 @@
using System.Numerics;
using AcDream.App.UI.Layout;
using AcDream.UI.Abstractions.Panels.SpewBox;
namespace AcDream.App.UI;
@ -83,11 +84,23 @@ internal sealed class SpewBoxController : IDisposable
/// (<c>leftEdge=3</c>/<c>rightEdge=3</c>, "centered" per
/// <c>ElementReader.ToAnchors</c>'s doc comment) mean the box is a
/// FIXED-width block horizontally centered in its parent, not a
/// full-viewport stretch — the constructor below anchors it that way
/// (a one-time centered <see cref="UiText.Left"/> computed against
/// <see cref="UiRoot.Width"/>, <see cref="AnchorEdges.Top"/> only) since
/// <see cref="AnchorEdges"/> has no "centered, fixed-width" flag
/// combination to express retail's mode 3 directly.
/// full-viewport stretch. <see cref="AnchorEdges"/> has no "centered,
/// fixed-width" flag combination to express retail's mode 3 directly, so
/// <see cref="Tick"/> recomputes a centered <see cref="UiText.Left"/>
/// against the CURRENT <see cref="UiRoot.Width"/> every frame instead of
/// anchoring.
/// <para>
/// CH2 re-review nit 1
/// (<c>docs/plans/2026-08-09-chat-parity-campaign.md</c>): the original
/// rework anchored with <see cref="AnchorEdges.Top"/> only, meaning
/// <see cref="UiElement.ApplyAnchor"/> captured the constructor's
/// one-time centered <c>Left</c> as a fixed left MARGIN
/// (<c>ComputeAnchoredRect</c>'s Left/Right-both-false branch) and
/// replayed that absolute pixel position forever — a window resize left
/// the box off-center. <see cref="Anchors"/> is now
/// <see cref="AnchorEdges.None"/> (so <c>ApplyAnchor</c> is a no-op) and
/// centering is owned entirely by the per-frame recompute below.
/// </para>
/// </summary>
private const float SpewBoxWidth = 450f;
private const float SpewBoxHeight = 72f;
@ -127,17 +140,32 @@ internal sealed class SpewBoxController : IDisposable
Name = "SpewBox",
// Centered fixed-width block (retail's "mode 3" edge code on
// both left and right) — see the AP-178 extent comment above.
// This is only the FIRST frame's value; Tick recomputes it
// every frame against the current root width (CH2 re-review
// nit 1 — see the extent comment's nit-1 paragraph).
Left = (root.Width - SpewBoxWidth) / 2f,
Top = TopOffset,
Width = SpewBoxWidth,
Height = SpewBoxHeight,
Anchors = AnchorEdges.Top,
Anchors = AnchorEdges.None,
Centered = true,
// AUTHORED MaxConcurrentItems is 4, not retail's code-default 1
// (see SpewBoxState.MaxConcurrentItems) — OneLine=true would
// silently collapse the box back down to showing only the
// newest of up to 4 concurrent lines.
OneLine = false,
// CH2 re-review nit 2 (docs/plans/2026-08-09-chat-parity-campaign.md):
// retail's own authored vertical justification for THIS element
// is unknown (register row AP-178 covers presentation
// unknowns) — top-aligned flow is acdream's own choice, made
// because it is the only placement consistent with "lines flow
// from the top, newest on top" (see Tick's ordering comment).
// HonorVerticalJustification opts the scrollable multi-line
// path into VerticalJustify without a full ConfigureDatState
// LayoutDesc binding, which this synthesized element does not
// have.
VerticalJustify = VJustify.Top,
HonorVerticalJustification = true,
ClickThrough = true,
ZOrder = int.MaxValue,
DefaultColor = SpewBoxColor,
@ -166,10 +194,29 @@ internal sealed class SpewBoxController : IDisposable
/// </param>
private void Tick(double nowSeconds)
{
// SpewBoxVM.Lines returns newest-first, matching retail's
// InsertItem(item, 0) — with OneLine now false and the AUTHORED
// MaxConcurrentItems == 4 (see SpewBoxState.MaxConcurrentItems),
// up to 4 lines render, newest on top.
// CH2 re-review nit 1 (docs/plans/2026-08-09-chat-parity-campaign.md):
// Anchors is AnchorEdges.None (see the AP-178 extent comment on
// SpewBoxWidth), so nothing else recenters this element on a
// window resize — recompute every frame against the CURRENT root
// width rather than the width captured once at construction.
_text.Left = (_root.Width - SpewBoxWidth) / 2f;
// CH2 re-review nit 3 (docs/plans/2026-08-09-chat-parity-campaign.md):
// deliberate inversion of UiText.LinesProvider's documented
// contract ("Provider of the lines to show, oldest-first" —
// UiText.cs). SpewBoxVM.Lines returns newest-first, matching
// retail's InsertItem(item, 0), and this controller feeds that
// order straight through WITHOUT reversing it. That is correct
// here specifically because the box is top-aligned
// (VerticalJustify.Top + HonorVerticalJustification above): index 0
// of the lines array draws at the TOP of the box, so feeding
// newest-first into a slot documented as oldest-first is exactly
// what makes "newest line on top" true. Reversing the feed to
// satisfy the doc comment literally would flip the visible order to
// oldest-on-top, which is wrong for this element — see
// SpewBoxControllerTests' rendered-order test. With OneLine now
// false and the AUTHORED MaxConcurrentItems == 4 (see
// SpewBoxState.MaxConcurrentItems), up to 4 lines render.
IReadOnlyList<SpewBoxLine> lines = _vm.Lines(nowSeconds);
_text.Visible = lines.Count > 0;
if (lines.Count == 0)

View file

@ -171,6 +171,20 @@ public sealed class UiText : UiElement, IUiDatStateful
/// </summary>
public VJustify VerticalJustify { get; set; } = VJustify.Center;
/// <summary>
/// Opts the SCROLLABLE multi-line path (i.e. <see cref="OneLine"/> ==
/// <see langword="false"/>) into <see cref="VerticalJustify"/> without
/// requiring a full <see cref="ConfigureDatState"/> LayoutDesc binding.
/// <see cref="ConfigureDatState"/> sets the equivalent internal flag
/// (<c>_honorDatVerticalJustification</c>) for DAT-imported text such as
/// spellbook tabs; synthesized (non-DAT) controllers that still want
/// top/bottom/center content flow instead of the historical bottom-
/// pinned transcript behavior set this directly. CH2 re-review nit 2
/// (<c>docs/plans/2026-08-09-chat-parity-campaign.md</c>): added for
/// <c>SpewBoxController</c>'s top-aligned, newest-line-on-top flow.
/// </summary>
public bool HonorVerticalJustification { get; set; }
/// <summary>The scroll model — also read by the linked UiScrollbar.</summary>
public UiScrollable Scroll { get; } = new();
@ -504,7 +518,7 @@ public sealed class UiText : UiElement, IUiDatStateful
Scroll.MaxScroll,
Scroll.ScrollY,
VerticalJustify,
_honorDatVerticalJustification);
_honorDatVerticalJustification || HonorVerticalJustification);
_lastBaseY = baseY;
// Normalised selection span (start <= end), if any.

View file

@ -54,18 +54,26 @@ public sealed class SpewBoxState
/// <summary>
/// The shipped LayoutDesc's AUTHORED value — no longer a placeholder.
/// CH2 REJECT-review rework, NIT 3
/// (<c>docs/research/2026-08-09-ch2-review-findings.md</c>): the
/// original C.7 sweep only searched <c>dats.Portal</c>, which has no
/// <c>0x10000016</c> (gmSpewBoxUI) element anywhere; extending the same
/// sweep to <c>dats.Local</c> (<c>client_local_English.dat</c>) found
/// it — LayoutDesc <c>0x21000011</c>, element <c>0x10000048</c>, whose
/// sole child (ListBox <c>0x10000049</c>, matching
/// <c>gmSpewBoxUI::PostInit</c>'s <c>GetChildRecursive(0x10000049)</c>
/// call verbatim) carries ListBox property <c>0x10000028</c> = the
/// integer <c>4</c>. Retail's own code default
/// (<c>gmSpewBoxUI::PostInit @0x004D5AB0</c>), used only when this
/// property is absent or unreadable, was <c>1</c> — the shipped layout
/// overrides it with <c>4</c>.
/// (<c>docs/research/2026-08-09-ch2-review-findings.md</c>), wording
/// corrected at the CH2 re-review nits pass
/// (<c>docs/plans/2026-08-09-chat-parity-campaign.md</c>, nit 6): the
/// original C.7 sweep's <c>dats.Portal</c> pass was not a meaningful
/// search — its id source was <c>DatCollection</c>'s top-level
/// AGGREGATE <c>GetAllIdsOfType&lt;LayoutDesc&gt;()</c>, not
/// <c>dats.Portal</c>'s own (which reports a count of ZERO for this
/// type), so querying those ids against <c>dats.Portal.TryGet</c>
/// established nothing about Portal either way. Extending the sweep
/// to <c>dats.Local</c> (<c>client_local_English.dat</c>) — this time
/// with a correctly-paired id source — found it: LayoutDesc
/// <c>0x21000011</c>, element <c>0x10000048</c>, whose sole child
/// (ListBox <c>0x10000049</c>, matching <c>gmSpewBoxUI::PostInit</c>'s
/// <c>GetChildRecursive(0x10000049)</c> call verbatim) carries ListBox
/// property <c>0x10000028</c> = the integer <c>4</c>. Retail's own
/// code default (<c>gmSpewBoxUI::PostInit @0x004D5AB0</c>), used only
/// when this property is absent or unreadable, was <c>1</c> — the
/// shipped layout overrides it with <c>4</c>. See
/// <c>SpewBoxLayoutDumpDiagnostic</c>'s own corrected RESULT comment
/// for the full id-source analysis.
/// </summary>
public const int MaxConcurrentItems = 4;

View file

@ -4,6 +4,7 @@ using AcDream.App.UI;
using AcDream.Core.Chat;
using AcDream.Core.Items;
using AcDream.Core.Social;
using AcDream.Runtime.Gameplay;
using AcDream.UI.Abstractions;
namespace AcDream.App.Tests.Net;
@ -250,6 +251,54 @@ public sealed class LiveSessionCommandRouterTests
Assert.Equal([0x50C4A54Au], options);
}
[Fact]
public void ShowWeenieErrorFriendsFull_ResolvesThroughAddText_AndLandsInSpewBoxNotChat()
{
// CH2 re-review nit 7 (docs/plans/2026-08-09-chat-parity-campaign.md):
// exercises the FULL retail path for id 0x0561 (the 50-friends-cap
// refusal): ClientCommandController.AddFriend's cap guard calls
// ShowWeenieError(0x0561u); the binding below mirrors
// LiveSessionRuntimeFactory.CreateCommandBindings' ShowWeenieError
// closure exactly (WeenieErrorMessages.Resolve then
// RuntimeCommunicationState.AddText) — the same chokepoint that
// fans a ClientLocal line out to SpewBox instead of chat
// (RuntimeCommunicationStateTests pins that half in isolation
// already). This is "the closest seam" per the review: every other
// LiveSessionRuntimeFactory test in this tree is a source-text
// conformance grep (ReadSource), not an instantiation — the real
// factory needs a live WorldSession/domain graph no test builds.
var communication = new RuntimeCommunicationState();
ClientCommandController.Bindings bindings = NewClientBindings() with
{
Friends = communication.Friends,
ShowWeenieError = code =>
{
(string? text, RetailLogTextType type) = WeenieErrorMessages.Resolve(code, null);
if (text is not null)
communication.AddText(text, type);
},
};
LiveSessionCommandRouter router = NewRouter(clientBindings: bindings);
router.Activate();
// Seed 50 friends so AddFriend's cap guard fires
// (ClientCommandController.cs's AddFriend).
var seeded = new List<FriendEntry>();
for (uint i = 0; i < 50; i++)
seeded.Add(new FriendEntry(i + 1, $"Friend{i}", true, false, [], []));
communication.Friends.Apply(new FriendsUpdate(FriendsUpdateType.Full, seeded));
router.Publish(new ExecuteClientCommandCmd(ClientCommandId.FriendsAdd, "OneTooMany"));
communication.SpewBox.Tick(0d);
Assert.Equal(1, communication.SpewBox.Count);
Assert.Equal(
"You may only have a maximum of 50 friends at once. If you wish to add more friends, you must first remove some.",
communication.SpewBox.Snapshot()[0].Text);
Assert.Equal(0, communication.Chat.Count);
Assert.Equal(50, communication.Friends.Count);
}
private static LiveSessionCommandRouter NewRouter(
ChatLog? chat = null,
TurbineChatState? turbine = null,

View file

@ -111,9 +111,14 @@ public sealed class SpewBoxControllerTests
public void Controller_QueueStaysBounded_AcrossManyTicksWithoutADrawPass()
{
// BLOCKER 1 acceptance (c): repeated enqueue+tick cycles must never
// let the visible set grow past SpewBoxState.MaxConcurrentItems
// (retail's code default, 1) — this is what "unbounded per-session
// leak" would have looked like if the drain never ran at all.
// let the visible set grow past SpewBoxState.MaxConcurrentItems.
// CH2 re-review nit 4 (docs/plans/2026-08-09-chat-parity-campaign.md):
// corrected — MaxConcurrentItems is the shipped LayoutDesc's
// AUTHORED value, 4, not retail's code-default of 1 (the fallback
// gmSpewBoxUI::PostInit uses only when the property is absent; see
// SpewBoxState.MaxConcurrentItems). This is what "unbounded
// per-session leak" would have looked like if the drain never ran
// at all.
var root = new UiRoot { Width = 1280f, Height = 720f };
var state = new SpewBoxState();
using var controller = new SpewBoxController(root, new SpewBoxVM(state));
@ -131,6 +136,33 @@ public sealed class SpewBoxControllerTests
Assert.Equal("line 49", text.LinesProvider!()[0].Text);
}
[Fact]
public void Controller_RenderedOrder_NewerMessageIsTheTopmostLine()
{
// CH2 re-review nit 3 (docs/plans/2026-08-09-chat-parity-campaign.md):
// pins the RENDERED order, not UiText.LinesProvider's documented
// contract (oldest-first) — SpewBoxController deliberately inverts
// that (see Tick's ordering comment) because the box is now
// top-aligned and retail shows the newest interface-text line on
// top. Drives root.Tick, not the provider, matching every other
// test in this file.
var root = new UiRoot { Width = 1280f, Height = 720f };
var state = new SpewBoxState();
using var controller = new SpewBoxController(root, new SpewBoxVM(state));
state.Enqueue("older message");
root.Tick(dt: 0d, nowMs: 1000L);
state.Enqueue("newer message");
root.Tick(dt: 0d, nowMs: 1001L);
UiText text = Assert.IsType<UiText>(root.Children.OfType<UiText>().Single());
IReadOnlyList<UiText.Line> rendered = text.LinesProvider!();
Assert.Equal(2, rendered.Count);
Assert.Equal("newer message", rendered[0].Text);
Assert.Equal("older message", rendered[1].Text);
}
[Fact]
public void Dispose_RemovesBothTheTextAndTheGlobalTimeSinkFromTheRoot()
{

View file

@ -21,14 +21,28 @@ namespace AcDream.App.Tests.UI;
/// tree.
///
/// <para>
/// <b>RESULT (2026-08-09, run against the installed <c>client_portal.dat</c>):</b>
/// the installed DAT's entire LayoutDesc id space is the CLOSED range
/// <c>0x21000000</c>-<c>0x21000075</c> (118 possible ids, 101 populated —
/// sanity-checked by confirming 3 independently-known ids, 0x2100002E
/// character window / 0x21000023 inventory / 0x21000016 toolbar prototype,
/// are all present). The sweep is therefore EXHAUSTIVE, and it finds
/// <b>ZERO</b> elements of class <c>0x10000016</c> anywhere in any of the
/// 101 layouts, and LayoutDesc <c>0x10000012</c> — the id the decompiled
/// <b>RESULT (2026-08-09, run against the installed <c>client_portal.dat</c>),
/// WORDING CORRECTED at the CH2 re-review nits pass
/// (<c>docs/plans/2026-08-09-chat-parity-campaign.md</c>, nit 6):</b> this
/// pass's id source, <c>allIds</c> below, is
/// <c>dats.GetAllIdsOfType&lt;LayoutDesc&gt;()</c> — the top-level AGGREGATE
/// accessor — NOT <c>dats.Portal.GetAllIdsOfType&lt;LayoutDesc&gt;()</c>,
/// which the very next line prints and which reports a count of
/// <b>ZERO</b>. The loop below then queries those aggregate ids against
/// <c>dats.Portal.TryGet</c>, a database whose own id enumeration for this
/// type is empty — so the original "the sweep is therefore EXHAUSTIVE...
/// finds ZERO elements... anywhere" conclusion never actually established
/// anything about <c>dats.Portal</c>: it iterated ids sourced from
/// elsewhere against a database that cannot be enumerated the same way.
/// The id-space/sanity-check numbers below (CLOSED range
/// <c>0x21000000</c>-<c>0x21000075</c>, 118 possible ids, 101 populated,
/// 3 known ids present) describe THAT aggregate list, not Portal's own.
/// What IS established — from the separate <c>dats.Local</c> sweep, see
/// RESULT 2 below — is that <c>client_local_English.dat</c> hosts the
/// SpewBox layout at LayoutDesc <c>0x21000011</c>. Whether
/// <c>dats.Portal</c> ALSO carries a <c>0x10000016</c> element remains
/// UNESTABLISHED; this sweep did not meaningfully search it. LayoutDesc
/// <c>0x10000012</c> — the id the decompiled
/// <c>CreateChildElementByEnum(null, 0x10000012, 0x1000004A)</c> call
/// appears to reference — exists but has zero top-level elements (it is
/// not the per-line template catalog; that id must resolve through a