Closed inyawfaze closed 5 days ago
I appreciate the effort you put towards adding compat for classic flavor, however I have some reservations regarding the implementation.
Libraries are mostly meant to add features that lie outside the scope of what blizzard provides, eg. LibGroupInfo
.
Implementing them in the way you have where they essentially aim to re-implement/override blizzard functions, is considered "polluting the global space" and generally frowned upon, since it has the potential to affect other addons.
Instead these compat functions should either be made locally in the files where they are needed, or some sort of util function should be made such that it's easier to use them across the entire addon, without interacting with the global space.
Looking at the lib itself I think some of it would actually taint/break, I'm fairly confident that the ping system is a mainline feature and should simply not be interacted with outside of mainline version.
As for the Aura handling, should most likely look into how blizzard handles them for classic flavor and then build internal Util functions for that. The same for tooltip handling, should also be a Util function.
If absorbs aren't available in classic flavor it would be ideal to simply omit those function calls/unload widget that rely on, it's unnecessary performance decrease to call that function or load the widget if it won't do anything.
As for HelpTip, that is most likely one of the bigger concerns, have you tested that the impl you made actually works? This system is already prone to tainting easily on mainline if you don't use it correctly, so I'd be worried that it would be even more finicky in classic.
If frames such as Boss and Focus don't work on classic, then those shouldn't be loaded either, along with any widgets that won't work.
An XML file that loads the various widgets based on flavor would be the best approach to this.
As for unit, I'm not sure if simply overriding const.UNIT
would be enough.
Defaults.Layouts
should also be overridden such that it doesn't include those frames, and widgets.
This can just be done in a simple if else
statements.
Just to finish off, I'm not trying to discourage the effort, the implementation just needs to be more integrated into the base addon. Trying to fix this simply with a library is bound to cause issues, and makes it harder to maintain in the future.
The library could most likely function as an unofficial patch of sorts. But it won't be merged, due to above mention concerns.
Thanks for the feedback, totally makes sense. My approach didn't reeeeally sit well with me either, after sleeping over it. It was pretty much a 'quick and dirty' approach. I agree that it's for the better to not merge this, but I'll keep fiddling with it.
As for the HelpTips. They only kind of work. They show up correctly, offer dismiss buttons (e.g. on the unit frame help tip), close and also store their acknowledged state. They do throw a Lua error on dismissal though, forgot about it after I had initially acknowledged them all in my first test round.
First time writing an addon that's meant to be plugged in as a lib somewhere, so I hope this works out. Referenced other addons to see how they do it.
!BlizzPolyfill adds different functions which are directly taken from the
BlizzardInterfaceCode
. Some serve as stubs so no Lua errors are thrown (e.g.pings
), others just add functions to tables that are only partially available in Era (e.g.AuraUtil
). I needed to DIY some functions, since these are not available via theBlizzardInterfaceCode
(e.g.GameTooltip:SetUnitBuffByAuraInstanceID
).Added the
LoadLibs
functionality parallel to how Cell does it.Steps to integrate this into Cell_UnitFrames:
Worked this out locally with symlinks, hope the automatic inclusion via .pkgmeta works out. 😅
Features that work with this:
Features that do not work/are unsupported on Classic Era:
LibHealComm
Features that are not thoroughly tested, but seem to work/not break other stuff/not throw errors: