Add Container widget

[zaynetro]

Creating this WIP PR early as I have encountered questions I would like some feedback on.

Background and padding seems straightforward at the moment.

My main struggle was border. When stroking the shape we can draw outside of the widget constraints. To compensate that I needed to shrink the child widget box constraints.

I've noticed that Button has the same issue with 1px being outside of given box constraints.

Current implementation:

No extra logic (this is what happens with Button):

My question is what is the right way to tackle this issue?

[cmyr]

My inclination would be to treat stroke like padding, and always stroke inside? I don't think widgets should draw outside of their layout bounds except in special circumstances, like if we allow drop shadows. This is just my gut feeling though, not sure if there are good arguments for another approach?

[raphlinus]

The "foreground" of a widget should be inside the layout bounds, though this is a bit of a subjective criterion. I do want to have more sophisticated tracking of paint bounds, but right now it's loose; you can assume you are allowed to paint anywhere. We're probably not systematic about this in our existing bordered widgets (button, textbox, etc), but it's probably not a huge deal because those borders tend to be thin.

Also, in low-dpi environments, stroking "inside" the layout bounds is more likely to yield a sharper line, though currently we're not quantizing layout to integers (we should).

[futurepaul]

Wow, I think every single one of my widgets is problematic in this regard! A simple solution (as in it's Piet's fault, not mine) for thin borders would be to have inside / outside / center options for stroke.

Fwiw, Flutter has both versions. Container counts the border as additional padding, while DecoratedBox paints the border outside its size and doesn't clip it.

[zaynetro]

Thanks for the comments! I plan to get back to this MR by the end of October. Currently overwhelmed at work...

[zaynetro]

OK. I think current container is already good enough. Let me know if you think something else must be included.

[cmyr]

/bloat

[github-actions]

🗜 Bloat check ⚖️

Comparing 5049969 against f92b2d7

target	old size	new size	difference
target/release/examples/calc	1010.11 KB	1008.19 KB	-1.91 KB (-0.19%)
target/release/examples/scroll_colors	963.99 KB	978.75 KB	14.77 KB (1.53%)
target/release/examples/multiwin	1008.66 KB	1007 KB	-1.66 KB (-0.16%)
target/release/examples/textbox	1018.25 KB	1015.71 KB	-2.55 KB (-0.25%)
target/release/examples/custom_widget	972.96 KB	970.58 KB	-2.38 KB (-0.24%)

[zaynetro]

Good to go?

[cmyr]

Some notes inline, and high-level thoughts here:

I have some reservations about this API. Part of this might be solved by documentation. The basic issue, to me, is that we're using a builder pattern but where method ordering matters, e.g.

// okay
let container = Container::new().color(RED_COLOR).child(other_widget);
// error
let container = Container::new().child(other_widget).color(RED_COLOR);

I was also confused, while reading, by the ContainerRaw type.

If I were designing this, I think I would try an API like,

impl Container {
    fn new<W: Widget<T> + 'static>(inner: Option<W>) -> Self {
        // ...
    } 

And I would remove the padding option, and make the user explicitly pass in a Padding as the child if they want padding. This would let us combine Container and ContainerRaw.

If we did want the padding convenience, we could have a separate constructor, like:

fn new_with_padding(inner: impl Widget<T>, uniform: f64) -> Self { ... }

But I might be more inclined to just let the user be explicit about padding, for now.

Also, while it's on my mind: an option I'd like to see on this eventually is corner radius.

[zaynetro]

ContainerRaw is basically simulating what TextBoxRaw or CheckboxRaw is doing. I followed the mechanism that we already have. Although, I agree that the constructing might need some improvements and I will think about those.

Thanks for the comments!

[cmyr]

I hadn't really looked at checkbox, so thanks for that; I probably would have had similar questions if I'd been doing the review for that PR. In the case of checkbox it seems like this pattern is particularly curious, since it would be just as easy to impl Widget on the Checkbox struct itself.

In any case, I would not consider this to be a general pattern to be followed in most cases; it should be a pattern we pick for specific reasons related to the constraints of the implementation.

So I think this is okay if you think it's the best approach, but you should not feel like this pattern is a requirement.

I would like some documentation for all public items. If you're not sure about what these docs should be feel free to ping me on zulip and we can talk about it.

Let me know when you think this is ready for another look!

[Felk]

Regarding the inside/outside drawing issue: Having dealt with various CSS styling and layouting headaches on the web, I've personally come to the following conclusions:

• Side effects to layouting shall always be contained within the widget. E.g. if A and B are 100px wide each, but B has a padding of 20px, B should not be 140px wide total, but stay at 100px. Anything else is imo unintuitive and makes layouting harder. Have a look at the documentation of the CSS-property box-sizing for more information.
• Now this isn't directly relevant to the border-drawing issue, because just drawing outside of the bounds wouldn't affect layouting. Nontheless, like cmyr already said, I'd expect a border to behave similarly to a padding and stay within the bounds.
• Also, for people being familiar with CSS, the behaviour of drawing outside of the element's bounds will probably be intuitively known as an outline instead of a border.

[zaynetro]

OK. I removed ContainerRaw in favour of just one struct. There is one question that I am not sure about. Otherwise, I think it is in its final shape.

@cmyr

[cmyr]

Okay, this looks good to me!

[cmyr]

alternatively,

self.style.border.as_ref().map(|b| b.width).unwrap_or_default();

(not necessary to change, just wanted to point out the one-liner)