diff --git a/crates/benilla-app/src/ui_script/addons.rs b/crates/benilla-app/src/ui_script/addons.rs index 493c95f6..664be124 100644 --- a/crates/benilla-app/src/ui_script/addons.rs +++ b/crates/benilla-app/src/ui_script/addons.rs @@ -2722,6 +2722,76 @@ mod tests { let _ = std::fs::remove_dir_all(home.parent().unwrap()); } + /// A saved variable the addon never assigned writes **no line**, and a `NAME = nil` line a + /// previous build already wrote is dropped on the way in. + /// + /// Both halves of one bug. An addon's saved file runs *after* its own Lua, so a `NAME = nil` + /// line deletes the default the addon set at file scope — and because the global is then nil + /// at logout, the next write puts the line back, which makes one bad write permanent. grom, + /// 2026-11-05: *"I can't even get number 2 to load right now."* Advanced Trade Skill Window 2 + /// declares 24 saved variables; 19 of them stood at `nil` in his file, including the three its + /// event handlers index unguarded (`ATSW_Profession`, `ATSW_Bags`, `ATSW_Bank`), each assigned + /// `{}` at file scope and each nil by the time the first handler ran. + #[test] + fn a_never_assigned_saved_variable_writes_no_line_and_a_nil_line_is_dropped_on_load() { + let _l = crate::local_state::test_env::ENV_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let _c = crate::local_state::test_env::EnvGuard::unset("WOW_CAPTURE"); + let (home, _h) = hermetic_root("nilsaved"); + // `Untouched` is declared and never assigned; `Defaulted` is ATSW2's shape — a table set + // at file scope and read by the addon's own code. + write_addon( + &home, + "Nilly", + "## Interface: 11200\n## SavedVariables: NillyDefaulted, NillyUntouched\nnilly.lua\n", + &[("nilly.lua", "NillyDefaulted = { seeded = true }\n")], + ); + let id = ("Realm".to_string(), "Char".to_string()); + + let mut script = UiScript::new().unwrap(); + script.set_screen_size(1024.0, 768.0); + let failures = load_third_party(&mut script, Some(&id), &[], true); + assert!(failures.is_empty(), "{failures:?}"); + crate::ui_script::shutdown_ui_state(&mut script, Some(&id), true); + + let path = crate::local_state::addon_saved_account_dir() + .expect("an account saved dir") + .join("Nilly.lua"); + let written = std::fs::read_to_string(&path).expect("the file was written"); + assert!( + written.contains("NillyDefaulted = {"), + "the table the addon set is saved:\n{written}" + ); + assert!( + !written.contains("NillyUntouched"), + "a nil global writes nothing at all — a `= nil` line would delete the addon's own \ + default on the next load:\n{written}" + ); + + // Now the migration: a file an older build poisoned still has to load harmlessly. + std::fs::write( + &path, + "NillyDefaulted = nil\nNillyUntouched = nil\nNillyOther = nil\n", + ) + .unwrap(); + let mut next = UiScript::new().unwrap(); + next.set_screen_size(1024.0, 768.0); + let failures = load_third_party(&mut next, Some(&id), &[], true); + assert!(failures.is_empty(), "{failures:?}"); + assert_eq!( + next.eval::("return NillyDefaulted ~= nil and NillyDefaulted.seeded") + .ok(), + Some(true), + "the addon's file-scope default survives a poisoned file" + ); + assert!( + next.eval::("return NillyOther == nil").unwrap(), + "a name the addon does not declare is left exactly as the file has it" + ); + let _ = std::fs::remove_dir_all(home.parent().unwrap()); + } + /// The reference writer's grammar (`0x7043f0`/`0x704480`): `NAME = value`, bracketed keys, TAB /// indent, a trailing comma on every entry, a bracketed key for a list entry too (`[1] = 1,`), /// and the file split by scope. Asserted as bytes, not by reloading. diff --git a/crates/benilla-app/src/ui_script/lifecycle.rs b/crates/benilla-app/src/ui_script/lifecycle.rs index b08f6cce..76649f59 100644 --- a/crates/benilla-app/src/ui_script/lifecycle.rs +++ b/crates/benilla-app/src/ui_script/lifecycle.rs @@ -794,9 +794,19 @@ pub(crate) fn load_ingame_ui_on_world_entry(world: &mut World) { } // The realm before the load, since addons read `GetRealmName()` at file scope; the roster // carries the realm-list entry this session connected to. + // + // A capture never logs in and so has no roster, and `$WOW_CAPTURE_SAVED=-` is + // already the way a capture is pointed at a player's own saved variables — so it names the + // realm here too. Without it the verb answered `""` while the saved file it had just loaded + // was keyed on `"C'Thun"`, and an addon that indexes `[realm][player]` at file scope then + // behaves under capture in a way it never does in play: `AdvancedTradeSkillWindow2.lua:296` + // is `ATSW_Profession[realm][player] = nil`, which with an empty realm and an unknown player + // raises `table index is nil` and takes the rest of a 6,000-line chunk with it. That is a + // broken instrument, not a broken addon — it reads exactly like the defect being measured. let realm = world .get_resource::() .and_then(|r| r.realm.as_ref().map(|r| r.name.clone())) + .or_else(|| crate::local_state::capture_saved_identity().map(|(realm, _)| realm)) .unwrap_or_default(); script.set_realm_name(&realm); // The player too, before the addon walk. The record first: the reference's @@ -806,6 +816,22 @@ pub(crate) fn load_ingame_ui_on_world_entry(world: &mut World) { if let Some(record) = world .get_resource::() .and_then(record_from_roster) + .or_else(|| { + // The other half of the realm above, and for the same reason: `$WOW_CAPTURE_SAVED` + // names the **character** whose saved variables this capture has just loaded, so + // `UnitName("player")` has to answer that name or an addon's `[realm][player]` key + // looks up rows it was not handed. Race, class and sex are presence-only here, as + // `manifest.rs`'s own offline record documents; a fixture that seats a real one + // later overwrites this, and the addon walk has already run by then. + crate::local_state::capture_saved_identity().map(|(_, name)| { + benilla_ui::script::PlayerRecord { + name, + race: Some(("Human".into(), "Human".into())), + class: Some(("Warrior".into(), "WARRIOR".into())), + sex: 2, + } + }) + }) { script.set_player_record(record); } diff --git a/crates/benilla-ui/src/script/addon.rs b/crates/benilla-ui/src/script/addon.rs index 09fe45ee..608558cb 100644 --- a/crates/benilla-ui/src/script/addon.rs +++ b/crates/benilla-ui/src/script/addon.rs @@ -514,6 +514,41 @@ fn load_bindings(lua: &Lua, name: &str, read: &Reader) { /// Run the addon's saved-variables files, account then per-character (`0x51f4b5`, `0x51f53b`), /// as chunks in the shared globals with no `setfenv` (`0x704bc0`, `0x704ae0`), between its files /// and `ADDON_LOADED` so saved values win; a missing file is skipped (`0x51f4a9`, `0x51f530`). +/// Strip `NAME = nil` lines naming one of this addon's declared saved variables. +/// +/// An addon's saved file runs **after** its own Lua (`0x51f4b5`), so such a line cannot carry +/// information — a global is already nil in a fresh VM — and can only delete the default the +/// addon set moments earlier. Ours used to write them (see +/// [`crate::script::UiScript::saved_variables_bytes_for`], which no longer does), and once one +/// was written the state was self-sustaining: the nil it forced was what the next shutdown read +/// back and wrote again. This is the migration for a file an older build already poisoned. +/// +/// Deliberately narrow: the exact shape our own writer emitted, one assignment per line, the name +/// matched against the `## SavedVariables` set rather than any identifier, and nothing else about +/// the chunk touched. A player's hand-written `X = nil` of a name the addon never declared is +/// left alone, and so is anything inside a table literal, which is indented. +fn drop_nil_assignments(bytes: Vec, declared: &[String]) -> (Vec, usize) { + if declared.is_empty() || !bytes.windows(6).any(|w| w == b" = nil") { + return (bytes, 0); + } + let mut out = Vec::with_capacity(bytes.len()); + let mut dropped = 0usize; + for line in bytes.split_inclusive(|b| *b == b'\n') { + let trimmed = line.strip_suffix(b"\n").unwrap_or(line); + let trimmed = trimmed.strip_suffix(b"\r").unwrap_or(trimmed); + let name = std::str::from_utf8(trimmed) + .ok() + .and_then(|s| s.strip_suffix(" = nil")) + .filter(|n| declared.iter().any(|d| d == n)); + if name.is_some() { + dropped += 1; + continue; + } + out.extend_from_slice(line); + } + (out, dropped) +} + fn load_saved_variables(lua: &Lua, i: usize) { let (name, account, character, has_account, has_character) = { let model = lua.app_data_ref::().expect("model"); @@ -526,6 +561,15 @@ fn load_saved_variables(lua: &Lua, i: usize) { !a.saved_variables_per_character.is_empty(), ) }; + let declared_names: Vec = { + let model = lua.app_data_ref::().expect("model"); + let a = &model.addons[i]; + a.saved_variables + .iter() + .chain(a.saved_variables_per_character.iter()) + .cloned() + .collect() + }; for (dir, declared) in [(account, has_account), (character, has_character)] { if !declared { continue; // declares none: no file is read, even if one exists @@ -545,6 +589,17 @@ fn load_saved_variables(lua: &Lua, i: usize) { continue; } }; + let (bytes, dropped) = drop_nil_assignments(bytes, &declared_names); + if dropped > 0 { + // A migration, not a policy: see `drop_nil_assignments`. + lua.app_data_mut::() + .expect("model") + .record_warning(format!( + "{}: dropped {dropped} `NAME = nil` line(s) written by an older build — \ + they would have deleted the defaults this addon just set", + path.display() + )); + } if let Err(e) = run_chunk( lua, &bytes, diff --git a/crates/benilla-ui/src/script/saved.rs b/crates/benilla-ui/src/script/saved.rs index f052b1ee..974336a0 100644 --- a/crates/benilla-ui/src/script/saved.rs +++ b/crates/benilla-ui/src/script/saved.rs @@ -46,14 +46,33 @@ impl super::UiScript { /// Serialize every registered global as `NAME = value` lines, in bytes, since a Lua string is /// bytes and the reference writes them raw. A value with no literal (a function, userdata, /// `inf`, `NaN`) is skipped with a warning where the reference drops it silently; `nil` is - /// written, as the reference records an untouched toggle. + /// written, as the reference records an untouched toggle. This is the **flat** file, read + /// before FrameXML runs, so a `NAME = nil` line and an absent one mean the same thing there. pub fn saved_variables_bytes(&self) -> Vec { - self.saved_variables_bytes_for(&self.saved_variable_names()) + self.serialize_saved(&self.saved_variable_names(), true) } /// [`UiScript::saved_variables_bytes`] over an explicit name list, an addon's own - /// `## SavedVariables` set. + /// `## SavedVariables` set — **and a nil global writes no line at all**, which is the one + /// difference from the flat file. + /// + /// An addon's file is executed *after* its own Lua has run (`0x51f4b5`, "after the files set + /// their defaults"), so here the two are not the same thing: a `NAME = nil` line does not + /// record "untouched", it **deletes the default the addon just set**. A session that wrote + /// one poisons every session after it, because the nil it forces is then what the next + /// shutdown reads back and writes again. + /// + /// That is not a hypothetical. grom, 2026-11-05: *"I can't even get number 2 to load right + /// now"* — `benilla-config/saved/AdvancedTradeSkillWindow2.lua` held `nil` for 19 of its 24 + /// names, including `ATSW_Profession` (set at `AdvancedTradeSkillWindow2.lua:294`), + /// `ATSW_Bags` (`:5929`) and `ATSW_Bank` (`:5986`) — all three assigned at file scope and all + /// three `nil` by the time the first event handler read them, which is the whole of that + /// addon's reported breakage. pub fn saved_variables_bytes_for(&self, names: &[String]) -> Vec { + self.serialize_saved(names, false) + } + + fn serialize_saved(&self, names: &[String], write_nil: bool) -> Vec { let mut out = Vec::new(); let mut unwritable = Vec::new(); // A widget writes as its global name, and only the model can turn a `T[0]` id into one. @@ -74,6 +93,9 @@ impl super::UiScript { continue; } }; + if !write_nil && matches!(value, Value::Nil) { + continue; + } let mut seen = HashSet::new(); match serialize(&value, 1, &mut seen, &name_of) { Some(text) => {