Totems element - looking for feedback

tecu · 04-05-11, 02:01 PM

I found out a little bit ago that shamans were not the only class to use the default TotemFrame element, and I've been meaning to update oUF_boring_totembar to not be shaman-only. I was also thinking that since DKs and druids use it too, maybe it's not really a niche feature anymore and that maybe haste would be interested in supporting it in oUF.

So I rewrote my addon with two things in mind: one, to copy the hell out of the style of the default oUF elements, and two, to only provide the features of the default Blizzard UI while allowing the user to override whatever they want.

I am posting this here to see if haste is interested in adopting my code (or if I should just release this as my own plugin), and to get feedback from other authors about what can be improved.

The plugin is attached to this post, and here is the (somewhat rambling) info text that I wrote along side it.

INTRO:

This creates a 'Totems' element for oUF. Its purpose is to provide the features of Blizzard's default TotemFrame to users of oUF. It is a departure from what I made before with oUF_boring_totembar in that it only has a very simple feature set (icon + cooldown frame), but allows the user to modify and add nearly anything they want using 'override' type functions that are common among the default oUF elements.

To accomplish this, I copied a LOT from the code style of other oUF elements, possibly at times without knowing entirely why. So if you look at the code and you see that I did something silly, please TELL ME so I can learn.

USAGE:

This mod only REQUIRES the following:

<player frame>.Totems = {
[1] = <child frame>,
[2] = <another child frame>,
...
[MAX_TOTEMS] = <the last chald frame>
}
- it will only work on frames whose unit is 'player'
- 'Totems' can be a table or a frame, it doesn't matter

The following are OPTIONAL:

Totems.disableCooldown: if not nil or false, the cooldown model will be hidden

Totems.disableTooltip: if not nil or false, the tooltip will not be shown on mouseover

Totems.disableDestroy: if not nil or false, you will not be able to right-click and destroy a totem

Totems.PreUpdateTotem = function( Totems, slot )
this will be called by the default update function before it does anything.

Totems.PostUpdateTotem = function( Totems, slot, haveTotem, name, start, duration, icon )
this will be called by the default update function just before it returns.

Totems.Override = function( self, event, slot, ... ):
if present, this function will be called on PLAYER_TOTEM_UPDATE and will completely replace the default update function.

Totems.PostSetupTotem = function( Totems, index )
this will be called just before the default totem setup function returns.

Totems.SetupTotem = function( Totems, index )
if present, this function will completely replace the default totem setup function. the default function will create a .Icon texture (if .Icon is absent or not a texture), a .Cooldown frame (if .Cooldown is absent or not a cooldown frame), and set up mouse On(Click|Enter|Leave) events.

USAGE NOTES:
- totem colors from the default ui are added to oUF's colors table if you want to use them.
- if .disableTooltip and .disableDestroy are set, the frame will not react to the mouse at all.
- a totem's slot and index are not necessarily the same, see below.

TOTEM SLOTS:
a totem 'slot' in the default ui doesn't necessarily correspond to its place on the screen. for example, on the default totem frame, the first totem is earth, but earth's slot id is two! additionally, all functions and events in the default ui operate based on the slot number rather than the totem's index. this is important if you want to override anything, but want to keep the default totem order. this mod provides two things to help with this: Totems.map and Totems.<totem>.slot .

.map is a table that allows you to look up a totem frame by a slot id. in other words, if you have a slot id, you can get the totem frame you want to adjust with Totems.map[slot].

.slot is pretty much the same thing in reverse. if you are playing with a totem frame, just look at totem.slot to know which totem it represents.

CODING NOTES AND QUESTIONS:
- what does oUF use .ForceUpdate for? i copied that part without giving it any thought.

- i also copied the Path style. i did it mostly so :RegisterEvent and :UnregisterEvent would be consistent in case the user messed with it somehow. is that a valid concern? is there more to it than this and convenience?

- do i need to disable the default totem frame

- are the Override functions and Pre/Post functions consistent with the rest of oUF? i am curious about both naming conventions and functionality really.

haste · 04-06-11, 03:41 AM

Hey,

I can't really quickly glare at your code from my phone, but I can answer some of the questions you have. Most of this post is written in chunks, so there might be some odd rambling here and there :P.

Some minor comments:

SetupTotem:
Based on your information, SetupTotem will replace user defined frames. In general oUF elements don't care what you give them, as long as the correct methods are there. So having the element replace pre-defined frames because they don't match a given type sounds like a bad idea.

I'm not really sure if it should define a Icon and Cooldown frame for you if it isn't present. In the future I don't want the elements to automatically create a bunch of frames, textures and fontstring without the layout asking for it, but I also don't want it to be a major hassle to add elements to a layout.

For now I would suggest to leave it as is, but don't replace already defined frames and such just because they aren't they type you want them to be.

Adding sane On{Click,Enter,Leave} is fine btw.

.map:
To me, this sounds like a needless complication. Zariel did the same thing in the RuneBar element before, and it really is easier to manage this in the layout. The positioning doesn't need to match the order they are assigned to the .Totem table.

Your questions:
.ForceUpdate:
Used to fake a true event to force the element to update. Mainly there to let people force oUF to update settings if something was changed by the user.

Path:
Previously oUF would register Overrides directly. This made it possible to disable a Override, without having the event unregister correctly, which means you end up with the element still being updated at times.

The solution is to proxy event registrations through a common function, which then calls the correct end point without having to know if it has been changed.

Disabling the default totem frame:
It depends. Will the default totem element show, continue to update or do anything odd if you don't?

Override and callbacks:
Still can't open zip files, so I'm not entirely sure. Override sounds correct at least.

I provide element:PreUpdate() and element:PostUpdate() "where it makes sense" and "where alternative solutions like using OnShow/OnLeave doesn't make sense". How this fits with the totem element I can't really answer.

I'm also a fan of the "implement the bare base and see what people want" feedback loop :P.

(I'll be able to take a proper look in 8 hours or so)

tecu · 04-06-11, 01:31 PM

Hm. I see what you are saying about SetupTotem and .map. One of the goals I had (that I did not mention before) was for the user to be able to have a faithful imitaiton of the default totem frame with the absolute minimum of effort (in this case, "just create four frames"). But letting the user add the .Icon and .Cooldown elements and deal with positioning themselves isn't asking much.

I'm going to hold off on changing anything until you actually look at the code, but based on your feedback I am leaning towards the following:

- do away with the mapping, the user can look at the SHAMAN_TOTEM_PRIORITIES/STANDARD_TOTEM_PRIORITIES tables themselves (or just ignore them) to set up the frames the way they want.

- remove the code that creates .Icon and .Cooldown, letting the user create them. In the update function I will assume that if those are present, they will function as expected.

- remove PostSetupTotem because it doesn't make sense to have it if I'm not creating the .Icon and .Cooldown anymore.

- remove .disableCooldown because it will be redundant. I didn't even use this in the best way before anyway.

- maybe rename Update to UpdateAll and UpdateTotem to Update so that (Pre|Post)UpdateTotem can become just (Pre|Post)Update

- leave the default TotemFrame alone. I never had an issue with it before when I left it alone, I was thinking 'better safe then sorry' when I decided to disable it this time.

Other notes:
Defining things on the frame itself like totem.name and totem.duration before calling :Show() and letting the user just register OnShow would probably work, but I like how a PostUpdate would stay consistent with other oUF elements.

And yeah, I started to go feature-crazy on the 'boring totembar' plugin and realized I needed to pare it back to the basics so I was not coding/supporting things that nobody (myself included!) cared about.

Mischback · 04-06-11, 01:45 PM

Tecu, I love your addon, just because it is so simple and "boring".

I'm really a fan of features, but please let the addon be as simple as possible! It is just great by what it is doing now, don't overcomplicate things.

When you're able to add more features, please let the API stay small. I'm not really aware of what droods and DKs do about totems (shrooms, I guess for druid, but no idea for DKs), and it would possibly be nice to incorporate this into an layout, but speaking as shaman: I don't care!

One thing I really liked of boring_totembar was the form of oUFs-API. Keep that in mind, please! I guess you haven't lost this focus, regarding your post here, but don't put too much into it. Perhaps it would be better to release a further plugin for other features.

haste · 04-06-11, 03:15 PM

This is a quick suggestion of how it could become:

Lua Code:

local parent, ns = ...
local oUF = ns.oUF
 
-- colors
-- from Interface/BUTTONS/UI-TotemBar.blp
oUF.colors.totems = {
    [FIRE_TOTEM_SLOT] = { 181/255, 073/255, 033/255 },
    [EARTH_TOTEM_SLOT] = { 074/255, 142/255, 041/255 },
    [WATER_TOTEM_SLOT] = { 057/255, 146/255, 181/255 },
    [AIR_TOTEM_SLOT] = { 132/255, 056/255, 231/255 }
}
 
local OnClick = function(self)
    DestroyTotem(self:GetID())
end
 
local OnEnter = function(self)
    if(not self:IsVisible()) then return end
 
    GameTooltip:SetOwner(self, 'ANCHOR_BOTTOMRIGHT')
    GameTooltip:SetTotem(self:GetID())
end
 
local OnLeave = function()
    GameTooltip:Hide()
end
 
local UpdateTotem = function(self, event, slot)
    local totems = self.Totems
 
    if(totems.PreUpdate) then totems:PreUpdate(slot) end
 
    local totem = totems[slot]
    local haveTotem, name, start, duration, icon = GetTotemInfo(slot)
    if(duration > 0) then
        if(totem.Icon) then
            totem.Icon:SetTexture(icon)
        end
 
        if(totem.Cooldown) then
            totem.Cooldown:SetCooldown(start, duration)
        end
 
        totem:Show()
    else
        totem:Hide()
    end
 
    if(totems.PostUpdate) then
        return totems:PostUpdate(slot, haveTotem, name, start, duration, icon)
    end
end
 
local Path = function(self, ...)
    return (self.Totems.Override or UpdateTotem) (self, ...)
end
 
local Update = function(self, event)
    for i = 1, MAX_TOTEMS do
        Path(self, event, i)
    end
end
 
local ForceUpdate = function(element)
    return Update(element.__owner, 'ForceUpdate')
end
 
local Enable = function(self)
    local totems = self.Totems
    
    if(totems) then
        totems.__owner = self
        totems.ForceUpdate = ForceUpdate
 
        for i = 1, MAX_TOTEMS do
            local totem = totems[i]
 
            totem:SetID(i)
            totem:SetScript('OnClick', OnClick)
            totem:SetScript('OnEnter', OnEnter)
            totem:SetScript('OnLeave', OnLeave)
        end
 
        self:RegisterEvent('PLAYER_TOTEM_UPDATE', Path)
 
        TotemFrame.Show = TotemFrame.Hide
        TotemFrame:Hide()
 
        TotemFrame:UnregisterEvent"PLAYER_TOTEM_UPDATE"
        TotemFrame:UnregisterEvent"PLAYER_ENTERING_WORLD"
        TotemFrame:UnregisterEvent"UPDATE_SHAPESHIFT_FORM"
        TotemFrame:UnregisterEvent"PLAYER_TALENT_UPDATE"
 
        return true
    end
end
 
local Disable = function(self)
    if(self.Totems) then
        TotemFrame.Show = nil
        TotemFrame:Show()
 
        TotemFrame:RegisterEvent"PLAYER_TOTEM_UPDATE"
        TotemFrame:RegisterEvent"PLAYER_ENTERING_WORLD"
        TotemFrame:RegisterEvent"UPDATE_SHAPESHIFT_FORM"
        TotemFrame:RegisterEvent"PLAYER_TALENT_UPDATE"
 
        self:UnregisterEvent('PLAYER_TOTEM_UPDATE', Path)
    end
end
 
oUF:AddElement("Totems", Update, Enable, Disable)

diff: http://paste.pocoo.org/raw/366821/

It's mainly a merge of the changes discussed above, with a round of sed on top of it.

Some minor notes:
- I just set OnClick, OnEnter and OnLeave because: They won't work correctly if EnableMouse isn't set and such. We could re-add a post-hook here, as people might want to replace them with their own.
- I don't check for unit == 'player' anymore. I expect people to be sane enough about it themselves.
- Entirely drycoded changes \

/.

tecu · 04-06-11, 04:33 PM

@haste:
You drycoded changes are working fine in game :D

PostUpdate:
This is definitely needed because PLAYER_TOTEM_UPDATE only fires once when you refresh a totem. If the user only watches for OnShow, they will not know if something changes in this case.

Mouse events:
A post hook or .disableX option would be needed here because if any of those OnX scripts are present on the button, it will still eat mouse clicks, even if Button:Disable() and Button:EnableMouse(false) have been called beforehand. (The scripts will still not 'work', though.) In this case maybe just check for Totems.enableMouse? That way at least the user asks you to set up default mouse behavior, and doesn't have to disable anything you did if they want to do their own thing.

Button:
Silly question, but a frame has to be a 'Button' to respond to mouse clicks right?

Question about single/double quoting strings:
I thought that single/double quotes work the same way in lua, is there a reason you use one over the other? I've been tending toward the single quotes just because I am lazy.

Anyway here's either a patch for your update that includes .enableMouse and the fully patched file. Dunno which of these you'd be interested in (I've used diff/patch like twice in my life before).
diff: http://paste.pocoo.org/raw/368125/
patched: http://paste.pocoo.org/raw/368126/

@Mischback:
I wasn't trying to add or remove things from the boring totembar plugin, but rather create something new and simple that would be very similar to what haste has been doing with other oUF elements. I have not decided anything about the future of oUF_boring_totembar; I'll have to wait and see what becomes of this first!

haste · 04-10-11, 05:17 AM

Originally Posted by tecu

PostUpdate:

I don't have any plans to remove it. If it sounded like that, then it was just me being a horrible typist.

Originally Posted by tecu

Mouse events:

Patch below uses the same solution as the aura element. So far no-one has asked for more there (but they also have a PostUpdate afterwards, which doesn't really make sense here).

Originally Posted by tecu

Button:

Yes, Frame's don't support OnClick. Patch below allows you to use a Frame instead, if you don't want any mouse interactions. Also note that using :SetScript('OnEnter', ...) and :SetScript('OnLeave', ...) will enable mouse catching if it isn't enabled.

Originally Posted by tecu

Question about single/double quoting strings:

In Lua they're equal.

I wrote the code no two different laptop, one which uses qwerty and one which uses colemak. The inconsistency mainly comes from that.

Originally Posted by tecu

Anyway here's either a patch for your update that includes .enableMouse and the fully patched file. Dunno which of these you'd be interested in (I've used diff/patch like twice in my life before).
diff: http://paste.pocoo.org/raw/368125/
patched: http://paste.pocoo.org/raw/368126/

I prefer patches. That way I can easily see what's changed between the versions, without accidentally missing something. Both work however, patches aren't exactly hard to generate :P.

---

We could just check the if the frame provided supports OnClick and if it captures mouse events:

Lua Code:

local parent, ns = ...
local oUF = ns.oUF
 
-- colors
-- from Interface/BUTTONS/UI-TotemBar.blp
oUF.colors.totems = {
    [FIRE_TOTEM_SLOT] = { 181/255, 073/255, 033/255 },
    [EARTH_TOTEM_SLOT] = { 074/255, 142/255, 041/255 },
    [WATER_TOTEM_SLOT] = { 057/255, 146/255, 181/255 },
    [AIR_TOTEM_SLOT] = { 132/255, 056/255, 231/255 }
}
 
local OnClick = function(self)
    DestroyTotem(self:GetID())
end
 
local UpdateTooltip = function(self)
    GameTooltip:SetTotem(self:GetID())
end
 
local OnEnter = function(self)
    if(not self:IsVisible()) then return end
 
    GameTooltip:SetOwner(self, 'ANCHOR_BOTTOMRIGHT')
    self:UpdateTooltip()
end
 
local OnLeave = function()
    GameTooltip:Hide()
end
 
local UpdateTotem = function(self, event, slot)
    local totems = self.Totems
 
    if(totems.PreUpdate) then totems:PreUpdate(slot) end
 
    local totem = totems[slot]
    local haveTotem, name, start, duration, icon = GetTotemInfo(slot)
    if(duration > 0) then
        if(totem.Icon) then
            totem.Icon:SetTexture(icon)
        end
 
        if(totem.Cooldown) then
            totem.Cooldown:SetCooldown(start, duration)
        end
 
        totem:Show()
    else
        totem:Hide()
    end
 
    if(totems.PostUpdate) then
        return totems:PostUpdate(slot, haveTotem, name, start, duration, icon)
    end
end
 
local Path = function(self, ...)
    return (self.Totems.Override or UpdateTotem) (self, ...)
end
 
local Update = function(self, event)
    for i = 1, MAX_TOTEMS do
        Path(self, event, i)
    end
end
 
local ForceUpdate = function(element)
    return Update(element.__owner, 'ForceUpdate')
end
 
local Enable = function(self)
    local totems = self.Totems
 
    if(totems) then
        totems.__owner = self
        totems.ForceUpdate = ForceUpdate
 
        for i = 1, MAX_TOTEMS do
            local totem = totems[i]
 
            totem:SetID(i)
 
            if(totem:HasScript'OnClick') then
                totem:SetScript('OnClick', OnClick)
            end
 
            if(totem:IsMouseEnabled()) then
                totem:SetScript('OnEnter', OnEnter)
                totem:SetScript('OnLeave', OnLeave)
 
                if(not totem.UpdateTooltip) then
                    totem.UpdateTooltip = UpdateTooltip
                end
            end
        end
 
        self:RegisterEvent('PLAYER_TOTEM_UPDATE', Path)
 
        TotemFrame.Show = TotemFrame.Hide
        TotemFrame:Hide()
 
        TotemFrame:UnregisterEvent"PLAYER_TOTEM_UPDATE"
        TotemFrame:UnregisterEvent"PLAYER_ENTERING_WORLD"
        TotemFrame:UnregisterEvent"UPDATE_SHAPESHIFT_FORM"
        TotemFrame:UnregisterEvent"PLAYER_TALENT_UPDATE"
 
        return true
    end
end
 
local Disable = function(self)
    if(self.Totems) then
        TotemFrame.Show = nil
        TotemFrame:Show()
 
        TotemFrame:RegisterEvent"PLAYER_TOTEM_UPDATE"
        TotemFrame:RegisterEvent"PLAYER_ENTERING_WORLD"
        TotemFrame:RegisterEvent"UPDATE_SHAPESHIFT_FORM"
        TotemFrame:RegisterEvent"PLAYER_TALENT_UPDATE"
 
        self:UnregisterEvent('PLAYER_TOTEM_UPDATE', Path)
    end
end
 
oUF:AddElement("Totems", Update, Enable, Disable)

diff: http://paste.pocoo.org/raw/368944/

tecu · 04-11-11, 05:29 PM

Hey. I think this is pretty solid. The only thing I have left to add is that the Blizzard TotemFrame (unlike the RuneFrame) is parented to the PlayerFrame so you can probably just leave it alone. I only just now looked this up, sorry.

If you need or want anything more from me, just post back here. Also, thanks a ton for all the feedback and comments; it has been very helpful to me.

haste · 04-12-11, 06:39 AM

I don't see why something hidden should be updating in the background. Anyway, I'll merge the code in when I get home from work.

I'd also like to thank you for your contribution

.

(was planning on writing some more and posting this before 9AM, now it's closing in to 3PM... wups!)