From d080b0c27342cef351314ef549f5e498fd45f0db Mon Sep 17 00:00:00 2001 From: Krathe Date: Sun, 2 Aug 2026 01:58:40 +0100 Subject: [PATCH 01/14] Pinned frames: never prune a set against unresolved roster or role data Two independent paths could delete auto-added members from a pinned set. Both write to saved config, so the pin was gone until something re-added it, and manualPlayers survives both -- which is why this only ever reproduced for people using the auto-add role filters. CleanOfflinePlayers pruned against a roster that had not populated yet. The existing guard only caught GetNumGroupMembers() == 0, but on a party->raid transition the member count goes live BEFORE GetRaidRosterInfo(i) resolves, and the raid branch of GetGroupRoster builds its table purely from GetRaidRosterInfo with no player fallback -- so the count passed while the roster was still completely empty and every auto-added pin pruned against it. Now guarded on the roster actually being populated, and partial population counts as not populated. Field log (2026-08-01 15:37:26, v4.9.0-alpha.1): "Mode changed from party to raid" logged "2 players in set, 0 valid" -- the 0 valid IS the empty roster -- and one second later "0 players in set". The tanks returned 21s later once the roster arrived and the auto-add pass re-added them. AutoPopulateSet's removal pass read a role map that coerces "NONE" to "DAMAGER". During role assignment a pinned TANK therefore momentarily reads as DAMAGER, and a set with only autoAddTanks enabled removed them. The coercion is correct for the ADD pass and is kept there; the REMOVE pass now reads a separate map holding only roles the game has actually assigned, so an unresolved role can no longer drive a deletion. --- Features/PinnedFrames.lua | 65 ++++++++++++++++++++++++++++++++++----- 1 file changed, 58 insertions(+), 7 deletions(-) diff --git a/Features/PinnedFrames.lua b/Features/PinnedFrames.lua index 0485e7dd..a642ccb0 100644 --- a/Features/PinnedFrames.lua +++ b/Features/PinnedFrames.lua @@ -534,8 +534,21 @@ function PinnedFrames:AutoPopulateSet(set, roster) return changed end - -- Build name → role map for the removal pass - local rosterRoles = {} -- shortName -> role + -- Build name → role map for the removal pass. + -- + -- TWO maps, deliberately. rosterRoles carries the NONE→DAMAGER coercion and + -- drives the auto-ADD pass (an unassigned player counts as DPS for adding — + -- long-standing behaviour). assignedRoles carries ONLY roles the game has + -- actually assigned, and is the one the auto-REMOVE pass reads. + -- + -- Removing on a coerced role is destructive: during zone-in / role assignment + -- UnitGroupRolesAssigned returns "NONE" for members whose role data has not + -- arrived yet, so a pinned TANK momentarily reads as DAMAGER and a set with + -- only autoAddTanks on would drop them from set.players — which is saved + -- config, so the pin is gone until something re-adds it. Never remove on + -- unknown data (same principle as Fix A/Fix B in CleanOfflinePlayers). + local rosterRoles = {} -- name -> role, NONE coerced to DAMAGER (add pass) + local assignedRoles = {} -- name -> role, only when actually assigned (remove pass) local isRaid = IsInRaid() -- Identify the player for the Exclude Self option (gates auto-add/remove of self). @@ -551,7 +564,12 @@ function PinnedFrames:AutoPopulateSet(set, roster) if fullName then local shortName = fullName:match("([^%-]+)") or fullName - local role = UnitGroupRolesAssigned(unit) + local rawRole = UnitGroupRolesAssigned(unit) + if rawRole and rawRole ~= "NONE" then + assignedRoles[shortName] = rawRole + assignedRoles[fullName] = rawRole + end + local role = rawRole if role == "NONE" then role = "DAMAGER" end rosterRoles[shortName] = role rosterRoles[fullName] = role @@ -591,9 +609,11 @@ function PinnedFrames:AutoPopulateSet(set, roster) if set.manualPlayers[playerName] then -- skip else - -- Only evaluate players still in the group - -- (offline/left players are handled by CleanOfflinePlayers) - local role = rosterRoles[playerName] + -- Only evaluate players still in the group whose role the game + -- has actually assigned (offline/left players are handled by + -- CleanOfflinePlayers; unassigned roles are left alone until + -- they resolve — see the assignedRoles note above). + local role = assignedRoles[playerName] if role then local matchesFilter = false if set.autoAddTanks and role == "TANK" then @@ -631,9 +651,40 @@ function PinnedFrames:CleanOfflinePlayers(set, roster) -- / just-left-the-group — keep the pins for next time rather than wiping them the -- instant the group disbands. Real leavers are pruned on the next update once the -- roster is non-empty. - if GetNumGroupMembers() == 0 then return false end + local numMembers = GetNumGroupMembers() + if numMembers == 0 then return false end roster = roster or GetGroupRoster() + + -- Fix C — Fix A's count check is not enough. On a party→raid transition (and + -- on zone-in) GetNumGroupMembers() goes live BEFORE GetRaidRosterInfo(i) + -- resolves, and the raid branch of GetGroupRoster builds the table purely + -- from GetRaidRosterInfo with no player fallback — so the count passes while + -- the roster is still completely empty, and every auto-added pin gets pruned + -- against it. manualPlayers survives (Fix B), which is why this only ever + -- reproduced for people using the auto-add role filters. + -- + -- Field log (2026-08-01 15:37:26, v4.9.0-alpha.1): "Mode changed from party + -- to raid — reinitializing" logged "2 players in set, 0 valid" — the 0 valid + -- IS the empty roster — and one second later the same set logged "0 players + -- in set". The tanks came back 21s later once the roster arrived and the + -- auto-add pass re-added them. + -- + -- roster carries a short-name ALIAS per cross-realm member on top of one + -- exact entry each, so its size is between numMembers and 2*numMembers when + -- healthy; fewer entries than members therefore means "not populated yet" + -- with no false positives. Partial population is treated as incomplete too — + -- pruning 15 of 20 raiders because only 5 have loaded is the same bug. + local rosterCount = 0 + for _ in pairs(roster) do + rosterCount = rosterCount + 1 + end + if rosterCount < numMembers then + DF:Debug("PINNED", "CleanOfflinePlayers skipped — roster not populated (%d entries for %d members)", + rosterCount, numMembers) + return false + end + local manual = set.manualPlayers local changed = false From e950ca149d6e289ffe4d27b9f22d4cdae09287f8 Mon Sep 17 00:00:00 2001 From: Krathe Date: Sun, 2 Aug 2026 01:58:48 +0100 Subject: [PATCH 02/14] Click casting: record showClaimed on the stuck-binds error The BINDINGS STILL ACTIVE line did not say how the frame got its binds, and the two ways have completely different release paths. A motion OnEnter arms Blizzard's secure OnLeave; an OnShow claim cannot, because Wrapped_OnLeave gates on `motion and self:GetAttribute("_wrapentered")` and only the motion branch of Wrapped_OnEnter sets that attribute. Restricted code cannot set it either -- HANDLE:SetAttribute rejects any name matching ^_ (RestrictedFrames.lua:523, Gethe live 4383ced3), so a show-claimed frame depends on OnHide, the next frame's claim, the mouseoverstate driver, or the insecure backstop. Without the field a log cannot tell the two apart after the fact, which cost a wrong reading of a user log on 2026-08-01. Record it and note the constraint next to the code that depends on it. --- ClickCasting/Frames.lua | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/ClickCasting/Frames.lua b/ClickCasting/Frames.lua index b0acadf6..b45e0225 100755 --- a/ClickCasting/Frames.lua +++ b/ClickCasting/Frames.lua @@ -2111,10 +2111,25 @@ function CC:SetupSecureHandlers(frame) local stateDriverRectKnown = self:GetAttribute("dfStateDriverRectKnown") local sdMouseX = self:GetAttribute("dfSDMouseX") local sdMouseY = self:GetAttribute("dfSDMouseY") + -- showClaimed separates the two ways this frame got its binds, and + -- the distinction decides whether a release was even POSSIBLE: a + -- claim made by the OnShow wrap can never be released by the secure + -- OnLeave, because Wrapped_OnLeave gates on + -- `motion and self:GetAttribute("_wrapentered")` and only the motion + -- branch of Wrapped_OnEnter sets that attribute. Restricted code + -- cannot set it either — HANDLE:SetAttribute rejects any name + -- matching ^_ (RestrictedFrames.lua:523, Gethe live 4383ced3). A + -- show-claimed frame therefore depends on OnHide, on the next + -- frame's claim, on the mouseoverstate driver above, or on the + -- backstop below. Without this field the log cannot tell a + -- show-claim from a motion-enter after the fact, and the two have + -- completely different release paths — a 2026-08-01 user log was + -- misread for exactly that reason. + local showClaimed = self:GetAttribute("dfShowClaimed") or 0 DF:DebugError("CLICK", "BINDINGS STILL ACTIVE after OnLeave %s! wrapLeave=%s mouseoverbutton=%s checkPassed=%s isSecureMO=%s postCheck=%s", frameName, tostring(wrapLeaveFired), mouseoverOnLeave, tostring(leaveCheckPassed), tostring(isSecureMouseover), postCheck) - DF:DebugError("CLICK", " clearedBy=%s stateDriverFired=%d underMouse=%s rectKnown=%s mousePos=%s,%s", - clearedBy, stateDriverCount, tostring(stateDriverUnderMouse), tostring(stateDriverRectKnown), tostring(sdMouseX), tostring(sdMouseY)) + DF:DebugError("CLICK", " clearedBy=%s showClaimed=%d stateDriverFired=%d underMouse=%s rectKnown=%s mousePos=%s,%s", + clearedBy, showClaimed, stateDriverCount, tostring(stateDriverUnderMouse), tostring(stateDriverRectKnown), tostring(sdMouseX), tostring(sdMouseY)) -- STUCK-BINDS BACKSTOP: the client sometimes skips the secure -- OnLeave wrap (and the mouseoverstate driver has a blind spot From 73c7fd40d69d9ef29ec5c6ec7517d03aa96effa8 Mon Sep 17 00:00:00 2001 From: Krathe Date: Sun, 2 Aug 2026 12:57:39 +0100 Subject: [PATCH 03/14] Click casting: restore the live hover during the sweep, not after it ApplyBindings wipes the header, which kills hover keybinds on whatever frame the cursor is on. ReassertHoverBinds exists to put them back, but it only ran at the tail of the batch walker -- and the walker yields between batches over an arbitrary `pairs` order, so a hovered frame landing late in the iteration stayed dead for the rest of that sweep, with every DF-bound key falling through to the action bar meanwhile. The reporter pressed their heal bind and cast the action-bar spell in that slot instead. Field capture with ElvUI loaded in LFR (2026-08-02): 500 registered frames per sweep, 240 of them ElvUI's, against the "100-150+" the batching was sized for. Two sweeps ran seconds apart at ~392 frame-applies each -- 12:37:37-38 in about a second, then 12:37:49-54 taking about six -- so the worst observed dead window is ~6s within one sweep. The hovered frame is now processed first, so its restore happens in the synchronous first batch. Two details matter: Its snippet is rebuilt before reasserting. The batch passes skipKeyboardUpdate, so at that point the frame still carries the OUTGOING snippet; reasserting without rebuilding would restore the previous profile's binds and silently cast the wrong spell, which is worse than no bind. The frame is passed explicitly. ReassertHoverBinds otherwise falls back to currentHoveredFrame, and in the same capture all five of its successes landed on a different frame than the one that had just been cleared. The tail call stays as a backstop for a cursor that moves mid-sweep. --- ClickCasting/Bindings.lua | 48 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/ClickCasting/Bindings.lua b/ClickCasting/Bindings.lua index e9da8bc3..c919fcad 100644 --- a/ClickCasting/Bindings.lua +++ b/ClickCasting/Bindings.lua @@ -638,6 +638,36 @@ function CC:ApplyBindings() end end + -- Hoist the frame under the cursor to the front of the sweep. + -- + -- The header wipe near the top of this function kills the live hover, and + -- nothing can put it back until that frame's own snippet has been rebuilt. + -- The tail of the batch walker did that -- but the walker yields between + -- batches, and `pairs` order is arbitrary, so a hovered frame landing late + -- in the iteration stayed dead for the rest of that sweep, and every + -- DF-bound key fell through to the action bar meanwhile (the reporter's + -- "4" cast their action-bar spell instead of the DF one). + -- + -- Field-measured with ElvUI loaded in LFR, 2026-08-02: 500 registered + -- frames per sweep, 240 of them ElvUI's -- worth noting the batching + -- below was sized for the "100-150+" its own comment assumes. Two sweeps + -- ran seconds apart, ~392 frame-applies each: 12:37:37-38 in about a + -- second, then 12:37:49-54 taking about six. So the worst observed dead + -- window is ~6s within a single sweep, NOT the whole span between them. + -- Processing this frame first collapses it to the synchronous first + -- batch either way. + local hovered = self.currentHoveredFrame + if hovered and hovered.IsMouseOver and hovered:IsMouseOver() then + for i = 2, #allFrames do + if allFrames[i] == hovered then + allFrames[i], allFrames[1] = allFrames[1], allFrames[i] + break + end + end + else + hovered = nil + end + if #allFrames > 0 then local BATCH_SIZE = 10 local batchIndex = 0 @@ -657,6 +687,24 @@ function CC:ApplyBindings() CC:ApplyBindingsToFrameUnified(allFrames[i], true) end + -- The hovered frame is index 1, so this runs in the first batch + -- (which is synchronous). Its snippet has to be rebuilt here + -- rather than waiting for RefreshKeyboardBindings at the tail: + -- the batch passes skipKeyboardUpdate, so the frame is carrying a + -- stale snippet at this point and reasserting without rebuilding + -- would restore the OUTGOING binds -- silently casting the + -- previous profile's spell, which is worse than no bind at all. + -- Pass the frame explicitly: ReassertHoverBinds otherwise falls + -- back to currentHoveredFrame, and in the field capture all five + -- of its successes landed on a different frame than the one that + -- had just been cleared. + if hovered and startIdx == 1 then + local target = hovered + hovered = nil + CC:UpdateFrameBindingAttributes(target) + CC:ReassertHoverBinds(target) + end + batchIndex = batchIndex + 1 if endIdx < #allFrames then From 14620018f661abe14d5dbacfaf8511a40a84ae44 Mon Sep 17 00:00:00 2001 From: Krathe Date: Sun, 2 Aug 2026 13:21:05 +0100 Subject: [PATCH 04/14] Click casting: one summary line per ApplyBindings sweep, not three per frame A sweep walks every registered frame, and each frame emitted three INFO lines (entry, ClearBindings, DONE). With ElvUI loaded that registry is ~590 frames, so a single sweep wrote ~1770 entries in a second or two, and sweeps fire on roster churn -- seven of them inside three minutes in one capture. At maxLines = 10000 that meant the log retained under three minutes of history, and the eviction was driven entirely by its own noisiest writer. Two consecutive attempts to capture a reported click-casting failure came back holding nothing but sweep chatter: the hover and PreClick lines around the failure had been flushed, and in one case the UI Reload marker with them. A debug log whose loudest writer destroys the evidence is worse than no log. The per-frame INFO is now gated behind a `quiet` flag that only the sweep sets, and the sweep emits a single line with frame count and elapsed ms (plus an INTERRUPTED variant when combat aborts it mid-walk). The flag is threaded as a parameter rather than held on CC: a suppression flag that leaked would silently disable logging, which is the exact failure mode being fixed here. Hovered-frame warnings and every downstream warning are untouched -- those are rare and are the ones worth keeping. --- ClickCasting/Bindings.lua | 52 +++++++++++++++++++++++++++++++-------- 1 file changed, 42 insertions(+), 10 deletions(-) diff --git a/ClickCasting/Bindings.lua b/ClickCasting/Bindings.lua index c919fcad..0b203438 100644 --- a/ClickCasting/Bindings.lua +++ b/ClickCasting/Bindings.lua @@ -361,7 +361,10 @@ local MODIFIER_COMBOS = { -- either identical (rescan/roster reapply, the common case) or one refresh -- behind (profile switch), and the batch-end rebuild — deferred to combat end -- if interrupted — overwrites it either way. -function CC:ClearBindingsFromFrame(frame, preserveSnippet) +-- `quiet` suppresses only the per-frame INFO line, never the hovered-frame WARN. +-- Set by the ApplyBindings sweep, which walks every registered frame: see the +-- volume note on ApplyBindings' summary line. +function CC:ClearBindingsFromFrame(frame, preserveSnippet, quiet) if not frame then return end -- Combat-safe by contract: only reached from ApplyBindingsToFrameUnified, -- which defers "bindingRefresh". Do not add a bare return here without one. @@ -374,7 +377,7 @@ function CC:ClearBindingsFromFrame(frame, preserveSnippet) local frameName = frame:GetName() or "unnamed" if isCurrentlyHovered then DF:DebugWarn("CLICK", "ClearBindings on HOVERED frame %s (preserving snippet/overrides)", frameName) - else + elseif not quiet then DF:Debug("CLICK", "ClearBindings %s", frameName) end @@ -671,12 +674,16 @@ function CC:ApplyBindings() if #allFrames > 0 then local BATCH_SIZE = 10 local batchIndex = 0 + local sweepStart = GetTime() + local applied = 0 local function ProcessNextBatch() if InCombatLockdown() then -- Combat started during batch - flag for retry after combat CC:Defer("bindingRefresh") CC.batchBindingTimer = nil + DF:Debug("CLICK", "ApplyBindings sweep INTERRUPTED by combat: %d/%d frames in %dms", + applied, #allFrames, (GetTime() - sweepStart) * 1000) return end @@ -684,7 +691,10 @@ function CC:ApplyBindings() local endIdx = math.min(startIdx + BATCH_SIZE - 1, #allFrames) for i = startIdx, endIdx do - CC:ApplyBindingsToFrameUnified(allFrames[i], true) + -- quiet=true: one summary line per sweep instead of three per + -- frame. See the note on that summary below. + CC:ApplyBindingsToFrameUnified(allFrames[i], true, true) + applied = applied + 1 end -- The hovered frame is index 1, so this runs in the first batch @@ -716,6 +726,21 @@ function CC:ApplyBindings() CC:RefreshKeyboardBindings() -- The header wipe above kills a live hover; put it straight back. CC:ReassertHoverBinds() + + -- ONE line per sweep. This used to be three INFO lines per + -- frame, and a sweep walks every registered frame -- with + -- ElvUI loaded that is ~590 frames, so ~1770 entries in a + -- second or two. At maxLines = 10000 that let a handful of + -- sweeps evict the entire history: two separate attempts to + -- capture a reported bug (2026-08-02) came back holding only + -- sweep noise, having flushed the hover and PreClick lines + -- around the actual failure -- and in one case the reload + -- marker too. A debug log whose loudest writer destroys the + -- evidence is worse than no log. The hovered-frame WARNs and + -- every per-frame warning still fire; only the routine + -- per-frame INFO chatter is folded into this. + DF:Debug("CLICK", "ApplyBindings sweep: %d frames in %dms", + applied, (GetTime() - sweepStart) * 1000) end end @@ -2769,7 +2794,10 @@ end -- Apply all bindings to a frame using unified macro approach -- skipKeyboardUpdate: when true, skip UpdateFrameBindingAttributes (caller will batch it) -function CC:ApplyBindingsToFrameUnified(frame, skipKeyboardUpdate) +-- `quiet` suppresses the two per-frame INFO lines (entry and DONE) and nothing +-- else -- the hovered-frame WARN below and every warning downstream still fire. +-- Only the ApplyBindings sweep sets it; see the volume note on its summary line. +function CC:ApplyBindingsToFrameUnified(frame, skipKeyboardUpdate, quiet) if not frame then return end if self:CombatGuard("bindingRefresh") then return end @@ -2784,7 +2812,9 @@ function CC:ApplyBindingsToFrameUnified(frame, skipKeyboardUpdate) -- Debug: track when bindings are reapplied (helps diagnose unexpected clears) local isHovered = (self.currentHoveredFrame == frame) or (frame.IsMouseOver and frame:IsMouseOver()) - DF:Debug("CLICK", "ApplyBindings %s hovered=%s", frameName, tostring(isHovered)) + if not quiet then + DF:Debug("CLICK", "ApplyBindings %s hovered=%s", frameName, tostring(isHovered)) + end if isHovered then DF:DebugWarn("CLICK", "ApplyBindings on HOVERED frame %s — bindings may flicker! caller: %s", frameName, debugstack(2, 1, 0) or "unknown") @@ -2855,7 +2885,7 @@ function CC:ApplyBindingsToFrameUnified(frame, skipKeyboardUpdate) -- left frames snippet-less for a whole fight (see ClearBindingsFromFrame). -- The no-bindings leg above must NOT preserve it -- nothing rewrites the -- snippet on a frame with no bindings, so there it has to be cleared. - self:ClearBindingsFromFrame(frame, skipKeyboardUpdate) + self:ClearBindingsFromFrame(frame, skipKeyboardUpdate, quiet) -- Register for clicks based on castOnDown option if frame.RegisterForClicks then @@ -2959,10 +2989,12 @@ function CC:ApplyBindingsToFrameUnified(frame, skipKeyboardUpdate) end -- Debug: confirm final attribute state after apply - local finalType1 = frame:GetAttribute("type1") - local finalMacro1 = frame:GetAttribute("macrotext1") - DF:Debug("CLICK", "ApplyBindings DONE %s type1=%s macro1=%s", - frameName, tostring(finalType1), finalMacro1 and finalMacro1:sub(1, 50) or "nil") + if not quiet then + local finalType1 = frame:GetAttribute("type1") + local finalMacro1 = frame:GetAttribute("macrotext1") + DF:Debug("CLICK", "ApplyBindings DONE %s type1=%s macro1=%s", + frameName, tostring(finalType1), finalMacro1 and finalMacro1:sub(1, 50) or "nil") + end -- Update keyboard binding snippet for WrapScript to use -- Skip when caller will batch-refresh all frames (e.g. ApplyBindings) From 0f89d5d517a6edae121c2a7391636c474f4630a8 Mon Sep 17 00:00:00 2001 From: Krathe Date: Sun, 2 Aug 2026 16:23:23 +0100 Subject: [PATCH 05/14] Click casting: register frames that gain their unit after creation Root cause of the long-standing "my own frame stops click casting until a reload" reports, and of click casting dying on pinned frames specifically. ClickCastFrames' __newindex rawsets the key BEFORE testing eligibility, and eligibility requires a unit. A header child is created before the header assigns units -- Frames/Init.lua says so itself: "the header pre-creates children but units aren't set until group members actually appear." So a fresh child was rawset (key now present, its one __newindex spent) and then refused for having no unit. Every later write bypassed the metatable entirely, because __newindex only fires for keys the table does not already hold. The frame stayed unregistered for the whole session -- no hooks, no bindings, every bound key falling through to the action bar -- while dfClickCastRegistered claimed it had worked. ScanForThirdPartyFrames would have repaired it, but it runs 1s and 3s after setup, so it only ever covered login. That is exactly why a reload was the only cure. Pinned frames took the brunt because a party<->raid mode change recreates all 40 children mid-session, far outside that window. Field capture (2026-08-02 16:18): a mode change created 40 pinned children, and the next 15 seconds of hovering and pressing a bound key produced ZERO OnEnter/OnLeave/PreClick entries. The same actions after a reload logged normally with the correct macro, and the binding sweep saw 357 frames before against 375 after. Fixed in two places: CC:EnsureRegistered is now the single entry point. When a frame is not yet eligible it is remembered rather than dropped, and it honours an explicit opt-out so a deliberately hidden frame is not resurrected. UnregisterFrame clears any pending entry unconditionally, since a frame can be unregistered while still only pending. The unit-assignment path in Headers.lua calls it, which is the moment the missing precondition actually arrives. DF's own RegisterFrameWithClickCast calls it too rather than trusting the table write, so a repeat registration cannot be silently swallowed by the absent metamethod. --- ClickCasting/Frames.lua | 53 ++++++++++++++++++++++++++++++++++++++--- Frames/Headers.lua | 14 +++++++++++ Frames/Init.lua | 7 ++++++ 3 files changed, 71 insertions(+), 3 deletions(-) diff --git a/ClickCasting/Frames.lua b/ClickCasting/Frames.lua index b45e0225..5b5ae34c 100755 --- a/ClickCasting/Frames.lua +++ b/ClickCasting/Frames.lua @@ -869,6 +869,45 @@ local function clickCastFrameEligible(frame) return unit ~= nil end +-- Register a frame with click casting, or remember it for when it becomes +-- eligible. This is the ONE entry point -- ClickCastFrames' __newindex, DF's own +-- RegisterFrameWithClickCast, and the unit-assignment path all come through here. +-- +-- Why it exists: __newindex fires only for keys the table does not already have, +-- and it rawsets before testing eligibility. A header child is created BEFORE the +-- header assigns its unit ("the header pre-creates children but units aren't set +-- until group members actually appear" -- Frames/Init.lua), so a fresh child was +-- rawset (key now present, __newindex spent) and then failed the unit test, and +-- every later write bypassed the metatable entirely. The frame stayed +-- unregistered for the session while dfClickCastRegistered claimed otherwise: no +-- hooks, no binds, keys falling through to the action bar. +-- +-- Field-proven (2026-08-02 16:18): a party->raid mode change recreated 40 pinned +-- children; the next 15 seconds of hovering and pressing a bound key produced +-- ZERO OnEnter/PreClick entries, and the sweep saw 357 frames against 375 after a +-- reload. ScanForThirdPartyFrames would have caught it, but it only runs 1s and +-- 3s after setup -- login only -- which is exactly why a reload was the only cure. +function CC:EnsureRegistered(frame) + if not frame then return false end + if self.registeredFrames and self.registeredFrames[frame] then return true end + if not (self.db and self.db.enabled) then return false end + + -- Respect an explicit opt-out: UnregisterFrameWithClickCast writes false, and + -- a frame hidden on purpose must not be resurrected when its unit arrives. + if ClickCastFrames and ClickCastFrames[frame] == false then return false end + + if clickCastFrameEligible(frame) then + if self.pendingRegistration then self.pendingRegistration[frame] = nil end + self:RegisterFrame(frame) + return true + end + + -- No unit yet. Hold it; the unit-assignment path retries. + self.pendingRegistration = self.pendingRegistration or {} + self.pendingRegistration[frame] = true + return false +end + function CC:SetupClickCastFramesGlobal() -- If our click casting is disabled, DON'T set up our metatable -- This allows Clique/Clicked to set up their own metatable and work normally @@ -898,12 +937,15 @@ function CC:SetupClickCastFramesGlobal() -- Always store the value in the table rawset(t, frame, enabled) - -- Process registration since our click casting is enabled + -- Process registration since our click casting is enabled. + -- EnsureRegistered rather than an inline eligibility test: the rawset + -- above has already spent this frame's one __newindex, so a frame that + -- is not eligible YET must be remembered rather than dropped. if CC.db and CC.db.enabled then if enabled == nil or enabled == false then CC:UnregisterFrame(frame) - elseif clickCastFrameEligible(frame) then - CC:RegisterFrame(frame) + else + CC:EnsureRegistered(frame) end end end @@ -2667,6 +2709,11 @@ end -- Unregister a unit frame from click-casting function CC:UnregisterFrame(frame) if not frame then return end + -- Drop any deferred registration first, and unconditionally: a frame can be + -- unregistered while still only PENDING (created, no unit yet, then hidden + -- before its unit arrived). Leaving it queued would let the unit-assignment + -- path resurrect a frame the caller has just opted out of. + if self.pendingRegistration then self.pendingRegistration[frame] = nil end if not self.registeredFrames then return end if not self.registeredFrames[frame] then return end diff --git a/Frames/Headers.lua b/Frames/Headers.lua index fafd9fb5..72cf7c03 100755 --- a/Frames/Headers.lua +++ b/Frames/Headers.lua @@ -779,6 +779,20 @@ function DF:InitializeHeaderChild(frame) -- Sync legacy DF.playerFrame for backward compatibility DF.playerFrame = self end + + -- Click casting needs a unit to accept a frame, and this is the + -- moment one arrives. A header child is created before the header + -- assigns units, so registration at creation is REFUSED for lack + -- of a unit -- and the ClickCastFrames metatable cannot retry, + -- because its rawset already spent that frame's one __newindex. + -- Without this the frame stays unregistered for the session: no + -- hooks, no binds, bound keys falling through to the action bar, + -- with only a /reload to clear it. Cheap and idempotent -- + -- EnsureRegistered returns immediately if already registered, and + -- honours an explicit opt-out. + if DF.ClickCast and DF.ClickCast.EnsureRegistered then + DF.ClickCast:EnsureRegistered(self) + end -- No per-frame event registration needed: global headerChildEventFrame -- handles all unit events and dispatches via unitFrameMap[unit]. diff --git a/Frames/Init.lua b/Frames/Init.lua index f622aa8e..a3ff757b 100644 --- a/Frames/Init.lua +++ b/Frames/Init.lua @@ -1141,6 +1141,13 @@ function DF:RegisterFrameWithClickCast(frame) if DF.clickCastReady and ClickCastFrames then ClickCastFrames[frame] = true + -- Do not rely on the table write alone. __newindex fires only for keys the + -- table does not already hold, so a frame that has been through here before + -- (or was rawset while ineligible) gets no metamethod at all and would be + -- silently skipped -- while the flag below claims success. Go direct. + if DF.ClickCast and DF.ClickCast.EnsureRegistered then + DF.ClickCast:EnsureRegistered(frame) + end frame.dfClickCastRegistered = true else -- Mark for deferred registration From f21d8008afe3fcd1a7839cacb146a67c5a106a65 Mon Sep 17 00:00:00 2001 From: Krathe Date: Sun, 2 Aug 2026 16:32:08 +0100 Subject: [PATCH 06/14] Frames: don't let a pinned frame become DF.playerFrame The OnAttributeChanged handler assigned DF.playerFrame from any child that gained unit=player, pinned frames included. DF.playerFrame is not a generic "whichever frame holds the player" pointer though -- around fifty consumers treat it as the party player frame specifically. SecureSort uses it as party slot 0, setting secure paths and swap frame refs on it, and UpdateAllFrames drives both its unit watch and its click-cast registration from it. A user who pins themselves and hides self from party frames had all of that pointed at a pinned frame. The sibling write fifty lines above already guards this exact case ("Skip for pinned frames - they must not remove main frame entries"); this one never got the same treatment. Also guards the test-mode block that called UnregisterUnitWatch and :Hide on DF.playerFrame without a nil check. It can legitimately be nil -- the same handler clears it when a frame gives up unit=player, and with "hide self from party frames" no main-frame child may ever be assigned that unit -- so this was already reachable, and narrowing who sets the pointer makes nil more likely. The neighbouring blocks all test it first. --- Frames/Headers.lua | 19 +++++++++++++++++-- Frames/Init.lua | 10 ++++++++-- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/Frames/Headers.lua b/Frames/Headers.lua index 72cf7c03..e45c047e 100755 --- a/Frames/Headers.lua +++ b/Frames/Headers.lua @@ -776,8 +776,23 @@ function DF:InitializeHeaderChild(frame) self.index = tonumber(num) elseif actualUnit == "player" then self.index = 0 - -- Sync legacy DF.playerFrame for backward compatibility - DF.playerFrame = self + -- Sync legacy DF.playerFrame for backward compatibility -- + -- main-frame children ONLY. + -- + -- DF.playerFrame is not a generic "whichever frame holds + -- unit=player"; ~50 consumers treat it as the party player + -- frame specifically. SecureSort uses it as party slot 0, + -- setting secure paths and swap frame refs on it, and + -- UpdateAllFrames drives its unit watch and its click-cast + -- registration from it. A pinned frame showing the player was + -- being adopted here, which hands all of that the wrong frame. + -- + -- The sibling write fifty lines above already guards exactly + -- this ("Skip for pinned frames - they must not remove main + -- frame entries"); this one never got the same treatment. + if not self.isPinnedFrame then + DF.playerFrame = self + end end -- Click casting needs a unit to accept a frame, and this is the diff --git a/Frames/Init.lua b/Frames/Init.lua index a3ff757b..144e668a 100644 --- a/Frames/Init.lua +++ b/Frames/Init.lua @@ -1836,8 +1836,14 @@ function DF:UpdateAllFrames() end end - -- Handle test mode player frame visibility - if DF.testMode and not (testFrameCount >= 1) then + -- Handle test mode player frame visibility. + -- + -- DF.playerFrame is legitimately nil at times -- the OnAttributeChanged + -- handler clears it when the frame holding unit=player gives it up, and with + -- "hide self from party frames" the header may never assign a main-frame + -- child that unit at all. The two lines below dereference it unguarded, so + -- this could throw. The sibling block above already tests it first. + if DF.testMode and not (testFrameCount >= 1) and DF.playerFrame then UnregisterUnitWatch(DF.playerFrame) DF.playerFrame:Hide() DF:UnregisterFrameWithClickCast(DF.playerFrame) From 2edfe77f0872fe89d7455fdd59cb191a0ea3c72a Mon Sep 17 00:00:00 2001 From: Krathe Date: Sun, 2 Aug 2026 16:46:27 +0100 Subject: [PATCH 07/14] Click casting: arm our own frames at creation, not at first unit Removes the last unbounded dead-bind window rather than shortening it. clickCastFrameEligible refused any frame without a "unit" attribute. Header children are created before the header assigns units, so registration was deferred until the unit arrived -- and if it arrived during combat, RegisterFrame defers under lockdown, so the wrap and the snippet could not be installed until combat ended. A frame that first gained its unit mid-fight had no keyboard binds for the REST OF THE FIGHT. That is the shape of the "my binds stopped working in the middle of a key" reports: not a bind pointing at the wrong thing, but a frame that was never armed. The gate was testing a fact the work does not use. Nothing in the hover-bind path reads the unit: the snippet is `owner:SetBindingClick(true, key, self, virtualBtn)`, the unit resolves at click time from the frame's own attribute, and applicability is decided by dfIsDandersFrame, which is set before registration is attempted. So our frames are now eligible from creation. By the time a unit lands, in combat or not, there is nothing left to do. A unit-less frame is hidden by RegisterUnitWatch and cannot be hovered, so its binds never activate until it holds someone. Foreign frames keep the strict unit test. That gate was added to stop click casting taking over buttons that are not unit frames at all (a toy button had its type1 replaced), and for anything we did not create, carrying a unit is the only way to tell. Cost: frames that never receive a unit are now registered too, so the ApplyBindings sweep covers more of them. The sweep summary line reports frame count and elapsed, so the real impact is measurable rather than guessed. Also instruments the two release paths that have never been observed doing work. dfHideFired counts every OnHide wrap RUN rather than only its clears, and the OnEnter line now samples it alongside the state-driver count. "Never cleared" is ambiguous between dead code and correctly-nothing-to-do, and those have opposite conclusions -- neither path gets deleted on absence alone. This is the measurement that kept the OnShow claim and removed the state-driver reclaim. --- ClickCasting/Frames.lua | 49 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 44 insertions(+), 5 deletions(-) diff --git a/ClickCasting/Frames.lua b/ClickCasting/Frames.lua index 5b5ae34c..3be7696b 100755 --- a/ClickCasting/Frames.lua +++ b/ClickCasting/Frames.lua @@ -855,15 +855,40 @@ end -- frame's click attributes to DF's macro/target actions, which BREAKS any -- non-unit secure button that lands in the global table — a toy/action button -- has no unit and can't receive unit-targeted casts anyway (bug #988, --- ToyPicker's button had its type1 replaced). Frames whose unit is assigned --- late (secure header children) are picked up by the next RegisterAllFrames --- sweep once the attribute exists. +-- ToyPicker's button had its type1 replaced). +-- +-- OUR OWN frames are exempt from the unit test -- see below. local function clickCastFrameEligible(frame) if type(frame) ~= "table" or not frame.GetObjectType or not frame.GetAttribute then return false end local objType = frame:GetObjectType() if objType ~= "Button" and objType ~= "Frame" then return false end + + -- Our frames are known unit frames the moment they exist, so they do not + -- wait for a unit to be assigned. + -- + -- Nothing in the hover-bind machinery reads the unit. The snippet is + -- `owner:SetBindingClick(true, key, self, virtualBtn)` -- the unit resolves + -- at click time from the frame's own attribute -- and applicability is + -- decided by dfIsDandersFrame, set before registration is even attempted + -- (Frames/Headers.lua). The unit test was gating the work on a fact the + -- work does not use. + -- + -- That gating is what made the worst case unbounded rather than brief. A + -- header child that first gains its unit DURING combat could not be + -- registered, because RegisterFrame defers under lockdown -- so its wrap and + -- snippet could not be installed until combat ended, leaving it with no + -- keyboard binds for the REST OF THE FIGHT. Arming at creation removes the + -- window rather than shortening it: by the time a unit arrives, in combat or + -- not, there is nothing left to do. A unit-less frame is hidden by + -- RegisterUnitWatch and cannot be hovered, so its binds never activate until + -- it actually holds someone. + if frame.dfIsDandersFrame == true then return true end + + -- Foreign frames keep the strict test: this gate exists to stop click + -- casting taking over buttons that are not unit frames at all, and for + -- anything we did not create, carrying a unit is the only way to tell. local unit = frame:GetAttribute("unit") if issecretvalue(unit) then return false end return unit ~= nil @@ -1842,7 +1867,14 @@ function CC:SetupSecureHandlers(frame) -- ClearBindings() even during combat — unlike HookScript OnHide. -- Covers the case where a frame is hidden while hovered (e.g., party -- member leaves group, pet dies) and OnLeave doesn't fire. + -- dfHideFired counts every time the wrap RUNS, not every time it clears. + -- Without it "clearedBy=onhide never appears" is ambiguous between "this + -- path is dead code" and "it runs constantly and correctly has nothing to + -- do", and those have opposite conclusions. Same measurement that settled + -- the OnShow claim (kept, load-bearing) against the state-driver reclaim + -- (deleted, never unique) -- do not delete this path on absence alone. local onHideSnippet = [[ + self:SetAttribute("dfHideFired", (self:GetAttribute("dfHideFired") or 0) + 1) if mouseoverbutton == self then self:SetAttribute("dfClearedBy", "onhide") self:ClearBindings() @@ -2009,11 +2041,18 @@ function CC:SetupSecureHandlers(frame) -- the wrap snippet, so when wrapEnter is false they are STALE values from the -- last successful cycle, not a description of THIS hover. "phase=7 with -- wrapEnter=false" means the PREVIOUS enter completed, nothing more. - DF:Debug("CLICK", "OnEnter %s unit=%s kbActive=%s hasKB=%s type1=%s wrapEnter=%s(%d) wrapLeave=%d phase=%d prev=%s postCheck=%s showClaimed=%d reasserted=%d", + -- sdFired / hideFired are the instrument-before-delete counters for the + -- two release paths that have never been observed doing work. Sampled + -- here because OnEnter is the one line that prints on every hover, so a + -- normal session builds the evidence without any extra logging. + local sdFired = self:GetAttribute("dfStateDriverCount") or 0 + local hideFired = self:GetAttribute("dfHideFired") or 0 + DF:Debug("CLICK", "OnEnter %s unit=%s kbActive=%s hasKB=%s type1=%s wrapEnter=%s(%d) wrapLeave=%d phase=%d prev=%s postCheck=%s showClaimed=%d reasserted=%d sdFired=%d hideFired=%d", frameName, tostring(unit), tostring(bindingsActive), tostring(hasKeyboardBindings), tostring(type1), tostring(wrapEnterFired), wrapEnterCount, wrapLeaveCount, - enterPhase, prevMouseover, postCheck, showClaimed, reasserted) + enterPhase, prevMouseover, postCheck, showClaimed, reasserted, + sdFired, hideFired) -- THE MEASUREMENT: was a redundant set path load-bearing on THIS hover? -- From 3a6e8e0757b250f0ab88937a76fad703b389e991 Mon Sep 17 00:00:00 2001 From: Krathe Date: Sun, 2 Aug 2026 17:03:10 +0100 Subject: [PATCH 08/14] Click casting: reconcile against ClickCastFrames instead of trusting __newindex Fixes a regression introduced earlier today and a long-standing twin of it. pendingRegistration was WRITE-ONLY. EnsureRegistered parked frames that were not yet eligible with the comment "the unit-assignment path retries", but that retry only exists for our own header children, via the explicit hook in Frames/Headers.lua. Nothing ever iterated the table. A third-party group header that registers its children before assigning them units had every child parked and never looked at again -- an entire foreign raid grid silently without click casting until a reload. That is the same write-only-flag antipattern this work has been removing, and I added it. The twin, which predates it: `ClickCastFrames[frame] = false` is the documented Clique-convention opt-out, and it never unregistered anything. __newindex fires only for keys the table does not already hold, and the rawset inside it spends each frame's one chance on the first write -- so for any frame that was ever registered, the unregister branch is unreachable. DF kept its bindings, wrap and snippet on frames whose owning addon had explicitly taken them back, for the rest of the session. Neither is observable by watching writes, so ReconcileClickCastFrames compares the two tables instead: register anything parked that has become eligible, release anything we hold that has since been marked false. Called from ApplyBindings, which runs on roster churn -- the same churn that creates and retires these frames. Cheap: the pending table is near-empty in steady state and the registry walk is a few hundred entries we already own. --- ClickCasting/Bindings.lua | 8 +++++ ClickCasting/Frames.lua | 65 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/ClickCasting/Bindings.lua b/ClickCasting/Bindings.lua index 0b203438..50a03e46 100644 --- a/ClickCasting/Bindings.lua +++ b/ClickCasting/Bindings.lua @@ -629,6 +629,14 @@ function CC:ApplyBindings() self:RefreshBlizzardClickCastClearing() end + -- Reconcile against the public table before deciding what to sweep. The + -- ClickCastFrames metatable structurally cannot see a retry or an opt-out -- + -- its rawset spends each frame's one __newindex -- so this is the only thing + -- that picks up a foreign frame which has since gained a unit, or releases + -- one whose owning addon has taken it back. Here because ApplyBindings runs + -- on roster churn, which is the same churn that creates and retires them. + self:ReconcileClickCastFrames() + -- Apply bindings to all registered frames in batches to avoid "script ran too long". -- With ElvUI or other addons, 100-150+ frames can be registered. Each frame requires -- ~300+ SetAttribute calls, so processing them all synchronously exceeds Lua's time limit. diff --git a/ClickCasting/Frames.lua b/ClickCasting/Frames.lua index 3be7696b..b98c0660 100755 --- a/ClickCasting/Frames.lua +++ b/ClickCasting/Frames.lua @@ -927,12 +927,75 @@ function CC:EnsureRegistered(frame) return true end - -- No unit yet. Hold it; the unit-assignment path retries. + -- No unit yet. Hold it; ReconcileClickCastFrames retries. self.pendingRegistration = self.pendingRegistration or {} self.pendingRegistration[frame] = true return false end +-- Reconcile CC's registry against the public ClickCastFrames table. +-- +-- Two things the ClickCastFrames metatable structurally cannot do, both because +-- __newindex fires only for keys the table does not already hold and the rawset +-- inside it spends that one chance immediately: +-- +-- 1. RETRY a frame that was ineligible when first written. EnsureRegistered +-- parks those in pendingRegistration. Our own header children are covered +-- by the explicit hook on unit assignment (Frames/Headers.lua), but nothing +-- covered anything else -- a third-party group header that registers its +-- children before assigning units had them parked and never looked at +-- again, so an entire foreign raid grid could silently have no click +-- casting until a reload. pendingRegistration was write-only. +-- +-- 2. Notice an opt-out. The documented Clique-convention unregister is +-- `ClickCastFrames[frame] = false`, and for any frame that has been +-- registered the key already exists, so that write lands in the table with +-- no metamethod at all. DF kept its bindings, its wrap and its snippet on a +-- frame whose owner had explicitly taken it back, for the rest of the +-- session. +-- +-- Both are reconciled by comparing the two tables rather than by trying to +-- observe writes. Cheap: pendingRegistration is near-empty in steady state, and +-- the registry walk is a few hundred entries against a table we already own. +function CC:ReconcileClickCastFrames() + if not ClickCastFrames then return end + if not (self.db and self.db.enabled) then return end + if InCombatLockdown() then return end + + -- (1) Anything parked that has since become eligible. + if self.pendingRegistration then + local nowReady = {} + for frame in pairs(self.pendingRegistration) do + if ClickCastFrames[frame] ~= false and clickCastFrameEligible(frame) then + nowReady[#nowReady + 1] = frame + end + end + for _, frame in ipairs(nowReady) do + self.pendingRegistration[frame] = nil + self:RegisterFrame(frame) + end + if #nowReady > 0 then + DF:Debug("CLICK", "Reconcile: registered %d frame(s) that became eligible", #nowReady) + end + end + + -- (2) Anything we hold that has been opted out of since. + if self.registeredFrames then + local revoked = {} + for frame in pairs(self.registeredFrames) do + if ClickCastFrames[frame] == false then + revoked[#revoked + 1] = frame + end + end + for _, frame in ipairs(revoked) do + self:UnregisterFrame(frame) + end + if #revoked > 0 then + DF:DebugWarn("CLICK", "Reconcile: released %d frame(s) opted out via ClickCastFrames", #revoked) + end + end +end + function CC:SetupClickCastFramesGlobal() -- If our click casting is disabled, DON'T set up our metatable -- This allows Clique/Clicked to set up their own metatable and work normally From 14e060b38fa0575e849800354e6664e22569fc25 Mon Sep 17 00:00:00 2001 From: Krathe Date: Sun, 2 Aug 2026 17:08:44 +0100 Subject: [PATCH 09/14] Click casting: close the P0 dead-bind windows found by the audit Five fixes, all of them cases where binds went dead rather than merely wrong. The batched sweep left every already-processed frame holding the OUTGOING snippet while its outgoing type- attributes had already been erased, so the keys that snippet bound pointed at cleared attributes -- dead AND stolen from the action bar -- for the rest of the sweep, or for the whole fight if combat interrupted it. The snippet is now rebuilt inline per frame. That costs nothing: the batch tail already called UpdateFrameBindingAttributes once per registered frame, so it is the same work moved earlier, and clear-then-rebuild now happens inside one call with no yield for combat to land in. The zone settle pass never ran at login. RegisterEvents is reached from inside the dispatch of PLAYER_ENTERING_WORLD, so our own frame never receives that event. Nameplate registration, the zone-in binding repair and the cold-start profile resolve -- the belt added for "no binds in my first arena of the day" -- were all absent until the first zone change. Factored out as ScheduleZoneSettle and kicked once at init; the timer key keeps it idempotent. Opposed deferred jobs let DRAIN_ORDER arbitrate instead of the caller. Both register/unregister and blizzardRegister/blizzardUnregister could hold the same payload, and the drain order alone decided the winner. On Blizzard's recycled nameplate pool the same frame is legitimately removed and re-added within one fight, so the drain tore down plates showing live units. Queueing one job now cancels its opposite for that payload: latest intent wins. UnregisterFrame returned early for a frame that was only QUEUED for registration, so the queued job survived and took over, one combat later, a frame the caller had explicitly opted out of. It now drops the queued entry before the early returns. SetEnabled skipped the header's dfClickCastEnabled attribute in combat with no deferral. The OnEnter snippet reads that attribute to decide whether to run, so enabling click casting mid-fight left every hover a no-op while the UI reported it working. Deferred as a new headerEnabled job, drained first. Also: ShouldBindingLoad treated a binding with no `enabled` field as disabled while the map grouping, special-action and item paths all treat absent as enabled. For a key whose bindings all lacked the field -- reachable via profile import, which normalizes nothing -- the builder dropped every one and the key got no map entry at all. Absent now means enabled everywhere. And GROUP_ROSTER_UPDATE is registered at last; this module had no roster event and relied on a hook plus a login-only scan. PLAYER_SPECIALIZATION_CHANGED is now unit-filtered to the player, so a raid member respeccing no longer costs a full ~500-frame sweep and the hover window with it. --- ClickCasting/Bindings.lua | 44 ++++++++++-- ClickCasting/Events.lua | 138 +++++++++++++++++++++++++++++++------- ClickCasting/Frames.lua | 10 ++- 3 files changed, 160 insertions(+), 32 deletions(-) diff --git a/ClickCasting/Bindings.lua b/ClickCasting/Bindings.lua index 50a03e46..5203d23c 100644 --- a/ClickCasting/Bindings.lua +++ b/ClickCasting/Bindings.lua @@ -336,7 +336,21 @@ function CC:ShouldBindingLoad(binding) -- -- Combat conditions are checked dynamically via state drivers / macro -- conditionals, not here. - return not not binding.enabled + -- + -- `~= false`, NOT `not not`. A binding with no `enabled` field at all was + -- read three different ways: the map grouping and the special-action and + -- item paths all treat absent as enabled (`enabled ~= false`), while this + -- helper treated it as disabled. So for a key whose bindings ALL lack the + -- field, the spell/macro builder dropped every one, produced no macro text, + -- and the key got no map entry -- completely dead, while the binding list + -- showed it as present and enabled. + -- + -- Reachable because nothing normalizes the field on the way in: the login + -- pass fixes up `frames`/`fallback`/`combat` but not `enabled`, and profile + -- import inserts bindings verbatim. Absent now means enabled everywhere, + -- which matches the majority of the existing paths and the UI's own + -- backwards-compatibility default. + return binding.enabled ~= false end -- Every modifier prefix combination we ever write, in SecureActionButtonTemplate @@ -699,9 +713,23 @@ function CC:ApplyBindings() local endIdx = math.min(startIdx + BATCH_SIZE - 1, #allFrames) for i = startIdx, endIdx do - -- quiet=true: one summary line per sweep instead of three per - -- frame. See the note on that summary below. - CC:ApplyBindingsToFrameUnified(allFrames[i], true, true) + -- skipKeyboardUpdate=FALSE deliberately. Deferring the snippet + -- rebuild to the batch tail left every already-processed frame + -- holding the OUTGOING snippet while its outgoing + -- type- attributes had already been erased -- so + -- the keys that snippet binds pointed at cleared attributes: + -- dead AND stolen from the action bar, for the rest of the + -- sweep, or for the whole fight if combat interrupted it. The + -- hovered-frame hoist above repairs exactly one frame. + -- + -- Rebuilding inline costs nothing: RefreshKeyboardBindings at + -- the tail already calls UpdateFrameBindingAttributes once per + -- registered frame, so this is the same N calls moved earlier. + -- It also closes the window the old comment worried about -- + -- clear and rebuild now happen inside one call with no yield + -- between them, so combat can no longer land in the gap. + -- quiet=true keeps it to one summary line per sweep. + CC:ApplyBindingsToFrameUnified(allFrames[i], false, true) applied = applied + 1 end @@ -1334,9 +1362,17 @@ function CC:SetEnabled(enabled) -- Update the header attribute so secure snippets know whether to run -- This is critical for allowing Clique/Clicked to work when we're disabled + -- + -- The OnEnter snippet reads this attribute to decide whether to run at all, + -- so if the write is skipped the DB says enabled and every hover no-ops. It + -- used to be skipped silently in combat with no deferral: toggling click + -- casting on during a fight left it dead until something else happened to + -- rewrite the attribute, while the UI reported it working. if self.header then if not InCombatLockdown() then self.header:SetAttribute("dfClickCastEnabled", enabled) + else + self:Defer("headerEnabled", enabled and "on" or "off") end end diff --git a/ClickCasting/Events.lua b/ClickCasting/Events.lua index 8c7b2c77..dbb2b384 100644 --- a/ClickCasting/Events.lua +++ b/ClickCasting/Events.lua @@ -12,7 +12,14 @@ function CC:RegisterEvents() eventFrame:RegisterEvent("PLAYER_REGEN_ENABLED") eventFrame:RegisterEvent("PLAYER_REGEN_DISABLED") - eventFrame:RegisterEvent("PLAYER_SPECIALIZATION_CHANGED") + -- Unit-filtered to "player". This event carries a unit and fires for every + -- party/raid member, and the handler runs CheckLoadoutProfileSwitch plus a + -- full ApplyBindings -- the ~500-frame batched sweep. Unfiltered, every + -- raid member respeccing cost a full sweep and the hover-bind window that + -- comes with it, for a profile decision that only ever concerns us. + eventFrame:RegisterUnitEvent("PLAYER_SPECIALIZATION_CHANGED", "player") + -- Roster churn is when frames are created and retired; see the handler. + eventFrame:RegisterEvent("GROUP_ROSTER_UPDATE") eventFrame:RegisterEvent("PLAYER_ENTERING_WORLD") eventFrame:RegisterEvent("PLAYER_LEVEL_UP") eventFrame:RegisterEvent("PLAYER_EQUIPMENT_CHANGED") @@ -89,9 +96,60 @@ function CC:RegisterEvents() CC:RefreshSpellGrid() end elseif event == "PLAYER_ENTERING_WORLD" then - -- Initial load or reload. Keyed: back-to-back loading screens - -- would otherwise stack several settle passes over each other. - CC:DeferAfter("zoneSettle", 0.5, function() + CC:ScheduleZoneSettle() + elseif event == "GROUP_ROSTER_UPDATE" then + -- Roster churn creates and retires frames -- including third-party + -- ones, whose only route in is the ClickCastFrames table that cannot + -- report a retry (see ReconcileClickCastFrames). This module used to + -- register no roster event at all and relied entirely on the + -- SecureUnitButton_OnLoad hook plus a login-only scan, which is how a + -- party->raid change could leave frames dead until a /reload. + if InCombatLockdown() then + CC:Defer("bindingRefresh") + else + CC:ReconcileClickCastFrames() + CC:ApplyBindings() + end + elseif event == "ARENA_PREP_OPPONENT_SPECIALIZATIONS" then + -- Arena frames should now exist + -- Belt: never enter an arena on an unresolved cold-start profile + CC:ResolveColdStartProfile("arena-prep") + CC:OnArenaPrep() + elseif event == "INSTANCE_ENCOUNTER_ENGAGE_UNIT" then + -- Boss frames should now exist + CC:OnBossEngage() + elseif event == "NAME_PLATE_UNIT_ADDED" then + -- A nameplate was added + local unitToken = ... + CC:OnNamePlateAdded(unitToken) + elseif event == "NAME_PLATE_UNIT_REMOVED" then + -- A nameplate was removed + local unitToken = ... + CC:OnNamePlateRemoved(unitToken) + elseif event == "HOUSE_EDITOR_MODE_CHANGED" then + -- Housing mode transitions can kill secure wraps; the repair + -- re-wraps every frame (self-defers in combat, cooldown-limited) + CC:RequestBindingRepair("housing-mode") + end + end) + + -- Our own PLAYER_ENTERING_WORLD registration happens INSIDE the dispatch of + -- that very event (Initialize is driven from a PEW handler elsewhere, which + -- calls InitializeSecureFrames -> RegisterEvents), so this frame never + -- receives the login PEW. Everything in the settle pass was therefore absent + -- at login and first ran on the next loading screen: nameplate registration, + -- the zone-in binding repair, and the cold-start profile resolve that exists + -- specifically for "none of my binds work in my first arena of the day". + -- Kick it once here; the key makes it idempotent against a real PEW landing + -- immediately after. + self:ScheduleZoneSettle() +end + +-- The post-loading-screen settle pass, factored out so it can also be kicked +-- once at init (see the note at the end of RegisterEvents). Keyed: back-to-back +-- loading screens reuse one pending pass rather than stacking several. +function CC:ScheduleZoneSettle() + CC:DeferAfter("zoneSettle", 0.5, function() -- Run one-time migration to convert bindings to root spells CC:MigrateBindingsToRootSpells() @@ -133,28 +191,6 @@ function CC:RegisterEvents() CC:CheckLoadoutProfileSwitch() end) end) - elseif event == "ARENA_PREP_OPPONENT_SPECIALIZATIONS" then - -- Arena frames should now exist - -- Belt: never enter an arena on an unresolved cold-start profile - CC:ResolveColdStartProfile("arena-prep") - CC:OnArenaPrep() - elseif event == "INSTANCE_ENCOUNTER_ENGAGE_UNIT" then - -- Boss frames should now exist - CC:OnBossEngage() - elseif event == "NAME_PLATE_UNIT_ADDED" then - -- A nameplate was added - local unitToken = ... - CC:OnNamePlateAdded(unitToken) - elseif event == "NAME_PLATE_UNIT_REMOVED" then - -- A nameplate was removed - local unitToken = ... - CC:OnNamePlateRemoved(unitToken) - elseif event == "HOUSE_EDITOR_MODE_CHANGED" then - -- Housing mode transitions can kill secure wraps; the repair - -- re-wraps every frame (self-defers in combat, cooldown-limited) - CC:RequestBindingRepair("housing-mode") - end - end) end -- ============================================================ @@ -182,6 +218,9 @@ end -- binding refresh that walks registered frames). local DRAIN_ORDER = { + -- First: the OnEnter snippet reads dfClickCastEnabled to decide whether to + -- run at all, so a stale value makes everything below it pointless. + "headerEnabled", "profileSwitch", "loadoutCheck", "register", @@ -196,6 +235,18 @@ local DRAIN_ORDER = { } local DEFERRED_JOBS = { + -- Carries the enabled state SetEnabled could not write during lockdown. + -- "last" wins: if the user toggled twice in one fight, the final state is + -- the one they meant. Stored as a string because Defer treats a nil payload + -- as "nothing queued", which would silently drop a toggle to OFF. + headerEnabled = { + kind = "value", policy = "last", + run = function(self, state) + if self.header then + self.header:SetAttribute("dfClickCastEnabled", state == "on") + end + end, + }, profileSwitch = { kind = "value", policy = "last", run = function(self, profileName) @@ -286,6 +337,25 @@ local DEFERRED_JOBS = { -- job name must not read as "queued", or the caller aborts and the work is lost -- with only an INFO line to show for it -- and INFO is exactly what the log's -- eviction policy discards first. +-- Jobs that mean the opposite of each other. Queueing one must CANCEL the other +-- for that payload, because the queue is otherwise order-blind: both entries +-- survive, and DRAIN_ORDER alone decides the outcome -- so the later intent +-- loses whenever it happens to sit earlier in the order. +-- +-- The case that bites is nameplates. Blizzard recycles a fixed pool of plate +-- frames, so within one fight the SAME frame object is legitimately removed and +-- re-added for different units. Both sets end up holding it, `register` drains +-- at slot 3 and `unregister` at slot 4, and the drain tears down a plate that is +-- on screen showing a live unit. The Blizzard-frame pair has the same shape from +-- a user toggling the option twice in combat: whatever they picked last, off +-- wins. +local OPPOSED_JOBS = { + register = "unregister", + unregister = "register", + blizzardRegister = "blizzardUnregister", + blizzardUnregister = "blizzardRegister", +} + function CC:Defer(job, payload) local def = DEFERRED_JOBS[job] if not def then @@ -295,6 +365,22 @@ function CC:Defer(job, payload) self.deferred = self.deferred or {} + -- Latest intent wins: drop the contradicting entry rather than letting + -- DRAIN_ORDER arbitrate between two things the caller never asked for both of. + local opposite = OPPOSED_JOBS[job] + if opposite and self.deferred[opposite] ~= nil then + local other = self.deferred[opposite] + if type(other) == "table" then + if payload ~= nil and other[payload] then + other[payload] = nil + DF:Debug("CLICK", "Defer: '%s' cancels queued '%s' for the same frame", job, opposite) + end + else + self.deferred[opposite] = nil + DF:Debug("CLICK", "Defer: '%s' cancels queued '%s'", job, opposite) + end + end + if def.kind == "set" then local set = self.deferred[job] if type(set) ~= "table" then diff --git a/ClickCasting/Frames.lua b/ClickCasting/Frames.lua index b98c0660..a40796ab 100755 --- a/ClickCasting/Frames.lua +++ b/ClickCasting/Frames.lua @@ -2813,9 +2813,15 @@ function CC:UnregisterFrame(frame) if not frame then return end -- Drop any deferred registration first, and unconditionally: a frame can be -- unregistered while still only PENDING (created, no unit yet, then hidden - -- before its unit arrived). Leaving it queued would let the unit-assignment - -- path resurrect a frame the caller has just opted out of. + -- before its unit arrived) or while sitting in the combat-deferred "register" + -- set. Both must go before the early returns below, because a frame in either + -- state is NOT in registeredFrames -- so this function used to return without + -- touching them, and the queued registration then took over, one combat + -- later, a frame the caller had just explicitly opted out of. if self.pendingRegistration then self.pendingRegistration[frame] = nil end + if self.deferred and type(self.deferred.register) == "table" then + self.deferred.register[frame] = nil + end if not self.registeredFrames then return end if not self.registeredFrames[frame] then return end From 3e00b3978d218b406c517e60332acd592d0daddb Mon Sep 17 00:00:00 2001 From: Krathe Date: Sun, 2 Aug 2026 17:10:40 +0100 Subject: [PATCH 10/14] Click casting: stop damaging third-party frames we cannot or should not bind Five foreign-frame faults from the audit. All of them left another addon's frame worse than we found it. Unnamed frames are now refused outright. HANDLE:SetBindingClick resolves its target through GetName() and errors on nil, so an anonymous frame can never be a hover-bind target -- but registering one was destructive rather than merely useless. ClearBlizzardClickCastFromFrame runs early and has no name check, so it wiped type1/type2 and every modifier variant, while all three functions that would have installed our bindings bail on the missing name further down. A third-party frame came out with no left-click target, no right-click menu, and nothing in their place, until a reload. If we cannot bind it, we do not touch it. unit1/unit2 are captured and restored. ClearBlizzardClickCastFromFrame nils them and nothing put them back, so a frame relying on per-button unit overrides lost that behaviour for the session. Mousewheel state is captured and restored. Every apply force-enabled the wheel and nothing ever disabled it again, so a frame that shipped with the wheel off started swallowing scroll events -- scrolling over it stopped scrolling the parent scrollframe. The pre-existing-entries loop now goes through EnsureRegistered. The eligibility gate lives there, so that loop adopted anything already parked in ClickCastFrames before our PLAYER_ENTERING_WORLD with no check whatsoever -- including a non-unit secure button, which is the precise case the gate was written for after a toy button had its type1 replaced. The third-party scan's issecretvalue guard now precedes the boolean test rather than following it. Evaluating a secret value in a condition throws, and this runs inside a C_Timer callback, so one such frame took out the remainder of the pattern list AND the ClickCastFrames sweep below it: no third-party registration at all for the session, from a single silent error at login. --- ClickCasting/Bindings.lua | 13 +++++++++- ClickCasting/Frames.lua | 53 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 61 insertions(+), 5 deletions(-) diff --git a/ClickCasting/Bindings.lua b/ClickCasting/Bindings.lua index 5203d23c..28d56e9a 100644 --- a/ClickCasting/Bindings.lua +++ b/ClickCasting/Bindings.lua @@ -557,12 +557,23 @@ function CC:RestoreBlizzardDefaults(frame) frame:SetAttribute("type2", orig.type2) frame:SetAttribute("*type1", orig.starType1) frame:SetAttribute("*type2", orig.starType2) + -- ClearBlizzardClickCastFromFrame nils these; nothing put them back, so + -- a frame relying on per-button unit overrides lost that for the session. + frame:SetAttribute("unit1", orig.unit1) + frame:SetAttribute("unit2", orig.unit2) else -- type1 = left click = target, type2 = right click = togglemenu frame:SetAttribute("type1", "target") frame:SetAttribute("type2", "togglemenu") end - + + -- Put the mousewheel back the way we found it. ApplyBindingsToFrameUnified + -- force-enables it on every apply, so without this a frame that shipped with + -- the wheel disabled kept swallowing scroll events after we handed it back. + if frame.EnableMouseWheel and frame.dfOriginalMouseWheel ~= nil then + frame:EnableMouseWheel(frame.dfOriginalMouseWheel) + end + -- Reset to standard click registration (AnyUp is default) if frame.RegisterForClicks then frame:RegisterForClicks("AnyUp") diff --git a/ClickCasting/Frames.lua b/ClickCasting/Frames.lua index a40796ab..be2975c6 100755 --- a/ClickCasting/Frames.lua +++ b/ClickCasting/Frames.lua @@ -748,13 +748,28 @@ function CC:CaptureOriginalClickBindings(frame) local t1, t2 = frame:GetAttribute("type1"), frame:GetAttribute("type2") local s1, s2 = frame:GetAttribute("*type1"), frame:GetAttribute("*type2") - if issecretvalue(t1) or issecretvalue(t2) or issecretvalue(s1) or issecretvalue(s2) then + -- unit1/unit2 are captured too: ClearBlizzardClickCastFromFrame nils them, + -- and nothing restored them. A foreign frame whose click actions relied on + -- per-button unit overrides lost that behaviour for the session. + local u1, u2 = frame:GetAttribute("unit1"), frame:GetAttribute("unit2") + if issecretvalue(t1) or issecretvalue(t2) or issecretvalue(s1) or issecretvalue(s2) + or issecretvalue(u1) or issecretvalue(u2) then frame.dfOriginalClickBindings = false else frame.dfOriginalClickBindings = { type1 = t1, type2 = t2, starType1 = s1, starType2 = s2, + unit1 = u1, unit2 = u2, } end + + -- Mousewheel state is captured separately (it is not an attribute). + -- ApplyBindingsToFrameUnified force-enables the wheel on every apply and + -- nothing ever turned it off again, so a frame that deliberately left the + -- wheel disabled started swallowing scroll events -- scrolling over it + -- stopped scrolling the parent scrollframe, for the rest of the session. + if frame.IsMouseWheelEnabled then + frame.dfOriginalMouseWheel = frame:IsMouseWheelEnabled() and true or false + end end function CC:ClearBlizzardClickCastFromFrame(frame) @@ -1039,11 +1054,18 @@ function CC:SetupClickCastFramesGlobal() end }) - -- Re-register any frames that were already in ClickCastFrames + -- Re-register any frames that were already in ClickCastFrames. + -- + -- Through EnsureRegistered, not RegisterFrame directly: the eligibility gate + -- lives in EnsureRegistered, so this loop used to register anything present + -- unconditionally. Anything an addon parked in the table before our + -- PLAYER_ENTERING_WORLD -- including a non-unit secure button, the exact + -- case the gate was written for after a toy button had its type1 replaced -- + -- was adopted with no check at all. for frame, enabled in pairs(existingFrames) do if enabled then rawset(ClickCastFrames, frame, true) - CC:RegisterFrame(frame) + CC:EnsureRegistered(frame) end end @@ -1104,8 +1126,15 @@ function CC:ScanForThirdPartyFrames() for _, frameName in ipairs(knownFramePatterns) do local frame = _G[frameName] if frame and type(frame) == "table" and frame.GetAttribute then - -- Check if it's a valid unit frame with a unit attribute + -- Check if it's a valid unit frame with a unit attribute. + -- The issecretvalue guard has to come BEFORE the boolean test, not + -- after it as it did for isProtected below: evaluating a secret value + -- in a condition throws, and this runs inside a C_Timer callback, so + -- the error took out the rest of the pattern list and the + -- ClickCastFrames sweep underneath it -- no third-party registration + -- at all for the session, from one silent error at login. local unit = frame:GetAttribute("unit") + if issecretvalue(unit) then unit = nil end if unit and not self.registeredFrames[frame] then -- Check if it's a protected secure frame local isProtected = frame.IsProtected and frame:IsProtected() @@ -1582,6 +1611,22 @@ function CC:RegisterFrame(frame) if self.registeredFrames[frame] then return end + -- An unnamed frame can never be a click-cast target. HANDLE:SetBindingClick + -- resolves its target through GetName() and errors on a nil name + -- (RestrictedFrames.lua), so no hover bind can ever point here. + -- + -- Registering one was actively destructive rather than merely useless: + -- ClearBlizzardClickCastFromFrame runs early and has no name check, wiping + -- type1/type2 and every modifier variant, while the three functions that + -- would have installed our bindings each bail on the missing name further + -- down. Net effect on a third-party frame: left-click no longer targets, + -- right-click no longer opens the menu, and nothing replaces either, until + -- a reload. If we cannot bind it, we do not touch it. + if frame.GetName and not frame:GetName() then + DF:DebugWarn("CLICK", "Refusing to register an unnamed frame — SetBindingClick requires a name") + return + end + -- Don't register during combat OR if secure frames aren't initialized yet -- (init drains the "register" job once secureFramesInitialized flips) if InCombatLockdown() or not self.secureFramesInitialized then From 26fa890383551f50d3115152a060e7d36a414e7b Mon Sep 17 00:00:00 2001 From: Krathe Date: Sun, 2 Aug 2026 17:11:42 +0100 Subject: [PATCH 11/14] Click casting: a key with only macro bindings is no longer silently dead BuildCombinedMacroForBindings builds every clause from `.spellName`, but findBestSpell can return a MACRO-type binding, which has none. A key carrying two macro bindings with different target types therefore produced no clauses, returned nil, and got no entry in the unified map -- completely dead, while the binding list showed it configured and enabled. The single-binding early returns hide it; it only bites once a key has two or more bindings that do not collapse into one category. Falls back to the single-binding builder for the best candidate instead of returning nil. A macro that ignores the friendly/hostile split is a compromise; a key that does nothing at all is a bug. --- ClickCasting/Bindings.lua | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/ClickCasting/Bindings.lua b/ClickCasting/Bindings.lua index 28d56e9a..3a95ee3e 100644 --- a/ClickCasting/Bindings.lua +++ b/ClickCasting/Bindings.lua @@ -2561,7 +2561,25 @@ function CC:BuildCombinedMacroForBindings(bindings, forGlobalBinding) end end - if #parts == 0 then return nil end + -- No clause was produced. Every clause above requires `.spellName`, but + -- findBestSpell will happily return a MACRO-type binding, which has none -- + -- so a key carrying two macro bindings with different target types built + -- nothing, returned nil, and got no entry in the unified map at all. That + -- key was completely dead while the binding list showed it as configured. + -- The single-binding early returns above hide it; it only bites once a key + -- has two or more bindings that do not collapse to one category. + -- + -- Fall back to the single-binding builder for the best candidate we have. A + -- macro that ignores the friendly/hostile split is a compromise; a key that + -- does nothing at all is a bug. + if #parts == 0 then + local fallbackBinding = anyBinding or friendlyBinding or hostileBinding + if fallbackBinding then + DF:Debug("CLICK", "Combined macro produced no clauses (macro-type binding); using single-binding build") + return self:BuildMacroTextForBinding(fallbackBinding, forGlobalBinding), fallbackBinding + end + return nil + end -- Check if any contributing binding has stopSpellTarget enabled local useStopSpellTarget = false From be98106a82ce14ab6cbc025dd9e74ea85f3a13a8 Mon Sep 17 00:00:00 2001 From: Krathe Date: Sun, 2 Aug 2026 19:57:53 +0100 Subject: [PATCH 12/14] Click casting: honour disable-while-mounted on shared keys, and give focus/assist a global bind The last two audit findings, both wrong-binds rather than dead-binds. BuildCombinedMacroForBindings never computed the mounted/flying condition. The single-binding builder stamps ",nomounted,noflying" into every clause it emits, so "disable while mounted" worked for every key with ONE binding and silently did nothing for every key with two or more -- the option appears to work right up until the key the user cares about happens to have a friendly/hostile split. Applied as a post-pass over the finished clause list rather than threaded through ten separate concatenations: one place to be correct, and it covers the unconditional [] and terminal always-cast forms a per-site edit would have missed. Special actions were stored with no globalMacroText, and the hovercast script skips any entry without macro text. So "focus, with a target fallback" worked while hovering a frame and did nothing at all while hovering nothing -- despite the fallback being the entire reason that key needs a global bind. BuildMacroTextForBinding has had working /focus and /assist branches all along; nothing ever reached them, because the special-action break fires first. Only focus and assist gain a global form. target and menu genuinely have none: /target cannot reach cross-instance players (the reason the native handler is used on frames at all) and there is no macro equivalent of the unit menu, so both correctly remain frame-only. --- ClickCasting/Bindings.lua | 56 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/ClickCasting/Bindings.lua b/ClickCasting/Bindings.lua index 3a95ee3e..52cb7533 100644 --- a/ClickCasting/Bindings.lua +++ b/ClickCasting/Bindings.lua @@ -2561,6 +2561,39 @@ function CC:BuildCombinedMacroForBindings(bindings, forGlobalBinding) end end + -- Mounted / flying suppression. + -- + -- The single-binding builder stamps ",nomounted,noflying" into every clause + -- it emits. This builder never computed it at all, so "disable while + -- mounted" worked for every key with ONE binding and silently did nothing + -- for every key with two or more -- the user sees the option working, right + -- up until the key they care about happens to have a friendly/hostile split. + -- + -- Applied as a post-pass over the finished clause list rather than threaded + -- through the ten separate concatenations above: one place to be correct, + -- and it covers the unconditional [] and terminal always-cast forms that a + -- per-site edit would have missed. + local mountedStr = "" + if self.db and self.db.global and self.db.global.disableWhileMounted then + mountedStr = ",nomounted,noflying" + elseif self.db and self.db.global and self.db.global.disableWhileFlying then + mountedStr = ",noflying" + end + if mountedStr ~= "" then + local bare = mountedStr:sub(2) -- drop the leading comma + for i, part in ipairs(parts) do + local cond, rest = part:match("^%[(.-)%]%s*(.*)$") + if cond == nil then + -- No bracket at all (always-cast with no combat condition). + parts[i] = "[" .. bare .. "] " .. part + elseif cond == "" then + parts[i] = "[" .. bare .. "] " .. rest + else + parts[i] = "[" .. cond .. mountedStr .. "] " .. rest + end + end + end + -- No clause was produced. Every clause above requires `.spellName`, but -- findBestSpell will happily return a MACRO-type binding, which has none -- -- so a key carrying two macro bindings with different target types built @@ -2689,8 +2722,31 @@ function CC:BuildUnifiedMacroMap() -- 2. Macro-based targeting (/target) does NOT work for cross-instance players -- 3. PreClick handlers can't check unit state (UnitIsDeadOrGhost not available in restricted Lua) -- Smart res still works on healing spell bindings - click dead player with heal = casts res + -- globalMacroText is what the HOVERCAST button binds, and it is a + -- separate question from how the action behaves ON a frame. + -- + -- On a frame these use native WoW handling (type="target" etc), so + -- macroText stays nil deliberately. But the hovercast script skips + -- any entry with no macro text at all, so "focus, with a target + -- fallback" worked while hovering a frame and did nothing at all + -- while hovering nothing -- despite the fallback being the entire + -- reason that key needs a global bind. BuildMacroTextForBinding has + -- had working /focus and /assist branches the whole time; nothing + -- ever reached them, because this break fires first. + -- + -- Only for the actions that have a macro form. target and menu do + -- not: /target cannot reach cross-instance players (the note below) + -- and there is no macro equivalent of the unit menu, so those two + -- correctly remain frame-only. + local specialType = specialBinding.actionType + local hasMacroForm = (specialType == "focus" or specialType == "assist" + or specialType == self.ACTION_TYPES.FOCUS + or specialType == self.ACTION_TYPES.ASSIST) + macroMap[keyString] = { macroText = nil, + globalMacroText = hasMacroForm + and self:BuildMacroTextForBinding(specialBinding, true) or nil, templateBinding = specialBinding, keyString = keyString, isSpecialAction = true, From 95c73bbcd682cb935f557b944786f37caef74848 Mon Sep 17 00:00:00 2001 From: Krathe Date: Sun, 2 Aug 2026 20:04:20 +0100 Subject: [PATCH 13/14] Click casting: remove code that could never run SetupHovercastButtonAttributes wrote attributes nothing could read. It named its slots with GetVirtualButtonName ("type-shiftmouse3") while the bindings that actually reach that button are installed by BuildHovercastSetupScript using GetHovercastSuffix ("type-dfmouseshift3") -- two disjoint namespaces on one button, so no click or key ever resolved to anything it set. Its clear loop had the same problem in reverse, clearing type1..5 / spell1..5 / macrotext1..5 which nothing on that button writes, so its own attributes accumulated untouched. The button is EnableMouse(false) as well, so it cannot be clicked at all. Deleting it also closes an unbounded leak: it called AddCombatConditional on the hovercast button, appending to a dfAttrDriverList that nothing ever unregistered or cleared -- it grew on every ApplyBindings for the whole session. The real hovercast path, ApplyGlobalBindings -> BuildHovercastSetupScript, is untouched. Also removed a `:gsub("BUTTON", "BUTTON")` that read as deliberate normalisation and only ever uppercased, and the ACTION_TYPES.FOLLOW branch in the action-name lookup -- ACTION_TYPES has no FOLLOW member, so that comparison was `actionType == nil` and a binding with no action type displayed as "Follow Unit" rather than falling through to "Unknown". Left in place: the uniqueKeys guard in BuildHovercastSetupScript. It is redundant (it dedupes the table key it is already iterating by, so it cannot fire twice) but removing it means unwinding a nested if inside a string-builder for no functional gain, which is churn risk without benefit. --- ClickCasting/Bindings.lua | 98 ++++++++++----------------------------- 1 file changed, 24 insertions(+), 74 deletions(-) diff --git a/ClickCasting/Bindings.lua b/ClickCasting/Bindings.lua index 52cb7533..4eec5de1 100644 --- a/ClickCasting/Bindings.lua +++ b/ClickCasting/Bindings.lua @@ -645,9 +645,6 @@ function CC:ApplyBindings() -- Build unified macro map (all bindings converted to macros) self.unifiedMacroMap = self:BuildUnifiedMacroMap() - -- Set up hovercast button attributes for third-party frame support - self:SetupHovercastButtonAttributes() - -- IMPORTANT: Clear Blizzard click-casting BEFORE applying our bindings -- This ensures our bindings take precedence and aren't overwritten if self.db.enabled then @@ -881,74 +878,24 @@ function CC:CreateHovercastButton() end) end --- Set up the hovercast button with spell attributes for third-party frame click casting --- This is called after bindings are built so the hovercast button can handle redirected clicks -function CC:SetupHovercastButtonAttributes() - if not self.hovercastButton then return end - -- Combat-safe by contract: only reached from ApplyBindings, which defers. - if InCombatLockdown() then return end - - local btn = self.hovercastButton - - -- Clear existing attributes - for i = 1, 5 do - btn:SetAttribute("type" .. i, nil) - btn:SetAttribute("spell" .. i, nil) - btn:SetAttribute("macrotext" .. i, nil) - end - - if not self.unifiedMacroMap then return end - - -- Set up attributes for each mouse binding - for keyString, data in pairs(self.unifiedMacroMap) do - local binding = data.templateBinding - local bindType = binding.bindType or "mouse" - - if bindType == "mouse" and binding.button then - local virtualBtn = self:GetVirtualButtonName(binding) - local actionType = binding.actionType or self.ACTION_TYPES.SPELL - - -- Check if this should be treated as a special action - local isSpecialAction = data.isSpecialAction - if isSpecialAction == nil then - isSpecialAction = (actionType == "menu" or actionType == "target" or - actionType == "focus" or actionType == "assist" or - actionType == self.ACTION_TYPES.MENU or - actionType == self.ACTION_TYPES.FOCUS or - actionType == self.ACTION_TYPES.ASSIST) - end - - if isSpecialAction then - if actionType == "menu" or actionType == self.ACTION_TYPES.MENU then - local typeAttr = "type-" .. virtualBtn - btn:SetAttribute(typeAttr, "togglemenu") - -- BUG #10 FIX: state-driver-based combat conditional - local combatCond = GetCombatCondition(binding) - if combatCond then - AddCombatConditional(btn, typeAttr, "togglemenu", combatCond) - end - elseif actionType == "target" then - local typeAttr = "type-" .. virtualBtn - btn:SetAttribute(typeAttr, "target") - -- BUG #860 FIX: state-driver-based combat conditional - local combatCond = GetCombatCondition(binding) - if combatCond then - AddCombatConditional(btn, typeAttr, "target", combatCond) - end - elseif actionType == "focus" or actionType == self.ACTION_TYPES.FOCUS then - btn:SetAttribute("type-" .. virtualBtn, "focus") - elseif actionType == "assist" or actionType == self.ACTION_TYPES.ASSIST then - btn:SetAttribute("type-" .. virtualBtn, "assist") - end - else - -- Use macro for all spell/macro bindings - -- This supports smart res, combat conditionals, fallbacks, etc. - btn:SetAttribute("type-" .. virtualBtn, "macro") - btn:SetAttribute("macrotext-" .. virtualBtn, data.macroText) - end - end - end -end +-- SetupHovercastButtonAttributes was removed here (2026-08-02): it wrote +-- attributes nothing could ever read. +-- +-- It named its slots with GetVirtualButtonName ("type-shiftmouse3"), while the +-- bindings that actually reach this button are installed by +-- BuildHovercastSetupScript using GetHovercastSuffix ("type-dfmouseshift3"). +-- Two disjoint namespaces on one button, so no click or key ever resolved to +-- anything it set. Its clear loop had the same problem in reverse: it cleared +-- type1..5 / spell1..5 / macrotext1..5, which nothing on this button writes, +-- so its own attributes accumulated untouched for the session. The button is +-- also EnableMouse(false), so it cannot be physically clicked either. +-- +-- Deleting it also closes an unbounded leak: it called AddCombatConditional on +-- the hovercast button, appending to a dfAttrDriverList that nothing ever +-- unregistered or cleared, growing on every ApplyBindings for the whole session. +-- +-- The real hovercast path is ApplyGlobalBindings -> BuildHovercastSetupScript, +-- which is unaffected. -- Get the suffix for a binding (like Clique's GetBindingPrefixSuffix) -- For global bindings, returns something like "dfbuttonshiftf" or "dfmouseshift3" @@ -1232,7 +1179,9 @@ function CC:GetBindingKeyString(binding) if num then mapped = "BUTTON" .. num else - mapped = binding.button:upper():gsub("BUTTON", "BUTTON") + -- Was `:gsub("BUTTON", "BUTTON")` — a no-op that read as + -- deliberate normalisation. It only ever uppercased. + mapped = binding.button:upper() end end key = key .. mapped @@ -1475,8 +1424,9 @@ function CC:GetBindingActionText(binding) return "Open Menu" elseif actionType == self.ACTION_TYPES.FOCUS then return "Focus Unit" - elseif actionType == self.ACTION_TYPES.FOLLOW then - return "Follow Unit" + -- The FOLLOW branch was removed: ACTION_TYPES has no FOLLOW member, so the + -- comparison was `actionType == nil` and a binding with no action type at + -- all displayed as "Follow Unit" instead of falling through to "Unknown". elseif actionType == self.ACTION_TYPES.ASSIST then return "Assist Unit" else From 0fe47f798137489fb25d7aad60663df05bf84fed Mon Sep 17 00:00:00 2001 From: Krathe Date: Mon, 3 Aug 2026 14:48:43 +0100 Subject: [PATCH 14/14] =?UTF-8?q?Click=20casting:=20address=20review=20?= =?UTF-8?q?=E2=80=94=20throttle=20the=20roster=20refresh,=20drop=20the=20d?= =?UTF-8?q?uplicate=20rebuild?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three items from review, two of them costs this change set introduced. GROUP_ROSTER_UPDATE called ApplyBindings immediately. ApplyBindings cancels any in-flight batch walker and re-wipes the header's override bindings before restarting from batch 0, so one call per roster event means a burst -- a raid forming, mass join/leave, role assignment, zone-in -- can restart the sweep faster than it completes, killing the live hover binds again on every restart. That is the same dead-key class this work exists to close, reached through a trigger I added. Now a keyed DeferAfter, the same shape as zoneSettle, so a burst coalesces into one pass. Nothing is lost by the delay: frames created during a roster event register through EnsureRegistered and the ClickCastFrames metatable, not through ApplyBindings, which only refreshes frames already registered. The batch tail called RefreshKeyboardBindings after the walk, which rebuilt every snippet a second time -- the batch now does it inline per frame. Checked before removing: that function's entire body is one loop over the same registry calling the same builder, additionally gated on dfKeyboardHandlersSetup, so it is a strict subset of what the batch already did and nothing else depended on it running once at the end. Its other six callers are untouched. Test frames are now excluded from eligibility explicitly. They carry dfIsDandersFrame "for consistency with live frames", so the unit-less exemption would have accepted them. In practice they are unreachable -- TestMode never calls RegisterFrameWithClickCast, RegisterAllFrames only walks header children, and they set `frame.unit` as a plain field rather than the unit attribute eligibility reads, so the old test refused them too. But leaving it to the absence of a caller is not a guard, and they are plain Buttons rather than secure unit buttons, so they should never be adopted. --- ClickCasting/Bindings.lua | 12 ++++++++++-- ClickCasting/Events.lua | 20 ++++++++++++++++++-- ClickCasting/Frames.lua | 11 +++++++++++ 3 files changed, 39 insertions(+), 4 deletions(-) diff --git a/ClickCasting/Bindings.lua b/ClickCasting/Bindings.lua index 4eec5de1..9216710c 100644 --- a/ClickCasting/Bindings.lua +++ b/ClickCasting/Bindings.lua @@ -765,9 +765,17 @@ function CC:ApplyBindings() -- More batches to process CC.batchBindingTimer = C_Timer.NewTimer(0, ProcessNextBatch) else - -- All frames processed - refresh keyboard bindings once for all frames + -- All frames processed. CC.batchBindingTimer = nil - CC:RefreshKeyboardBindings() + -- No RefreshKeyboardBindings here any more. The batch now + -- rebuilds each frame's snippet inline (skipKeyboardUpdate is + -- false above), and RefreshKeyboardBindings does nothing but + -- loop the same registry calling the same builder -- a strict + -- subset, since it additionally gates on + -- dfKeyboardHandlersSetup. Keeping it meant every sweep built + -- every snippet twice. Verified there is no other dependency: + -- the function's whole body is that one loop. + -- -- The header wipe above kills a live hover; put it straight back. CC:ReassertHoverBinds() diff --git a/ClickCasting/Events.lua b/ClickCasting/Events.lua index dbb2b384..cdf90648 100644 --- a/ClickCasting/Events.lua +++ b/ClickCasting/Events.lua @@ -107,8 +107,24 @@ function CC:RegisterEvents() if InCombatLockdown() then CC:Defer("bindingRefresh") else - CC:ReconcileClickCastFrames() - CC:ApplyBindings() + -- Keyed and delayed, NOT immediate. ApplyBindings cancels any + -- in-flight batch walker and re-wipes the header's override + -- bindings before restarting from batch 0. Calling it once per + -- roster event means a burst -- a raid forming, mass join/leave, + -- role assignment, zone-in -- can restart the sweep faster than + -- it completes, and every restart kills the live hover binds + -- again. That is the same dead-key class this whole change set + -- exists to close, reached through a new trigger. + -- + -- Nothing is lost by waiting: frames created during the roster + -- event register through EnsureRegistered and the ClickCastFrames + -- metatable, not through ApplyBindings, which only refreshes + -- bindings on frames already registered. Same keyed-DeferAfter + -- shape as zoneSettle, so a burst coalesces into one pass. + CC:DeferAfter("rosterSettle", 0.5, function() + CC:ReconcileClickCastFrames() + CC:ApplyBindings() + end) end elseif event == "ARENA_PREP_OPPONENT_SPECIALIZATIONS" then -- Arena frames should now exist diff --git a/ClickCasting/Frames.lua b/ClickCasting/Frames.lua index be2975c6..334566d3 100755 --- a/ClickCasting/Frames.lua +++ b/ClickCasting/Frames.lua @@ -899,6 +899,17 @@ local function clickCastFrameEligible(frame) -- not, there is nothing left to do. A unit-less frame is hidden by -- RegisterUnitWatch and cannot be hovered, so its binds never activate until -- it actually holds someone. + -- Test-mode frames are excluded explicitly. They carry dfIsDandersFrame "for + -- consistency with live frames" (TestMode/TestFramePool.lua), but they are + -- deliberately plain Buttons rather than secure unit buttons, and they set + -- `frame.unit` as a plain field rather than the unit ATTRIBUTE this function + -- reads -- so the old test would have refused them anyway. Nothing currently + -- registers a test frame (TestMode never calls RegisterFrameWithClickCast, + -- and RegisterAllFrames only walks header children), but exempting on the + -- shared flag would have left that door held shut by nothing more than the + -- absence of a caller. + if frame.dfIsTestFrame then return false end + if frame.dfIsDandersFrame == true then return true end -- Foreign frames keep the strict test: this gate exists to stop click