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

Move internal events to a separate enum. #833

Merged
merged 2 commits into from
Apr 13, 2020

Conversation

xStrom
Copy link
Member

@xStrom xStrom commented Apr 13, 2020

As discussed in #826 this PR does the following:

  • Moves all internal Event variants into a InternalEvent enum and adds the Event::Internal(InternalEvent) variant.
  • Moves all internal LifeCycle variants into a InternalLifeCycle enum and adds the LifeCycle::Internal(InternalLifeCycle) variant.

Closes #826

Comment on lines -391 to -392
// Show the scrollbars any time our size changes
Event::Size(_) => self.reset_scrollbar_fade(ctx, &env),
Copy link
Member Author

Choose a reason for hiding this comment

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

This was a Scroll bug, but it's not easy to fix. More info in #834.

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.

Looks good to me

@luleyleo luleyleo added the breaking change pull request breaks user code label Apr 13, 2020
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

broadly good, but I'm unsure about Size. I haven't read #834 yet, which may be relevant? But my inclination is to not make size private, but maybe change its behaviour.

#[derive(Debug, Clone)]
pub enum InternalEvent {
/// Sent to the root widget when the window size changes.
Size(Size),
Copy link
Member

Choose a reason for hiding this comment

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

There's an ongoing question about whether or not this should actually be more widely used, and sent down the tree by default; and even if not I do think of it as public API, custom widgets would regularly want to handle it I think?

@xStrom
Copy link
Member Author

xStrom commented Apr 13, 2020

Well Size is only sent to the root widget and it's window resize, not layout size changes. By changing its behavior, you mean that every widget should be informed about window resizing? In that case, it should at least be renamed to WindowSize. Or you're thinking about changing its meaning?

@cmyr
Copy link
Member

cmyr commented Apr 13, 2020

yes, it could be WindowSize. Basically I think that this is an event that applications might want to handle explicitly, such as by recentering the contents of a scroll view.

@xStrom
Copy link
Member Author

xStrom commented Apr 13, 2020

Okay, what about propagation? Still send it only to the root or to everyone?

@luleyleo
Copy link
Collaborator

Wouldn't it be more useful if widgets could get an event when their own size changes?
Not sure what the window size would really be useful for.

@xStrom
Copy link
Member Author

xStrom commented Apr 13, 2020

I'm somewhat skeptical of the value of the window size event as well. However for now I brought it back to the public Event enum under the better describing WindowSize name. We can discuss its value further, but I don't want that holding up this merge.

@xStrom xStrom merged commit a0278cb into linebender:master Apr 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change pull request breaks user code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hide internal variants of LifeCycle from docs
3 participants