Bug: AddButton not Applying the Correct Skin

InfusOnWoW commented 5 years ago

WeakAuras is not only dynamically creating groups based on the installed auras, but also is reusing buttons. That is we have a pool of buttons that gets reused between different groups. I recently fixed that we never informed MSQ that a button moved to a new group.

But due to user reports of that leading to bugs, we need to revert that: https://github.com/WeakAuras/WeakAuras2/pull/1131

The user has configured masque to be globally disabled.

If I read the code correctly, what is happening is that on the RemoveButton (which was entirely superflous since AddButton does that already), the Button is skinned with a "Blizzard" skin.

This is typically not a problem since that skin would later be overriden by the correct skin.

Except if the new group is disabled.

This can be reproduced by with the above PR reverted, and with these two auras, which show the buffs on the player's target/focus. Applying buffs with/without target/focus should show the issues pretty much immediately.

!nBvuZjUnq4Fn9Xmd2hbsEmHyAOtodf5KR9fLtylB0eBjFwYqipC)27URSbooAA60x6mmeLvR2D1333UcEapHZSC2DYnCwfNLHFatfnMTG15FjMZEDEEUv64tVy84HdUC0WbdgpiC0OXxZZVy4LC2UEhUCu41xgmACWWbxf(PGRW9VIZe601MMfgL2XztIItIwcjTwKk5XHC2kr6lq(A1qYJnAjCGuNYOTuT5enoybCYCLwzx7xdl9wH1UgvrHSbDpMUpDgWLzYvT55j7QHOEF0dlM(4dW(0)kABeHyDillNLz9X12UsUrQDm4uQx5SNNCdl5zwYnlHG1IjLvxk2bbFVRlAKKRSfrpabNSbjtkkDR5mTOsA7R0w9HsZ5n1iYuTwahgmacPSmVdMsMVakXsvH(aK1yCcey4XGVLgbXuTw5ZPLcRLVIUmPOXQ2sNQpR(Dp1Qv9M8uJoKRuvcNKcwQOuIqkBLPjdVYE2PrwafHhtZ2bxqvksF1WjmaxX0KxYxRf6mzg)wOcaY0j0UPaXAG4S8M7M9iRpUDQN4GraVcAGPav3aWgWJcNGkaVcAkA1Nx2KLrrXK8GWterGSKRk884bn1mnfCy)wm4jB(ZR)TXlE6Q6)avrozJwu(eOEiCniGctMQx)HisRdY986JnrGeEIJuOSmqqHgF2RVSsms2E5gHkWzRek9h379A(pM)E(EFJqrj0fdmPt(QlyQbK1iPhheEONfbMZ2FMAmLzMTA2wfKaKeBTotvceQhRZqjcRT7V9UgLvqcN)DTVGagRZjMsqA0xSrAXQsV45fPS(guy7wIaaLG))I)WehRWH(sqXHBgXj4Nq67prFpe)21DN7iGBNNKm)ZlN9R3N43iezUPLIciBZFm5HzXrC2APQynqFJgEwfA)0H)7deoBpYrdQovVGcKJyp6cmbUaaSxrtgNfZMDxKhA8nZRADoJE(gzdmATdm(aGw6hXLdJe6QK)XZC84nfeGdpaI9kBvzWyDc23Zn(UQqKjkRx7NyTVL7uIJ2GZ(f7rbaM51OE77F93Bfz4mVV)1KKFEi4BgtLVFT1z2JTqOQ9ZnZnql6rz(8rLoupbT6DgTsoEcjtJqd)wRO6llUDR4juEa0ML06V37W21qFqj0KyPc)hEa(9EY9Kxt)BEZ9hFd)WtUDVy3bmN73cCMhLPXxBKjEJF2KbC7fbdOEbuoC7p)eH1PsFz3DDti2pe7CYEkdBaaAyiQ24)1d

!nBvxVjoou0Fn7JvIKY225rGgMbjMaloDhP9bZ4K4eSQJDuSdu6dZV99ETtGSSSDz1(YiHOP3C)YNZ5En0aAcLyOKN57PKkkjh)aMkB0haRR(wmL82QIcd3sNF3Jp9uq4J3)Rp(0Opn(HWhPf3n(bk5yNdXb3tjmv2oDZATqzPKzrXjrBGkuZY404qkjLL9kK8wfuPyTIdbKzfALX1iwwJfEaISqOeMD(NHh9wHNTnIYsEd6ESR57mGpMZtBlksowdz9lrlxp)LLW7D)lRTHfI9bxkxKB8510MY3ZvwceL4nkz7SjKKTKKjBGK1IfLulzhHKtuSkExuUqGcWzs7UZjzDd3LeY6OLlr)AvNBnRV5By5IwdGdJgbbYLfDWuYQ1qlkfLQZqwJ2YqGHgd(k1mhT0A4BZKmJHM6omzOXQwPv0Jp(3EPvJ4D(LgTixjQywUlzzmjhHusQUjhpYE2PHxcnHhtZpcWGidPVAicnWveLZl(B1mvopNof6aGmTmLDoqSAipBM88Ixi959Kub0ncqdanvrdaUapYSmxd4vqZrR(6sMTjkk2jpC4jIiqvkeLEg5SMAHYLC49TyYFAA(SnYpLoLeGQilVrXK)oOEC4AqGln5IE9hIiTwO2RQhAYbsyeduOKCqqHg361xgoMjtVCZHkqSvmH629(KM)283Z3NgekLWilWKw(B2G5AqwJKECq4Gb0r)dZNzATmxFqroiGcGKyRXQRsGu9sDokriTD)T31O8sNW5)24liGX(CMwcsJ(MnsXsLEXZRCE9euyB3GaGRa)8I)WghdZI(6GIZNmhNGFcDFFV77X432UZChbmDvsYQVUzXN)sI)fHiZnxYkHQT6LKLlIJOKDCr5oG(Ey8vvO9Bh()Vq4QZidwuDPEbfidyp3bygCaayVYTLCrmzXZrEOXpmN2ATA1Q98gy1AhyCdGw2T4Y5vcDDY)AmdxVjGeC(2oCw5GihwX7G9tCJFQkezcz9o)gRtJCxsCUxqj)IzqcOK5nI3)X3)TwwoUZ7hFp5klbFxRR8ZRTw9jSfsvTFVjmhuYTdk91tRlQEgk9d2T6C8cw2TdLuzN)hhMoE04jO(a4nJtS)rxeB2bdcsykX468)YnWF0DU30vRxCj(GBM9xz3Jmx7xdCLRLDlW2Zt8g)Qohy37cg5MgqbX0)(LegRi71Jp3TJ40ASRj8DvypGqJdr9g9p)

StormFX commented 5 years ago

So the issue is that RemoveButton is bypassing the group's disabled state and applying the default skin when it shouldn't, right?

Edit: Since you seem to have a testing environment in place already, try the following:

Change this line in AddButton:

From:

                        Parent:RemoveButton(Button)

to:

                        ButtonData = ButtonData or Parent.Buttons[Button]
                        Parent.Buttons[Button] = nil

The reskinning of the button in RemoveButton when called in AddButton is redundant. This should address that. It also allows for only the button to be passed when changing groups, as it pulls the ButtonData from the old group prior to the reassignment. This is just a quick fix to see if it fixes the issue. As I said in the other ticket, I'm in the middle of a rewrite so a permanent solution may be awhile.

StormFX commented 5 years ago

I've made a commit that should address this. I'll leave the ticket open until I get a release finished and it's live-tested.

InfusOnWoW commented 5 years ago

Sorry for not responding, I was trying to create a small test case of what we do, but somehow it didn't behave like I expected.

I just tested your actual commit, and with that it seems fine! Thanks!

InfusOnWoW commented 5 years ago

Any idea when you are going to do a release?

StormFX commented 5 years ago

I'm rewriting a lot of it in my spare time, but I've got quite a bit of it finished. I'd say I'm about 70-75% done. Hopefully, within the next two weeks, if everything goes well.

InfusOnWoW commented 5 years ago

Please do tell us before a release, and we can arrange a bit of testing on our discord.

StormFX commented 5 years ago

Will do.

StormFX commented 5 years ago

I've released an alpha for testing purposes. I still have some improvements to make, but at least this will get some of the stuff I needed to do out of the way.

InfusOnWoW commented 5 years ago

I just tested this alpha and it seemed to work nicely. Once that masque version is out, I intend to commit something like https://github.com/InfusOnWoW/WeakAuras2/commit/85bfb2ae99bddd401b6b745d84124b39b186f874 to WeakAuras.

Note, that not every aura currently has a uniqueId, so we won't be using staticIds for all auras yet, but that's coming at some point.

StormFX commented 5 years ago

Since this issue is resolved, I'm going to close this. If you run into an issues, let me know.

SFX-WoW / Masque

Bug: AddButton not Applying the Correct Skin #47