Skip to content
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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Fix issue #309 #346

wants to merge 8 commits into from

Conversation

f90
Copy link
Contributor

@f90 f90 commented Mar 6, 2020

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?

Create function for getting all guild members and filtering according to on/offline and same-zone criteria
Better variable naming and comments
@lantisnt
Copy link
Contributor

lantisnt commented Mar 6, 2020

I have separate guild for that on one of my alts. One thing: please change tabs to spaces when editing code :)

@f90
Copy link
Contributor Author

f90 commented Mar 6, 2020

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?

@lantisnt
Copy link
Contributor

lantisnt commented Mar 6, 2020

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?

@f90
Copy link
Contributor Author

f90 commented Mar 6, 2020

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?

f90 and others added 5 commits March 7, 2020 14:22
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
@f90 f90 marked this pull request as ready for review March 8, 2020 01:30
@f90
Copy link
Contributor Author

f90 commented Mar 8, 2020

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.

Copy link
Contributor

@lantisnt lantisnt left a 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).

Modules/AwardDKP.lua Show resolved Hide resolved
Modules/AwardDKP.lua Outdated Show resolved Hide resolved
local curTime = time();
local curOfficer = UnitName("player")

if MonDKP:CheckRaidLeader() then -- only allows raid leader to disseminate DKP
Copy link
Contributor

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.

Copy link
Contributor Author

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:

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.

Modules/AwardDKP.lua Show resolved Hide resolved
@f90
Copy link
Contributor Author

f90 commented Mar 8, 2020

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).

@f90
Copy link
Contributor Author

f90 commented Mar 14, 2020

Can this pull request be confirmed/looked at please, @Roeshambo ?
Also I was wondering when the latest improvements will be packaged into a new release. The last release is already very long ago (December?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants