Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Board widget for absolute positioning #591

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

RagibHasin
Copy link
Contributor

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

@RagibHasin
Copy link
Contributor Author

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

@Philipp-M
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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.

Copy link
Contributor

@PoignardAzur PoignardAzur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

masonry/src/widget/board.rs Outdated Show resolved Hide resolved
masonry/src/widget/board.rs Outdated Show resolved Hide resolved
masonry/src/widget/board.rs Outdated Show resolved Hide resolved
masonry/src/widget/board.rs Outdated Show resolved Hide resolved
masonry/src/widget/board.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Philipp-M Philipp-M left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
masonry/src/widget/board.rs Outdated Show resolved Hide resolved
masonry/src/widget/board.rs Outdated Show resolved Hide resolved
masonry/src/widget/board.rs Outdated Show resolved Hide resolved
masonry/src/widget/board.rs Outdated Show resolved Hide resolved
@RagibHasin
Copy link
Contributor Author

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

@PoignardAzur
Copy link
Contributor

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

masonry/src/widget/board.rs Outdated Show resolved Hide resolved
masonry/src/widget/board.rs Outdated Show resolved Hide resolved
@RagibHasin
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Member

DJMcNab commented Sep 17, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

I would go that route then.

@Philipp-M
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

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

Copy link
Contributor

@Philipp-M Philipp-M left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Added a few suggestions.

Comment on lines 111 to 134
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>
{
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
Comment on lines +164 to +165
if self.transform != prev.transform {
element.set_transform(self.transform);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm this is definitely not trivial to get right (i.e. idiomatic, robust and as little extra-code as possible).

This is a dirty/hacky sketch of how to make this work, but I'm not really happy about it, as it requires boolean dirty flags for every property, and is not optimal when reading values (View::State can be used though for this issue, to cache previously set values), when considering a potential future API like:

Rect::new().rotate().translate()

I.e. that views read previously set values (in this case the transform from rotate in translate).

In xilem_web this is less of an issue, as a minimal changeset needs to be computed anyway (accessing DOM via js is expensive), so that justifies the modifier API there, which among other things accomplishes this.

So in that regard a big struct is more convenient (but requires wrapper types). I'm not sure how future-proof that is either, when considering a lot of attributes per widget (i.e. unnecessary memory usage when using the view), and to my thinking, this general pattern (composing views with new types based on trait-bounds on View::Element) should be possible.

@PoignardAzur and @DJMcNab do you have any suggestions here?

@RagibHasin
Copy link
Contributor Author

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

Copy link
Contributor

@Philipp-M Philipp-M left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Comment on lines +98 to +101
pub type AnyBoardView<State, Action = ()> =
dyn AnyView<State, Action, ViewCtx, BoardElement> + Send + Sync;

pub struct BoardElement {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Comment on lines +105 to +108
pub struct BoardElementMut<'w> {
parent: WidgetMut<'w, widget::Board>,
idx: usize,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +257 to +258
pub trait BoardSequence<State, Action = ()>:
ViewSequence<State, Action, ViewCtx, BoardElement>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

pub trait GraphicsExt<State, Action>: GraphicsView<State, Action> + Sized {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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 {...}
  1. 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)
    }
    ...
}

Comment on lines +164 to +165
if self.transform != prev.transform {
element.set_transform(self.transform);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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.

Copy link
Contributor

@PoignardAzur PoignardAzur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +37 to +48
/// 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);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +186 to +188
fn accessibility_role(&self) -> Role {
Role::GenericContainer
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +227 to +233
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")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +173 to +179
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
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +182 to +184
let transform = self
.transform
.then_translate(-self.shape.bounding_box().origin().to_vec2());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

@RagibHasin
Copy link
Contributor Author

So, I'm back and after some idle thoughts about this, I think we should reach a consensus about design first.

@PoignardAzur 's concern about unnecessary complexity involving SvgElement trait comes from @Philipp-M 's wish to replicate the paradigm of SVG more-or-less completely.
@Philipp-M also mentioned the SVG <g> element as a motivating example.
But I think a corresponding group view would result in more than one SvgElement in masonry and we should treat it as a ViewSequence. We can then have the attribute methods directly on that group view. But I think that may create problems with nested views.

I propose that we explicitly support SVG shape elements only along with any absolutely placed regular widget (equivalent to <foregnElement>) and design our code accordingly. Most else are emualable in Xilem reactively I think. Also, it would allow us to remove the SvgElement machinery.

@Philipp-M
Copy link
Contributor

See also Add Affine transform to Widget trait?.

Or what I mean with it, is that we may get away without introducing an extra svg context, by making this board view the zstack (in SwiftUI terms). I.e. that widgets just take the full width/height by default of the zstack, but can be sized individually by something like SizedBox and positioned by a transform. That may simplify everything. I'm not sure if I missed something where a dedicated SVG context would make sense otherwise.

But I agree, reaching consensus is a good idea.

@RagibHasin
Copy link
Contributor Author

Yeah, I've seen it.

My understanding is that to enable arbitrary transform for any widget, we would need to modify how paint works now. Does Masonry currently allows a parent to draw its children manually, in stead of doing in in the framework?

@RagibHasin
Copy link
Contributor Author

And the code as it sits, kind of does what you described with getting the entirety of the board to draw, translates, sized-boxing just not the other affine transforms, as I think it would require the parent to draw the child.

@Philipp-M
Copy link
Contributor

My understanding is that to enable arbitrary transform for any widget, we would need to modify how paint works now.

Not just that, it also needs to transform pointer events, filtering of it etc. When supporting a z-value in the transform (probably not in the first iteration though), it gets even more complicated. This'll likely be a more complicated change to get it right, but I think worth it.

@RagibHasin
Copy link
Contributor Author

RagibHasin commented Sep 30, 2024

I think these features have been sitting in all our personal wishlists 😄.

I think worth it.

I, too, think so.

But for the MVP here, which in my vision includes only sized-boxing and translation, I think we should remove the SvgElement abstraction but keep the type erased Child in the Board widget.

How can a consensus be reached here? The large timezone difference makes it difficult for me to be active when most maintainers are.

cc @DJMcNab @PoignardAzur

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants