Add a `FlexElement` to support explicit spacers and flex parameters for `Flex` in xilem #428

Philipp-M · 2024-07-15T19:47:13Z

Takes ideas from #235 (in particular the masonry side).

This implements support for explicit spacers in the Flex view and flex parameters for views (see examples flex and mason for more details).
It also adds a way (AnyFlexChild) to dynamically switch between spacers and widgets (with optional flex parameters).
masonry::widget::Flex::layout is also updated to be in sync with current druid.

Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>

Philipp-M · 2024-07-15T19:51:15Z

xilem/src/lib.rs

@@ -139,6 +141,13 @@ pub trait WidgetView<State, Action = ()>:
    View<State, Action, ViewCtx, Element = Pod<Self::Widget>> + Send + Sync
 {
    type Widget: Widget;
+
+    fn flex(self, params: impl Into<FlexParams>) -> FlexItem<Self, State, Action>


Having a builder method for this on the WidgetView trait, is obviously open for discussion, I'm not sure of it yet:

What may or may not make sense from an API standpoint is to support these kinds of methods, when the returned element is also a WidgetView, FlexItem is not a WidgetView, so it can't be used outside of the context of a Flex container view.

I think if we have this, it should be a separate FlexExt trait. That is, this method shouldn't be here.

I agree that the methods on WidgetView should return other WidgetViews.

(I wonder if we should have boxed(self) -> Box<dyn AnyWidgetView<State, Action>>)

Philipp-M · 2024-07-15T19:59:06Z

masonry/src/widget/flex.rs

-        // TODO
-        self.ctx.widget_state.children_changed = true;
-        self.ctx.widget_state.needs_layout = true;
+        self.ctx.children_changed();
    }

    // TODO - remove


I guess that could be removed now?

Philipp-M · 2024-07-15T19:59:15Z

masonry/src/widget/flex.rs

+                        any_use_baseline |= alignment == CrossAxisAlignment::Baseline;
+
+                        let old_size = ctx.widget_state.layout_rect().size();
+                        let child_bc = self.direction.constraints(&loosened_bc, 0.0, 9000.0); // TODO Infinity does lead to NaN and then crashes the app, if this is too big, it leads to floating point issues


I think I remember discussions about this, to avoid crashes (NaN and the likes), I set it to a high but not too high number (as vello goes crazy then, with weird floating point artifacts), the issue also exists for the current layout method

to avoid crashes (NaN and the likes), I set it to a high but not too high number

Pretty sure there's a Codeless Code fable about this. 😃

The fundamental problem is that Masonry's layout model is confused, not that Flex had the wrong maximum constraint. Though I guess this works until we fix the underlying problem.

Philipp-M · 2024-07-15T20:05:33Z

xilem/src/view/flex.rs

