From e95de4a5ad85d1a1d80050226576c56c3a9c4b76 Mon Sep 17 00:00:00 2001 From: grom Date: Sun, 18 Oct 2026 15:55:24 -0600 Subject: [PATCH] log: the session's warnings are 330 down to 129, and the 201 that went were ours MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A full inventory of the player's 2026-10-18 session: **330 WARN lines, 67 distinct shapes, and not one ERROR**. Read one at a time against the code and the reference, most of what remained was the client complaining about itself. Three causes, measured by replaying the new rules over that same log: **330 → 129**. **A label is not a key (33 lines).** `text=` resolves as a GlobalStrings key and a miss shows the raw value, which is faithful; the warning on a key-shaped miss is ours. But a miss is already a string no locale defines, so a single all-caps word is a label the author typed — `OK`, `UP`, `DOWN`, `MT`, `TPS`, `TOP`, `BOTTOM`, `WIM`, `TWA`, 42 lines of them in one session, every one rendering exactly as the reference renders it. The predicate now wants an underscore, which keeps the shape that cannot be anything else: `ATSW_ALLREAGENTLISTFRAME_CHN`, an addon's own locale row missing from the locale it shipped, which really does paint its key name on screen. The shipped-XML sweep keeps its own broader rule, where `text="DELETE"` must resolve and a collision is a bug either way; its comment now says so. **Prose is not a missing file (12 lines, and phantom load failures).** A `.toc` is read line by line, `#` opening a metadata line and everything else a filename, so a wrapped `## Notes:` paragraph and the `--` section headers many addons write become entries that no file answers. The reference opens them, fails, and says nothing. We called each one a load failure, which put it in `/errors` and counted it in the "N addon load failures" line at every login — ColorPickerPlus shipping "two files it does not contain" that are really one two-line sentence. A line that cannot name a file is now a debug line; one that looks like a file and is absent stays a warning, because then the package really is short a piece (BigWigs' `Raids\Other\Nerubian.lua`). The addon harness shares the predicate, so its package audit stops making the same claim. **One cause speaks once (168 lines → 9).** The stock inspect panel's sixteen slots each name a frame `Blizzard_InspectUI` has never been asked to build, and QuickHeal's one undefined template is inherited ten times — per UI load, so 78 lines a session for two causes. A document's log warnings now collapse by shape: the first two keep their detail, the rest become one "N more like …" line, and `/errors` still gets every one, because the panel is where detail belongs. Three shapes were checked and deliberately left, because the client is right: `SetScript("OnInputLanguageChanged")` must keep raising (an unfired script kind raises by this project's own rule, and nothing can fire an IME edge here); the `SetOwner: unknown anchor 'BOTTOMRIGHT'` line is TurtleWoW's own `Turtle_ShopUI.lua:530`, where our mode-0 fallback already matches the reference and the line is already once per session; and a `.toc`-listed `Bindings.xml` walks as FrameXML with `` an unknown frame type, which the reference logs too (the bindings themselves load by their own path and work). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn --- crates/benilla-app/src/addon_harness/mod.rs | 8 ++ crates/benilla-app/src/ui_script/addons.rs | 116 +++++++++++++++++- .../src/ui_script/shipped_xml_tests.rs | 9 +- crates/benilla-ui/src/loader/mod.rs | 15 ++- crates/benilla-ui/src/loader/tests.rs | 30 ++++- 5 files changed, 170 insertions(+), 8 deletions(-) diff --git a/crates/benilla-app/src/addon_harness/mod.rs b/crates/benilla-app/src/addon_harness/mod.rs index 87c75625..39f66cc5 100644 --- a/crates/benilla-app/src/addon_harness/mod.rs +++ b/crates/benilla-app/src/addon_harness/mod.rs @@ -1574,6 +1574,14 @@ fn load_addon_files(script: &UiScript, root: &Path, name: &str, toc: &Toc) -> Fi for file in &toc.files { let path = benilla_ui::loader::join_ref(&base, file); let Some(bytes) = read_under(root, &path) else { + // A `.toc` line that cannot name a file is prose, not an incomplete package: the + // comments addons write and the wrapped `## Notes:` paragraphs + // ([`crate::ui_script::addons::is_file_entry`]). Counting them told the reader that + // ColorPickerPlus ships two files it does not contain, when what it ships is a + // two-line sentence. + if !crate::ui_script::addons::is_file_entry(file) { + continue; + } errors.push(format!("{file}: not found")); // Whose package is incomplete: `join_ref` has collapsed the `..`s as the 1.12 client // does (`0x6ede10`), so a path still inside the addon's folder is its own manifest diff --git a/crates/benilla-app/src/ui_script/addons.rs b/crates/benilla-app/src/ui_script/addons.rs index 0a4bfd8b..20167f89 100644 --- a/crates/benilla-app/src/ui_script/addons.rs +++ b/crates/benilla-app/src/ui_script/addons.rs @@ -121,6 +121,15 @@ impl Addon { // Resolved once into the source's path space, for `read` and the loader alike. let path = benilla_ui::loader::join_ref(&self.prefix(), file); let Some(bytes) = self.read(&path) else { + // Prose, not a file: a `.toc` line that cannot name one ([`is_file_entry`]). + if !is_file_entry(file) { + debug!( + "ui_script: {}/{file}: not a file reference — a `.toc` line that is a \ + comment or a wrapped metadata paragraph, as the reference also reads it", + self.name + ); + continue; + } let e = format!("{}/{file}: not found", self.name); // Severity follows whose manifest is wrong. Ours (the builtin, or a `benilla.toc` // line the player's chain lacks) is an ERROR. A player's addon is the package's @@ -168,11 +177,35 @@ impl Addon { for t in &report.traces { info!("ui_script({}/{file}): {t}", self.name); } + // One document's warnings collapse by shape in the LOG, because a single cause + // speaks many times: the stock inspect panel's sixteen slots each name a frame + // `Blizzard_InspectUI` has not been asked to build, and QuickHeal's one undefined + // template is inherited ten times. The shape is the message without its trailing + // identifier, the first two of each keep their detail, and the rest become a count — + // 78 lines of the player's session were four causes. `/errors` still gets every one: + // the panel is where the detail belongs. + const PER_SHAPE: usize = 2; + // Ordered, so the tail lines read the same run to run. + let mut per_shape: std::collections::BTreeMap<&str, usize> = + std::collections::BTreeMap::new(); for w in &report.warnings { - warn!("ui_script({}/{file}): {w}", self.name); + let shape = warning_shape(w); + let seen = per_shape.entry(shape).or_default(); + *seen += 1; + if *seen <= PER_SHAPE { + warn!("ui_script({}/{file}): {w}", self.name); + } // Retained for `/errors` too, here because only this caller knows the file. script.report_warning(&format!("{}/{file}: {w}", self.name)); } + for (shape, n) in per_shape.iter().filter(|(_, n)| **n > PER_SHAPE) { + warn!( + "ui_script({}/{file}): {} more like \"{shape}: …\" ({n} in all) — /errors \ + lists them", + self.name, + n - PER_SHAPE + ); + } // A file the document names and the provider lacks is a `.toc` line naming no file: // never a script error, as the reference logs `Couldn't open %s` and carries on, but a // `failures` entry, so our boot tests catch one in a `benilla.toc` document. @@ -201,6 +234,40 @@ impl Addon { } } +/// A document warning's shape: the message without its trailing identifier, which is what makes +/// sixteen "Couldn't find relative frame: InspectSlot" lines one cause. A message with no +/// `": "` is its own shape, which is how ten identical "unknown template" lines collapse too. +fn warning_shape(warning: &str) -> &str { + warning.rsplit_once(": ").map_or(warning, |(head, _)| head) +} + +/// Whether a `.toc` line is a file reference at all, asked only of a line that resolved to +/// nothing. A `.toc` is read line by line — `#` opens a metadata line and everything else is a +/// filename — so a wrapped `## Notes:` paragraph and the `--` comments many addons write become +/// entries, and the reference tries to open them, fails, and says nothing. Two of the player's own +/// addons do it: `ColorPickerPlus`'s Notes wrap onto two bare lines, `DifficultBulletinBoard` +/// heads each section with `-- API files`. +/// +/// Those are not missing files, so they are not load failures: counting them put phantom entries +/// in `/errors` and inflated the "N addon load failures" line at every login. A line that *is* +/// shaped like a file and absent stays a warning, because then the addon really is missing a +/// piece (`BigWigs`'s `Raids\Other\Nerubian.lua`). +pub(crate) fn is_file_entry(entry: &str) -> bool { + let name = entry.trim(); + !name.starts_with("--") + && name + .rsplit(['/', '\\']) + .next() + .unwrap_or(name) + .rsplit_once('.') + .is_some_and(|(stem, ext)| { + !stem.is_empty() + && ["lua", "xml", "toc"] + .iter() + .any(|k| ext.eq_ignore_ascii_case(k)) + }) +} + /// Whether a manifest entry is a Lua chunk: by extension, case-insensitively, with `\` a /// separator like `/` (a `.toc` is written for a Windows client). fn is_lua(entry: &str) -> bool { @@ -916,6 +983,53 @@ impl Walk { #[cfg(test)] mod tests { + /// A `.toc` line that cannot name a file is prose, and prose is not a missing file. The cases + /// are the real ones: `ColorPickerPlus`'s wrapped `## Notes:` and + /// `DifficultBulletinBoard`'s `--` section headers against the files they sit between. + #[test] + fn a_toc_line_that_is_prose_is_not_a_file_reference() { + for file in [ + "DifficultBulletinBoard.lua", + "env\\defaults.lua", + "libs\\AceOO-2.0\\AceOO-2.0.lua", + "FishingOutfitFrame.xml", + "Raids\\Other\\Nerubian.lua", + "Bindings.XML", + ] { + assert!(super::is_file_entry(file), "{file} names a file"); + } + for prose in [ + "-- API files", + "-- Environment files (static data, constants, exclusion rules)", + "standard Color Picker to provide text entry for colors (RGB and hex values),", + "copy and paste, and starting color and copied color swatches.", + "", + ".lua", + ] { + assert!(!super::is_file_entry(prose), "{prose:?} names no file"); + } + } + + /// The log's collapse key: one cause, however many frames it names. + #[test] + fn repeated_warnings_share_a_shape_and_distinct_causes_do_not() { + let shape = super::warning_shape; + let inspect = [ + "BCP_Inspect: Couldn't find relative frame: InspectHeadSlot", + "BCP_Inspect: Couldn't find relative frame: InspectFeetSlot", + ]; + assert_eq!(shape(inspect[0]), shape(inspect[1])); + assert_eq!( + shape(inspect[0]), + "BCP_Inspect: Couldn't find relative frame" + ); + // No trailing identifier: the whole message is the shape, so ten of them still collapse. + let template = "unknown template 'QuickHealMeleeDPSListEntryTemplate' referenced by \ + inherits; skipping"; + assert_eq!(shape(template), template); + assert_ne!(shape(template), shape(inspect[0])); + } + /// NTFS `$I30` order, not byte order: case folds away and `_` (0x5F) sorts after `Z` (0x5A). #[test] fn the_walk_orders_names_the_way_ntfs_lists_a_directory() { diff --git a/crates/benilla-app/src/ui_script/shipped_xml_tests.rs b/crates/benilla-app/src/ui_script/shipped_xml_tests.rs index d3586669..2f8b74f3 100644 --- a/crates/benilla-app/src/ui_script/shipped_xml_tests.rs +++ b/crates/benilla-app/src/ui_script/shipped_xml_tests.rs @@ -477,7 +477,8 @@ fn every_shipped_text_attribute_answers_against_the_real_global_strings() { let s = benilla_ui::script::UiScript::new().expect("VM"); s.run(&String::from_utf8_lossy(&src)).expect("runs clean"); - // The loader's key-shape predicate, restated because it is private. + // The key shape, deliberately broader than the loader's runtime predicate (which ignores a + // miss with no underscore, a label rather than a key): this sweep is over our own files. let key_shaped = |v: &str| { v.len() >= 2 && v.chars().any(|c| c.is_ascii_uppercase()) @@ -524,7 +525,11 @@ fn every_shipped_text_attribute_answers_against_the_real_global_strings() { /// (`GlobalStrings.lua:898`). The `text=` half is the sweep above. #[test] fn no_shipped_script_sets_a_global_string_key_as_display_text() { - /// `loader::is_global_string_key`'s predicate, restated because it is private. + /// The key shape, deliberately **broader** than `loader::is_global_string_key`: that one gates + /// a runtime warning and so ignores a miss with no underscore (`text="OK"` is a label an addon + /// author typed). Here every all-caps word counts, because this sweep is over our OWN shipped + /// files, where `text="DELETE"` must resolve and a literal that collides with a real key is a + /// bug either way. Restated rather than shared, for that reason and because it is private. fn key_shaped(s: &str) -> bool { s.len() >= 2 && s.chars().any(|c| c.is_ascii_uppercase()) diff --git a/crates/benilla-ui/src/loader/mod.rs b/crates/benilla-ui/src/loader/mod.rs index 44b65bbf..aacd1f31 100644 --- a/crates/benilla-ui/src/loader/mod.rs +++ b/crates/benilla-ui/src/loader/mod.rs @@ -790,10 +790,21 @@ pub(super) fn children_named_any<'a>( .filter(move |c| tags.iter().any(|t| c.tag.eq_ignore_ascii_case(t))) } -/// Whether a `text=` value is shaped like a GlobalStrings key: `SCREAMING_SNAKE` with a letter, two -/// characters or more (a one-character `text="X"` is a glyph). +/// Whether a `text=` value that resolved to nothing is shaped like a GlobalStrings key worth +/// warning about: `SCREAMING_SNAKE` **with an underscore**, a letter, and two characters or more. +/// +/// The underscore is the whole discrimination, and it is about the miss rather than the shape. A +/// key that exists never reaches here ([`Loader::resolve_text`] returns its value first), so every +/// candidate is already a string no `GlobalStrings.lua` defines — and a single all-caps word that +/// no locale defines is a label the author typed, not a key they got wrong. One session of the +/// player's own addons produced 42 such lines from `OK`, `UP`, `DOWN`, `MT`, `TPS`, `TOP`, +/// `BOTTOM`, `WIM` and `TWA`, every one a literal the reference also shows as typed. With the +/// underscore required, what is left is the shape that cannot be anything but a key: +/// `ATSW_ALLREAGENTLISTFRAME_CHN`, an addon's own locale row missing from the locale it shipped, +/// which does render as its own key name on screen. fn is_global_string_key(s: &str) -> bool { s.len() >= 2 + && s.contains('_') && s.chars().any(|c| c.is_ascii_uppercase()) && s.chars() .all(|c| c.is_ascii_uppercase() || c.is_ascii_digit() || c == '_') diff --git a/crates/benilla-ui/src/loader/tests.rs b/crates/benilla-ui/src/loader/tests.rs index eb013a7f..44448bc8 100644 --- a/crates/benilla-ui/src/loader/tests.rs +++ b/crates/benilla-ui/src/loader/tests.rs @@ -2052,11 +2052,35 @@ mod loader_tests { } #[test] - fn key_shape_is_screaming_snake_of_two_or_more() { - for yes in ["DELETE", "EXIT_GAME", "CHARACTER_POINTS1_COLON", "AB", "A1"] { + fn key_shape_is_screaming_snake_with_an_underscore() { + for yes in [ + "EXIT_GAME", + "CHARACTER_POINTS1_COLON", + "ATSW_ALLREAGENTLISTFRAME_CHN", + "A_1", + ] { assert!(is_global_string_key(yes), "{yes} is key-shaped"); } - for no in ["X", "", "Send Mail", "No results found.", "Okay", "1", "12"] { + // The last nine are the real labels one session of the player's addons warned about: + // uppercase, keyless, and exactly what the author meant to show. + for no in [ + "X", + "", + "Send Mail", + "No results found.", + "Okay", + "1", + "12", + "OK", + "UP", + "DOWN", + "MT", + "TPS", + "TOP", + "BOTTOM", + "WIM", + "TWA", + ] { assert!(!is_global_string_key(no), "{no} is not key-shaped"); } }