varren / ViragDevTool

World of Warcraft Debugger developer addon
36 stars 12 forks source link

Updates for Shadowlands and convert to Color Mixin API #11

Closed brittyazel closed 2 years ago

brittyazel commented 3 years ago

Hi Varren,

I thought I'd try helping out a bit. I went through and did a round of cleanups and fixes, notably many XML fixes. Further, I noticed how many times in the code strings were colored using hex values concatenations, so I did my best to port the addon over to the new Color Mixin API. I think I replicated the way you had it before, but I'm sure I may have missed one or two formatting cues.

Let me know if you have any questions or if you want anything. I would love to get this merged and get a version out for Shadowlands :-)

Oh, and I changed the border from a transparent red to an opaque gray, much in the spirit of a "dark mode" and IDE may have. Feel free to reject that commit if you want to stick with the red.

CDAGaming commented 3 years ago

@brittyazel This looks really nice, though I have a few bugs to report (Albeit these bugs are present before this PR, but I wanted to bring them to light here

image

For some reason, there is a green small button on the top right, in addition to the glow on the info button being offset vertically by a bit. If both of those can be diagnosed, that would be good, but so far this pr is LGTM!

Edit: IT appears that green slider is actually a column resize button. It would make sense that this button should actually be properly integrated into the UI, if this is possible.

Edit 2: For the glow on the info button, the following change to ViragDevTool.xml would suffice: image

brittyazel commented 3 years ago

Yeah that green button is the column slider. I'll see what I can do. I can definitely make it "look" better. I think it's defined in the XML, so tweaking the color/opacity there should be reasonable.

I added your changes. The column separator isn't "great", but it's the best I can do in the 20min before my monday morning meetings lol

CDAGaming commented 3 years ago

@brittyazel Looking much better!

I also found a bug which may be related to the color changes: image

brittyazel commented 3 years ago

Hrm That's a weird bug, it's showing in Blizzards XML parser, which means it's some change I made to the xml. How can I replicate it?

CDAGaming commented 3 years ago

@brittyazel Not sure, it just happened randomly for me, though it's showing it's deriving from:

resultStr = concat(self.colors.lightblue:WrapTextInColorCode(name), self.colors.gray:WrapTextInColorCode("<"), self.colors.gray:WrapTextInColorCode(">"))

Which was adjusted in ccbd7a1d6881c4552d0811e4501910210f7e4edf .

brittyazel commented 3 years ago

Can you try that patch and see if it triggers again?

CDAGaming commented 3 years ago

@brittyazel Seems to be fixed/stopped occurring on my end!

brittyazel commented 3 years ago

seems like the variable "name" may or may not exist, and that trying to set a color on a non-existent string throws an error lol

CDAGaming commented 3 years ago

Would make sense, since the way the old line was: resultStr = self.colors.gray:WrapTextInColorCode(concat(name, "<", ">"))

Is in such a way that name never had a color put on itself, but broke in your implementation since you added additional colors to the line that effected name itself.

brittyazel commented 3 years ago

any thoughts @varren? I'm happy to make any changes you like

CDAGaming commented 3 years ago

any thoughts @varren? I'm happy to make any changes you like

varren's been inactive on github for the last few months, unfortunatly.

brittyazel commented 3 years ago

I'm not exactly sure how to split my pull requests, but I did an initial port of the addon to using Ace3 libs, such as Ace-addon, Ace-console, Ace-db. In the future we could use Ace-locale and others if necessary. I'm not sure if you ever intended on ViragDevTools using an addon framework, but I thought it could greatly simplify the loading and unloading process as well as handle database management.

varren commented 3 years ago

@brittyazel sry i was away from github for quite some time and missed this thread completely. I actually don't have WoW installed rights now, so can't even check the pull request, sry.

brittyazel commented 3 years ago

All good. I'm not playing much right now either. If you ever have time, please check it out, if not it's fine too.