search

Karl-HeinzSchneider / WoW-DragonflightUI

World of Warcraft (classic) Addon to bring the new Retail UI to WoW Classic!
https://www.curseforge.com/wow/addons/dragonflight-ui-classic
MIT License
31 stars 13 forks source link

Compat updates for `LFGBulletinBoard` #235

Closed juemrami closed 1 month ago

juemrami commented 2 months ago

Describe the bug

  1. Issue from https://github.com/Vysci/LFG-Bulletin-Board/issues/301
  2. Compatibility for Cataclysm results in lua errors

Additional context

  1. The DF.Compatibility:LFGBulletinBoard runs newOnUpdate(btn) during addon load, when the user is not dragging the minimap button, and this causes its saved position value to be updated ever addon reload.

  2. Lines here check for the existence of our addon (on any client) and tries to run the compatibility patch, however in Compatibility.lua the patch is only defined for Era clients. Results in following error WowClassicT_w7d1UFOWbX

Versions: Era/Cata Clients.

Notes: In https://github.com/Vysci/LFG-Bulletin-Board/pull/302. Ive opted any user with LibDBIcon-1.0 installed into using it to manage their button. You guy bundle it, so any overlapping users should be able to take advantage of that and the button will just play nicely.

Where it doesn't play nicely is when a user opts out using LibDBIcon so that they can free offset the button the minimap.

As a workaround, ive change the name of our minimap button frame so that your patch wont run, but that leaves an unskinned button whenever our users arent using the LibDBIcon compatibility. (see image) WowClassicT_87n5MYJRsg

juemrami commented 2 months ago

Our changes arent on the release version yet. Atm they're just out on a beta release, for the user that reported the issue.

So no rush on your end.

Karl-HeinzSchneider commented 1 month ago

Hey, thanks for the very well written issue, and PR! I'll include your fix in the next update.

What is your reason to not just use LibDBIcon? Felt a bit odd when browsing your code.

My compatibility is kind of a hack, yeah. Not using it on Cata(/Wrath) was an oversight, but somehow nobody reported any error. I hope it was not too much work to investigate on your end 😅

LFGBulletinBoard is definitly one of the addons I recommend everyone for Era/SoD!

juemrami commented 1 month ago

What is your reason to not just use LibDBIcon? Felt a bit odd when browsing your code.

There really is none, i just came into the codebase about 4 months ago, i also had alot of questions lol. We will move to LibDBIcon at some point. The biggest concern for me is that the repo owner doesnt use bigwigs packager whenever hes drops a release, and im worried about adding external libs because he might forget to include them at some point since hes still manually packaging.

All that "LibGPI" in our codebase stuff is inherited from the OLD version of the addon (2019 classic release). I guess the original author was against bundling these external libs in his addon so he made his own half-baked "libraries" (the minimap stuff seems to mostly be copy paste from LibDBIcon).

Karl-HeinzSchneider commented 1 month ago

I also started without really any Lib, but some are really useful, especially LibDBIcon seems great cause it makes it easy to style all other addon buttons at once 😅

Manual packaging is my worst nightmare, the bigwigs packager makes it so easy, setup is like 15min if you only need the basic stuff. 1-2 times I had to make a small hotfix, when my packager wasn't set up for that and I had already merged something into main.. manuall file editing and repackaging, checked everything 3-4 times and still something went wrong lol. You should definitly push for adding the auto packager!

I merged your PR into the next version, which I will release later today. Style should work, only the position would be not perfect till your addon got the update with LibDBIcon.