Implement overflow layout

[sclu1034]

This layout adds overflow behavior:

• adds a scrollbar
• adds scrollbar dragging to scroll
• adds mouse scroll behavior
• allows children to render at (near) infinite size in scroll direction

TODOs

• Implement
• Write docs
• Write tests
• Write examples
• SHOWSTOPPER: Wait for base.fit_widget hides information from the parent widget #3531 and adjust :layout to whatever comes from that
• Investigate compatibility of nested layouts
• Investigate per-widget scroll mode, i.e. "scroll step" becomes "scroll n-th next widget into view"
• Allow forcing the scrollbar to always be displayed
• Margin between content and scrollbar
• Styling options for the scrollbar background
  • Background color at the very least
  • Maybe also a generic widget slot to allow for stuff like hover effects

Demonstration

recording-20210331_224618.mp4

[codecov]

Codecov Report

Merging #3309 (d84dc1e) into master (70524e7) will increase coverage by 0.32%.
The diff coverage is 95.26%.

@@            Coverage Diff             @@
##           master    #3309      +/-   ##
==========================================
+ Coverage   90.66%   90.99%   +0.32%     
==========================================
  Files         852      904      +52     
  Lines       54440    57502    +3062     
==========================================
+ Hits        49358    52323    +2965     
- Misses       5082     5179      +97     

Flag	Coverage Δ
gcov	90.99% <95.26%> (+0.32%)	⬆️
luacov	93.70% <95.26%> (+0.20%)	⬆️

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

Impacted Files	Coverage Δ
lib/wibox/layout/overflow.lua	82.83% <82.83%> (ø)
lib/wibox/hierarchy.lua	99.47% <100.00%> (+0.01%)	⬆️
lib/wibox/layout/fixed.lua	71.71% <100.00%> (+0.94%)	⬆️
lib/wibox/layout/init.lua	100.00% <100.00%> (ø)
spec/wibox/layout/overflow_spec.lua	100.00% <100.00%> (ø)
tests/examples/wibox/layout/defaults/overflow.lua	100.00% <100.00%> (ø)
...ests/examples/wibox/layout/overflow/fill_space.lua	100.00% <100.00%> (ø)
...mples/wibox/layout/overflow/scrollbar_position.lua	100.00% <100.00%> (ø)
...xamples/wibox/layout/overflow/scrollbar_widget.lua	100.00% <100.00%> (ø)
...examples/wibox/layout/overflow/scrollbar_width.lua	100.00% <100.00%> (ø)
... and 228 more

[Aire-One]

Here is a first review. I'll do a second pass later...

I have adopted the "nit" notation for "not important changes" (aka dismissable comments) ;)

Here are some oter notes:

I think the position property name is misleading. It can lead to confusion with the scrollbar_position property and is not very descriptive to its usage. Maybe something like "scroll_factor" can be better?

I can see you have added inherited documentation from fixed layout. Can you add the ldoc @baseclass wibox.widget.fixed tag to these blocks waiting for my other PR to be merged? So that, the doc for this module is fully operational by itself. We can do a patch later to add the @supermodule tag and remove duplicated doc.

BTW, I can't see some methods like fixed:add and fixed:remove. I think we are supposed to have access to these methods as well.

Also, in regard to the "use gears magic", I suggest removing the documentation for your getters and setters.
Unless you want to wait my other PR to use @hidden here...

Examples tests/examples/wibox/layout/overflow/scrollbar_width.lua and tests/examples/wibox/layout/overflow/scrollbar_widget.lua don't show scrollbar at all. I have fixed it by adding a fourth widget.

[Elv13]

Fantastic contribution. Many people, me included, have used many hacky tricks to get this kind of feature. At first I feared it one one of those sub-surface type container, but no, it is implemented very nicely! I really like that it's using a custom widget for the scrollbar. At first I was thinking the slider widget code might have been re-used/shared, but looking into it closer, what you did it better.

One bug I think this has, but I have not tested, is when using this within a mirror or rotate container. This is why the geo.hierarchy:get_matrix_from_device and matrix:transform_point(mouse.x, mouse.y)) lines from the slider widget are important. There is some legitimate use case for mixing those containers with the overflow one. The main one if people using it to rotate their entire wibox 90 degree and having something like the taskbar in an overflow container. It's one of those annoying corner case, but I would like it to be fixed. It ain't that hard to fix.

[psychon]

What if the overflow container is shown in more than one place with different sizes? Assigning size-information to "something in self" sounds like either a bug or a bug-waiting-to-happen to me.

This problem is why the scroll container does all that funky business with weak tables.

(Being able to show a widget in more than one place is great for users, but it brings so many complications...)

[sclu1034]

This might be a showstopper then. Knowing the exact geometry is pretty much required for (useful) scrolling to work.

I did check out the scroll container, but can't claim to understand its funky business yet. And even if I did, combining funky business with user input sounds like a "bug-waiting-to-happen" as well.

It would probably require changes to the core of the widget system so that when something attempts to re-use an existing instance, that widget may request to be cloned instead (maybe with custom logic attached to the clone function).

[actionless]

could it be imposed some limitation against multiple instances? (so it will throw a warning and won't attempt to draw more than one instance)

[sclu1034]

That would certainly be an option and I would be fine with imposing such a limitation.
But this would certainly be the first widget to do so, although looking at the rework of wibox.widget.graph it's not the only one that would benefit from adding widgets with this kind of limitation.

[actionless]

arent wibox/wibar/popup also could have only one instance of a given geometry?

[psychon]

That container works via the context argument that is passed in to widgets. When context.screen is the expected screen, it works normally. Otherwise, :fit returns 0, 0 and :layout does nothing. Anyway, you can still show a widget multiple times on a given screen.

(And I guess this widget does not work correctly when used in a titlebar and you move the client to another screen. Or when used in a wibox and that wibox is moved to another screen. Nothing invalidates the caches or triggers a relayout in these cases.)

[actionless]

i think we could create the new container to accomodate this case, by just storing which parent widget first displayed it, and ignore all other fit/layout request from different parent widgets?

[actionless]

however, from the second thought, i feel like it should be acomodated here in overflow container - just have internal table for storing data for multiple parent widgets

[actionless]

like

    self._private.instances[parent_id].avail_in_dir = avail_in_dir
    self._private.instances[parent_id].used_in_dir = used_in_dir

[psychon]

by just storing which parent widget first displayed it,

Imaginary bug report: Here is some code that first displays a widget in A, then, on a key binding, it moves things to B. However, the widget disappears.

Also, a widget does not really know its parent. The existing caches do not track the parent widget. So, if you once return 0, 0, the cache will keep returning that also for other parents.

[psychon]

This clips what is drawn, but the input code does not know that you clipped something. Assuming you place a widget partially outside of the container (I have not yet entirely understood :layout() yet and am not 100% sure what is done here), it will still be possible to click into the not-visible parts of the widget.

(The scroll container does dark magic that results in no mouse clicks being possible; not really much better.)

[sclu1034]

Your assumption is correct. Another user reported this as well, as a general problem with clipped drawing. If I remember them correctly, clicks fire for any non-visible part of a widget inside a container with some cr:clip(), even if that part doesn't extend outside the container's geometry.

For the specific case here, this patch supposedly helps: xinhaoyuan@384b9ea, but I guess there are other use cases that need things to work like they do currently.

[psychon]

If clipping of child widgets is something that is needed... well, I guess it might (in theory) be possible to extend wibox.widget.base.place_widget_* with a shape parameter.

However, the result can easily end up being ugly: Doing hit tests against a gears.shape requires creating a temporary cairo surface and context, applying the shape, and then checking with something like cr:in_fill(x, y). This sounds like a "nice" way to end up with high memory usage and/or slow code.

[sclu1034]

I don't think we would need a separate clip shape per child widget. A single method widget.clip_shape on the layout/parent should be enough.

And the special case of widget.clip_shape == gears.rectangle (or by checking an additional property) could then be optimized to skip the temporary cairo surface and use the patch linked above.

[sclu1034]

The problem is that base.fit_widget() returns 0, 0 for any widget whose visible property is false

Oh, that's bad. I didn't realize until now, but base.fit_widget actually encodes multiple different things into 0, 0.

When spacing is involved, it actually makes a difference whether a widget is visible = false and both the widget and the spacing should be skipped, or if the widget only reports 0, 0 due to some internal state but should still be shown (or rather, should still be accounted for with spacing).
Similarly, if the layout doesn't have enough space to fit all children, currently the only way a child widget can report "that's not enough space for me" is, again, returning 0, 0. But that's the condition where wibox.layout.fixed should break the loop.

[HumblePresent]

currently the only way a child widget can report "that's not enough space for me" is, again, returning 0, 0

Can't child widgets return a size beyond what's available in the layout to indicate there isn't enough space? I thought fit() was supposed to return a preferred size even if that's not what they end up receiving.

[Elv13]

I don't know if it's relevant, but that reminds me of that bug 83c31f9

The 0x0 widgets are tricky to handle...

[sclu1034]

Can't child widgets return a size beyond what's available in the layout to indicate there isn't enough space? I thought fit() was supposed to return a preferred size even if that's not what they end up receiving.

Sure, that might be slightly less ambiguous. But that's still using a return value of two numbers that could be valid to encode information that shouldn't be encoded with numbers. And it wouldn't help at all with the visible = false part.

[sclu1034]

I've extracted the discussion about base.fit_widget into a separate issue, since this affects all layouts.

[HumblePresent]

I have another instance of unexpected behavior. I know I'm a bit of a thorn in the side of this PR right now, but better to work these things out now rather than later when someone creates an issue right 😁.

When I click on the scrollbar without moving the mouse the scrollbar moves and the scroll position changes. As soon as I move the mouse a little the scrollbar and and scroll position seem to snap back to their original location before being clicked on. I would expect the scrollbar to behave in a typical manner where it does not move until the mouse is moved.

[HumblePresent]

Just tested again and that last change fixed my issue.

[Aire-One]

@sclu1034, do you think it's now ready? I think we are alright.

I'd like to have the input from other maintainers about the new clip_child_extends, tho. I really think it should be documented. Worst case scenario : it will be marked as "inherited from widget.base" and the user will ignore it, like they already ignore "children" on most widgets.

Also, to mitigate the concern @psychon pointed that this layout can only be used at one place at a time, as opposed to how the widget system should work. Maybe we should have a mention of this in the module description. (It can be a good excuse to implement some sort of information/alert component in the doc https://getbootstrap.com/docs/4.0/components/alerts/)

[sclu1034]

do you think it's now ready?

That would be up to the reviewers to decide.
It's still affected by #3531, so it's "only" as much broken as all the other layouts.

it will be marked as "inherited from widget.base" and the user will ignore it, like they already ignore "children" on most widgets.

That's not a good comparison.
.children is a user-facing API (changing child widgets is the whole point of a layout, after all).
However, .clip_child_extends is not a user-facing API, just like :fit or :layout. A consumer of a widget should not change that property on the widget, only the widget's' implementation.
So, just like how :fit doesn't show up in the "Object methods" section, clip_child_extends should not show up in the "Object properties" section.

Would prefixing it with _ help with marking it as private?

[Elv13]

Sorry for the delay, I didn't work on AwesomeWM since xmas. I did some testing and it works. My earlier comments are addressed and the API looks good. Thanks a lot for this.

Now, yes, this container has some unfixeable limitations, but it provides a lot of value for people who don't use the more advanced widget system features. Thanks a lot for this contribution!

[HumblePresent]

Before this gets merged I do want to point out that the changes to fixed.lua do still break fixed layouts with any child widgets that have visible = false. By break I mean widgets that come after an "invisible" widget in the layout will not be shown regardless of their visible property. Is it possible to merge this PR without the changes to fixed.lua and make any necessary changes after #3531 has been resolved?

need to check @HunblePresent comment

[sclu1034]

As far as I can see, we have three options:

• Get base.fit_widget hides information from the parent widget #3531 fixed, which the current issue here blocks on
• Revert changes and reintroduce the bug described as 1. above (wibox.layout.overflow will be somewhat broken after merge)
• Keep the current bug with stopping on the first visible = false (regression in wibox.layout.fixed)

Ideally, we would do the first. However, if you do want this PR merged now, the second option would be preferable.

[TorchedSammy]

after using this myself i have noticed 2 problems:

• the doc entry is wrong for the vertical layout (https://safe.kashima.moe/qi0g6vvj0jq7.png)
• :reset() does not reset the scroll position

havent used it enough to notice any other problems, besides maybe the default step value being too low in my opinion

[Crylia]

I noticed when using two overflow containers (one inside another one) that both get scrolled, when scrolling with the mouse wheel. When scrolling you should probably check which is the nearest overflow widget under the cursor and only scroll that one.

[TorchedSammy]

some other things i recommend adding:

• background for the scrollbar widget
• spacing between contents and scrollbar (an easier way to do that)

[sclu1034]

For now this is fully blocked on #3531 .