Skip to content

#1397: Fix fill + max #1569

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

#1397: Fix fill + max #1569

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 52 additions & 1 deletion native/src/layout/flex.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,9 @@ where
let mut nodes: Vec<Node> = Vec::with_capacity(items.len());
nodes.resize(items.len(), Node::default());

let mut capped: Vec<bool> = Vec::with_capacity(items.len());
capped.resize(items.len(), false);

if align_items == Alignment::Fill {
let mut fill_cross = axis.cross(limits.min());

Expand Down Expand Up @@ -147,6 +150,54 @@ where
}
}

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;
}
}
}

Comment on lines +153 to +200
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>,
}

let remaining = available.max(0.0);

for (i, child) in items.iter().enumerate() {
Expand All @@ -156,7 +207,7 @@ where
}
.fill_factor();

if fill_factor != 0 {
if fill_factor != 0 && !capped[i] {
let max_main = remaining * fill_factor as f32 / fill_sum as f32;
let min_main = if max_main.is_infinite() {
0.0
Expand Down