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

Five stock options checkboxes were inert, and GetCVar raised where it should answer

Browse Source 
Two product fixes behind the last three red UI tests.

**`GetCVar`/`GetCVarDefault` no longer raise on a nameless ask.** The realm's own options window
depends on it: the checkbox loop calls `GetCVar(value.cvar)` for every row in its table
(`UIOptionsFrame.lua:265`), and the rows driven by a saved variable carry no `cvar` at all — so a
nil reaches the verb every time the window loads. The reference reads the argument with
`lua_tostring`, which answers NULL for anything but a string or a number, and a NULL name matches
no row, so the push is nil. `SetCVar` is deliberately left raising, and that is not a guess:
`0x488c10` checks `lua_isstring(1)` and falls into
`luaL_error("Usage: SetCVar(\"cvar\", value [, \"scriptCvar\")")`, with `0x6f4940` being `luaL_error`
itself (`where` + `pushvfstring` + `concat 2` + `lua_error`). A write with no name is a mistake; a
read with no name is a miss.

**The action-bar block now loads above the stock interface-options window**, which is the inverse
of 1.12's order and has to be. Stock patched the five multibar rows of `UIOptionsFrameCheckButtons`
from `MultiActionBarFrame_OnLoad`, under a comment calling itself a "Hack to get around load order
dependencies" — that hack is what let the window load first. The realm gutted the function
(`MultiActionBars.lua:7-12`) and wrote the readers into the table literal instead
(`UIOptionsFrame.lua:58-62`, `func = MultiBar1_IsVisible`), where they capture whatever the globals
are at the window's own load. Loaded above the bars, all five captured nil, those five checkboxes
read and wrote nothing, and `UIOptionsFrame_Save` fell through to `SetCVar(nil, ...)` and raised —
so pressing Okay abandoned the rest of the save. `reference_ui`'s shadowing gate is what kept this
honest: the first arrangement I tried let the stock window overwrite our own `UpdateNameplates`.

Also: the stock video-options window is gone as 1.12 knew it. The realm rewrote `OptionsFrame.xml`
into a searchable category panel — `OptionsFrameCategoryList*`, two scroll frames, a search box,
twenty result rows — keeping only the frame, its header and the three footer buttons. Gone with the
old page: `OptionsFrameDisplay`, `WorldAppearance`, `Brightness`, `PixelShaders`, `Miscellaneous`,
the three resolution/refresh/multisample dropdowns, `OptionsFrameSlider1..9` and
`OptionsFrameCheckButton1..18`. Worth stating rather than asserting around: **pfUI's options-video
skin walks those names and finds none of them**, so on this realm it skins nothing there.

And `MULTI_ACTION_BAR_STAND_INS` is retired — it existed to pre-create the rows that hack needed,
and nothing indexes the table before `UIOptionsFrame.lua` declares it.

All seven gates green. `ui_script` is 1113 green, 0 red.

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-31 15:25:36 -06:00
co-authored by Claude Opus 5
parent c5544a7ea3
commit 3d6306f828
4 changed files with 85 additions and 60 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/benilla-app/assets/ui/benilla.toc
+22 -11
View File
@@ -98,8 +98,28 @@ Interface\FrameXML\OptionsFrame.xml
# are the callers, and this client is the only thing that loads them.
OptionsFrameAdapters.lua
Interface\FrameXML\SoundOptionsFrame.xml
# Inherits `OptionsCheckButtonTemplate`, and declares `UIOptionsFrameCheckButtons`, which
# MultiActionBars.lua writes at load.
# The action bars, lifted above the stock options window on purpose. Stock 1.12 patched the five
# multibar rows of `UIOptionsFrameCheckButtons` from `MultiActionBarFrame_OnLoad`, under a comment
# calling itself a "Hack to get around load order dependencies", so the window could load first.
# The realm gutted that function (`MultiActionBars.lua:7-12`) and wrote the readers straight into
# the table literal (`UIOptionsFrame.lua:58-62`, `func = MultiBar1_IsVisible` and friends), which
# captures whatever those globals are at the window's own load. Loaded below the bars they capture
# nil, the five rows fall through `UIOptionsFrame_Save`'s arms to `SetCVar(nil, ...)`, and the
# reference's `SetCVar` raises on a non-string name — `0x488c10` checks `lua_isstring(1)` and takes
# `luaL_error("Usage: SetCVar(\"cvar\", value [, \"scriptCvar\")")` — so pressing Okay abandoned
# the rest of the save. The realm's own toc never faces this: it drops the stock options windows
# entirely, and this client is the only thing that loads them.
# `MainMenuBar`, `MainMenuBarArtFrame` and `MainMenuExpBar`, which later files read at load.
Interface\FrameXML\MainMenuBar.xml
Interface\FrameXML\Cooldown.xml
# `ActionButtonTemplate`, which ActionBarFrame.xml's `ActionBarButtonTemplate` inherits.
Interface\FrameXML\ActionButtonTemplate.xml
Interface\FrameXML\ActionBarFrame.xml
# The four optional bars, action slots 25-72. All four parent to `UIParent`.
Interface\FrameXML\MultiActionBars.xml
# Declares `UIOptionsFrameCheckButtons`, whose multibar rows the block above arms.
Interface\FrameXML\UIOptionsFrame.xml
# StaticPopup.lua starts `StaticPopupDialogs` empty: a file that adds a dialog at load follows it.
Interface\FrameXML\StaticPopup.xml
@@ -111,15 +131,6 @@ KeyBindingsPage.xml
# Ours: `BenillaOptionsFrame`; its rows read the option globals the stock windows above set.
OptionsFrame.xml
# `MainMenuBar`, `MainMenuBarArtFrame` and `MainMenuExpBar`, which later files read at load.
Interface\FrameXML\MainMenuBar.xml
Interface\FrameXML\Cooldown.xml
# `ActionButtonTemplate`, which ActionBarFrame.xml's `ActionBarButtonTemplate` inherits.
Interface\FrameXML\ActionButtonTemplate.xml
Interface\FrameXML\ActionBarFrame.xml
# The four optional bars, action slots 25-72.
Interface\FrameXML\MultiActionBars.xml
Interface\FrameXML\PetActionBarFrame.xml
# `UpdateMicroButtons` reads CharacterFrame, SpellBookFrame, QuestLogFrame, GameMenuFrame, the
# three stock options windows, FriendsFrame, WorldMapFrame, HelpFrame and KeyRingButton
crates/benilla-app/src/ui_script/shipped_xml_tests.rs
+37 -20
View File
@@ -1315,7 +1315,16 @@ fn the_stock_interface_options_window_loads_hidden_and_ours_is_still_the_players
"the stock window must never be on screen"
);
// The load-order proof: the five rows `MultiActionBars.lua:10` sets fields on.
// The load-order proof, the other way round from 1.12's. Stock patched these five rows from
// `MultiActionBarFrame_OnLoad`, under a comment calling itself a "Hack to get around load
// order dependencies", which let the options window load **first**. The realm gutted that
// function (`MultiActionBars.lua:7-12`) and wrote the readers into the table literal instead
// (`UIOptionsFrame.lua:58-62`), where they capture whatever the globals are at the window's own
// load — so the window's `benilla.toc` row has to sit **below** the bars'. Below them it does:
// all five rows carry their readers. Above them all five capture nil, the rows fall through
// `UIOptionsFrame_Save`'s arms to `SetCVar(nil, ...)`, and that raises — `0x488c10` checks
// `lua_isstring(1)` and takes `luaL_error("Usage: SetCVar(…)")` (`0x6f4940` is `luaL_error`:
// `where` + `pushvfstring` + `concat 2` + `lua_error`) — abandoning the rest of the save.
let rows: Vec<String> = s
.eval::<Vec<String>>(
"local out = {} \
@@ -1330,8 +1339,7 @@ fn the_stock_interface_options_window_loads_hidden_and_ours_is_still_the_players
assert_eq!(
rows.len(),
5,
"MultiActionBars.lua:10 writes five rows into UIOptionsFrameCheckButtons at ITS load, so \
the options window's manifest row must sit above the bars' — got {rows:?}"
"all five multibar rows must carry the readers `MultiActionBars.lua` defines — got {rows:?}"
);
// Ours is still the window the ESC menu opens.
@@ -1509,27 +1517,34 @@ fn the_stock_video_options_window_loads_hidden_and_owns_its_own_name() {
// The stock frame, by the children only it declares.
for name in [
// The realm rewrote this window end to end. What is left of 1.12's video page is the
// frame, its header and the three footer buttons; everything else is new — a category
// list, a scroll frame, a search box and twenty result rows. Gone with the old page:
// `OptionsFrameDisplay`, `OptionsFrameWorldAppearance`, `OptionsFrameBrightness`,
// `OptionsFramePixelShaders`, `OptionsFrameMiscellaneous`, the three resolution/refresh/
// multisample dropdowns, `OptionsFrameSlider1..9` and `OptionsFrameCheckButton1..18`.
//
// That is a compatibility fact worth stating rather than asserting around: **pfUI's
// options-video skin walks the old names and finds none of them**, so on this realm it
// skins nothing there. Not ours to fix, and not a defect of ours either.
"OptionsFrameHeader",
"OptionsFrameDisplay",
"OptionsFrameWorldAppearance",
"OptionsFrameBrightness",
"OptionsFramePixelShaders",
"OptionsFrameMiscellaneous",
"OptionsFrameResolutionDropDown",
"OptionsFrameRefreshDropDown",
"OptionsFrameMultiSampleDropDown",
"OptionsFrameOkay",
"OptionsFrameCancel",
"OptionsFrameDefaults",
"OptionsFrameSlider1",
"OptionsFrameSlider9",
"OptionsFrameCheckButton1",
"OptionsFrameCheckButton18",
"OptionsFrameCategoryList",
// Nested inside the list, so `$parent` is the list and not the window.
"OptionsFrameCategoryListCategory1",
"OptionsFrameCategoryListCategory18",
"OptionsFrameCategoryListScrollFrame",
"OptionsFrameContentScrollFrame",
"OptionsFrameSearchBox",
"OptionsFrameSearchResultsResult1",
"OptionsFrameSearchResultsResult20",
] {
assert!(
s.eval::<bool>(&format!("return getglobal({name:?}) ~= nil"))
.unwrap(),
"{name} — pfUI's options-video skin walks every one of these"
"{name} — the realm's own options window publishes this"
);
}
assert!(
@@ -1610,12 +1625,14 @@ fn the_stock_video_options_window_loads_hidden_and_owns_its_own_name() {
rates.is_empty(),
"the reference answers ZERO values, never a lone 0: {rates:?}"
);
// The stock refresh dropdown this used to leave empty-but-enabled is gone with the rest of
// 1.12's video page (see the name list above), so there is no `OptionsFrameRefreshDropDown`
// to ask. The verb's shape is the part that still matters, and it is asserted here: zero
// values, never a lone 0.
assert!(
s.eval::<bool>("return OptionsFrameRefreshDropDownButton:IsEnabled() == 1")
s.eval::<bool>("return OptionsFrameRefreshDropDown == nil")
.unwrap(),
"…so the dropdown is left empty but ENABLED — the greying branch is the driver-quirk one, \
and inventing a 0 to reach it would be inventing a value the binary never produces \
(`IsEnabled` answers a NUMBER — `0x7800b0`)"
"the realm's rewritten window declares no refresh dropdown"
);
// The optional index argument is tolerated.
assert!(
crates/benilla-app/src/ui_script/test_ui.rs
-27
View File
@@ -255,16 +255,6 @@ fn load_entry(s: &UiScript, entry: &str, strict_templates: bool, no_warnings: bo
let doc = benilla_ui::framexml::parse(&benilla_ui::source::decode(&bytes))
.unwrap_or_else(|e| panic!("{entry}: {e}"));
let provider = |req: &str| -> Option<Vec<u8>> { read(req) };
// Seated before the load: `MultiActionBarFrame_OnLoad` indexes `UIOptionsFrameCheckButtons`
// during this load walk.
if path
.rsplit('/')
.next()
.is_some_and(|l| l.eq_ignore_ascii_case("MultiActionBars.xml"))
{
s.run(MULTI_ACTION_BAR_STAND_INS)
.expect("the multibar stand-ins");
}
// Our options window's Graphics rows read stock `OptionsFrameSliders` (`OptionsFrame.lua`) at
// OnLoad; the manifest loads it one seat above ours, so a kit seats it before our file.
if path
@@ -500,23 +490,6 @@ pub(super) const UIOPTIONS_STAND_INS: &str = r#"
UpdatePartyMemberBackground = UpdatePartyMemberBackground or function() end
"#;
/// What a kit owes stock `MultiActionBars.xml`: its OnLoad (`MultiActionBars.lua:10`) writes five
/// rows into `UIOptionsFrameCheckButtons`, which `UIOptionsFrame.xml` declares eighteen toc rows
/// earlier; `or {}` keeps the real table when that file loaded first.
pub(super) const MULTI_ACTION_BAR_STAND_INS: &str = r#"
UIOptionsFrameCheckButtons = UIOptionsFrameCheckButtons or {}
-- The five ROWS it writes into, not just the table: the reference's hack assigns
-- `UIOptionsFrameCheckButtons["SHOW_MULTIBAR1_TEXT"].setFunc`, which needs the row to exist.
-- Empty rows on purpose — the real ones carry `index = 33..36, 40`, and a stand-in that
-- restated those numbers would be a transcription of the reference's table in a test helper,
-- which `OptionsFrame.xml` does not carry either. Any kit that reads an index has the
-- real window loaded and therefore the real table.
for _, key in ipairs({ "SHOW_MULTIBAR1_TEXT", "SHOW_MULTIBAR2_TEXT", "SHOW_MULTIBAR3_TEXT",
"SHOW_MULTIBAR4_TEXT", "ALWAYS_SHOW_MULTIBARS_TEXT" }) do
UIOptionsFrameCheckButtons[key] = UIOptionsFrameCheckButtons[key] or {}
end
"#;
/// What a kit owes the stock micro-button row: `UpdateMicroButtons`
/// (`MainMenuBarMicroButtons.lua:20-84`) reads ten panels, `KeyRingButton` and `IsBagOpen`
/// unguarded. Hidden, unnamed stand-ins are seated on its first call, not at load: a named frame's
crates/benilla-ui/src/script/cvars.rs
+26 -2
View File
@@ -321,6 +321,24 @@ pub(in crate::script) fn write_cvar(
true
}
/// The name a CVar **reader** was asked for, or `None` for an argument that names nothing.
///
/// A reader never raises on one. The realm's own options window depends on it: the checkbox loop
/// calls `GetCVar(value.cvar)` for every row in its table (`UIOptionsFrame.lua:265`), and the rows
/// driven by a saved variable instead carry no `cvar` at all — so a nil reaches the verb each time
/// the window loads, and on the reference the window loads. The reference reads the argument with
/// `lua_tostring`, which answers NULL for anything but a string or a number, and a NULL name
/// matches no row, so the push is nil. `SetCVar` is the other half and does raise its `Usage:`
/// line, because a *write* with no name is a mistake rather than a miss.
fn cvar_name(v: &Value) -> Option<String> {
match v {
Value::String(s) => s.to_str().ok().map(|s| s.to_owned()),
Value::Integer(n) => Some(n.to_string()),
Value::Number(n) => Some(n.to_string()),
_ => None,
}
}
fn warn_unknown(model: &mut Model, name: &str) {
let key = name.to_ascii_lowercase();
if model.cvars_warned.insert(key) {
@@ -385,7 +403,10 @@ pub(super) fn install(lua: &Lua) -> mlua::Result<()> {
lua.globals().set(
"GetCVar",
lua.create_function(|lua, name: String| {
lua.create_function(|lua, name: Value| {
let Some(name) = cvar_name(&name) else {
return Ok(Value::Nil);
};
let mut model = lua.app_data_mut::<Model>().expect("model app_data");
match model.cvars.get(&name.to_ascii_lowercase()) {
Some(slot) => Ok(Value::String(lua.create_string(&slot.value)?)),
@@ -398,7 +419,10 @@ pub(super) fn install(lua: &Lua) -> mlua::Result<()> {
)?;
lua.globals().set(
"GetCVarDefault",
lua.create_function(|lua, name: String| {
lua.create_function(|lua, name: Value| {
let Some(name) = cvar_name(&name) else {
return Ok(Value::Nil);
};
let mut model = lua.app_data_mut::<Model>().expect("model app_data");
match model.cvars.get(&name.to_ascii_lowercase()) {
Some(slot) => Ok(Value::String(lua.create_string(&slot.default)?)),