Guild Management addon help

[Caellian]

Hi,

So, here's an addon i've written to help my wife manage her HUGE guild since between raiding and work, there isn't much time left.

Here's the code, i'll explain the issues after.

http://pastebin.com/WLhBsPpQ

It has been working quite well for the past month or so but it has one issue i am unable to fix myself.

I would like a way to feed whoever needs to be promoted or kicked into a table and then do what needs to be done on each of the elements of that table 1 by 1 on a timer (1 or 2 sec between each action) and then obviously remove what's done from the table.

Can anyone help me with this ?

Thanks

[Phanx]

if not (myChars or herChars) then return end

Based on this line, the code you posted will never run at all, since myChars and herChars are not defined when that line is read (or anywhere else in the file). Please post your entire, actual code when asking for help with code. Posting partial or fake code is just a waste of time -- both for the person who reads your code and makes suggestions that may not even be relevant to your real code, and for you to read the suggestions and figure out whether they are relevant.

If your real code contains private information (such as your character/server names) you can change them to dummy values, but don't alter the basic structure or the types of variables (eg. if myChars is a table in your real code, it should be a table in the code you post).

Originally Posted by Caellian I would like a way to feed whoever needs to be promoted or kicked into a table and then do what needs to be done on each of the elements of that table 1 by 1 on a timer (1 or 2 sec between each action) and then obviously remove what's done from the table.

I'd suggest using an animation group instead of your GuildManagement function. Replace the whole function with this:

http://pastebin.com/qiVTsemb

Then, anywhere you would call GuildManagement(), call timer:Play() instead.

