Gui: putting code where my mouth was (refactor Gui event registry)

[gmt]

From: "Gregory M. Turner" <gmt@be-evil.net>
Date: Thu, 15 Feb 2018 02:31:27 -0800
Subject: [PATCH 2/2] Gui: putting some code where my mouth was

Since I shot my mouth off on the forums about what a
mess stdlib/event/gui was, I felt I pretty much had
to cook this up or else look like a whiner.

Lots of this is untested but it is confirmed not to be
100% completely broken in every aspect.

In theory it's mostly backward-compatible.  It

* removes global Event dependency from event/gui module
* stops abusing Event._registry in Gui
* relys entirely on Event features for all communication
  with factorio

Wish I knew how to run the test suite... anyhow it's
a jumping off point.  I wouldn't just merge this without
much more rigorous testing than I've given it.
---
 stdlib/event/gui.lua             | 171 ++++++++++++++++++++++++++++-----------
 stdlib/utils/scripts/console.lua |   2 +-
 2 files changed, 125 insertions(+), 48 deletions(-)

diff --git a/stdlib/event/gui.lua b/stdlib/event/gui.lua
index 99493c9..1d8c534 100644
--- a/stdlib/event/gui.lua
+++ b/stdlib/event/gui.lua
@@ -2,42 +2,14 @@
 -- @module Gui
 -- @usage local Gui = require('stdlib/event/gui')

-require('stdlib/event/event')
+local Event = require('stdlib/event/event')

 Gui = {_module_name = "Gui"} --luacheck: allow defined top
 setmetatable(Gui, {__index = require('stdlib/core')})

 local fail_if_missing = Gui.fail_if_missing

---- Registers a function for a given event and matching gui element pattern.
--- @tparam defines.events event_id valid values are `defines.events.on_gui_*` from @{defines.events}
--- @tparam string gui_element_pattern the name or string regular expression to match the gui element
--- @tparam function handler the function to call when the event is triggered
--- @return (<span class="types">@{Gui}</span>)
-function Gui.register(event_id, gui_element_pattern, handler)
-    fail_if_missing(event_id, "missing event name argument")
-    fail_if_missing(gui_element_pattern, "missing gui name or pattern argument")
-
-    if type(gui_element_pattern) ~= "string" then
-        error("gui_element_pattern argument must be a string")
-    end
-
-    if handler == nil then
-        return Gui.remove(event_id, gui_element_pattern)
-    end
-
-    if not Event._registry[event_id] then
-        Event._registry[event_id] = {}
-        script.on_event(event_id, Gui.dispatch)
-    end
-
-    if Event._registry[event_id][gui_element_pattern] then
-        log("Same handler already registered for Gui event "..event_id..".")
-    end
-    Event._registry[event_id][gui_element_pattern] = handler
-
-    return Gui
-end
+Gui._registry = {}

 --- Calls the registered handlers.
 -- @tparam {defines.events,...} event an array of @{defines.events} as raised by @{LuaBootstrap.raise_event|script.raise_event}
@@ -46,17 +18,25 @@ function Gui.dispatch(event)

     if event.element and event.element.valid then
         event.tick = event.tick or game.tick
-        for gui_element_pattern, handler in pairs(Event._registry[event.name]) do
+        for gui_element_pattern, handlers in pairs(Gui._registry[event.name]) do
             if event.element and event.element.valid then
                 local match_str = event.element.name:match(gui_element_pattern)
                 if match_str then
                     event.match = match_str
                     event.state = event.name == defines.events.on_gui_checked_state_changed and event.element.state or nil
                     event.text = event.name == defines.events.on_gui_text_changed and event.element.text or nil
-                    setmetatable(event, { __index = { _handler = handler } })
-                    local success, err = pcall(handler, event)
-                    if not success then
-                        game.print(err)
+                    local metahandler = {}
+                    setmetatable(event, {__index = metahandler})
+                    for _, handler in ipairs(handlers) do
+                        metahandler._handler = handler
+                        local success, err = pcall(handler, event)
+                        if not success then
+                            if game and game.print then
+                                game.print(err)
+                            else
+                                print(err)
+                            end
+                        end
                     end
                 end
             end
@@ -64,25 +44,120 @@ function Gui.dispatch(event)
     end
 end

---- Removes the handler with matching gui element pattern from the event.
--- @tparam defines.events event_id valid values are `defines.events.on_gui_*` from @{defines.events}
--- @tparam string gui_element_pattern the name or string regular expression for a handler to remove
+--- Registers a function for a given event and matching gui element pattern.
+-- @tparam defines.events event_ids valid values are `defines.events.on_gui_*` from @{defines.events},
+-- or a list of them.
+-- @tparam string gui_element_pattern the name or string regular expression to match the gui element
+-- @tparam function handler the function to call when the event is triggered
 -- @return (<span class="types">@{Gui}</span>)
-function Gui.remove(event_id, gui_element_pattern)
-    fail_if_missing(event_id, "missing event argument")
+function Gui.register(event_ids, gui_element_pattern, handler)
+    fail_if_missing(event_ids, "missing event name argument")
+    fail_if_missing(gui_element_pattern, "missing gui name or pattern argument")
+
+    if type(event_ids) == "table" then
+        for _, v in pairs(event_ids) do
+            Gui.register(v, gui_element_pattern, handler)
+        end
+        return Gui
+    end

     if type(gui_element_pattern) ~= "string" then
         error("gui_element_pattern argument must be a string")
     end

-    if Event._registry[event_id] then
-        if Event._registry[event_id][gui_element_pattern] then
-            Event._registry[event_id][gui_element_pattern] = nil
+    if handler == nil then
+        return Gui.remove(event_ids, gui_element_pattern)
+    end
+
+    if not Gui._registry[event_ids] then
+        Gui._registry[event_ids] = {}
+        Event.register(event_ids, Gui.dispatch)
+    end
+    if not Gui._registry[event_ids][gui_element_pattern] then
+        Gui._registry[event_ids][gui_element_pattern] = {}
+    else
+        local _, reg_index = table.find(Gui._registry[event_ids][gui_element_pattern], function(v) return v == handler end)
+        local warning = "Gui: handler re-registered for event " .. event_ids .. "[" ..  gui_element_pattern .. "]; "
+        if reg_index and reg_index ~= #Gui._registry[event_ids][gui_element_pattern] then
+            table.remove(Gui._registry[event_ids][gui_element_pattern], reg_index)
+            log(warnmsg ..  "reordering to bottom.")
+            table.insert(Gui._registry[event_ids][gui_element_pattern], handler)
+            Event.register(event_ids, Gui.dispatch)
+            return Gui
+        elseif reg_index then
+            log(warnmsg .. "re-registering event.")
+            Event.register(evnet_id, Gui.dispatch)
+            return Gui
+        end
+    end
+
+    table.insert(Gui._registry[event_ids][gui_element_pattern], handler)
+    return Gui
+end
+
+--- Removes the handler with matching gui element pattern from the event.
+-- @tparam defines.events event_ids valid values are `defines.events.on_gui_*` from @{defines.events}, or
+-- a list of them
+-- @tparam string gui_element_pattern (optional) name or string regular expression for a handler to remove
+-- @tparam function handler (optional) handler to deregister from specified Gui events.
+-- @return (<span class="types">@{Gui}</span>)
+function Gui.remove(event_ids, gui_element_pattern, handler)
+    fail_if_missing(event_ids, "missing event argument")
+
+    if type(event_ids) == "table" then
+        for _, v in pairs(event_ids) do
+            Gui.remove(v, gui_element_pattern, handler)
+        end
+        return Gui
+    end
+
+    if gui_element_pattern and type(gui_element_pattern) ~= "string" then
+        error("gui_element_pattern argument must be a string")
+    end
+
+    if not Gui._registry[event_ids] then
+        log("Request to remove non-registered gui event handler for " .. event_ids .. " ignored.")
+        return Gui
+    end
+
+    if gui_element_pattern and not Gui._registry[event_ids][gui_element_pattern] then
+        log("Request to remove non-registered <event, pattern> tuple: <" .. event_ids ..
+            ", " .. gui_element_pattern .. "> ignored.")
+        return Gui
+    end
+
+    -- recurse, if needed, to replace the optional arguments
+    -- with concrete values from the registry.
+    if not gui_element_pattern then
+        for pattern, _ in pairs(Gui._registry[event_ids]) do
+            Gui.remove(event_ids, pattern, handler)
         end
