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 Reskinning of buttons that were skinned, but are in a disabled group #298

Closed
wants to merge 1 commit into from

Conversation

InfusOnWoW
Copy link

In WeakAuras we heavly reuse buttons even between groups. In the issue a button that was previously in a enabled group is now moved to a disabled group. In that case there's a skin on it (that is it has Regions), so ReSkin should apply the default skin again.

Fixes WeakAuras Issue:
WeakAuras/WeakAuras2#4168

In WeakAuras we heavly reuse buttons even between groups. In the issue a
button that was previously in a enabled group is now moved to a disabled
group. In that case there's a skin on it (that is it has Regions), so
ReSkin should apply the default skin again.

Fixes WeakAuras Issue:
WeakAuras/WeakAuras2#4168
@InfusOnWoW
Copy link
Author

@StormFX I don't know whether that's the correct fix, since I don't really have a good understanding of the Masque internals. But I think Reskin should for buttons that were at one point skinned, do the same as RemoveButton would do. That is call SkinButton(Button, Regions, false).

I suspect that AddButton should actually have a similar fix. If the group is disabled, but the button was skinned (that is has regions), then it should be skinned via SkinButton(Button, Regions, false).

But I didn't include that in this PR as I wasn't sure whether my fix is even correct.

@StormFX
Copy link
Member

StormFX commented Jan 7, 2023

ReSkin is meant to be used to reapply settings to a button that was modified outside of Masque, so should always exit if the group is disabled. I do agree, however, that Masque should account for buttons that were previously in another group in the event that the new group is disabled. The problem is that if a new button is added to a disabled group, it, too, will be "reskinned" with the default skin, which should not happen (because not all buttons have the DF art style by default). I'll have to look into it further.

@StormFX
Copy link
Member

StormFX commented Jan 7, 2023

FWIW, I'm already working on some minor API changes so I'll add this to my list. I've moved this to #299.

@StormFX StormFX self-assigned this Jan 7, 2023
@StormFX StormFX added the Under Review This issue is under review. label Jan 7, 2023
@StormFX StormFX modified the milestone: 10.0.5 Jan 7, 2023
@InfusOnWoW
Copy link
Author

Feel free to ping me if you have something to test or need more information.

@StormFX
Copy link
Member

StormFX commented Jan 16, 2023

This should be fixed in c848221 so closing. If there's any problems with it, you can let me know in #299. Thanks. :)

@StormFX StormFX closed this Jan 16, 2023
@InfusOnWoW
Copy link
Author

Thanks, I'll forward that message.

@StormFX
Copy link
Member

StormFX commented Jan 19, 2023

I'll be dropping a beta soon, so someone may want to test that specifically.

@EJeyp
Copy link

EJeyp commented Feb 2, 2023

This should be fixed in c848221 so closing. If there's any problems with it, you can let me know in #299. Thanks. :)

Hey, I have been running into an issue with my WA where some icons sometimes appeared to get rescaled randomly. After quite of bit of searching I figured out it was linked to Masque and ended up here. Now the issue seems to match this bug, as it only happens when I have masque enabled but disabled for the icons of my WA.

Trouble is, I am currently on version 10.0.5 of Masque, which I think should already have the fix you mention above?
Here's an example of it happening, all 3 lines of DoTs should look similar, but on the last one the icon for the Curse of Agony seems to be half its height and much wider than it should.
image

This is on the WotLK Classic version of WoW.

@StormFX
Copy link
Member

StormFX commented Feb 6, 2023

I've moved the conversation to #308, as to keep it separate and to avoid notification spam for others who may be subscribed to this pull request.

@SFX-WoW SFX-WoW deleted a comment from EJeyp Feb 6, 2023
@SFX-WoW SFX-WoW deleted a comment from EJeyp Feb 6, 2023
@SFX-WoW SFX-WoW deleted a comment from EJeyp Feb 6, 2023
@SFX-WoW SFX-WoW deleted a comment from EJeyp Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Under Review This issue is under review.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants