ItemVersion

ItemVersion

216k Downloads

lua error in tradeskill reagents mouseover

Kherae opened this issue · 17 comments

commented

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
commented

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.

commented

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.

Untitled

commented
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.

commented

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

commented

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:

image

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.

commented

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.

commented

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.

commented

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.

commented

Untitled

commented

looks like that did it.

commented
Edit: moving this stuff to https://github.com//issues/5
commented

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.

commented
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.

commented

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.

commented

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 itemIds. 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.

commented

Released: https://www.curseforge.com/wow/addons/itemversion/files/3222555

(Version number is actually 2021.9.0)

commented

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.