Skip to content
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

fix: prevent building component overlay with stale layout #1341

Merged
merged 4 commits into from
May 14, 2022

Conversation

nicksenger
Copy link
Contributor

In the implementation of on_event for a component's overlay (both pure and impure), after processing local messages which may mutate the state and in turn alter the output of Component::view, the cached element and overlay must be rebuilt. Currently, it is rebuilt with a layout tree which is potentially stale, because the shape of the layout tree is dependent upon the cached element which may have changed.

I've added a preliminary fix here, which is to rebuild the cached element only after processing any local messages, then attempt to rebuild the overlay at the beginning of on_event if it is absent.

I'll open this as a draft because it feels a bit odd to intentionally leave the cached overlay element as None after on_event. I haven't yet thought of another way to resolve this though.

@nicksenger nicksenger changed the title fix: prevent building overlay with stale layout fix: prevent building component overlay with stale layout May 11, 2022
@hecrj hecrj added this to the 0.4.3 milestone May 12, 2022
Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

Good catch!

I think the root cause of the issue here is in the UserInterface::update implementation. Specifically, we are not relayouting the base layer when an overlay invalidates the layout through a Shell, causing the layout to be stale for further events.

I have tried to fix this in 296e157. I needed to rewrite the code to use a for loop instead of iterators as well as use ManuallyDrop to please the borrow checker, but it seems to be happy in the end.

Could you test if these changes fix the issue?

@hecrj hecrj marked this pull request as ready for review May 13, 2022 18:45
@hecrj hecrj added bug Something isn't working layout shell labels May 13, 2022
@nicksenger
Copy link
Contributor Author

That explanation makes sense, and the solution looks good!

I had to make one small change in order for it to fix the issue though:

Since the component overlay will always invalidate the layout when there are local messages produced in on_event, and invalidation of the layout now results in the overlay being rebuilt in UserInterface::update, the overlay no longer needs to be rebuilt after processing local messages in the component's on_event (which is what triggered the panic, since it was still using the same layout to produce the overlay from the newly rebuilt cached element).

Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

Makes sense. Less code to worry about!

Let's merge 🚢

@hecrj hecrj merged commit ba33e92 into iced-rs:master May 14, 2022
hecrj added a commit that referenced this pull request May 31, 2022
fix: prevent building component overlay with stale layout
@hecrj hecrj modified the milestones: 0.4.3, 0.5.0 Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working layout shell
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants