LibResInfo - resurrection info without comms

[myrroddin]

Originally Posted by Phanx just like you do for LibDataBroker, but for reasons that actually make sense.

For those wondering, LibDataBroker-1.1 does not have a .toc file, and thus is not a standalone library. And I agree, that decision was bizarre.

On topic, thank you Phanx.

[myrroddin]

I was testing things, and lo and behold, line 108 in LibResInfo has a typo: instead of the spellID for Redemption, the spellID for Frost Protection is used instead.

Change 7238 to 7328 and fixed.

[Phanx]

Good catch. Fixed.

[myrroddin]

Not a LRI issue, but since LRI is the only SVN checkout from WoWI I use, I'm asking here. Sorry for the off-topic!

I'm sure the following link will work in pkgmetas, but when I try to get a direct download or SVN checkout for testing (I'm not ready to commit my AddOn to alpha –– that will upset users) I get weirdness and errors in TortoiseSVN.

svn://svn.wowinterface.com/LibResInfo-976/trunk

Turns into this link when "Copy link address" is selected in my browser:

http://svn.wowinterface.com/listing.php?repname=LibResInfo&path=%2Ftrunk%2F#_trunk_

Is that normal, and I am jumping at cats?

[Phanx]

Yes, that is normal. The link's target URL points to a web-based view of the SVN repository, where you can browse and view the files in the repository through your web browser instead of your SVN client and text editor. The link's text points to the actual SVN repository, which you need an actual SVN client to interact with. Be this cat, not this one.

[myrroddin]

Bwahahahahhahahhahaha!!!! Love those cats!

[myrroddin]

I found either a bug or an inconsistency in the callbacks ResExpired and ResCastCancelled: they are the only callbacks where the casterUnit, casterGUID, targetUnit, and targetGUID are reversed in their returns from the other callbacks.

All other callbacks are as above, yet those two are casterGUID, casterUnit, targetGUID, targetUnit.

Line 463, which fires ResCastCanelled, has different returns than lines 324, 335, and 352. I'll have to assume that was intentional, but I can't fathom why.

[myrroddin]

After reading the above, I thought perhaps I didn't write with clarity. The API page for callbacks suggests code for ResExpired and ResCastCancelled to be something like this:

function MyAddon:LibResInfo_ResExpired(targetUnit, targetGUID)
    -- do something
end
 
function MyAddon:LibResInfo_ResCastCancelled(casterUnit, casterGUID, targetUnit, targetGUID)
    -- do something
end

But according to LRI's code, the callbacks are actually

function MyAddon:LibResInfo_ResExpired(targetGUID, targetUnit)
  -- oops, better make sure we have the right info!
end
 
function MyAddon:LibResInfo_ResCastCancelled(casterGUID, casterUnit, targetGUID, targetUnit)
    -- backwards?
end

All other callbacks follow the API page.

[Phanx]

You know, I noticed that when you first started posting about SmartRes; I could have sworn I adjusted all of the callbacks and functions so they all returned values in the same order (target, then caster, then other info), but apparently I did not. I was going to just leave it as-is to avoid breaking compatibility, but I'm not actually aware of any addons other than mine and yours that are using the lib yet, and have no way to search for them, so I'm just going to go ahead and make the change I thought I already made.

However, I would like to do a bit of testing before I commit. I'm sending you a PM with my BattleTag; please send me an invite (and let me know what times you're usually on) so we can do some testing.

I'm also going to go ahead and add spellID to the end of the arguments passed with the ResCastStarted callback, so you won't need to call UnitCastingInfo anymore to get it. LRI already calls UnitCastingInfo to get the endTime, so there's no reason to make addons call the same function again to find out which spell is being cast.

This information will not be provided with the lib:UnitIsCastingRes API, however, as LRI does not already call UnitCastingInfo here, and not all addons need this information.

I also noticed that ResCastCancelled was inconsistently returning guid/unit instead of unit/guid, so that's getting fixed as well.

[Fizzlemizz]

sRaidFrames uses LibResInfo-1.0.

Originally Posted by Phanx ... but I'm not actually aware of any addons other than mine and yours that are using the lib yet.

[Phanx]

The new, standardized order of arguments will be:

Functions:

status, endTime, casterUnit, casterGUID          = lib:UnitHasIncomingRes(unit)
        endTime, targetUnit, targetGUID, isFirst = lib:UnitIsCastingRes(unit)

Callbacks:

"LibResInfo_ResCastStarted",   targetUnit, targetGUID, casterUnit, casterGUID, endTime
"LibResInfo_ResCastCancelled", targetUnit, targetGUID, casterUnit, casterGUID
"LibResInfo_ResCastFinished",  targetUnit, targetGUID, casterUnit, casterGUID
"LibResInfo_ResPending",       targetUnit, targetGUID
"LibResInfo_ResExpired",       targetUnit, targetGUID
"LibResInfo_ResUsed",          targetUnit, targetGUID

I'll update the documentation pages once I actually make the changes.

After further consideration, I'm on the fence about the previously proposed addition of a spellID arg to the ResCastStarted callback, though I am now considering adding an isFirst arg to the Started/Cancelled/Finished callbacks. Feedback welcome.

I also added a list of addons using the lib to the download page; currently that list consists of Grid, oUF_Phanx, SmartRes2, and sRaidFrames. If anyone knows of any other addons, please let me know. I'm happy to make any necessary changes for addons with open repositories.

On the other hand, it may be better to bump the major version to 1.1. Thoughts, anyone?

[myrroddin]

I like the new standard, actually. True, this may cause some breakage, but you write Grid, SmartRes2 is in development, and that only leaves sRaidFrames, and I don't know how changes will affect that AddOn. My point being, I don't think a major version bump is necessary; sRaidFrames can adapt, if need be. If the case was messing with dozens of AddOns, that would be different. It is only now that LibResInfo is getting put through its paces, and us authors were expecting things to break.

As for spellID, I could go either way. I don't see it is strictly necessary. While LRI might use UnitCastingInfo(), and if spellID is provided, that removes some potential redundancy. The trouble is, it is potential. If an AddOn really requires the spellID, it isn't hard to get. No need to provide the barn with the horse.

On the other hand, isFirst might truly be useful to add to callbacks, given that two of the AddOns are unit frames and SR2 is all about being first or not. Again, it isn't hard to pull that from the library API, so all you would be doing is creating your own redundancy. Or remove it from the API. That might be a bad idea, however.

My input would be that if you were to add isFirst to a callback, the only one that benefits is ResCastStarted(). ResCastCancelled() and ResCastFinished() don't gain anything from knowing whose cast started first. After all, either way, the cast is done, first or not.

The one duplication that is provably useful is endTime in both APIs and ResCastStarted(), with endTime being the resurrection time out in ResExpired() rather than the cast time.

The short version of this is there is no need to bump major versions when LibResInfo is barely off the ground, and providing the kitchen sink in callbacks is not necessary either. Standardizing the callbacks is all that's needed.

On a side note, here's a temporary fix: update the documentation to reflect the current code, and when you change things, then update again. But I am itchy and want to test things out! Darn impatience!!

But that's just me. Speaking of which, SmartRes2 does have an open repository, but there is no point in anyone but me at this time making a commit. The last alpha, r218, doesn't have a lot in common code with what r219 will look like.

[myrroddin]

Oh, and Dridzt isn't worried about callback or API changes in LibResInfo. From the latest changelog of sRaidFrames:

r760 | dridzt | 2013-02-19 05:34:17 +0000 (Tue, 19 Feb 2013) | 3 lines Changed paths: M /trunk/.pkgmeta - Fix LibResInfo-1.0 directory depth (again) - No tag for SRF until Phanx is done changing LRI api. (ref: http://www.wowinterface.com/forums/s...0&postcount=69)

[Phanx]

Originally Posted by myrroddin If the case was messing with dozens of AddOns, that would be different.

Yeah, I checked about a dozen popular unit/raid frame addons and didn't find any others using the lib, so I think a change is fine at this point.

Originally Posted by myrroddin ... if you were to add isFirst to a callback, the only one that benefits is ResCastStarted(). ResCastCancelled() and ResCastFinished() don't gain anything from knowing whose cast started first. After all, either way, the cast is done, first or not.

ResCastStarted always means "this unit is getting a res", but ResCastCancelled does not always mean "this unit isn't getting a res anymore" because the cancelled cast might not be the only one. However, typing this out, I realized that it actually doesn't matter whether the cancelled cast was the first; it only matters whether the cancelled cast was the only res casting.

However, since not all addons need this info, and getting it requires some extra work, I'm just going to leave it out of callbacks, and let addons use the API to get it if they want it. Same goes for the spellID, and now I'm actually considering also removing the endTime and caster info out of callbacks, and have them provide only the target unit like most real events (eg. UNIT_HEALTH).

[myrroddin]

Oops, I meant to say that ResPending() still needs the expiry time as part of its callback information. I erroneously called it ResExpired() above. I am sure you have done this research already, but I thought the time out was 120 seconds, not 60? I haven't verified, just going from memory.

[Phanx]

It used to be 120 seconds, but while I was writing the lib, testing showed that it is now 60 seconds. I'm not sure when they changed it, as I didn't raid and hardly played at all during Cataclysm, but I'd guess it was either at the beginning of Cata, or at the beginning of Mists.

Also, I was editing my previous post while you were posting, so you might want to re-read it.

[myrroddin]

Originally Posted by Phanx I'm actually considering also removing the endTime and caster info out of callbacks, and have them provide only the target unit like most real events (eg. UNIT_HEALTH).

Something like this, perhaps?

function MyAddon:LibResInfo_ResCastStarted(targetUnit, targetGUID)
    local hasIncomingRes, endTime, casterUnit, casterGUID = LibResInfo:UnitHasIncomingRes(targetUnit/targetGUID)
    local spellName, _, _, icon = UnitCastingInfo(casterUnit)
    -- do stuff
end

That would cut down a great deal of duplication indeed.

[myrroddin]

ResCastFinished and ResCastCancelled could follow suit above, replacing targetUnit, targetGUID with casterUnit, casterGUID. The API can give us targets. ResPending is the only one that needs the extra return (targetUnit, targetGUID, expiry_time).

I think you are quite correct to shrink the other callbacks to either target or caster, as appropriate. Just don't remove return values from UnitHasIncomingRes or UnitIsCastingRes, although you can change the order if you wish.

[Phanx]

Originally Posted by myrroddin ResCastFinished and ResCastCancelled could follow suit above, replacing targetUnit, targetGUID with casterUnit, casterGUID. The API can give us targets. ResPending is the only one that needs the extra return (targetUnit, targetGUID, expiry_time).

Actually, ResCastFinished and ResCastCancelled need both the caster and the target, since the data returned by the API is updated before callbacks are fired. If you call UnitIsCastingRes on the caster in response to either of those callbacks, you will get nil, because the caster is no longer casting a res. Likewise, if you call UnitHasIncomingRes after those callbacks, if the caster whose cast finished or was cancelled was the only one casting a res on that unit, you will get nil, because the unit no longer has an incoming res.

ResPending actually doesn't need the expiry time, since you can get that from UnitHasIncomingRes, but after typing the previous paragraph, I realized I'll need to keep both caster and target in at least ResCastFinished and ResCastCancelled, since there's no other way to get that info, so I might as well just keep all of the current callback args (though the order will change as detailed in a previous post) to keep them consistent. Also, all of the info currently passed is always on hand when the callbacks are fired, so there isn't any extra work involved, whereas spellID or isFirst are not always on hand, and would require extra work in some cases.

[myrroddin]

I realized the same thing as I was crawling into bed. I also realized that it would be more intuitive to have caster passed to ResCastStarted rather than target. However, if you are keeping all the arguments, then it becomes moot.

Just so I understand, endTime from UnitHasIncomingRes is when that person's Accept timer runs out (60 seconds), and not the cast time of the caster's resurrection spell? Whereas UnitIsCastingRes' endTime is the cast time of the spell (10 or less seconds)? If that is the case, then I wonder, does LRI add the cast time to 60 in order to get the true time out?

What I mean is, lets say I want to know when Phanx's Accept button expires if Jixx resurrects her. Let's say Jixx's haste makes his Resurrection cast = 8.9 seconds, which means Phanx times out in 68.9 seconds –– at the start of Jixx's cast. Now, this isn't hard to figure out if LRI does not directly provide this information. All I need to know is Jixx's cast time and add it to 60.

Sounds good, although if all that above is true, then perhaps renaming UnitHasIncomingRes' endTime to expiryTime might be in order to lessen confusion.

The other way to look at it, just to add confusion, is that UnitHasIncomingRes's endTime is not 60 + cast time seconds, and is just end time of the caster's resurrection. I was thinking that is the case, but if I was wrong, that might explain a few results during my tests.

Personally, I would have endTime in both UnitHasIncomingRes and UnitIsCastingRes = cast time, and make a note on the API page that authors should add 60 to get the Accept expiration time. My reasoning is that authors tend to want the cast time not the expiry time, which I pointed out is simple addition anyway.

Sometimes you want to examine the target for when an incoming cast ends, other times you want to examine the caster.

... In other news, I haven't theory-crafted this much since I wrote LibGuildBankComm, and it is nice to have the tête-à-tête.