From 6e8c4488e114c7b3da537633ef453f711b255fff Mon Sep 17 00:00:00 2001 From: grom Date: Mon, 2 Nov 2026 14:17:55 -0700 Subject: [PATCH] ui: the override-binding family exists and says what it does not do MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ClearOverrideBindings` was absent outright, so `Cinematic/Camera.lua:470` raised in grom's chat on every camera shot that ended — and a raise mid-handler costs the rest of the handler, not just the line. These are 2.0's secure-binding layer: a second, unsaved binding table a frame owns, outranking the saved one while that frame wants it and dropped whole by `ClearOverrideBindings(owner)`. This client models the saved table only (`script/keybind.rs`), so: - the five setters are no-ops that **warn once each** through the script warning channel. Silence was the alternative and it is the worse one — a setter that quietly binds nothing is the same class of fault as a hook that quietly arms nothing, and the caller cannot find out. - `ClearOverrideBindings` is a plain no-op and does not warn. Dropping a table that was never filled *is* nothing, and warning there would fire on every `ClearRecast()` while the player does nothing unusual. The module header lists what a real layer needs — an override table keyed by owner in `Model`, a generation bump for `bindings.rs`'s `sync_dispatch`, an override arm in `BindingDispatch::map` that wins over the saved chord, and a `Bound::Click` — together with the `SecureHandler*` half in `secure.rs`, which is the same table seen from inside a snippet and no-op for the same reason. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn --- .../src/script/compat/classic_api/mod.rs | 2 + .../compat/classic_api/override_bindings.rs | 112 ++++++++++++++++++ .../src/script/tests/reference_surface.rs | 14 +++ 3 files changed, 128 insertions(+) create mode 100644 crates/benilla-ui/src/script/compat/classic_api/override_bindings.rs diff --git a/crates/benilla-ui/src/script/compat/classic_api/mod.rs b/crates/benilla-ui/src/script/compat/classic_api/mod.rs index 850e48e8..de869c6f 100644 --- a/crates/benilla-ui/src/script/compat/classic_api/mod.rs +++ b/crates/benilla-ui/src/script/compat/classic_api/mod.rs @@ -72,6 +72,7 @@ mod uimap; // `pub(in …compat)` for its `plate_frame`, which SuperWoW's `UnitNameplate` shares. pub(in crate::script::compat) mod nameplate; mod new_items; +mod override_bindings; mod pixel_util; mod player_cache; mod player_info; @@ -137,6 +138,7 @@ pub(in crate::script) fn install(lua: &Lua) -> mlua::Result<()> { cvar::install_globals(lua)?; g.set("C_NamePlate", nameplate::install(lua)?)?; g.set("C_NewItems", new_items::install(lua)?)?; + override_bindings::install(lua)?; g.set("C_Macro", macro_ext::install(lua)?)?; g.set("C_Item", item::install(lua)?)?; g.set("C_AddOns", addons::install(lua)?)?; diff --git a/crates/benilla-ui/src/script/compat/classic_api/override_bindings.rs b/crates/benilla-ui/src/script/compat/classic_api/override_bindings.rs new file mode 100644 index 00000000..272bdd1f --- /dev/null +++ b/crates/benilla-ui/src/script/compat/classic_api/override_bindings.rs @@ -0,0 +1,112 @@ +//! The override-binding globals: `SetOverrideBinding` and its four typed siblings, and +//! `ClearOverrideBindings`. +//! +//! These are 2.0's secure-binding layer, not 1.12's. The reference binding table this client does +//! model (`script/keybind.rs`) is the *saved* one — one command per chord, persisted under +//! `benilla-config/bindings/` and repainted onto the action bar through `UPDATE_BINDINGS`. An +//! override is the other thing entirely: a second, unsaved table a frame owns, which outranks the +//! saved one while that frame wants it and is dropped whole by `ClearOverrideBindings(owner)`. +//! Nothing here implements that second table, so: +//! +//! - **The five setters are no-ops that say so**, once each, through the script warning channel. +//! Silence was the alternative and it is the worse one: a setter that quietly binds nothing is +//! the same class of fault as a hook that quietly arms nothing, and the caller has no way to +//! find out. `Cinematic/Camera.lua:461` is the shipped caller — hold a key during a camera shot +//! and it should recast — so the warning names a feature that is off, not a mystery. +//! - **`ClearOverrideBindings(owner)` is a plain no-op and does not warn.** Dropping a table that +//! was never filled *is* nothing; warning there would fire on every `ClearRecast()` the addon +//! makes while the player is doing nothing unusual. Before this it was absent outright, and the +//! raise it threw (`attempt to call global 'ClearOverrideBindings' (a nil value)`) was the one +//! line of this family that reached grom's chat. +//! +//! What a real layer needs, so the next pass starts from the measurement rather than the idea: an +//! override table keyed by owner in `Model`, a generation bump so `bindings.rs`'s `sync_dispatch` +//! rebuilds, an override arm in `BindingDispatch::map` that wins over the saved chord, and a +//! `Bound::Click` that calls a named button's `Click` — plus the `SecureHandler*` half +//! (`compat/classic_api/secure.rs`'s `SetBindingClick` and friends), which is the same table seen +//! from inside a snippet and is no-op for exactly the same reason. + +use mlua::{Lua, MultiValue, Value}; + +use crate::script::Model; + +/// A setter that does nothing and records why, the first time it is called. +/// +/// Per-function `Cell`, not one shared set: five calls to five different setters are five +/// different missing features and each deserves its own line, while five calls to the same one +/// are one. +fn warned_noop(lua: &Lua, name: &'static str) -> mlua::Result { + let fired = std::cell::Cell::new(false); + lua.create_function(move |lua, _: MultiValue| { + if !fired.replace(true) { + if let Some(mut model) = lua.app_data_mut::() { + model.record_warning(format!( + "{name}: this client has no override-binding layer, so the key is left bound \ + as it was — the call is ignored (docs/ADDON_COMPAT.md)" + )); + } + } + Ok(Value::Nil) + }) +} + +pub(super) fn install(lua: &Lua) -> mlua::Result<()> { + let g = lua.globals(); + for name in [ + "SetOverrideBinding", + "SetOverrideBindingClick", + "SetOverrideBindingSpell", + "SetOverrideBindingItem", + "SetOverrideBindingMacro", + ] { + g.set(name, warned_noop(lua, name)?)?; + } + // No warning: see the module header. + g.set( + "ClearOverrideBindings", + lua.create_function(|_, _: MultiValue| Ok(Value::Nil))?, + )?; + Ok(()) +} + +#[cfg(test)] +mod tests { + use crate::script::UiScript; + + #[test] + fn a_setter_warns_once_and_the_clear_is_quiet() { + let mut s = UiScript::new().unwrap(); + s.run(r#"SetOverrideBindingClick(UIParent, true, "T", "SomeButton")"#) + .unwrap(); + s.run(r#"SetOverrideBindingClick(UIParent, true, "Y", "SomeButton")"#) + .unwrap(); + s.run(r#"ClearOverrideBindings(UIParent)"#).unwrap(); + s.run(r#"ClearOverrideBindings(UIParent)"#).unwrap(); + let warns = s.take_warnings(); + assert_eq!(warns.len(), 1, "warn-once per setter: {warns:?}"); + assert!( + warns[0].contains("SetOverrideBindingClick"), + "the warning names the verb: {warns:?}" + ); + } + + #[test] + fn each_setter_gets_its_own_line() { + let mut s = UiScript::new().unwrap(); + s.run(r#"SetOverrideBinding(UIParent, true, "T", "JUMP")"#) + .unwrap(); + s.run(r#"SetOverrideBindingSpell(UIParent, true, "Y", "Fireball")"#) + .unwrap(); + assert_eq!(s.take_warnings().len(), 2); + } + + #[test] + fn the_clear_the_camera_calls_no_longer_raises() { + let mut s = UiScript::new().unwrap(); + // `Cinematic/Camera.lua:470`, which raised `attempt to call global + // `ClearOverrideBindings' (a nil value)` on every camera shot that ended. + s.run(r#"ClearOverrideBindings(CreateFrame("Button"))"#) + .unwrap(); + assert!(s.take_warnings().is_empty()); + } +} diff --git a/crates/benilla-ui/src/script/tests/reference_surface.rs b/crates/benilla-ui/src/script/tests/reference_surface.rs index 4b78b7d3..824ffab9 100644 --- a/crates/benilla-ui/src/script/tests/reference_surface.rs +++ b/crates/benilla-ui/src/script/tests/reference_surface.rs @@ -393,6 +393,20 @@ fn allowed_beyond_1_12() -> HashSet<&'static str> { // modern colour object; 1.12 passes colours as loose r,g,b numbers. "ColorMixin", "CreateColor", + // 2.0's override-binding layer (script/compat/classic_api/override_bindings.rs): a + // second, unsaved binding table a frame owns, outranking the saved one while it wants it. + // This client models the saved table only, so the five setters are no-ops that WARN once + // each and the clear is a silent one — dropping a table that was never filled is nothing. + // They are here rather than absent because the absence raised: `ClearOverrideBindings` is + // `Cinematic/Camera.lua:470`, on every camera shot that ended, and a raise mid-handler + // costs the rest of the handler. A feature test on these learns "present", which is wrong + // and is why the setters announce themselves in the log instead of binding silently. + "ClearOverrideBindings", + "SetOverrideBinding", + "SetOverrideBindingClick", + "SetOverrideBindingItem", + "SetOverrideBindingMacro", + "SetOverrideBindingSpell", // ClassicAPI's tInvert (script/compat/classic_api/lua_ext.rs): value->key table flip, a // Blizzard utility 1.12 has no counterpart for. "tInvert",