Replace BoxConstraints and update Layout function #701
Conversation
There was a problem hiding this comment.
General note: it feels like you could have kept the diff a lot smaller by keeping the BoxConstraints type and changing its internals.
As it is it feels like a lot of diff lines are just replacing one thing by another semantically equivalent other thing, which makes the whole PR harder to review.
Overall, I'm wondering whether this code actually moves us closer to Taffy integration. I guess the ContentFill enum is closer to taffy's AvailableSpace, but is that necessary?
From the perspective of what I'd like Masonry's layout to look like long term, this isn't it. I've described my thought process a few times for the direction I wanted layout to move towards, and this PR is at best orthogonal to that direction.
(My thoughts on the subject are scattered in a few places, though. I'll go into more details on Zulip.)
That doesn't mean we can't accept it, just that a lot of its changes would be almost completely erased by the now imminent layout refactor.
masonry/src/lib.rs
Outdated
| extern crate core; | ||
|
|
There was a problem hiding this comment.
Is that supposed to be here?
There was a problem hiding this comment.
My IDE must have added it. Removed.
masonry/src/passes/layout.rs
Outdated
| let input_size = match root.size_policy { | ||
| WindowSizePolicy::User => window_size, // Note: this was "tight" | ||
| WindowSizePolicy::Content => BiAxial::UNBOUNDED, | ||
| }; | ||
| let content_fill = BiAxial { | ||
| horizontal: Exact, | ||
| vertical: Exact, | ||
| }; |
There was a problem hiding this comment.
The combo of "unbounded input size" with "exact content fill" doesn't make sense to me. Does it work in practice?
There was a problem hiding this comment.
That's an illegal combination. I hit it when testing size_policy. But the current code does not deal with unbounded dimensions well.
It makes me think that I should merge all layout parameters into one type to validate that the combinations of parameters are valid.
| let child_fill = if self.must_fill { | ||
| BiAxial::new(ContentFill::Exact, ContentFill::Exact) | ||
| } else { | ||
| // TODO: Somehow allow specifying which axis should overflow. | ||
| // once that happens, we can specify the major axis as Max, and the minor as Constrain. | ||
| BiAxial::new(ContentFill::Constrain, ContentFill::Constrain) | ||
| }; |
There was a problem hiding this comment.
Be aware that in the future we're likely to remove the must_fill field. Having a scroll area that somehow "must" be filled doesn't really make sense.
This pull request works towards implementing changes proposed in #37
It does so by replacing
BoxConstraintswith a generic type that has a vertical and horizontal component. The Layout function has theBoxConstraintsparameter replaced withavailable_spaceandrequested_fill.available_spaceis like the Size type; it uses the new generic type withf64as the type on the axes.requested_filltakes the place of theminsize in BoxConstraints, and will have more future uses with Intrinsic sizing. When set toExact, the widgets should behave the way they did when they previously got BoxConstraints with the min equaling the max. I did not find any instances where min wasn't set to either 0 by 0 or the max.I chose
BiAxialfor the name for the generic 2D type that is used for the layout parameters. I think it is descriptive while not colliding with the names of other types.Sizewas proposed in #37, but it collided with the Size type in several libraries, including Kurbo. And for the uses that were proposed, includingrequested_fill, I felt like a more generic name made sense. If the community doesn't like the name, I'm open to changing it.This code is ready for high-level review, including discussions on whether or not this is the way forward, as well as naming and file structure.
This design is a major step forward towards implementing Taffy integration.
The code still has at least one bug (like in the Flex example). I need to fix the linter errors, validate the tests, and fix the ordering of the code in the new files. But the examples should work mostly.