raethkcj / RatingBuster

An item comparison tool for WoW Classic.
GNU General Public License v2.0
57 stars 14 forks source link

Compatibility changes #84

Closed Anonomit closed 2 years ago

Anonomit commented 2 years ago

I'm working on an addon which is also modifying tooltip lines. It's currently compatible with RatingBuster in enUS only, but these changes should allow for compatibility in any locale (without a lot of extra pattern matching).

What I'd like is to be able to have RatingBuster modify a single line of text at a time. I've pulled a bit out of the ProcessTooltip function and made it into a new ProcessLine function which can be run externally on any line of text. I could elaborate on why this is necessary if you'd like, but it makes compatibility far easier to be able to add in RatingBuster changes after my own.

I also have found that manually setting tooltip/fontString widths is unnecessary. I've been modifying tooltip lines for around a year and have never needed to manually set a width. It seems to recalculate padding in the OnShow() handler (as indicated here). So calling tooltip:Show() after modifying text ought be all you need (you already do this). If you set the width manually, then any other addons which modify text must also set the width manually since the automatic padding will no longer work.

As for da54c77eb4e2968b5ac166c13d0500921fe0c8c4, I don't know what originally caused #55 for some people, but I suspect that it also had to do with manually setting widths.

Anonomit commented 2 years ago

I guess I should link this too. Here's the addon in question which is also modifying tooltips: ZeraTooltip. It's being almost entirely rewritten for wrath (it's currently a huge mess), and that's not on github yet, but hopefully you get the idea of what I'm doing and why these changes make compatibility easier/possible with everything I plan to add. I'll update the repo once I have more of the features rebuilt.

Anyway, if you want to chat about tooltip or addon stuff sometime then I'd be happy to connect over discord or ingame. I have a few tooltip ideas that I plan to implement and I would definitely like to keep things compatible with RatingBuster.

raethkcj commented 2 years ago

Hi! I'm definitely open to making changes for compatibility.

That line resizing was a workaround for a very strange bug where Tooltip:Show() was not properly recalculating width.

It occurred in Classic on any item with a "damage and healing" stat, when RatingBuster modified lines to add stat breakdowns: truncated_tooltip The Intellect line here was inserted intentionally to break it for this example, but it occurred all over the place in normal use of RatingBuster. The only code used to reproduce this was:

GameTooltip:HookScript("OnTooltipSetItem", function(tooltip)
      _G[tooltip:GetName().."TextLeft2"]:SetText("+22 Intellect (+0.39% Spell Crit, +2.2 MP5)")
      tooltip:Show()
end)

Like you said, the tooltip:Show() should have been enough to recalculate the width, but in this case it truncated the final two words on a completely separate line (the "damage and healing" line should end in "to 19"). The bug wasn't caused by the exact text, but by some fractional computed width of the characters in the Intellect line.

The workaround was intended to modify the width the smallest amount possible; it simply rounded the current width up to the nearest whole pixel. The widening bug in #55 was a side effect of both this workaround and a separate change; when the original workaround was written, RatingBuster was using hooksecurefunc("GameTooltip", "SetInventoryItem") among other secure hooks, but the widening bug didn't occur until the hooking was switched to use GameTooltip:HookScript. Modifying the width of the fontString instead of the tooltip in https://github.com/raethkcj/RatingBuster/commit/da54c77eb4e2968b5ac166c13d0500921fe0c8c4 then worked as a fix for both bugs.

The bug persisted for all of Classic and at least several months of TBC; I stopped tracking it after I implemented the workaround. However, I'm currently unable to reproduce it on the Classic SoM PTR. So it seems either Blizzard implemented a fix for it directly, or it was fixed as a side effect of some other change.

If the bug really is gone, then I'm all for removing the resizing, but I'll try to hack at a bit first to make sure I haven't just forgotten how to reproduce it.

As for ProcessLine, that should be fine. It looks like I'll need to add link to the args but that's more my fault than yours, as item passed to SplitDoJoin was supposed to be link. I'm sure you can see most the addon is in need of more refactoring, but I'm not the original author and just trying to keep it working for Classic 😛

Anonomit commented 2 years ago

The bug persisted for all of Classic and at least several months of TBC

That could have been before I started playing with tooltips then. Hopefully the issue is fixed. I'm mostly shortening tooltip lines rather than lengthening them, and I am specifically changing spellpower lines, so it's possible that I wouldn't have noticed even if the issue still exists. Thanks for the explanation.

hooksecurefunc("GameTooltip", "SetInventoryItem")

I'm currently using this method primarily since I'm trying to only read text from "fresh" tooltips, and I need the function name to clone a tooltip. I still have to hook OnTooltipSetItem and sometimes use it because tooltips are weird.

most the addon is in need of more refactoring

I'm aware that you're not the original author, but I was surprised to see how little pattern matching was actually being done while still supporting so many locales. RatingBuster only needs to recognize primary and secondary stats, whereas I'm trying to recognize and reword a lot more. The pattern matching for this has been harrowing.

raethkcj commented 2 years ago

Thankfully, most of the localization was done from 2008-2011 for the original version, and I'm able to just copy that and distribute it for this Classic version.

If I were to start this project from scratch, I'd probably try to leverage GlobalStrings (WoWTools or Townlong Yak for examples) as much as possible, as most text that appears in tooltips seems to come from the format strings there. You'd still have to doctor up the tokens a bit to match or extract information, such as replacing %d with (%d+) to capture numbers, but you could probably abstract most of that to make it automatic and reusable. There are also a million little localization quirks, but they can still be worked into something usable.

Anonomit commented 2 years ago

I'd probably try to leverage GlobalStrings

This is what I'm doing to recognize lines that are either a primary or secondary stat. As you've noticed, the strings are incomplete and some tooltip lines just do what they want.

You'd still have to doctor up the tokens a bit

Here's the monstrosity that I'm using currently. I got this from a friend and have only modified it slightly. I've tested it in german and korean locales possibly but I can't remember. Both those locales do stuff with patterns that I wanted to be able to read (ex. Stamina +10 instead of +10 Stamina) text:gsub("%%%d%$", "%%"):gsub("[+-]", "%%%1"):gsub("%(", "%%("):gsub("%)", "%%)"):gsub("%%c", "([+-])"):gsub("%%d", "(%%d+)"):gsub("%.", "%%."):gsub("%%s", "(.*)"):gsub("|4[^:]-:[^:]-:[^:]-;", ".-"):gsub("|4[^:]-:[^:]-;", ".-")