lua error in tradeskill reagents mouseover
Kherae opened this issue · 17 comments
Describe the bug
lua error.
To Reproduce
Steps to reproduce the behavior:
hover over a reagent in a tradeskill window. example:coarse leather, in the coarse leather waistguard recipe under zandalari leatherworking
Additional context
Message: Interface\AddOns\ItemVersion\Api.lua:12: attempt to index field '?' (a nil value)
Time: Sun Feb 7 02:25:18 2021
Count: 4
Stack: Interface\AddOns\ItemVersion\Api.lua:12: attempt to index field '?' (a nil value)
[string "@Interface\AddOns\ItemVersion\Api.lua"]:12: in function `getItemExpac'
[string "@Interface\AddOns\ItemVersion\Tooltip.lua"]:6: in function <Interface\AddOns\ItemVersion\Tooltip.lua:4>
[string "@Interface\AddOns\ItemVersion\Tooltip.lua"]:24: in function <Interface\AddOns\ItemVersion\Tooltip.lua:14>
[string "=[C]"]: ?
[string "=[C]"]: ?
[string "=[C]"]: ?
[string "=[C]"]: ?
[string "@Interface\AddOns\Altoholic\Tooltip.lua"]:702: in function `SetRecipeReagentItem'
[string "@Interface\AddOns\Blizzard_TradeSkillUI\Blizzard_TradeSkillDetails.lua"]:761: in function `OnReagentMouseEnter'
[string "*:OnEnter"]:1: in function <[string "*:OnEnter"]:1>
Locals: itemId = 50
(*temporary) = <table> {
7 = <table> {
}
1 = <table> {
}
2 = <table> {
}
4 = <table> {
}
8 = <table> {
}
9 = <table> {
}
5 = <table> {
}
3 = <table> {
}
6 = <table> {
}
}
(*temporary) = nil
(*temporary) = nil
(*temporary) = "attempt to index field '?' (a nil value)"
Private = <table> {
itemIdToVersionId = <table> {
}
majorToExpac = <table> {
}
versionIdToVersion = <table> {
}
}```
I code lua for other game stuff. some things you can do to 'help' your addon include having proper fallbacks, and comparisons. may not be as nice looking, might be very annoying, but it means fewer lua errors. for reference: in lua expressions a variable `a` evaluates to true if it is non-false and non-nil. the statement `a and a[b]` will never error if 'a' is nil, while `a[b]` by itself would. functions are the same. what these code segments below do is verify that all the stuff in question does, in fact ,exist. probably overkill, but I get no errors with it. up to you to set the default text in the fallback cases.
your tooltip.lua: replace relevant pair of lines (8 and 9 as of this writing) with these:
```lua
local left = format('|cFFCA3C3C%s|r %s', version_label_text or "", ItemVersion and ItemVersion.versionString(version) or "")
local right = format('|cFFCA3C3C%s|r %s', expac_label_text or "", expac and expac.canonName or "")
your Api.lua:
local AddonName, Private = ...
function ItemVersion.getItemVersion(itemId)
return Private and Private.versionIdToVersion and Private.itemIdToVersionId and itemId and Private.itemIdToVersionId[itemId] and Private.versionIdToVersion[Private.itemIdToVersionId[itemId]]
end
function ItemVersion.getItemExpac(itemId)
local dummy
if Private and itemId and Private.majorToExpac and Private.versionIdToVersion and Private.itemIdToVersionId then
dummy=Private.itemIdToVersionId[itemId]
if dummy then
dummy=Private.versionIdToVersion[dummy]
if dummy then
dummy=dummy.major
if dummy then
dummy=Private.majorToExpac[dummy]
end
end
end
end
return dummy or ""
end
function ItemVersion.versionString(version)
return format("%d.%d.%d.%d", version and version.major or "0", version and version.minor or "0", version and version.patch or "0", version and version.build or "0")
end
Hey @Kherae, thanks for the report!
I'm not able to reproduce your issue, which I need to be able to do to fix it.
Could you try a few things that might help?:
-
Ensure your addon version is
2021.05.0
. Reason I ask is because I did recently fix an issue similar to this: #2.One way to do that is to put the following command into a WoW chatbox and see what it prints out.
/run print(GetAddOnMetadata("ItemVersion", "Version"))
-
Provide a screenshot of the tooltip that triggers the bug (at least including the frame/window where the moused-over item is)
I also noticed that Interface\AddOns\Altoholic\Tooltip.lua
is in the stack trace you provided. I'm not sure how Altoholic builds its tooltips, so that may be the source of the issue. Is the tooltip something that Altoholic provides? If so, I have mixed feeling about supporting that because it creates a slippery slope of supporting any/all other third-party addons. This addon just targets tooltips that implement the standard Blizzard tooltip function GetItem(), which is most of them. (You can see where we call that here)
Regarding you suggestions about checking for nullity on attribute access (e.g. if foo and foo.bar
), I understand where you're coming from on that, but I'm not going to do that when the object can't possibly be nil. What you're suggesting is like saying:
local foo = {}
foo.bar = 123
if foo and foo.bar then
print(foo.bar)
end
foo
can't be nil
here, so the additional check on it is redundant.
On the other hand, ItemVersion.getItemVersion
, where the itemId
might not exist in ItemVersion's database, that's a valid case. I considered that when I wrote that code, but if that is true, I want a lua bug to show up, because it means that we're missing an item. Hopefully, users who experience that issue come here to make an issue for it. I don't want there to be a graceful degradation, such as setting 0.0.0.0
, as you suggested. My current thoughts on this are maybe to print some error message like "ItemVersion does not know about item with id = . Please create a bug report".
Also note that, except for maybe a few hours after a Tuesday reset during which new items are introduced, I've yet to see an item that ItemVersion does not know about. If you find one, make an issue.
version in TOC: ## Version: 2021.5.0
yeah, the excessive redundancy was more of an example, really. I have a tendency to add a bit extra, because in my codework I keep ending up with people who manage to break even simple stuff, somehow.
I disabled everything except your addon and addon control panel, and the error is still there:
Message: Interface\AddOns\ItemVersion\Api.lua:12: attempt to index field '?' (a nil value)
Time: Sun Feb 7 12:54:52 2021
Count: 3
Stack: Interface\AddOns\ItemVersion\Api.lua:12: attempt to index field '?' (a nil value)
[string "@Interface\AddOns\ItemVersion\Api.lua"]:12: in function `getItemExpac'
[string "@Interface\AddOns\ItemVersion\Tooltip.lua"]:6: in function <Interface\AddOns\ItemVersion\Tooltip.lua:4>
[string "@Interface\AddOns\ItemVersion\Tooltip.lua"]:24: in function <Interface\AddOns\ItemVersion\Tooltip.lua:14>
[string "=[C]"]: ?
[string "=[C]"]: in function `SetRecipeReagentItem'
[string "@Interface\AddOns\Blizzard_TradeSkillUI\Blizzard_TradeSkillDetails.lua"]:761: in function `OnReagentMouseEnter'
[string "*:OnEnter"]:1: in function <[string "*:OnEnter"]:1>
Locals: itemId = 50
(*temporary) = <table> {
7 = <table> {
}
1 = <table> {
}
2 = <table> {
}
4 = <table> {
}
8 = <table> {
}
9 = <table> {
}
5 = <table> {
}
3 = <table> {
}
6 = <table> {
}
}
(*temporary) = nil
(*temporary) = nil
(*temporary) = "attempt to index field '?' (a nil value)"
Private = <table> {
itemIdToVersionId = <table> {
}
majorToExpac = <table> {
}
versionIdToVersion = <table> {
}
}
look at the variables, though: "itemId = 50"
the item ID for the reagent in the crafting recipe sure as hell isn't 50.
furthermore, with the addons folder cleared out except itemversion, it STILL errors, with the same error, though it seems to have a delay before it fully loads the info and errors the first time.
As for the desire for an error...that is understandable. I just immensely hate lua errors popping up. In this case, it very much is an issue specifically with ItemVersion itself.
local function OnTooltipSetItem(tooltip)
local _, link = tooltip:GetItem()
print(link)
tested with this and it's returning an empty link
led me to search and landed on this:
https://www.wowinterface.com/forums/showthread.php?t=52644
the fact it's still a thing around 6 years later is telling, heh.
I just had a thought while washing dishes. your addon is not working right in this case, however I have an addon installed that DOES work, that you might study: https://www.curseforge.com/wow/addons/idtip
Ah, ok, I was able to reproduce the errant behavior, but it manifested in a way that did not produce a lua error (by coincidence). It's still a bug though.
I don't have a leatherworker, but for blacksmithing, see this:
The item id is 60. Let's look that up on wowhead:
https://www.wowhead.com/item=60 It's for Layered Tunic, and old, old item that has nothing to do with this recipe.
(Note too that Elvui, the addon the shows the item id in my screenshot, makes the same mistake, so this is a bug there too.)
I've run into this strange behavior with items from the crafting window before in issue #2. The crafting tooltips are weird and break some guarantees about how the GameTooltip
(the tooltip that displays items) works. From my investigation, Blizzard overloads these tooltips somehow and calling GetItem()
on them does not return the expected results.
I will do some more research on seeing if there's a way to extract the proper item id in these cases. If it is possible, I will write a fix. If it is not possible, I will simply not have ItemVersion add anything to the tooltip.
Also, about idTip, that was actually how I found out how to add things to the tooltip. They have support for more than just items, but many other objects, such as spells, mounts, achieves and, especially, they handle this case properly, I think. I will see how they do it and try to do the same.
Here's how they do it: https://github.com/silverwind/idTip/blob/master/idTip.lua#L241
It's hacky. It checks if the tradeskill window is visible while adding the info to the tooltip. I'm sure the author isn't happy about it either.
I'm no stranger to having to use hacky workarounds, it really sucks having to do so...ending up having to deal with major migraines there.
Changing the function to this should work now:
local function OnTooltipSetItem(tooltip)
local _, link = tooltip:GetItem()
-- happens when looking at crafting spells at profession trainer, despite this function
-- only being called on item tooltips: blizzard strangeness.
if not link then
return
end
local itemIdStr = strmatch(link, 'item:(%d*)')
-- happens when looking at crafting reagents. Unfortunate and expensive hack due to more blizzard
-- strangeness
-- source: idTip at https://github.com/silverwind/idTip/blob/63ca82d84a5e050fd6e83ee20da863e164192a1a/idTip.lua#L240
if itemIdStr == "" and TradeSkillFrame ~= nil and TradeSkillFrame:IsVisible() and GetMouseFocus().reagentIndex then
local selectedRecipe = TradeSkillFrame.RecipeList:GetSelectedRecipeID()
for i = 1, 8 do
if GetMouseFocus().reagentIndex == i then
itemIdStr = C_TradeSkillUI.GetRecipeReagentItemLink(selectedRecipe, i):match("item:(%d*)") or nil
break
end
end
end
if not itemIdStr then
return
end
itemId = tonumber(itemIdStr)
local left, right = tooltipString(itemId)
tooltip:AddDoubleLine(left, right);
tooltip:Show()
end
If you don't mind, please change the function in your installed addon and see if it works for you too.
It's the super bowl, so I will not get around to merging and releasing this fix until sometime this coming week.
new one, from picking up 'doomhammer' in the legion quest for it. really think there should be fallback code in place in this segment.
Is this related to tradeskill reagents?
If not, please create a new issue. Not trying to be mean here. It's about project management. Issues should be about one thing, so that a fix can be made to just that issue, not a multitude of them.
Also, please take a look at https://guides.github.com/features/mastering-markdown/ and use
code blocks (see "Syntax highlighting" section in that link)
when you want to include output from a program. It's nearly impossible to read comments that do not have that. You can use the "Preview" tab on top of the comment input box to test if your Markdown is working right.
Message: Interface\AddOns\ItemVersion\Api.lua:12: attempt to index field '?' (a nil value)
Time: Fri Feb 12 15:49:42 2021
Count: 1
Stack: Interface\AddOns\ItemVersion\Api.lua:12: attempt to index field '?' (a nil value)
[string "@Interface\AddOns\ItemVersion\Api.lua"]:12: in function `getItemExpac'
[string "@Interface\AddOns\ItemVersion\Tooltip.lua"]:6: in function <Interface\AddOns\ItemVersion\Tooltip.lua:4>
[string "@Interface\AddOns\ItemVersion\Tooltip.lua"]:44: in function <Interface\AddOns\ItemVersion\Tooltip.lua:14>
[string "=[C]"]: ?
[string "=[C]"]: in function `SetLFGDungeonReward'
[string "*:OnEnter"]:5: in function <[string "*:OnEnter"]:1>
Locals: itemId = nil
(*temporary) = <table> {
7 = <table> {
}
1 = <table> {
}
2 = <table> {
}
4 = <table> {
}
8 = <table> {
}
9 = <table> {
}
5 = <table> {
}
3 = <table> {
}
6 = <table> {
}
}
(*temporary) = nil
(*temporary) = nil
(*temporary) = "attempt to index field '?' (a nil value)"
Private = <table> {
itemIdToVersionId = <table> {
}
majorToExpac = <table> {
}
versionIdToVersion = <table> {
}
}
this is with the fix discussed earlier, when mousing over an anima item in the LFG interface (such as from the daily dungeon reward). without it, the tooltip works, albeit improperly (same issues as with reagents). It should also be noted that my debug code returned a similarly mangled item tooltip, as in the issue I linked.
These errors further reinforce my believe that there should either be a fallback, or the item version string should not be added, in the case that the addon cannot properly gather the information; we keep finding further examples of Blizz's code making it difficult.
Hey there @Kherae. Sorry for the delay. I live in Texas, and we recently had a huge blizzard that make things difficult.
These errors further reinforce my believe that there should either be a fallback, or the item version string should not be added, in the case that the addon cannot properly gather the information;
I went ahead and fixed this issue in 4c9e68a and db29e72. The implementation was to simply show nothing in these cases. I fixed up the regular expression that extracts itemId
s. I also added a feature to print an error message if the id does not exist in the database.
I understand if that's not really the answer you wanted to hear. We did find a workaround in idTip, but as you've seen, I can't copy from that project unless they provide a license that allows it. I understood your comment there about there being no other way to do it, but this solution is so unique that I don't think that explanation would hold up if accusations were made.
If you happen to find another addon that does something similar with a compatible license or some written code-reuse guidelines, then we can do this! If not, the issue I opened with that project (silverwind/idTip#69) (and even the PR I just made adding a license (silverwind/idTip#70)) might get resolved soon™ and I can add the fix I commented above.
At the end of the day, it's really unfortunate Blizzard broke the tooltips in this way. That's why we're having this issue to begin with.
I will release version 2021.8.3
tonight and you can test it out.
Released: https://www.curseforge.com/wow/addons/itemversion/files/3222555
(Version number is actually 2021.9.0)
heard about the blizzard, hope everything's okay there. as for the implementation...it's definitely an acceptable outcome, considering. and yes, a shame that blizz botched the UI code somehow.
testing thus far shows everything working about as well as can be hoped. no errors thus far.