diff --git a/ClickCasting/Bindings.lua b/ClickCasting/Bindings.lua index e9da8bc3..9216710c 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 @@ -361,7 +375,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 +391,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 @@ -540,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") @@ -617,15 +645,20 @@ 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 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. @@ -638,15 +671,49 @@ 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 + 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 @@ -654,7 +721,42 @@ function CC:ApplyBindings() local endIdx = math.min(startIdx + BATCH_SIZE - 1, #allFrames) for i = startIdx, endIdx do - CC:ApplyBindingsToFrameUnified(allFrames[i], 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 + + -- 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 @@ -663,11 +765,34 @@ 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() + + -- 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 @@ -761,74 +886,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" @@ -1112,7 +1187,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 @@ -1253,9 +1330,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 @@ -1347,8 +1432,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 @@ -2433,7 +2519,58 @@ function CC:BuildCombinedMacroForBindings(bindings, forGlobalBinding) end end - if #parts == 0 then return nil 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 + -- 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 @@ -2543,8 +2680,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, @@ -2721,7 +2881,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 @@ -2736,7 +2899,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") @@ -2807,7 +2972,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 @@ -2911,10 +3076,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) diff --git a/ClickCasting/Events.lua b/ClickCasting/Events.lua index 8c7b2c77..cdf90648 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,76 @@ 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 + -- 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 + -- 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 +207,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 +234,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 +251,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 +353,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 +381,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 b0acadf6..334566d3 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) @@ -855,20 +870,158 @@ 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. + -- 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 + -- 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 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; 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 @@ -898,22 +1051,32 @@ 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 }) - -- 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 @@ -974,8 +1137,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() @@ -1452,6 +1622,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 @@ -1800,7 +1986,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() @@ -1967,11 +2160,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? -- @@ -2111,10 +2311,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 @@ -2652,6 +2867,17 @@ 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) 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 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 diff --git a/Frames/Headers.lua b/Frames/Headers.lua index fafd9fb5..e45c047e 100755 --- a/Frames/Headers.lua +++ b/Frames/Headers.lua @@ -776,8 +776,37 @@ 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 + -- 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 diff --git a/Frames/Init.lua b/Frames/Init.lua index f622aa8e..144e668a 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 @@ -1829,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)