grom/benilla-octo
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Two real macro-window bugs, and three expectations the realm had moved on from

Browse Source 
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn
This commit is contained in:
grom
2026-10-29 15:33:50 -06:00
co-authored by Claude Opus 5
parent 48c27d45bf
commit 76e022ba78
2 changed files with 144 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/benilla-app/src/ui_script/macro_tests.rs
+64 -30
View File
@@ -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::<bool>("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::<bool>("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::<String>("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::<i64>("return GetNumMacroIcons()").unwrap(), 3);
for i in 1..=3 {
assert_eq!(
s.eval::<String>("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::<bool>(&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::<bool>("return MacroPopupButton4:IsVisible()")
!s.eval::<bool>("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::<String>("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::<bool>(&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]
crates/benilla-ui/src/script/macros.rs
+80 -4
View File
@@ -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<String> {
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::<String>(&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![