wibox.layout.manual:insert() implementation

[Aire-One]

Following my issue #2556, here is my implementation try :)

Some notes:

• From my understanding, manual_layout._private.widgets and manual_layout._private.pos are arrays so elements can be "magically" shifted/organized ;
• I used table.insert because it shifts up the elements, so it will keep the widgets ordered (https://www.lua.org/manual/5.3/manual.html#pdf-table.insert) ;
• I wrote the code under the documentation-comment because I think it's easier to read the code / review it. All functions from this file are not written with this rule... Tell me if I need to change that ;
• I don't know how to do that : New elements will not be accessible as children by their id property ;

[codecov]

Codecov Report

Merging #2557 into master will decrease coverage by 0.03%.
The diff coverage is 20%.

@@            Coverage Diff             @@
##           master    #2557      +/-   ##
==========================================
- Coverage   84.58%   84.54%   -0.04%     
==========================================
  Files         497      497              
  Lines       33720    33641      -79     
==========================================
- Hits        28521    28442      -79     
  Misses       5199     5199

Flag	Coverage Δ
#c_code	72.76% <ø> (-0.27%)	⬇️
#functionaltests	72.8% <20%> (-0.02%)	⬇️
#lua53	87.65% <20%> (-0.02%)	⬇️
#samples	74.65% <20%> (-0.02%)	⬇️
#unittests	58.17% <ø> (ø)	⬆️

Impacted Files	Coverage Δ
lib/wibox/layout/manual.lua	93.75% <20%> (-4.92%)	⬇️
common/buffer.h	4.76% <0%> (-71.71%)	⬇️
luaa.h	47.82% <0%> (-19.65%)	⬇️
common/luaobject.h	90% <0%> (-10%)	⬇️
common/signal.h	96.15% <0%> (-3.85%)	⬇️
common/util.h	87.5% <0%> (-2.16%)	⬇️
objects/button.h	100% <0%> (ø)	⬆️
globalconf.h	100% <0%> (ø)	⬆️
common/xembed.h	0% <0%> (ø)	⬆️
objects/client.c	79.29% <0%> (+0.15%)	⬆️
... and 3 more

[Elv13]

Another way would have been manual_layout.insert = manual_layout.add, but both are ok. Thanks for fixing that.

[actionless]

@Elv13 i see the implementation of those 2 methods is slightly different

so i have to clarify, what is the difference in expected result of both?

[Elv13]

A right nevermind, that would be insert = function(_, ...) return add(...) end. The current version is better. The logic was that manual order is irrelevant.

[actionless]

sorry, i still didn't got it clearly

[Elv13]

sorry, i still didn't got it clearly [...] what is the difference in expected result of both?

In the end there is little difference. insert was accidentally documented and is implemented by the other layouts so it's a pseudo expected method to exist. The order exists in the API because the .children property is part of the base widget API and is ordered. Therefor :insert() technically make sense in all layouts, regardless of how they get displayed. However since the manual layout widget position is provided by a point or a callback, then it has no visual impact.

[actionless]

i still can't understand if it makes sense to just use the same implementation instead of copying the code or those differences are intended?

[Elv13]

@Aire-One version is better as it produces the correct .children. My hack would have looked the same, but it would not have produced a correct .children.

[Aire-One]

Sorry, I'm a little late in the discussion, I was studying for an exam...

I'm glad you approved my code.
Thanks for yours time and reviews :)

However I have to disagree with these statements:

The logic was that manual order is irrelevant.

However since the manual layout widget position is provided by a point or a callback, then it has no visual impact.

While the order does theoretically not have an impact, I found out it has. It changes the drawing order of the widgets. When some children of a manual layout overlaps each other, the order will change the one that is drawn on top of the others.

Here is a short test code I wrote to run within xephyr to test widget overlapping:

require('rc')

local awful = require('awful')
local beautiful = require('beautiful')
local gears = require('gears')
local wibox = require('wibox')

local w_background = wibox.widget {
    widget = wibox.widget.imagebox,
    resize = true,
    image = beautiful.awesome_icon,
    forced_height = 16,
    forced_width = 16,
    point = awful.placement.centered
}

local w_icon = wibox.widget {
    layout = wibox.container.background,
    wibox.widget.base.empty_widget(),
    bg = '#eeeeee',na
    shape = gears.shape.rounded_bar,
    forced_height = 16,
    point = awful.placement.maximize_horizontally + awful.placement.bottom
}

local w1 = wibox.widget {
    layout  = wibox.layout.manual,
    w_background,
    w_icon
}

local w2 = wibox.widget {
    layout  = wibox.layout.manual,
    w_icon,
    w_background
}

awful.screen.w = wibox {
    type = 'dock', ontop = true,
    visible = true, opacity = 1,
    width = 200, height = 30
}
awful.screen.w:setup {
    layout = wibox.layout.flex.horizontal,
    w1,
    w2
}

And here is a preview of the result:

So, order do have an impact on the visual aspect of the layout. That's why the implementation of insert can't be the same than add.

---

@Aire-One version is better as it produces the correct .children. My hack would have looked the same, but it would not have produced a correct .children.

But it still misses something because you can't access inserted children by their ids.

Here is an example of what I mean:

local my_manual_layout = wibox.widget {
    layout  = wibox.layout.manual,
    { id = 'w_role', ... }
}
my_manual_layout:insert(1, wibox.widget { id = 'insert_role', ...})
my_manual_layout.w_role -- this works
my_manual_layout.new_role -- this fails

I suspect it have something to do with #2181 but I'm not sure on how it actually works...

[Elv13]

I suspect it have something to do with #2181

It has indeed

but I'm not sure on how it actually works...

It does not ;).

The original "scope" of :get_children_by_id() was limited to DOM-like queries on the widget trees created using :setup(). At that point in time, the imperative and declarative styles were still very different and not quite compatible with each other. Since then many pull requests have better integrated them using concepts such as delegate template. #2508 goes further by allowing both widget declaration and definition in every containers and layouts. Some ongoing pull requests move all widgets constructor to use named parameters. The finality is to re-unify both style. For this, :get_children_by_id() has to be moved to the hierarchy instead of being implemented in the declarative tree parser. Once all 3 "projects" are finished, then there will be one widget system with a very flexible and permissive syntax. But right now, 4.5 years since I started upstreaming my config widget system, this is still work in progress, but large progress has been made.

[Elv13]

Thanks

(the force push was to set a commit message instead of "try 1")