From 89dc99f4d1e6eae3012fdf6cf714fb520830b50a Mon Sep 17 00:00:00 2001 From: grom Date: Fri, 30 Oct 2026 09:02:47 -0600 Subject: [PATCH] What the second player actually saw: four panel entries and no nameplates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A second player's four `/errors` entries, all from one session, none of them a fault in the two addons he was reporting: 1. `FloatingChatFrame.xml: SetScript("OnInputLanguageChanged") unsupported`. Ours. The slot is real 1.12 and the *realm's own* FrameXML sets it on `ChatFrameEditBox`, so every player met a red error on every login for a line none of them wrote. `SCRIPT_KINDS` now accepts it and never fires it, which is also what the reference does — a client with no IME never fires it either. The rule the old test stated is unchanged and restated: a 1.12 slot benilla neither fires nor finds in a shipped file still raises. 2. `bindings.xml: root element is , not ; tolerated`. Every `Bindings.xml` in the corpus has that root and `bindings_xml` reads it correctly. No longer said. 3. `bindings.xml: Unknown frame type: Binding`, counted 69. Faithful — the reference logs the same line (`0x871124`) for every row while `0x51f400` registers them separately — and ours registers them too: AlternativeClassicActionBars' 69 bindings all answer `GetBinding`. Now a trace, so the terminal keeps it and the panel does not. 4. `SetOwner: unknown anchor 'BOTTOMRIGHT' on owner 'WorldFrame'`, counted 292. The hunt for that caller ended a while ago: it is `Shop_ProcessEntries` in the realm's own `Turtle_ShopUI`, off the player's patch chain, and the reference accepts an unknown anchorType silently (mode 0, ANCHOR_LEFT), as we do. The diagnostic has done its job; it is now host-log-once (`Model::warn_host_only_once`) and not retained. The panel is where someone looks to find what is broken in *their* interface. A line that is faithful, or names a file they cannot reach, crowds out the ones that are theirs. ---- A second player sent a video: Options → Nameplates with all six boxes ticked, the V binding set, and no plates at all — while the overhead *names* drew fine. `VPlateMode::default().range_sq` was `0.0`, and the gate is `distance_squared > range_sq`. Zero is not "no cull"; it is "cull everything, at any distance". `NameplateRange` is a registered CVar with default `"80"`, and a registered default is not written to `config.toml`, so no `CvarChanged` ever fired and the field never left its derived value. This is the trap the paragraph three fields above already documents, and which `occlusion` and `spread` were fixed by writing their registered defaults into `Default`; `range_sq` was left behind. It stayed hidden because `NameConfig::default()` — the overhead names — already carried `80.0 * 80.0`, so names drew and only plates were missing, which reads as a nameplate-addon problem rather than an engine one. It could not be reproduced here either: grom's own `config.toml` carries `NP_NameplateDistance = "62"`, which seeds the same field through the other observer arm. `the_plate_range_default_agrees_with_its_registered_cvar` now pins the default to `registered_pairs()`'s row and pins the plate and the name culls to each other, since a disagreement between those two is what made this read as somebody else's bug. It also answers the other half of his report. The gate `continue`s before the plate is seated at all, so with `range_sq` at 0 no plate frame ever exists — and UnitPlates works by adopting the plate frames under WorldFrame. One bug, both symptoms: "nameplates will not turn on" and "UnitPlates does nothing" are the same line. And a packaging answer that comes before both, for UnitPlates specifically: GitHub's "Download ZIP" unpacks `Redbu11dev/UnitPlates` as `UnitPlates-main` with `UnitPlates.toc` inside it. The client looks for `/.toc`, so the addon never loads — here or on the real client. The harness says it in as many words: "no manifest under other-addons/UnitPlates-main — is that an addon folder?". It has to be renamed `UnitPlates`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn --- crates/benilla-app/src/vplates.rs | 39 ++++++++++++++++-- crates/benilla-ui/src/framexml.rs | 8 +++- crates/benilla-ui/src/loader/mod.rs | 18 ++++++-- crates/benilla-ui/src/loader/tests.rs | 13 +++++- crates/benilla-ui/src/script/mod.rs | 8 +++- crates/benilla-ui/src/script/model.rs | 19 +++++++++ crates/benilla-ui/src/script/tests/input.rs | 41 ++++++++++--------- crates/benilla-ui/src/script/tooltip/verbs.rs | 40 ++++++++++-------- docs/ADDON_COMPAT.md | 38 +++++++++++++++++ 9 files changed, 180 insertions(+), 44 deletions(-) diff --git a/crates/benilla-app/src/vplates.rs b/crates/benilla-app/src/vplates.rs index d0722814..ebfbf330 100644 --- a/crates/benilla-app/src/vplates.rs +++ b/crates/benilla-app/src/vplates.rs @@ -82,8 +82,8 @@ pub(crate) struct VPlateMode { /// Written out rather than derived, because **a derived `false` here is not the same thing for /// every field**. `enemies`/`friends` are deliberately clear at boot (the reference leaves the -/// saved globals nil) and `range_sq` is seeded by the observer because the CVar it reads is one a -/// player's `config.toml` carries. `occlusion`'s CVar is registered `"1"` and is normally *absent* +/// saved globals nil) and `range_sq` carries its registered default, because the CVar it reads is +/// one a player's `config.toml` usually does *not*. `occlusion`'s CVar is registered `"1"` and is normally *absent* /// from that file — and a boot only emits `CvarChanged` for rows the file actually carries, so /// nothing would ever tell this field its real default. A derived `false` therefore shipped the /// feature off while the CVar read on, and writing `"1"` over a value already `"1"` is a no-op @@ -94,7 +94,15 @@ impl Default for VPlateMode { enemies: false, friends: false, critters: false, - range_sq: 0.0, + // `NameplateRange`'s registered default, 80 yd, squared — and it has to be written + // here for exactly the reason the paragraph above gives. A `0.0` derived default is + // not "no cull": the gate is `distance_squared > range_sq`, so zero culls *every* + // unit at any distance, and a player whose `config.toml` carries no `NameplateRange` + // row (the normal case — a registered default is not written to the file) got no + // nameplates at all, with both checkboxes ticked and V pressed. The overhead names + // kept working the whole time, because `NameConfig::default()` already wrote 80 here; + // that asymmetry is what the pairing below now forbids. + range_sq: 80.0 * 80.0, // Must equal the `NameplateOcclusion` row's registered default; `the_occlusion_default_ // agrees_with_its_registered_cvar` is that check. occlusion: true, @@ -924,6 +932,31 @@ mod tests { /// shipped off for exactly this reason: the CVar read `"1"`, the derived `Default` gave /// `false`, and writing `"1"` over `"1"` is a no-op that emits no event, so neither the /// checkbox nor the console command could correct it. + #[test] + fn the_plate_range_default_agrees_with_its_registered_cvar() { + let registered: f32 = crate::cvars::registered_pairs() + .find(|(n, _)| n.eq_ignore_ascii_case("NameplateRange")) + .map(|(_, d)| d) + .expect("NameplateRange is registered") + .parse() + .expect("a distance"); + assert_eq!( + VPlateMode::default().range_sq, + registered * registered, + "a derived default here is not `no cull` but `cull everything`: the gate is \ + `distance_squared > range_sq`, so 0 hides every plate for every player whose \ + config.toml has no NameplateRange row — which is every player who never moved the \ + slider" + ); + assert_eq!( + crate::nameplates::NameConfig::default().range_sq, + VPlateMode::default().range_sq, + "the plate and the overhead name share one cull: if they disagree a unit shows one \ + label where it should show the other, which is how this was missed — the names were \ + right and only the plates were gone" + ); + } + #[test] fn the_occlusion_default_agrees_with_its_registered_cvar() { let registered = crate::cvars::registered_pairs() diff --git a/crates/benilla-ui/src/framexml.rs b/crates/benilla-ui/src/framexml.rs index 6a993d63..049c8936 100644 --- a/crates/benilla-ui/src/framexml.rs +++ b/crates/benilla-ui/src/framexml.rs @@ -144,7 +144,13 @@ pub fn parse(text: &str) -> Result { let root = doc.root_element(); let mut warnings = Vec::new(); - if !root.tag_name().name().eq_ignore_ascii_case("Ui") { + // A `` root is not a deviation: it is the shape every `Bindings.xml` in the corpus + // has, the reference's own file loader walks the root's children regardless of the root's + // tag, and `bindings_xml` reads the rows out of it correctly. Saying so once per addon put + // the line in front of the player beside real faults, so it is left unsaid. + if !root.tag_name().name().eq_ignore_ascii_case("Ui") + && !root.tag_name().name().eq_ignore_ascii_case("Bindings") + { warnings.push(format!( "root element is <{}>, not ; tolerated (the reference's file loader walks \ the root's children regardless of the root's own tag)", diff --git a/crates/benilla-ui/src/loader/mod.rs b/crates/benilla-ui/src/loader/mod.rs index ea21c47c..3318a076 100644 --- a/crates/benilla-ui/src/loader/mod.rs +++ b/crates/benilla-ui/src/loader/mod.rs @@ -583,9 +583,21 @@ impl Loader<'_> { // miss logs `"Unknown frame type: %s"` (`0x871124`, at `0x6ee356`), builds nothing for // the node and goes on to the next sibling. Only the Lua door raises. if crate::script::object::registered_frame_kind(self.lua(), &el.tag).is_none() { - self.report - .warnings - .push(format!("Unknown frame type: {}", el.tag)); + // `` is the one unknown type that is *expected*: a `.toc`-listed + // `Bindings.xml` goes through this loader (`0x6ede10` from `0x6edd51`) and the + // reference logs the same line for every row, while `0x51f400` reads the rows + // separately and registers them. Ours does too — AlternativeClassicActionBars' + // 69 bindings all answer `GetBinding`. So it is a trace, not a warning: as a warning + // it reached the player's error panel 69 times per addon, next to real faults. + if el.tag.eq_ignore_ascii_case("Binding") { + self.report + .traces + .push(format!("Unknown frame type: {}", el.tag)); + } else { + self.report + .warnings + .push(format!("Unknown frame type: {}", el.tag)); + } return None; } diff --git a/crates/benilla-ui/src/loader/tests.rs b/crates/benilla-ui/src/loader/tests.rs index 4657feb0..ddd34ead 100644 --- a/crates/benilla-ui/src/loader/tests.rs +++ b/crates/benilla-ui/src/loader/tests.rs @@ -1098,6 +1098,11 @@ mod loader_tests { /// A `.toc` line hands a listed `Bindings.xml` to `0x6ede10` (from `0x6edd51`), so each /// `` logs as an unknown frame type; `0x51f400` reads the bindings regardless. + /// + /// Ours logs the same line, as a **trace**: the host's terminal keeps it and the player's + /// error panel does not. As a warning it was 69 entries per addon for + /// AlternativeClassicActionBars, whose 69 bindings all register and all answer `GetBinding` — + /// a count that large reads as a fault, beside the faults that are the player's. #[test] fn a_bindings_document_loaded_as_framexml_raises_nothing() { let s = UiScript::new().unwrap(); @@ -1111,12 +1116,18 @@ mod loader_tests { assert!(report.errors.is_empty(), "{:?}", report.errors); assert_eq!( report - .warnings + .traces .iter() .filter(|w| *w == "Unknown frame type: Binding") .count(), 2, "one log line per : {:?}", + report.traces + ); + assert!( + report.warnings.is_empty(), + "and none of them a warning — a Bindings.xml that loads correctly has nothing to \ + tell the player: {:?}", report.warnings ); assert!( diff --git a/crates/benilla-ui/src/script/mod.rs b/crates/benilla-ui/src/script/mod.rs index 63424a5b..5274278a 100644 --- a/crates/benilla-ui/src/script/mod.rs +++ b/crates/benilla-ui/src/script/mod.rs @@ -434,8 +434,14 @@ pub const SCREEN: crate::layout::Handle = 0; /// ScrollFrame's three scroll kinds are `+0x32c`/`+0x334`/`+0x33c` (script-name map `0x786c40`). /// The EditBox's vtable (`0x81c910`) replaces the key and char slots: an EditBox never fires /// `OnKeyDown`, and fires `OnChar` only from `Insert`, with the inserted text (`0x77c13c`). -const SCRIPT_KINDS: [&str; 42] = [ +const SCRIPT_KINDS: [&str; 43] = [ "OnLoad", + // Accepted and never fired, deliberately: the realm's own `FloatingChatFrame.xml` sets it on + // `ChatFrameEditBox`, so refusing it put a red `SetScript: unsupported script` in front of + // every player on every login for a line none of them wrote. The reference registers the slot + // (an IME language switch) and a client with no IME never fires it either, so accepting and + // never calling it is what 1.12 does, not a silent failure of ours. + "OnInputLanguageChanged", "OnEvent", "OnUpdate", // The model pane's two, fired by the tick's model pass (`tick_model_panes`): `OnUpdateModel` diff --git a/crates/benilla-ui/src/script/model.rs b/crates/benilla-ui/src/script/model.rs index 6757e184..1d1e2f8e 100644 --- a/crates/benilla-ui/src/script/model.rs +++ b/crates/benilla-ui/src/script/model.rs @@ -1452,6 +1452,11 @@ pub(crate) struct Model { /// The tokens a combat-stats verb has already answered the stand-in for, so the warning it /// records is one line per VM rather than one per call (`char_stats::with_unit_stats`). pub(crate) stats_stand_in_warned: std::collections::HashSet, + + /// Messages [`Model::warn_host_only_once`] has already put on the host's terminal, so a + /// diagnostic about something the player cannot act on says itself once a session and never + /// reaches the retained log. + pub(crate) host_only_warned: std::collections::HashSet, pub(crate) inventory_slots: char_stats::InventorySlots, /// The six bank-bag slots, ids 64..=69, fed from the player descriptor's guids. pub(crate) bank_bag_slots: char_stats::BankBagSlots, @@ -1722,6 +1727,19 @@ impl Model { self.warnings.push(msg); } + /// [`Self::warn_host_only`], said once a session: the host's terminal gets the first copy and + /// the retained log — the one `/errors` and `BenillaScriptLogFrame` read — gets none. + /// + /// For a diagnostic that is **true, settled and not the player's to act on**. The panel is + /// where someone looks to find what is broken in *their* interface, so a line that names a + /// file off their own patch chain, or an engine note the reference logs too, crowds out the + /// lines that are theirs. The terminal and the session log still carry it. + pub(crate) fn warn_host_only_once(&mut self, msg: String) { + if self.host_only_warned.insert(msg.clone()) { + self.warnings.push(msg); + } + } + /// A model pane's subject: the guid its `SetUnit` pinned ([`crate::widget::ModelState::unit_guid`]), /// else its token re-read, for a pane seated before its unit streamed in. /// @@ -2347,6 +2365,7 @@ impl Model { player_combat_stats: None, pet_combat_stats: None, stats_stand_in_warned: Default::default(), + host_only_warned: Default::default(), inventory_slots: Default::default(), bank_bag_slots: Default::default(), inventory_alerts: [0; 12], diff --git a/crates/benilla-ui/src/script/tests/input.rs b/crates/benilla-ui/src/script/tests/input.rs index dfe262b9..31573c93 100644 --- a/crates/benilla-ui/src/script/tests/input.rs +++ b/crates/benilla-ui/src/script/tests/input.rs @@ -559,29 +559,32 @@ fn set_script_on_double_click_is_accepted_because_something_fires_it() { assert!(s.eval::("return ran").unwrap(), "…and it FIRED"); } -/// Script names nothing fires still raise; a name leaves this list only when something fires it. +/// A script name nothing resolves still raises rather than being silently accepted. +/// +/// A name leaves this test for one of two reasons. `OnHyperlinkEnter`/`Leave` left when +/// `pointer::update_hyperlink_hover` started firing them off the rasterized link spans, and +/// `OnMessageScrollChanged` when the scroll verbs did — something started firing them. +/// `OnInputLanguageChanged` left for the other reason: **the reference's own interface sets it** +/// (`FloatingChatFrame.xml`, on `ChatFrameEditBox`), so refusing it is a fault report about a line +/// the player did not write, and a client with no IME never fires it there either — see +/// `SCRIPT_KINDS`' own comment. What is left is the case neither reason covers: a slot no 1.12 +/// resolver has at all. #[test] -fn the_unfired_script_kinds_still_raise_rather_than_silently_accepting() { +fn an_unresolvable_script_kind_still_raises_rather_than_silently_accepting() { let s = script(); s.run(r#"Raiser = CreateFrame("EditBox", "RaiserBox")"#) .unwrap(); - for name in [ - // A 2.0 secure-frame slot; no 1.12 resolver has it. - "OnAttributeChanged", - // Real 1.12 slots that benilla does not fire. `OnHyperlinkEnter`/`Leave` left this list - // when `pointer::update_hyperlink_hover` started firing them off the rasterized link - // spans, and `OnMessageScrollChanged` when the scroll verbs did — which is the rule this - // test states, not an exception to it. - "OnInputLanguageChanged", - ] { - let err = s - .run(&format!(r#"Raiser:SetScript("{name}", function() end)"#)) - .unwrap_err(); - assert!( - err.to_string().contains(name), - "{name} must raise by name, got {err}" - ); - } + // A 2.0 secure-frame slot; no 1.12 resolver has it, and nothing shipped sets it. + let err = s + .run(r#"Raiser:SetScript("OnAttributeChanged", function() end)"#) + .unwrap_err(); + assert!( + err.to_string().contains("OnAttributeChanged"), + "it must raise by name, got {err}" + ); + // …while the one that left does not raise, and stores nothing anyone can fire. + s.run(r#"Raiser:SetScript("OnInputLanguageChanged", function() end)"#) + .expect("a slot the reference's own FrameXML sets must be accepted"); } /// Hiding the hovered frame fires its `OnLeave` inside the hide, before its `OnHide` (`0x764ba0`'s diff --git a/crates/benilla-ui/src/script/tooltip/verbs.rs b/crates/benilla-ui/src/script/tooltip/verbs.rs index 6e0b85cb..f834dcb7 100644 --- a/crates/benilla-ui/src/script/tooltip/verbs.rs +++ b/crates/benilla-ui/src/script/tooltip/verbs.rs @@ -140,10 +140,15 @@ pub(in crate::script) fn install(lua: &Lua) -> mlua::Result<()> { // in TurtleWoW's own `Interface\FrameXML\Turtle_ShopUI`, off the // player's patch chain: not an addon, not ours, nothing the player // can fix, and our mode-0 fallback already matches the reference. 283 - // identical lines a session bury every other warning in the log; the - // first is still the whole diagnostic, and the retained row's `count` - // keeps the true total for `BenillaScriptLogFrame`. - model.record_warning_once(format!( + // identical lines a session bury every other warning in the log. + // + // And `warn_host_only_once`, not `record_warning_once`: the retained + // row showed up in the player's own `/errors` panel as one entry + // counted 292 times, next to the faults that *are* theirs, for a call + // the reference itself accepts silently (see the mode-0 note above) + // from a file off their patch chain. The terminal and the session log + // still carry the full diagnostic, once. + model.warn_host_only_once(format!( "SetOwner: unknown anchor '{other}' on owner '{owner_name}' \ from {site} (mode 0, ANCHOR_LEFT)" )); @@ -512,10 +517,15 @@ mod tests { } /// The hunt is over, so the repeats are noise: one terminal line per distinct `(anchor, owner, - /// site)`, while the retained log still counts every occurrence. The caller the diagnostic - /// found is `Shop_ProcessEntries` in TurtleWoW's own `Interface\FrameXML\Turtle_ShopUI` — off - /// the player's patch chain, so nothing in the session can stop it calling, and 283 copies of - /// one unfixable warning bury every other line in the log. + /// site)`, and **nothing retained**. The caller the diagnostic found is `Shop_ProcessEntries` + /// in TurtleWoW's own `Interface\FrameXML\Turtle_ShopUI` — off the player's patch chain, so + /// nothing in the session can stop it calling, and 283 copies of one unfixable warning bury + /// every other line in the log. + /// + /// It stopped being retained when a second player's `/errors` opened on four entries and one + /// of them was this, counted 292 times, for a call the reference itself accepts silently + /// (the mode-0 note on `SetOwner` above). A panel someone reads to find what is broken in + /// their own interface must not lead with the one line that is not theirs. #[test] fn a_repeated_unknown_anchor_reaches_the_host_once_and_the_retained_row_counts_them_all() { let mut s = UiScript::new().unwrap(); @@ -541,14 +551,12 @@ mod tests { host[0].contains("on owner 'NoisyHost'") && host[0].contains("ShopProcessEntries"), "the one line kept must still name the owner and the call site: {host:#?}" ); - let row = s - .diagnostics() - .into_iter() - .find(|d| d.message.contains("unknown anchor 'BOTTOMRIGHT'")) - .expect("the warning is retained for the player's log"); - assert_eq!( - row.count, 283, - "…and nothing is lost: the retained row carries the true total" + assert!( + !s.diagnostics() + .into_iter() + .any(|d| d.message.contains("unknown anchor 'BOTTOMRIGHT'")), + "…and the player's retained log takes none of it: the session log has the line, and \ + `/errors` is for faults the player can act on" ); } diff --git a/docs/ADDON_COMPAT.md b/docs/ADDON_COMPAT.md index e0da0717..0158d471 100644 --- a/docs/ADDON_COMPAT.md +++ b/docs/ADDON_COMPAT.md @@ -1404,6 +1404,44 @@ the failure is silent on both clients. `loader/tests.rs`'s `a_bindings_document_loaded_as_framexml_raises_nothing` pins the count deliberately. Noise, faithfully. +### Round two: what the second player actually saw + +He reported both addons "still not working" and sent four screenshots of the error panel. Not one +of the four was a fault in either addon — all four were **benilla telling him about things that +were either faithful or not his**, and the panel is the first place someone looks to find what is +broken in their interface: + +| entry | count | verdict | +|---|---|---| +| `FloatingChatFrame.xml: SetScript("OnInputLanguageChanged") unsupported` | 1 | **ours** — a stock script slot we refused. Now in `SCRIPT_KINDS`, accepted and never fired, as the reference has it. The line is in the realm's *own* FrameXML, so every player met it on every login. | +| `bindings.xml: root element is , not ; tolerated` | 1 | noise — every `Bindings.xml` in the corpus has that root and ours reads it correctly. No longer said. | +| `bindings.xml: Unknown frame type: Binding` | 69 | faithful (the reference logs it too, `0x871124`), and the bindings all register — `GetBinding` answers for all 69. Now a **trace**: the terminal keeps it, the player's panel does not. | +| `SetOwner: unknown anchor 'BOTTOMRIGHT' on owner 'WorldFrame'` | 292 | `Turtle_ShopUI.lua:530`, the realm's own file, off his patch chain, and the reference accepts an unknown anchorType silently (mode 0). Now host-log-once and not retained. | + +There is also a **packaging** answer, and it is the first thing to check with him: GitHub's "Download +ZIP" unpacks `Redbu11dev/UnitPlates` as a folder named **`UnitPlates-main`**, and it ships +`UnitPlates.toc` inside. The client looks for `/.toc` and finds nothing, so the +addon never loads at all — on this client or on the real one. Our own copy under `other-addons/` +has the same name, and the harness says so plainly: *"no manifest under +other-addons/UnitPlates-main — is that an addon folder?"*. It has to be renamed `UnitPlates`. + +And the one he was really reporting, which was in neither addon: **nameplates would not turn on at +all.** `VPlateMode::default().range_sq` was `0.0`, and the gate is `distance_squared > range_sq` — +so zero does not mean "no cull", it means "cull everything at any distance". `NameplateRange` is a +registered CVar with default `"80"`, and a registered default is *not written to `config.toml`*, so +no `CvarChanged` ever fired and the field never left its derived value. Any player who had never +moved that slider got no nameplates, with both checkboxes ticked and V pressed. + +And it is the other half of "UnitPlates does nothing" too: the gate `continue`s **before the plate +is seated**, so with `range_sq` at 0 no plate frame ever exists, and UnitPlates works by adopting +the plate frames under `WorldFrame`. One line, both symptoms. + +This is the same trap documented three fields up for `occlusion` and `spread`, which were fixed by +writing their registered defaults into `Default`; `range_sq` was left behind. The overhead *names* +kept working the whole time, because `NameConfig::default()` already carried `80.0 * 80.0` — and +that asymmetry is exactly what made it look like an addon problem. `the_plate_range_default_agrees_ +with_its_registered_cvar` now pins both to the registered row and to each other. + ### The harness grew a `--plates` step A nameplate addon is the one kind the survey could not see working: its subject is a `WorldFrame`