View Single Post
12-29-14, 08:59 PM   #12
Phanx
Cat.
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 5,617
There is no value in delaying any of this stuff. You can just do all your function hooking in the main chunk, since all of the functions you're hooking are defined in FrameXML and not in LoD portions of the UI. You can also just call SetScale on the BuffFrame in the main chunk, since the default UI never calls that method anywhere. Just get rid of the whole frame and event handler.

However, for future reference if you do have things that need to be delayed, I'd just use the first style. Factoring out each item into its own function is wasteful. As a general rule, if a function will only ever be called from one place (either one time or 1000 times) you should just take the code from that function, and move it to the place where you call the function instead. Otherwise you're just wasting CPU cycles by adding another function call.

The only reason to factor setup things out into separate functions would be if you were going to make each item toggle-able on the fly with in-game options, but hooksecurefunc isn't togglable -- once it's done, it can't be undone -- so you would need to check the setting inside the hook function, and there still wouldn't be any point in having a separate function that does nothing but install the hook. Overwriting SecondsToTimeAbbrev and setting scales, however, are undoable, so you could toggle those on the fly, so if you went that way, it would be sensible to factor those out into a function that either did or undid those things, eg.

Code:
local old_SecondsToTimeAbbrev = SecondsToTimeAbbrev
local new_SecondsToTimeAbbrev = function(seconds)
    -- your code here
end
function MyAddon:EnableTimeHook(enable)
    if enable then
        SecondsToTimeAbbrev = new_SecondsToTimeAbbrev
    else
        SecondsToTimeAbbrev = old_SecondsToTimeAbbrev
    end
end
A couple other issues with the code in your first post:

Code:
local origSecondsToTimeAbbrev = _G.SecondsToTimeAbbrev
local function SecondsToTimeAbbrevHook(seconds)
	origSecondsToTimeAbbrev(seconds)

	local tempTime
	if (seconds >= 86400) then
		tempTime = ceil(seconds / 86400)
		return '|cffffffff%dd|r', tempTime
	end

	if (seconds >= 3600) then
		tempTime = ceil(seconds / 3600)
		return '|cffffffff%dh|r', tempTime
	end

	if (seconds >= 60) then
		tempTime = ceil(seconds / 60)
		return '|cffffffff%dm|r', tempTime
	end

	return '|cffffffff%d|r', seconds
end
SecondsToTimeAbbrev = SecondsToTimeAbbrevHook
1. There is no reason to call the original SecondsToTimeAbbrev function inside your replacement. The only thing this function does is return a formatted string. Since you do not use the string the original function returns, calling it is just a waste of CPU time.

2. There is no reason to assign a value to your tempTime value when you just return that value on the very next line. Just return the value directly and skip creating a variable:

Code:
	if (seconds >= 86400) then
		return '|cffffffff%dd|r', ceil(seconds / 86400)
	end
3. Unless you plan to toggle the function replacement on the fly, there is no reason to assign the replacement to a different name, and then assign the global to that replacement. Just assign the replacement function directly to the global:

Code:
function SecondsToTimeAbbrev(seconds)
	-- your code here
end
4. This function is used in many places in the UI. Since you are not actually using colors to represent anything (ie. everything is just colored white) you should remove the color codes, so the time strings will inherit the color being used by the font string they are displayed in.

Code:
hooksecurefunc('AuraButton_Update', function(self, index)
For clarity I would recommend not naming the first argument for this function self. That name is typically used for frame references, but the first argument here is not a frame reference -- it's a string containing the global name of a frame. Blizzard code names this argument buttonName, and I'd personally stick with that, but names like name or selfName would also be less confusing.

Code:
	if (self:match('Debuff')) then
		duration:SetFont(font, 12, 'THINOUTLINE')
	else
		duration:SetFont(font, 12, 'THINOUTLINE')
	end
1. You're not actually doing anything differently if the icon is for a debuff, so there's no reason for this check at all, unless this is a copy-paste error and you did mean to do something differently, in which case...

2. This is a good example of why naming a string value self is confusing. My first thought when I saw this part was that your code should be raising an "attempt to call match (a nil value)" error, and this will probably be the first throught of most experienced Lua programmers who look at this code. It will also be confusing to you if don't look at this code for a few weeks, months or even years, and then come back to it to make a change in the future.

3. String operations (match, find, format, etc.) are fairly expensive and should be avoided when possible. In this case, your goal is to distinguish between buff icons and debuff icons, so you can achieve that more efficiently by checking for to see if the buffon has a symbol member -- debuff icons have it, but buff icons don't.

Code:
	if button.symbol then
		-- it's a debuff icon
	else
		-- it's a buff icon
	end
And finally, as others have been trying to communicate in a rather roundabout and confusing fashion, there is a problem with your second code:

Lua Code:
  1. local cBuffs = CreateFrame("Frame")
  2. cBuffs:RegisterEvent("ADDON_LOADED")
  3. cBuffs:SetScript("OnEvent", function(self, event, arg1)
  4.     if event == "ADDON_LOADED" and arg1 == "cBuffs" then
  5.         -- some stuff was here
  6.     end
  7.  
  8.     -- some other stuff was here
  9.    
  10.     self:UnregisterEvent("ADDON_LOADED")
  11. end)

First, not really a problem, but you don't need to check if event == "ADDON_LOADED". Your frame is not registered for any other events, so it will never receive any other events. Every event it receives will be an ADDON_LOADED event.

Second, and more importantly, the "some other stuff" and the UnregisterEvent lines are called on any ADDON_LOADED event, not just the one where arg1 == "cBuffs", so you're adding a slash command and then unregistering your event on the first event received. That's probably your addon's event, but it's not guaranteed to be, and if it's not, your frame will never receive your addon's event because you already told it to stop listening. You need to either (a) wrap the entire contents of the event handler in the check:

Code:
	if arg1 == "cBuffs" then
		-- some stuff was here

		-- some other stuff was here
	
		self:UnregisterEvent("ADDON_LOADED")
	end
or (b) save yourself some tab key pressing and just use a return statement at the top of the function:

Code:
	if arg1 ~= "cBuffs" then return end

	-- some stuff was here

	-- some other stuff was here
	
	self:UnregisterEvent("ADDON_LOADED")
__________________
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.
  Reply With Quote