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 incorrect LifeCycleCtx with FocusChanged event #878

Merged
merged 1 commit into from
Apr 25, 2020
Merged

Conversation

cmyr
Copy link
Member

@cmyr cmyr commented Apr 24, 2020

We were handling this event separately, and were accidentally
reusing the parent context, instead of the child context;
this has the concrete problem that the context had
an incorrect WidgetId.

This fix feels like the lesser of two evils; we add a mechanism
for explicitly substituting a new event for the one received,
ensuring that we only call the child's lifecycle method in one place.

(this explains what I was doing with #876 😅)

Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

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

This is definitely an improvement. I've ran into these wrong context cases myself but was holding off with the fix due to thinking of doing a through test function first.

The propagation logic shouldn't be changed though.

Comment on lines 648 to 650
substitute_event = Some(LifeCycle::FocusChanged(change));
}
false
true
Copy link
Member

Choose a reason for hiding this comment

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

The true should be inside the if old != new block and false otherwise. An else block with false would work.

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch

We were handling this event separately, and were accidentally
reusing the parent context, instead of the child context;
this has the concrete problem that the context had
an incorrect WidgetId.

This fix feels like the lesser of two evils; we add a mechanism
for explicitly substituting a new event for the one received,
ensuring that we only call the child's lifecycle method in one place.
Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

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

Looks good!

@cmyr cmyr merged commit e89a35a into master Apr 25, 2020
@cmyr cmyr deleted the focus-changed-fixup branch September 16, 2020 21:01
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