Gui tags

[RedRafe]

Proposal to extend gui handlers registration to tags as well.

Scope

This PR adds a new .tag field (internally __@level-RedMew__event_handler_tag__) to the utils.gui.lua module, that can be used to associate an handler to the value assigned to this key-value pair inside a LuaGuiElement::tags
This allows children of the same LuaGuiELement to share "name" and handlers, and keeps the same syntax currently used for name.

Relevant changes:

---@ utils.gui.lua

Gui.tag = '__@level-RedMew__'

local function handler_factory(event_id)
    local handlers

    local function on_event(event)
        ...
        local element = event.element
        local tag = element.tags[Gui.tag]
        local handler = handlers[element.name] or handlers[tag] -- searches for registered tags too
        if not handler then
            return
        end
        ...
        handler(event)
    end
end

Examples

Comparison and usage of current/proposed changes

1. Current behavior with LuaGuiElement::name

local button_name = Gui.uid_name()

-- @ element creation
local button = parent.add { name = button_name, type = 'button' }

-- @ handler registration
Gui.on_click(button_name, function(event) ... end)

2. Extension to LuaGuiElement::tags

In addition to name, tags are also filtered by the gui handler factory, allowing such syntax:

local button_tag = Gui.uid_name()

-- @ element creation
local button_1 = parent.add { type = 'button', tags = { [Gui.tag] = button_tag }
local button_2 = parent.add { type = 'button', tags = { [Gui.tag] = button_tag }
local button_3 = parent.add { type = 'button', tags = { [Gui.tag] = button_tag }

-- @ handler registration
Gui.on_click(button_tag, function(event) ... end)

Notes:

1. Usage of name is not deprecated, both name and tags can be used to register the handler
2. Registering 2 different handlers via name and tag to the same LuaGuiElement will result in only 1 being called, as name (more domain specific) will take precedence over tags
3. It is considered a bad practice to register multiple handlers via name+tags to the same LuaGuiElement

Edit:

1. Usage of name to register gui event handlers has been deprecated in favor if usage of tags.
2. Tags will handle all eventhandlers, while names are currently used to index parent-children relations (see top toolbar and player.gui.screen|center|left|top)

[grilledham]

I think this is a good idea and I'd accept as is, but I do have a preference to give tags precedence over names for events.

[RedRafe]

I see now what you mean with tags taking precedence over names. Then yes... We don't use elem names much in the codebase to access children I think (gui data replaces the tree names cascade to access children\parents via elem path), but if we indeed free the names from being used as handlers keys, and use always tags, then we could also remove the lookup in the event factory for elem.name and just check tag, then only use names for accesses like parent.flow.table.button.... not the best for complex hierarchies, but still useful for 1-2 deep structures instead of caching all the refs in storage. Would free some ram & keep gui handlers lightweight

[RedRafe]

Refactoring all the instances of name = ... to tags = { [Gui.tag] = ...}.

I'm gonna list a few critical files that have been ported but I would like to take another moment to review and test in game:

features/gui/poll.lua
features/gui/tasklist.lua
features/gui/player_list.lua (* will require grilledham's review)
features/gui/toast.lua

[RedRafe]

We don't use elem names much in the codebase to access children

Actually, after refactoring all names to tags, I realized that player.gui.screen|center|top|left do use names to access the unique frame built by each module.

I've left those (+ a few of other exceptions of quick-access), and those elements will have redundant name+tags with same key (or even different, if wanted), because name is used as unique ID to register the main frame into player's gui, while tags is used by the event handler to map eventual handlers to that element.

As a rule of thumb, left elements do not have associated handlers, while top|center|screen elements usually have a custom_close (for window frames) or gui_click (for toolbar buttons) associated to them

[grilledham]

I think this case can get away with not needing the name, but I think all other cases of Gui.add_top_element need it, so maybe should keep just in case.

[RedRafe]

Actually, this name is needed cuz Gui.add_top_element indexes children by name (same as cutscene controller which I reverted).

I may be not too happy with top & left having mandatory argument name now that it has been kinda deprecated but oh well... Maybe I'll figure something out while refactoring around, dont have a nice solution yet

[grilledham]

The Gui.add_top_element in poll.lua and evolution_progress.lua are missing a tags = { [Gui.tag] = main_button_name }

[grilledham]

We don't use elem names much in the codebase to access children

Actually, after refactoring all names to tags, I realized that player.gui.screen|center|top|left do use names to access the unique frame built by each module.

I've left those (+ a few of other exceptions of quick-access), and those elements will have redundant name+tags with same key (or even different, if wanted), because name is used as unique ID to register the main frame into player's gui, while tags is used by the event handler to map eventual handlers to that element.

As a rule of thumb, left elements do not have associated handlers, while top|center|screen elements usually have a custom_close (for window frames) or gui_click (for toolbar buttons) associated to them

Thanks for doing this. I know this is a lot of tedious work. I think having elements with the same name and tag value is good in cases where they are both used.

[RedRafe]

The Gui.add_top_element in poll.lua and evolution_progress.lua are missing a tags = { [Gui.tag] = main_button_name }

I did forgot to add the tag for poll, but evolution doesnt need a tag since there's no handler/window associated with it being clicked. (double checking w/ you)

[RedRafe]

I'm not 100% happy with the current state of this draft, so I will try to refactor Gui util once more to address these issues:

1. I'd prefer Gui.event_tag to be something internally used by the module and not necessarily exported for other modules to use (if I can achieve so). This should offload some of the logic from the element definitions around the codebase back to the core Gui module instead

2. I don't like the overlap of name and tag, especially when they share the same uid. The uid overlap is technically allowed because name and tag serve two different purposes, but I think it's conceptually misleading and harder to debug/review/(understand for new contributors) when the same uid is used differently depending on who calls it.

Possible solutions that I'm broadly exploring:

• Invert the uid assignment: give handlers an uid instead, and have LuaGuiElems reference the uid of the handler responsible of their logic. Only uid assigned to LuaGuiElems will be for their name, if necessary for root access, and be totally unrelated to handlers'.

• Empowering Gui module: implement more helper methods on Gui to move logic from other modules back to it. Take responsibility of namespacing, add/remove LuaGuiElem, handle stored data, support classes/metatables, with the possibility in the future to expand Gui with a style lib / components lib to unify styles and be easily reused by other modules without having to reimplement them.

• Possible reworks of the toolbar and all Gui.add_Xmethods to deprecated name or have support for tags as well

[RedRafe]

Regarding this draft, would you (@grilledham ) consider it an improvement if:

1. We get rid of Gui.destroy/Gui.clear and have Gui module track all the data itself so it's 100% guaranteed there's no memory leak due to human-errors in creating/destroying data/LuaGuiElement. Could also simplify some gui tests, but keeping the gui data explorer is nice even if I've never used it so would leave as is

2. We split Gui into a "module/library" thing, and just call require 'utils.gui' to return the assembled lib of the main submodules (data, events, styles)

[grilledham]

Regarding this draft, would you (@grilledham ) consider it an improvement if:

1. We get rid of Gui.destroy/Gui.clear and have Gui module track all the data itself so it's 100% guaranteed there's no memory leak due to human-errors in creating/destroying data/LuaGuiElement. Could also simplify some gui tests, but keeping the gui data explorer is nice even if I've never used it so would leave as is
2. We split Gui into a "module/library" thing, and just call require 'utils.gui' to return the assembled lib of the main submodules (data, events, styles)

@RedRafe

1. Yes, if you can come up with a good way to do it, I think that would be an improvement.
2. As it Gui would just expose the other file's functionality as a convenience thing? Maybe I'm not understanding what you are suggesting here, but I don't see that as adding much.