diff --git a/docs/plans/2026-08-24-character-panel-parity-campaign.md b/docs/plans/2026-08-24-character-panel-parity-campaign.md index 339c92a3..06f0fe90 100644 --- a/docs/plans/2026-08-24-character-panel-parity-campaign.md +++ b/docs/plans/2026-08-24-character-panel-parity-campaign.md @@ -233,12 +233,40 @@ not yet acted on): to a wire arrival; the reverse still holds (every genuine new row has a `TitleAdded` firing). -**CT3 — Titles page UI.** Bind the authored page through the standard +**CT3 — Titles page UI. REVIEW-CLOSED 2026-08-24: landed `03e073b7`, +Opus dual-lens review (1 BLOCKER + 2 should-fix + notes, all applied +in the fix round below).** Bind the authored page through the standard GUI classes (`UiTemplateListBox`/`UiScrollbar`/`UiButton` — zero bespoke widgets): sorted rows via the ported title-table lookup, selection, ghost-when-current logic (state 0xd contract), display-title text, Set-as-Display round trip, scrollbar. Retires half of AP-109. +**CT3 fix round (Opus dual-lens review, 2026-08-24).** BLOCKER: ported +`Refresh @0x0049abc0`'s unconditional `SetSelectedItem(nullptr, 1)` +(`@0x0049ac5a`) — selection now clears on BOTH `TableReplaced` and +`DisplayTitleChanged`, regardless of whether the previously-selected id +is still earned in the new table, but deliberately survives +`TitleAdded` (`RecvNotice_AddCharacterTitle @0x0049a990` splices one +row without ever touching `m_pSelectedItem` — a genuinely different +retail method from `Refresh`). SHOULD-FIX: `AddTitleToList @0x0049A840`'s +early-outs (`@0x0049a873`/`@0x0049a914`) ported — an id of 0, or an id +`CharacterTitleResolver.Resolve` fails to resolve, now produces NO row +at all (the `"Unknown"` fallback literal belongs only to the +display-title text, never a row — this was previously ported +backwards); rows use the row template's own authored `DefaultColor` +instead of a hardcoded white, and each row/display-text `UiText.Line[]` +is built once per text change and cached instead of reallocated every +draw call. Notes also applied: corrected two comments that falsely +claimed the Titles page authors its own copies of the raise buttons +(verified against the fixture — it does not; the hide loop that +comment guarded is a defensive no-op, kept only for the +contentPage-not-found fallback path), switched the row sort from +`List.Sort` to a stable `OrderBy`/`ThenBy` (ties broken by title id), +wrapped the title-resolver delegate in the same `DatLock` the +row-template resolver already takes (`RetailUiRuntime.MountCharacter`), +and set the list box's authored 24px row height so wheel/line scroll +lands row-aligned. + **CT4 — Header identity block.** Retail composition: name; " "; PK status line — authored fonts/colors (pure white per probe), live refresh on display-title change and PK diff --git a/src/AcDream.App/UI/Layout/CharacterStatController.cs b/src/AcDream.App/UI/Layout/CharacterStatController.cs index 5ad9865b..4f50311d 100644 --- a/src/AcDream.App/UI/Layout/CharacterStatController.cs +++ b/src/AcDream.App/UI/Layout/CharacterStatController.cs @@ -374,8 +374,12 @@ public static class CharacterStatController // Mutable selected-index box: -1 = nothing selected. // Gather EVERY copy of the raise buttons in the tree. The raise button ids - // (0x10000246, 0x100005EB) appear once per tab page (Attributes/Skills/Titles) - // in the dat inheritance structure; ImportedLayout._byId keeps only the LAST + // (0x10000246, 0x100005EB) each appear TWICE under BOTH the Attributes page + // (0x1000022B) and the Skills page (0x1000022C) — once per footer-state group + // (0x10000247/0x10000241) — four copies total. Verified against the committed + // fixture (CT3 fix round): Titles (0x10000539) authors NO copies of its own — + // correcting this comment's earlier, false "once per tab page + // (Attributes/Skills/Titles)" claim. ImportedLayout._byId keeps only the LAST // mounted copy. We collect all copies so we can hide them all initially and // show/hide the correct set when a row is selected. // @@ -526,10 +530,19 @@ public static class CharacterStatController if (showTitles) { - // Titles authors its own (unused) copies of the raise buttons - // (CT1 ground truth); nothing on this page ever selects a stat - // row, so keep them hidden rather than rebuilding a list this - // tab does not show. + // CT3 fix round: the prior comment here ("Titles authors its + // own copies of the raise buttons") was FALSE — verified + // against the fixture, Titles (0x10000539) has none; see the + // corrected collection comment above. contentPage.Visible = + // false (just above) already suppresses the Attributes + // page's real raise-button copies for both draw and click + // routing (UiElement early-returns on an invisible node + // before descending to children), so this loop is a no-op + // in the common case. It is kept only as a defensive + // fallback for the case where contentPage was not found at + // bind time (contentPage is null, line ~525) but allRaise1/ + // allRaise10 were still populated via the tree-walk/FindElement + // fallback above. foreach (var b in allRaise1) b.Visible = false; foreach (var b in allRaise10) b.Visible = false; } diff --git a/src/AcDream.App/UI/Layout/CharacterTitlesController.cs b/src/AcDream.App/UI/Layout/CharacterTitlesController.cs index 2e4a44a5..77f86951 100644 --- a/src/AcDream.App/UI/Layout/CharacterTitlesController.cs +++ b/src/AcDream.App/UI/Layout/CharacterTitlesController.cs @@ -1,6 +1,6 @@ using System; using System.Collections.Generic; -using System.Numerics; +using System.Linq; using AcDream.Runtime; namespace AcDream.App.UI.Layout; @@ -29,7 +29,13 @@ namespace AcDream.App.UI.Layout; /// codebase needs one either (Friends/Squelch/Fellowship/Allegiance/chargen /// skills/the Options tabs all rebuild-on-change the same way) — the /// resulting VISIBLE order is retail-exact even though the underlying -/// mechanism is "rebuild," not "splice." +/// mechanism is "rebuild," not "splice." AddTitleToList itself +/// early-outs (@0x0049a873/@0x0049a914) before ever reaching +/// the insert — retail NEVER creates a row for id 0 or for an id +/// GetCharacterTitleFromID fails to resolve, so +/// skips those ids entirely rather than falling back to a placeholder row +/// text (CT3 fix round — the "Unknown" literal belongs ONLY to the +/// display-title text below, never to a row). /// /// /// Selection + highlight. Retail's InfoRegion::SetState @@ -43,6 +49,27 @@ namespace AcDream.App.UI.Layout; /// swap, unlike pages whose row template lacks a state-based highlight. /// /// +/// Selection lifetime (CT3 fix round — BLOCKER). Refresh +/// @0x0049abc0 itself calls SetSelectedItem(nullptr, 1) +/// (@0x0049ac5a) UNCONDITIONALLY, before it repopulates the list — +/// every code path that reaches Refresh() drops the current +/// selection outright, regardless of whether the previously-selected title +/// id is still earned. Refresh() runs on BOTH +/// (0x0029) +/// and (the +/// display-title half of 0x002B), so +/// and both clear +/// before rebuilding/re-highlighting. +/// RecvNotice_AddCharacterTitle @0x0049a990 (the add half of +/// 0x002B, ) is a DIFFERENT retail method +/// that splices one row into mTitleList without ever touching +/// m_pSelectedItem — selection SURVIVES a title add. Concretely: +/// after the user sets a display title and ACE echoes 0x002B, the +/// previously-highlighted row goes dark and the Set-as-Display button +/// re-ghosts, exactly like retail — but earning a brand-new title while a +/// row is selected leaves that selection alone. +/// +/// /// The "Set as Display Title" button (0x10000535). /// UpdateButtons @0x0049A500 (CORRECTED per the campaign plan's CT1 /// fix round): Ghosted (state 0xD) UNLESS a row is SELECTED whose title id @@ -149,6 +176,12 @@ public sealed class CharacterTitlesController : IDisposable return null; } listBox.TemplateResolver = templateResolver; + // The row template (0x10000536) authors a 270x24 box (CT1 ground + // truth §3); UiTemplateListBox's own DefaultLineHeight is 16, which + // would desync wheel/line scroll from the actual row pitch + // (CharacterManagementUiController.cs:463 sets its own row height + // the same way for the same reason). + listBox.LineHeight = 24; uint scrollbarElementId = listBox.ScrollbarElementId; UiElement? scrollbarElement = scrollbarElementId == 0 @@ -175,6 +208,14 @@ public sealed class CharacterTitlesController : IDisposable titles.DisplayTitleChanged += controller.OnDisplayTitleChanged; controller.RebuildRows(); + // Bind-time refresh (CT3 fix round NOTE 6): retail itself only + // shows "Unknown" until the first notice arrives (nothing runs + // Refresh() before Refresh() is first called), but ACE always sends + // 0x0029 at SendSelf before this panel can even open, so the + // pre-notice "Unknown" frame is unreachable in live play. Refreshing + // at bind time instead keeps a window RE-mount (tab re-open, panel + // rebuild) consistent with whatever the table already holds, rather + // than flashing "Unknown" for one frame before the next notice. controller.RefreshDisplayText(); controller.RefreshButtonGhost(); return controller; @@ -201,12 +242,15 @@ public sealed class CharacterTitlesController : IDisposable /// /// 0x0029 CharacterTitle — retail's own Refresh() is /// unconditional here (CT2 review anchor 1), and UnPack always - /// rebuilds mTitleList from scratch. - /// itself decides whether the current selection survives (it does when - /// the selected id is still earned in the new table). + /// rebuilds mTitleList from scratch. BLOCKER fix (CT3 fix round): + /// Refresh also calls SetSelectedItem(nullptr, 1) + /// (@0x0049ac5a) unconditionally, BEFORE it repopulates — so the + /// selection is cleared here regardless of whether the previously + /// selected id is still earned, not merely dropped when it disappears. /// private void OnTableReplaced() { + ClearSelection(); RebuildRows(); RefreshDisplayText(); RefreshButtonGhost(); @@ -216,6 +260,10 @@ public sealed class CharacterTitlesController : IDisposable /// 0x002B UpdateTitle, add half — CT2's F1 fix already dedupes /// this event to genuine new memberships only (a repeat add fires no /// event at all), so every firing here is a real new row. + /// RecvNotice_AddCharacterTitle @0x0049a990 splices the one new + /// row into mTitleList without ever touching + /// m_pSelectedItem (unlike Refresh's unconditional + /// clear) — selection deliberately survives a title add. /// private void OnTitleAdded(uint titleId) { @@ -223,36 +271,68 @@ public sealed class CharacterTitlesController : IDisposable RefreshButtonGhost(); } + /// + /// 0x002B UpdateTitle, display half — this is the other trigger + /// for retail's Refresh() (CT2 review anchor 1), so it carries + /// the same unconditional SetSelectedItem(nullptr, 1) + /// (@0x0049ac5a) as . This handler + /// does not call (the row SET is unchanged — + /// only the display title moved), so it re-applies highlights directly + /// to actually dark out the previously-selected row. + /// private void OnDisplayTitleChanged(uint titleId) { + ClearSelection(); + ApplyRowHighlights(); RefreshDisplayText(); RefreshButtonGhost(); } + /// Refresh @0x0049abc0's SetSelectedItem(nullptr, + /// 1) (@0x0049ac5a) — clears the tracked selection only; the + /// caller is responsible for re-applying row highlights and the button + /// ghost state afterward. + private void ClearSelection() => _selectedTitleId = null; + /// /// Full sorted rebuild — see the class remarks for why this port /// rebuilds rather than performing retail's literal single-row /// positional insert. Preserves scroll position - /// (). The current - /// selection survives when the selected id is still present in the - /// rebuilt row set; otherwise it is cleared here so the Set-as-Display - /// button's ghost state can never desync from what is actually - /// highlighted (a selection pointing at a no-longer-visible row would - /// leave the button enabled with nothing shown selected). + /// (). Callers that + /// mirror retail's unconditional Refresh() selection clear + /// () call + /// themselves before this runs; the check below is a defensive + /// fallback for any other caller ( included) + /// so a selection can never point at a row that no longer exists. /// private void RebuildRows() { _listBox.FlushPreservingScroll(); _rows.Clear(); + // AddTitleToList @0x0049A840 early-outs (@0x0049a873/@0x0049a914): + // retail never creates a row for id 0 or for an id + // GetCharacterTitleFromID fails to resolve — "Unknown" is the + // display-title text's OWN fallback (RefreshDisplayText), never a + // row's (CT3 fix round — was previously ported backwards). // A3/CT2 doc warning: EarnedTitleIds allocates a fresh array per // read — safe here (a UI refresh call site, not a per-frame poll). - var sorted = new List<(uint Id, string Text)>(); + var candidates = new List<(uint Id, string Text)>(); foreach (uint id in _titles.EarnedTitleIds) - sorted.Add((id, _resolveTitle(id) ?? UnknownTitleText)); + { + if (id == 0) continue; + string? text = _resolveTitle(id); + if (text is null) continue; + candidates.Add((id, text)); + } // FindSortedInsertPosition @0x0049A760: ordinal string sort on the - // resolved display text. - sorted.Sort(static (a, b) => string.CompareOrdinal(a.Text, b.Text)); + // resolved display text. OrderBy is a STABLE sort (unlike + // List.Sort) so equal-text rows keep retail's insert-after- + // equals order; ties are broken by title id for full determinism. + List<(uint Id, string Text)> sorted = candidates + .OrderBy(static c => c.Text, StringComparer.Ordinal) + .ThenBy(static c => c.Id) + .ToList(); foreach ((uint id, string text) in sorted) { @@ -272,8 +352,14 @@ public sealed class CharacterTitlesController : IDisposable if (UiElement.FindDescendant(row, RowTextId) is UiText rowText) { - string capturedText = text; - rowText.LinesProvider = () => [new UiText.Line(capturedText, Vector4.One)]; + // Build the line array once per text change and capture it + // — LinesProvider runs every draw, so a `=> [new Line(...)]` + // literal would allocate a fresh array every frame + // (pattern: CharacterCreationSkillsPage.cs:829). DefaultColor + // is the row template's own authored font color, not a + // hardcoded white. + UiText.Line[] lines = [new UiText.Line(text, rowText.DefaultColor)]; + rowText.LinesProvider = () => lines; } _rows.Add(new Row(row, id)); @@ -314,7 +400,10 @@ public sealed class CharacterTitlesController : IDisposable { if (_displayText is null) return; string text = _resolveTitle(_titles.DisplayTitleId) ?? UnknownTitleText; - _displayText.LinesProvider = () => [new UiText.Line(text, Vector4.One)]; + // Cached array, authored color — same reasoning as the row text + // above (CT3 fix round). + UiText.Line[] lines = [new UiText.Line(text, _displayText.DefaultColor)]; + _displayText.LinesProvider = () => lines; } /// diff --git a/src/AcDream.App/UI/RetailUiRuntime.cs b/src/AcDream.App/UI/RetailUiRuntime.cs index 88ee83ac..0bfa426a 100644 --- a/src/AcDream.App/UI/RetailUiRuntime.cs +++ b/src/AcDream.App/UI/RetailUiRuntime.cs @@ -4080,10 +4080,20 @@ public sealed class RetailUiRuntime : IDisposable lock (_bindings.Assets.DatLock) return titleRowTemplates.Resolve(templateLayoutId, templateElementId); } + // CT3 fix round: CharacterTitleResolver.Resolve reads the SAME + // IDatReaderWriter (EnumMapper + StringTable lookups) as the row + // template resolver just above — DatCollection is documented not + // thread-safe, so this delegate needs the identical DatLock scope, + // not just the template resolver. + string? TitleResolver(uint titleId) + { + lock (_bindings.Assets.DatLock) + return _bindings.Character.TitleResolver.Resolve(titleId); + } _characterTitlesController = Layout.CharacterTitlesController.Bind( layout.Root, _bindings.Character.Titles, - _bindings.Character.TitleResolver.Resolve, + TitleResolver, TitleTemplateResolver, _bindings.Character.SendSetTitle); diff --git a/tests/AcDream.App.Tests/UI/Layout/CharacterTitlesControllerTests.cs b/tests/AcDream.App.Tests/UI/Layout/CharacterTitlesControllerTests.cs index fa3dd684..122d24b2 100644 --- a/tests/AcDream.App.Tests/UI/Layout/CharacterTitlesControllerTests.cs +++ b/tests/AcDream.App.Tests/UI/Layout/CharacterTitlesControllerTests.cs @@ -95,6 +95,37 @@ public sealed class CharacterTitlesControllerTests Assert.Same(h.ListBox.Scroll, scrollbar.Model); } + [Fact] + public void Fixture_PageCaptions_ResolveToNonEmptyText() + { + // Pins this class's own remarks claim (CT3 fix round item 5b): + // LayoutImporter.BuildText already resolves every element's + // authored StringInfo caption at import time, so the Titles page's + // two static captions (0x1000052E/0x10000531) must actually carry a + // resolvable authored StringInfo -- not silently come through as + // empty/missing text -- even though this controller never touches + // either element itself. FixtureLoader.LoadCharacter() passes NO + // string resolver (it needs no live DAT for structural conformance + // checks elsewhere), so this test rebuilds the SAME committed + // fixture with a stub resolver that stands in for + // DatStringResolver.Resolve -- exercising the real + // ResolveAuthoredString → stringResolve pipeline + // (DatWidgetFactory.cs) without needing a live StringTable. + static string? StubResolve(UiStringInfoValue info) => + info.TableId != 0u && info.StringId != 0u ? "" : null; + ImportedLayout layout = LayoutImporter.Build( + FixtureLoader.LoadCharacterInfos(), + static _ => (0u, 0, 0), + null, + stringResolve: StubResolve); + + var currentTitleCaption = Assert.IsType(layout.FindElement(0x1000052Eu)); + var titlesEarnedCaption = Assert.IsType(layout.FindElement(0x10000531u)); + + Assert.Equal("", currentTitleCaption.LinesProvider()[0].Text); + Assert.Equal("", titlesEarnedCaption.LinesProvider()[0].Text); + } + [Fact] public void Bind_MissingListBox_ReturnsNullWithoutThrowing() { @@ -206,15 +237,34 @@ public sealed class CharacterTitlesControllerTests } [Fact] - public void Rows_UnresolvedTitle_ShowsRetailUnknownLiteral() + public void Rows_UnresolvedTitle_ProducesNoRow() { + // AddTitleToList @0x0049A840 early-outs (@0x0049a873/@0x0049a914): + // retail never creates a row for an id GetCharacterTitleFromID + // fails to resolve -- "Unknown" is exclusively the display-title + // text's own Refresh fallback literal (below), never a row's + // (CT3 fix round -- this was previously ported backwards). Harness h = BindWithEarnedTitles( [99u], displayTitleId: 0u, names: []); + Assert.Empty(h.Rows); + } + + [Fact] + public void Rows_TitleIdZero_ProducesNoRow() + { + // Same early-out (@0x0049a873), the OTHER guarded case: retail + // never creates a row for id 0 even if a resolver were somehow + // willing to answer for it. + Harness h = BindWithEarnedTitles( + [0u, 1u], + displayTitleId: 0u, + names: new() { [0u] = "Should Never Appear", [1u] = "Adventurer" }); + UiElement row = Assert.Single(h.Rows); - Assert.Equal("Unknown", h.RowText(row)); + Assert.Equal("Adventurer", h.RowText(row)); } // ── Selection + highlight ───────────────────────────────────────────── @@ -288,6 +338,7 @@ public sealed class CharacterTitlesControllerTests names: new() { [1u] = "Adventurer", [13u] = "War Mage" }); UiElement warMageRow = h.Rows.Single(r => h.RowText(r) == "War Mage"); ((UiDatElement)warMageRow).OnClick!(); + List rowsBeforeClick = h.Rows.ToList(); h.SetDisplayButton.OnClick!(); @@ -297,6 +348,12 @@ public sealed class CharacterTitlesControllerTests // ever changes from a DisplayTitleChanged event). Assert.Equal(1u, h.Titles.DisplayTitleId); Assert.Equal("Adventurer", h.DisplayText.LinesProvider().Single().Text); + // The click is wire-only: no RebuildRows, no selection change. The + // row set is the SAME instances in the same order, and War Mage + // stays selected/highlighted/enabled exactly as before the click. + Assert.Equal(rowsBeforeClick, h.Rows); + Assert.Equal(RowHighlightSprite, h.RowMedia(warMageRow)); + Assert.True(h.SetDisplayButton.Enabled); } [Fact] @@ -331,8 +388,14 @@ public sealed class CharacterTitlesControllerTests } [Fact] - public void TableReplaced_SelectedTitleStillEarned_KeepsSelectionHighlighted() + public void TableReplaced_ClearsSelection_EvenWhenTheSelectedIdIsStillEarned() { + // BLOCKER fix (CT3 fix round): Refresh @0x0049abc0 calls + // SetSelectedItem(nullptr, 1) (@0x0049ac5a) UNCONDITIONALLY, before + // it repopulates the list -- a still-earned selected id is no + // defense. A byte-identical resend must still dark out the row and + // re-ghost the button (this test used to assert the OPPOSITE and + // was wrong). Harness h = BindWithEarnedTitles( [1u, 5u], displayTitleId: 0u, names: new() { [1u] = "Adventurer", [5u] = "Life Mage" }); @@ -340,13 +403,64 @@ public sealed class CharacterTitlesControllerTests ((UiDatElement)lifeMageRow).OnClick!(); Assert.True(h.SetDisplayButton.Enabled); - // A resend of the SAME table (retail's own Refresh() is - // unconditional — CT2 review anchor 1) must not silently desync the - // ghost state from the still-valid selection. + // Same table resent (retail's own Refresh() is unconditional — + // CT2 review anchor 1) — the id (5) is STILL earned afterward, yet + // the selection must still be dropped. h.Titles.ReplaceTable(0u, [1u, 5u]); UiElement rebuiltLifeMageRow = h.Rows.Single(r => h.RowText(r) == "Life Mage"); - Assert.Equal(RowHighlightSprite, h.RowMedia(rebuiltLifeMageRow)); + Assert.Equal(RowNormalSprite, h.RowMedia(rebuiltLifeMageRow)); + Assert.False(h.SetDisplayButton.Enabled); + } + + [Fact] + public void DisplayTitleChanged_ClearsSelection_EvenWhenTheSelectedIdIsStillEarned() + { + // The display-change twin of the TableReplaced test above: 0x002B's + // display half is retail's OTHER Refresh() call site, so it carries + // the same unconditional SetSelectedItem(nullptr, 1) (@0x0049ac5a). + // This handler never calls RebuildRows (the row SET does not + // change), so it specifically proves the highlight is re-applied + // via ApplyRowHighlights even without a rebuild. Selecting a + // DIFFERENT id than the one becoming the new display title isolates + // this from the already-covered "selection == new display title" + // ghost case (DisplayTitleChanged_UpdatesTextAndReevaluatesGhost): + // id 1 remains earned and still differs from the new display id 5, + // yet selection must still clear. + Harness h = BindWithEarnedTitles( + [1u, 5u], displayTitleId: 0u, + names: new() { [1u] = "Adventurer", [5u] = "Life Mage" }); + UiElement adventurerRow = h.Rows.Single(r => h.RowText(r) == "Adventurer"); + ((UiDatElement)adventurerRow).OnClick!(); + Assert.Equal(RowHighlightSprite, h.RowMedia(adventurerRow)); + Assert.True(h.SetDisplayButton.Enabled); // selected(1) != display(0) + + h.Titles.ApplyUpdateTitle(5u, setAsDisplay: true); + + Assert.Equal(RowNormalSprite, h.RowMedia(adventurerRow)); + Assert.False(h.SetDisplayButton.Enabled); + } + + [Fact] + public void TitleAdded_PreservesSelection() + { + // RecvNotice_AddCharacterTitle @0x0049a990 splices the new row into + // mTitleList without ever touching m_pSelectedItem -- a DIFFERENT + // retail method from Refresh, and the one case that must NOT clear + // selection. This is the case most at risk from the blocker fix + // above (it would be trivial to over-clear on every wire event). + Harness h = BindWithEarnedTitles( + [1u], displayTitleId: 0u, + names: new() { [1u] = "Adventurer", [13u] = "War Mage" }); + UiElement adventurerRow = Assert.Single(h.Rows); + ((UiDatElement)adventurerRow).OnClick!(); + Assert.Equal(RowHighlightSprite, h.RowMedia(adventurerRow)); + Assert.True(h.SetDisplayButton.Enabled); + + h.Titles.ApplyUpdateTitle(13u, setAsDisplay: false); + + UiElement rebuiltAdventurerRow = h.Rows.Single(r => h.RowText(r) == "Adventurer"); + Assert.Equal(RowHighlightSprite, h.RowMedia(rebuiltAdventurerRow)); Assert.True(h.SetDisplayButton.Enabled); }