Walking before I Run....

cokedrivers · 05-04-14, 09:47 AM

Ok so instead of trying to figure out a "complete" UI I think I'm going to step back and try to just handle one addon.

So (for now) BasicUI will be put on the back shelf. I will upload the current version I have and let people us it if the like.

The single addon I will be trying to keep up is nData (WoWInterface and GitHub).

Can someone take a look at the zip file below to see if I have everything working correctly.

Thanks for everyone's help.
Coke

Phanx · 05-04-14, 07:12 PM

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.

cokedrivers · 05-04-14, 09:54 PM

Thank You Phanx for all this tips, I know it like reinventing the wheel but I guess people like what they like. I have 3k downloads for nData sence I set it up. I know not a lot but there are people that use it.

This part of the new PlacePlugin throws a error:

Code:

local area = math.ceil(i / 3)

Tell me I can not do arethmic on "i" string.

Error:

Code:

2x nData\nData-5.4.7.lua:225: attempt to perform arithmetic on local "i" (a table value)
nData\nData-5.4.7.lua:225: in function "PlacePlugin"
nData-5.4.7\Plugins\Stat1.lua:34: in function "func"
nData\nData-5.4.7.lua:301: in function <nData\nData.lua:280>
(tail call): ?
<in C code>
<string>:"safecall Dispatcher[1]":9: in function <string>:"safecall Dispatcher[1]":5
(tail call): ?
AckisRecipeList-2.5.5\libs\AceAddon-3.0\AceAddon-3.0-12.lua:558: in function "EnableAddon"
AckisRecipeList-2.5.5\libs\AceAddon-3.0\AceAddon-3.0-12.lua:651: in function <AckisRecipeList\libs\AceAddon-3.0\AceAddon-3.0.lua:636>
<in C code>
FrameXML\UIParent.lua:306: in function "UIParentLoadAddOn"
FrameXML\UIParent.lua:329: in function "CombatLog_LoadUI"
FrameXML\UIParent.lua:742: in function <FrameXML\UIParent.lua:705>

Locals:
nil

Also the current use for the PlacePlugin is nData:PlacePlugin(position, plugin) your new way shows nData:PlacePlugin(plugin, i).

So when I got the plugin it self it uses nData:PlacePlugin(db.bags, Text) how will this new way work?

Coke

Edit:
All the other adjustments are working great. Thank You

Phanx · 05-04-14, 10:30 PM

Originally Posted by cokedrivers

This part of the new PlacePlugin throws a error:

Code:

local area = math.ceil(i / 3)

Tell me I can not do arethmic on "i" string.

That means you did not make all of the other related changes, and you are still passing a string like "P3" instead of the numeric index from an ordered table.

Originally Posted by cokedrivers

Also the current use for the PlacePlugin is nData:PlacePlugin(position, plugin) your new way shows nData:PlacePlugin(plugin, i).

Yes, you need to change it to what I posted.

Originally Posted by cokedrivers

So when I got the plugin it self it uses nData:PlacePlugin(db.bags, Text) how will this new way work?

Instead of a string like "P3" you need to pass a number like 3.

Also, please stop doing this:

Code:

## Title: |cffCC3333 n|rData

Putting colors at the beginning of your TOC title breaks alphabetical sorting. If your addon is the only one that does it, you can just say "it's at the end of the list" which is only mildly annoying (though still annoying) but when every addon author does it, and uses a different color, addons end up being listed in basically random order, which is extremely obnoxious and totally defeats the purpose of alphabetizing the list in the first place.

If you want to display your addon's name with colors in-game (eg. the title on your config panel) go for it, but please do not include color codes in any strings that are used in alphabetized lists.

Phanx · 05-04-14, 11:08 PM

You'll also need to change your options to accommodate this... though after looking at them, your current setup doesn't really work anyway, because it doesn't do anything to stop users from setting every single plugin to position 5, for example, which would result in all of them being shown on top of each other.

Hardcoding all of your plugins into your options table is not ideal, as it rather defeats the point of setting up the name/constructor table, and requires a lot of writing the same thing over and over. Here's a better way; also, I made some additional changes to the other stuff to make it easier to manage

In your defaults:

Code:

		-- Stat Locations
		plugins = {
			bags			= 9,		-- show space used in bags on panel.
			system 		= 0,		-- show total memory and others systems info (FPS/MS) on panel.
			guild 		= 4,		-- show number on guildmate connected on panel.
			dur 			= 8,		-- show your equipment durability on panel.
			friends 		= 6,		-- show number of friends connected.
			spec 			= 5,		-- show your current spec on panel.
			coords 		= 0,		-- show your current coords on panel.
			pro 			= 7,		-- shows your professions and tradeskills
			stat1 		= 1,		-- Stat Based on your Role (Avoidance-Tank, AP-Melee, SP/HP-Caster)
			stat2 		= 3,		-- Stat Based on your Role (Armor-Tank, Crit-Melee, Crit-Caster)
			recount 		= 2,		-- Stat Based on Recount"s DPS
			calltoarms 	= 0,		-- Show Current Call to Arms.
		}

Define a table you will use for sorting:

Code:

local classColor, currentFightDPS
local pluginOrder = {} -- add this

In the same section, define a reusable sorting function:

Code:

local function sortPlugins(a, b)
	if not a then return true end
	if not b then return false end
	local orderA, orderB = db.plugins[a] or 100, db.plugins[b] or 100
	if orderA == orderB then
		return strupper(a) < strupper(b)
	else
		return orderA < orderB
	end
end

Then create all the plugins without placing them yet:

Code:

	self.plugins = {}
	for name, state in pairs(db.plugins) do
		if state > 0 then
			local func = self.pluginConstructors[name]
			self.plugins[name] = func and func() or nil
			tinsert(pluginOrder, name)
		end
	end

Then immediately afterward, sort them using the function you made above, and place them:

Code:

	sort(pluginOrder, sortPlugins)

	for i = 1, #pluginOrder do
		local name = pluginOrder[i]
		db.plugins[name] = i -- fix any weirdness
		self:PlacePlugin(self.plugins[name], i)
	end

For your options, AceConfig can't handle indexed tables for select values, so you'll have to use strings:

Code:

	local statposition = {
		["0"] = L["Not Shown"],
		["1"] = L["Position #1"],
		["2"] = L["Position #2"],
		["3"] = L["Position #3"],
		["4"] = L["Position #4"],
		["5"] = L["Position #5"],
		["6"] = L["Position #6"],
		["7"] = L["Position #7"],
		["8"] = L["Position #8"],
		["9"] = L["Position #9"],
	}

