Click casting: fix frames that never registered, and the pinned-frame bugs that made it fire

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