Constrain padding to resolved inner / outer widget sizes #1494

tarkah · 2022-10-27T19:17:53Z

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 padding instead 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 constrain is the best, I suck at naming :P

Before	After

bungoboingo · 2022-10-27T19:24:59Z

core/src/size.rs

@@ -34,6 +34,22 @@ impl Size {
            height: self.height + padding.vertical() as f32,
        }
    }
+
+    /// Returns the minimum of each component of this size and another
+    pub fn min(self, other: Self) -> Self {


Was just thinking that I wanted to add this to Iced the other day. Yay 🥳

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.

Mingun · 2022-10-27T19:25:26Z

core/src/padding.rs

@@ -71,6 +73,18 @@ impl Padding {
    pub fn horizontal(self) -> u16 {
        self.left + self.right
    }
+
+    /// Constrains the padding to fit between the inner & outer [`Size`]
+    pub fn constrain(self, inner: Size, outer: Size) -> Self {


The algorithm of constraining in min-max range usually named clamp, so

Suggested change

pub fn constrain(self, inner: Size, outer: Size) -> Self {

pub fn clamp(self, inner: Size, outer: Size) -> Self {

I thought of clamp, but since this isn't passing it a min / max Padding I was unsure if it made sense

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!

constrain a value..

Welp, guess constrain works then :P

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

I think just leave it as constrain; it's readable & easy to follow.

I renamed it to fit in 7476663.

bungoboingo · 2022-10-27T19:26:45Z

core/src/size.rs

@@ -68,3 +84,14 @@ impl From<Size> for Vector<f32> {
        Vector::new(size.width, size.height)
    }
 }
+
+impl std::ops::Sub for Size {


while ur in here, a >, < and =<, => would be appreciated as well for Size if you're in the mood

Not sure these make sense for Size. Which property would we use for comparison? Area? Where would that be useful?

Area is the big thing I was working with the other day, yeah. Probably could be a separate struct though.

bungoboingo · 2022-10-27T19:37:50Z

LGTM, tested examples (and removed scrollable from a few to see how the base container would function) and see no regressions, only improvements!

hecrj

Looks good! Thanks! 🙇

I made some small changes here and there. Let me know if I broke anything, and we can merge!

hecrj · 2022-11-08T03:06:06Z

core/src/size.rs

@@ -68,3 +84,14 @@ impl From<Size> for Vector<f32> {
        Vector::new(size.width, size.height)
    }
 }
+
+impl std::ops::Sub for Size {


Not sure these make sense for Size. Which property would we use for comparison? Area? Where would that be useful?

hecrj · 2022-11-08T03:06:37Z

core/src/padding.rs

@@ -71,6 +73,18 @@ impl Padding {
    pub fn horizontal(self) -> u16 {
        self.left + self.right
    }
+
+    /// Constrains the padding to fit between the inner & outer [`Size`]
+    pub fn constrain(self, inner: Size, outer: Size) -> Self {


I renamed it to fit in 7476663.

hecrj · 2022-11-08T03:13:01Z

core/src/padding.rs

+            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),


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.

tarkah · 2022-11-08T03:22:42Z

Changes look great, fit is a fit naming choice!

tarkah added 2 commits October 27, 2022 11:50

Constrain padding to inner & outer sizes

2c103f8

Fix text input padding

ea4b3cd

bungoboingo reviewed Oct 27, 2022

View reviewed changes

Mingun reviewed Oct 27, 2022

View reviewed changes

bungoboingo reviewed Oct 27, 2022

View reviewed changes

hecrj added improvement An internal improvement widget layout labels Nov 8, 2022

hecrj added 4 commits November 8, 2022 03:56

Rename Padding::constrain to fit

7476663

Rearrange layout code to improve readability

914f099

Cast to u16 first then divide by 2 in Padding::fit

24d031b

Remove redundant std::convert namespace in padding

04087b2

hecrj approved these changes Nov 8, 2022

View reviewed changes

hecrj merged commit ac737cb into iced-rs:master Nov 8, 2022

hecrj added this to the 0.5.0 milestone Nov 8, 2022

tarkah deleted the fix/padding branch November 8, 2022 17:01

hecrj mentioned this pull request Nov 10, 2022

Release 0.5 — Stabilization of stateless widgets, first-class theming, widget operations, Lazy widget, and more! #1520

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Constrain padding to resolved inner / outer widget sizes #1494

Constrain padding to resolved inner / outer widget sizes #1494

tarkah commented Oct 27, 2022 •

edited

Loading

bungoboingo Oct 27, 2022 •

edited

Loading

Mingun Oct 27, 2022

Mingun Oct 27, 2022

tarkah Oct 27, 2022 •

edited

Loading

bungoboingo Oct 27, 2022 •

edited

Loading

tarkah Oct 27, 2022 •

edited

Loading

tarkah Oct 27, 2022 •

edited

Loading

bungoboingo Oct 27, 2022

hecrj Nov 8, 2022

bungoboingo Oct 27, 2022

hecrj Nov 8, 2022

bungoboingo Nov 8, 2022

bungoboingo commented Oct 27, 2022

hecrj left a comment

hecrj Nov 8, 2022

hecrj Nov 8, 2022

hecrj Nov 8, 2022

tarkah commented Nov 8, 2022 •

edited

Loading

	pub fn constrain(self, inner: Size, outer: Size) -> Self {
	pub fn clamp(self, inner: Size, outer: Size) -> Self {

Constrain padding to resolved inner / outer widget sizes #1494

Constrain padding to resolved inner / outer widget sizes #1494

Conversation

tarkah commented Oct 27, 2022 • edited Loading

bungoboingo Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarkah Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

bungoboingo Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

tarkah Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

tarkah Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bungoboingo commented Oct 27, 2022

hecrj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarkah commented Nov 8, 2022 • edited Loading

tarkah commented Oct 27, 2022 •

edited

Loading

bungoboingo Oct 27, 2022 •

edited

Loading

tarkah Oct 27, 2022 •

edited

Loading

bungoboingo Oct 27, 2022 •

edited

Loading

tarkah Oct 27, 2022 •

edited

Loading

tarkah Oct 27, 2022 •

edited

Loading

tarkah commented Nov 8, 2022 •

edited

Loading