fix(masonry): flex layout not running after removing child #981
Conversation
not-my-profile
changed the title
There was a problem hiding this comment.
This is really good. I still have a few more thoughts.
Overall, the flex implementation is not written in a way which convinces me of its correctness, but this should only possibly make things more correct.
I think that cases where a spacer's height changes won't be handled properly, but that's a pre-existing issue. It suggests that a more fundamental fix is needed, i.e. storing the value of remaining, and setting any_changed based on that changing instead. I don't think that needs to happen in this PR.
| @@ -28,6 +28,7 @@ pub struct Flex { | |||
| fill_major_axis: bool, | |||
| children: Vec<Child>, | |||
| old_bc: BoxConstraints, | |||
| child_removed: bool, // TODO: why can't we use WidgetState.children_changed instead? | |||
There was a problem hiding this comment.
The reason we can't use children_changed here is two reasons:
- It will always be handled "before" we get to layout. Indeed, that's the reason that it doesn't have a getter - at this point in the timeline, it doesn't have a meaningful value.
- It doesn't handle spacers being removed
There was a problem hiding this comment.
That is, we should remove this comment.
| #[test] | ||
| fn remove_child_after_expanded_child() { | ||
| let widget = Flex::column() | ||
| .with_flex_child( | ||
| SizedBox::new(Label::new("child 1")) | ||
| .expand() | ||
| .background(palette::css::PURPLE), | ||
| 1., | ||
| ) | ||
| .with_flex_child(Label::new("child 2"), 1.0); | ||
|
|
||
| let window_size = Size::new(200.0, 150.0); | ||
| let mut harness = TestHarness::create_with_size(widget, window_size); | ||
|
|
||
| harness.edit_root_widget(|mut flex| { | ||
| let mut flex = flex.downcast::<Flex>(); | ||
| Flex::remove_child(&mut flex, 1); | ||
| }); | ||
| assert_render_snapshot!(harness, "flex_remove_child_after_expanded_child"); | ||
| } |
There was a problem hiding this comment.
Would you mind adding a similar test where the removed item is a spacer? I believe that this PR should also fix that, but I'd like to be sure.
| @@ -1009,7 +1012,8 @@ impl Widget for Flex { | |||
| // indicates that the box constrains for the following children have changed. Therefore they | |||
| // have to calculate layout again. | |||
| let bc_changed = self.old_bc != *bc; | |||
| let mut any_changed = bc_changed; | |||
| let mut any_changed = bc_changed || self.child_removed; | |||
There was a problem hiding this comment.
Effectively, any_changed is being used a proxy for if the amount of "available flex space" has changed since last run, right?
This all feels very fragile.
I wonder if just expressing that "directly" would make this better? This doesn't need to happen in this PR
There was a problem hiding this comment.
I think the root cause is that layout caching really shouldn't happen in widget implementations (e.g. in Flex::layout()) and should happen at the Masonry level instead.
On the medium term, I expect we'll fix that in the coming weeks with the layout refactor.
On the short term, I'm fine merging this PR, ideally with a comment explaining what I just said.
While working on implementing a devtools widget for Masonry I found a bug with the Flex widget (see the test I added). I found a fix but it feels more like a workaround than a clean solution because I think we should be able to use
WidgetState.children_changedbut I couldn't make it work (let mut any_changed = bc_changed || ctx.widget_state.children_changed;doesn't work for some reason?).Without the fix the render result is: