From 87bf5904ff2444953f4b0cd735b9cc4d0c1dc246 Mon Sep 17 00:00:00 2001 From: Sam Sung <51686767+samwhosung@users.noreply.github.com> Date: Mon, 12 Oct 2026 04:17:03 +0700 Subject: [PATCH] A region keeps its own name; only the name lookup is first-come (#944) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Region names lived in one first-wins name → id map that also served `GetName`, so a second region given a taken name answered `GetName()` with nil and was `` in its error strings. In 1.12.1 the name lives on the region: `SetName` (`0x76c650`) keeps the region's own copy at `[+0x98]`, freed by its destructor (`0x76c560`) and read back by `GetName`. Only `_G` is first-come (`0x701bd0`, `0x701c8a`). Each region now carries its name; `GetName` and the error strings read it directly, and the name map is a first-come lookup only, whose entry a free drops only when it holds that region. Tests: same name twice, `$parentText` under colliding parents, the second region's error string, a free leaving a same-named sibling's name. Fixes #939 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5.5 --- crates/benilla-ui/src/loader/widgets.rs | 6 +- crates/benilla-ui/src/script/loot.rs | 2 +- crates/benilla-ui/src/script/mod.rs | 2 +- crates/benilla-ui/src/script/model.rs | 8 +- .../src/script/object/events_regions.rs | 7 +- crates/benilla-ui/src/script/region.rs | 53 +++++--- .../src/script/tests/name_targets.rs | 119 ++++++++++++++++++ crates/benilla-ui/src/script/tests/regions.rs | 4 +- crates/benilla-ui/src/script/tooltip/mod.rs | 6 +- crates/benilla-ui/src/widget/mod.rs | 5 + 10 files changed, 177 insertions(+), 35 deletions(-) diff --git a/crates/benilla-ui/src/loader/widgets.rs b/crates/benilla-ui/src/loader/widgets.rs index 50456b61..b82ae91a 100644 --- a/crates/benilla-ui/src/loader/widgets.rs +++ b/crates/benilla-ui/src/loader/widgets.rs @@ -118,9 +118,9 @@ impl Loader<'_> { continue; // build_texture_child already reported why }; this.install_texture(wrapper, tag, ®ion, None, dbg); - // Freeing the displaced region took its name out of the registry `GetName()` - // reads, which a replacement of the same name takes back: the reference keeps the - // name on the region (`0x76c650`). `_G` keeps its first holder (`0x701c8a`). + // Freeing the displaced region took its entry out of the name lookup, which a + // replacement of the same name takes back. The replacement's `GetName()` is its + // own (`0x76c650`); `_G` keeps its first holder (`0x701c8a`). if let Some(rname) = raw.name().map(|n| framexml::resolve_name(n, self_name)) { crate::script::region::publish_region_name(this.lua(), &rname, ®ion); } diff --git a/crates/benilla-ui/src/script/loot.rs b/crates/benilla-ui/src/script/loot.rs index 9938fec4..3a6d7881 100644 --- a/crates/benilla-ui/src/script/loot.rs +++ b/crates/benilla-ui/src/script/loot.rs @@ -342,7 +342,7 @@ mod tests { /// The region data behind a named texture: its portrait key and file. fn portrait_of(s: &UiScript, name: &str) -> (Option, Option) { let model = s.model_ref(); - let id = model.region_names[name]; + let id = model.region_by_name[name]; let rh = *model.id_to_region.get(&id).expect("a region"); let data = &model.region_data[&rh]; (data.portrait_unit.clone(), data.texture.clone()) diff --git a/crates/benilla-ui/src/script/mod.rs b/crates/benilla-ui/src/script/mod.rs index 24232e20..b896a358 100644 --- a/crates/benilla-ui/src/script/mod.rs +++ b/crates/benilla-ui/src/script/mod.rs @@ -1198,7 +1198,7 @@ impl UiScript { }); } // Region leaves publish into their own name table, not the arena's. - let id = *model.region_names.get(name)?; + let id = *model.region_by_name.get(name)?; let h = *model.id_to_region.get(&id)?; model.arena.region(h).map(|r| match r.kind { crate::widget::RegionKind::Texture => "Texture", diff --git a/crates/benilla-ui/src/script/model.rs b/crates/benilla-ui/src/script/model.rs index fd865f95..35eac102 100644 --- a/crates/benilla-ui/src/script/model.rs +++ b/crates/benilla-ui/src/script/model.rs @@ -183,8 +183,10 @@ pub(crate) struct Model { pub(crate) frame_to_id: HashMap, pub(crate) id_to_region: IdMap, pub(crate) region_to_id: HashMap, - /// Region name to id, first wins: `SetPoint` anchors to a sibling region by name, after frames. - pub(crate) region_names: HashMap, + /// Region name to the first region that took it, for name lookups only (`SetPoint` anchoring + /// to a sibling by name, after frames): first come, as the `_G` entry is (`0x701bd0`). A + /// region's own name, for `GetName` and error strings, is [`crate::widget::Region::name`]. + pub(crate) region_by_name: HashMap, /// Which handlers each frame has; the closures themselves live in Lua's `REG_SCRIPTS` table. pub(crate) scripts: HashMap>, @@ -1126,7 +1128,7 @@ impl Model { frame_to_id: HashMap::new(), id_to_region: IdMap::default(), region_to_id: HashMap::new(), - region_names: HashMap::new(), + region_by_name: HashMap::new(), scripts: HashMap::new(), on_update_frames: Vec::new(), on_update_joins: 0, diff --git a/crates/benilla-ui/src/script/object/events_regions.rs b/crates/benilla-ui/src/script/object/events_regions.rs index 90b809db..9b3d0cc0 100644 --- a/crates/benilla-ui/src/script/object/events_regions.rs +++ b/crates/benilla-ui/src/script/object/events_regions.rs @@ -359,9 +359,12 @@ fn create_region( crate::framexml::resolve_name(&name, &super::parent_token_base(&model, Some(owner))) }; publish_global(lua, &name, &wrapper)?; - // First name wins, as for frames; a sibling's `SetPoint` can then name this region. + // The region keeps its own name; the lookup a sibling's `SetPoint` names it by stays + // first-come, as `_G` is. let mut model = lua.app_data_mut::().expect("model"); - model.region_names.entry(name).or_insert(id); + if let Some(rh) = model.id_to_region.get(&id).copied() { + crate::script::region::set_region_name(&mut model, rh, id, &name); + } } Ok(wrapper) } diff --git a/crates/benilla-ui/src/script/region.rs b/crates/benilla-ui/src/script/region.rs index c5f2a40d..a2807880 100644 --- a/crates/benilla-ui/src/script/region.rs +++ b/crates/benilla-ui/src/script/region.rs @@ -378,7 +378,7 @@ pub(super) fn install(lua: &Lua) -> mlua::Result<()> { "SetPortraitToTexture", lua.create_function(|lua, (name, path): (String, String)| { let mut model = lua.app_data_mut::().expect("model"); - let Some(id) = model.region_names.get(&name).copied() else { + let Some(id) = model.region_by_name.get(&name).copied() else { return Ok(()); }; let Some(rh) = model.id_to_region.get(&id).copied() else { @@ -632,25 +632,33 @@ fn install_region_methods(lua: &Lua) -> mlua::Result<()> { Ok(()) } -/// This region's global name, or `None` when anonymous: a scan of the region-name registry, the -/// one authority, linear in named regions. +/// This region's own name, or `None` when anonymous: the string on the region (`[+0x98]`, read by +/// `GetName` through vtable `+4`, `0x46ff70`), whether or not it holds the lookup entry. pub(super) fn region_name_of(model: &Model, id: u32) -> Option { - model - .region_names - .iter() - .find(|&(_, &v)| v == id) - .map(|(k, _)| k.clone()) + let rh = *model.id_to_region.get(&id)?; + model.arena.region(rh)?.name.clone() } -/// Publish a region's name into the registry a named `relativeTo` resolves through, first name -/// winning as for frames. For sub-textures a setter creates and the XML loader names later -/// (`SetThumbTexture`, `SetColorWheelTexture`); `CreateTexture(name)` publishes its own. +/// `CScriptRegion::SetName` (`0x76c650`): the region keeps its own copy of the (already expanded) +/// name, replacing the old one, and offers it to the name lookup, where the first holder stays +/// (`_G`'s `0x701bd0` writes only when the entry is empty, `0x701c8a`). +pub(crate) fn set_region_name(model: &mut Model, rh: RegionHandle, id: u32, name: &str) { + if let Some(region) = model.arena.region_mut(rh) { + region.name = Some(name.to_string()); + } + model.region_by_name.entry(name.to_string()).or_insert(id); +} + +/// Name a region the loader or a setter built nameless or under another name, as +/// [`set_region_name`]. For sub-textures a setter creates and the XML loader names later +/// (`SetThumbTexture`, `SetColorWheelTexture`); `CreateTexture(name)` names its own. pub(crate) fn publish_region_name(lua: &Lua, name: &str, region: &Table) { - let Ok(id) = super::object::decode_id(region) else { + let Ok(rh) = region_handle_of(lua, region) else { return; }; let mut model = lua.app_data_mut::().expect("model app_data"); - model.region_names.entry(name.to_string()).or_insert(id); + let id = model.region_id(rh); + set_region_name(&mut model, rh, id, name); } /// The type `GetObjectType()` answers and `IsObjectType` matches; `"Region"` for a dead handle. @@ -670,9 +678,16 @@ pub(super) fn region_type_name(model: &Model, rh: RegionHandle) -> &'static str pub(crate) fn free_region(model: &mut Model, rh: RegionHandle) { model.region_data.remove(&rh); model.region_resolved.remove(&rh); + let name = model.arena.region(rh).and_then(|r| r.name.clone()); if let Some(id) = model.region_to_id.remove(&rh) { model.id_to_region.remove(&id); - model.region_names.retain(|_, v| *v != id); + // The lookup entry goes only if this region is the one it holds; a later holder of the + // same name was never in it. + if let Some(name) = name { + if model.region_by_name.get(&name) == Some(&id) { + model.region_by_name.remove(&name); + } + } // Its node leaves the cached graph with its edges at the next resolve, and whatever // anchored to it re-resolves without it. model.name_layout_node(id); @@ -759,13 +774,11 @@ pub(crate) fn implicit_creation_anchor_lua(lua: &Lua, wrapper: &Table) -> mlua:: /// The receiver's name for error strings and what a leading `$parent` expands to, the owner frame /// (`+0x9c`), read under a borrow dropped before the ladder reads `_G`. pub(super) fn region_ladder_context(lua: &Lua, rh: RegionHandle) -> (String, String) { - let mut model = lua.app_data_mut::().expect("model"); - let id = model.region_id(rh); + let model = lua.app_data_mut::().expect("model"); let who = model - .region_names - .iter() - .find(|(_, &v)| v == id) - .map(|(n, _)| n.clone()) + .arena + .region(rh) + .and_then(|r| r.name.clone()) .unwrap_or_else(|| UNNAMED.to_string()); let owner = model.arena.region(rh).map(|r| r.owner); let base = super::object::parent_token_base(&model, owner); diff --git a/crates/benilla-ui/src/script/tests/name_targets.rs b/crates/benilla-ui/src/script/tests/name_targets.rs index 13156ad8..f0a0aa4b 100644 --- a/crates/benilla-ui/src/script/tests/name_targets.rs +++ b/crates/benilla-ui/src/script/tests/name_targets.rs @@ -258,3 +258,122 @@ fn a_constructor_name_expands_parent() { s.run(r#"Icon:SetPoint("TOPLEFT", "RootKid", "TOPLEFT", 0, 0)"#) .unwrap(); } + +/// A region's name lives on the region (`CScriptRegion::SetName 0x76c650` stores its own copy at +/// `[+0x98]`); only the `_G` entry is first-come (`0x701bd0` writes it only when empty). +#[test] +fn two_regions_with_one_name_both_answer_it_and_g_keeps_the_first() { + let s = script(); + s.run( + r#" + Host = CreateFrame("Frame", "NameHost") + First = Host:CreateTexture("SharedRegionName") + Second = Host:CreateTexture("SharedRegionName") + Third = Host:CreateFontString("SharedRegionName") + "#, + ) + .unwrap(); + for r in ["First", "Second", "Third"] { + assert_eq!( + s.eval::>(&format!("return {r}:GetName()")) + .unwrap() + .as_deref(), + Some("SharedRegionName"), + "{r} keeps the name it was given" + ); + } + assert!( + s.eval::(r#"return SharedRegionName == First and SharedRegionName ~= Second"#) + .unwrap(), + "_G holds the first region only" + ); +} + +/// The `$parent` expansion reads the owner frame, so two owners that share a name give their +/// regions one expanded name; both answer it. +#[test] +fn parent_named_regions_under_colliding_parents_both_answer_their_name() { + let s = script(); + s.run( + r#" + PA = CreateFrame("Frame", "CollidingParent") + PB = CreateFrame("Frame", "CollidingParent") + TextA = PA:CreateFontString("$parentText") + TextB = PB:CreateFontString("$parentText") + "#, + ) + .unwrap(); + for r in ["TextA", "TextB"] { + assert_eq!( + s.eval::>(&format!("return {r}:GetName()")) + .unwrap() + .as_deref(), + Some("CollidingParentText"), + "{r} answers its expanded name" + ); + } +} + +/// The receiver in an error string is the region's own name, taken or not. +#[test] +fn the_error_string_of_a_region_with_a_taken_name_uses_its_name() { + let s = script(); + s.run( + r#" + Host = CreateFrame("Frame", "ErrHost") + First = Host:CreateTexture("ErrRegionName") + Second = Host:CreateTexture("ErrRegionName") + "#, + ) + .unwrap(); + for r in ["First", "Second"] { + let err = s + .run(&format!(r#"{r}:SetPoint()"#)) + .expect_err("SetPoint with no arguments raises") + .to_string(); + assert!( + err.contains("Usage: ErrRegionName:SetPoint("), + "{r}'s error names the region: {err}" + ); + let err = s + .run(&format!("{r}:IsObjectType(nil)")) + .expect_err("IsObjectType(nil) raises") + .to_string(); + assert!( + err.contains(r#"Usage: ErrRegionName:IsObjectType("TYPE")"#), + "{r}'s error names the region: {err}" + ); + } +} + +/// A freed region takes its lookup entry with it only when the entry is its own: the first +/// holder going leaves a later same-named region's `GetName` alone. +#[test] +fn freeing_a_region_leaves_a_same_named_siblings_name() { + let s = script(); + s.run( + r#" + Host = CreateFrame("Frame", "FreeHost") + First = Host:CreateTexture("FreedRegionName") + Second = Host:CreateTexture("FreedRegionName") + "#, + ) + .unwrap(); + { + let lua = s.lua(); + let mut model = lua.app_data_mut::().expect("model"); + let id = *model.region_by_name.get("FreedRegionName").expect("held"); + let rh = *model.id_to_region.get(&id).expect("first region"); + crate::script::region::free_region(&mut model, rh); + assert!( + !model.region_by_name.contains_key("FreedRegionName"), + "the freed holder's entry is gone" + ); + } + assert_eq!( + s.eval::>("return Second:GetName()") + .unwrap() + .as_deref(), + Some("FreedRegionName") + ); +} diff --git a/crates/benilla-ui/src/script/tests/regions.rs b/crates/benilla-ui/src/script/tests/regions.rs index 7b7f504f..c1d651a5 100644 --- a/crates/benilla-ui/src/script/tests/regions.rs +++ b/crates/benilla-ui/src/script/tests/regions.rs @@ -549,7 +549,7 @@ fn desaturation_rides_the_extract_for_art_and_never_for_a_solid() { fn desaturated(s: &UiScript, name: &str) -> bool { let lua = s.lua(); let model = lua.app_data_ref::().expect("model"); - let id = *model.region_names.get(name).expect("region name"); + let id = *model.region_by_name.get(name).expect("region name"); let h = *model.id_to_region.get(&id).expect("region handle"); model.region_data.get(&h).is_some_and(|d| d.desaturated) } @@ -996,7 +996,7 @@ fn the_constructors_string_arguments_are_four_shapes_not_one() { let layer_of = |s: &UiScript, name: &str| { let lua = s.lua(); let model = lua.app_data_ref::().expect("model"); - let id = *model.region_names.get(name).expect("published region"); + let id = *model.region_by_name.get(name).expect("published region"); let rh = *model.id_to_region.get(&id).expect("live region"); model.arena.region(rh).expect("region").draw_layer }; diff --git a/crates/benilla-ui/src/script/tooltip/mod.rs b/crates/benilla-ui/src/script/tooltip/mod.rs index d4e1e6ee..e7611348 100644 --- a/crates/benilla-ui/src/script/tooltip/mod.rs +++ b/crates/benilla-ui/src/script/tooltip/mod.rs @@ -110,7 +110,7 @@ fn ensure_lines(lua: &Lua, this: &Table, n: usize) -> mlua::Result<()> { return None; } let id = model - .region_names + .region_by_name .get(&format!("{frame_name}Text{cell}{i}")) .copied()?; let rh = model.id_to_region.get(&id).copied()?; @@ -228,12 +228,12 @@ fn ensure_lines(lua: &Lua, this: &Table, n: usize) -> mlua::Result<()> { // Adopted pairs are already named and published by the loader. if adopted_left.is_none() { let ln = format!("{frame_name}TextLeft{i}"); - model.region_names.entry(ln.clone()).or_insert(left_id); + crate::script::region::set_region_name(&mut model, left, left_id, &ln); publish.push((left_id, ln)); } if adopted_right.is_none() { let rn = format!("{frame_name}TextRight{i}"); - model.region_names.entry(rn.clone()).or_insert(right_id); + crate::script::region::set_region_name(&mut model, right, right_id, &rn); publish.push((right_id, rn)); } } diff --git a/crates/benilla-ui/src/widget/mod.rs b/crates/benilla-ui/src/widget/mod.rs index 67643e69..1148a273 100644 --- a/crates/benilla-ui/src/widget/mod.rs +++ b/crates/benilla-ui/src/widget/mod.rs @@ -234,6 +234,10 @@ pub struct Region { /// `Region:SetParent(nil)`: unlinked and not drawn, but not destroyed (`0x77fd10` with a null /// parent). The owner's `regions` keeps it so `destroy` frees it; `SetParent(frame)` relinks. pub detached: bool, + /// The region's own name, the expanded string `SetName` keeps at `[+0x98]` (`0x76c650`); `None` + /// while anonymous. Only the `_G` entry is first-come (`0x701bd0`), so two regions can hold + /// one name, and `GetName` answers each its own. + pub name: Option, } /// A cursor over a frame's child list for [`WidgetArena::next_child`]: the child last returned and @@ -697,6 +701,7 @@ impl WidgetArena { sub_level, decl_seq, detached: false, + name: None, }); let handle = RegionHandle { index, generation }; self.frame_mut(owner)