Since you cannot pass extra variables (eg. enabled) this way, it uses a flag set on the timer object instead. By default the flag is set, so no actions will actually be performed (you'll just see prints describing the actions that would otherwise happen). To make it actually promote/remove people, set timer.testMode = false before timer:Play().

Other notes:

(1) Rather than mentally switching between 0-based and 1-based rank indices, just add 1 to the rank from GetGuildRosterInfo so it matches the 1-based rank you'll be passing to SetGuildMemberRank and you don't have to think about it.

(2) I cleaned up some of the redundant if (X and Y and Z) or (X and Y and Q) or (X and Y and A) then logic. If you really like checking X every time, I guess you can change it back, but remember that rankIndex needs to be adjsuted by +1 in anything copied from your old code.

(3) There is some basic protection against infinite looping if an action is failing. Rather than attempting the same action over and over, it should print a message telling you about the failed action, and then stop running.

(4) Finally, I did not test anything in-game, so there may be typos. If there are, and you can't figure them out on your own, please post your new complete code and the error message from BugSack.

[Caellian]

Wow, animations, this is such a neat trick, thanks Phanx.

But i see two things happening with the testing mode:

1. PROMOTE playerX nil

so, eventhough they're in the right rank (rank change being nil), it still goes through them, is it intended ?

2. It still prints loads of them at the same time, meaning that, if i disable the testing mode, it will still promote/kick a gazillion of players at the same time, correct ?

[Phanx]

http://pastebin.com/KniSdGbT

(1) Oops, I forgot to add a check to make sure a target rank was actually set before trying to do the promotion.

(2) No, I just forgot to stop the loop when faking an action in test mode (vs. actually performing the action).

Both issues should be fixed in the above paste.

[Caellian]

So, this is what it looks like now:

http://pastebin.com/CMhPYmJK

The kicking part was just fine, or so i think.

The promoting part seems to have a problem it didn't have before. Right after a reloadUI it does the promoting but if someone joins the guild, it doesn't get promoted right away, i have to reloadUI to make it happen.

[Phanx]

The way you're handling GUILD_ROSTER_UPDATE seems pretty weird in general. Rather than imposing an arbitrary 5-second delay, I'd suggest re-registering it only after all actions have completed. Change:

	-- Start it again if we did something.
	if didAction then
		self:Play()
	end

To:

	if didAction then
		-- Start it again if we did something.
		self:Play()
	else
		-- All done, start listening again:
		caelCore.management:RegisterEvent("GUILD_ROSTER_UPDATE")
	end

Then get rid of all the PLAYER_ENTERING_WORLD event stuff, and change your GUILD_ROSTER_UPDATE code to:

		self:UnregisterEvent("GUILD_ROSTER_UPDATE")

		-- No need to do this function call and comparison twice:
		local inTheGuild = GetGuildInfo("player") == "We Did It"

		if inTheGuild then
			SetGuildRosterShowOffline(true)
			GuildRosterShowOfflineButton:Disable()
		end

		if GuildFrame and not GuildFrame:IsVisible() then
			SortGuildRoster("online")
			CalendarManagement()
			if inTheGuild and (CanGuildPromote() or CanGuildRemove()) then
				-- The timer will reregister the event when it's done:
				return timer:Play()
			end
		end

		-- If we got this far we didn't run the timer, so no more changes will be made:
		self:RegisterEvent("GUILD_ROSTER_UPDATE")

Also:

(1) Instead of "if event == A then X end, if event == B then Y end" you should use "if event == A then X elseif event == B then Y end", so you're not processing a bunch of if checks that you already know won't pass.

(2) Don't use LoadAddOn directly on Blizzard addons. The calendar, in particular, can break if you load it too early. Instead, just listen for those addons' ADDON_LOADED events and delay running any code that depends on them until it fires. If you absolutely need to run that code without waiting for the user to open the calendar, use Blizzard's special methods, such as Calendar_LoadUI() and GuildFrame_LoadUI(), to ensure that any dependencies get loaded in the right order.

(3) Rather than using SPELLS_CHANGED as an initialization event, you should really use an event that's actually intended for initialization purposes, such as your addon's own ADDON_LOADED event, or PLAYER_LOGIN.

(4) Why on earth are you using a while loop with a manually-incremented index, instead of for i = 1, GetNumGuildMembers() ?

[Caellian]

A quick answer to (4) before i do any of the changes, i did that because for some reason when i used a for loop, some members were simply ignored by the addon, not promoted or kicked, nothing.

I swiched for a while loop and it worked so i assumed it was correct

[Caellian]

Well, i believe i did what you suggested, but now, nothing happen, at all.

http://pastebin.com/kr8Wf7mY

This is my currently working version

http://pastebin.com/5xWnaUcw

[Phanx]

You're never actually registering for the GUILD_ROSTER_UPDATE event. Change:

for _, event in next, {
	"PLAYER_ENTERING_WORLD",
	"PLAYER_LOGIN",
} do
	management:RegisterEvent(event)
end

to:

management:RegisterEvent("PLAYER_LOGIN")

and add:

	if event == "PLAYER_LOGIN" then
		Calendar_LoadUI()
		GuildFrame_LoadUI()
		self:RegisterEvent("GUILD_ROSTER_UPDATE")
	end

[Caellian]

So far, so good, it seems to work, but i've checked it again and if i use the for loop

for playerIndex = 1, GetNumGuildMembers() do

some members are simply being ignored (say 10% of the guild) but if i use the while loop

while GetGuildRosterInfo(playerIndex) ~= nil do

It works a lot better, only a few (say 2%) are ignored.

I have absolutely no clue why though.

[Phanx]

I'm guessing it may be related to how you return out of the whole function when you encounter someone of high rank, instead of just skipping the processing on that particular loop iteration. For example, instead of:

if rank == 1 or rank == 2 or rank == 3 then
   return
end

-- do stuff

try doing:

if rank > 3 then
   -- do stuff
end

inside your loop, so that encountering a high rank doesn't stop all further processing.

[Caellian]

You guessed right

Ok then, off for some more testing.

[ravagernl]

Originally Posted by Phanx I'm guessing it may be related to how you return out of the whole function when you encounter someone of high rank, instead of just skipping the processing on that particular loop iteration. [...] inside your loop, so that encountering a high rank doesn't stop all further processing.

[OFFTOPIC]
One should be able to use the keyword "continue" instead of "return" to continue the for loop(makes sense). It's a shame this conflicts with the BASIC "goto" programming style(yuck) in Lua.

One can however, enclose the for block in a "repeat" block to simulate "continue" by using the "break" keyword.

for i = 1, 10 do
    repeat
         if i % 2 then break end
         print(i..' is an odd number.')
    until true
end

[/OFFTOPIC]

The solution that Phanx has posted is better however, since it does away with 2 unneeded comparions.

[Caellian]

No issue so far, seems to be working perfectly, thanks Phanx.

[Caellian]

I have one more request if you don't mind:

I have this list with 2 tables, one for my characters, and one for my wife's characters.

http://pastebin.com/7A5CdaHX

The thing is, to avoid the mess, i would like to make the management addon only work for one of us if both of us are online.

Do you have any idea how i could achieve that ?

I'm guessing it's something obvious i'm overlooking but right know i can only get it to work for one of us while the other one is offline.

[ravagernl]

Originally Posted by Caellian I have one more request if you don't mind: I have this list with 2 tables, one for my characters, and one for my wife's characters. http://pastebin.com/7A5CdaHX The thing is, to avoid the mess, i would like to make the management addon only work for one of us if both of us are online. Do you have any idea how i could achieve that ? I'm guessing it's something obvious i'm overlooking but right know i can only get it to work for one of us while the other one is offline.

You could send addon messages on PLAYER_LOGIN and PLAYER_LOGOUT events to notify the other's addon and just check who has the higher rank(if same ranks, then compare by name).

You could probably use the "OFFICER" type since normal members cant promote or kick anyway.

[Caellian]

Originally Posted by ravagernl You could send addon messages on PLAYER_LOGIN and PLAYER_LOGOUT events to notify the other's addon and just check who has the higher rank(if same ranks, then compare by name). You could probably use the "OFFICER" type since normal members cant promote or kick anyway.

Ok but sending them what exactly ?

[Phanx]

Actually since you are in the same guild, you don't need to use addon comms at all. Just pick one of you to take priority. When GUILD_ROSTER_UPDATE fires, before you start the management timer, loop over the guild roster and return out if any of the other's characters are online.

Since I didn't have any work to do at work today, I couldn't stop myself from rewriting the whole thing. I gave you managing priority; if you want your wife to have priority, swap "herChars" to "myChars" and "myToons" to "herToons" in the GUILD_ROSTER_UPDATE handler. I changed it to avoid forcibly loading the GuildUI or CalendarUI, and split up your giant OnEvent handler into individual functions for each event, among other things. See the inline comments for details.

http://pastebin.com/THTFmmg9

On a side note, it wasn't clear whether the character lists were actually in the same file or not. I put them in there, but if they are actually in their own file, and you want to be able to access any of those variables from other files, you should add them to the addon's namespace table (the one you're naming "caelCore") instead of making them globals (as you were doing with the "herChars" and "myChars" variables).

[Caellian]

You are right, they are not in the same file, they are in my "lib" but yeah, as globals because i use them in several differents addons.

Now let's take a look at this and be amazed

[Phanx]

If you absolutely need globals, you should put the related items in a uniquely named table, and then access MyAddonData.myChars or caelLib.myChars instead of myChars, for example. Putting a variable in the global scope with a generic name like myChars is just asking for trouble...