Add IconOverlay2 to Item Button

[shawngmc]

Add IconOverlay2 support to ItemButton in a future-proof way, and add the call to an addon.isRetail guard block.

Replacement for #445. (My original fork has been deleted and there are a number of changes that would need brought up to speed.)

[shawngmc]

Note: I tested this with a piece of Azerite gear.

[Meivyn]

Technically you add support for both overlays (IconOverlay and IconOverlay2), the frames are already created on the button by the base game. I am unsure what the childrenNames are used for, so not sure if adding the overlays to it is useful either.

I know that I suggested this change, but I'm wondering if we could avoid making that many calls to GetContainerItemInfo by caching it somewhere, but I haven't looked into it, and this code is completely fine as is. The or 0 is there to make sure that if the slot is empty, SetItemButtonOverlay would still handle hiding the overlays. In this case however, we might want to avoid the call:

if self.hasItem then
	local _, _, quality = GetContainerItemInfo(self.bag, self.slot)
	SetItemButtonOverlay(self, self.itemId or self.itemLink, quality)
else
	SetItemButtonOverlay(self, 0)
end

[Cidan]

but I'm wondering if we could avoid making that many calls to GetContainerItemInfo by caching it somewhere

GetContainerItemInfo is already cached locally in the WoW client - this call is not networked unless there's a miss.

I haven't had a chance to check the code on this yet (busy week at my day job!), but can someone please confirm this code does not break Classic and Wrath as well? Also, please be sure to test as many interactions as possible, i.e. sort your bags after messing them up, deposit and remove from the bank, open the bag in "Backback" sorted mode and try to break it, etc. This has to be done for all three versions of WoW on the market today.

Thanks!

[Cidan]

(Note: I see that it's in an addon.isRetail block, but the code must be tested everywhere regardless please. Thank you!)

[Meivyn]

GetContainerItemInfo is already cached locally in the WoW client - this call is not networked unless there's a miss.

Isn't it still faster/less overhead to cache the values and access them than querying the method every time?

(Note: I see that it's in an addon.isRetail block, but the code must be tested everywhere regardless please. Thank you!)

Have to say that I do not see the relevance of testing this outside of Retail considering that it's not possible that this condition fails. The code would never run - you would basically only test self.isRetail which we know wouldn't fail already. That doesn't make sense to me.

Having said that, I did notice some bug while testing it, but it isn't related. IsSameLinkButLevel seems to incorrectly return true and the GUID query doesn't account for multiple stacks on the same slot. Other than that, nothing to report.

As a side note, I find the new behavior of marking swapped items as "Recent Items" not really convenient and especially annoying for testing.

[Cidan]

Isn't it still faster/less overhead to cache the values and access them than querying the method every time?

It may be, I have not tested nor benched this, but generally this call is on the order of microseconds. We'd be adding complexity around cache invalidation, and adding more memory pressure to the client should we build a cache for this. If this becomes an issue in the future, we can evaluate this, but I suspect it won't be.

Have to say that I do not see the relevance of testing this outside of Retail considering that it's not possible that this condition fails.

Because the code is loaded in all environments, all environments must be tested and QA'd, regardless of well-known paths. This is to ensure regressions not only in our code, but in Blizzard's code, do not cause issues. Unfortunately, because Blizzard has decided to significantly alter the API in some instances between versions, one single regression could break this addon for thousands or tens of thousands of people, even when gated appropriately behind a flag. We recently witnessed this with Wrath when Blizzard changed the flags used to detect if a user is in Wrath -- this broke addon.isWrath.

Having said that, I did notice some bug while testing it, but it isn't related. IsSameLinkButLevel seems to incorrectly return true and the GUID query doesn't account for multiple stacks on the same slot. Other than that, nothing to report.

Can you open a bug report for this, with your exact tests, a reproduction case, items used, etc?

As a side note, I find the new behavior of marking swapped items as "Recent Items" not really convenient and especially annoying for testing.

Testing is a side-effect in this instance. Please feel free to issue a feature request on this, with a proposed design so we can evaluate it.

Thanks!

[Meivyn]

We'd be adding complexity around cache invalidation, and adding more memory pressure to the client should we build a cache for this.

I'd argue that we do not really care about memory pressure in Lua. But I agree that it might get complicated for not much. This is likely not related, but I was thinking about this to avoid issues like the freeze that happens when you sort the bags, which doesn't happen on base UI. So there's likely some progress that can be made on the optimization.

We recently witnessed this with Wrath when Blizzard changed the flags used to detect if a user is in Wrath -- this broke addon.isWrath.

I still can't believe that they've done this out of nowhere, and broke multiple add-ons, including Bartender, with a minor patch. But you make a point. Either way, I already tested this on Wrath and Vanilla. And I just completed testing it on Retail.

Testing is a side-effect in this instance. Please feel free to issue a feature request on this, with a proposed design so we can evaluate it.

I mean, I simply expect it to behave like when sameItem is true. Simply swap the item where you put them, and do not move them into other categories (i.e. Recent Items). Not sure if this is actually an intended behavior or not either. To me, "Recent Items" should contain only new items that you looted recently. So I would personally consider this a bug, not a feature request.

[Cidan]

I mean, I simply expect it to behave like when sameItem is true. Simply swap the item where you put them, and do not move them into other categories (i.e. Recent Items). Not sure if this is actually an intended behavior or not either. To me, "Recent Items" should contain only new items that you looted recently. So I would personally consider this a bug, not a feature request.

Please open a bug on this if you feel this is a bug -- let's not pollute the PR discussion please.

@shawngmc Are you ready to merge these changes?

edit: Also open a bug on the bag freeze, please.

[Talyrius]

Technically you add support for both overlays (IconOverlay and IconOverlay2), the frames are already created on the button by the base game. I am unsure what the childrenNames are used for, so not sure if adding the overlays to it is useful either.

This is probably worth looking into. In any case, it doesn't make sense to add the second one without adding the first as well.

I know that I suggested this change, but I'm wondering if we could avoid making that many calls to GetContainerItemInfo by caching it somewhere, but I haven't looked into it, and this code is completely fine as is. The or 0 is there to make sure that if the slot is empty, SetItemButtonOverlay would still handle hiding the overlays. In this case however, we might want to avoid the call:

if self.hasItem then
	local _, _, quality = GetContainerItemInfo(self.bag, self.slot)
	SetItemButtonOverlay(self, self.itemId or self.itemLink, quality)
else
	SetItemButtonOverlay(self, 0)
end

Additional function calls should be avoided whenever possible for optimal performance in Lua. They're much more resource expensive than the added conditional checking—at the expense of extra code verbosity, of course.

[Cidan]

Additional function calls should be avoided whenever possible for optimal performance in Lua.

While true, I think this sort of optimization doesn't behoove a bag addon, whose calls are not continuous, with the CPU's of 2022. I want to be careful about pre-optimizing something that hasn't been shown to be a problem arbitrarily for the sake of being CPU time optimal, at the cost of readability.

There are other parts of the code that are very much in need of time optimization, and I have a few ideas on how to tackle them. However, it will require a significant amount of code rewrite, and should be done at a later date.

[Talyrius]

Implemented in #865 via 720c5ae.