Skip to content

Conversation

@Philipp-M
Copy link
Member

This got a little bit larger than I would've hoped, and I'm not entirely sure about it (more code without immediate benefit, apart from making the transform a property, which possibly allows other APIs e.g. in Xilem).

It introduces a new marker trait GlobalProperty with impl<P: GlobalProperty, W: Widget> HasProperty<P> for W {} as suggested in #1278
And a core property Transform which is a new type wrapper around kurbo::Affine, it could reasonably also be a directly Affine (less boilerplate for sure), but I guess a new type probably is worth it non-the-less.

This unfortunately disallows other things I had in mind, due to orphan rules (this would result in a similar behavior as xilem_webs OneOf):

impl<
    A: Widget + FromDynWidget + HasProperty<P> + ?Sized,
    B: Widget + FromDynWidget + HasProperty<P> + ?Sized,
    C: Widget + FromDynWidget + HasProperty<P> + ?Sized,
    D: Widget + FromDynWidget + HasProperty<P> + ?Sized,
    E: Widget + FromDynWidget + HasProperty<P> + ?Sized,
    F: Widget + FromDynWidget + HasProperty<P> + ?Sized,
    G: Widget + FromDynWidget + HasProperty<P> + ?Sized,
    H: Widget + FromDynWidget + HasProperty<P> + ?Sized,
    I: Widget + FromDynWidget + HasProperty<P> + ?Sized,
    P: Property,
> HasProperty<P> for OneOfWidget<A, B, C, D, E, F, G, H, I>
{
}

So another case why I'm not sure about merging this. But as it's basically finished, it's at the very least worth opening this.

Comment on lines -215 to +222
child_state.transform_changed = true;
changed_properties.mark_as_changed::<Transform>();
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure whether it's the right thing to change this. Alternatively we could leave the transform_changed flag within WidgetState additionally, and check both in the compose pass (or set transform_changed as well in WidgetMut::property_changed).

Copy link
Member

Choose a reason for hiding this comment

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

I think this alternative is the better way around. My understanding is that the semantics of prop_has_changed are very clear, which is that they mark whether the transform has changed during the lifetime of this WidgetMut, right?

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

I'm finding this a bit hard to reason about.

}
}

/// When properties of a widget were modified this should also be reflected by [`ChangedProperties::mark_as_changed`]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// When properties of a widget were modified this should also be reflected by [`ChangedProperties::mark_as_changed`]
/// When properties of a widget were modified this should also be reflected by [`ChangedProperties::mark_as_changed`].

}

impl ChangedProperties {
/// Mark that the property `P` has changed
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Mark that the property `P` has changed
/// Mark that the property `P` has changed.

etc.


use crate::core::{GlobalProperty, Property};

/// The local transform of a widget
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// The local transform of a widget
/// The local transform of a widget.

Comment on lines -215 to +222
child_state.transform_changed = true;
changed_properties.mark_as_changed::<Transform>();
Copy link
Member

Choose a reason for hiding this comment

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

I think this alternative is the better way around. My understanding is that the semantics of prop_has_changed are very clear, which is that they mark whether the transform has changed during the lifetime of this WidgetMut, right?

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.

2 participants