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

Add WidgetPod::set_origin, soft-deprecate set_layout_rect #1289

Merged
merged 1 commit into from
Nov 16, 2020

Conversation

cmyr
Copy link
Member

@cmyr cmyr commented Oct 7, 2020

As outlined in #1242.

Allowing a parent to give a widget a size different from the size
it had computed in its layout method had the potential to cause
subtle breakage. With this change, we now always use the size
returned by the widget's layout method, and expect the container
to set the origin only, using WidgetPod::set_origin.

This does not remove or (yet) officially deprecate
WidgetPod::set_layout_rect; I am curious to see if there is any
existing code where a widget was being given a size different
from what it had returned, and deprecating this method would
cause us to fail in CI. I expect to deprecate it before 0.7.0, though.

This is motivated by the work on baselines; baseline offsets are one
of the things that would be subtly broken if the child's chosen
size and the final size differed.

@cmyr cmyr added the S-needs-review waits for review label Oct 8, 2020
/// its own [`Widget::layout`] implementation, then possibly modify the returned [`Size`], and
/// finally call this `set_layout_rect` method on the child to set the final layout [`Rect`].
/// its own [`Widget::layout`] implementation, and then call `set_origin` to
/// position those children.
///
/// The child will receive the [`LifeCycle::Size`] event informing them of the final [`Size`].
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we still need LifeCycle::Size if your size is always determined by your own layout function?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was wondering about this too. I guess it depends on how this event is actually used? I could see it making sense in some cases, because it means the widget doesn't need to internally hold on to past sizes. I'm not actually sure if this matters, though? I don't believe I use this event anywhere.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess I can't imagine anything that I would want to do in lifecycle that I couldn't just do in layout 🤷 Given our lack of API stability, my inclination would be to get rid of it and then put it back later if someone finds a use. But I'll give you the green button anyway...

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe @xStrom would like to chime in? Removing this would also let us remove some special-case code we use to send this notification..

@cmyr cmyr added S-ready PR is ready to merge and removed S-needs-review waits for review labels Oct 12, 2020
As outlined in #1242.

Allowing a parent to give a widget a size different from the size
it had computed in its `layout` method had the potential to cause
subtle breakage. With this change, we now always use the size
returned by the widget's layout method, and expect the container
to set the origin only, using WidgetPod::set_origin.

This does not remove or (yet) officially deprecate
WidgetPod::set_layout_rect; I am curious to see if there is any
existing code where a widget was being given a size different
from what it had returned, and deprecating this method would
cause us to fail in CI. I expect to deprecate it before 0.7.0, though.

This is motivated by the work on baselines; baseline offsets are one
of the things that would be subtly broken if the child's chosen
size and the final size differed.
@cmyr cmyr merged commit 76e9290 into master Nov 16, 2020
@cmyr cmyr deleted the set-origin-not-rect branch November 16, 2020 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-ready PR is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants