Make the wibox get_children_by_id reliable

[Elv13]

Output of awesome --version:

4.0 to git-master.

Right now it doesn't cross the wibox.widget {} sections that can be pasted from the documentation inside of a wibox :setup{} section. So copy-pasted documentation examples end up confusingly "working" visually, but are not reachable. This was reported on reddit and is indeed a prime example of confusing anti-features.

Opening this issue so it is not forgotten.

[psychon]

So... basically, get_children_by_id and the concept of an id have to be added to wibox.widget.base and the implementation would then use for _, w in self:get_children() do if w.id == id then table.insert(result, w) end end?

[actionless]

does that also implies what we need to add some recursive check to ensure what id is unique?

or if id is the same list of widgets should be returned?

[Elv13]

id are not unique. The API is like JavaScript. It returns a table with all the result for that id.

The problem right now is that if you create widgets with wibox.widget {...} you end up with a get_children_by_id method there and they are not in sync with the wibar/wibox top level get_children_by_id.

As @psychon sugested solution demonstrate, the easy way to do this is O(N) during lookup instead of O(1) when cached. That sucks. A less terrible way is to do it in wibox.widget.base by checking if there is an existing id table when building the tree. But that has the drawback of keeping track when widgets are removed or added to all subtrees. In that case the widgets created using wibox.widget{...} can be shared across multiple wiboxes so we can't just merge all subtrees.

So both solutions have big drawbacks and it's why it wasn't implemented yet. In theory, the lookup happens more often than building new trees, so it should be prioritized in term of performance (slower tree building for faster lookups instead of the other way around). This is why I think the solution should involve being able to track changes to the subtrees. That also make the solution more complicated. Maybe moving that to the hierarchy might help.

[actionless]

and does having ids worth it?

i think using variable or table to store references when it's needed is quite viable solution. especially if infrastructure for ids could not only encomplicate the code but also cause some performance affect

[Elv13]

Honestly, I don't know. I made it this way so people coming from JavaScript don't have to learn anything new and make the wibox "DOM-like". This issue was reported twice and I never seen the alternate wibox.parent.parent.widget style syntax used anywhere. I guess few people who care about accessing widgets do prefer the get_children_by_id() API.

One way or another, it uses IDs, so something has to be done. As said by the previous commit, maybe it's a good idea to extend the hierarchy itself to manage that list instead of inventing a new system just for IDs.

[psychon]

the easy way to do this is O(N) during lookup instead of O(1) when cached. That sucks.

Sigh. I'd say "premature optimisation", but no one would listen to me anyway...

So, how about amortised O(1) performance? You have a first commit that does the "ugly" O(N) lookup. If you want, you can measure the performance (relative to e.g. redrawing a wibox, or something). At this point I'd likely conclude "fast enough" and be done, but since you want this to be faster: Next, you add a cache to get_children_by_id() that caches the result of the O(N) lookup so that the next lookup becomes O(1) (assuming you want to call Lua table indexing O(1)). This cache would be managed similar to widget_dependencies in wibox.widget.base, i.e. it is invalidated by clear_caches in that function.

I'm not quite sure what you want to do in wibox.hierarchy, but the above sounds less complex to me.

P.S.: Why does widget_dependencies have __mode = "kv"? The v in there seems to be terribly wrong to me. Oh, and it was even me who did that in commit bd47edb...

Edit: Actually, you want to wire up get_children_by_id to get_cache in there. That handles the invalidation for you automatically... kind of.

[psychon]

Since this is listed as a release-blocker and progress on it was... well, slow, here is an untested implementation of get_children_by_id for drawables that can easily be forwarded to by wibox, if needed. Since @Elv13 wrote that he wants this in the hierarchy, I guess returning stale data when a relayout is pending is fine. But I guess that's (part of) the price of having a cache (actually, by invalidating the cache when _need_relayout is set instead of unset, even that problem could be overcome relatively easily; I guess the best course of action here is to move setting _need_relayout into a helper function which then also invalidates the cache)

