-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix issue #309 #346
base: master
Are you sure you want to change the base?
Fix issue #309 #346
Conversation
I have separate guild for that on one of my alts. One thing: please change tabs to spaces when editing code :) |
Does it automatically create separate DKP tables and all if you are in a different guild on a different character on the same server? So that it doesnt interfere? For the indentation I am a bit confused, if I look at other code in this repo i see tabs everywhere - whats the standard for this repo? Tabs? 3 spaces? |
you need to swap them manually, i just dont care and wipe mine, ill get a sync from an officer anytime I didnt find any rules for the style and theres many different in a code so i assume standard lua (2tabs per ident) is in place, right? or i am thinking wrong? |
OK i am going to test it on my end by backing up the WTF folder beforehand and then replacing all the Monolith DKP files including savedvariables stuff. With indentation I have no idea, I didn't do Lua programming before! One shortcoming of this implementation by the way is that I couldn't find a way to check online/zone status for non-guild members on the standby list, so those will not be awarded DKP if one activates the same-zone OR online-only requirements, otherwise they will be! Is that fine? Or is there some way to check this for non-guild members? |
Fix parenthesis error
…t all into AwardDKP.lua) Fix with auto-award (boss-kill award) using incstandby option not autoincstandby to determine whether standby get DKP
OK i personally tested the code now. I significantly cleaned up the code around DKP awarding (e.g. only one function to award raid(+standby) some DKP, and reusing that function for both time-based and boss-kill DKP). And now standbys are properly handled (excluded when they are offline or in the wrong zone depending on options set). From my end this is ready to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested if it's backward compatible? This is a big rework (though kudos for it).
local curTime = time(); | ||
local curOfficer = UnitName("player") | ||
|
||
if MonDKP:CheckRaidLeader() then -- only allows raid leader to disseminate DKP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disagree. This limits functionality. It should be officer from whitelist. RL doesn't need to be the one responsible for DKP. For us ML does it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For boss-kill DKP ("autoaward") this check is in place right now, see
https://github.com/Roeshambo/MonolithDKP/blob/0f5f77db5c19c8af192a0607f8d8da9cfb32cc19/Modules/AutoAward.lua
For time-based DKP (RaidTimer.lua), its also only given as raid-leader:
MonolithDKP/Modules/RaidTimer.lua
Line 119 in 0f5f77d
if IsInRaid() and MonDKP:CheckRaidLeader() and core.IsOfficer then |
So basically nothing is changed with this pull request in this regard. Why it is like that in the first place, i am not sure but @Roeshambo should know?
We could think about changing it in the future though. But it might create problems with keeping the DKP tables in sync if multiple officers assign DKP instead of one of them that then keeps syncing changes to them.
I tested it with another player that has officer rights with the latest public release version of the addon, and it seems like he sees different DKP in the table. I think the officers would have to update, but everyone else should be fine as DKP table changes get synced to them by the officer(s). |
Can this pull request be confirmed/looked at please, @Roeshambo ? |
Fix for issue #309
Other changes:
Create function for getting all guild members and filtering according to on/offline and same-zone criteria
Better variable naming and comments
However, this is completely untested and probably doesnt work yet as I don't know how to test within WoW when I already have Monolith installed for my Guild and don't want to mess anything up with my own custom version. Can someone give advice on how to test or do it on their end?