diff --git a/crates/benilla-app/src/ui_loot_roll.rs b/crates/benilla-app/src/ui_loot_roll.rs index 0068c28d..b24a21b1 100644 --- a/crates/benilla-app/src/ui_loot_roll.rs +++ b/crates/benilla-app/src/ui_loot_roll.rs @@ -9,6 +9,15 @@ //! within [`TEMPLATE_HOLD_MAX_MS`], opens the frame unresolved, because a frame that never opens //! cannot even be passed on; the reference's callback fires on success only. //! +//! **That last deviation buys nothing on this realm, and the file is why.** Its +//! `GroupLootFrame_OnShow` now opens `if ( not name ) then this:Hide(); return; end` +//! (`LootFrame.lua:382-385`), where stock 1.12 painted straight through a nil name — so a popup +//! opened unresolved closes itself on the same frame and the player never sees it, let alone +//! passes on it. Nothing to work around here: the hold above is what actually saves the roll, and +//! the fallback is now only a guarantee that we never *withhold* the event. Kept, because a +//! future realm file may paint through again, and because the alternative is a roll that is never +//! announced at all. +//! //! At most [`MAX_OPEN`] rolls are open at once (`0x61b430` tests `[0xc4dda8]` against 4). A roll //! whose template is in finds the slots full and waits, deferred, in arrival order; every close //! (our vote `0x61bf46`, a win `0x61bc6b`, all-passed `0x61b74c`) walks the list oldest first and diff --git a/crates/benilla-app/src/ui_script/group_loot_tests.rs b/crates/benilla-app/src/ui_script/group_loot_tests.rs index 62c75032..21dad5b0 100644 --- a/crates/benilla-app/src/ui_script/group_loot_tests.rs +++ b/crates/benilla-app/src/ui_script/group_loot_tests.rs @@ -330,9 +330,28 @@ fn cancel_loot_roll_hides_only_that_frame() { .unwrap()); } -/// A roll with its item template in flight opens without error, with no name and no icon. +/// A roll whose item template is still in flight **closes its own popup**, and that is this +/// realm's own rule, not ours: its `GroupLootFrame_OnShow` opens with +/// +/// ```lua +/// local texture, name, count, quality, bindOnPickUp = GetLootRollItemInfo(this.rollID); +/// if ( not name ) then +/// this:Hide(); +/// return; +/// end +/// ``` +/// +/// (`LootFrame.lua:380-385`), where stock 1.12 painted straight through a nil name. So an +/// unresolved roll shows nothing at all — no blank plate, no placeholder icon — and the slot it +/// claimed is free again for the next roll. +/// +/// `ui_loot_roll`'s own deviation is what keeps that from losing a roll: it holds +/// `START_LOOT_ROLL` until the item template lands (the reference fires from the arrival +/// callback, `0x61b460`), and its `TEMPLATE_HOLD_MAX_MS` fallback — "open the frame unresolved, +/// because a frame that never opens cannot even be passed on" — is the one case this realm's +/// `OnShow` overrules. Nothing to fix on our side; the file decides. #[test] -fn in_flight_roll_opens_without_a_name_or_icon() { +fn an_in_flight_roll_closes_its_own_popup() { let _data = benilla_formats::wow_data_or_skip!(); let mut s = setup(); load_group_loot(&s); @@ -347,23 +366,31 @@ fn in_flight_roll_opens_without_a_name_or_icon() { "an in-flight roll (all-nil GetLootRollItemInfo) must not error: {:?}", s.errors() ); - assert!(s - .eval::("return GroupLootFrame1:IsVisible()") - .unwrap()); - assert_eq!(s.eval::("return GroupLootFrame1.rollID").unwrap(), 9); - // `SetText(nil)` (`LootFrame.lua:274`) leaves no text, not "nil". - assert_eq!( - s.eval::>("return GroupLootFrame1Name:GetText()") + assert!( + !s.eval::("return GroupLootFrame1:IsVisible()") .unwrap(), - None, - "" + "the realm's OnShow hides a popup whose item it cannot name" + ); + // The rollID is still stamped — `GroupLootFrame_OpenNewFrame` wrote it before `Show()` — and + // the hide leaves the frame free, so the next roll takes this same slot. + assert_eq!(s.eval::("return GroupLootFrame1.rollID").unwrap(), 9); + s.fire_event( + "START_LOOT_ROLL", + vec![ScriptValue::Int(7), ScriptValue::Int(42_000)], + ); + assert!(s.errors().is_empty(), "script errors: {:?}", s.errors()); + assert!( + s.eval::("return GroupLootFrame1:IsVisible()") + .unwrap(), + "the slot the unresolved roll let go of is the one the next roll claims" + ); + assert_eq!( + s.eval::("return GroupLootFrame1Name:GetText()") + .unwrap(), + "Staff of Jordan" ); - assert!(!s - .eval::("return GroupLootFrame1Decoration:IsShown()") - .unwrap()); - // No placeholder icon: `GroupLootFrame_OnShow` passes the nil texture to `SetTexture` - // (`LootFrame.lua:273`), which leaves the icon empty. + // And nothing painted a placeholder icon on the way through. s.resolve(); let has_fallback_icon = s.extract().iter().any(|q| { matches!(&q.content, QuadContent::Texture { path: Some(p), .. } @@ -376,9 +403,12 @@ fn in_flight_roll_opens_without_a_name_or_icon() { assert!(s.errors().is_empty(), "script errors: {:?}", s.errors()); } -/// The stock popup paints only in `OnShow`, so a `START_LOOT_ROLL` that beats its roll to the model -/// stays blank. On a cache miss the reference fires it from the item-template arrival callback -/// (`0x61b460`, armed by `0x61b310`), and `feed_loot_rolls` holds it the same way. +/// The popup paints only in `OnShow`, so a `START_LOOT_ROLL` that beats its roll to the model has +/// nothing to paint — and on this realm it does not even open: `GroupLootFrame_OnShow` hides on a +/// nil name (`LootFrame.lua:382-385`), and an unknown roll answers the reference's whole miss tail +/// (`0x4c31a3`). Pushing the snapshot afterwards changes nothing, because nothing re-enters +/// `OnShow` — there is no repaint path in the file, which is exactly why `ui_loot_roll` holds the +/// event for the item template instead of firing and hoping. #[test] fn nothing_repaints_a_frame_that_opened_before_its_snapshot() { let _data = benilla_formats::wow_data_or_skip!(); @@ -405,9 +435,11 @@ fn nothing_repaints_a_frame_that_opened_before_its_snapshot() { vec![ScriptValue::Int(7), ScriptValue::Int(42_000)], ); assert!(s.errors().is_empty(), "script errors: {:?}", s.errors()); - assert!(s - .eval::("return GroupLootFrame1:IsVisible()") - .unwrap()); + assert!( + !s.eval::("return GroupLootFrame1:IsVisible()") + .unwrap(), + "an unknown roll answers a nil name, and this realm's OnShow hides on it" + ); assert_eq!( s.eval::>("return GroupLootFrame1Name:GetText()") .unwrap(), @@ -418,7 +450,12 @@ fn nothing_repaints_a_frame_that_opened_before_its_snapshot() { s.set_loot_rolls(rolls()); assert!(s.errors().is_empty(), "script errors: {:?}", s.errors()); - // No repaint: nothing re-enters `OnShow`, as in 1.12. + // No repaint, and no re-open: nothing re-enters `OnShow`, as in 1.12. + assert!( + !s.eval::("return GroupLootFrame1:IsVisible()") + .unwrap(), + "a snapshot arriving late does not re-open the popup either" + ); assert_eq!( s.eval::>("return GroupLootFrame1Name:GetText()") .unwrap(), @@ -440,11 +477,6 @@ fn nothing_repaints_a_frame_that_opened_before_its_snapshot() { "and no icon arrives either — same missing repaint, same one cause" ); assert!(s.errors().is_empty(), "script errors: {:?}", s.errors()); - assert_eq!( - s.eval::>("return GroupLootFrame1Name:GetText()") - .unwrap(), - unpainted - ); } /// `UIPARENT_MANAGED_FRAME_POSITIONS["GroupLootFrame1"]` (baseY 60, bottomEither 42, pet 42; @@ -614,8 +646,14 @@ fn the_roll_template_carries_the_reference_name_and_the_parts_addons_reach_for() .unwrap()); } -/// The stock `GroupLootFrame_OnShow` reads `ITEM_QUALITY_COLORS[quality].r` with no fallback -/// (`LootFrame.lua:275-276`), so an unresolved or unknown roll must still answer a quality. +/// `GroupLootFrame_OnShow` reads `ITEM_QUALITY_COLORS[quality].r` with no fallback +/// (`LootFrame.lua:398-399`), so an unresolved or unknown roll must still answer a quality — and +/// `GetLootRollItemInfo`'s miss tail is the reference's `nil, nil, 1.0, 1.0, nil` (`0x4c31a3`), +/// quality **1**, not `GetLootSlotInfo`'s -1. +/// +/// On this realm the nil-name hide (`:382`) returns before that line is reached, so the quality is +/// checked at the verb rather than through a paint: the protection is for the *stock* file and for +/// any addon that calls `GetLootRollItemInfo` itself, both of which index that table unguarded. #[test] fn the_stock_group_loot_frame_survives_an_in_flight_roll() { let _data = benilla_formats::wow_data_or_skip!(); @@ -638,21 +676,28 @@ fn the_stock_group_loot_frame_survives_an_in_flight_roll() { s.errors() ); } + // Both hid themselves on the nil name, so neither is left on screen. assert!(s - .eval::("return GroupLootFrame1:IsVisible() and GroupLootFrame2:IsVisible()") + .eval::("return not GroupLootFrame1:IsVisible() and not GroupLootFrame2:IsVisible()") .unwrap()); - // The name is painted Common, set at `LootFrame.lua:276`. - let painted: (f64, f64, f64) = s - .eval("local r, g, b = GroupLootFrame1Name:GetTextColor()\nreturn r, g, b") - .unwrap(); - let common: (f64, f64, f64) = s - .eval("local r, g, b = GetItemQualityColor(1)\nreturn r, g, b") - .unwrap(); - assert!( - (painted.0 - common.0).abs() < 1e-6 - && (painted.1 - common.1).abs() < 1e-6 - && (painted.2 - common.2).abs() < 1e-6, - "the stock roll popup paints the miss tail's Common, got {painted:?} want {common:?}" - ); + // The miss tail itself: quality 1, which is what the stock file would have indexed. + for roll in [9, 99] { + assert_eq!( + s.eval::(&format!( + "local t, n, c, q = GetLootRollItemInfo({roll}) return q" + )) + .unwrap(), + 1, + "roll {roll}'s miss tail must answer a quality `ITEM_QUALITY_COLORS` has" + ); + assert!( + s.eval::(&format!( + "local t, n, c, q = GetLootRollItemInfo({roll}) \ + return ITEM_QUALITY_COLORS[q] ~= nil" + )) + .unwrap(), + "roll {roll}: the quality must index the colour table the file reads unguarded" + ); + } } diff --git a/crates/benilla-app/src/ui_script/test_ui.rs b/crates/benilla-app/src/ui_script/test_ui.rs index 757ff2fb..44b1394c 100644 --- a/crates/benilla-app/src/ui_script/test_ui.rs +++ b/crates/benilla-app/src/ui_script/test_ui.rs @@ -386,16 +386,31 @@ pub(super) const UIPARENT_STAND_INS: &str = r#" -- FCF_SetWindowName concatenating its nil GetName (l.693), FCF_SaveDock handing -- SetChatWindowDocked window 0 (l.1284), and FCF_UpdateButtonSide doing arithmetic on its -- unresolvable GetRight (l.1019). That cost 136 of this crate's tests. - local chat_windows = { ChatFrame1 = true, ChatFrame2 = true } + -- A managed row's own KEY is never seated here, and the exception the chat windows used + -- to be is now the rule. The pass reads every row through `frame = _G[index]; if ( frame )` + -- (UIParent.lua:1962), so an absent one is skipped and costs nothing; what *is* reached + -- unguarded is the custom-positioning tail, and every name that tail touches + -- (`MultiBarBottomLeft` l.2041, `ShapeshiftBar*` l.2043-2057, `PetActionBarFrame` l.2065, + -- `BattlefieldMinimapTab` l.2036…) is in the explicit list above, where it can be read + -- and reasoned about. + -- + -- Seating a key is not merely unnecessary, it is destructive, because a frame's publish to + -- `_G` is non-overwriting (`0x701bd0`): a stand-in seated at this pass — which runs AT + -- LOAD, from `ShapeshiftBar_OnLoad` (`BonusActionBarFrame.lua:109` -> `:157`) — shadows + -- the real frame for the VM's life if its own file loads later, and the real one, properly + -- built and parented, becomes unreachable by name. That cost `ChatFrame1`/`ChatFrame2` + -- **137** of this crate's tests, and then `GroupLootFrame1` (a row at `UIParent.lua:1909`, + -- declared in `LootFrame.xml`, loaded after) **five more**: the real roll popups got no + -- ``, so no name, no icon, no bind-on-pickup decoration, and `CANCEL_LOOT_ROLL` + -- reached no registered frame. `FramerateLabel` and `TutorialFrameParent` are the same + -- shape waiting to happen. local named = {} - for name, row in pairs(UIPARENT_MANAGED_FRAME_POSITIONS) do - if not row.isVar and not chat_windows[name] then table.insert(named, name) end - -- …and every row's ANCHOR TARGET (`anchorTo`, l.1668), which is a different set: the - -- keys are the frames being MOVED, the targets are what they move relative to, and - -- several targets (`ActionButton1`, `MainMenuBarArtFrame`) are declared in files a - -- one-window kit never loads. An unresolvable name is a RAISE, so a target the kit is - -- missing aborts `UIParent_ManageFramePositions` mid-pass rather than anchoring to - -- the parent and carrying on. + for _, row in pairs(UIPARENT_MANAGED_FRAME_POSITIONS) do + -- A row's ANCHOR TARGET still seats, and it is a different set: the keys are the + -- frames being MOVED, the targets are what they move relative to, and several targets + -- (`ActionButton1`, `MainMenuBarArtFrame`) are declared in files a one-window kit + -- never loads. An unresolvable target is a RAISE at l.1668, which aborts the pass + -- mid-way rather than anchoring to the parent and carrying on. if row.anchorTo then table.insert(named, row.anchorTo) end end benilla_seat(named)