Add Board widget for absolute positioning #591

RagibHasin · 2024-09-16T10:32:44Z

Adds a Board widget, view and a corresponding Xilem example.
Also, adds a Shape widget for drawing all built-in Kurbo shapes.

RagibHasin · 2024-09-16T12:03:20Z

@waywardmonkeys , it may interest you.
@DJMcNab , if you would have a look at it.

Philipp-M · 2024-09-16T13:27:13Z

I think we want a more xilem_svg like API for these kinds of use-cases (that is on the roadmap as well). It's something I have planned to do (but have not yet found time for it).

See e.g. the xilem_web svgtoy example or the stroke-expansion-demo.
I believe we want to be close to the semantics as SVG.

So if my skim of that code is right, the Board widget/view would be similar to a simplified <svg>.

As Daniel wrote on zulip, I do believe as well, that we want to have some kind of <foreignObject> for things breaking out of the absolute positioned layout (I believe that this purpose fullfills BoardViewItem via .positioned() currently).

FYI to avoid Rect::new().view() there's an OrphanView trait. See here how to use this (for this exact use-case :))
It's not yet clear whether we want to have a big-struct for these kinds of views to compose attributes as done in this PR with ShapeItem, or composing types such as in xilem_web like the xilem_web::svg::common_attrs::{Stroke, Fill} views.
I slightly lean to that, as this is likely more efficient, and allows for more functional flexibility (e.g. in xilem_web something like frozen(|| Rect::new(..)).fill(Color::BLUE) is possible) and potentially less boilerplate.

RagibHasin · 2024-09-16T13:55:06Z

Thanks! Didn't know about OrphanView. I would definitely look into it.

You're right, it is heavily inspired by Xilem Web's SVG views. But I cannot see how I can make strokes and fills into a wrapper view without a large boilerplate which would be needed anyway. Although I never used frozen from Xilem Web, so have to look into it.

Philipp-M · 2024-09-16T14:13:00Z

