`xilem_masonry`: Create a generic `View` trait, and use it to complete the `Flex` view #235

DJMcNab · 2024-04-30T15:06:55Z

Functionally, this code works, but there's a lot of complexity around the different traits which have been added, which is unfortunate.

That is, the ergonomics are good, but that comes at the cost of some how understandable it is.

DJMcNab · 2024-05-01T20:57:05Z

Marking as draft because the distribution of "responsibilities" between the traits isn't very clear.

I still think this pattern is viable, but the implementation is really messy

Philipp-M

but that comes at the cost of some how understandable it is.

Yep, took me some time to dive through this...
Pretty clever stuff, neat trick to do this with the SequenceCompatible trait.

What I think though, is that we should document this thoroughly (at some time, not necessarily in this PR), to avoid scaring off new Contributors with that added complexity. But what I like, is that the added complexity is mostly internal at the core, so I don't think it's necessary to have a deep understanding of all of this to just add simple stuff (widgets etc.), unless a completely new ViewSequence<SpliceElement> context is necessary (like FlexElement).

I guess we need the differentiation between the traits MasonryView and View because of FlexElement?
(I can see some use cases with having a "generic" View though anyway, maybe we somehow are able to marry/merge xilem_web with xilem_masonry with that at some point?)

One thing I'm wondering is, how much compilation time this adds (for reasonbly large view logic functions), because of added associated trait bounds (which seem to be the main culprit for added compilation time).

(Btw. related, but I prefer this, i.e. strong-typed at the expense of complexity as long as it only stays (mostly) inside of xilems implementation over dynamic typing.)

I'm cautiously approving this, but I think it's a good idea to discuss this tomorrow before merging.

Philipp-M · 2024-05-01T21:04:11Z

crates/masonry/src/widget/flex.rs

+        };
+        let new_child = new_flex_child(params.into(), widget);
+        *child = new_child;
+        // TODO


Are these TODO comments, a result of copy paste, or is there really something todo? (and what?)

Btw. I think the two lines below this, could be summed up with

self.ctx.children_changed();

couldn't it?

Philipp-M · 2024-05-01T21:04:34Z

crates/masonry/src/widget/widget_mut.rs

+}
+
+impl<'a, W: Widget + StoreInWidgetMut> WidgetMut<'a, W> {
+    pub(crate) fn new(parent_widget_state: &'a mut WidgetState, inner: W::Mut<'a>) -> Self {


I think all of this unsafe code is only "safe" because it's only possible to construct this type within masonry isn't it?

Not super comfortable with this, because it's... well unsafe as well as (slightly) complex (not the best combination). Not sure whether all of that could be achieved with just some reference counting.

Philipp-M · 2024-05-01T21:04:53Z

crates/xilem_masonry/src/lib.rs

+
+pub trait ViewElement {
+    type Mut<'a>;
+    type Erased: ViewElement;


Would be good to at least add some basic documentation to this trait. E.g. that this is type-erased and not necessarily a deleted element.

Philipp-M · 2024-05-01T21:05:16Z

crates/xilem_masonry/src/sequence.rs

+    ) -> R;
+}
+
+impl<SourceElement: ViewElement, SequenceElement: ViewElement> SequenceCompatible<SourceElement>


Maybe put all the bounds into the where clause (for ease of understanding this)? Or is this intended?

Philipp-M · 2024-05-01T21:05:21Z

crates/xilem_masonry/src/sequence.rs

+    }
+}
+
+pub struct TrivialWrapper<T>(T);


I don't see any use of this, is this a remnant from previous experiments?

Philipp-M · 2024-05-01T21:05:31Z

crates/xilem_masonry/src/sequence.rs

@@ -113,11 +176,16 @@ pub struct OptionSeqState<InnerState> {
 }

 // TODO(DJMcNab): Add the generation to the id path


Not related to this PR, but I don't think that comment is necessary anymore?

Philipp-M · 2024-05-01T21:05:44Z

crates/xilem_masonry/src/view/flex.rs

+use crate::{
+    sequence::SequenceCompatible, ChangeFlags, ElementSplice, MasonryView, VecSplice, View,
+    ViewElement, ViewSequence,
+};

 // TODO: Allow configuring flex properties. I think this actually needs its own view trait?
 pub fn flex<VT, Marker>(sequence: VT) -> Flex<VT, Marker> {


(not directly related to that PR)

I'm not sure whether we actually should add the type bounds here (generally for these "component" functions).

I think for one it helps a little bit with error messages, and since I think it should only be used inside a View context, it also shows, what is actually allowed here as sequence. (But well, unfortunately it adds a lot of generic parameters obviously)

Philipp-M · 2024-05-01T21:05:50Z

crates/xilem_masonry/src/view/flex.rs

@@ -16,6 +19,13 @@ pub fn flex<VT, Marker>(sequence: VT) -> Flex<VT, Marker> {
    }
 }

+pub fn flex_item<V>(view: V, params: impl Into<FlexParams>) -> FlexWrapper<V> {


More of a UX thing and not entirely relevant to that PR, we could also achieve this via an extension trait. So that this is possible via the builder pattern (maybe also within MasonryView, to avoid extra traits? (see also e.g. here))

Continues linebender#235

DJMcNab · 2024-05-22T15:38:19Z

This track of work is continuing in #310

Continues linebender#235

…#310) This: 1) Renames the current/old `xilem_core` to `xilem_web_core` and moves it to the `xilem_web/xilem_web_core` folder 2) Creates a new `xilem_core`, which does not use (non-tuple) macros and instead contains a `View` trait which is generic over the `Context` type 3) Ports `xilem` to this `xilem_core`, but with some functionality missing (namely a few of the extra views; I expect these to straightforward to port) 4) Ports the `mason` and `mason_android` examples to this new `xilem`, with less functionality. This continues ideas first explored in #235 The advantages of this new View trait are: 1) Improved support for ad-hoc views, such as views with additional attributes. This will be very useful for layout algorithms, and will also enable native *good* multi-window (and potentially menus?) 2) A lack of macros, to better enable using go-to-definition and other IDE features on the traits Possible disadvantages: 1) There are a few more traits to enable the flexibility 2) It can be less clear what `Self::Element::Mut` is in the `rebuild` function, because of how the resolution works 3) When implementing `View`, you need to specify the context (i.e. `impl<State, Action> View<State, Action, [new] ViewCtx> for Button<State, Action>`. --------- Co-authored-by: Philipp Mildenberger <philipp@mildenberger.me>

…or `Flex` in xilem (#428) 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>

DJMcNab added 7 commits April 29, 2024 17:33

Make the Masonry view trait generic

37fb8ee

Allow WidgetMut to act much more like an exclusive reference type

9bb54ca

Make the ViewSequence trait sufficiently generic

8ce312c

Wire up flex with ViewSequence

1df9ad7

Add the spacer view

e764bc0

Integrate setting flex properties

a7b31b7

Address clippy failures

374cc5d

DJMcNab requested a review from Philipp-M April 30, 2024 15:36

DJMcNab marked this pull request as draft May 1, 2024 20:57

Philipp-M approved these changes May 1, 2024

View reviewed changes

DJMcNab mentioned this pull request May 18, 2024

xilem: Add Memoization views (Memoize and Arc<impl View>) #267

Merged

DJMcNab added a commit to DJMcNab/xilem that referenced this pull request May 20, 2024

Investigation into a more generic view trait

d405dd4

Continues linebender#235

DJMcNab mentioned this pull request May 20, 2024

Move xilem onto a new xilem_core, which uses a generic View trait #310

Merged

DJMcNab closed this May 22, 2024

DJMcNab deleted the consequential branch May 22, 2024 15:38

DJMcNab added a commit to DJMcNab/xilem that referenced this pull request May 23, 2024

Investigation into a more generic view trait

fa6db98

Continues linebender#235

DJMcNab added a commit to DJMcNab/xilem that referenced this pull request May 24, 2024

Investigation into a more generic view trait

36941f7

Continues linebender#235

DJMcNab added a commit to DJMcNab/xilem that referenced this pull request Jun 3, 2024

Investigation into a more generic view trait

39f1bc5

Continues linebender#235

Philipp-M mentioned this pull request Jul 15, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

`xilem_masonry`: Create a generic `View` trait, and use it to complete the `Flex` view #235

`xilem_masonry`: Create a generic `View` trait, and use it to complete the `Flex` view #235

Uh oh!

DJMcNab commented Apr 30, 2024

Uh oh!

DJMcNab commented May 1, 2024 •

edited

Loading

Uh oh!

Philipp-M left a comment

Uh oh!

Philipp-M May 1, 2024

Uh oh!

Philipp-M May 1, 2024

Uh oh!

Philipp-M May 1, 2024

Uh oh!

Philipp-M May 1, 2024

Uh oh!

Philipp-M May 1, 2024

Uh oh!

Philipp-M May 1, 2024

Uh oh!

Philipp-M May 1, 2024

Uh oh!

Philipp-M May 1, 2024

Uh oh!

DJMcNab commented May 22, 2024

Uh oh!

Uh oh!

		@@ -113,11 +176,16 @@ pub struct OptionSeqState<InnerState> {
		}

		// TODO(DJMcNab): Add the generation to the id path

xilem_masonry: Create a generic View trait, and use it to complete the Flex view #235

xilem_masonry: Create a generic View trait, and use it to complete the Flex view #235

Uh oh!

Conversation

DJMcNab commented Apr 30, 2024

Uh oh!

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

Uh oh!

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!

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 commented May 22, 2024

Uh oh!

Uh oh!

`xilem_masonry`: Create a generic `View` trait, and use it to complete the `Flex` view #235

`xilem_masonry`: Create a generic `View` trait, and use it to complete the `Flex` view #235

DJMcNab commented May 1, 2024 •

edited

Loading