Add test spec for wibox.layout.stack

[dixslyf]

So, I'm back again after looking into awesome's unit tests for 2 days, and I've drafted a test spec for wibox.layout.stack. I'm not too confident since I'm inexperienced in unit testing — I don't have a clear idea of what should or should not be tested — so I imagine that some changes will have to be made.

Since wibox.layout.stack is based on wibox.layout.fixed, I basically copied wibox.layout.fixed's test spec and made changes and additions. The 1st commit in this pull request just performs that copy so that it's easier to see what the subsequent commits do. The 2nd modifies the tests so that they pass. The rest are additional tests.

Here's a summary of the additional tests added — ~~a few of them will fail for now since this pull request doesn't include the fixes from #3187~~ #3187 has been merged; raise top test still fails, see its bullet point:

• with widgets with enough space
  • layout with offset: tests layout when vertical_offset and horizontal_offset are applied
  • layout with offset and spacing: same as previous but with spacing set as well. I felt that this was needed as the spacing affects the size of the child widgets.
  • layout with top_only: tests layout when top_only is set. Only the top child widget should be placed.
  • layout raise ~~(fails; needs #3187)~~: tests layout after raising the 2nd widget. Ensures that the 2nd widget actually gets raised.
• without enough width/height
  • layout with offset: same as before, but not too sure about this one. wibox.widget.base.place_widget_via_matrix() throws an A widget's width cannot be negative: -1 assertion error in this test case, because wibox.layout.stack minuses out the offsets when calculating its child widgets' widths & heights in its :layout() method. There is no check to clamp the width & height to 0 if they go negative. Should that check be added to wibox.layout.stack:layout()? For now, this test just asserts that an error will occur.
  • I did not include a layout with offset and spacing test here. Wasn't sure if needed.
• emitting signals: added ones are all for the wibox.layout.stack:raise() methods — grouped together in a describe block
  • raise ~~(fails; needs #3187)~~: tests if widget::layout_changed is emitted when using :raise(). Does not account for raising index 1 (the top child widget) — see the next bullet point.
  • raise top (fails): asserts that no widget::layout_changed signal is emitted if we :raise() the top widget I think there is a bug in wibox.layout.stack:raise() not covered by #3187 — there is no check to see if the passed index is 1. It should not do anything when the index is 1 as that is already the top child widget; raising the top to the top doesn't change the layout, so widget::layout_changed should not be emitted. wibox.layout.stack:raise_widget() doesn't have this problem as it performs this check (before it calls :raise()).
  • raise_widget ~~(fails; needs #3187)~~: same as raise but uses :raise_widget() instead.
  • raise_widget top: same as raise top but uses :raise_widget() instead of :raise(). Passes as a check for index 1 is performed in wibox.layout.stack:raise_widget()

BTW, shouldn't the description titles for the without enough height and without enough width describe blocks be swapped in all of the wibox.layout.* tests? I might be misunderstanding it, but it looks like they're testing for not having enough width in without enough height, and not having enough height in without enough width.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (ed6cdf8) 88.21% compared to head (4bfc0ca) 79.43%. Report is 751 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3190      +/-   ##
==========================================
- Coverage   88.21%   79.43%   -8.79%     
==========================================
  Files         662      471     -191     
  Lines       44953    25881   -19072     
==========================================
- Hits        39657    20558   -19099     
- Misses       5296     5323      +27     

Flag	Coverage Δ
gcov	?
luacov	79.43% <ø> (-11.64%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

see 285 files with indirect coverage changes

[dixslyf]

Forgot to mention — should test cases for negative offsets and negative spacing be added? Both are allowed in wibox.layout.stack, but I'm not sure if the behaviour they show are desirable. Negative offsets can cause the child widgets to have negative positional x or y values, so they get cut off by their parent. Negative spacing causes them to be bigger and can also result in negative positional x or y values.

[psychon]

Negative offsets can cause the child widgets to have negative positional x or y values, so they get cut off by their parent.

What do you mean exactly? Children outside of their parent should work fine. @Elv13 more or less explicitly requested that back when the widget code was written. However, I'm not sure if I ever saw anyone using that feature...

[dixslyf]

Negative offsets can cause the child widgets to have negative positional x or y values, so they get cut off by their parent.

What do you mean exactly? Children outside of their parent should work fine. @Elv13 more or less explicitly requested that back when the widget code was written. However, I'm not sure if I ever saw anyone using that feature...

A and B are what the current implementation does: A is with positive offsets; B is with negative offsets. C is what my intuition had expected negative offsets to be like. Now that I think about it, B makes sense. Children outside of their parent do work and that's fine. However, if I wanted to create C using a stack layout, I can't think of an easy way to do so.

[Elv13]

However, I'm not sure if I ever saw anyone using that feature...

My config uses it ;)

You are probably right about C. It is the most intuitive outcome and the most useful. B make sense in itself, but is probably not what most people would expect and find useful.

[dixslyf]

Perhaps an additional boolean property could be added to wibox.layout.stack that if set to true, turns A into C (with the offsets still being positive)? The :layout() method would have to be modified. Not sure what such a property would be called.

We could also add additional offset properties that apply to the layers as a whole, rather than to each individual layer. This one feels a bit messy though.

[psychon]

Ah, thanks for those images. They help a lot.

Do you expect the widgets in B to be smaller / scaled down / get assigned a smaller width & height? Or would you expect them to be clipped and the part outside of the current widget to just be invisible? I am asking because the clipping variant is not currently possible with the widget system. You can clip away the drawing, but mouse clicks would still be forwarded to the widget.

(Also: Sorry, but I still haven't really looked at the commits yet)

[dixslyf]

I am asking because the clipping variant is not currently possible with the widget system.

I'm a bit confused here, maybe I misunderstood your question. B is what wibox.layout.stack currently does for negative offsets, which is the clipping variant isn't it? What do you mean it's not currently possible with the widget system?

(Also: Sorry, but I still haven't really looked at the commits yet)

No worries! We all have things to do! Edit: In fact, I think we should deal with #3192 first since that could affect the commits here.

[psychon]

I'm a bit confused here, maybe I misunderstood your question. B is what wibox.layout.stack currently does for negative offsets, which is the clipping variant isn't it?

local w = wibox {
    screen = screen[1],
    x = 10,
    y = 10,
    width = 100,
    height = 100,
}
local function textbox(text)
    local w = wibox.widget.textbox(text)
    w:set_valign("top")
    w:set_align("left")
    return w
end
local stack = wibox.layout.stack()
stack.horizontal_offset = -10
stack.vertical_offset = -10
stack:add(textbox("first"))
stack:add(textbox("second"))
stack:add(textbox("third"))
w.widget = wibox.container.margin(wibox.container.background(stack, "#f00"), 30, 10, 30, 10)
w.visible = true

The red area shows the space assigned to the stack widget. The 2nd and 3rd textboxes are outside of that. Thus, there is no clipping. I think it is more like image A, but moves towards top/left a bit, if you understand what I mean.

Does this make it clearer what I mean?

[dixslyf]

Ah, that makes it much clearer.

Having them clipped does make sense to me, but I see that them being visible makes sense as well. My intuition expected not any of those two variants, but rather something like C, so with third being positioned at the top left of the stack layout widget (top left of red bg).

I guess the main point of what I expected was that setting negative offsets (of the same magnitude) should just reverse the positions of the layers (the x and y coordinates, not the layer ordering). So, A would be (and is) with positive offsets, and C would be with negative offsets of the same magnitude of A's. Notice how the green layer is still the top layer, but the layers' positions are reversed. So, no "going out of the parent" involved.

BTW, I won't be able to work on this for a while (~2 weeks, maybe more). I just got enlisted into the military and don't have access to my computer. I am looking into setting up AwesomeWM on my phone by using Termux's ability to install distros though.

[actionless]

just intuitively for me indeed seems having negative offset makes sense for moving a layer to opposite direction of coordinates, while for layer order would make sense some separate thing like z-index

(but please don't take my opinion on this too seriously, since as i mentioned before i never had a usecase for that type of widget)