-        if table.size(Event._registry[event_id]) == 0 then
-            Event._registry[event_id] = nil
-            script.on_event(event_id, nil)
+        return Gui
+    elseif not handler then
+        local reg = Gui._registry[event_ids][gui_element_pattern]
+        for i = #reg, 1, -1 do
+            Gui.remove(event_ids, gui_element_pattern, reg[i])
         end
+        return Gui
+    end
+
+    local reg = Gui._registry[event_ids][gui_element_pattern]
+    for i = #reg, 1, -1 do
+        if reg[i] == handler then
+            table.remove(reg, i)
+        end
+    end
+    if #reg == 0 then
+        Gui._registry[event_ids][gui_element_pattern] = nil
+    end
+    local something_in_there = false
+    for _ in pairs(Gui._registry[event_ids]) do
+        something_in_there = true
+        break
+    end
+    if not something_in_there then
+        Gui._registry[event_ids] = nil
+        Event.remove(event_ids, Gui.dispatch)
     end
     return Gui
 end
@@ -135,6 +210,8 @@ function Gui.on_value_changed(gui_element_pattern, handler)
     return Gui.register(defines.events.on_gui_value_changed, gui_element_pattern, handler)
 end

-Event.Gui = Gui
+if not Event.Gui then
+   Event.Gui = Gui
+end

 return Gui
diff --git a/stdlib/utils/scripts/console.lua b/stdlib/utils/scripts/console.lua
index 7c0b4f4..47fbe5b 100644
--- a/stdlib/utils/scripts/console.lua
+++ b/stdlib/utils/scripts/console.lua
@@ -9,7 +9,7 @@
 -- /c remote.call("my_interface", "show", game.player)
 -- --In the window that appears you can run lua code directly on your mod, including globals.

-require('stdlib/event/gui')
+local Gui = require('stdlib/event/gui')

 -- munge hyphens as they otherwise need escaping in lua's regexnih pattern language
 local prefix = string.gsub(script.mod_name, '%-', '_')
-- 
2.16.1

[gmt]

This doesn't quite local the Gui thing because I suspect that would break lots of stuff. But, you know "decoupling," or some buzzword! Speaking seriously, to me something like the above seems more maintainable/comprehensible than what's in data-library now.

[gmt]

Almost forgot, it needs this first:

From: "Gregory M. Turner" <gmt@be-evil.net>
Date: Wed, 14 Feb 2018 22:56:53 -0800
Subject: [PATCH 1/2] gui: make lingering Event.Gui reference direct

Signed-off-by: Gregory M. Turner <gmt@be-evil.net>
---
 stdlib/event/gui.lua | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/stdlib/event/gui.lua b/stdlib/event/gui.lua
index 037cdd6..99493c9 100644
--- a/stdlib/event/gui.lua
+++ b/stdlib/event/gui.lua
@@ -28,7 +28,7 @@ function Gui.register(event_id, gui_element_pattern, handler)

     if not Event._registry[event_id] then
         Event._registry[event_id] = {}
-        script.on_event(event_id, Event.Gui.dispatch)
+        script.on_event(event_id, Gui.dispatch)
     end

     if Event._registry[event_id][gui_element_pattern] then
-- 
2.16.1

[Nexela]

Can you post this as a PR, Will make it easier for to check it out and peek at it.

[gmt]

Sure, that is Afforess/Factorio-Stdlib/pull/113. Feels a bit dirty though ... wip PRs are not so OCD friendly...

Anyhow your request inspired me to make a branch for it, Nexela, so even if that's closed you can use gmt/Factorio-Stdlib/tree/data-library_gui-refactor-wip

[Nexela]

Closing this issue, to keep all comments on #113