SFX-WoW / Masque

A button-skinning engine for World of Warcraft.
Other
44 stars 11 forks source link

Bug: Group callback not triggering when changing most config settings #316

Closed Mayron closed 1 year ago

Mayron commented 1 year ago

Game Flavor

Retail, Classic Era, Classic Wrath

Game Version

3.4.1

Add-On Version

10.0.5

Description

I have read the documentation for the Group API and saw that I can register a callback to trigger when a setting is changed for my addon's group:

https://github.com/SFX-WoW/Masque/wiki/SetCallback

It reads:

Callbacks registered to a group will only be fired when that group's skin settings change.

So from this, I assumed it would fire for any group-level setting I changed. In my case, I wanted it to trigger my callback if I toggle on or off the Gloss option, or update colors.

However, the bug seems to be that it only triggers my callback for the Scaling related options, as well as if I disable or enable the entire group. I noticed that it only triggers my callback if the group's ReSkin method is called (as shown on lines #362, #365, and #376):

https://github.com/SFX-WoW/Masque/blob/main/Core/Group.lua#L352

Is the documentation misleading and this is expected? Or can this be changed so that it fires my callback? If you don't need to reskin for all settings then maybe just call the callback outside of the ReSkin method?

Thanks for the great work.

Error Message

No response

Steps to Reproduce

  1. Add a callback for an addon group like so:
local masque = _G.LibStub("Masque", true);
local masqueGroup = masque:Group("MyAddOn", "Auras");

masqueGroup:SetCallback(function(GroupID, Group, SkinID, Backdrop, Shadow, Gloss, Colors, Disabled)
  print("Hello from my callback!")
  print(GroupID, Group, SkinID, Backdrop, Shadow, Gloss, Colors, Disabled);
end);
  1. Go into the Masque config menu and navigate to your group's settings (e.g., MyAddOn > Auras).

  2. Toggle on and off the Gloss effect setting and other settings and notice that only the setting to Disable/Enable the group and changing its Scale-related settings will call the group's callback function to print the above messages.

Screenshots

no-callback.webm

Notes

No response

StormFX commented 1 year ago

You are correct. It should fire when settings in the options panel are changed. I must have neglected that part when I switched to region-specific functions rather than simply reskinning the entire button.

StormFX commented 1 year ago

Just an update: I'm going to rework the group callbacks so that instead of returning all the group's setting every time any setting is changed, it only returns the setting name and value that was changed. This should slightly reduce overhead in cases where only a single setting is monitored.

Mayron commented 1 year ago

That's all that I need and yes that sounds more practical. Thanks for the update!

StormFX commented 1 year ago

When Masque 10.0.7 is released, you'll be able to register your callback as follows:

local MSQ = LibStub("Masque", true)

if MSQ then
    local Group = MSQ:Group("MyAddon", "MyGroup")

    if Group.RegisterCallback then
        -- Using a function.
        local function myCallback(Group, Option, Value)
            if Option == "Gloss" then
                if Value then
                    -- Do stuff
                else
                    -- Do other stuff
                end
            end
        end

        Group:RegisterCallback(myCallback)

        -- Using a method.
        function myAddon:myCallback(Group, Option, Value)
        --  function myAddon.myCallback(self, Group, Option, Value)
            if Option == "Gloss" then
                if Value then
                    -- Do stuff
                else
                    -- Do other stuff
                end
            end
        end

        Group:RegisterCallback(myCallback, myAddon)
    end
end

The callback will only be fired on the group or groups that have a callback registered and it will return group object, the string name of the option that was changed and its new value, instead of returning all of the values every time any option is changed.

Edit: I do recommend doing a version check by testing for the RegisterCallback method of the group API, as shown above.

StormFX commented 1 year ago

This should be fixed in today's release. If you run into any problems, let me know. Closing.

Mayron commented 1 year ago

@StormFX thank you! This works perfectly and as expected. Nice work!

StormFX commented 1 year ago

If you ever hop into my Discord, DM so that I can give the appropriate roles. Thanks for the support! <3

Edit: Also, need your preferred handle so that I can update the supporters list.