diff --git a/lib/wibox/drawable.lua b/lib/wibox/drawable.lua
index cf9a129e..786bc0d0 100644
--- a/lib/wibox/drawable.lua
+++ b/lib/wibox/drawable.lua
@@ -27,6 +27,16 @@ local visible_drawables = {}
 
 local systray_widget
 
+local function _do_get_children_by_id(self, id)
+    local result = {}
+    for _, w in pairs(self:get_children()) do
+        if w.id == id then
+            table.insert(result, w)
+        end
+    end
+    return result
+end
+
 -- Get the widget context. This should always return the same table (if
 -- possible), so that our draw and fit caches can work efficiently.
 local function get_widget_context(self)
@@ -104,6 +114,9 @@ local function do_redraw(self)
                 x = 0, y = 0, width = width, height = height
             })
         end
+
+        -- Kill the cache
+        self._get_children_by_id = require("gears.cache")(_do_get_children_by_id)
     end
 
     -- Clip to the dirty area
@@ -309,6 +322,10 @@ function drawable:_inform_visible(visible)
     end
 end
 
+function drawable:get_children_by_id(id)
+    return self._get_children_by_id(self, id)
+end
+
 local function emit_difference(name, list, skip)
     local function in_table(table, val)
         for _, v in pairs(table) do
@@ -383,6 +400,8 @@ function drawable.new(d, widget_context_skeleton, drawable_name)
         end
     end
 
+    ret._get_children_by_id = function() return {} end
+
     -- Only redraw a drawable once, even when we get told to do so multiple times.
     ret._redraw_pending = false
     ret._do_redraw = function()

[psychon]

And just because, here is the beginning of a hierarchy-based approach:

diff --git a/lib/wibox/hierarchy.lua b/lib/wibox/hierarchy.lua
index e0da295c..e1d1b2e2 100644
--- a/lib/wibox/hierarchy.lua
+++ b/lib/wibox/hierarchy.lua
@@ -48,6 +48,7 @@ local function hierarchy_new(redraw_callback, layout_callback, callback_arg)
         },
         _parent = nil,
         _children = {},
+        _children_by_id = {},
         _widget_counts = {},
     }
 
@@ -190,6 +191,24 @@ function hierarchy_update(self, context, widget, width, height, region, matrix_t
             x = new_x, y = new_y, width = new_width, height = new_height
         })
     end
+
+    -- Update children by id
+    self._children_by_id = {}
+    local function merge(children_by_id)
+        for id, other_list in pairs(children_by_id) do
+            local own_list = self._children_by_id[id] or {}
+            self._children_by_id[id] = own_list
+            for _, w in ipairs(other_list) do
+                table.insert(own_list, w)
+            end
+        end
+    end
+    if self._widget.id then
+        merge({ [self._widget.id] = { self } })
+    end
+    for _, h in ipairs(self._children) do
+        merge(h._children_by_id)
+    end
 end
 
 --- Create a new widget hierarchy that has no parent.
@@ -282,6 +301,13 @@ function hierarchy:get_children()
     return self._children
 end
 
+--- TODO
+-- @arg todo
+-- @return todo
+function hierarchy:get_children_recursively_by_id(id)
+    return self._children_by_id[id] or {}
+end
+
 --- Count how often this widget is visible inside this hierarchy. This function
 -- only works with widgets registered via `count_widget`.
 -- @param widget The widget that should be counted

[actionless]

can this be closed now or after you'll move it to hierarchy?

[Elv13]

can this be closed now or after you'll move it to hierarchy?

It has to be ported to the hierarchy before we can really unify the wibox constructors (as we both discussed and agreed upon elsewhere). Example:

works:

local l = wibox.widget {
    {
        id = "foobar",
        text = "foobar",
        widget =  wibox.widget.textbox,
    },
    layout = wibox.layout.fixed.horizontal
}

broken:

local l = wibox.layout.fixed.horizontal {
    wibox.widget.textbox {
        id = "foobar",
        text = "foobar",
    }
}

Of course, in this example, there is a whole bunch of issue. First of all, the fixed need to support {} instead of (...). Then the textbox constructor need to support args. Beside this, the id wont work correctly. As long as we rely on all widgets doing things "right", this will keep regressing. Moving this to the hierarchy would fix this.