search

happenslol / WarpDeplete

Mythic+ Timer Addon for World of Warcraft
MIT License
16 stars 21 forks source link

Properly show force % above 100% #65

Closed Aeon234 closed 7 months ago

Aeon234 commented 8 months ago

Tied to "Show forces percent above 100%" if checked, properly calculates the percentage and count of forces above 100%. Otherwise, will clamp to 100%.

Previously, this button didn't really do anything. Hasn't done so in a while as far as i could tell. The behavior it has now makes sense based on what the button says.

happenslol commented 8 months ago

It's unfortunate that we can't get extra count information from the API, since this means that we'll lose that information on relog/reload and it won't always be accurate due to combat log events not being reliable.

Also, we need to check whether the user has MDT installed, and prevent the "Show past 100%" option from being selected if it isn't, since we don't have the pull counts in that case.

If possible, I want to make sure that the current code paths (without the option enabled) are not changed to much, so that we don't have users running into bugs without changing anything. Simply not adding the mob that would push currentCount over totalCount to the currentCount and then handling the extra count separately is acceptable though, I think.

Aeon234 commented 8 months ago

Alright, we pushed the requested changes. It should flow with what you had in mind, but let us know if you're still not satisfied. Since you wanted to keep currentCount unchanged and accurate, we had to change Util.lua's Util.formatForcesText. i.e. all instances of text being formatted with currentCount, we made it currentCount + extraCount

Tested it both with MDT on and off and with it clamped and unclamped. Worked perfectly fine in all cases.

happenslol commented 7 months ago

Awesome, thanks so much for the changes! I have to be a bit stricter than I usually would in reviews for this addon, since I don't have an active WoW subscription and can't do much testing myself - So I want the code to stay as maintainable and readable as possible for other contributors.

Since you already tested this actively, I'm fine with merging this as-is. If there are bug reports that can be traced back to this, I'll just revert it and link you in the issue so you can take care of it and re-merge it, if that's fine with you?

Aeon234 commented 7 months ago

Yup not a problem!