I've committed these changes (with a few small edits). I'm pretty confident it works as expected after testing in both raids and BGs.
Your committed changes are incomplete. For example, in UnitIncomingHealGet(), the HealSize table is being referenced via an index that may not be fully qualified with the realm name. Also, it is not good that not every table that is indexed via unit name is using a fully qualified name if you are pursuing these changes because the various Heal* tables are very tightly-coupled -- within the internal code of the library, you've now made it required that it must be kept in mind which names must be fully qualified and which may not be when accessing the information stored in the Heal* tables.
The change you had proposed to expand the API were minor, but the implementation details to do it correctly were much larger than the patches you had posted. I simply didn't have enough time to review your changes before you committed them.
Also, it is not good that not every table that is indexed via unit name is using a fully qualified name if you are pursuing these changes because the various Heal* tables are very tightly-coupled -- within the internal code of the library, you've now made it required that it must be kept in mind which names must be fully qualified and which may not be when accessing the information stored in the Heal* tables.
If all names passed to the internals of library are fully qualified, then this shouldn't be an issue.
The change you had proposed to expand the API were minor, but the implementation details to do it correctly were much larger than the patches you had posted. I simply didn't have enough time to review your changes before you committed them.
To be pedantic, I didn't expand the API - I fixed (or attempted to fix) what I perceive to be a bug.
At any rate, if my changes are incomplete, or you would prefer not to take them, I will be delighted to back them out (or you can if you wish).
I would have held off on checking in the changes had I known that you (or anyone else) were actually reviewing the changes. After several days, the only feedback was a note about perhaps using GUIDs internally.
If all names passed to the internals of library are fully qualified, then this shouldn't be an issue.
And my point is that they're not in the latest revision on trunk. There is no point in the code where names are canonicalized to be "name-realm" beyond which you know that what you're dealing with is only fully qualified names. If you're pushing to make a change like this, you need these types of guarantees in the code so that it's maintainable.
To be pedantic, I didn't expand the API - I fixed (or attempted to fix) what I perceive to be a bug.
At any rate, if my changes are incomplete, or you would prefer not to take them, I will be delighted to back them out (or you can if you wish).
I would have held off on checking in the changes had I known that you (or anyone else) were actually reviewing the changes. After several days, the only feedback was a note about perhaps using GUIDs internally.
This confuses me because in your original post, you said you didn't want to commit without review and permission. As this is xbeeps' project, permission runs through him, though public code review is always good. You posted a patch, then one day later I post a query, then the next day you commit your changes. Perhaps I misunderstood your stated intentions.
If you look back over the recent posts on this thread, you'll find there is a stated intent to keep trunk as "always working" as it is referenced as an svn:external library by many different projects. And you'll find the discussion came about because I broke the trunk due to a misunderstanding on how projects were embedding this library both for release and for development. Let's not go down that same path again.
I'm delighted to see people looking at the code and fixing shortfalls and bugs. If LHC is broken in the battlegrounds, this should be fixed. There is even a noted workaround in CHAT_MSG_ADDON() for battlegrounds which was left alone by your patch which should probably be affected in some way. I would recommend backing out your changes which are incomplete and posting a more complete patch for review and later, commit permission from the project author.
Look at the implementation of unitFullName(). If the unit is in the local realm, the it doesn't return "name-realm", it only returns "name".
Exactly. A fully qualified name for someone on your realm does not include your realm.
A is on realm One.
B is on realm One.
C is on realm Two.
To A, A is "A", B is "B", and C is "C-Two".
To B, A is "A", B is "B", and C is "C-Two".
To C, A is "A-One", B is "B-One", and C is "C".
This is how the Blizzard API sees it.
For example,:
A calls UnitName( "B" ) - The API returns "B", nil.
A calls UnitName( "B-One" ) - The API returns nil, nil.
From this, we see that a name that includes your local realm name is considered invalid.
With these changes, A stores heals to B in HealSize["B"], and C stores them in HealSize["B-One"].
And my point is that they're not in the latest revision on trunk. There is no point in the code where names are canonicalized to be "name-realm" beyond which you know that what you're dealing with is only fully qualified names. If you're pushing to make a change like this, you need these types of guarantees in the code so that it's maintainable.
The Blizzard APIs will return a fully qualified name. The issue in LibHealComm is where it receives names in CHAT_MSG_ADDON. What one client considers to be a fully qualified name may not be the same to other clients. You'll note that these are the only changes I made - I attempt to fully qualify names that are received from other clients.
This confuses me because in your original post, you said you didn't want to commit without review and permission. As this is xbeeps' project, permission runs through him, though public code review is always good. You posted a patch, then one day later I post a query, then the next day you commit your changes. Perhaps I misunderstood your stated intentions.
You're right - I probably committed too early, before all reviewers had a chance to weigh in. I had intended to wait for reviews; but in my defense, I took the lack of response to be indifference. What would an acceptable amount of time to wait have been?
If you look back over the recent posts on this thread, you'll find there is a stated intent to keep trunk as "always working" as it is referenced as an svn:external library by many different projects. And you'll find the discussion came about because I broke the trunk due to a misunderstanding on how projects were embedding this library both for release and for development. Let's not go down that same path again.
Absolutely. I would hate to break the trunk for other projects that had referenced LibHealComm as an svn:external. However, I would argue that battlegrounds didn't work before, and now they do (in some capacity). As far as I know, I didn't break anything - I have spent several hours testing these changes both in raids and BGs.
I'm delighted to see people looking at the code and fixing shortfalls and bugs. If LHC is broken in the battlegrounds, this should be fixed. There is even a noted workaround in CHAT_MSG_ADDON() for battlegrounds which was left alone by your patch which should probably be affected in some way.
The workaround you speak of is to try to ensure that the sender's name is fully qualified, and is not affected by my changes.
I would recommend backing out your changes which are incomplete and posting a more complete patch for review and later, commit permission from the project author.
I have no issues backing out my changes, but where is it incomplete? Even if it is incomplete, is it worse or better than before?
First, sorry about being so absent in this thread. To be honest i almost forgot about it because it had been dead for a while, and I only came back to see if i could find some background information about r44, and to my surprise found several pages of discussion about various stuff I had to catch up on! I was of the impression that i would receive a mail when something was posted here (i used to), but haven't got any for a long time, so maybe i somehow opted out of that!
Anyway, regarding the battleground fix:
I (now) understand the nature of the problem, and therefore also understand how the fix Greltok submitted works. It is also clear to me that this is not just a hack or a fix (whatever that means) but actually is the correct, complete and minimal solution to handle a quirk about fully qualified names, that i was not aware of when i added the support for fully qualified names long ago. However, as you took the liberty to submit it, i also took the liberty to optimise it. It contained some redundant checks and it was not necessary to have it active when not in a battleground. I also adjusting the naming to make it a little bit clearer how it works. Nevertheless the functionality remains the same, so thanks for the contribution!
Regarding HoT support, i see there are different views, but i don't see any reason why all requirements cannot be met without conflict. I plan on adding hot's as a separate table inside the library, along with new callbacks and perhaps new functions for retrieving condensed information from this table. UnitIncomingHealGet functionality will be extended in a way that does not modify it's existing behaviour, unless an additional parameter is set to true (i.e. a includeHealOverTime boolean). I believe this is what has already been suggested here (but to be honest that was what i had in mind even before reading it!), and i'm sure everyone here will agree that is a solution that will suit everyone. If not, raise your voice now.
As for using development branches it's fine by me. I was not aware that alpha versions ended up in other addons - i thought they had to be tagged as release first. Oh well. I usually go a long way to ensure that what i submit does not break anything using both offline and online checks, so never really had a need for development branches, but given the current situation, i think it's a good idea to branch for each feature, then merge into trunk and delete the branch. Everyone are welcome to do the branch and develop, but please let me review the changes before merging into trunk. And of course you're much more likely to get the green light for a merge if i'm aware of the work you're doing in a branch, so i can have a say in it.
I'd like to thank jlam for his excellent code contributions, they are top quality and respects the existing architecture of the library in all regards. I highly appreciate this, as it can be quite a challenge to keep the library up to date with changes to skills and abilities.
Note: I have not submitted my changes to the battleground fix yet, but I'll do so later today when i have done basic testing in battlegrounds.
I was not aware that alpha versions ended up in other addons - i thought they had to be tagged as release first.
In theory, that's what is supposed to happen, but in practice, it's much, much easier to simply add the trunk to your svn-externals or your .pkgmeta once and forget about it, than to have to go update for a new tag every time one is created. :p
Just a note on using "tag: latest". If you want people to embed the library using that you need to tell them, but this library has been in existence long before that option was available, it existed when we used svn:externals which always pulled the latest version. So informing people after the fact is a much harder if you are still concerned about trying not to break other addons.
For people using 'tag: latest' in their externals it also means those libraries need to actually be tagged when needed or their addons will break from using outdated versions of libraries, so its a double issue full of problems unless both the library authors and those using them acknowledge the correct way for that library.
A new author blindly using 'tag: latest' on all their externals is just bound to experience trouble from libraries that do not tag often the same way that an author not using 'tag: latest' will experience trouble with a library who's developers are expecting everybody to be using it.
I am using LibHealComm in my priest addon call BobsToolbox. I am getting all incoming healz with no problem. My addon shows all incoming healz from other players. I sometime see my own healz on other players and I always see my healz on myself. Now to my question. Do I need to be doing something to send my healz on some channel or something? How do I send notification of my healz? :confused:
Why do you consistently write "healz"? If you mean heals, the answer to your first question is no.
I was just goofing off. I was in a silly mood. :D Thanks for the answer but I'm not sure where to go from here. I will have to go back to the code with this new knowledge and maybe it will lead me down a different path.
I am using LibHealComm in my priest addon call BobsToolbox. I am getting all incoming healz with no problem. My addon shows all incoming healz from other players. I sometime see my own healz on other players and I always see my healz on myself. Now to my question. Do I need to be doing something to send my healz on some channel or something? How do I send notification of my healz? :confused:
I'm pretty sure that if LHC3 is loaded it will broadcast your healing activity automatically. I could be wrong though.
You are not wrong. Addons have a "read-only" relationship with the library. They have no influence on how it works, and they only retrieve information from the library.
I'm pretty sure that if LHC3 is loaded it will broadcast your healing activity automatically. I could be wrong though.
I tried it last few night in a Naxx and Vault. It appears that everything is working. Sometimes it fails to detect my prayer of healing and binding heal. Of course this could still be an issue with my addon. :)
My goal posting here was to not try and report "defects" but to learn more about the library. I must say that this library is very sweet and I would like to thank the developer(s) that provides it for everyone.
I tried fixing the "new" Prayer of Healing by adding a new spellInfo.Type but I havn't found a way to figure out which unitIDs are in the subgroup of the target.
Tracking the people in raid is the only way I can think of.
I tried fixing the "new" Prayer of Healing by adding a new spellInfo.Type but I havn't found a way to figure out which unitIDs are in the subgroup of the target.
Tracking the people in raid is the only way I can think of.
Anyone got an idea or can fix LHC himself?
Please try out the latest (alpha), and let me know if it works as intended. Be aware that all clients needs to be updated, as the new Prayer of Healing requires a new message id, so it will not be recognized by older clients, and older clients send out messages with wrong information (regarding PoH only of course).
We could certainly do that, but we would still need a way to fully resolve the name that we pass to UnitGUID().
Your committed changes are incomplete. For example, in UnitIncomingHealGet(), the HealSize table is being referenced via an index that may not be fully qualified with the realm name. Also, it is not good that not every table that is indexed via unit name is using a fully qualified name if you are pursuing these changes because the various Heal* tables are very tightly-coupled -- within the internal code of the library, you've now made it required that it must be kept in mind which names must be fully qualified and which may not be when accessing the information stored in the Heal* tables.
The change you had proposed to expand the API were minor, but the implementation details to do it correctly were much larger than the patches you had posted. I simply didn't have enough time to review your changes before you committed them.
UnitIncomingHealGet() calls:
local targetName = unitFullName(unit);
resulting in a fully qualified name.
If all names passed to the internals of library are fully qualified, then this shouldn't be an issue.
To be pedantic, I didn't expand the API - I fixed (or attempted to fix) what I perceive to be a bug.
At any rate, if my changes are incomplete, or you would prefer not to take them, I will be delighted to back them out (or you can if you wish).
I would have held off on checking in the changes had I known that you (or anyone else) were actually reviewing the changes. After several days, the only feedback was a note about perhaps using GUIDs internally.
Look at the implementation of unitFullName(). If the unit is in the local realm, the it doesn't return "name-realm", it only returns "name".
And my point is that they're not in the latest revision on trunk. There is no point in the code where names are canonicalized to be "name-realm" beyond which you know that what you're dealing with is only fully qualified names. If you're pushing to make a change like this, you need these types of guarantees in the code so that it's maintainable.
This confuses me because in your original post, you said you didn't want to commit without review and permission. As this is xbeeps' project, permission runs through him, though public code review is always good. You posted a patch, then one day later I post a query, then the next day you commit your changes. Perhaps I misunderstood your stated intentions.
If you look back over the recent posts on this thread, you'll find there is a stated intent to keep trunk as "always working" as it is referenced as an svn:external library by many different projects. And you'll find the discussion came about because I broke the trunk due to a misunderstanding on how projects were embedding this library both for release and for development. Let's not go down that same path again.
I'm delighted to see people looking at the code and fixing shortfalls and bugs. If LHC is broken in the battlegrounds, this should be fixed. There is even a noted workaround in CHAT_MSG_ADDON() for battlegrounds which was left alone by your patch which should probably be affected in some way. I would recommend backing out your changes which are incomplete and posting a more complete patch for review and later, commit permission from the project author.
Exactly. A fully qualified name for someone on your realm does not include your realm.
A is on realm One.
B is on realm One.
C is on realm Two.
To A, A is "A", B is "B", and C is "C-Two".
To B, A is "A", B is "B", and C is "C-Two".
To C, A is "A-One", B is "B-One", and C is "C".
This is how the Blizzard API sees it.
For example,:
A calls UnitName( "B" ) - The API returns "B", nil.
A calls UnitName( "B-One" ) - The API returns nil, nil.
From this, we see that a name that includes your local realm name is considered invalid.
With these changes, A stores heals to B in HealSize["B"], and C stores them in HealSize["B-One"].
The Blizzard APIs will return a fully qualified name. The issue in LibHealComm is where it receives names in CHAT_MSG_ADDON. What one client considers to be a fully qualified name may not be the same to other clients. You'll note that these are the only changes I made - I attempt to fully qualify names that are received from other clients.
You're right - I probably committed too early, before all reviewers had a chance to weigh in. I had intended to wait for reviews; but in my defense, I took the lack of response to be indifference. What would an acceptable amount of time to wait have been?
Absolutely. I would hate to break the trunk for other projects that had referenced LibHealComm as an svn:external. However, I would argue that battlegrounds didn't work before, and now they do (in some capacity). As far as I know, I didn't break anything - I have spent several hours testing these changes both in raids and BGs.
The workaround you speak of is to try to ensure that the sender's name is fully qualified, and is not affected by my changes.
I have no issues backing out my changes, but where is it incomplete? Even if it is incomplete, is it worse or better than before?
Anyway, regarding the battleground fix:
I (now) understand the nature of the problem, and therefore also understand how the fix Greltok submitted works. It is also clear to me that this is not just a hack or a fix (whatever that means) but actually is the correct, complete and minimal solution to handle a quirk about fully qualified names, that i was not aware of when i added the support for fully qualified names long ago. However, as you took the liberty to submit it, i also took the liberty to optimise it. It contained some redundant checks and it was not necessary to have it active when not in a battleground. I also adjusting the naming to make it a little bit clearer how it works. Nevertheless the functionality remains the same, so thanks for the contribution!
Regarding HoT support, i see there are different views, but i don't see any reason why all requirements cannot be met without conflict. I plan on adding hot's as a separate table inside the library, along with new callbacks and perhaps new functions for retrieving condensed information from this table. UnitIncomingHealGet functionality will be extended in a way that does not modify it's existing behaviour, unless an additional parameter is set to true (i.e. a includeHealOverTime boolean). I believe this is what has already been suggested here (but to be honest that was what i had in mind even before reading it!), and i'm sure everyone here will agree that is a solution that will suit everyone. If not, raise your voice now.
As for using development branches it's fine by me. I was not aware that alpha versions ended up in other addons - i thought they had to be tagged as release first. Oh well. I usually go a long way to ensure that what i submit does not break anything using both offline and online checks, so never really had a need for development branches, but given the current situation, i think it's a good idea to branch for each feature, then merge into trunk and delete the branch. Everyone are welcome to do the branch and develop, but please let me review the changes before merging into trunk. And of course you're much more likely to get the green light for a merge if i'm aware of the work you're doing in a branch, so i can have a say in it.
I'd like to thank jlam for his excellent code contributions, they are top quality and respects the existing architecture of the library in all regards. I highly appreciate this, as it can be quite a challenge to keep the library up to date with changes to skills and abilities.
Note: I have not submitted my changes to the battleground fix yet, but I'll do so later today when i have done basic testing in battlegrounds.
In theory, that's what is supposed to happen, but in practice, it's much, much easier to simply add the trunk to your svn-externals or your .pkgmeta once and forget about it, than to have to go update for a new tag every time one is created. :p
-
View User Profile
-
View Posts
-
Send Message
Curse Premiumshows that you can set " tag: latest "
That should pull the last release tag ( or beta / alpha depending on what's available)
That's what authors should be using in projects.
For people using 'tag: latest' in their externals it also means those libraries need to actually be tagged when needed or their addons will break from using outdated versions of libraries, so its a double issue full of problems unless both the library authors and those using them acknowledge the correct way for that library.
A new author blindly using 'tag: latest' on all their externals is just bound to experience trouble from libraries that do not tag often the same way that an author not using 'tag: latest' will experience trouble with a library who's developers are expecting everybody to be using it.
-
View User Profile
-
View Posts
-
Send Message
Curse Premium-
View User Profile
-
View Posts
-
Send Message
Curse Premium-
View User Profile
-
View Posts
-
Send Message
Curse PremiumI was just goofing off. I was in a silly mood. :D Thanks for the answer but I'm not sure where to go from here. I will have to go back to the code with this new knowledge and maybe it will lead me down a different path.
I'm pretty sure that if LHC3 is loaded it will broadcast your healing activity automatically. I could be wrong though.
-
View User Profile
-
View Posts
-
Send Message
Curse PremiumI tried it last few night in a Naxx and Vault. It appears that everything is working. Sometimes it fails to detect my prayer of healing and binding heal. Of course this could still be an issue with my addon. :)
My goal posting here was to not try and report "defects" but to learn more about the library. I must say that this library is very sweet and I would like to thank the developer(s) that provides it for everyone.
Thanks,
Bobtehbuildr
Tracking the people in raid is the only way I can think of.
Anyone got an idea or can fix LHC himself?
Please try out the latest (alpha), and let me know if it works as intended. Be aware that all clients needs to be updated, as the new Prayer of Healing requires a new message id, so it will not be recognized by older clients, and older clients send out messages with wrong information (regarding PoH only of course).
Our priest from 4 heals group1 and the people marked out of range are not even in the instance.
The range of PoH is 30 yards by default and with Holy Reach Talent you can increase it by 10%/20% so its either 30 or 33/36 yards range.