Thread: Asking for help
View Single Post
05-22-14, 07:16 PM   #2
Phanx
Cat.
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 5,617
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)
__________________
Retired author of too many addons.
Message me if you're interested in taking over one of my addons.
Don’t message me about addon bugs or programming questions.

Last edited by Phanx : 05-23-14 at 06:12 PM.
  Reply With Quote