+    }
+}
+
+pub struct FlexItem<V, State, Action> {


Because of parameterization issues, I needed to add these generic parameters.

I guess we need to do this a lot more for other views as well, to seamlessly support stuff like Adapt or in general inference for these variables, when it's not explicit somewhere...

Yeah, this is a choice we have to make, and I think adding them to the View type is the right one, to keep the type inference failures local.

Philipp-M

Yeah that's better.

I think in the future it would be great to somehow implement a similar mechanism as WidgetMut to access the Child to avoid needing access to the parent/flex-container, but I don't think it should be done in this PR, and I'm still not sure if it's worth it...

DJMcNab

I haven't carefully examined the Masonry level changes, but I also think that's fine, because we're planning on changing Flex to use Taffy at some point anyway.

I think that Masonry flex needs to support something akin to CSS gap, because currently this significantly regresses all of our examples. That doesn't need to happen in this PR, but it does need to happen extremely soon after this lands.

The Xilem level changes look as I'd expect. If you want to add AnyFlexView in this PR, that would be very useful @Philipp-M, but I'm not going to block on it.

I would like to get @PoignardAzur's sign-off on WidgetMut::reborrow before landing.

DJMcNab · 2024-07-16T13:11:28Z

xilem/src/lib.rs

@@ -139,6 +141,13 @@ pub trait WidgetView<State, Action = ()>:
    View<State, Action, ViewCtx, Element = Pod<Self::Widget>> + Send + Sync
 {
    type Widget: Widget;
+
+    fn flex(self, params: impl Into<FlexParams>) -> FlexItem<Self, State, Action>


I think if we have this, it should be a separate FlexExt trait. That is, this method shouldn't be here.

I agree that the methods on WidgetView should return other WidgetViews.

(I wonder if we should have boxed(self) -> Box<dyn AnyWidgetView<State, Action>>)

xilem/src/view/flex.rs

PoignardAzur · 2024-07-17T11:24:38Z

Re: reborrow_mut, I wonder if you could skip the is_reborrow value and just apply merge_up multiple time. It's supposed to be idempotent anyway.

In theory it could have surprising effects if some other code reads the parent state and doesn't expect it to change; in practice the fact that WidgetMut holds a mut reference to said parent state means that will never happen.

PoignardAzur · 2024-07-17T11:24:55Z

Otherwise, LGTM.

DJMcNab · 2024-07-17T12:04:51Z

Re: reborrow_mut, I wonder if you could skip the is_reborrow value and just apply merge_up multiple time. It's supposed to be idempotent anyway.

The is_reborrow field is entirely an optimisation, because merge_up is a relatively heavy operation

PoignardAzur · 2024-07-18T08:36:40Z

because merge_up is a relatively heavy operation

It's not that heavy. About a dozen boolean flags getting OR-ed. (That's 16 bytes with bools, 2 with bitblags).

The two non-trivial bits of logic are text registrations and cursor changes, both of which should probably be moved out of merge_up.

The more concerning part is this:

        self.text_registrations
            .append(&mut child_state.text_registrations);

Which we probably don't want to duplicate. Once we remove that line, we can remove the is_reborrow flag. In the meantime I'd suggest adding a todo comment, but we can merge either way.

Philipp-M · 2024-07-18T08:41:10Z

we can remove the is_reborrow flag.

Does it hurt otherwise, I mean it's just little bit of extra mental overhead, and I assume WidgetMut::Drop will get more expensive in the future.

Btw. I have started with the AnyFlexView, it's more complicated than I thought (requiring all the generational id stuff), I want to get that in here as well, as I see value for it too.

And I will likely also add the FlexExt stuff, which I agree with

PoignardAzur · 2024-07-18T09:01:26Z

Does it hurt otherwise, I mean it's just little bit of extra mental overhead,

Fair enough, it's a minor point.

and I assume WidgetMut::Drop will get more expensive in the future.

I don't expect it to get much more expensive, but it's a fair point too.

… that can either be a spacer or a flex item, also documented a lot of the flex implementation

Philipp-M

I've started documenting (including doc tests, which at least check if all the typing checks out), added the WidgetView::boxed as I think it's an uncontroversal change, and the FlexExt trait, and of course added an AnyFlexChild view, which in its impl View logic is very similar to impl ViewSequence for Option<impl ViewSequence>. I've also pulled the Axis into View and reexported relevant enums for flex_item from masonry.

Since this is a bigger update, @DJMcNab I think it would make sense if you check at least the impl View for AnyFlexChild.

Philipp-M · 2024-07-18T22:35:11Z

xilem/src/view/flex.rs

+    /// # }
+    ///
+    /// ```
+    fn into_any_flex(self) -> AnyFlexChild<State, Action>


I've tried to do this via a blanket impl, but it doesn't work, since I can't specialize over View<Element = FlexElement> and WidgetView, and otherwise that impl intersects, so I hardcoded these directly on either V: WidgetView as well as for either FlexItem and FlexSpacer...

Did you try View<Element = Pod<W>> and View<Element = FlexElement>? The first should be equivalent to WidgetView, but might be more friendly to the compiler

Yep I tried, I did not make it to work, unfortunately...

Philipp-M · 2024-07-18T22:39:17Z

xilem/examples/mason.rs

        .collect::<Vec<_>>();
+
+    let flexy_flex_sequence = [2, 3, 4].map(|c| {


I guess this is our catch-all playground example? This also uses [impl ViewSequence; N] btw.

Yeah - it's fine to stick anything in here 😆

DJMcNab

Thanks! A few minor feedback things, then we can land this.

xilem/src/view/flex.rs

DJMcNab · 2024-07-19T07:05:58Z

xilem/src/view/flex.rs

+/// ))
+/// # }
+///
+/// ```


Ideally we'd have an image here. We really should set up linebender.org/assets at some point

(To be clear, this is fine as-is)

Yeah our documentation story can certainly be improved, but I hope this PR does improve it a little bit at least.

I also wonder, if the doc-tests could be combined with snapshots, such that this really also tests the logic (while reducing the amount of manual tests) (But I doubt that this is simple, if at all possible within in reason)

DJMcNab · 2024-07-19T07:14:15Z

xilem/src/view/flex.rs

+                    this.rebuild(prev, view_state.inner.as_mut().unwrap(), ctx, element)
+                }),
+            (AnyFlexChild::Spacer(prev), AnyFlexChild::Spacer(this)) => {
+                View::<(), (), ViewCtx>::rebuild(this, prev, &mut (), ctx, element)


We should have a comment at the spacer view that it is an invariant that this view doesn't deal with messages.

DJMcNab · 2024-07-19T07:19:50Z

xilem/src/view/flex.rs

-    scratch: AppendVec<Pod<Box<dyn Widget>>>,
+pub enum FlexElement {
+    // Avoid making the enum massive for the spacer cases by boxing
+    Child(Box<Pod<Box<dyn Widget>>>, FlexParams),


I think FlexElement is only transitory, so it doesn't matter if the spacer case is huge.
That is, we're never storing a Vec<FlexElement> for very long.

Well actually I had it before without double boxing, this is your code from #235, but I'm also fine without it.

DJMcNab · 2024-07-19T07:22:55Z

xilem/src/view/flex.rs

+    }
+}
+
+pub struct FlexItem<V, State, Action> {


Yeah, this is a choice we have to make, and I think adding them to the View type is the right one, to keep the type inference failures local.

xilem/examples/mason.rs

DJMcNab · 2024-07-19T07:26:04Z

xilem/examples/mason.rs

        .collect::<Vec<_>>();
+
+    let flexy_flex_sequence = [2, 3, 4].map(|c| {


Yeah - it's fine to stick anything in here 😆

DJMcNab · 2024-07-19T07:27:07Z

xilem/examples/mason.rs

+    let flexy_flex_sequence = [2, 3, 4].map(|c| {
+        if data.count.abs() % c == 0 {
+            FlexSpacer::Fixed(30.0 * c as f64).into_any_flex()
+        } else {
+            button(format!("flexy +{c}"), move |data: &mut AppData| {
+                data.count += c;
+            })
+            .into_any_flex()
+        }
+    });


This feels FizzBuzz adjacent - not sure that we should action that though

Ah yeah, lets make it FizzBuzz ^^

Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>

…e boxing

See https://developer.mozilla.org/en-US/docs/Web/CSS/gap Since #428, we no longer have default spacing between flex elements in Masonry. This makes the examples quite ugly. Choices made based on [#xilem > Porting Taffy Integration to New Xilem](https://xi.zulipchat.com/#narrow/stream/354396-xilem/topic/Porting.20Taffy.20Integration.20to.20New.20Xilem). Of particular note is the choice to not have this be overwritten by spacers, due to: > My first thought is that users are going to be very confused when they add a space between two items and the result is the items are closer together. There is no such concept of a spacer in CSS parlance

DJMcNab · 2024-08-09T16:30:15Z

masonry/src/widget/flex.rs

+        params: impl Into<FlexParams>,
+    ) {
+        let p = params.into();
+        tracing::info!("{:?}", p);


This needs to be removed.

Add a FlexElement to support explicit spacers for Flex in xilem

49cfe7f

Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>

Philipp-M commented Jul 15, 2024

View reviewed changes

fix typo from druid sync

76a25c1

Philipp-M commented Jul 15, 2024

View reviewed changes

Philipp-M mentioned this pull request Jul 16, 2024

Integrate tokio for async communication with Xilem #423

Merged

Make flex work without inferior mutability

56b288b

Philipp-M commented Jul 16, 2024

View reviewed changes

Correct and move comment

ec9abbe

DJMcNab approved these changes Jul 17, 2024

View reviewed changes

Finish sentence

432adef

PoignardAzur approved these changes Jul 17, 2024

View reviewed changes

Philipp-M added 2 commits July 19, 2024 00:12

Add an AnyFlexChild view, which is a widget-type erased flex child,…

3bd2be4

… that can either be a spacer or a flex item, also documented a lot of the flex implementation

Provide doc for WidgetView::boxed

cf9f5b1

Philipp-M commented Jul 18, 2024

View reviewed changes

Philipp-M requested a review from DJMcNab July 18, 2024 22:42

Philipp-M added 3 commits July 19, 2024 00:55

Merge remote-tracking branch 'upstream/main' into xilem_flex-element

ddec58c

Fix typos

202fd27

Remove clippy reason, as it seems to be experimental rustc feature

7e1e236

DJMcNab approved these changes Jul 19, 2024

View reviewed changes

Philipp-M and others added 2 commits July 19, 2024 09:39

Apply suggestions from code review

e2aa0bd

Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>

Make the any flex seq in the mason example FizzBuzz, and remove doubl…

74e541e

…e boxing

Philipp-M added this pull request to the merge queue Jul 19, 2024

Merged via the queue into linebender:main with commit 4fdcb26 Jul 19, 2024

Philipp-M deleted the xilem_flex-element branch July 19, 2024 08:07

DJMcNab mentioned this pull request Jul 19, 2024

Add a CSS gap inspired Flex property. #437

Merged

RagibHasin mentioned this pull request Aug 6, 2024

Fix non-standard textbox behavior in Windows #489

Merged

DJMcNab reviewed Aug 9, 2024

View reviewed changes

Philipp-M mentioned this pull request Aug 9, 2024

Cleanup of unnecessary tracing and "external" .flex() of num_row in the calc example #502

Merged

		.collect::<Vec<_>>();

		let flexy_flex_sequence = [2, 3, 4].map(\|c\| {

Add a FlexElement to support explicit spacers and flex parameters for Flex in xilem #428

Add a FlexElement to support explicit spacers and flex parameters for Flex in xilem #428

Uh oh!

Conversation

Philipp-M commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Philipp-M Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Philipp-M Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PoignardAzur Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Philipp-M left a comment

Choose a reason for hiding this comment

Uh oh!

DJMcNab left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PoignardAzur commented Jul 17, 2024

Uh oh!

PoignardAzur commented Jul 17, 2024

Uh oh!

DJMcNab commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PoignardAzur commented Jul 18, 2024

Uh oh!

Philipp-M commented Jul 18, 2024

Uh oh!

PoignardAzur commented Jul 18, 2024

Uh oh!

Philipp-M left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DJMcNab left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Add a `FlexElement` to support explicit spacers and flex parameters for `Flex` in xilem #428

Add a `FlexElement` to support explicit spacers and flex parameters for `Flex` in xilem #428

Philipp-M commented Jul 15, 2024 •

edited

Loading

Philipp-M Jul 15, 2024 •

edited

Loading

Philipp-M Jul 15, 2024 •

edited

Loading

PoignardAzur Jul 16, 2024 •

edited

Loading

DJMcNab commented Jul 17, 2024 •

edited

Loading