erik/acdream
1
0
Fork 0
Code Issues Pull requests Projects Releases 6 Packages Wiki Activity Actions

fix(CT): CT3 fix round — port Refresh's unconditional selection clear, drop unresolvable-id rows

Browse source 
Opus dual-lens review of CT3 (03e073b7) found 1 BLOCKER + 2 SHOULD-FIX + notes.

BLOCKER: CharacterTitlesController never ported Refresh @0x0049abc0's own
SetSelectedItem(nullptr, 1) (@0x0049ac5a) — retail clears the current
title selection UNCONDITIONALLY on every Refresh() call, regardless of
whether the previously-selected id is still earned. OnTableReplaced
(0x0029) and OnDisplayTitleChanged (the display half of 0x002B) are
retail's two Refresh() call sites, so both now clear _selectedTitleId
before rebuilding/re-highlighting. OnTitleAdded (0x002B's add half) is a
DIFFERENT retail method — RecvNotice_AddCharacterTitle @0x0049a990 splices
one row without ever touching m_pSelectedItem — so it deliberately still
preserves selection. Net effect: after the user sets a display title and
ACE echoes 0x002B, the previously-highlighted row now goes dark and the
Set-as-Display button re-ghosts, matching retail; earning a new title
while a row is selected still leaves that selection alone.

SHOULD-FIX: ported AddTitleToList @0x0049A840's early-outs
(@0x0049a873/@0x0049a914) — an id of 0, or an id CharacterTitleResolver
fails to resolve, now produces NO row at all. The "Unknown" fallback
literal belongs only to the display-title text (Refresh @0x0049abc0's
other half), never a row — this was previously ported backwards.

SHOULD-FIX: rows and the display text now use their UiText's own authored
DefaultColor instead of a hardcoded Vector4.One, and each LinesProvider
now returns a cached UiText.Line[] built once per text change instead of
allocating a fresh array literal every draw call (pattern:
CharacterCreationSkillsPage.cs:829).

Notes (all ruled in): corrected two CharacterStatController 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 given Visible's draw/click cascade,
kept only for the contentPage-not-found fallback); switched the row sort
from List.Sort to a stable OrderBy/ThenBy (ties broken by title id) so
equal-text rows keep retail's insert-after-equals order; wrapped the
title-resolver delegate in RetailUiRuntime.MountCharacter with the same
DatLock the row-template resolver already takes (DatCollection is
documented not thread-safe); set the list box's authored 24px row height
so wheel/line scroll lands row-aligned; kept the bind-time display-text
refresh with a comment explaining why the pre-notice "Unknown" frame is
unreachable in live play (ACE always sends 0x0029 before this panel can
open).

Tests: inverted TableReplaced_SelectedTitleStillEarned_KeepsSelectionHighlighted
into TableReplaced_ClearsSelection_EvenWhenTheSelectedIdIsStillEarned (cites
@0x0049ac5a), added its DisplayTitleChanged twin, and added
TitleAdded_PreservesSelection (the case most at risk from the blocker fix).
Inverted Rows_UnresolvedTitle_ShowsRetailUnknownLiteral into
Rows_UnresolvedTitle_ProducesNoRow (cites @0x0049a873/@0x0049a914) and added
Rows_TitleIdZero_ProducesNoRow for the other early-out. Extended
ClickingSetDisplay_..._AndMutatesNothingLocally to assert the row set and
selection are untouched by the click. Added
Fixture_PageCaptions_ResolveToNonEmptyText, which rebuilds the committed
character_2100002E.json fixture with a stub string resolver to pin this
class's own claim that the two page captions (0x1000052E/0x10000531) carry
a resolvable authored StringInfo.

Verified pre-existing/unrelated: the full hermetic suite run surfaced 2
failures in AcDream.App.Tests (LiveEntityNetworkBranchRoutingTests IL-shape
assertion, GameWindowRenderLeafCompositionTests IL-shape assertion) that
also fail with these five files stashed back to their pre-fix-round state —
confirmed unrelated to this change.

Build green. CharacterTitlesControllerTests: 24/24 (was 21, +3 net after
one invert-and-split and two new facts). Full hermetic solution suite
(Lane!=InstalledDat/PreparedPackage/Live/Manual/Timing/Windows/Linux/
SystemFont, Purpose!=Diagnostic, Status!=KnownFailure): only the two
pre-existing IL-shape failures above; every other project green.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik 2026-08-24 23:28:28 +02:00
parent 4ea946257d
commit 4cc9448b0a
5 changed files with 287 additions and 33 deletions
Download patch file Download diff file Expand all files Collapse all files

128
tests/AcDream.App.Tests/UI/Layout/CharacterTitlesControllerTests.cs
View file

@ -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 ? "<resolved>" : null;
ImportedLayout layout = LayoutImporter.Build(
FixtureLoader.LoadCharacterInfos(),
static _ => (0u, 0, 0),
null,
stringResolve: StubResolve);
var currentTitleCaption = Assert.IsType<UiText>(layout.FindElement(0x1000052Eu));
var titlesEarnedCaption = Assert.IsType<UiText>(layout.FindElement(0x10000531u));
Assert.Equal("<resolved>", currentTitleCaption.LinesProvider()[0].Text);
Assert.Equal("<resolved>", 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<UiElement> 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);
}