Code:

					DataGroup = {
						type = "group",
						order = 6,
						name = L["Text Positions"],
						guiInline  = true,
						get = function(info)
							return tostring(db.plugins[ info[#info] ])
						end,
						set = function(info, value)
							value = tonumber(value)
							local name = info[#info]
							-- Remove this plugin from the order:
							for i = 1, #pluginOrder do
								if pluginOrder[i] == name then
									tremove(pluginOrder, i)
								end
							end
							-- Re-sort the remaining plugins:
							sort(pluginOrder, sortPlugins)

							if value > 0 then
								-- Put this plugin back in the desired place,
								-- but only if it's enabled:
								tinsert(pluginOrder, name, value)
							else
								-- Mark it disabled in the db:
								db.plugins[name] = 0
							end

							-- Now re-place all the plugins, and update
							-- the db values according to the new order:
							for i = 1, #pluginOrder do
								local name = pluginOrder[i]
								db.plugins[name] = i
								self:PlacePlugin(self.plugins[name], i)
							end
						end
						args = {},
					},

Code:

	local t = options.args.datatext.args.DataGroup.args
	for name in pairs(self.pluginConstructors) do
		t[name] = {
			type = "select",
			name = name,
			values = statposition,
		}
	end

	return options

Phanx · 05-04-14, 11:19 PM

Also I peeked at one of your modules (Professions.lua), and you're leaking globals again; I assume the others have the same issue. For example, near the top you're leaking "db", "hexa" and "hexb" as globals.

Also you're doing some things wrong again:

Code:

nData:PlacePlugin(db.pro, Text)

1 - Aside from needing to update this for the ordering changes, you should be passing the plugin button/frame object to PlacePlugin, not the object's font string. Otherwise you will end up with the same problems you had before, with invisible clickable regions, etc.

2 - You should not be placing the plugin inside its constructor function anyway. Just delete this line entirely.

Code:

Text:SetFormattedText(hexa.."Professions"..hexb)

There's no reason to use SetFormattedText if you're not actually formatting anything. If you're passing a plain string that doesn't require formatting, use SetText instead.

Code:

if v ~= nil then
	local name, texture, rank, maxRank = GetProfessionInfo(v)
	Text:SetFormattedText(hexa.."Professions"..hexb)
end

If you're not using any of the info returned by GetProfessionInfo, save yourself the function call and don't call it at all.

Code:

local plugin = CreateFrame('Button', nil, Datapanel)

I was going to tell you this is pointless because "Datapanel" isn't defined in this scope... and then I looked at your other file and noticed you've reverted to using this generic name as the global name of your frame. Don't do that. Global object names, like all globals, should be unique, and should clearly identify the addon creating them. "Datapanel" doesn't satisfy either of those criteria. "nData_Datapanel" would be better -- it clearly identifies the frame as belonging to the nData addon, and it's extremely unlikely to conflict with any global created by any other addon or the default UI.

However, specifying a parent here is still pointless, because you're going to re-parent the plugin frame later anyway.

Code:

self:SetAllPoints(Text)

Instead of that, do this:

Code:

self:SetWidth(self:GetStringWidth())

Otherwise you will mess up parenting again.

cokedrivers · 05-05-14, 10:53 AM

Originally Posted by Phanx

That means you did not make all of the other related changes, and you are still passing a string like "P3" instead of the numeric index from an ordered table.

Yes, you need to change it to what I posted.

Instead of a string like "P3" you need to pass a number like 3.

Also, please stop doing this:

Code:

## Title: |cffCC3333 n|rData

Putting colors at the beginning of your TOC title breaks alphabetical sorting. If your addon is the only one that does it, you can just say "it's at the end of the list" which is only mildly annoying (though still annoying) but when every addon author does it, and uses a different color, addons end up being listed in basically random order, which is extremely obnoxious and totally defeats the purpose of alphabetizing the list in the first place.

If you want to display your addon's name with colors in-game (eg. the title on your config panel) go for it, but please do not include color codes in any strings that are used in alphabetized lists.

Originally Posted by Phanx

Also I peeked at one of your modules (Professions.lua), and you're leaking globals again; I assume the others have the same issue. For example, near the top you're leaking "db", "hexa" and "hexb" as globals.

Also you're doing some things wrong again:

Code:

nData:PlacePlugin(db.pro, Text)

1 - Aside from needing to update this for the ordering changes, you should be passing the plugin button/frame object to PlacePlugin, not the object's font string. Otherwise you will end up with the same problems you had before, with invisible clickable regions, etc.

2 - You should not be placing the plugin inside its constructor function anyway. Just delete this line entirely.

Code:

Text:SetFormattedText(hexa.."Professions"..hexb)

There's no reason to use SetFormattedText if you're not actually formatting anything. If you're passing a plain string that doesn't require formatting, use SetText instead.

Code:

if v ~= nil then
	local name, texture, rank, maxRank = GetProfessionInfo(v)
	Text:SetFormattedText(hexa.."Professions"..hexb)
end

If you're not using any of the info returned by GetProfessionInfo, save yourself the function call and don't call it at all.

Code:

local plugin = CreateFrame('Button', nil, Datapanel)

I was going to tell you this is pointless because "Datapanel" isn't defined in this scope... and then I looked at your other file and noticed you've reverted to using this generic name as the global name of your frame. Don't do that. Global object names, like all globals, should be unique, and should clearly identify the addon creating them. "Datapanel" doesn't satisfy either of those criteria. "nData_Datapanel" would be better -- it clearly identifies the frame as belonging to the nData addon, and it's extremely unlikely to conflict with any global created by any other addon or the default UI.

However, specifying a parent here is still pointless, because you're going to re-parent the plugin frame later anyway.

Code:

self:SetAllPoints(Text)

Instead of that, do this:

Code:

self:SetWidth(self:GetStringWidth())

Otherwise you will mess up parenting again.

Let me start with Thank You for Helping again, now that that is said I'm missing something or just not understanding your codeing.

I went threw and change everything you stated above but apperantly I got lost and something aint correct cause as you stated earlier I get the tooltip not the text ont he bar.

Also, please stop doing this:

The reason this is done is to keep this nData inline with the NeavUI addons (see image below).

Below is the addon zipped up with your changes (I think).

Coke