cannonpalms / LibThreatClassic

A library for multi-target threat tracking in WoW Classic (1.13.2).
GNU Lesser General Public License v2.1
0 stars 1 forks source link

The versioning mechanism is bypassed, potentially preventing the latest version from loading when several addons including the lib are loaded #16

Open cannonpalms opened 4 years ago

cannonpalms commented 4 years ago

Migrated from https://github.com/EsreverWoW/LibThreatClassic/issues/14 Originally created by @bjornenalfa on Fri, 08 Nov 2019 21:31:33 GMT


cannonpalms commented 4 years ago

This needs to be confirmed and resolved ASAP. Proper library version behavior could potentially be a major issue for this fork.

DDCorkum commented 4 years ago

Edit: see my later post below. I was missing something in my first read of the code.


I think this is a genuine problem. Look at lines 59 to 63 of ThreatClassic-1.0.lua:

local MAJOR, MINOR = "ThreatClassic-1.0", 10
assert(LibStub, MAJOR .. " requires LibStub")

-- local ThreatLib = LibStub:NewLibrary(MAJOR, MINOR)
-- if not ThreatLib then return end

The way libstub works is that MAJOR should be a constant name, while MINOR should increment with each new version so that it blocks out older ones. I would have expected this code to be something like this:

local MAJOR, MINOR = "ThreatClassic", 10
assert(LibStub, MAJOR .. " requires LibStub")

-- if this version or a newer one is already installed, go no further
local __, minor = LibStub:GetLibrary(MAJOR, true)
if (minor and minor => MINOR) then return end

-- register this addon
local ThreatLib = LibStub:NewLibrary(MAJOR, MINOR)
if not ThreatLib then return end

The good news is that because this addon uses the table returned by LibStub (instead of globals), you can safely make the change and it will not be interfered with by older addons. Those addons won't be able to share threat data any more until updating, but they will still work independently of other addons who do update.

It would be worse if globals were being overwritten, because then older addons would stop newer ones from working at all.

PS. Are you now asserting yourself as the lead for this project? If so, good luck! I came across this and am thinking of incorporating the library into CT Raid Assist to expand the number of compatible addons using it.

dfherr commented 4 years ago

Hi @DDCorkum thanks for this info. I'm also thinking about taking this over, fixing the bugs and maintaining it. As this fork also didn't have a single code change yet. I would rerelease it as major LibThreatClassic2 to get out of this possibly incompatible namespace. https://github.com/dfherr/LibThreatClassic2

Isn't directly writing into the LibStub "registry" kind of setting globals?

https://github.com/cannonpalms/LibThreatClassic/blob/master/ThreatClassic-1.0.lua#L65-L80

Using your code suggestion these lines have to be deleted right?

DDCorkum commented 4 years ago

Oh, I didn't see those. It looks like the original developer decided to use Ace and then manually write a reference into LibStub. The rationale was probably to allow non-Ace addons to fetch using LibStub, while Ace addons can just go ahead and fetch via Ace.

This does somewhat change my earlier post. I'm a little less familiar on Ace itself.

As for who runs the development, I think it would be best if there were a team effort rather than a number of forks.

dfherr commented 4 years ago

The rationale was probably to allow non-Ace addons to fetch using LibStub, while Ace addons can just go ahead and fetch via Ace.

This does somewhat change my earlier post. I'm a little less familiar on Ace itself.

i checked the current ace documentation and peaked into other addons and it looks like this is an outdated way of doing things. Instead ace embedding is done using the embeds.xml file. As the majority of the code is from the original classic times and this just got ported, I assumed the embeds.xml got added, but the in-code embedding didn't get removed.

As for who runs the development, I think it would be best if there were a team effort rather than a number of forks.

absolutely agree. I'm happy to add collaborators right from the start. I'm reluctant to have a fork as the "main active" project, as the goal of a fork is to push changes upstream not to overtake development. But regardless where the active development is, someone has to start making code changes. This fork just ported the issues ¯_(ツ)_/¯ I already fixed half of the open bugs now and will continue this evening here: https://github.com/dfherr/LibThreatClassic2

nano1895 commented 4 years ago

@dfherr That's awesome that you've gone ahead and started fixing bugs. You may want to get into contact with the author of this fork on discord. https://discord.gg/fightclub (same username @cannon) as he is active there and I'm not sure how much work he's already done thats not pushed up / reflected in the repo.

And count me in as someone whos interested in helping develop this project, not too experienced in writing addons but I can learn. Having an accurate threat meter would help my guild alot in terms of fine tuning our dps in raid so very interested in getting an updated version out there.