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

Feature Request: Static ChargeCooldowns #58

Closed
ZinohJoe opened this issue Apr 9, 2019 · 10 comments
Closed

Feature Request: Static ChargeCooldowns #58

ZinohJoe opened this issue Apr 9, 2019 · 10 comments
Assignees
Labels
Completed This task has been completed. Enhancement A feature request or improvement.

Comments

@ZinohJoe
Copy link

ZinohJoe commented Apr 9, 2019

https://github.com/Zinoh/Masque/commit/09482effd86b29b4443bdb9f998d1c3d9dca75c5#diff-29640bbe49e3344125235d5c78a95054L155

Simple fix.

Also, is it intended that the chargeCooldown region only be usable by Action buttons? Due to the line above:

155: if bType ~= "Action" then return end

In my local copy I simply moved that line to below the chargeCooldown related stuff to test it out in my local copy of TellMeWhen (which hasn't updated to the new parameters yet so using the Legacy setting). Wasn't sure of your intention though.

@StormFX
Copy link
Member

StormFX commented Apr 9, 2019

ChargeCooldowns are recycled, thus they're created and [re]assigned dynamically. That particular line of code exists only to catch any CCDs that are assigned to buttons when they're skinned. So the existing code is correct.

As far as the placement of the "Action" check, I've no problem with moving it down. I was unaware that any add-ons were using them outside the context of Action buttons.

@StormFX StormFX added the Enhancement A feature request or improvement. label Apr 9, 2019
@StormFX StormFX self-assigned this Apr 9, 2019
@ZinohJoe
Copy link
Author

ZinohJoe commented Apr 9, 2019

Ahh okay. TellMeWhen uses a seperate frame for chargeCooldowns. I wasn't able to get it skinned without changing that line though.

@StormFX
Copy link
Member

StormFX commented Apr 9, 2019

You're referring to the "Action" check? That's the only thing that's changed.

@ZinohJoe
Copy link
Author

ZinohJoe commented Apr 9, 2019

The change from Button -> Regions

@StormFX
Copy link
Member

StormFX commented Apr 9, 2019

The current implementation is how it's always been, so if you had to change it to get it to work, that means it's never worked.

@ZinohJoe
Copy link
Author

ZinohJoe commented Apr 9, 2019

You're right. I didn't realize it wasn't intended for Masque to skin deliberately created static charge cooldown frames the same as it does for regular cooldown frames. Assumed it was an oversight.

I have a custom circular skin I made that I use in TMW. I wanted Masque to skin TMW's charge frame so it would follow the circularedge and utilize custom edge textures if desired. It only took the change I initially mentioned here (along with moving the Action check line) and adding the following line to TMW to allow Masque to skin its charge frame.

https://github.com/Zinoh/TellMeWhen/commit/eed6e2fc27a55120c283305a5242969b8a975a08#diff-d5ef1ed81d47d0ddfff60601cf13eeb1

Edit: I only use Masque to skin WeakAuras and TellMeWhen so I wasn't aware of how the charge cooldown skinning is implemented in regular action buttons. I see that now.

@ZinohJoe
Copy link
Author

ZinohJoe commented Apr 9, 2019

I realize now that an addon like TMW should utilize the UpdateCharge API already in Masque to skin its charge frame. I'll see how to implement that in TMW and submit a ticket there. Apologies if I caused any confusion :).

@StormFX
Copy link
Member

StormFX commented Apr 9, 2019

A ChargeCooldown is just a Cooldown with its SetDrawSwipe set to false, so there's really no reason to distinguish between the two, aside from that. Even if an author uses both the edge and swipe, Masque should catch it.

If TMW uses a static ChargeCooldown, then I've no problems adding a check for that in the Regions table as that demonstrates a use case. Note that the key will be ChargeCooldown. It seems TMW actually skips that particular region anyhow, and only passes Icon, HotKey , Count and Cooldown. So you'd have to submit a request to TMW to have that region added.

I've yet to notify the author of the changes to :AddButton(), so that will need to be updated as well. As far as I'm aware, TMW doesn't provide actual clickable buttons but they have some regions that an Action button has so the logical course would be to pass "Action" as the type and then set Strict to true.

Edit: In regards to your follow-up post, if the ChargeCooldown is static (never changes its parent), there's no reason to call :UpdateCharge() unless TMW changes something that Masque has done.

@ZinohJoe
Copy link
Author

ZinohJoe commented Apr 9, 2019

That makes sense. I believe TMW utilizes a seperate frame for charges so that GCD swipes can be shown independently without interrupting the re-charge animation (which utilizes an edge w/ no swipe). As far as I can tell the charge frame does remain static upon icon creation. It does seem then in that case adding a check in the Regions table would be necessary.

Right, until now TMW didn't pass the charge frame as a skinnable frame to Masque. I was hoping to submit a ticket to change that and demonstrate it working. That sent me down the rabbit hole of making the changes that I ended up making, heh. Thanks for the advice.

@StormFX
Copy link
Member

StormFX commented Apr 9, 2019

No problem. I'm currently trying to go through my skins, but I'll make the appropriate changes that should be in the next alpha.

@StormFX StormFX added the Fixed This issue has been fixed. label Apr 10, 2019
@StormFX StormFX added Completed This task has been completed. and removed Fixed This issue has been fixed. labels Sep 1, 2020
@StormFX StormFX added this to the Release: 8.2.7 milestone Apr 27, 2022
@StormFX StormFX changed the title [FIX] Correct Charge local assignment Correct Charge Assignment Apr 27, 2022
@StormFX StormFX changed the title Correct Charge Assignment Feature Request: Static ChargeCooldowns Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Completed This task has been completed. Enhancement A feature request or improvement.
Projects
Archived in project
Development

No branches or pull requests

2 participants