#1397: Fix fill + max #1569
#1397: Fix fill + max #1569
Conversation
| for (i, child) in items.iter().enumerate() { | ||
| let fill_factor = match axis { | ||
| Axis::Horizontal => child.as_widget().width(), | ||
| Axis::Vertical => child.as_widget().height(), | ||
| } | ||
| .fill_factor(); | ||
|
|
||
| if fill_factor != 0 { | ||
| let max_main = available * fill_factor as f32 / fill_sum as f32; | ||
| let min_main = if max_main.is_infinite() { | ||
| 0.0 | ||
| } else { | ||
| max_main | ||
| }; | ||
|
|
||
| let (min_width, min_height) = if align_items == Alignment::Fill { | ||
| axis.pack(min_main, cross) | ||
| } else { | ||
| axis.pack(min_main, axis.cross(limits.min())) | ||
| }; | ||
|
|
||
| let (max_width, max_height) = if align_items == Alignment::Fill { | ||
| axis.pack(max_main, cross) | ||
| } else { | ||
| axis.pack(max_main, max_cross) | ||
| }; | ||
|
|
||
| let child_limits = Limits::new( | ||
| Size::new(min_width, min_height), | ||
| Size::new(max_width, max_height), | ||
| ); | ||
|
|
||
| let layout = child.as_widget().layout(renderer, &child_limits); | ||
| let size = layout.size(); | ||
|
|
||
| let unused = match axis { | ||
| Axis::Horizontal => max_width - size.width, | ||
| Axis::Vertical => max_height - size.height, | ||
| }; | ||
| if unused > 0.0 { | ||
| capped[i] = true; | ||
| fill_sum -= fill_factor; | ||
| available -= axis.main(size); | ||
| nodes[i] = layout; | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I really really woud like to avoid introducing additional cost to this function. It's a very critical path for our layout performance.
Instead of introducing additional logic, I'd like to explore other options first. Like maybe changing Widget::width and Widget::height to return further information, enhancing our Limits type, etc.
The most important shortcoming of the current layout logic here is that a Fill inside a Shrink will cause the Shrink to fill all the remaining space in the parent, independently of anything. A container with a Fill child should somehow be Fill as well.
I believe we should focus on rethinking our layout engine to deal with this basic shortcoming first, as doing so may shine a light on other inconsistencies (like this one).
There was a problem hiding this comment.
Instead of introducing additional logic, I'd like to explore other options first. Like maybe changing
Widget::widthandWidget::heightto return further information, enhancing ourLimitstype, etc.
Please correct me if I'm mistaken, but wouldn't any Widget changes still be dependent on receiving the available space from resolve? Like in this specific example, the widget width/height either needs to act like Length::Units(max) (if available space > max) or Length::Fill (if available space <= max), and the outcome would affect the available space for any remaining Fill widgets. I'm not quite sure how that would be possible without changing this function (or some equivalent of it in an updated layout engine).
There was a problem hiding this comment.
Do you think it would make sense to have Length variants like this? (just FillMax would solve #1397, but the others are for the sake of example)
pub enum Length {
// Either a few very specific combinations:
FillMin(u16),
FillMax(u16),
FillWithin {
min: u16,
max: u16,
},
// Or something more generic that may allow unwanted combinations:
FillAll {
portion: Option<u16>,
min: Option<u16>,
max: Option<u16>,
}
}
That way, resolve would know if any of the widgets have a max set, and it could only trigger the extra code/cost if there are widgets that need it.
If it doesn't belong in Length directly, then maybe have Widget::width return a Width like this?
struct Width {
length: Length,
max: Option<u16>,
}
|
As I mentioned in #1722, I think we should explore the limits of a single pass layout engine a bit more before we decide to introduce more complexity. On a related note, I am working on fixing some layout inconsistencies in the Let's close this for now, and revisit it in the future! |
This adds a pass so that, for example, when a container is both
.width(Length::Fill)and.max_width(200), if it ends up being capped by its max, then its sibling containers can fill the unused space.This fixes #1397.
Before
After