Starting at the top:
Code:
local FlagTimerFrame = CreateFrame ("FRAME", "FlagTimerFrame", UIParent, nil)
That should be "Frame", not "FRAME"; the API will auto-correct it for you, but it's better to be in the habit of using the correct format.
"FlagTimerFrame" isn't a good choice for your frame's global name. Global object names, like all global variables, should be unique (so they won't conflict with other addons or the default UI) and should clearly identify which addon owns them.
You don't need to specify
nil there; just end the list of arguments with
UIParent.
Code:
FlagTimerFrame:RegisterEvent("ADDON_LOADED")
FlagTimerFrame:RegisterEvent("VARIABLES_LOADED")
FlagTimerFrame:RegisterEvent("PLAYER_ENTERING_WORLD")
FlagTimerFrame:RegisterEvent("PLAYER_LOGIN")
Registering these events currently does nothing, because you don't set an OnEvent script for your frame, so it doesn't have anything to do when an event fires. Also, based on the list of events, it looks like you want to do a one-time initialization thing. If that's the case, just use PLAYER_LOGIN and get rid of the others.
In particular, VARIABLES_LOADED has not been an appropriate event for addon loading procedures in many years, as it fires many times throughout the loading process in response to the game client receiving synced settings, macros, keybinds, and other data from the server.
However, it doesn't look like anything in your code needs to be initialized on login, so you can probably just remove all these event registrations.
On the other hand, you
do need to register for the events that fire when your PVP flag changes, and hide/show the frame appropriately. That doesn't happen on its own.
Code:
FlagTimerFrame:SetScale(1.0, 1.0)
SetScale only accepts one value, and the default scale of any object is 1, so there is no need to set the scale to 1 unless you have previously set it to something else. Just delete this line.
Code:
FlagTimerFrame:SetPoint("CENTER",ofsx,ofsy)
You don't define the
ofsx and
ofsy variables anywhere, so you are looking for these in the global namespace, where it is very likely they don't exist, or if they do exist, they were defined by another addon and may not even be numbers. Just pass "CENTER" to place the frame in the middle of the screen, or use actual numbers to offset the position.
Code:
FlagTimerFrame:Hide(true)
The Hide method does not accept any arguments. There's no need to pass
true here.
Code:
FlagTimerFrame:RegisterForDrag("RightButton")
There's nothing specifically wrong with
this line, but there's no point in registering your frame to be dragged if you're not going to use drag-specific scripts. Either change OnMouseUp/OnMouseDown to OnDragStart/OnDragStop, or don't bother registering any buttons for dragging.
Also, your motion scripts are way more complicated than they need to be; setting and checking that
isMoving key doesn't actually do anything, so there's no reason to do it.
Code:
color = RAID_CLASS_COLORS[select(2, UnitClass("player"))]
You are creating a global
color variable here, which is extremely likely to collide with leaked globals from other addons (or even the default UI). There is no reason to make this global. Add a
local keyword in front of it.
Code:
local Time = FlagTimerFrame:CreateFontString("Time", "OVERLAY", FontInstance)
"Time" is a horrible global name for anything. Font strings do not need global names, so get rid of this (use
nil) or at least give it a unique name that won't collide with everything under the sun.
You never defined any variable called
FontInstance so you are effectively passing
nil there, unless some other addon defined such a variable in the global scope, in which case it's pretty unlikely that it's actually pointing to a valid font object template name. Get rid of this.
Code:
Time:SetFont("Fonts\\Neuropol.ttf", 13, nil)
This is not a valid font path. While
you can add fonts to the top-level Fonts folder on your local machine, if you share this addon with someone else, they will get a "font not set" error. Put the font file in your addon's folder and adjust the path accordingly, eg. "Interface\\AddOns\\MyAddon\\Neuropol.ttf".
And again, there's no need to pass
nil at the end of a list of arguments. Just stop the list with the last non-nil value.
Code:
local function On_Update()
PVPTime, e = (GetPVPTimer()/1000)
PVPStringFormat()
end
You are putting both of the variables on the first line in the global scope, and again, these are some of the worst global names imaginable. Since you don't actually use these variables anywhere, or call their containing function from anywhere, just get rid of the whole thing.
Code:
local f = CreateFrame("frame")
You don't need to create a second frame to run scripts on, and you're currently creating
two extra frames, only one of which you actually do anything with.
Code:
IsPVPTimerRunning()
if IsPVPTimerRunning() then
The first function call here does absolutely nothing except waste CPU time. The second call doesn't use the value returned by the first call -- it just calls the function a second time.
Code:
PVPTimeMes = (" "..(sec>60 and math.floor(sec/60)..":" or "")..(sec%60).." ")
Again, you are putting this in the global namespace, which is bad.
Code:
function PVPStringFormat()
Time:SetText(PVPTimeMes)
end
Another global. Also, since you only call this function in one place, there's no reason for it to be a function at all. Just set the text directly in the place where you currently call this function.
---------------------------------------------------------------------------
Not tested, but this should work better, and is much cleaner:
Code:
local frame = CreateFrame ("Frame", "MyPVPFlagTimer", UIParent)
frame:SetPoint("CENTER")
frame:SetWidth(47)
frame:SetHeight(17)
frame:SetAlpha(0.8)
frame:Hide()
frame:SetClampedToScreen(true)
frame:SetMovable(true)
frame:SetUserPlaced(true)
frame:EnableMouse(true)
frame:RegisterForDrag("RightButton")
frame:SetScript("OnDragStart", function(self, button)
if button == "RightButton" then
self:StartMoving()
end
end)
frame:SetScript("OnDragStop", frame.StopMovingOrSizing)
frame:SetScript("OnHide", frame.StopMovingOrSizing)
frame:SetBackdrop({
bgFile = "Interface\\Buttons\\WHITE8x8", tile = true, tileSize = 0,
edgeFile = "Interface\\Buttons\\WHITE8x8", edgeSize = 5,
insets = { left = 2, right = 2, top = 2, bottom = 2 }
})
frame:SetBackdropColor(0,0,0,1)
frame:SetBackdropBorderColor(0,0,0,0.8)
local text = frame:CreateFontString(nil, "OVERLAY")
text:SetPoint("CENTER", 2, 0)
text:SetFont("Interface\\AddOns\\MyAddpn\\Neuropol.ttf", 13) -- Change "MyAddon" to the name of your addon's folder, and move the font file.
do
local _, class = UnitClass("player")
local color = (CUSTOM_CLASS_COLORS or RAID_CLASS_COLORS)[class]
text:SetTextColor(color.r, color.g, color.b)
end
local UPDATE_INTERVAL, updateTime = 1, 1
local function OnUpdate(self, elapsed)
updateTime = updateTime - elapsed
if updateTime < 0 then
local t = floor(GetPVPTimer() / 1000)
if t > 60 then
text:SetFormattedText("%d:%d", floor(t / 60), t % 60)
else
text:SetText(t)
end
updateTime = UPDATE_INTERVAL
end
end
frame:RegisterUnitEvent("UNIT_FACTION", "player")
frame:SetScript("OnEvent", function(self, event, unit)
if UnitIsPVPFreeForAll("player") or UnitIsPVP("player") then
local t = GetPVPTimer()
if t > 0 and t < 301000 then
updateTime = UPDATE_INTERVAL
self:SetScript("OnUpdate", OnUpdate)
else
self:SetScript("OnUpdate", nil)
text:SetText("PVP")
end
self:Show()
else
self:Hide()
end
end)