From 76e022ba78c11788b2c5ca8de25a79fa16cb98ff Mon Sep 17 00:00:00 2001 From: grom Date: Thu, 29 Oct 2026 15:33:50 -0600 Subject: [PATCH] Two real macro-window bugs, and three expectations the realm had moved on from MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The macro window's four reds were four separate things, two of them bugs a player meets. **Every macro made through the icon chooser had a blank icon.** This realm rewrote `Blizzard_MacroUI` to keep bare upper-cased icon names in its own `MACRO_ICON_FILENAMES` — `UpdateMacroIconFilenames` strips `INTERFACE\ICONS\` off `GetMacroIconInfo` — and `MacroPopupOkayButton_OnClick` hands that name straight to `CreateMacro`. `icon_path` took any string as a finished path, so `GetMacroInfo` answered `ABILITY_AMBUSH` and `MacroFrame_Update`'s `macroIcon:SetTexture(texture)` resolved nothing. The engine composes the prefix itself: `Interface\Icons\` is at `0x44ca64`, inside `UIMacros.cpp`'s own string block between the `DeleteMacro`/`GetMacroInfo`/`GetNumMacros`/`CreateMacro` registration names and `MACRONODE`, which is what says the macro code owns it. A number is still a chooser index, and a string that already carries a separator is still a path. **Selecting a macro with no icon raised and took the whole details pane down.** `Blizzard_MacroUI.lua:84-85` feeds `GetMacroInfo`'s texture to `SetTexture` and to `strupper` with no guard, so a nil answered there is `bad argument #1 to 'strupper' (string expected, got nil)` inside `MacroFrame_Update` — no editor, no body box, no character counter. The reference cannot be answering nil, or its own UI would raise the same way: a macro with no icon of its own now reads as the client's own `INV_Misc_QuestionMark` (`0x447fe4`, same UI block), which is also the name the realm's chooser puts in row 1. The other three were stale readings of a file the realm has rewritten, each checked against the chain copy rather than guessed: - the character counter is `MacroFrameCharLimitText:SetText(this:GetNumLetters() .. "/" .. this:GetMaxLetters())` in the realm's own XML — "21/510", with no `MACROFRAME_CHAR_LIMIT` and no "Characters Used" tail; - the chooser shows `GetNumMacroIcons() + 1` rows, because `UpdateMacroIconFilenames` inserts `INV_MISC_QUESTIONMARK` at index 1 ahead of anything the engine offers, so every list index is one past the engine's; - both tabs call `PanelTemplates_TabResize(0)` with no cap and read `text="GENERAL"`/`text="CHARACTER"`, where stock passed -15, capped tab 2 at 150 and formatted the player's name into it. That test was *about* the name, so it now checks the row is independent of it instead. 195 failures from 203, no regressions (`comm -13` over the sorted names). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn --- .../benilla-app/src/ui_script/macro_tests.rs | 94 +++++++++++++------ crates/benilla-ui/src/script/macros.rs | 84 ++++++++++++++++- 2 files changed, 144 insertions(+), 34 deletions(-) diff --git a/crates/benilla-app/src/ui_script/macro_tests.rs b/crates/benilla-app/src/ui_script/macro_tests.rs index 16973dea..f33f286b 100644 --- a/crates/benilla-app/src/ui_script/macro_tests.rs +++ b/crates/benilla-app/src/ui_script/macro_tests.rs @@ -119,7 +119,12 @@ fn new_then_pick_an_icon_then_okay_creates_the_macro() { ); s.run(r#"MacroPopupEditBox:SetText("Ambush")"#).unwrap(); - s.run("MacroPopupButton1:Click()").unwrap(); + // Button **2**, not 1: this realm's `UpdateMacroIconFilenames` inserts + // `INV_MISC_QUESTIONMARK` at index 1 before it walks the spellbook and `GetMacroIconInfo`, + // so the pushed list starts at 2. Each button carries its list index as its `SetID`, so the + // click selects `ABILITY_AMBUSH` — the name the realm's UI keeps, upper-cased with + // `INTERFACE\ICONS\` stripped off. + s.run("MacroPopupButton2:Click()").unwrap(); assert!( s.eval::("return MacroPopupOkayButton:IsEnabled() ~= 0") .unwrap(), @@ -138,7 +143,13 @@ fn new_then_pick_an_icon_then_okay_creates_the_macro() { .eval::<(String, String)>("local n, t = GetMacroInfo(1) return n, t") .unwrap(); assert_eq!(name, "Ambush"); - assert_eq!(tex, "Interface\\Icons\\Ability_Ambush"); + assert_eq!( + tex, "Interface\\Icons\\ABILITY_AMBUSH", + "`MacroPopupOkayButton_OnClick` hands `CreateMacro` the bare name its own list holds, and \ + the engine composes `Interface\\Icons\\` onto it (`0x44ca64`, in `UIMacros.cpp`'s string \ + block) — the case is the UI's, and texture lookup is case-insensitive. Storing the bare \ + name verbatim left every macro made here with a blank icon." + ); assert!( !s.eval::("return MacroPopupFrame:IsVisible()") .unwrap(), @@ -169,10 +180,16 @@ fn typing_a_body_and_closing_the_window_commits_it() { .unwrap(), "OnTextChanged marks the window dirty" ); + // The realm's own `Blizzard_MacroUI.xml` writes this counter itself — + // `MacroFrameCharLimitText:SetText(this:GetNumLetters() .. "/" .. this:GetMaxLetters())` in + // `MacroFrameText`'s `OnTextChanged` — and never reads `MACROFRAME_CHAR_LIMIT`, so there is no + // "Characters Used" tail and the cap is the `letters="510"` its own EditBox declares. (Our + // `install_macro_letters_patch` raises both for an install whose FrameXML still says 255; on + // this chain it has nothing left to do.) assert_eq!( s.eval::("return MacroFrameCharLimitText:GetText()") .unwrap(), - "21/255 Characters Used" + "21/510" ); s.run(r#"HideUIPanel(MacroFrame)"#).unwrap(); @@ -314,8 +331,17 @@ fn the_icon_chooser_shows_the_pushed_list_and_hides_its_tail() { s.run("MacroNewButton:Click()").unwrap(); no_errors(&s, "popup"); + // The engine's list is the three the app pushed; the chooser's is **four**, because this + // realm's `UpdateMacroIconFilenames` inserts `INV_MISC_QUESTIONMARK` at index 1 before it + // walks the spellbook and `GetMacroIconInfo`. So the shown count is `GetNumMacroIcons() + 1` + // and every list index is one past the engine's. assert_eq!(s.eval::("return GetNumMacroIcons()").unwrap(), 3); - for i in 1..=3 { + assert_eq!( + s.eval::("return MACRO_ICON_FILENAMES[1]").unwrap(), + "INV_MISC_QUESTIONMARK", + "the realm's own first row, ahead of anything the engine offers" + ); + for i in 1..=4 { assert!( s.eval::(&format!("return MacroPopupButton{i}:IsVisible()")) .unwrap(), @@ -323,7 +349,7 @@ fn the_icon_chooser_shows_the_pushed_list_and_hides_its_tail() { ); } assert!( - !s.eval::("return MacroPopupButton4:IsVisible()") + !s.eval::("return MacroPopupButton5:IsVisible()") .unwrap(), "past the end of the list the button hides — not a blank square" ); @@ -339,21 +365,34 @@ fn the_icon_chooser_shows_the_pushed_list_and_hides_its_tail() { .unwrap(), "the picked icon is checked" ); + // The name the realm's list holds for that row, which is what `CreateMacro` is handed: the + // second pushed path, stripped and upper-cased. + assert_eq!( + s.eval::("return MacroPopupFrame.selectedIconTexture") + .unwrap(), + "ABILITY_BACKSTAB" + ); } -/// Both tabs pass `PanelTemplates_TabResize` a -15 padding and tab 2, whose label carries the -/// player name, a 150 cap; a short and a long name run both arms of the cap. +/// Both tabs size themselves off their own label, with **no padding and no cap**: this realm's +/// `Blizzard_MacroUI.xml` calls `PanelTemplates_TabResize(0)` in each tab's `OnLoad` (`:502`, +/// `:524`), where stock 1.12 passed `-15` and gave tab 2 a 150-unit cap. +/// +/// Its labels changed with it — `text="GENERAL"` and `text="CHARACTER"`, global-string keys, where +/// stock formatted `CHARACTER_SPECIFIC_MACROS` with `UnitName("player")`. So the tab row no longer +/// grows with the player's name at all, which is what this test used to be about: it ran a short +/// and a long name through the cap's two arms. Both names are kept, now as a check that the row is +/// *independent* of them. #[test] fn the_two_tabs_fit_inside_the_window() { let _data = benilla_formats::wow_data_or_skip!(); - /// `sideWidths`: twice `TabButtonTemplate`'s 16-unit end slice (`UIPanelTemplates.lua:41`). + /// `sideWidths`: twice `TabButtonTemplate`'s 16-unit end slice (`UIPanelTemplates.lua:42`). const SIDES: f64 = 32.0; - /// The stock padding on both tabs (`Blizzard_MacroUI.xml:488`, `:511`). - const PAD: f64 = -15.0; - /// The stock cap on tab 2 (`Blizzard_MacroUI.xml:511`). - const CAP: f64 = 150.0; + /// What this realm passes (`Blizzard_MacroUI.xml:502`, `:524`). + const PAD: f64 = 0.0; - for (name, cap_should_bind) in [("Probe", false), ("Bartholomewthelongnamed", true)] { + let mut rows = Vec::new(); + for name in ["Probe", "Bartholomewthelongnamed"] { let mut s = harness_named(name); s.run("ShowMacroFrame()").unwrap(); select_first(&s); @@ -371,25 +410,15 @@ fn the_two_tabs_fit_inside_the_window() { assert_eq!( w1, label1 + PAD + SIDES, - "tab 1 is text − 15 + the end slices" + "tab 1 is its text plus the slices" ); - let uncapped = label2 + PAD + SIDES; - if cap_should_bind { - assert_eq!( - w2, - CAP + PAD + SIDES, - "the reference's 150 cap binds on a long name" - ); - assert!( - uncapped > CAP + PAD + SIDES, - "…and the name really was over it" - ); - } else { - assert_eq!(w2, uncapped, "a short name is under the cap"); - } + assert_eq!(w2, label2 + PAD + SIDES, "tab 2 the same, with no cap"); assert!( - w2 <= CAP + PAD + SIDES, - "the clamp may tighten the reference's cap, never loosen it" + !s.eval::(&format!( + "return string.find(MacroFrameTab2Text:GetText(), '{name}') ~= nil" + )) + .unwrap(), + "tab 2's label carries the player name, which this realm's `text=\"CHARACTER\"` does not" ); // The tab ends inside the plate the player sees, not merely inside the 384-unit frame. @@ -405,7 +434,12 @@ fn the_two_tabs_fit_inside_the_window() { "the tab row ({left1} + {w1} + {w2}) runs off the 384-wide window" ); no_errors(&s, "tab fit"); + rows.push((w1, w2)); } + assert_eq!( + rows[0], rows[1], + "the row must not move with the player's name any more" + ); } #[test] diff --git a/crates/benilla-ui/src/script/macros.rs b/crates/benilla-ui/src/script/macros.rs index 135e1d30..726f7b4a 100644 --- a/crates/benilla-ui/src/script/macros.rs +++ b/crates/benilla-ui/src/script/macros.rs @@ -316,9 +316,18 @@ pub(super) fn install(lua: &Lua) -> mlua::Result<()> { let Some(m) = model.macros.get(index) else { return Ok(MultiValue::from_vec(vec![Value::Nil])); }; + // Never nil. A macro with no icon reads as the engine's own default, because this + // realm's `Blizzard_MacroUI.lua:84-85` takes `GetMacroInfo`'s texture straight into + // `SetTexture` **and** `strupper(texture)` with no guard — selecting a macro whose + // icon was never chosen raised `bad argument #1 to 'strupper' (string expected, got + // nil)` inside `MacroFrame_Update` and left the editor's whole details pane down. The + // reference cannot be answering nil there, or its own UI would raise the same way; the + // client carries `INV_Misc_QuestionMark` (`0x447fe4`) in the same UI block as the + // macro API's `Interface\Icons\` prefix, and the realm's own chooser list puts that + // very name in row 1. let texture = match &m.texture { Some(t) => Value::String(lua.create_string(t)?), - None => Value::Nil, + None => Value::String(lua.create_string(DEFAULT_ICON)?), }; Ok(MultiValue::from_vec(vec![ Value::String(lua.create_string(&m.name)?), @@ -366,7 +375,8 @@ pub(super) fn install(lua: &Lua) -> mlua::Result<()> { })?, )?; - // CreateMacro stores `iconIndex`, an index into the chooser list, as its path. + // CreateMacro's icon argument goes through `icon_path`: an index into the chooser list, or + // an icon name the engine composes its `Interface\Icons\` prefix onto. g.set( "CreateMacro", lua.create_function(|lua, args: MultiValue| { @@ -432,10 +442,43 @@ pub(super) fn install(lua: &Lua) -> mlua::Result<()> { Ok(()) } -/// Resolve a chooser-list index to its texture path; a string is taken as a path already. +/// The icon prefix the macro code composes a bare name against: the literal at `0x44ca64`, in +/// `UIMacros.cpp`'s own string block — it sits between the `DeleteMacro`/`GetMacroInfo`/ +/// `GetNumMacros`/`CreateMacro` registration names and `MACRONODE`, which is what says the macro +/// code owns it rather than some other icon consumer. +const ICON_PREFIX: &str = "Interface\\Icons\\"; + +/// What `GetMacroInfo` answers for a macro with no icon of its own: the client's own +/// `INV_Misc_QuestionMark` (`0x447fe4`, the same UI block), composed against [`ICON_PREFIX`]. +const DEFAULT_ICON: &str = "Interface\\Icons\\INV_Misc_QuestionMark"; + +/// Resolve `CreateMacro`/`EditMacro`'s icon argument to the texture path `GetMacroInfo` answers. +/// +/// A number is an index into the chooser list (`GetMacroIconInfo`'s order), which already holds +/// full paths. A **string is an icon name**, and the engine composes [`ICON_PREFIX`] onto it: the +/// reference saves the icon by name (`MACRO %d "%s" %s`, `0x44cb60`) and hands a path back, which +/// is the only reading that makes this realm's own macro UI work — +/// `Blizzard_MacroUI.lua`'s `MacroPopupOkayButton_OnClick` passes +/// `MACRO_ICON_FILENAMES[selectedIcon]`, a bare upper-cased name its own +/// `UpdateMacroIconFilenames` built by stripping `INTERFACE\ICONS\` off `GetMacroIconInfo`, and +/// `MacroFrame_Update` then does `macroIcon:SetTexture(texture)` straight off `GetMacroInfo`. +/// Storing the bare name verbatim gave every macro made through that popup a blank icon in the +/// list, because `SetTexture("ABILITY_AMBUSH")` resolves to nothing. +/// +/// A string that already carries a separator is a path and is kept whole, so an addon that passes +/// `GetMacroIconInfo`'s own answer back is not double-prefixed. fn icon_path(model: &Model, v: Option<&Value>) -> Option { match v { - Some(Value::String(s)) => Some(s.to_string_lossy()), + Some(Value::String(s)) => { + let name = s.to_string_lossy(); + Some( + if name.is_empty() || name.contains('\\') || name.contains('/') { + name + } else { + format!("{ICON_PREFIX}{name}") + }, + ) + } Some(v) => { let i = v.as_integer()?; let i = usize::try_from(i).ok()?.checked_sub(1)?; @@ -450,6 +493,39 @@ mod tests { use super::*; use crate::script::UiScript; + /// The icon argument's three readings, which this realm's own macro UI depends on: an index + /// into the chooser list, a bare name the engine prefixes, and a path left whole. + #[test] + fn the_icon_argument_is_an_index_a_name_or_a_path() { + let s = seeded(); + s.run(r#"CreateMacro("ByIndex", 1, "")"#).unwrap(); + s.run(r#"CreateMacro("ByName", "ABILITY_AMBUSH", "")"#) + .unwrap(); + s.run(r#"CreateMacro("ByPath", "Interface\\Icons\\Ability_BackStab", "")"#) + .unwrap(); + let tex = |i: u32| { + s.eval::(&format!("local _, t = GetMacroInfo({i}) return t")) + .unwrap() + }; + assert_eq!( + tex(1), + "Interface\\Icons\\Ability_Ambush", + "the list's own path" + ); + assert_eq!( + tex(2), + "Interface\\Icons\\ABILITY_AMBUSH", + "a bare name is composed against the engine's prefix, case as handed — \ + `SetTexture` resolves case-insensitively and the reference stores the name it was \ + given" + ); + assert_eq!( + tex(3), + "Interface\\Icons\\Ability_BackStab", + "a path is not prefixed twice" + ); + } + fn seeded() -> UiScript { let mut s = UiScript::new().unwrap(); s.set_macro_icons(vec![