-
Notifications
You must be signed in to change notification settings - Fork 120
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
base: main
Are you sure you want to change the base?
Conversation
@waywardmonkeys , it may interest you. |
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. So if my skim of that code is right, the As Daniel wrote on zulip, I do believe as well, that we want to have some kind of FYI to avoid |
Thanks! Didn't know about 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 |
the 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). |
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. |
There was a problem hiding this 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.
There was a problem hiding this 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.
@PoignardAzur is it possible to get a mutable reference of a widget from a |
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 Anyway, I think it would help to add a method in |
Another case for the extension traits? AFAIK only I.e. something like
A |
This is essentially running into pass specification being half-completed. Our current plan is to make At a type-system level, Masonry doesn't know that the |
An But anyway, in a |
I would go that route then. |
Yes, but not with the |
I want to leave panicking to the caller, which if used right, should never happen. Otherwise, some unwitting caller may call get mut on I actually wanted a reference to a |
Thanks, @Philipp-M . This solution is far more elegant. |
There was a problem hiding this 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.
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> | ||
{ | ||
} |
There was a problem hiding this comment.
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:
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.
if self.transform != prev.transform { | ||
element.set_transform(self.transform); |
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
I think it is now reasonably complete for a full review. |
There was a problem hiding this 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> {} |
There was a problem hiding this comment.
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...
pub type AnyBoardView<State, Action = ()> = | ||
dyn AnyView<State, Action, ViewCtx, BoardElement> + Send + Sync; | ||
|
||
pub struct BoardElement { |
There was a problem hiding this comment.
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;
pub struct BoardElementMut<'w> { | ||
parent: WidgetMut<'w, widget::Board>, | ||
idx: usize, | ||
} |
There was a problem hiding this comment.
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.
pub trait BoardSequence<State, Action = ()>: | ||
ViewSequence<State, Action, ViewCtx, BoardElement> |
There was a problem hiding this comment.
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:
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 { |
There was a problem hiding this comment.
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:
- 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)
}
...
}
if self.transform != prev.transform { | ||
element.set_transform(self.transform); |
There was a problem hiding this comment.
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.
Thanks for the guidance in bringing this so far @Philipp-M , but I have to step away from this for a few days. I would be back in two weeks in-sha-Allah. |
There was a problem hiding this 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.
/// 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); | ||
} |
There was a problem hiding this comment.
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 ofshape.bounding_box().size()
.
There was a problem hiding this comment.
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.
fn accessibility_role(&self) -> Role { | ||
Role::GenericContainer | ||
} |
There was a problem hiding this comment.
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.
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") | ||
} |
There was a problem hiding this comment.
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.
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 | ||
} |
There was a problem hiding this comment.
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.
let transform = self | ||
.transform | ||
.then_translate(-self.shape.bounding_box().origin().to_vec2()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
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 I propose that we explicitly support SVG shape elements only along with any absolutely placed regular widget (equivalent to |
See also Add Or what I mean with it, is that we may get away without introducing an extra svg context, by making this board view the But I agree, reaching consensus is a good idea. |
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? |
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. |
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. |
I think these features have been sitting in all our personal wishlists 😄.
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 How can a consensus be reached here? The large timezone difference makes it difficult for me to be active when most maintainers are. |
Adds a Board widget, view and a corresponding Xilem example.
Also, adds a Shape widget for drawing all built-in Kurbo shapes.