Reply
Thread Tools Display Modes
Unread 01-15-13, 01:54 AM   #61
myrroddin
Premium Member
 
myrroddin's Avatar
Premium Member
AddOn Author - Click to view addons
Join Date: Oct 2008
Posts: 422
Originally Posted by Phanx View Post
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 is offline   Reply With Quote
Unread 02-15-13, 10:39 PM   #62
myrroddin
Premium Member
 
myrroddin's Avatar
Premium Member
AddOn Author - Click to view addons
Join Date: Oct 2008
Posts: 422
[BUG] Redemption spellID has typo

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.
myrroddin is offline   Reply With Quote
Unread 02-16-13, 06:23 AM   #63
Phanx
A Pyroguard Emberseer
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 4,738
Good catch. Fixed.
__________________
Author/maintainer of Grid, PhanxChat, ShieldsUp, and many more.
Troubleshoot an addonTurn any code into an addonMore addon resources
Need help with your code? Post all of your actual code! Attach or paste your files.
Please don’t PM me about addon bugs or code questions. Post a comment or forum thread instead!
Phanx is offline   Reply With Quote
Unread 02-16-13, 11:47 AM   #64
myrroddin
Premium Member
 
myrroddin's Avatar
Premium Member
AddOn Author - Click to view addons
Join Date: Oct 2008
Posts: 422
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.
Code:
svn://svn.wowinterface.com/LibResInfo-976/trunk
Turns into this link when "Copy link address" is selected in my browser:
Code:
http://svn.wowinterface.com/listing.php?repname=LibResInfo&path=%2Ftrunk%2F#_trunk_
Is that normal, and I am jumping at cats?
myrroddin is offline   Reply With Quote
Unread 02-17-13, 01:34 AM   #65
Phanx
A Pyroguard Emberseer
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 4,738
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.
__________________
Author/maintainer of Grid, PhanxChat, ShieldsUp, and many more.
Troubleshoot an addonTurn any code into an addonMore addon resources
Need help with your code? Post all of your actual code! Attach or paste your files.
Please don’t PM me about addon bugs or code questions. Post a comment or forum thread instead!
Phanx is offline   Reply With Quote
Unread 02-17-13, 07:45 AM   #66
myrroddin
Premium Member
 
myrroddin's Avatar
Premium Member
AddOn Author - Click to view addons
Join Date: Oct 2008
Posts: 422
Bwahahahahhahahhahaha!!!! Love those cats!
myrroddin is offline   Reply With Quote
Unread 02-17-13, 09:16 PM   #67
myrroddin
Premium Member
 
myrroddin's Avatar
Premium Member
AddOn Author - Click to view addons
Join Date: Oct 2008
Posts: 422
Bug or Inconsistency?

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 is offline   Reply With Quote
Unread 02-18-13, 12:11 AM   #68
myrroddin
Premium Member
 
myrroddin's Avatar
Premium Member
AddOn Author - Click to view addons
Join Date: Oct 2008
Posts: 422
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:
Lua Code:
  1. function MyAddon:LibResInfo_ResExpired(targetUnit, targetGUID)
  2.     -- do something
  3. end
  4.  
  5. function MyAddon:LibResInfo_ResCastCancelled(casterUnit, casterGUID, targetUnit, targetGUID)
  6.     -- do something
  7. end
But according to LRI's code, the callbacks are actually
Lua Code:
  1. function MyAddon:LibResInfo_ResExpired(targetGUID, targetUnit)
  2.   -- oops, better make sure we have the right info!
  3. end
  4.  
  5. function MyAddon:LibResInfo_ResCastCancelled(casterGUID, casterUnit, targetGUID, targetUnit)
  6.     -- backwards?
  7. end
All other callbacks follow the API page.

Last edited by myrroddin : 02-18-13 at 12:17 AM. Reason: ResExpired only has target, not caster, duh!
myrroddin is offline   Reply With Quote
Unread 02-18-13, 10:44 PM   #69
Phanx
A Pyroguard Emberseer
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 4,738
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.
__________________
Author/maintainer of Grid, PhanxChat, ShieldsUp, and many more.
Troubleshoot an addonTurn any code into an addonMore addon resources
Need help with your code? Post all of your actual code! Attach or paste your files.
Please don’t PM me about addon bugs or code questions. Post a comment or forum thread instead!

Last edited by Phanx : 02-18-13 at 11:08 PM.
Phanx is offline   Reply With Quote
Unread 02-18-13, 11:07 PM   #70
Fizzlemizz
A Rage Talon Dragon Guard
 
Fizzlemizz's Avatar
AddOn Author - Click to view addons
Join Date: Dec 2011
Posts: 321
sRaidFrames uses LibResInfo-1.0.

Originally Posted by Phanx View Post
... but I'm not actually aware of any addons other than mine and yours that are using the lib yet.
__________________
FizzleMizz Maintainer of Discord Unit Frames and Discord Art.
Author of FauxMazzle

Last edited by Fizzlemizz : 02-18-13 at 11:11 PM.
Fizzlemizz is offline   Reply With Quote
Unread 02-19-13, 01:29 AM   #71
Phanx
A Pyroguard Emberseer
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 4,738
The new, standardized order of arguments will be:

Functions:
Code:
status, endTime, casterUnit, casterGUID          = lib:UnitHasIncomingRes(unit)
        endTime, targetUnit, targetGUID, isFirst = lib:UnitIsCastingRes(unit)
Callbacks:
Code:
"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?
__________________
Author/maintainer of Grid, PhanxChat, ShieldsUp, and many more.
Troubleshoot an addonTurn any code into an addonMore addon resources
Need help with your code? Post all of your actual code! Attach or paste your files.
Please don’t PM me about addon bugs or code questions. Post a comment or forum thread instead!
Phanx is offline   Reply With Quote
Unread 02-19-13, 02:45 AM   #72
myrroddin
Premium Member
 
myrroddin's Avatar
Premium Member
AddOn Author - Click to view addons
Join Date: Oct 2008
Posts: 422
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.

Last edited by myrroddin : 02-19-13 at 02:52 AM. Reason: Grammar, please!
myrroddin is offline   Reply With Quote
Unread 02-19-13, 02:48 AM   #73
myrroddin
Premium Member
 
myrroddin's Avatar
Premium Member
AddOn Author - Click to view addons
Join Date: Oct 2008
Posts: 422
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)
myrroddin is offline   Reply With Quote
Unread 02-19-13, 03:42 AM   #74
Phanx
A Pyroguard Emberseer
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 4,738
Originally Posted by myrroddin View Post
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 View Post
... 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).
__________________
Author/maintainer of Grid, PhanxChat, ShieldsUp, and many more.
Troubleshoot an addonTurn any code into an addonMore addon resources
Need help with your code? Post all of your actual code! Attach or paste your files.
Please don’t PM me about addon bugs or code questions. Post a comment or forum thread instead!

Last edited by Phanx : 02-19-13 at 03:45 AM.
Phanx is offline   Reply With Quote
Unread 02-19-13, 03:45 AM   #75
myrroddin
Premium Member
 
myrroddin's Avatar
Premium Member
AddOn Author - Click to view addons
Join Date: Oct 2008
Posts: 422
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.
myrroddin is offline   Reply With Quote
Unread 02-19-13, 03:46 AM   #76
Phanx
A Pyroguard Emberseer
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 4,738
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.
__________________
Author/maintainer of Grid, PhanxChat, ShieldsUp, and many more.
Troubleshoot an addonTurn any code into an addonMore addon resources
Need help with your code? Post all of your actual code! Attach or paste your files.
Please don’t PM me about addon bugs or code questions. Post a comment or forum thread instead!
Phanx is offline   Reply With Quote
Unread 02-19-13, 03:54 AM   #77
myrroddin
Premium Member
 
myrroddin's Avatar
Premium Member
AddOn Author - Click to view addons
Join Date: Oct 2008
Posts: 422
Originally Posted by Phanx View Post
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?
Lua Code:
  1. function MyAddon:LibResInfo_ResCastStarted(targetUnit, targetGUID)
  2.     local hasIncomingRes, endTime, casterUnit, casterGUID = LibResInfo:UnitHasIncomingRes(targetUnit/targetGUID)
  3.     local spellName, _, _, icon = UnitCastingInfo(casterUnit)
  4.     -- do stuff
  5. end
That would cut down a great deal of duplication indeed.
myrroddin is offline   Reply With Quote
Unread 02-19-13, 04:06 AM   #78
myrroddin
Premium Member
 
myrroddin's Avatar
Premium Member
AddOn Author - Click to view addons
Join Date: Oct 2008
Posts: 422
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.
myrroddin is offline   Reply With Quote
Unread 02-19-13, 04:27 AM   #79
Phanx
A Pyroguard Emberseer
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 4,738
Originally Posted by myrroddin View Post
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.
__________________
Author/maintainer of Grid, PhanxChat, ShieldsUp, and many more.
Troubleshoot an addonTurn any code into an addonMore addon resources
Need help with your code? Post all of your actual code! Attach or paste your files.
Please don’t PM me about addon bugs or code questions. Post a comment or forum thread instead!
Phanx is offline   Reply With Quote
Unread 02-19-13, 01:07 PM   #80
myrroddin
Premium Member
 
myrroddin's Avatar
Premium Member
AddOn Author - Click to view addons
Join Date: Oct 2008
Posts: 422
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.
myrroddin is offline   Reply With Quote
Reply

Go BackWoWInterface » AddOns, Compilations, Macros » Released AddOns » LibResInfo - resurrection info without comms

Thread Tools
Display Modes

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

vB code is On
Smilies are On
[IMG] code is On
HTML code is Off