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

Propagate events to hidden widgets in Either #1351

Merged
merged 6 commits into from
Oct 31, 2020
Merged

Propagate events to hidden widgets in Either #1351

merged 6 commits into from
Oct 31, 2020

Conversation

andrewhickman
Copy link
Contributor

@andrewhickman andrewhickman commented Oct 27, 2020

Fixes #1350

The should_propagate_to_hidden methods are taken from the tabs widget. I made them public since they seem useful for external widget implementors.

Copy link
Collaborator

@jneem jneem 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 to me.

@andrewhickman
Copy link
Contributor Author

andrewhickman commented Oct 29, 2020

This currently has an issue when using a Spinner (or other animated widget). Either doesn't lay out its hidden child, so there is a warning when it gets AnimFrame events

WARN  druid::core] Widget 'druid::widget::spinner::Spinner' received an event (AnimFrame(167854600)) without having been laid out. This likely indicates a missed call to set_layout_rect.

I'm not sure whether it should lay out both children (as Tabs does) or not send AnimFrame events to the child

@andrewhickman
Copy link
Contributor Author

I've changed the warning to only be emitted for events that have should_propagate_to_hidden() == false. @cmyr does that sound reasonable? It means Event::Command and Event::AnimFrame will no longer generate the warning.

Copy link
Collaborator

@luleyleo luleyleo left a comment

Choose a reason for hiding this comment

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

This should have a changlog entry (probably multiple)

@luleyleo luleyleo added the S-waiting-on-author waits for changes from the submitter label Oct 30, 2020
Copy link
Collaborator

@luleyleo luleyleo left a comment

Choose a reason for hiding this comment

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

Thanks!

@andrewhickman
Copy link
Contributor Author

CI had a spurious error but that seems to have resolved itself now

@luleyleo
Copy link
Collaborator

@andrewhickman this looks good to me, should we merge it?

@andrewhickman
Copy link
Contributor Author

Yep, its ready to merge 👍

@luleyleo luleyleo removed the S-waiting-on-author waits for changes from the submitter label Oct 31, 2020
@luleyleo luleyleo merged commit a30b1ef into linebender:master Oct 31, 2020
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.

Excessive invalidation when using Either
3 participants