Migrate layout pass #529
Migrate layout pass #529
Conversation
masonry/src/widget/widget_pod.rs
Outdated
| @@ -504,26 +505,12 @@ impl<W: Widget> WidgetPod<W> { | |||
| /// | |||
| /// [`layout`]: Widget::layout | |||
| pub fn layout(&mut self, parent_ctx: &mut LayoutCtx, bc: &BoxConstraints) -> Size { | |||
There was a problem hiding this comment.
To keep the diff small. We can change all the widgets in a follow-up PR. I'll include a comment to that effect.
There was a problem hiding this comment.
All of the widgets already appear to be translated in this PR?
There was a problem hiding this comment.
Oh, you're right. Well I removed the WidgetPod method and converted the last users.
masonry/src/contexts.rs
Outdated
| /// their [`layout`] method. | ||
| /// | ||
| /// [`layout`]: Widget::layout | ||
| pub fn run_layout<W: Widget>(&mut self, child: &mut WidgetPod<W>, bc: &BoxConstraints) -> Size { |
There was a problem hiding this comment.
We should ensure that this method is clearly pointed to in the docs for Widget::layout.
I'd also encourage moving this method higher up in this impl block, because it is among the most important methods on LayoutCtx
PoignardAzur
force-pushed
the
implement_pass_spec_layout
branch
from
69c802d
to
dc58cea
Compare
|
|
||
| // TODO - negative rects? | ||
| /// Return `true` if all of `smaller` is within `larger`. | ||
| fn rect_contains(larger: &Rect, smaller: &Rect) -> bool { |
| parent_ctx: &mut LayoutCtx<'_>, | ||
| pod: &mut WidgetPod<W>, | ||
| bc: &BoxConstraints, | ||
| ) -> bool { |
There was a problem hiding this comment.
The semantics of this bool return value are very unclear. I can see through digging through the code it's called_widget, i.e. whether the widget's method was actually called and therefore whether the checks need to be ran?
There was a problem hiding this comment.
I think this pattern pulled more weight when every WidgetPod method used it and call_widget_method_with_checks was used everywhere. We'll probably refactor it away soon. In the meantime, I added a comment for clarity.
| state.needs_compose = true; | ||
| state.is_expecting_place_child_call = true; | ||
| // TODO - Not everything that has been re-laid out needs to be repainted. | ||
| state.needs_paint = true; |
There was a problem hiding this comment.
Why no request_paint/request_compose?
There was a problem hiding this comment.
I think I wrote this code before I renamed those flags. Will fix.
| mouse_pos: parent_ctx.mouse_pos, | ||
| }; | ||
|
|
||
| widget.layout(&mut inner_ctx, bc) |
There was a problem hiding this comment.
Would this be the case to evaluate a potentially cached layout result? I guess that would be what request_layout would be needed for?
There was a problem hiding this comment.
True. I'm adding a comment and leaving it for a future PR.
|
|
||
| state.local_paint_rect = state | ||
| .local_paint_rect | ||
| .union(new_size.to_rect() + state.paint_insets); |
There was a problem hiding this comment.
I'm not a huge fan of using Add for Insets, but that's a Kurbo thing - this is an idiomatic usage thereof
masonry/src/passes/layout.rs
Outdated
|
|
||
| fn log_layout_issues(type_name: &str, size: Size) { | ||
| if size.width.is_infinite() { | ||
| warn!("Widget `{type_name}` has an infinite width."); |
There was a problem hiding this comment.
Should we log the id here as well?
| } | ||
| } | ||
|
|
||
| pub(crate) fn run_layout_on<W: Widget>( |
There was a problem hiding this comment.
We should have a small doc comment here. E.g.:
| pub(crate) fn run_layout_on<W: Widget>( | |
| /// Run [`Widget::layout`] method on the widget contained in `pod`. | |
| /// This will be called by [`LayoutCtx::run_layout`], which is itself called in the parent widget's `layout`. | |
| pub(crate) fn run_layout_on<W: Widget>( |
masonry/src/widget/widget_pod.rs
Outdated
| @@ -504,26 +505,12 @@ impl<W: Widget> WidgetPod<W> { | |||
| /// | |||
| /// [`layout`]: Widget::layout | |||
| pub fn layout(&mut self, parent_ctx: &mut LayoutCtx, bc: &BoxConstraints) -> Size { | |||
There was a problem hiding this comment.
All of the widgets already appear to be translated in this PR?
PoignardAzur
force-pushed
the
implement_pass_spec_layout
branch
from
48a8567
to
5228188
Compare
|
Will merge before this evening unless a blocker comes up. |
| ); | ||
| } | ||
|
|
||
| state.needs_layout = false; |
There was a problem hiding this comment.
There's a little bit of knowledge encoded in this line, so it could be worth having a comment here.
Not going to block on that, though
No description provided.