Rogue can't apply poison to main hand weapon
jdelvare opened this issue · 8 comments
Since version 2023.4.1, BOM refuses to apply any poison to my main hand weapon. Poison can still be applied to my off hand weapon.
The message I get is: "Can't enchant main hand".
Last known working version for me was 2023.2.0. I will test 2023.4.0.
This is a confusing report, because i play rogue quite often and the poison feature was functioning for me.
Version 2023.4.0 works fine. The regression is in version 2023.4.1.
Unfortunately, neither 2023.4.0 nor 2023.4.1 is tagged in git. Last tag in git is 2022.12.2. It would be nice to have all releases properly tagged to make it easier to investigate regressions.
Bug found. Most likely you are not affected because you are playing in English.
The bug is that you check the main hand item type and sub type by name ("Weapon" and "Fishing Poles"). These ItemInfo fields are localized, so when playing in a language different that English, the strings are different ("Arme" and "Cannes à pêche" for me in French).
The fix is to use the numeric IDs instead of the strings. Here's a patch I wrote, which makes things work again for me.
buffomat-fix-weapon-type-check.patch.txt
(Patch may not apply easily due to the mix of line terminators in the original source tree - some files use DOS-style CRLF while others use UNIX-style LF. But at least you can see what I did to address the problem.)
We may have to update function itemCacheModule:LoadItem() with the new fields as well for consistency, I admit I don't quite understand the difference between BOM.GetItemInfo() and itemCacheModule:LoadItem().
BOM.GetItemInfo does immediate ::GetItemInfo call, and caches it. This is all good until you need to query a few hundred items (and spells via GetSpellInfo) on addon startup, which can lead to missing item icons, names, and instant disconnect too. The solution is to use itemMixin (in itemCacheModule:LoadItem), and let it call back when info is ready - this is used on startup to prevent these problems from happening.
I have added explanation to both functions, and added those new 2 fields. Thanks for noting.
Thanks for adding the comments. Not sure why you wrote that BOM.GetItemInfo "does not cache" though, as this function is clearly reading from, and writing to, itemCacheModule.cache[].