the Frozen view is a xilem_core view so both available in xilem (it's e.g. shown in the memoization example) as well as xilem_web. It's basically Memoize without data (or data == ()).

Yeah in case of multiple view impls, that itself is somewhat boilerplate indeed, but I do believe trivial enough to live with it or a case for (proc-)macros (but I don't think that's worth it for now at least, I guess it depends on how similar that boilerplate is).

RagibHasin · 2024-09-16T15:10:03Z

After mulling it over, I think I now understand how stroke and fill can be attribute-like views. I will keep you posted on how it goes.

PoignardAzur

Masonry part looks good to me. I'm less confident about the Xilem part.

masonry/src/widget/board.rs

Philipp-M

I wonder, whether we should have a separate View derived trait constraining the context.

Something like (I renamed BoardElement to a more general VectorElement, maybe SvgElement/SvgView makes also sense):

trait VectorView<State, Action>: View<State, Action, ViewCtx, Element = Pod<Self::VectorElement> + Send + Sync {
    type VectorElement: VectorElement;
}

I'm also not yet sure if an enum will suffice in the future, I would think separate types make sense here, to be more flexible.

For the widget container roughly either something like struct PositionedElement(impl Widget, Size, Position) or struct ForeignElement(impl Widget, Size, Position). And then have separate types for the other vector elements, like Shape if it's reasonably possible to cover all kurbo shapes with this type, and something like a VectorGroup etc.

Having a separate types for vector elements have the advantage to allow specialized views on top of some of these with the type system and be more extensible also outside of xilem/masonry.

regarding the attributes something like this would be possible then:

trait VectorExt<State, Action>: VectorView<State, Action> {
    fn stroke(self,...) -> Stroke<Self, State, Action>
    where
        Self::Element: WithStroke
    {
    }
}

where relevant elements would have this trait implemented (in this case impl WithStroke for Shape).
(This is also hinting the extension traits which I think are planned in masonry.)

We could have a separate trait for the vector elements/context, something like:

trait VectorElement: Widget {
    fn origin(&self) -> Point; // relative to parents transform
    fn size(&self) -> Size;
    // maybe also setters
}

to constrain elements, with that we don't need the enum Child in masonry, and all the duplicated logic there.

masonry/src/widget/board.rs

RagibHasin · 2024-09-17T06:05:03Z

@PoignardAzur is it possible to get a mutable reference of a widget from a Pod<W in View::build?

PoignardAzur · 2024-09-17T08:44:29Z

I don't think so, but I'm a bit hazy on Xilem's logic.

masonry/src/widget/board.rs

RagibHasin · 2024-09-17T09:50:44Z

I don't think so, but I'm a bit hazy on Xilem's logic.

Is there a reason for it? Because I think a view should be able to modify its children upon build. Otherwise, things are getting very complicated.

Kurbo shape views and their attribute views would then need a custom pod type, which somehow does not satisfy Board: WidgetView.

Anyway, I think it would help to add a method in xilem::Pod for getting a mutable reference to the underlying widget with reference to a ViewCtx. Would that be acceptable? It would need additional plumbing in Masonry too.

Philipp-M · 2024-09-17T10:29:44Z

Because I think a view should be able to modify its children upon build. Otherwise, things are getting very complicated.

Another case for the extension traits? AFAIK only WidgetRef<T> should not be able to mutate the widget.

I.e. something like trait ShapeMut (or separate traits for single properties as described in my previous comment) with all the methods currently in impl<'a> WidgetMut<'a, Shape> {} which is implemented for WidgetMut<'a, Shape>, WidgetPod<Shape> and Shape itself. Maybe we could start with this here? (And do a pass for the other widgets/properties later).

Anyway, I think it would help to add a method in xilem::Pod for getting a mutable reference to the underlying widget with reference to a ViewCtx.

A DerefMut and Deref to WidgetPod makes sense, I don't see though why it needs ViewCtx? It's not dependent on the ViewCtx AFAICS?

DJMcNab · 2024-09-17T10:39:07Z

This is essentially running into pass specification being half-completed. Our current plan is to make WidgetView::build return an OrphanPod, where getting exclusive access would hopefully be relatively trivial.

At a type-system level, Masonry doesn't know that the WidgetPod actually still contains the Widget. A short-term hacked-in method to allow that access would make sense to me.

RagibHasin · 2024-09-17T11:17:45Z

A DerefMut and Deref to WidgetPod makes sense, I don't see though why it needs ViewCtx? It's not dependent on the ViewCtx AFAICS?

An Inserted WidgetPod requires additional context to resolve into its target widget as essentially it becomes a key to a slot in an arena (Olivier's language, not mine).

But anyway, in a View::build the WidgetPod has yet to be inserted and thus contains the widget inlined, so in thar case it would be possible to do without any ViewCtx and return a Option<&mut impl Widget>.

RagibHasin · 2024-09-17T11:18:25Z

A short-term hacked-in method to allow that access would make sense to me.

I would go that route then.

Philipp-M · 2024-09-17T11:41:15Z

An Inserted WidgetPod requires additional context to resolve into its target widget as essentially it becomes a key to a slot in an arena (Olivier's language, not mine).

Yes, but not with the ViewCtx that doesn't have the widget arena to access the widget in case it's Inserted as described by Daniel, so yeah probably just panic!, when it's Inserted?

RagibHasin · 2024-09-17T11:53:30Z

An Inserted WidgetPod requires additional context to resolve into its target widget as essentially it becomes a key to a slot in an arena (Olivier's language, not mine).

Yes, but not with the ViewCtx that doesn't have the widget arena to access the widget in case it's Inserted as described by Daniel, so yeah probably just panic!, when it's Inserted?

I want to leave panicking to the caller, which if used right, should never happen. Otherwise, some unwitting caller may call get mut on WidgetPod, and crash the program in some other place.

I actually wanted a reference to a ViewCtx as a sanity check for that purpose, but now think that we would still need to check for availability as a ViewCtx is available in all view methods.

RagibHasin · 2024-09-17T17:36:46Z

I wonder, whether we should have a separate View derived trait constraining the context.

Thanks, @Philipp-M . This solution is far more elegant.

Philipp-M

Yeah that's much closer to what I've imagined for a xilem_svg API :)

Added a few suggestions.

Philipp-M · 2024-09-17T19:25:32Z

xilem/src/view/board/style_modifier.rs

+impl<State: 'static, Action: 'static> GraphicsExt<State, Action> for kurbo::PathSeg {}
+impl<State: 'static, Action: 'static> GraphicsExt<State, Action> for kurbo::Arc {}
+impl<State: 'static, Action: 'static> GraphicsExt<State, Action> for kurbo::BezPath {}
+impl<State: 'static, Action: 'static> GraphicsExt<State, Action> for kurbo::Circle {}
+impl<State: 'static, Action: 'static> GraphicsExt<State, Action> for kurbo::CircleSegment {}
+impl<State: 'static, Action: 'static> GraphicsExt<State, Action> for kurbo::CubicBez {}
+impl<State: 'static, Action: 'static> GraphicsExt<State, Action> for kurbo::Ellipse {}
+impl<State: 'static, Action: 'static> GraphicsExt<State, Action> for kurbo::Line {}
+impl<State: 'static, Action: 'static> GraphicsExt<State, Action> for kurbo::QuadBez {}
+impl<State: 'static, Action: 'static> GraphicsExt<State, Action> for kurbo::Rect {}
+impl<State: 'static, Action: 'static> GraphicsExt<State, Action> for kurbo::RoundedRect {}
+
+impl<State: 'static, Action: 'static, V: GraphicsView<State, Action>> GraphicsExt<State, Action>
+    for Transform<V, State, Action>
+{
+}
+impl<State: 'static, Action: 'static, V: GraphicsView<State, Action>> GraphicsExt<State, Action>
+    for Fill<V, State, Action>
+{
+}
+impl<State: 'static, Action: 'static, V: GraphicsView<State, Action>> GraphicsExt<State, Action>
+    for Stroke<V, State, Action>
+{
+}


This can be achieved generally with a blanket impl:

Suggested change

impl<State: 'static, Action: 'static> GraphicsExt<State, Action> for kurbo::PathSeg {}

impl<State: 'static, Action: 'static> GraphicsExt<State, Action> for kurbo::Arc {}

impl<State: 'static, Action: 'static> GraphicsExt<State, Action> for kurbo::BezPath {}

impl<State: 'static, Action: 'static> GraphicsExt<State, Action> for kurbo::Circle {}

impl<State: 'static, Action: 'static> GraphicsExt<State, Action> for kurbo::CircleSegment {}

impl<State: 'static, Action: 'static> GraphicsExt<State, Action> for kurbo::CubicBez {}

impl<State: 'static, Action: 'static> GraphicsExt<State, Action> for kurbo::Ellipse {}

impl<State: 'static, Action: 'static> GraphicsExt<State, Action> for kurbo::Line {}

impl<State: 'static, Action: 'static> GraphicsExt<State, Action> for kurbo::QuadBez {}

impl<State: 'static, Action: 'static> GraphicsExt<State, Action> for kurbo::Rect {}

impl<State: 'static, Action: 'static> GraphicsExt<State, Action> for kurbo::RoundedRect {}

impl<State: 'static, Action: 'static, V: GraphicsView<State, Action>> GraphicsExt<State, Action>

for Transform<V, State, Action>

{

}

impl<State: 'static, Action: 'static, V: GraphicsView<State, Action>> GraphicsExt<State, Action>

for Fill<V, State, Action>

{

}

impl<State: 'static, Action: 'static, V: GraphicsView<State, Action>> GraphicsExt<State, Action>

for Stroke<V, State, Action>

{

}

impl<State: 'static, Action: 'static, V: GraphicsView<State, Action>> GraphicsExt<State, Action>

for V

{

}

Which also has the advantage to work with Memoize etc.

xilem/src/view/board/kurbo_shape.rs

Philipp-M · 2024-09-17T21:25:09Z

xilem/src/view/board/style_modifier.rs

+        if self.transform != prev.transform {
+            element.set_transform(self.transform);


I believe we want to check if the child views have changed the transform (and similar for other properties) as well, and overwrite it in case.
That's one reason why there's the rather complicated modifier logic in xilem_web so that this does not lead to surprising results (i.e. the most outer modifier always overwrites previous modifiers of the same type).

I think the simplest way is just something like:

Suggested change

if self.transform != prev.transform {

element.set_transform(self.transform);

if self.transform != element.transform {

element.set_transform(self.transform);

But that may have the disadvantage of producing unnecessary repaints, when there's other modifiers that transform that property.

We could also use dirty flags etc. to avoid that issue, something like if self.transform != prev.transform || element.transform_dirty() {}, or something similar.

How can the properties be changed other than by the parent?

Consider the following:

Rect::new().transform(inner).transform(outer)

when inner changes, the new value of this will be the new element value unless outer changes as well at the same time, while I would expect, that outer overwrites this value regardless.

RagibHasin · 2024-09-18T06:12:05Z

I think it is now reasonably complete for a full review.

Philipp-M

Mostly looks good, there's a few suggestions on the xilem side.

Philipp-M · 2024-09-18T17:48:22Z

xilem/src/view/board/kurbo_shape.rs

+
+use crate::{Pod, ViewCtx, WidgetView};
+
+pub trait GraphicsView<State, Action = ()>: WidgetView<State, Action, Widget: SvgElement> {}


Maybe it makes sense to restructure the xilem side a little bit as well.
I do think, since this is kind of the start of the native xilem_svg implementation already, that it would make sense to put this trait in a module xilem::view::svg (I still lean towards SvgView or VectorView as name for this trait).

And every view that implements this trait, as a submodule (which is not just these kurbo shapes, but also the modifiers as well as PositionedView etc).
The modifiers can be summed together as they are right now or put in separate submodules, I don't have a strong opinion here...

Philipp-M · 2024-09-18T18:23:16Z

xilem/src/view/board.rs

+pub type AnyBoardView<State, Action = ()> =
+    dyn AnyView<State, Action, ViewCtx, BoardElement> + Send + Sync;
+
+pub struct BoardElement {


I don't think this extra type is necessary.
I think you can just use Pod<W: SvgElement>.
And the AnyView could be (see below for more info)

pub type AnySvgView<State, Action = ()> = dyn AnyView<State, Action, ViewCtx, Pod<DynWidget<Box<dyn SvgElement>>>> + Send + Sync;

Philipp-M · 2024-09-18T18:23:19Z

xilem/src/view/board.rs

+pub struct BoardElementMut<'w> {
+    parent: WidgetMut<'w, widget::Board>,
+    idx: usize,
+}


And I think this could be removed as well, since I think we can just use the ViewElement implementation in the top of xilem of Pod<W>.

I believe we need something like: impl<W: SvgElement> SuperElement<Pod<W>> for Pod<Box<dyn SvgElement>> then instead.

(It could well be that this needs some plumbing on the masonry side, in particular something like WidgetPod<W: SvgElement>::svg_boxed())

The index here is already in the BoardSplice, the only other place I see where this is necessary, is for the AnyElement impl. But I think we can reuse the slightly modified DynWidget, roughly like this:

pub struct DynWidget<W = Box<dyn Widget>> { inner: WidgetPod<W>, }

with impl SvgElement for DynWidget<Box<dyn SvgElement>>, which would make all of this more generally useful.

This would make a lot of code below unnecessary too.

Philipp-M · 2024-09-18T18:23:44Z

xilem/src/view/board.rs

+pub trait BoardSequence<State, Action = ()>:
+    ViewSequence<State, Action, ViewCtx, BoardElement>


Same here, this could be with the suggestions above:

Suggested change

pub trait BoardSequence<State, Action = ()>:

ViewSequence<State, Action, ViewCtx, BoardElement>

pub trait SvgSequence<State, Action = ()>:

ViewSequence<State, Action, ViewCtx, Pod<Box<dyn SvgElement>>>

Philipp-M · 2024-09-18T18:25:02Z

xilem/src/view/board/style_modifier.rs

+    }
+}
+
+pub trait GraphicsExt<State, Action>: GraphicsView<State, Action> + Sized {


I think this is not accurate anymore, since e.g. a PositionedView is a GraphicsView as well.

So I think there are two ways to solve this and I don't have a strong opinion here either, IME the first option plays better with rust-analyzer though:

Separate modifier traits with a super element bound on a class of elements or one element, so something like this (similar as xilem_web::interfaces::*):

pub trait KurboShapeExt<State, Action>: GraphicsView<State, Action, Widget = KurboShape> + Sized {...}

As Olivier called it "Warehouse" traits, so something like:

pub trait SvgExt<State, Action>: GraphicsView<State, Action> + Sized { fn transform(self, affine: Affine) -> Transform<Self, State, Action> where // potentially in the future some kind of transform trait bound, maybe it's also general enough to be directly in the `SvgElement` trait Self::Element = KurboShape { transform(self, affine) } ... }

Philipp-M · 2024-09-18T18:25:48Z

xilem/src/view/board/style_modifier.rs

+        if self.transform != prev.transform {
+            element.set_transform(self.transform);


Consider the following:

Rect::new().transform(inner).transform(outer)

when inner changes, the new value of this will be the new element value unless outer changes as well at the same time, while I would expect, that outer overwrites this value regardless.

RagibHasin · 2024-09-19T00:35:51Z

Thanks for the guidance in bringing this so far @Philipp-M , but I have to step away from this for a few days.
In the meantime, feel free to push into this branch if you would like to drive this PR forward.

I would be back in two weeks in-sha-Allah.

PoignardAzur

Overall, I'm worried about the amount of code in this PR which does nothing but delegate to an internal implementation. It's a major code smell, and a hint that you're using too many layers of abstractions in your design.

PoignardAzur · 2024-09-19T17:14:14Z

masonry/src/widget/board.rs

+/// A trait representing a widget which knows its origin and size.
+pub trait SvgElement: Widget {
+    // Origin of the widget relative to its parent
+    fn origin(&self) -> Point;
+    // Size of the widget
+    fn size(&self) -> Size;
+
+    // Sets the origin of the widget relative to its parent
+    fn set_origin(&mut self, origin: Point);
+    // Sets the size of the widget
+    fn set_size(&mut self, size: Size);
+}


This trait strikes me as a symptom of trying way too hard to be clever in your design. Especially given that some implementations of the trait panic in some methods.

I think you could go pretty far by going a simpler route:

No SvgElement trait.

Both widgets and shapes are stored inside PositionedElement.

PositionedElement doesn't implement Widget.

The default way to add a shape is to add a PositionedElement with a KurboShape widget, an origin of shape.bounding_box().origin() and a size of shape.bounding_box().size().

This trait strikes me as a symptom of trying way too hard to be clever in your design.

That's mostly on me, the reason why I was pushing towards this, is to have strongly typed "border", to enforce only the "specialized" class of "svg" widgets within this kind of context (i.e. have a tree-hierarchy in near/mid-future).

I'd agree here, when this would only be one layer (similar as Flex etc.), but my plan with all of this is to have a completely separate (specialized) widget tree for the svg implementation.

I'll take a closer look in the following week and may use this PR as basis for a (xilem_)svg implementation.

PositionedElement doesn't implement Widget.

The way I see PositionedElement is <foreignObject>, and with all the previously said, I think it makes sense to have this implement widget as well.

PoignardAzur · 2024-09-19T17:16:40Z

masonry/src/widget/board.rs

+    fn accessibility_role(&self) -> Role {
+        Role::GenericContainer
+    }


Open question: is this the right role for this widget, or is there a more specific role for it? I guess "Canvas" doesn't really apply.

PoignardAzur · 2024-09-19T17:19:47Z

masonry/src/widget/kurbo.rs

+    fn set_origin(&mut self, _: Point) {
+        panic!("a shape does not support setting its origin after creation")
+    }
+
+    fn set_size(&mut self, _: Size) {
+        panic!("a shape does not support setting its size after creation")
+    }


It bears mentioning again: the fact that these trait methods panic is a code smell. To be clear, I'm not saying they should be rewritten to not panic. Rather, this code is a hint that the broader design should be reconsidered.

PoignardAzur · 2024-09-19T17:21:59Z

masonry/src/widget/kurbo.rs

+    fn layout(&mut self, _ctx: &mut LayoutCtx, bc: &BoxConstraints) -> Size {
+        let size = self.shape.bounding_box().size();
+        if !bc.contains(size) {
+            warn!("The shape is oversized");
+        }
+        size
+    }


You probably shouldn't warn every time a widget overflows its parent.

Also, this code is valid with the Board as a parent, but I'm not sure what results it would produce with any widget type as a parent. It's not a blocking problem, though.

PoignardAzur · 2024-09-19T17:24:50Z

masonry/src/widget/kurbo.rs

+        let transform = self
+            .transform
+            .then_translate(-self.shape.bounding_box().origin().to_vec2());


Same as above.

RagibHasin added 3 commits September 16, 2024 16:28

Add Board widget for absolute positioning

aa29fa2

Fix clippy

845f856

Add test snapshots

991f3e5

PoignardAzur reviewed Sep 16, 2024

View reviewed changes

Philipp-M reviewed Sep 16, 2024

View reviewed changes

PoignardAzur reviewed Sep 17, 2024

View reviewed changes

masonry/src/widget/board.rs Outdated Show resolved Hide resolved

masonry/src/widget/board.rs Outdated Show resolved Hide resolved

Rewrite using the idea from @Philipp-M

b32e0f4

dirty sketch of SvgElement trait

6571b0a

Philipp-M reviewed Sep 17, 2024

View reviewed changes

RagibHasin added 2 commits September 18, 2024 11:15

Simplify BoardElement

4140de1

Removed AnyBoardChild

267b0f3

RagibHasin force-pushed the board branch from 60294b9 to 267b0f3 Compare September 18, 2024 06:05

Fix typos

eec3b53

Rearrange code as @PoignardAzur suggested

dcb6255

Make unboxed PositionedView possible

8726882

Philipp-M reviewed Sep 18, 2024

View reviewed changes

PoignardAzur requested changes Sep 19, 2024

View reviewed changes

		if self.transform != prev.transform {
		element.set_transform(self.transform);


		use crate::{Pod, ViewCtx, WidgetView};

		pub trait GraphicsView<State, Action = ()>: WidgetView<State, Action, Widget: SvgElement> {}

		pub trait BoardSequence<State, Action = ()>:
		ViewSequence<State, Action, ViewCtx, BoardElement>

Add Board widget for absolute positioning #591

Are you sure you want to change the base?

Add Board widget for absolute positioning #591

Conversation

RagibHasin commented Sep 16, 2024

RagibHasin commented Sep 16, 2024

Philipp-M commented Sep 16, 2024

RagibHasin commented Sep 16, 2024

Philipp-M commented Sep 16, 2024

RagibHasin commented Sep 16, 2024

PoignardAzur left a comment

Choose a reason for hiding this comment

Philipp-M left a comment

Choose a reason for hiding this comment

RagibHasin commented Sep 17, 2024

PoignardAzur commented Sep 17, 2024

RagibHasin commented Sep 17, 2024

Philipp-M commented Sep 17, 2024

DJMcNab commented Sep 17, 2024

RagibHasin commented Sep 17, 2024

RagibHasin commented Sep 17, 2024

Philipp-M commented Sep 17, 2024

RagibHasin commented Sep 17, 2024

RagibHasin commented Sep 17, 2024

Philipp-M 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

Choose a reason for hiding this comment

RagibHasin commented Sep 18, 2024

Philipp-M 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RagibHasin commented Sep 19, 2024

PoignardAzur 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment