Constrain padding to resolved inner / outer widget sizes#1494
Conversation
| } | ||
|
|
||
| /// Returns the minimum of each component of this size and another | ||
| pub fn min(self, other: Self) -> Self { |
There was a problem hiding this comment.
Was just thinking that I wanted to add this to Iced the other day. Yay 🥳
There was a problem hiding this comment.
It could be worth to use euclid for geometric primitives. It also allow to create different types for different coordinate systems in order to not mix them.
| } | ||
|
|
||
| /// Constrains the padding to fit between the inner & outer [`Size`] | ||
| pub fn constrain(self, inner: Size, outer: Size) -> Self { |
There was a problem hiding this comment.
The algorithm of constraining in min-max range usually named clamp, so
| pub fn constrain(self, inner: Size, outer: Size) -> Self { | |
| pub fn clamp(self, inner: Size, outer: Size) -> Self { |
There was a problem hiding this comment.
I thought of clamp, but since this isn't passing it a min / max Padding I was unsure if it made sense
There was a problem hiding this comment.
khronos defines clamp to just be "constrain a value to lie between two further values" so think it works fine for this case. also just makes sense 2 me!
There was a problem hiding this comment.
constrain a value..
Welp, guess constrain works then :P
There was a problem hiding this comment.
clamp just seems like it should only take Self, Self is my only concern. There's no concept for Padding.min(Size) / Padding.max(Size), so Padding.clamp(Size, Size) feels awkward
There was a problem hiding this comment.
I think just leave it as constrain; it's readable & easy to follow.
| } | ||
| } | ||
|
|
||
| impl std::ops::Sub for Size { |
There was a problem hiding this comment.
while ur in here, a >, < and =<, => would be appreciated as well for Size if you're in the mood
There was a problem hiding this comment.
Not sure these make sense for Size. Which property would we use for comparison? Area? Where would that be useful?
There was a problem hiding this comment.
Area is the big thing I was working with the other day, yeah. Probably could be a separate struct though.
|
LGTM, tested examples (and removed scrollable from a few to see how the base container would function) and see no regressions, only improvements! |
hecrj
left a comment
hecrj
left a comment
There was a problem hiding this comment.
Looks good! Thanks! 🙇
I made some small changes here and there. Let me know if I broke anything, and we can merge!
| } | ||
| } | ||
|
|
||
| impl std::ops::Sub for Size { |
There was a problem hiding this comment.
Not sure these make sense for Size. Which property would we use for comparison? Area? Where would that be useful?
| } | ||
|
|
||
| /// Constrains the padding to fit between the inner & outer [`Size`] | ||
| pub fn constrain(self, inner: Size, outer: Size) -> Self { |
| top: self.top.min((available.height as u16) / 2), | ||
| right: self.right.min((available.width as u16) / 2), | ||
| bottom: self.bottom.min((available.height as u16) / 2), | ||
| left: self.left.min((available.width as u16) / 2), |
There was a problem hiding this comment.
I have changed these lines so we perform the cast to u16 first and then divide by 2. Integer division by 2 is way faster, and I believe both approaches produce equivalent results in the end.
|
Changes look great, |
4 participants
During layout, padding isn't constrained to the difference between the child widget and parent widgets max limits. This causes the parent widget to have the wrong resolved size and for the child widget to get offset by
paddinginstead of the constrained padding.This fixes the issue by constraining the padding to the min between itself and
limits.max() - child.size(). So far I've fixed Container, Button & Text Input as I've seen instances of this for all of them, but haven't combed the other widgets yet for padding.Below is an example of the fix implemented on
Todos, where both text input and buttons are bleeding over when their size should be 0. I've tested these fixes on some larger projects I maintain and couldn't find any regression in layout.Feel free to rename things, not sure
constrainis the best, I suck at naming :P