-
Notifications
You must be signed in to change notification settings - Fork 175
masonry: Refactor local transform as Property
#1374
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
base: main
Are you sure you want to change the base?
Conversation
| child_state.transform_changed = true; | ||
| changed_properties.mark_as_changed::<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.
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).
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 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?
DJMcNab
left a comment
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'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`] |
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.
| /// 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 |
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.
| /// 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 |
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.
| /// The local transform of a widget | |
| /// The local transform of a widget. |
| child_state.transform_changed = true; | ||
| changed_properties.mark_as_changed::<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 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?
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
GlobalPropertywithimpl<P: GlobalProperty, W: Widget> HasProperty<P> for W {}as suggested in #1278And a core property
Transformwhich is a new type wrapper aroundkurbo::Affine, it could reasonably also be a directlyAffine(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):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.