Open sclu1034 opened 2 years ago
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.
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.
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.
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?".
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.
(and bump it)
The current situation is:
base.fit_widget
returns 0, 0
when not .visible
0, 0
"legitimately" for any other reason they want to, and do so
base.fit_widget
will clamp values to the available sizeTherefore:
0, 0
, it cannot distinguish between
0, 0
big, and should be rendered as such.visible = false
and should not be renderedwidth
and height
it provided, it cannot distinguish between
.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
:
not .visible
, then spacing should be skipped for the widget, too0, 0
, it should still be rendered and spacing should not be skippedHave base.fit_widget
return all the information that it's currently hiding/mixing together:
@Elv13 @Aire-One your thoughts?
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
Also, I would prefer the third argument to be an
hints
table. This will avoid the mess we had withawful.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
.
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.
And regardless of what the decision ends up at eventually, it does make sense to put that behind a api_level >= 5
gate.
More elaborate description at https://github.com/awesomeWM/awesome/issues/3531#issuecomment-1243799059.
Extracted from #3309, as this is a more general issue with layouts:
Reworded, the problem is that
base.fit_widget
encodes multiple different things into0, 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 reports0, 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 is1, 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, returning0, 0
. But that's the condition wherewibox.layout.fixed
shouldbreak
the loop.So there is currently no way for a layout to distinguish between
0, 0
because it isvisible == false
. The layout should skip the widget and keep iterating0, 0
because the remaining space is too small. The layout should stop iteratingCurrent proposal: https://github.com/awesomeWM/awesome/issues/3531#issuecomment-999126630