diff --git a/crates/benilla-app/assets/ui/AIOHandlers.lua b/crates/benilla-app/assets/ui/AIOHandlers.lua index be47c98c..80e64a69 100644 --- a/crates/benilla-app/assets/ui/AIOHandlers.lua +++ b/crates/benilla-app/assets/ui/AIOHandlers.lua @@ -11,17 +11,31 @@ -- only that benilla now loads AIO at all (for the calendar). So the blocks this client does not -- implement are registered as no-ops, which is what "no handler" was always supposed to mean. -- --- `OctoRadio`: the realm's radio is driven entirely from the client — a station click sends --- `.radio ` as a SAY command and the server answers with `SMSG_PLAY_MUSIC` --- (`OctoRadioPlaceholder.lua`'s own header says so, and that file contains no AIO call at all). --- Whatever this block carries is therefore supplementary to a feature that already works, so --- dropping it costs nothing visible. Logged once so a block that turns out to matter is still --- findable. +-- `IGNORED` is empty, and the shape around it is what matters. `AIO.RegisterEvent` *asserts* when +-- a name is already taken (`AIO.lua:736`) and AIO has no way to unregister, so a name claimed +-- here is claimed against every later registration — including the realm's own, if a client +-- update implements the block we were standing in for. That is not hypothetical: this file used to +-- claim `OctoRadio` on the evidence that `OctoRadioPlaceholder.lua` carried no AIO call at all, +-- and the 2026-10-23 update built the radio out into a file twice the size whose +-- `RegisterNowPlayingFeed` registers exactly that name at `VARIABLES_LOADED` — ten tests' worth of +-- raise, from a row that had simply gone stale. +-- +-- Hence both halves below. The claim waits for `PLAYER_LOGIN`, after the `VARIABLES_LOADED` where +-- FrameXML and addons register and before any block can arrive, and each name goes through +-- `pcall`, so a realm that implements one of these takes the name and this file steps aside +-- instead of raising. `reference_ui`'s tests fail a row the realm has implemented, so the next one +-- is caught in the gate rather than in a session. +local IGNORED = {} + if AIO and AIO.RegisterEvent and not AIO_SERVER then - local ignored = { "OctoRadio" } - for _, name in ipairs(ignored) do - AIO.RegisterEvent(name, function() - -- Nothing: the block is for a feature this client does not implement. - end) - end + local claim = CreateFrame("Frame", "BenillaAioShimFrame") + claim:RegisterEvent("PLAYER_LOGIN") + claim:SetScript("OnEvent", function() + for _, name in ipairs(IGNORED) do + -- Discarded on purpose: a failure means the name is taken, which is the good case. + pcall(AIO.RegisterEvent, name, function() + -- Nothing: the block is for a feature this client does not implement. + end) + end + end) end diff --git a/crates/benilla-app/src/ui_script/battlefield_tests.rs b/crates/benilla-app/src/ui_script/battlefield_tests.rs index 9b7b238a..72d331d8 100644 --- a/crates/benilla-app/src/ui_script/battlefield_tests.rs +++ b/crates/benilla-app/src/ui_script/battlefield_tests.rs @@ -207,12 +207,28 @@ fn the_list_window_opens_on_battlefields_show_and_joins_the_selection() { s.run("BattlefieldFrameJoinButton:Click()").unwrap(); assert_eq!(s.take_battlefield_join_requests(), vec![(9, false)]); + // `BATTLEFIELDS_CLOSED` does **not** close this list on OctoWoW, and that is the realm's own + // decision rather than a gap here. Its `BattlefieldFrame.lua` returns out of that handler for + // the raw-shown list (bug tracker #330 and its follow-up): the server stamps + // `SMSG_BATTLEFIELD_LIST`'s battlemaster guid with the *player's own*, so the client treats the + // window as an NPC conversation anchored where it opened and fires the close as soon as you + // step off that spot — on foot, on a taxi, or on a boat that carries you off it. The realm + // queues from anywhere by design, so there is no proximity rule left to honour. + // + // This assertion used to read the other way, against an August copy of that file in an archive + // that outranked the realm's own (`chain::demote_stale_snapshots`). It is pinned in the + // realm's direction now, and the close button below is what proves the list still closes. s.fire_event("BATTLEFIELDS_SHOW", vec![]); assert!(visible(&s, "BattlefieldFrame")); s.fire_event("BATTLEFIELDS_CLOSED", vec![]); + assert!( + visible(&s, "BattlefieldFrame"), + "the realm swallows the leash's event: stepping off the anchor must not shut the list" + ); + s.run("BattlefieldFrameCloseButton:Click()").unwrap(); assert!( !visible(&s, "BattlefieldFrame"), - "the leash's event hides it" + "…and its own close button still does close it (`BattlefieldFrame_OctoHide`)" ); } diff --git a/crates/benilla-app/src/ui_script/questlog_tests.rs b/crates/benilla-app/src/ui_script/questlog_tests.rs index d1d75f9c..c3a34684 100644 --- a/crates/benilla-app/src/ui_script/questlog_tests.rs +++ b/crates/benilla-app/src/ui_script/questlog_tests.rs @@ -772,10 +772,20 @@ fn empty_quest_log_hides_rows_and_disables_abandon() { .unwrap(), "no selection: the detail pane is hidden, header and all" ); + // Against the realm's own cap, not 1.12's 20: OctoWoW's `QuestLogFrame.lua` sets + // `MAX_QUESTLOG_QUESTS = 25` and drives slots 21-25 from its own poller, since those ride + // update-fields the client's descriptor table calls combat ratings and so never raise the + // quest-log event. Read from the global rather than pinned, so the next bump is data and not a + // failure; this read 20 while an August copy of that file outranked the realm's own + // (`chain::demote_stale_snapshots`). + let cap = s + .eval::("return MAX_QUESTLOG_QUESTS") + .expect("the realm's quest-log cap"); + assert!(cap >= 20, "the cap cannot be below 1.12's own: {cap}"); assert_eq!( s.eval::("return QuestLogQuestCount:GetText()") .unwrap(), - "Quests: |cffffffff0/20|r" + format!("Quests: |cffffffff0/{cap}|r") ); } diff --git a/crates/benilla-app/src/ui_script/reference_ui.rs b/crates/benilla-app/src/ui_script/reference_ui.rs index 482046c3..0f25a93b 100644 --- a/crates/benilla-app/src/ui_script/reference_ui.rs +++ b/crates/benilla-app/src/ui_script/reference_ui.rs @@ -484,6 +484,62 @@ mod tests { } } + /// Our AIO shim claims a block name against every later registration, the realm's own + /// included, because `AIO.RegisterEvent` asserts on a duplicate and AIO cannot unregister. So + /// a name in `IGNORED` must be one the realm's own interface does *not* register, or the row + /// has gone stale and the shim is standing in front of a feature that now exists. + /// + /// Exactly what happened on 2026-10-23: the row claimed `OctoRadio` on the evidence that the + /// radio file carried no AIO call, and the update built it out into one that registers that + /// name at `VARIABLES_LOADED`. The shim yields through `pcall` now rather than raising, so this + /// is about the row being wrong rather than fatal — but a wrong row still means a dead feature, + /// and this is where it gets noticed. + #[test] + fn no_aio_block_we_shim_is_one_the_realm_now_handles() { + let _data = benilla_formats::wow_data_or_skip!(); + let shim = super::super::content::read("AIOHandlers.lua").expect("our AIO shim"); + let names: Vec = shim + .lines() + .find(|l| l.trim_start().starts_with("local IGNORED")) + .map(|l| { + l.split('"') + .skip(1) + .step_by(2) + .map(str::to_string) + .collect() + }) + .expect("the IGNORED list, as one line of string literals"); + if names.is_empty() { + return; + } + let Some(rows) = super::realm_toc_rows() else { + return; + }; + let mut taken = Vec::new(); + for row in &rows { + let path = format!("{}{}", super::FRAMEXML_PREFIX, row.replace('/', "\\")); + let Some(bytes) = super::read(&path) else { + continue; + }; + let src = benilla_ui::source::decode(&bytes); + for name in &names { + // `AIO.AddHandlers("X", …)` and `AIO.RegisterEvent("X", …)` both claim the name. + if src.contains(&format!("AddHandlers(\"{name}\"")) + || src.contains(&format!("RegisterEvent(\"{name}\"")) + { + taken.push(format!("{name} (registered in {row})")); + } + } + } + assert!( + taken.is_empty(), + "AIOHandlers.lua stands in for AIO blocks the realm now handles itself \u{2014} drop \ + them from IGNORED, or the shim claims the name first and the real handler never \ + runs: {}", + taken.join(", ") + ); + } + /// A [`super::DECLINED`] row must still be a row the realm ships, or it is a note about a file /// nobody has any more — and the next reader would take it for a live decision. #[test] diff --git a/crates/benilla-app/src/ui_script/resolve_bench.rs b/crates/benilla-app/src/ui_script/resolve_bench.rs index 38a609ca..e00b6fab 100644 --- a/crates/benilla-app/src/ui_script/resolve_bench.rs +++ b/crates/benilla-app/src/ui_script/resolve_bench.rs @@ -223,6 +223,18 @@ fn an_authored_span_is_read_without_resolving_and_a_derived_one_is_not() { ); } +/// Frames in the player's own interface that redraw on a timer rather than on an event, which this +/// bench stands down before measuring. Each row says why the redraw is legitimate, so a row that +/// stops being true is a row to delete. +const POLLS_ON_A_TIMER: &[(&str, &str)] = &[( + "OctoQuestLogPoller", + "OctoWoW's own, in its `QuestLogFrame.lua`: every 0.2 s it calls `GetNumQuestLogEntries()` and \ + on a change redraws the log and the watch. Load-bearing on the reference client rather than a \ + convenience — quest slots 21-25 ride update-fields the descriptor table calls combat ratings, \ + so nothing raises the quest-log event for them and the poll is the only thing that notices an \ + accepted quest", +)]; + /// With nothing happening, the settled shipped UI costs zero gate walks per frame: this catches a /// shipped handler writing a layout input every frame. `WOW_LAYOUT_TOUCH_TRACE=:` names /// the writer on a live run (`docs/CONTRIBUTING.md`, "Running it unattended"). @@ -246,6 +258,31 @@ fn the_settled_shipped_ui_costs_no_gate_walk_on_a_quiet_frame() { ) .expect("the shipped UI's OnUpdate handlers must run under app_frame_ticked"); + // The realm's own periodic handlers are stood down for the measurement, by name and with the + // reason, because the question here is whether *our* shipped UI writes a layout input on a + // quiet frame. A handler that redraws on a timer is not that, and tolerating it by raising the + // threshold instead would blunt the only instrument that catches an every-frame writer: one of + // those costs a walk per frame where a 0.2 s poller costs one per twelve. + // + // Keeping the list explicit is the point. A realm handler that appears here and is not listed + // fails this test, which is how the next one gets read rather than absorbed. + for (frame, why) in POLLS_ON_A_TIMER { + let armed = s + .eval::(&format!( + "local f = getglobal(\"{frame}\") return f ~= nil and f:GetScript(\"OnUpdate\") ~= nil" + )) + .unwrap_or(false); + assert!( + armed, + "`{frame}` is listed as a timed redraw this test stands down, but the loaded \ + interface has no such frame with an OnUpdate — drop the row ({why})" + ); + s.run(&format!( + "getglobal(\"{frame}\"):SetScript(\"OnUpdate\", nil)" + )) + .expect("stand down a timed redraw"); + } + let before = s.layout_gate_walks(); for _ in 0..20 { app_frame_ticked(&mut s, 1.0 / 60.0); diff --git a/crates/benilla-formats/src/chain.rs b/crates/benilla-formats/src/chain.rs index 7efb175d..5077cb87 100644 --- a/crates/benilla-formats/src/chain.rs +++ b/crates/benilla-formats/src/chain.rs @@ -59,6 +59,78 @@ fn mount_order(dir_names: &[String]) -> Vec { order } +/// The realm's interface manifest, the file whose copies say which archives hold a stale snapshot +/// of the realm's own data ([`demote_stale_snapshots`]). +const REALM_TOC: &str = "Interface\\FrameXML\\FrameXML.toc"; + +/// The rows of a `FrameXML.toc`, lowercased with separators normalised, for comparing two copies as +/// sets. Deliberately not the real toc parser (that lives in `benilla-ui`, which this crate does not +/// depend on): membership is all this needs, and a row is a line that is neither blank nor a +/// comment. +fn toc_row_keys(bytes: &[u8]) -> std::collections::HashSet { + String::from_utf8_lossy(bytes) + .lines() + .map(|l| l.trim().trim_start_matches('\u{feff}')) + .filter(|l| !l.is_empty() && !l.starts_with('#')) + .map(|l| l.replace('/', "\\").to_ascii_lowercase()) + .collect() +} + +/// Deviation from the reference's mount law, and the only one here: an archive carrying a **strict +/// subset** of another archive's `FrameXML.toc` is a stale snapshot of the realm's own data, and is +/// mounted below every `patch-?.MPQ` instead of wherever its name sorted it. +/// +/// `mount_order` is the reference's and stays so: `patch-?.MPQ` by case-folded name, last wins, so +/// a letter patch outranks a digit patch. A realm that ships its interface in `patch-5.mpq` is +/// therefore shadowed by any `patch-A.mpq` beside it — which on grom's install is exactly what +/// happened. `patch-A.mpq` holds an August snapshot of the realm's data: an older `FrameXML.toc`, +/// `GlobalStrings.lua`, the `OctoLocale` files, `AIO.lua`, the calendar, and a `Spell.dbc` 20 KB +/// behind. Every one of them won, so the client ran August's interface and August's spells with no +/// way to tell. +/// +/// A toc that is a strict subset is what makes this decidable rather than a guess: it lists *fewer* +/// files than another copy and removes none, so it is an earlier edition of the same manifest, and +/// an archive holding an earlier edition of the realm's manifest holds earlier editions of the +/// realm's files. The demotion is to just below the patch set and no further, so anything only that +/// archive carries still resolves over `patch.MPQ` and the base archives — the point is to stop it +/// beating the realm's current patches, not to switch it off. An archive whose toc differs by +/// *removal* is a different manifest, not an older one, and is left exactly where its name puts it. +/// +/// `WOW_CHAIN_DEMOTE=0` keeps the reference's order unchanged, for comparing against it. +fn demote_stale_snapshots(order: &mut Vec, toc_of: impl Fn(&str) -> Option>) { + if std::env::var("WOW_CHAIN_DEMOTE").is_ok_and(|v| v == "0") { + return; + } + let tocs: Vec<(String, std::collections::HashSet)> = order + .iter() + .filter(|n| is_patch_glob_match(n)) + .filter_map(|n| toc_of(n).map(|b| (n.clone(), toc_row_keys(&b)))) + .filter(|(_, rows)| !rows.is_empty()) + .collect(); + // The richest copy among the patches is the realm's current manifest; anything strictly inside + // it is an earlier edition of it. + let Some((_, richest)) = tocs.iter().max_by_key(|(_, rows)| rows.len()) else { + return; + }; + let stale: Vec = tocs + .iter() + .filter(|(_, rows)| rows.len() < richest.len() && rows.is_subset(richest)) + .map(|(name, _)| name.clone()) + .collect(); + if stale.is_empty() { + return; + } + // Just below the patch set: above `patch.MPQ` and the base archives, below every `patch-?`. + let first_patch = order + .iter() + .position(|n| is_patch_glob_match(n)) + .unwrap_or(order.len()); + order.retain(|n| !stale.iter().any(|s| s == n)); + for (i, name) in stale.into_iter().enumerate() { + order.insert(first_patch + i, name); + } +} + impl Chain { /// Open a `Data` directory's archives in [`mount_order`], or a single `.MPQ` file. /// @@ -79,15 +151,34 @@ impl Chain { .collect(); // read_dir order is arbitrary; sort so case-variant ties resolve deterministically. names.sort(); - for name in mount_order(&names) { - let mpq = path.join(&name); - archives.push( - Archive::open(&mpq).with_context(|| format!("opening {}", mpq.display()))?, - ); - } - if archives.is_empty() { + let mut order = mount_order(&names); + if order.is_empty() { bail!("no known vanilla MPQs found in {}", path.display()); } + // Opened before the order is final, because the order depends on what they hold: + // `demote_stale_snapshots` reads one file out of each patch archive to decide. + let mut opened: Vec<(String, Option)> = Vec::with_capacity(order.len()); + for name in &order { + let mpq = path.join(name); + let archive = + Archive::open(&mpq).with_context(|| format!("opening {}", mpq.display()))?; + opened.push((name.clone(), Some(archive))); + } + demote_stale_snapshots(&mut order, |want| { + opened + .iter() + .find(|(name, _)| name == want) + .and_then(|(_, a)| a.as_ref()) + .filter(|a| a.contains(REALM_TOC) && !a.is_delete_marker(REALM_TOC)) + .and_then(|a| a.read_file(REALM_TOC).ok()) + }); + for name in &order { + let slot = opened + .iter_mut() + .find(|(n, a)| n == name && a.is_some()) + .expect("every ordered name was opened above"); + archives.push(slot.1.take().expect("checked by the find")); + } } else { archives.push( Archive::open(path).with_context(|| format!("opening MPQ {}", path.display()))?, @@ -284,6 +375,67 @@ mod tests { assert!(!is_patch_glob_match("mypatch-2.MPQ")); } + /// The demotion: an archive whose `FrameXML.toc` is a strict subset of another's is an earlier + /// edition of the realm's manifest, so it drops below the whole patch set — above `patch.MPQ` + /// and the base archives, so anything only it carries still resolves. + #[test] + fn a_stale_snapshot_drops_below_the_patch_set() { + let toc = |rows: &[&str]| rows.join("\n").into_bytes(); + let mut order = owned(&[ + "dbc.MPQ", + "patch.MPQ", + "patch-1.mpq", + "patch-5.mpq", + "patch-A.mpq", + "Patch-B.mpq", + "speech2.MPQ", + ]); + demote_stale_snapshots(&mut order, |name| match name { + // The realm's current manifest, and an August copy of it with three rows missing. + "patch-5.mpq" => Some(toc(&["A.lua", "B.lua", "C.lua", "D.lua"])), + "patch-A.mpq" => Some(toc(&["A.lua"])), + _ => None, + }); + assert_eq!( + order, + owned(&[ + "dbc.MPQ", + "patch.MPQ", + "patch-A.mpq", + "patch-1.mpq", + "patch-5.mpq", + "Patch-B.mpq", + "speech2.MPQ", + ]), + "below every patch-?, above patch.MPQ and the base archives" + ); + } + + /// A toc that *differs* rather than lags is a different manifest, not an earlier edition of + /// one, and nothing here can arbitrate between them — so the reference's order stands. + #[test] + fn a_toc_that_drops_a_row_is_left_where_its_name_puts_it() { + let toc = |rows: &[&str]| rows.join("\n").into_bytes(); + let before = owned(&["patch.MPQ", "patch-5.mpq", "patch-A.mpq"]); + let mut order = before.clone(); + demote_stale_snapshots(&mut order, |name| match name { + "patch-5.mpq" => Some(toc(&["A.lua", "B.lua", "C.lua"])), + // Fewer rows, but one the richest copy does not have: not a subset. + "patch-A.mpq" => Some(toc(&["A.lua", "Gone.lua"])), + _ => None, + }); + assert_eq!(order, before); + + // And the richest copy is never demoted against itself. + let mut order = before.clone(); + demote_stale_snapshots(&mut order, |name| match name { + "patch-5.mpq" => Some(toc(&["A.lua"])), + "patch-A.mpq" => Some(toc(&["A.lua"])), + _ => None, + }); + assert_eq!(order, before, "equal manifests are not stale ones"); + } + /// A letter patch mounts **above** a digit patch, because the reference sorts the glob's hits /// by case-folded name and walks them so the last wins. Pinned on its own because it is not a /// detail: a realm that ships its interface in `patch-5.mpq` has it shadowed by any @@ -299,6 +451,52 @@ mod tests { ); } + /// The invariant the demotion exists for, on whatever chain this machine has: the archive that + /// wins the realm's `FrameXML.toc` is the one holding the richest copy of it. Vacuous on an + /// install with a single copy, and the whole point on one with a stale snapshot mounted high. + /// Prints where the realm's most update-sensitive files resolve, so `--nocapture` after a + /// client update shows whether its archives are the ones being read. + #[test] + fn the_realms_current_data_wins_over_a_stale_snapshot() { + let Some(data) = crate::wow_data() else { + eprintln!("skipped: no client data"); + return; + }; + let chain = Chain::open(&data).expect("open the patch chain"); + let copies = chain.read_all(REALM_TOC); + if copies.is_empty() { + eprintln!("skipped: this chain carries no FrameXML.toc"); + return; + } + let rows = |bytes: &[u8]| toc_row_keys(bytes).len(); + let (winner_at, winner_bytes) = copies.last().expect("a copy"); + let richest = copies + .iter() + .max_by_key(|(_, bytes)| rows(bytes)) + .expect("a copy"); + for name in [ + REALM_TOC, + "Interface\\FrameXML\\GlobalStrings.lua", + "DBFilesClient\\Spell.dbc", + ] { + let at = chain.find_file_archive(name); + eprintln!( + "{name} resolves to {}", + at.map_or("nowhere".into(), |p| p.display().to_string()) + ); + } + assert_eq!( + rows(winner_bytes), + rows(&richest.1), + "the realm's toc is won by {} with {} rows while {} holds {} \u{2014} a stale snapshot \ + is still outranking the realm's own data", + winner_at.display(), + rows(winner_bytes), + richest.0.display(), + rows(&richest.1) + ); + } + /// [`Chain::read_all`]'s contract, on a real chain: the copies come back in ascending priority /// and the last of them is the one [`Chain::read`] answers with. #[test]