base.fit_widget hides information from the parent widget

[sclu1034]

More elaborate description at #3531 (comment).

---

Extracted from #3309, as this is a more general issue with layouts:

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

Reworded, the problem is that base.fit_widget 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) [similar to how spacing is already accounted for when the widget is 1, 1 big, which is almost indistinguishable, visually].
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.

So there is currently no way for a layout to distinguish between

• the child widget returns 0, 0 because it is visible == false. The layout should skip the widget and keep iterating
• the child widget returns 0, 0 because the remaining space is too small. The layout should stop iterating

---

Current proposal: #3531 (comment)

[sclu1034]

While this came up with the overflow layout, the current implementation of wibox.layout.fixed already has issues with this. In this test, both widget:fit and widget:layout fail:

diff --git a/spec/wibox/layout/fixed_spec.lua b/spec/wibox/layout/fixed_spec.lua
index a272b7910..ebf868809 100644
--- a/spec/wibox/layout/fixed_spec.lua
+++ b/spec/wibox/layout/fixed_spec.lua
@@ -183,6 +183,29 @@ describe("wibox.layout.fixed", function()
         end) -- with spacing
     end) -- with widgets
 
+    describe("with invisible child widget", function()
+        it("skips spacing", function()
+            local first = utils.widget_stub(30, 10)
+            local second = utils.widget_stub(30, 10)
+            local third = utils.widget_stub(30, 15)
+            local spacing_widget = utils.widget_stub(30, 10)
+
+            layout.spacing = 10
+            layout.spacing_widget = spacing_widget
+
+            second.visible = false
+
+            layout:add(first, second, third)
+
+            assert.widget_layout(layout, { 30, 100 }, {
+                p(first, 0, 0, 30, 10),
+                p(spacing_widget, 0, 10, 30, 10),
+                p(third, 0, 20, 30, 15),
+            })
+            assert.widget_fit(layout, { 30, 100 }, { 30, 35 })
+        end)
+    end)
+
     describe("emitting signals", function()
         local layout_changed
         before_each(function()

The output of assert.widget_fit is

Failure → spec/wibox/layout/fixed_spec.lua @ 187
wibox.layout.fixed with invisible child widget skips spacing
spec/wibox/layout/fixed_spec.lua:200: Offering ((number) 30, (number) 100) to widget and expected ((number) 30, 35), but got (30, 55)

For assert.widget_layout, see the screenshot in #3530.

[sclu1034]

One potential solution would be to add an additional return value:

function mywidget:fit(w, h)
	h = math.max(h, 10)
	return w, h
end

function base.fit_widget(widget, ...)
	if widget.visible == false then
		return nil, nil, true
	end

	return widget:fit(...)
end

local w, h, skip = base.fit_widget(...)

If the returned size is greater than the available space, it is up to the layout to decide what that means. The common case would likely be "stop processing more widgets, as I've exhausted the available space".

If skip == true, the layout should act as if the widget didn't even exist (e.g. wibox.layout.fixed should also skip any spacing around this widget). The obvious case for this is w.visible == false, but a widget's :fit may also want to report this based on its own logic/state.

---

Update 1:
Remove the fits flag and simply have the widget return its minimum size instead, even if that is greater than the available space.

Update 2:
Re-wording for clarity.

[HumblePresent]

In the current implementation of fit() for the fixed layout, if base.fit_widget() returns 0, 0 for whatever reason the layout spacing is also added (unless it's for the last child widget). There is no distinction between a child who returned 0, 0 because it's visible property is false and a widget who returned 0, 0 based on some internal state. If we want layouts to distinguish between those two cases we would need to implement some variation of the change proposed by @sclu1034 above.

I do not think a child widget returning 0, 0 to communicate "that's not enough space for me" is strictly necessary or expected in the current pattern. The child widget is expected to return the desired space, while it is up to the layout's fit() implementation to determine if that request can be accommodated or not. That is my impression based on the code but maybe @Elv13 would have more insight.

To summarize my thoughts, I agree that a change would need to me made for layouts to be able to distinguish between the first two 0, 0 cases, but I don't think it's necessary to support the third.

[sclu1034]

I do not think a child widget returning 0, 0 to communicate "that's not enough space for me" is strictly necessary or expected in the current pattern

Which is why my proposal includes a dedicated boolean value for that. It might be enough to just return a value greater than what was provided, but I'm currently not sure if that would be free of edge cases.

The child widget is expected to return the desired space

I guess you mean the right thing, but the wording is a bit off. If it was "desired" space, I would expect imagebox:fit() to always return the size of the image file, since that would be the ideal size for the widget. But with resize = true, an imagebox could "make due" with a smaller size or expand by upscaling when more space is available.
So the semantics of :fit are "given this amount of space, how much of it do you want?".

[HumblePresent]

I guess you mean the right thing, but the wording is a bit off.

The docs describe fit() as

This function is called to select the size of your widget. The arguments to this function are the available space and it should return its desired size. Note that this function only provides a hint which is not necessarily followed.

Yes, some widgets like the imagebox adjust their size based on what's available but it's still up to each widget's implementation to determine its size needs and whether to take into account the available size or not.

Something to note is that base.fit_widget() will never return anything greater than the original width and height passed as parameters. The output of the widget's fit() method is clamped if it's greater than the sizes base.fit_widget() was called with. If there is not enough space for a widget it simply uses the rest of the available space. In that case I can see value in adding a mechanism for widgets to communicate that there is not enough space for them to render fully. I think we just need to decide if there is a need for it or if the current layout implementations are ok the way they handle it now.

I thought of a specific case that exemplifies the change in behavior being discussed here. Say we have a fixed layout that is constrained in the direction of the layout and then add several widgets to it. Eventually, a widget is added but there is not enough space for the whole widget to be drawn, so it just takes the remaining available space. When the layout and child widgets are drawn the one that doesn't have enough space will still be drawn, but will be clipped because of the layout size constraint. If instead, the widget that didn't have enough space returned a boolean indicating that, it would not be drawn at all and the layout would not expand to it's absolute max size. The first scenario is what happens in the current implementation and the second would be possible if we made this type of change. I think we should consider whether we want widgets that extend beyond available space to be drawn and clipped or not drawn at all. I suppose the widgets' fit() method could still return their desired size along with the boolean and allow the layout implementations to decide what to do with the information.

[sclu1034]

To summarize (or clarify) the problem

(and bump it)

The current situation is:

• base.fit_widget returns 0, 0 when not .visible
• widgets may return 0, 0 "legitimately" for any other reason they want to, and do so
  • sometimes, they return that when they don't fit in the available space
• base.fit_widget will clamp values to the available size

Therefore:

• if a container/layout receives 0, 0, it cannot distinguish between
  • the widget really is 0, 0 big, and should be rendered as such
  • the widget is set to .visible = false and should not be rendered
  • the widget is too big
• if a container/layout receives values equal to the width and height it provided, it cannot distinguish between
  • the widget really wants that size (will be rendered correctly)
  • the widget doesn't fit and was clamped (will be clipped)
• if a user actively sets .visible = true, .forced_width = 0, .forced_height = 0, they explicitly ask for a situation where the widget is rendered at 0, 0

There are likely various containers/layouts that have problems with this situation, but the one where it surfaced in #3309 is wibox.layout.fixed:

• if a widget is not .visible, then spacing should be skipped for the widget, too
• if a widget is "legitimately" sized 0, 0, it should still be rendered and spacing should not be skipped
• if a widget requires/wants more than the available size, subsequent child widgets must not be rendered (i.e. iteration must stop)
• if a widget requires/wants more than the available size, the user should be able to decide whether it is still rendered (but clipped) or if it is not rendered

Suggested solution

Have base.fit_widget return all the information that it's currently hiding/mixing together:

• do not clamp the returned size anymore, so that widgets can report when they don't fit by returning more than what's available
• add a third, boolean return value that encodes "widget should not be rendered/should be skipped"

[sclu1034]

@Elv13 @Aire-One your thoughts?

[Elv13]

I am not against, but there is some tiny risks of breaking some people. While git grep -E '[(][a-z1-9]+[.]fit_widget' returns nothing to worry about (code doing function foo(a,b,c) if c then --[[...]] end end; foo(base.fit_widget), some code expose the result of fit_widget, like the taglist and tasklist. Those can be cleaned up on our side. Time has proven again and again that someone, somewhere will be affected if we "just make the change". That can either be documented in the changelog, or we an see if we can fix the larger problem without actually breaking the API (see below).

Also, I would prefer the third argument to be an hints table. This will avoid the mess we had with awful.widget.common where it accumulated endless return values.

if a widget is "legitimately" sized 0, 0, it should still be rendered and spacing should not be skipped

That will be hard to do in API level 4, but could be enabled for API level 5[1]. Even with the hints, some people might have used this "feature" on purpose in custom widgets.

[1] Note that API level 5 does not mean AwesomeWM v5.0, it means "this will be the default for rc.lua in AwesomeWM v4.5. API level 4 is the default in v4.4. This is just a way to introduce opt-in behavior changes without breaking existing configs (Android style). People are free to use whatever API level they want. Modules are free to assert(awesome.api_level >= 5, "This module doesn't support antiquity! It's 2032 already, AwesomeWM 4.5 has been released for like... 2 months") if they don't support older API levels.

if a widget requires/wants more than the available size, subsequent child widgets must not be rendered (i.e. iteration must stop)

We need to be careful here. This was submitted(-ish) and reverted a couple time already. They need to be part of layout regardless because otherwise the signals are not handled properly. I think it can be done, but removing invisible widgets from the layout is not really possible (if they are in the middle, or etc, etc, etc).

do not clamp the returned size anymore, so that widgets can report when they don't fit by returning more than what's available

That will be very tricky to do safely. We need to design something which limits the blast radius of such change. Coming from a Qt background, I fully understand why that's better than what we do. And also why size policies + min/max/preferred sizes solve real problems. But how can we make this drop-in without causing an user revolt like the last time we made a change to this API (3.5->4.0 when ctx was added for the DPI support. tl;dr: it didn't go well).

Maybe adding an optional :fit_policy() method which can be implemented instead of :fit() (both implemented, but :fit() ignored when the layout/container supports :fit_policy())? This way widgets who care can express the whole min/max/preferred/policy/xy_offset values while keeping the simpler API for everything else? Note that widgets are allowed to draw outside of their clip to implement shadows and powerlines, this adds a few corner cases. I also don't think there is a lot of popular 3rd party widget layouts out there. There's plenty of containers, but they are a lot less affected by the issue (I think) you are attempting to solve, like spacing duplication ambiguity.

edit: typo

[sclu1034]

Also, I would prefer the third argument to be an hints table. This will avoid the mess we had with awful.widget.common where it accumulated endless return values.

That will add the overhead of a table allocation, though. If we do expect a large selection of infrequently used hints, that would make sense, but there is only so much information that :fit can provide in the first place.
One of the earlier proposals did include two new boolean values (one for skip, one for "does not fit"), but I wouldn't expect it to grow much beyond that.

(some profiling on the widget system would be interesting, though)

I don't know the history of awful.widget.common, but just at a glance, it seems to approach the state of a jack of all trades. So it makes sense that return values got out of hand there.

if a widget requires/wants more than the available size, subsequent child widgets must not be rendered (i.e. iteration must stop)

We need to be careful here. This was submitted(-ish) and reverted a couple time already. They need to be part of layout regardless because otherwise the signals are not handled properly. I think it can be done, but removing invisible widgets from the layout is not really possible (if they are in the middle, or etc, etc, etc).

That is how wibox.layout.fixed currently works, though.
Once the available space is used up, break stops the loop and no more child widgets are added to result.

awesome/lib/wibox/layout/fixed.lua

Lines 64 to 84 in 6ca2fbb

	if index ~= widgets_nr or not self._private.fill_space then
	h = select(2, base.fit_widget(self, context, widget, w, h))
	zero = h == 0
	end
	if y - spacing >= height then
	-- pop the spacing widget added in previous iteration if used
	if spacing_widget then
	table.remove(result)
	-- Avoid adding zero-sized widgets at an out-of-bound
	-- position.
	y = y - spacing
	end
	-- Never display "random" widgets as soon as a non-zero sized
	-- one doesn't fit.
	if not zero then
	break
	end
	end

And I'm not aware of a way to change this without having "random widgets displayed", as the comment puts it. We're lacking a separate "consider these widgets for signals, but not for display" results table for :layout.
Widget of size 0, 0 could still be handled as they are right now, except it would be explicit when spacing is or isn't wanted.
And generally, when I've been talking about "skipping" or "stop iterating", I didn't mean "do not consider signals anymore" You obviously need those to allow changes in .visible or the return value of :fit to propagate.
I'm merely talking about the visual side of things.

But either way, having base.fit_widget return more information shouldn't break any of this. It does change API and may require changing existing code, but worst case scenario, the layout just ignores the extra information, best case scenario, the extra information allows it to do a more accurate/efficient decision.

Maybe adding an optional :fit_policy() method which can be implemented instead of :fit() (both implemented, but :fit() ignored when the layout/container supports :fit_policy())? This way widgets who care can express the whole min/max/preferred/policy/xy_offset values while keeping the simpler API for everything else?

I'm not aware of any issues with :fit that would require changes/a replacement. Everything talked about so far is within spec for :fit.
The issues all arise in what the wrapper base.fit_widget does to the result of :fit.
E.g. widgets are already "allowed" to have :fit return values greater than what was passed in. It's base.fit_widget that clamps that.

[sclu1034]

And regardless of what the decision ends up at eventually, it does make sense to put that behind a api_level >= 5 gate.