View Single Post
05-04-14, 07:12 PM   #2
Phanx
Cat.
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 5,617
First and foremost: LibDataBroker. Why are you embedding it when your addon does not use it? Though, really, your coding life would be much simpler if you did use it, and simpler still if you just used one of the many many existing Broker bars out there, that not only have as few (CargoShip, NinjaPanel) or as many (Bazooka, DockingStation) features as you want (StatBlocks and ChocolateBar fall closer to the middle of the feature spectrum, I think) but also support any number of plugins instead of a fixed 9, support an infinitely larger variety of existing plugins, and have compatibility already built into thousands of other addons.

There are plenty of other areas of the UI you could be spending your time on that aren't 100% wheel reinvention, and most of those other areas would be more immediately rewarding, in that you would be spending more time customizing appearances and behaviors with results you could enjoy right away, instead of spending a lot of time on a largely invisible backend framework before actually doing anything interesting.

However, if you really want to keep on reinventing the wheel with a data panel:

There's no need to nil out OnInitialize, as AceAddon will only ever call it once.

Don't forget to re-set your "db" upvalue in your Refresh function if you plan to support profiles:
Code:
db = self.db.profile
Remove all of the "if db.enabled" checks from your OnEnable function, and add this in your OnInitialize function if you're keeping the in-game enable toggle:
Code:
self:SetEnabledState(db.enable)
This way AceAddon will automatically call OnEnable only if your addon is actually enabled. In your enable toggle option, you should then call self:Enable() or self:Disable() according to the state of the toggle.

You're embedding LibSharedMedia-3.0 and the very large AceGUI-3.0-SharedMediaWidgets but not using them. If you don't plan to use them, get rid of these libraries.

On a related note, don't put unrelated libraries into the same folder. The LibStub folder should ONLY contain LibStub. Put LibBasicAdjust-1.0 and tekShiner in their own folders, or just in the top-level Libs folder.

And, don't include additional copies of LibStub and CallbackHandler in other libraries, as you're doing with LibDataBroker, and do embed the deepest path you can, so for LibDataBroker, you should be embedding the inner LibDataBroker-1.1 folder that only includes the library's Lua file and a README, not the outer folder that also includes a TOC and other clutter.

You should either (a) do the panel creation -- self:CreatePanel() -- from OnInitialize insead of OnEnable, or
(b) remove the "enable" option, since CreatePanel is only designed to be called once, but if you let users toggle the addon in-game (which, honestly, seems pointless in a self-contained addon they can just disable at the addon screen, or uninstall, if they want to stop using) then OnEnable can be called over and over. Also, if you keep the "enable" option, then you should add an OnDisable method that hides all the panels and does anything else you need to do to "turn off" the addon. AceAddon/AceEvent will automatically unregister any events registered on your addon object, but you should manually unregister events registered on other frames (eg. you can't use RegisterUnitEvent on an AceAddon object because AceEvent doesn't support it, and your individual data plugins are not AceAddon objects anyway).

Unconditionally showing your frame on PLAYER_ENTERING_WORLD when you intend for it to be hidden in vehicles and pet battles may yield unexpected results if the user reloads their UI while in a vehicle or pet battle. I'd suggest changing that block as follows:
Code:
		if event == "UNIT_ENTERING_VEHICLE" or event == "PET_BATTLE_OPENING_START" then
			self:Hide()
		elseif event == "UNIT_EXITED_VEHICLE" or event == "PET_BATTLE_CLOSE" then
			self:Show()
		else
			self:SetShown(not C_PetBattles.IsInBattle() and not UnitHasVehicleUI("player"))
		end
(You may actually just be able to use the SetShown line instead of the whole block, but I'd need to test to verify that those functions return true during their respective entering/opening events.)

Rather than storing your plugin order using strings, eg. "P3", and then using a giant if-else chain on each plugin as it's created, I'd suggest using a simple indexed table to store the order:
Code:
		-- Stat Locations
		enabledPlugins = {
			"stat1",
			"recount",
			"stat2",
			"guild",
			"spec",
			"friends",
			"pro",
			"dur",
			"bags",
		},
		disabledPlugins = {
			"calltoarms",
			"coords",
			"system",
		},
Code:
	self.plugins = {}
	for i = 1, #db.enabledPlugins do
		local name = db.enabledPlugins[i]
		local func = self.pluginConstructors[name]
		if func then
			local plugin = func()
			self.plugins[name] = plugin
			self:PlacePlugin(plugin, i)
		end
	end
Code:
function nData:PlacePlugin(plugin, i)
	local area = math.ceil(i / 3)
	
	local parent
	if area == 1 then
		parent = StatPanelLeft
	elseif area == 2 then
		parent = StatPanelCenter
	elseif area == 3 then
		parent = StatPanelRight
	end
	if not parent then
		return -- somehow you passed a value > 9
	end

	plugin:SetParent(parent)
	plugin:SetPoint("TOP")
	plugin:SetPoint("BOTTOM")
	if i == (area * 3 - 2) then
		plugin:SetPoint("LEFT", 30, 0)
	else
		plugin:SetPoint("LEFT", parent[i-1], "RIGHT", 10, 0) -- adjust for padding between plugins
	end
	parent[i] = plugin
end
In your options table, keep in mind that "disabled" is inherited, so you don't need to define "disabled = function() return not db.enable end" on every single option. Just define it once in the options table root, up with your top-level get/set functions, and then override it with "disabled = false" on just the enable toggle option.

Your options table would be easier to extend and maintain if you wrote it in the desired order, instead of grouping options by type. Putting all the separators first, for example, requires reading through the order properties of the entire options table to figure out where those separators will actually appear in-game. You also have a lot of duplicate orders, which would be much easier to spot if your table was better organized.

Finally, you don't need to specify orders for every single option. Use low numbers to force important stuff like the enable toggle to the top, and negative numbers to force low-priority items like reset buttons to the bottom, and let AceGUI arrange the rest alphabetically in between.
__________________
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