From 466272ec5538dcc9c6d13a80b73321e27207fa41 Mon Sep 17 00:00:00 2001 From: Erik Date: Sun, 6 Sep 2026 15:43:56 +0200 Subject: [PATCH] =?UTF-8?q?fix(plugin-ui):=20Slice=20B=20review=20fixes=20?= =?UTF-8?q?=E2=80=94=20no=20magenta=20for=20bad=20DIDs,=20contract=20thres?= =?UTF-8?q?hold,=20integral=20icon=20bindings,=20nearest=20did=20art,=20lo?= =?UTF-8?q?ud=20markup=20errors?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two Opus reviews of commit 8217a349e (Slice B: DAT icons in plugin markup) found one BLOCKING defect and 14 SHOULD-FIX findings. All 15 fixed here in one commit per the review contract. BLOCKING (finding 1): an unresolvable did painted a magenta square. TextureCache.GetOrUploadRenderSurface's 1x1 magenta placeholder for a missing RenderSurface is load-bearing for authored chrome, but RetailMarkupIconResolver.ResolveDid only short-circuited did==0, so any other unresolvable id fell through to that placeholder and got scaled up by UiMarkupIcon/UiMarkupList/UiSimpleButton -- the classic resolve(0)-style footgun (claude-memory/feedback_ui_resolve_zero_magenta.md), just triggered by a missing id instead of a literal 0. Fixed by probing Portal/HighRes existence via IDatReaderWriter.TryGet BEFORE ever calling GetOrUploadRenderSurface -- that TryGet already serializes concurrent DAT access internally (DatDatabaseWrapper's own _databaseLock), the same synchronization IconComposer.TryDecode relies on, so no additional lock was needed. RetailMarkupIconResolver now takes IDatReaderWriter + TextureCache directly (RetailUiAssets gained a TextureCache field, its one construction site in InteractionRetainedUiComposition.cs updated) instead of the old resolveSprite delegate, since it also needs the nearest-sampled upload path for finding 6 below. Finding 2 -- Smoke panel wiring bugs: its list fed iconkind="spell" raw art DIDs (PluginSpellInfo.IconId) instead of spell ids, so IMarkupIconResolver.ResolveSpell composited the wrong (or no) badge every row. SmokeIconPanel.Binding.SpellIds now yields SpellId (the printed text still shows IconId alongside). The bare-index demo and the descriptor both moved from the unverified literal 7735 to 0x165 -- retail's real Melee Defense skill icon (SampleData.cs:64, 0x06000165) -- so the owner's visual gate proves real art, not a guess. StartVisible flipped true, and a character with no self-buffs known falls back to spell 1's real catalog entry (or an honest "no spells known" row with icon 0 if even that fails) rather than fabricating art. Finding 3 -- PluginIcons.Normalize's threshold was silently rewritten from the contract's 0x01000000 to 0x06000000 during Slice B. Restored to 0x01000000; the class/method XML docs now state the number directly (no cref to the private const); the test table adds 0x02000000 (a value that only distinguishes the two thresholds) and 0x01000000 itself (passes through unchanged). Finding 4 -- an unknown iconkind (e.g. "spel") only threw when a resolver happened to be wired, because BuildIconSource/ BuildRowIconResolve validated inside their own null-icons early return. A new ValidateIconKind helper runs UNCONDITIONALLY before that branch, so a malformed iconkind is a Build-time author error on every host. Finding 5 -- BindUintLiteralOrBinding required an exact uint property type, rejecting the int-typed bindings Decal-facing code commonly uses (MosswartMassacre's HudPictureBox.Image is int end to end). It now matches BindUint's existing leniency: any property, converted via Convert.ToUInt32 at read time. BindUintList likewise now accepts IEnumerable alongside IEnumerable (unchecked per-element reinterpret -- icon ids never go negative in practice). Finding 6 -- TextureCache._renderSurfaceGpuTextures was keyed by id alone, so whichever caller asked for a given RenderSurface id FIRST won the sampler for every later caller of the same id -- UiDatFont's glyph atlases already request nearest:true while ResolveChrome's background art requests nearest:false, so this was a real, reachable collision, not hypothetical. Rekeyed to (id, nearest); RetailMarkupIconResolver. ResolveDid now requests nearest:true (pixel-exact 32x32 icon art); ResolveChrome is untouched (still nearest:false/linear). Audited every other _renderSurfaceGpuTextures use site (TryGetValue/set/Dispose iteration+Clear) plus the separate _nearestUiTextureSources/ _linearUiTwinHandles/_uploadMetadata dictionaries (all keyed by handle or accounting name, unaffected) -- no other eviction/accounting path assumed id-only keying. Finding 7 -- column-reservation semantics, per the DECIDED shape: MarkupDocument now sets button.IconSource / list.IconIdsSource + IconResolve ONLY when a resolver (icons parameter) is actually wired -- previously button.IconSource was always assigned (even to an always-empty func on an icons:null host); combined with this finding's other half -- UiSimpleButton.OnDraw now reserves its icon column whenever IconSource is non-null, regardless of a per-frame resolve miss, so a bound id that goes briefly to 0 no longer slides the caption back and forth -- would have permanently reserved a blank column on such a host. UiMarkupList already reserved its column whenever IconIdsSource was set; no draw-side change needed there. Finding 8 -- added a with/without-icons comparison test for UiMarkupList (mirroring the existing UiSimpleButton one): asserts the row text quad's x is strictly greater with an icon column present, and the icon quad itself has non-zero width. Finding 9 -- (empty string) was still treated as "has a tooltip" by a bare attribute-presence check, making the icon swallow clicks with no visible tooltip ever appearing. Now uses !string.IsNullOrWhiteSpace, matching ApplyCommon's own predicate for every other element's tooltip. Finding 10 -- PluginShelfButton.OnDraw drew nothing when a non-zero descriptor icon id resolved to no texture (a bad Decal index, a DAT id from a different install), rather than falling back to Initials the way a zero id already did. Now decides once, on the first draw (memoized, so Initials' string work never repeats every frame): a failed resolve permanently switches Text to the initials fallback, computed and assigned BEFORE base.OnDraw actually paints the caption. Finding 11 -- MarkupDocument.AddElement's switch had no default arm, so an unknown or miscased element name (, ) silently vanished from the built tree instead of failing loudly like every other malformed-markup case. Added a default arm that throws FormatException. Ran AcDream.Plugins.MossTank.Tests (337/337, unchanged) and the full App markup suite to confirm no existing markup relies on an unknown element. Finding 12 -- PluginPanelDescriptor.IconSurfaceId's XML doc now states that a bare Decal index is accepted and normalized, citing PluginIcons.Normalize. Finding 13 -- docs/plugin-ui-markup.md: replaced the blanket "wrong type/missing property throws at Build" sentence with the per-attribute truth table the review produced (which attributes are silent at runtime vs. throw at Build, and each one's bound CLR/delegate type); restated the icon-id boundary as 0x01000000; added the "do NOT add 0x06000000 to the four already-full IconId records" warning (citing SkillBase._iconID / UIRegion::SetImageByDID @0x004f150e); documented that 0x-prefixed hex is required (an unprefixed all-digit literal parses as decimal); noted unknown element names now throw; called out list colors (0xRRGGBB) vs. color=/background=/border= (#AARRGGBB) as non-interchangeable grammars; documented the root binding-only exception; corrected the shelf's collapse toggle glyphs (, not the old doc's arrows) and the 28px collapsed-tab size; added the IconId record-equality API-v1 note; and called out iconkind as per- (mixed id spaces need pre-normalized DIDs; the composited spell badge has no did-space escape hatch) plus the existing one-text-column LIMITATION being deferred to MossTank. Coverage added for finding 14: a PluginSidePanelTests case proving the shelf button normalizes a bare descriptor index before resolving, and a reflection-based unit on AppAutomationSurface.ProjectWorldObject (its public callers gate on IsAvailable, which needs a fully connected session heavier than this mapping needs -- the plan's own documented fallback) proving PluginWorldObject.IconId carries ClientObject.IconId through unchanged; PluginInventoryItem.IconId uses the identical one-line pattern inline in CaptureOwnedItems, reviewed by inspection. Finding 15: recorded a "Review ledger" section in the plan doc with both slices' commits, both review verdicts, and the two items explicitly deferred to the MossTank plugin work (multi-column list, root literal visible). Verification: full solution builds green. Targeted filter (Markup|PluginSidePanel|PluginIcons|AppAutomation|TextureCache| UiDatFont) passes 131/131, including the two InstalledDat-lane tests (RetailMarkupIconResolverInstalledDatTests, AppAutomationSurfaceIconInstalledDatTests) actually resolving against the real installed DAT, not skipping. AcDream.Plugins.MossTank.Tests passes 337/337 unchanged. Full AcDream.App.Tests suite: 7351 passed / 97 skipped / 36 failed -- identical failure set/count to the 7334/97/36 baseline (the +17 passes are exactly the new/expanded tests: 2 new PluginIconsTests.Normalize theory rows, 10 new MarkupIconTests cases, 2 new PluginSidePanelTests cases, 1 new AppAutomationSurfaceTests case, and the 2 new standalone test files). Co-Authored-By: Claude Fable 5.1 --- .../2026-09-06-plugin-shelf-and-dat-icons.md | 26 ++ docs/plugin-ui-markup.md | 137 +++++++++-- .../InteractionRetainedUiComposition.cs | 3 +- src/AcDream.App/Rendering/TextureCache.cs | 21 +- src/AcDream.App/UI/IMarkupIconResolver.cs | 59 ++++- src/AcDream.App/UI/MarkupDocument.cs | 187 +++++++++++---- src/AcDream.App/UI/PluginSidePanel.cs | 37 ++- src/AcDream.App/UI/RetailUiRuntime.cs | 10 +- src/AcDream.App/UI/UiPanel.cs | 18 +- .../IUiRegistry.cs | 7 + .../PluginIcons.cs | 39 +-- src/AcDream.Plugins.Smoke/SmokeIconPanel.cs | 100 ++++++-- .../Plugins/AppAutomationSurfaceTests.cs | 43 ++++ .../Plugins/PluginIconsTests.cs | 4 +- ...extureCacheRenderSurfaceSamplerKeyTests.cs | 190 +++++++++++++++ tests/AcDream.App.Tests/UI/MarkupIconTests.cs | 223 ++++++++++++++++++ .../UI/PluginSidePanelTests.cs | 90 +++++++ ...tailMarkupIconResolverInstalledDatTests.cs | 65 +++++ 18 files changed, 1124 insertions(+), 135 deletions(-) create mode 100644 tests/AcDream.App.Tests/Rendering/TextureCacheRenderSurfaceSamplerKeyTests.cs create mode 100644 tests/AcDream.App.Tests/UI/RetailMarkupIconResolverInstalledDatTests.cs diff --git a/docs/plans/2026-09-06-plugin-shelf-and-dat-icons.md b/docs/plans/2026-09-06-plugin-shelf-and-dat-icons.md index 2c94c004e..38bae66e0 100644 --- a/docs/plans/2026-09-06-plugin-shelf-and-dat-icons.md +++ b/docs/plans/2026-09-06-plugin-shelf-and-dat-icons.md @@ -252,3 +252,29 @@ all four places, list icon column aligned with rows. - **Out of scope (file, do not build):** a `/plugins` chat verb, a plugin manager panel, icon support in `menu`/`tab`, per-plugin shelves, item icon drag from plugin panels. + +## Review ledger + +- Slice A commits: `01b98ca30` (feat: movable, collapsible plugin shelf), + `4fada238e` (review fixes: real grip/toggle children, dialog z-order, + persisted collapse/hide intent), `718005b21` (residuals: padding-drag pin, + live dock detection, single clamper, chord text, register row), `2ebcc0164` + (grip sized from the DAT font, collapsed tab findable). +- Slice B commit: `8217a349e` (feat: DAT icons in plugin markup — icon + element, button/list icons, plugin icon ids). +- Slice A review verdict: two independent Opus lenses (architecture; + plugin-author/retail) found the shelf's shape sound; fix round landed the + three commits above and closed clean. +- Slice B review verdict: two independent Opus lenses found 15 findings (1 + BLOCKING — an unresolvable `did` painted a scaled-up magenta placeholder; + 14 SHOULD-FIX spanning the normalize threshold, per-attribute binding + leniency, sampler-keying, column-reservation semantics, and doc accuracy). + All 15 fixed in one review fix round (this commit); see its message for + the per-finding breakdown. +- **Deferred to the MossTank plugin work** (explicitly out of scope for this + fix round, carried forward rather than built here): + - Multi-column `` — today's list is one text column plus the + optional Slice B icon column; real tabular rows are MossTank's problem. + - Root `` binding-only literal support — the root element + currently accepts only `{Binding}` for `visible`, not a literal + `visible="true"/"false"` the way every child element does. diff --git a/docs/plugin-ui-markup.md b/docs/plugin-ui-markup.md index a6b540cfa..7b067a35c 100644 --- a/docs/plugin-ui-markup.md +++ b/docs/plugin-ui-markup.md @@ -54,15 +54,38 @@ Every attribute that isn't a plain literal is either: never touches `UiElement` objects directly, and never from a thread other than the one that calls `Tick`. -A binding that resolves to the wrong CLR type, or names a property that -doesn't exist, throws `FormatException` **at `Build`** — the same moment any -other malformed attribute throws — never silently at draw time. A resolved -binding that returns an out-of-range or default value (0, empty, null) at -*runtime* draws nothing/looks empty; it never throws after the panel has -loaded. +A binding failure's severity is per-attribute, not one blanket rule — see the +table below. An unrecognized `{Prop}` that resolves loudly (any row marked +"Throws") throws `FormatException` **at `Build`**, the same moment any other +malformed attribute throws, never silently at draw time. The attributes +marked "Silent" instead fall back to something visible-but-harmless at +*runtime* (the literal text, `null`, or `0`) — a plugin author who typos one +of those sees a wrong-looking value on screen rather than a crash, so double +check those four against the markup by eye. + +| Attribute(s) | On a missing/mistyped `{Prop}` | Bound CLR type | +|---|---|---| +| `label text`, `field text`, `menu selected`, `tooltip` (any element) | **Silent** — `BindString` falls back to the literal attribute text itself (a typo'd `{Typo}` renders as the literal string `{Typo}`) | `string` (via `.ToString()`) | +| `meter cur`, `meter max` | **Silent** — `BindUint` returns `null` (the meter shows no cur/max) | `uint?` (accepts any integral type) | +| `meter fill`, `slider value` | **Silent** — `BindFloat` returns `0` | `float?`/`float` | +| `list items`, `menu items` | Throws | `IEnumerable` | +| `list colors` | **Silent** if omitted (no color override); throws if present but mistyped | `IEnumerable` | +| `list icons` (Slice B) | Throws if present but mistyped; omitting it entirely means no icon column at all | `IEnumerable` **or** `IEnumerable` | +| ``/`