ui_script: the LoadOnDemand log line stops claiming "not loaded"

A LoadOnDemand addon can already be in memory when the walk reaches its row, because an addon
ahead of it loaded it from its own `ADDON_LOADED` — which is how every AI_VoiceOver data module
actually arrives: 224 ms of Lua inside AI_VoiceOver's handler, before the walk gets to the
`AI_VoiceOverData_*` rows at all. The line was printed unconditionally, so it went on saying "no
LoadAddOn() yet" about addons that had loaded. That reads as a bug and is not one; it cost a
session chasing a missing `GetAddOnEnableState` that was never the problem. Gated on a new
`UiScript::addon_is_loaded`.

`docs/ADDON_COMPAT.md` now records why that global stays unbuilt, since the evidence was gathered
anyway: upstream ClassicAPI's own `docs/API.md` does not define it either, and AI_VoiceOver gates a
modern-shaped `GetAddOnInfo` on the same `if not GetAddOnEnableState`, so defining it would leave
`VoiceOver.lua:84` reading `loadable` where it means `reason` and pop the "delete your old folder"
dialog on every login.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn

crates/benilla-app/src/ui_script/addons.rs

@@ -759,10 +759,17 @@ pub(super) fn load_third_party(
         // `## LoadOnDemand: 1` waits for `LoadAddOn`: `0x51f600` loads only records whose
         // LoadOnDemand byte is 0.
         if addon.toc.load_on_demand() {
-            info!(
-                "ui_script: {} is LoadOnDemand — not loaded (no LoadAddOn() yet)",
-                addon.name
-            );
+            // Said only while it is still true. An addon earlier in the walk can have loaded this
+            // one already, from its own `ADDON_LOADED`: AI_VoiceOver's `OnInitialize` does
+            // exactly that for its three data modules, 224 ms of it, and this line then went on
+            // claiming "no LoadAddOn() yet" for addons that were already in memory — which reads
+            // as the bug it is not, and cost a debugging session saying so.
+            if !script.addon_is_loaded(&addon.name) {
+                info!(
+                    "ui_script: {} is LoadOnDemand — not loaded (no LoadAddOn() yet)",
+                    addon.name
+                );
+            }
             continue;
         }
         // Re-armed per addon, so one runaway cannot fail every addon after it; a dependency

crates/benilla-ui/src/script/addon.rs

@@ -701,6 +701,16 @@ impl super::UiScript {
     }
 
     /// Mark an addon the startup walk ran as loaded, for `IsAddOnLoaded`.
+    /// Is this addon's load already done? True for one the startup walk ran **and** for one an
+    /// earlier addon pulled in with `LoadAddOn` from its own `ADDON_LOADED` handler, which is how
+    /// every LoadOnDemand data module actually arrives.
+    pub fn addon_is_loaded(&self, name: &str) -> bool {
+        self.model_ref()
+            .addons
+            .iter()
+            .any(|a| a.name.eq_ignore_ascii_case(name) && a.loaded)
+    }
+
     pub fn mark_addon_loaded(&mut self, name: &str) {
         if let Some(a) = self
             .model_mut()

docs/ADDON_COMPAT.md

@@ -158,6 +158,7 @@ in this file on the strength of the namreeb research. pepo's v2 has its own
 | `GameTooltip:GetUnitGUID` | **done — a deliberate reversal, with a new mechanism** | `compat/classic_api/tooltip.rs`, the 9th reader method. The `GameTooltip (reader methods)` row above lists this as *not built*, and the objection recorded there was and is correct: a unit *token* is resolved against the live snapshots per call, so re-resolving `TooltipState::unit_token` at read time answers for whatever that token names **now** — right most of the time and silently wrong exactly when it matters, because a `"mouseover"` tooltip routinely outlives its mouseover. What changed is not the standard but the mechanism: the guid is now **recorded beside the token at `SetUnit` time** (new `TooltipState::unit_guid`, `widget/kinds/mod.rs`; written by `tooltip_unit.rs`'s `set_live_token`, cleared by `tooltip/mod.rs`'s `clear_content` with the rest of the content). The getter therefore hands back a set-time fact and resolves nothing, which *is* the modern getter's promise — and the stale-token case it used to be wrong about is now the case it is most right about. Shape pinned by the live call site, not by memory: pfUI's `modules/tooltip.lua:81` is `local _, guid = GameTooltip:GetUnitGUID()` feeding `UnitTokenFromGUID`, so it returns `(name, guid)` — `GetUnit()`'s two-value shape with the guid in the token's place, and **no third value**, since nothing pins a third position. The *name* is resolved from the guid (through `Model::unit`'s SuperWoW raw-guid arm), not from the token, so the pair always describes one unit; a guid nothing can reach any more answers the guid with no name, because the identity outlives the snapshot. Nothing at all when `SetUnit` saw guid 0, which leaves `HasUnit()` true and is what pfUI's own `guid and UnitTokenFromGUID(guid)` reads. Built because the absence cost **14 errors in one play session** — that call site is on the tooltip's `OnShow`, so it fires on every unit hover. The test fails against the rejected mechanism, not merely against absence: `get_unit_guid_keeps_answering_for_the_unit_set_unit_saw_after_the_token_moves_on` |
 | `INVSLOT_FIRST_EQUIPPED` / `INVSLOT_LAST_EQUIPPED`, `GetInventoryItemDurability` | done | `compat/classic_api/item.rs`'s new `install_globals` — bare globals, which is how upstream's own list spells them (it writes `C_Macro.CreateMacro` in full and leaves these unqualified). The two constants are **1 and 19**, read off this tree rather than a later client's headers: `char_stats.rs`'s `InventorySlots` is documented "0 ammo, 1..=19 equipment, 20..=23 the equipped bags", its `SLOT_INFO` is `PaperDollItemFrame.dbc`'s own table with `HeadSlot` 1 and `TabardSlot` 19, and `item.rs`'s existing `inventory_slot_token` enumerates the same 19. Neither name is 1.12's (`reference/1.12-globals.tsv` has no `INVSLOT_*` row at all — 1.12 spells each slot as `GetInventorySlotInfo("HeadSlot")`), and their absence was a session's **first** error: pfUI uses them as `for` bounds at module scope four times (`modules/panel.lua:414`, `:445`, `modules/itemcount.lua:7`) and a nil bound raises "'for' initial value must be a number", taking the enclosing block with it. The rest of the `INVSLOT_*` family is deliberately left out: nothing installed asks, and 1.12 already answers each slot by name. `GetInventoryItemDurability(slot)` is the one member of the `GetInventoryItem*` family 1.12 lacks — the reference ships the `GetInventoryItemBroken` predicate and never the numbers behind it — and needed **no new feed**: `InvSlotView::durability` already carries `ITEM_FIELD_DURABILITY`/`ITEM_FIELD_MAXDURABILITY` (fed by `ui_char.rs`'s `slot_view`), which the item tooltip's "Durability X / Y" line and the doll's red tint already render. Slot-only, no unit argument, as the modern function and every pfUI call site have it. Three answers kept distinct: **no values** for an empty slot (what `if cur and max then` branches on), the live pair for a damageable item, and **`(0, 0)`** for a worn item with no durability at all — the feed stores that as `None`, so it is unfolded here, because a worn ring is not an empty finger |
 | `GetSpellBonusDamage` | done | `script/char_stats.rs`, beside the `UnitDamage` that reads the same wire field. A school's spell power: 1.12 has no character-sheet line for it and no global to ask, which is why it reads as a later-client function — but **the number is already streamed**. A vanilla server folds item spell power into `PLAYER_FIELD_MOD_DAMAGE_DONE_POS`/`_NEG` (`_ApplyItemBonuses` → `ApplySpellDamageBonus`), the very field `UnitDamage` already reports index 0 of as the physical pos/neg pair, so schools `[1..7]` were arriving and being dropped at the feed: `ui_char.rs` read `player_mod_damage_done_pos(0)` alone. The snapshot now carries `damage_bonus_pos`/`_neg` as `[i32; 7]` (the shape `resistance_pos`/`_neg` already use) and `UnitDamage` takes `[0]` of it, so there is one model of the field rather than a scalar and a derived duplicate. **Why it mattered**: pfUI's player module polls schools 2..7 every 0.25s from its throttled `OnUpdate` to draw a "+N SP" readout (`modules/player.lua:92`), so on a caster the missing global raised *per tick* — 433 runtime errors in one logged session on a fresh warlock, the single largest error source in the log, and invisible on a warrior because the readout resolves to 0 and never shows. Schools are 1-based (1 physical … 7 arcane) and anything off that range answers **0, not nil and not a raise**, which is what the call site's own `or 0` assumes. Its sibling `GetSpellBonusHealing` stays **unbuilt and is not approximable**: vanilla has no healing player field at all — 0x40A is `PLAYER_FIELD_BYTES`, the healing-done fields are a 2.0 addition — so there is nothing on the wire to read, and the number would have to be summed from equipped item stats. Nothing installed calls it. `UnitSpellHaste`, which the same pfUI function calls one line earlier, stays `todo` for the reason its own row gives (no haste stat exists in 1.12); it is reached only when that module's haste display is switched on, which this player's config has off |
+| `GetAddOnEnableState` (the legacy global) | **deliberately not done — checked, not assumed** | Upstream's own `docs/API.md` AddOns section lists the ten entries above and **not** this one, and keeps 1.12's `GetAddOnInfo` with `enabled` 4th. So the DLL actually loaded on this install does not define it either: adding it here would be a *divergence from the player's own client*, not parity with it. It would also break the one addon that asked for it. `AI_VoiceOver/Compatibility.lua:121` gates **two** definitions on one `if not GetAddOnEnableState` — its own enable-state fallback *and* a modern-shaped `GetAddOnInfo` that drops the `enabled` return. Define the global and VoiceOver keeps 1.12's 8-tuple while reading it at modern offsets, so `VoiceOver.lua:84`'s `select(5, GetAddOnInfo("VoiceOver")) ~= "MISSING"` reads `loadable` where it means `reason`, is true for a folder that is not installed, and raises the "delete your old VoiceOver folder" popup every login — twice, `AI_VoiceOver_112` too. Recorded because this was first diagnosed the other way round: the three `AI_VoiceOverData_*` modules look unloaded in the log and are not. They load inside AI_VoiceOver's own `ADDON_LOADED`, 224 ms of it, before the walk ever reaches their rows — and the walk's `is LoadOnDemand — not loaded (no LoadAddOn() yet)` line used to be printed unconditionally, so it went on saying "not loaded" about addons already in memory. That line is now gated on `UiScript::addon_is_loaded` |
 | `C_Item.GetItemInfo` | done, delegating | `compat/classic_api/item.rs`. Calls the **stock 1.12 global** (`script/item_stats.rs`), which owns the template cache and the `CMSG_ITEM_QUERY` warm an uncached id records — so a modern caller feeds the one shared cache and a hook over the stock global is honoured, same discipline as `C_AddOns`. The shape is modern WoW's, by exactly the transform `item_stats.rs`'s own comment already names ("later clients insert `itemLevel` at 4"): returns 1..3 pass through, **position 4 is nil**, stock's 4..9 land at 5..10. Nil rather than a number because item level is not in `SMSG_ITEM_QUERY_SINGLE_RESPONSE` and 1.12 exposes no such concept (the same reason `GetCurrentItemLevel` stays unbuilt) — a fabricated 0 would read as a level-0 item where nil reads as "not known here", which a caller can tell apart. Modern positions and not 1.12's because a live call site against the real DLL pins them: pfUI's `modules/bags.lua:457` reads `local _, _, q, _, _, _, itype` and tests `itype == "Quest"` at `:475`, and position 7 is the subType *string* in the modern run against the stack *count* in 1.12's — forwarding the nine verbatim would have left that test silently never firing. Same evidentiary standard as the `ColorMixin` row below. Stops at 10: modern's 11th onward (sellPrice, classID, subclassID, bindType, …) are facts this client holds, but nothing available pins their *order* and each already has its own `*ByID` entry, so they are left off rather than placed from memory |
 | `IsSpellKnown` (bare) | done | `compat/classic_api/spell.rs`'s new `install_globals`, matching upstream's own heading "Spell (`C_Spell`, plus several bare globals)". It hands out **the same function value** `C_Spell.IsSpellKnown` holds, lifted out of that table rather than written twice — so the two spellings cannot drift and there is no second body to maintain (`the_bare_is_spell_known_is_the_very_function_c_spell_holds` asserts the identity, not just the behaviour). Not a 1.12 name: 1.12 asks "do I know this" only by walking the book with `GetSpellName(i, "spell")`, never by id. pfUI's `modules/bags.lua:703` calls it bare, 3 occurrences in one session |
 | `GetMacroSpell` (bare) | done — **the "not built" note in `macro_ext.rs` was wrong about this client** | `compat/classic_api/macro_ext.rs`'s `install_globals`. The triage item that produced this said `C_Macro.GetMacroSpell` already existed and only needed exposing; it did not exist anywhere, and that file's header gave a reason — "resolving a body's first `/cast` line needs the rank-aware spellbook lookup `C_Spell`'s row notes is unbuilt". **Both halves do exist and are already load-bearing.** `benilla-app`'s `ui_macro/run.rs` is a traced reimplementation of the reference's own macro→spell derivation (`0x4efe00` writing `[rec+0x564]`, reading the `SLASH_CAST%d` aliases as Lua globals exactly as `0x703bf0` does, plus a `CastSpellByName(` arm) and resolves through `spellbook.rs`'s `resolve_spell_by_name`, the same resolver `CastSpellByName` and `/cast` use; the action bar has been reading its result for a macro slot's cooldown, usability and range all along. So this is **not new derivation** — it is the existing one, fed into the VM as `Model::macro_bound_spells` by the existing `rebind_macro_spells` system (no new system, no new file, no ceiling movement) and read back by id through the same `spell_display` `C_Spell.GetSpellName` uses. Returns `(name, rank)`, two values only: pfUI reads `effect, rank = GetMacroSpell(id)` (`libs/libpredict.lua:1027`) and compares the first to a spell name (`modules/swingtimer.lua:430`), so a bare-spellID return (what the newest clients give) would fail both, and no third value is appended because nothing pins its position. Rank is a string, `""` when rankless, the rule the stock `GetSpellName` already keeps. Only resolved spells cross the feed: `BoundSpell::None` and `BoundSpell::Unresolved` are both "no spell" to Lua, which has one way to say it, while the bar needs them apart. Stated latency: one frame after an in-Lua `EditMacro`, the identical delay the action bar already has, from the identical source. `GetMacroItem` stays unbuilt — the reference's macro record binds a spell and has no item counterpart to derive |