Integrated Classic support #184

inyawfaze · 2024-11-21T18:38:23Z

Native support for Classic this time.

HelpTips and several widgets will not get loaded for Classic, via exclusion from it's .toc.
Unsupported frames (focus/boss) are excluded for Classic.

Exclusion of these features from the baseline const, defaults and builder tables & conditionally adding them in for the Retail version.

Some util functions are added in esp. regarding auras.
I spent quite some time researching how to retrieve the buff index needed for the aura tooltips in Classic, an on demand util function seemed reasonable, as the alternative requires constantly mapping the index on every buff change.

Following this approach now only available features will be visible in the addon menu.
Role icons are disabled for Classic for now, as their LibGroupInfo requirement from Cell is not loaded for Classic. This should be able to get reimplemented fairly easily.

Typings: definitely not my strongsuit. Had to change some enums to classes as they have keys included conditionally. At least that's what my LuaLS suggested, I hope thats the correct approach. These changes will definitely add some warnings.

Retail: every feature I took out for Classic worked for me in Retail. I don't think there should be any regression/breaking changes in this.

I think this approach is way healthier than my last one, I'm happy I redid this (and will keep working on it if it's not up to standard)!

TLDR/Commit Message:

Seperate .toc which does not load HelpTips and some widgets
adjusted constants and defaults
removes unsupported widgets from builder
removes unsupported health formats
helper functions for missing AuraUtil
seperate aura tooltip handling
several util functions

- Seperate .toc which does not load HelpTips and some widgets - adjusted constants and defaults - removes unsupported widgets from builder - removes unsupported health formats - helper functions for missing AuraUtil - seperate aura tooltip handling - several util functions

inyawfaze · 2024-11-21T18:43:12Z

Yeah, the lua ci runner does not seem to approve my changes. I'm not too sure if I can fix those myself, but I'll take a look and learn about it. 😅

Krealle · 2024-11-22T11:16:22Z

I will try to take a look over this later today - but changing enums to classes is a big no-no.

Enums essentially describe an input to be x value out of y enum table.
Whilst a class is used to define a table.

So if you have a function that expects a string to be one of the entries in an enum, then you can type it as such, but if it becomes a class, then it would expect it to be a table.

Krealle

Okay, lots of stuff going on here. Overall the direction you went with this PR is in the right one.
There are things that need to addressed and changed however.

As for typing I've tried to point out the areas where it went wrong and how to resolve them.

As for the HelpTips - The file should still be loaded, but just make dummy functions for Acknowledge and Show funcs so we can reduce complexity. Could also just be a HelpTipVanilla.lua file.

I have some reservations about how the Defaults are handled, but that's not really something that I'm that concerned with atm.

If you have any questions about any of the comments feel free to ask them.

Krealle · 2024-11-22T18:01:13Z

Cell_UnitFrames_Vanilla.toc

+# Widgets/Bars/HealAbsorb.lua
+
+Widgets/Icons/RaidIcon.lua
+# Widgets/Icons/RoleIcon.lua # this could get reintegrated via LibGroupInfo support


I'm not sure why this would be turned off, since it doesn't actually use a lib, but UnitGroupRolesAssigned, which should be functional in classic.
https://warcraft.wiki.gg/wiki/API_UnitGroupRolesAssigned

Krealle · 2024-11-22T18:03:03Z

Data/Constants.lua

+CUF.vars.isRetail = WOW_PROJECT_ID == WOW_PROJECT_MAINLINE
+CUF.vars.isVanilla = WOW_PROJECT_ID == WOW_PROJECT_CLASSIC
+CUF.vars.isBCC = WOW_PROJECT_ID == WOW_PROJECT_BURNING_CRUSADE_CLASSIC and LE_EXPANSION_LEVEL_CURRENT == LE_EXPANSION_BURNING_CRUSADE
+CUF.vars.isWrath = WOW_PROJECT_ID == WOW_PROJECT_WRATH_CLASSIC
+CUF.vars.isCata = WOW_PROJECT_ID == WOW_PROJECT_CATACLYSM_CLASSIC
+CUF.vars.isTWW = LE_EXPANSION_LEVEL_CURRENT == LE_EXPANSION_WAR_WITHIN


I think it would make more sense to have these be set in Init.lua.

Krealle · 2024-11-22T18:19:48Z

Menu/Builder.lua

+        if CUF.vars.isRetail then
+            CUF.HelpTips:Acknowledge(tagHint, L.HelpTip_TagHintButton)
+        end
    end)

-    CUF.HelpTips:Show(tagHint, {
-        text = L.HelpTip_TagHintButton,
-        dbKey = "tagHintButton_Builder",
-        buttonStyle = HelpTip.ButtonStyle.None,
-        alignment = HelpTip.Alignment.Center,
-        targetPoint = HelpTip.Point.LeftEdgeCenter,
-    })
+    if CUF.vars.isRetail then
+        CUF.HelpTips:Show(tagHint, {
+            text = L.HelpTip_TagHintButton,
+            dbKey = "tagHintButton_Builder",
+            buttonStyle = HelpTip.ButtonStyle.None,
+            alignment = HelpTip.Alignment.Center,
+            targetPoint = HelpTip.Point.LeftEdgeCenter,
+        })
+    end


I think in regards to HelpTips, that it would be better to simply create dummy functions instead of adding all of these if statements across the codebase.

Would also make any future implementation easier.

Krealle · 2024-11-22T18:22:19Z

Menu/Builder.lua


+if CUF.vars.isRetail then
+    Builder.MenuOptions.ClassBarOptions = 22;
+    Builder.MenuOptions.CastBarEmpower = 26;
+    Builder.MenuOptions.ShieldBarOptions = 30;
+end
+


It's unnecessary to gate these behind flavor, since they'll only be used if the widget for them are enabled.
So better to avoid the added complexity, and having to deal with typing issues.

Krealle · 2024-11-22T18:23:00Z

Menu/GeneralTab.lua

        self:ShowBlizzardFramesPopup()
-        CUF.HelpTips:Acknowledge(self.blizzardFramesButton, L.HelpTip_BlizzardFramesToggle)
+        if CUF.vars.isRetail then
+            CUF.HelpTips:Acknowledge(self.blizzardFramesButton, L.HelpTip_BlizzardFramesToggle)
+        end
    end)
    self.blizzardFramesButton:SetPoint("TOPLEFT", self.useScalingCB, "BOTTOMLEFT", 0, -10)

-    CUF.HelpTips:Show(self.blizzardFramesButton, {
-        text = L.HelpTip_BlizzardFramesToggle,
-        dbKey = "blizzardFramesToggle",
-        buttonStyle = HelpTip.ButtonStyle.None,
-        alignment = HelpTip.Alignment.Center,
-        targetPoint = HelpTip.Point.TopEdgeCenter,
-    })
+    if CUF.vars.isRetail then
+        CUF.HelpTips:Show(self.blizzardFramesButton, {
+            text = L.HelpTip_BlizzardFramesToggle,
+            dbKey = "blizzardFramesToggle",
+            buttonStyle = HelpTip.ButtonStyle.None,
+            alignment = HelpTip.Alignment.Center,
+            targetPoint = HelpTip.Point.TopEdgeCenter,
+        })
+    end


same here, remove guard clause.

Krealle · 2024-11-22T18:52:53Z

Widgets/Bars/CastBar.lua

    castBar.channeling = event == "UNIT_SPELLCAST_CHANNEL_START"
-    castBar.empowering = event == "UNIT_SPELLCAST_EMPOWER_START"

-    if castBar.empowering then
-        endTime = endTime + GetUnitEmpowerHoldAtMaxTime(unit)
+    if CUF.vars.isRetail then
+        castBar.empowering = event == "UNIT_SPELLCAST_EMPOWER_START"
+        if castBar.empowering then
+            endTime = endTime + GetUnitEmpowerHoldAtMaxTime(unit)
+        end
    end


Unnecessary guard clause since the event can never trigger in non mainline flavor.

Krealle · 2024-11-22T18:53:20Z

Widgets/Bars/CastBar.lua


-    if castBar.empowering then
+    if CUF.vars.isRetail and castBar.empowering then
        castBar:AddStages(numStages)
    end


Same as above, unnecessary guard clause.

Krealle · 2024-11-22T19:03:10Z

Data/Defaults.lua

+if CUF.vars.isRetail then
+    ---@type ShieldBarWidgetTable
+    Defaults.Widgets.shieldBar = {
+        enabled = false,
+        frameLevel = 9,
+        point = "RIGHT",
+        reverseFill = false,
+        overShield = false,
+    }
+end


Shouldn't Class bar and heal absorb also be gated here?

Krealle · 2024-11-22T19:07:14Z

Data/Defaults.lua

+
+-- Focus was introduced with TBC
+if not CUF.vars.isVanilla then
+    Defaults.Layouts.focus = {


You need to use the const to make the typing happy, it's a bit dumb xd

Suggested change

Defaults.Layouts.focus = {

Defaults.Layouts[CUF.constants.UNIT.FOCUS]

Krealle · 2024-11-22T19:13:34Z

Data/Constants.lua

+
+-- Focus was introduced with TBC
+if not CUF.vars.isVanilla then
+    table.insert(const.BlizzardFrameTypes, CUF.constants.UNIT.FOCUS)
+end
+
+-- Boss frames were intoduced with Wrath
+if not CUF.vars.isVanilla and not CUF.vars.isBCC then
+    table.insert(const.BlizzardFrameTypes, CUF.constants.UNIT.BOSS)
+end


NIT: inserting like this will mess up the order in which they are displayed.

inyawfaze · 2024-11-22T21:45:59Z

Awesome feedback, thanks!
I'll be going over this tomorrow and make adjustments where needed.

Krealle · 2024-11-24T12:55:13Z

I also think you should add ---@diagnostic disable: inject-field to the top of Constants.lua. Should help with removing all the typing nightmares there.

Krealle requested changes Nov 22, 2024

View reviewed changes

Krealle mentioned this pull request Nov 27, 2024

[Feature] Classic Era version #186

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrated Classic support #184

Integrated Classic support #184

inyawfaze commented Nov 21, 2024

inyawfaze commented Nov 21, 2024

Krealle commented Nov 22, 2024

Krealle left a comment

Krealle Nov 22, 2024

Krealle Nov 22, 2024

Krealle Nov 22, 2024

Krealle Nov 22, 2024

Krealle Nov 22, 2024

Krealle Nov 22, 2024

Krealle Nov 22, 2024

Krealle Nov 22, 2024

Krealle Nov 22, 2024

Krealle Nov 22, 2024

inyawfaze commented Nov 22, 2024

Krealle commented Nov 24, 2024

	Defaults.Layouts.focus = {
	Defaults.Layouts[CUF.constants.UNIT.FOCUS]

Integrated Classic support #184

Are you sure you want to change the base?

Integrated Classic support #184

Conversation

inyawfaze commented Nov 21, 2024

inyawfaze commented Nov 21, 2024

Krealle commented Nov 22, 2024

Krealle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inyawfaze commented Nov 22, 2024

Krealle commented Nov 24, 2024