Issue 2836313002: BoxLayout now suports per-view margins.

kylix_rd

Description was changed from ========== BoxLayout now suports per-view margins. Call BoxLayout::SetViewMargins(view, margins) to set ...

3 years, 8 months ago (2017-04-25 14:22:34 UTC) #1

kylix_rd

kylixrd@chromium.org changed reviewers: + pkasting@chromium.org, sky@chromium.org

3 years, 8 months ago (2017-04-25 14:22:34 UTC) #2

kylix_rd

Here's the initial cut at adding per-view margins to BoxLayout. If this passes muster, I'll ...

3 years, 8 months ago (2017-04-25 14:24:23 UTC) #3

sky

https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout.cc File ui/views/layout/box_layout.cc (right): https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout.cc#newcode15 ui/views/layout/box_layout.cc:15: : view_(nullptr), layout_(nullptr), margins_(0, 0, 0, 0){}; (0,0,0,0) is ...

3 years, 8 months ago (2017-04-25 17:28:59 UTC) #4

Peter Kasting

My needs are minimal -- I won't have cases with adjacent margins, so collapsing behavior ...

3 years, 8 months ago (2017-04-25 21:43:34 UTC) #5

sky

https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout.cc File ui/views/layout/box_layout.cc (right): https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout.cc#newcode306 ui/views/layout/box_layout.cc:306: gfx::Size BoxLayout::GetPreferredSizeForChildWidth(const View* host, It seems like what you're ...

3 years, 8 months ago (2017-04-25 22:46:07 UTC) #6

kylix_rd

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-04-26 17:41:21 UTC) #7

kylix_rd

https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout.cc File ui/views/layout/box_layout.cc (right): https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout.cc#newcode15 ui/views/layout/box_layout.cc:15: : view_(nullptr), layout_(nullptr), margins_(0, 0, 0, 0){}; On 2017/04/25 ...

3 years, 8 months ago (2017-04-26 17:41:33 UTC) #8

https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout.cc
File ui/views/layout/box_layout.cc (right):

https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout....
ui/views/layout/box_layout.cc:15: : view_(nullptr), layout_(nullptr),
margins_(0, 0, 0, 0){};
On 2017/04/25 17:28:58, sky wrote:
> (0,0,0,0) is the default, so need to specify margins_ here.

Done.

https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout....
ui/views/layout/box_layout.cc:20: BoxLayout::ViewWrapper::ViewWrapper(const
ViewWrapper& wrapper)
On 2017/04/25 17:28:58, sky wrote:
> = default?

Done.

https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout....
ui/views/layout/box_layout.cc:34: const_cast<View*>(view_)->SetBounds(
On 2017/04/25 17:28:58, sky wrote:
> Why the const_cast here?

The view_ field is declared as "const View* view_;" Most references to the view
are const. So I elected to cast the "const" away only at the one point that
modifies the view.

https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout....
ui/views/layout/box_layout.cc:306: gfx::Size
BoxLayout::GetPreferredSizeForChildWidth(const View* host,
On 2017/04/25 22:46:07, sky wrote:
> It seems like what you're doing here is getting the max of the heights, which
> includes the margins. Shouldn't you calculate the max of the margins and then
> add that to that the max of the views? For example, if the alignment is
> horizontal and you have two views, A and B then I believe you're calculating
the
> pref height as:
> 
> Max(a_wrapper.preferred_size().height(), b_wrapper.preferred_size().height());
> 
> where _wrapper.preferred_size() includes the margin.
> 
> Don't you want:
> Max(A.margin.top, B.margin.top) +
> Max(a.preferred_size.height(), b.preferred_size().height()) +
> Max(A.margin.bottom, B.margin.bottom);
> ?
> Similarly when you are positioning views I would assume you position them
> relative to the max of the margins along the opposite axis.

The next patch should resolve this confusion. The short answer is that the
width/height of each view always includes its margins. No collapsing is done at
this time.

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2836313002/20001

3 years, 8 months ago (2017-04-26 17:42:20 UTC) #9

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-04-26 18:40:05 UTC) #10

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 8 months ago (2017-04-26 18:40:06 UTC) #11

sky

https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout.cc File ui/views/layout/box_layout.cc (right): https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout.cc#newcode34 ui/views/layout/box_layout.cc:34: const_cast<View*>(view_)->SetBounds( On 2017/04/26 17:41:33, kylix_rd wrote: > On 2017/04/25 ...

3 years, 8 months ago (2017-04-26 19:57:07 UTC) #12

kylix_rd

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-04-27 18:56:31 UTC) #13

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2836313002/40001

3 years, 7 months ago (2017-04-27 18:56:57 UTC) #14

kylix_rd

This patch addresses more feedback and also moves to using a class property on View ...

3 years, 7 months ago (2017-04-27 18:58:01 UTC) #15

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-04-27 20:08:04 UTC) #16

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 7 months ago (2017-04-27 20:08:05 UTC) #17

sky

Could you please clarify what 'more feedback'? Specifically are you still working on the patch ...

3 years, 7 months ago (2017-04-27 22:21:45 UTC) #18

kylix_rd

On 2017/04/27 22:21:45, sky wrote: > Could you please clarify what 'more feedback'? Specifically are ...

3 years, 7 months ago (2017-04-28 13:26:21 UTC) #19

sky

On 2017/04/26 17:41:33, kylix_rd wrote: > https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout.cc > File ui/views/layout/box_layout.cc (right): > > https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout.cc#newcode15 > ...

3 years, 7 months ago (2017-04-28 19:20:59 UTC) #20

On 2017/04/26 17:41:33, kylix_rd wrote:
>
https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout.cc
> File ui/views/layout/box_layout.cc (right):
> 
>
https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout....
> ui/views/layout/box_layout.cc:15: : view_(nullptr), layout_(nullptr),
> margins_(0, 0, 0, 0){};
> On 2017/04/25 17:28:58, sky wrote:
> > (0,0,0,0) is the default, so need to specify margins_ here.
> 
> Done.
> 
>
https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout....
> ui/views/layout/box_layout.cc:20: BoxLayout::ViewWrapper::ViewWrapper(const
> ViewWrapper& wrapper)
> On 2017/04/25 17:28:58, sky wrote:
> > = default?
> 
> Done.
> 
>
https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout....
> ui/views/layout/box_layout.cc:34: const_cast<View*>(view_)->SetBounds(
> On 2017/04/25 17:28:58, sky wrote:
> > Why the const_cast here?
> 
> The view_ field is declared as "const View* view_;" Most references to the
view
> are const. So I elected to cast the "const" away only at the one point that
> modifies the view.
> 
>
https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout....
> ui/views/layout/box_layout.cc:306: gfx::Size
> BoxLayout::GetPreferredSizeForChildWidth(const View* host,
> On 2017/04/25 22:46:07, sky wrote:
> > It seems like what you're doing here is getting the max of the heights,
which
> > includes the margins. Shouldn't you calculate the max of the margins and
then
> > add that to that the max of the views? For example, if the alignment is
> > horizontal and you have two views, A and B then I believe you're calculating
> the
> > pref height as:
> > 
> > Max(a_wrapper.preferred_size().height(),
b_wrapper.preferred_size().height());
> > 
> > where _wrapper.preferred_size() includes the margin.
> > 
> > Don't you want:
> > Max(A.margin.top, B.margin.top) +
> > Max(a.preferred_size.height(), b.preferred_size().height()) +
> > Max(A.margin.bottom, B.margin.bottom);
> > ?
> > Similarly when you are positioning views I would assume you position them
> > relative to the max of the margins along the opposite axis.
> 
> The next patch should resolve this confusion. The short answer is that the
> width/height of each view always includes its margins. No collapsing is done
at
> this time.

So again, I think this is wrong. I think the margin along the cross axis should
be the max of the margins for each view as I outlined above.

kylix_rd

On 2017/04/28 19:20:59, sky wrote: > On 2017/04/26 17:41:33, kylix_rd wrote: > > > https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout.cc ...

3 years, 7 months ago (2017-05-02 20:37:39 UTC) #21

On 2017/04/28 19:20:59, sky wrote:
> On 2017/04/26 17:41:33, kylix_rd wrote:
> >
>
https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout.cc
> > File ui/views/layout/box_layout.cc (right):
> > 
> >
>
https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout....
> > ui/views/layout/box_layout.cc:15: : view_(nullptr), layout_(nullptr),
> > margins_(0, 0, 0, 0){};
> > On 2017/04/25 17:28:58, sky wrote:
> > > (0,0,0,0) is the default, so need to specify margins_ here.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout....
> > ui/views/layout/box_layout.cc:20: BoxLayout::ViewWrapper::ViewWrapper(const
> > ViewWrapper& wrapper)
> > On 2017/04/25 17:28:58, sky wrote:
> > > = default?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout....
> > ui/views/layout/box_layout.cc:34: const_cast<View*>(view_)->SetBounds(
> > On 2017/04/25 17:28:58, sky wrote:
> > > Why the const_cast here?
> > 
> > The view_ field is declared as "const View* view_;" Most references to the
> view
> > are const. So I elected to cast the "const" away only at the one point that
> > modifies the view.
> > 
> >
>
https://codereview.chromium.org/2836313002/diff/1/ui/views/layout/box_layout....
> > ui/views/layout/box_layout.cc:306: gfx::Size
> > BoxLayout::GetPreferredSizeForChildWidth(const View* host,
> > On 2017/04/25 22:46:07, sky wrote:
> > > It seems like what you're doing here is getting the max of the heights,
> which
> > > includes the margins. Shouldn't you calculate the max of the margins and
> then
> > > add that to that the max of the views? For example, if the alignment is
> > > horizontal and you have two views, A and B then I believe you're
calculating
> > the
> > > pref height as:
> > > 
> > > Max(a_wrapper.preferred_size().height(),
> b_wrapper.preferred_size().height());
> > > 
> > > where _wrapper.preferred_size() includes the margin.
> > > 
> > > Don't you want:
> > > Max(A.margin.top, B.margin.top) +
> > > Max(a.preferred_size.height(), b.preferred_size().height()) +
> > > Max(A.margin.bottom, B.margin.bottom);
> > > ?
> > > Similarly when you are positioning views I would assume you position them
> > > relative to the max of the margins along the opposite axis.
> > 
> > The next patch should resolve this confusion. The short answer is that the
> > width/height of each view always includes its margins. No collapsing is done
> at
> > this time.
> 
> So again, I think this is wrong. I think the margin along the cross axis
should
> be the max of the margins for each view as I outlined above.

I think I understand what you're getting at here. I consider this part of the
work to collapse the margins. That includes collapsing each view's margins with
the surrounding insets as well as the max of the margins along the cross axis.

I'm working on doing the margin collapse along all axes. I think I'll have a
fairly simple solution in the next day or so.

sky

Glad to hear it! Don't hesitate to ask questions if I wasn't clear in my ...

3 years, 7 months ago (2017-05-03 00:03:09 UTC) #22

Glad to hear it!

Don't hesitate to ask questions if I wasn't clear in my feedback.

  -Scott

On Tue, May 2, 2017 at 1:37 PM, <kylixrd@chromium.org> wrote:

> On 2017/04/28 19:20:59, sky wrote:
> > On 2017/04/26 17:41:33, kylix_rd wrote:
> > >
> >
> https://codereview.chromium.org/2836313002/diff/1/ui/
> views/layout/box_layout.cc
> > > File ui/views/layout/box_layout.cc (right):
> > >
> > >
> >
> https://codereview.chromium.org/2836313002/diff/1/ui/
> views/layout/box_layout.cc#newcode15
> > > ui/views/layout/box_layout.cc:15: : view_(nullptr), layout_(nullptr),
> > > margins_(0, 0, 0, 0){};
> > > On 2017/04/25 17:28:58, sky wrote:
> > > > (0,0,0,0) is the default, so need to specify margins_ here.
> > >
> > > Done.
> > >
> > >
> >
> https://codereview.chromium.org/2836313002/diff/1/ui/
> views/layout/box_layout.cc#newcode20
> > > ui/views/layout/box_layout.cc:20: BoxLayout::ViewWrapper::
> ViewWrapper(const
> > > ViewWrapper& wrapper)
> > > On 2017/04/25 17:28:58, sky wrote:
> > > > = default?
> > >
> > > Done.
> > >
> > >
> >
> https://codereview.chromium.org/2836313002/diff/1/ui/
> views/layout/box_layout.cc#newcode34
> > > ui/views/layout/box_layout.cc:34: const_cast<View*>(view_)->SetBounds(
> > > On 2017/04/25 17:28:58, sky wrote:
> > > > Why the const_cast here?
> > >
> > > The view_ field is declared as "const View* view_;" Most references to
> the
> > view
> > > are const. So I elected to cast the "const" away only at the one point
> that
> > > modifies the view.
> > >
> > >
> >
> https://codereview.chromium.org/2836313002/diff/1/ui/
> views/layout/box_layout.cc#newcode306
> > > ui/views/layout/box_layout.cc:306: gfx::Size
> > > BoxLayout::GetPreferredSizeForChildWidth(const View* host,
> > > On 2017/04/25 22:46:07, sky wrote:
> > > > It seems like what you're doing here is getting the max of the
> heights,
> > which
> > > > includes the margins. Shouldn't you calculate the max of the margins
> and
> > then
> > > > add that to that the max of the views? For example, if the alignment
> is
> > > > horizontal and you have two views, A and B then I believe you're
> calculating
> > > the
> > > > pref height as:
> > > >
> > > > Max(a_wrapper.preferred_size().height(),
> > b_wrapper.preferred_size().height());
> > > >
> > > > where _wrapper.preferred_size() includes the margin.
> > > >
> > > > Don't you want:
> > > > Max(A.margin.top, B.margin.top) +
> > > > Max(a.preferred_size.height(), b.preferred_size().height()) +
> > > > Max(A.margin.bottom, B.margin.bottom);
> > > > ?
> > > > Similarly when you are positioning views I would assume you position
> them
> > > > relative to the max of the margins along the opposite axis.
> > >
> > > The next patch should resolve this confusion. The short answer is that
> the
> > > width/height of each view always includes its margins. No collapsing
> is done
> > at
> > > this time.
> >
> > So again, I think this is wrong. I think the margin along the cross axis
> should
> > be the max of the margins for each view as I outlined above.
>
> I think I understand what you're getting at here. I consider this part of
> the
> work to collapse the margins. That includes collapsing each view's margins
> with
> the surrounding insets as well as the max of the margins along the cross
> axis.
>
> I'm working on doing the margin collapse along all axes. I think I'll have
> a
> fairly simple solution in the next day or so.
>
> https://codereview.chromium.org/2836313002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

kylix_rd

On 2017/05/03 00:03:09, sky wrote: > Glad to hear it! > > Don't hesitate to ...

3 years, 7 months ago (2017-05-03 18:29:22 UTC) #23

On 2017/05/03 00:03:09, sky wrote:
> Glad to hear it!
> 
> Don't hesitate to ask questions if I wasn't clear in my feedback.
> 
>   -Scott

If the current patch can get Peter unblocked, might we go ahead and land it
while I work on the margin collapsing? I will probably take at least the rest of
the week and into next to work through the reviews, write the unit-tests and get
the margin collapsing landed.

I'd also like to look at merging/replacing the functionality of BoxLayout with
AlignLayout. I have an idea to minimize the impact (other than the
class/module/header names) on the existing code.
 
> On Tue, May 2, 2017 at 1:37 PM, <mailto:kylixrd@chromium.org> wrote:
> 
> > On 2017/04/28 19:20:59, sky wrote:
> > > On 2017/04/26 17:41:33, kylix_rd wrote:
> > > >
> > >
> > https://codereview.chromium.org/2836313002/diff/1/ui/
> > views/layout/box_layout.cc
> > > > File ui/views/layout/box_layout.cc (right):
> > > >
> > > >
> > >
> > https://codereview.chromium.org/2836313002/diff/1/ui/
> > views/layout/box_layout.cc#newcode15
> > > > ui/views/layout/box_layout.cc:15: : view_(nullptr), layout_(nullptr),
> > > > margins_(0, 0, 0, 0){};
> > > > On 2017/04/25 17:28:58, sky wrote:
> > > > > (0,0,0,0) is the default, so need to specify margins_ here.
> > > >
> > > > Done.
> > > >
> > > >
> > >
> > https://codereview.chromium.org/2836313002/diff/1/ui/
> > views/layout/box_layout.cc#newcode20
> > > > ui/views/layout/box_layout.cc:20: BoxLayout::ViewWrapper::
> > ViewWrapper(const
> > > > ViewWrapper& wrapper)
> > > > On 2017/04/25 17:28:58, sky wrote:
> > > > > = default?
> > > >
> > > > Done.
> > > >
> > > >
> > >
> > https://codereview.chromium.org/2836313002/diff/1/ui/
> > views/layout/box_layout.cc#newcode34
> > > > ui/views/layout/box_layout.cc:34: const_cast<View*>(view_)->SetBounds(
> > > > On 2017/04/25 17:28:58, sky wrote:
> > > > > Why the const_cast here?
> > > >
> > > > The view_ field is declared as "const View* view_;" Most references to
> > the
> > > view
> > > > are const. So I elected to cast the "const" away only at the one point
> > that
> > > > modifies the view.
> > > >
> > > >
> > >
> > https://codereview.chromium.org/2836313002/diff/1/ui/
> > views/layout/box_layout.cc#newcode306
> > > > ui/views/layout/box_layout.cc:306: gfx::Size
> > > > BoxLayout::GetPreferredSizeForChildWidth(const View* host,
> > > > On 2017/04/25 22:46:07, sky wrote:
> > > > > It seems like what you're doing here is getting the max of the
> > heights,
> > > which
> > > > > includes the margins. Shouldn't you calculate the max of the margins
> > and
> > > then
> > > > > add that to that the max of the views? For example, if the alignment
> > is
> > > > > horizontal and you have two views, A and B then I believe you're
> > calculating
> > > > the
> > > > > pref height as:
> > > > >
> > > > > Max(a_wrapper.preferred_size().height(),
> > > b_wrapper.preferred_size().height());
> > > > >
> > > > > where _wrapper.preferred_size() includes the margin.
> > > > >
> > > > > Don't you want:
> > > > > Max(A.margin.top, B.margin.top) +
> > > > > Max(a.preferred_size.height(), b.preferred_size().height()) +
> > > > > Max(A.margin.bottom, B.margin.bottom);
> > > > > ?
> > > > > Similarly when you are positioning views I would assume you position
> > them
> > > > > relative to the max of the margins along the opposite axis.
> > > >
> > > > The next patch should resolve this confusion. The short answer is that
> > the
> > > > width/height of each view always includes its margins. No collapsing
> > is done
> > > at
> > > > this time.
> > >
> > > So again, I think this is wrong. I think the margin along the cross axis
> > should
> > > be the max of the margins for each view as I outlined above.
> >
> > I think I understand what you're getting at here. I consider this part of
> > the
> > work to collapse the margins. That includes collapsing each view's margins
> > with
> > the surrounding insets as well as the max of the margins along the cross
> > axis.
> >
> > I'm working on doing the margin collapse along all axes. I think I'll have
> > a
> > fairly simple solution in the next day or so.
> >
> > https://codereview.chromium.org/2836313002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

sky

I believe Peter is out of the office right now, so I'm not sure he ...

3 years, 7 months ago (2017-05-04 03:51:09 UTC) #24

I believe Peter is out of the office right now, so I'm not sure he is
blocked. Am I wrong there?

I would also prefer to land this the right way rather than land something
we know needs a lot of changes.

  -Scott

On Wed, May 3, 2017 at 11:29 AM, <kylixrd@chromium.org> wrote:

> On 2017/05/03 00:03:09, sky wrote:
> > Glad to hear it!
> >
> > Don't hesitate to ask questions if I wasn't clear in my feedback.
> >
> > -Scott
>
> If the current patch can get Peter unblocked, might we go ahead and land it
> while I work on the margin collapsing? I will probably take at least the
> rest of
> the week and into next to work through the reviews, write the unit-tests
> and get
> the margin collapsing landed.
>
> I'd also like to look at merging/replacing the functionality of BoxLayout
> with
> AlignLayout. I have an idea to minimize the impact (other than the
> class/module/header names) on the existing code.
>
>
> > On Tue, May 2, 2017 at 1:37 PM, <mailto:kylixrd@chromium.org> wrote:
> >
> > > On 2017/04/28 19:20:59, sky wrote:
> > > > On 2017/04/26 17:41:33, kylix_rd wrote:
> > > > >
> > > >
> > > https://codereview.chromium.org/2836313002/diff/1/ui/
> > > views/layout/box_layout.cc
> > > > > File ui/views/layout/box_layout.cc (right):
> > > > >
> > > > >
> > > >
> > > https://codereview.chromium.org/2836313002/diff/1/ui/
> > > views/layout/box_layout.cc#newcode15
> > > > > ui/views/layout/box_layout.cc:15: : view_(nullptr),
> layout_(nullptr),
> > > > > margins_(0, 0, 0, 0){};
> > > > > On 2017/04/25 17:28:58, sky wrote:
> > > > > > (0,0,0,0) is the default, so need to specify margins_ here.
> > > > >
> > > > > Done.
> > > > >
> > > > >
> > > >
> > > https://codereview.chromium.org/2836313002/diff/1/ui/
> > > views/layout/box_layout.cc#newcode20
> > > > > ui/views/layout/box_layout.cc:20: BoxLayout::ViewWrapper::
> > > ViewWrapper(const
> > > > > ViewWrapper& wrapper)
> > > > > On 2017/04/25 17:28:58, sky wrote:
> > > > > > = default?
> > > > >
> > > > > Done.
> > > > >
> > > > >
> > > >
> > > https://codereview.chromium.org/2836313002/diff/1/ui/
> > > views/layout/box_layout.cc#newcode34
> > > > > ui/views/layout/box_layout.cc:34: const_cast<View*>(view_)->
> SetBounds(
> > > > > On 2017/04/25 17:28:58, sky wrote:
> > > > > > Why the const_cast here?
> > > > >
> > > > > The view_ field is declared as "const View* view_;" Most
> references to
> > > the
> > > > view
> > > > > are const. So I elected to cast the "const" away only at the one
> point
> > > that
> > > > > modifies the view.
> > > > >
> > > > >
> > > >
> > > https://codereview.chromium.org/2836313002/diff/1/ui/
> > > views/layout/box_layout.cc#newcode306
> > > > > ui/views/layout/box_layout.cc:306: gfx::Size
> > > > > BoxLayout::GetPreferredSizeForChildWidth(const View* host,
> > > > > On 2017/04/25 22:46:07, sky wrote:
> > > > > > It seems like what you're doing here is getting the max of the
> > > heights,
> > > > which
> > > > > > includes the margins. Shouldn't you calculate the max of the
> margins
> > > and
> > > > then
> > > > > > add that to that the max of the views? For example, if the
> alignment
> > > is
> > > > > > horizontal and you have two views, A and B then I believe you're
> > > calculating
> > > > > the
> > > > > > pref height as:
> > > > > >
> > > > > > Max(a_wrapper.preferred_size().height(),
> > > > b_wrapper.preferred_size().height());
> > > > > >
> > > > > > where _wrapper.preferred_size() includes the margin.
> > > > > >
> > > > > > Don't you want:
> > > > > > Max(A.margin.top, B.margin.top) +
> > > > > > Max(a.preferred_size.height(), b.preferred_size().height()) +
> > > > > > Max(A.margin.bottom, B.margin.bottom);
> > > > > > ?
> > > > > > Similarly when you are positioning views I would assume you
> position
> > > them
> > > > > > relative to the max of the margins along the opposite axis.
> > > > >
> > > > > The next patch should resolve this confusion. The short answer is
> that
> > > the
> > > > > width/height of each view always includes its margins. No
> collapsing
> > > is done
> > > > at
> > > > > this time.
> > > >
> > > > So again, I think this is wrong. I think the margin along the cross
> axis
> > > should
> > > > be the max of the margins for each view as I outlined above.
> > >
> > > I think I understand what you're getting at here. I consider this part
> of
> > > the
> > > work to collapse the margins. That includes collapsing each view's
> margins
> > > with
> > > the surrounding insets as well as the max of the margins along the
> cross
> > > axis.
> > >
> > > I'm working on doing the margin collapse along all axes. I think I'll
> have
> > > a
> > > fairly simple solution in the next day or so.
> > >
> > > https://codereview.chromium.org/2836313002/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2836313002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

kylix_rd

Here's BoxLayout with full margin collapsing. I've also got most of the work done on ...

3 years, 7 months ago (2017-05-09 16:24:16 UTC) #26

kylix_rd

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-09 18:54:55 UTC) #27

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2836313002/100001

3 years, 7 months ago (2017-05-09 18:55:50 UTC) #28

sky

https://codereview.chromium.org/2836313002/diff/40001/ui/views/view.h File ui/views/view.h (right): https://codereview.chromium.org/2836313002/diff/40001/ui/views/view.h#newcode1705 ui/views/view.h:1705: VIEWS_EXPORT extern const ui::ClassProperty<gfx::Insets*>* const kMarginsKey; Please move this ...

3 years, 7 months ago (2017-05-09 19:26:04 UTC) #29

kylix_rd

Some things were already addressed in a subsequent patch prior to the comments. Others are ...

3 years, 7 months ago (2017-05-09 20:07:06 UTC) #30

Some things were already addressed in a subsequent patch prior to the comments.
Others are in process.

https://codereview.chromium.org/2836313002/diff/80001/ui/views/examples/box_l...
File ui/views/examples/box_layout_example.cc (right):

https://codereview.chromium.org/2836313002/diff/80001/ui/views/examples/box_l...
ui/views/examples/box_layout_example.cc:393:
base::StringToInt(between_child_spacing_->text(), &child_spacing);
On 2017/05/09 19:26:03, sky wrote:
> Remember that conversion may fail and return false, in which case the field
> isn't updated.

According to the doc, errors will set |*output| to 0, which is fine in this
instance.

https://codereview.chromium.org/2836313002/diff/80001/ui/views/examples/box_l...
File ui/views/examples/box_layout_example.h (right):

https://codereview.chromium.org/2836313002/diff/80001/ui/views/examples/box_l...
ui/views/examples/box_layout_example.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2017/05/09 19:26:03, sky wrote:
> 2017, and we don't use (c) in chromium.

Thanks.

Copy-pasta error. Copied block from another file and didn't update.

https://codereview.chromium.org/2836313002/diff/80001/ui/views/layout/box_lay...
File ui/views/layout/box_layout.cc (right):

https://codereview.chromium.org/2836313002/diff/80001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:328: if (collapse_margins_spacing_) {
On 2017/05/09 19:26:03, sky wrote:
> Generally we prefer early return than lots of nesting.

"if" block gone in subsequent patch.

https://codereview.chromium.org/2836313002/diff/80001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:350: void
BoxLayout::AdjustCrossAxisForMargin(const ViewWrapper& child,
On 2017/05/09 19:26:03, sky wrote:
> I don't think this implements what I outlined in comment #6:
> https://codereview.chromium.org/2836313002/#msg6 . My suggestion there is I
> think the cross axis margins should be the max of across all children. I say
> this as to do otherwise means the views aren't going to be aligned nicely on
the
> cross axis, they'll be staggered.

In all modes other than stretch, they'll be staggered anyway. If alignment is
wanted, cross-axis margins should be set consistently. I was just maintaining
that "feature" of the layout.

https://codereview.chromium.org/2836313002/diff/80001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:449: DCHECK_EQ(host_, host);
On 2017/05/09 19:26:03, sky wrote:
> Why bother with passing in host when host_ is already a member? Same comment
for
> the other functions.

I was keeping the established pattern... odd that it may be. I'll go ahead and
fix it. (remove all the redundant "host" parameters).

https://codereview.chromium.org/2836313002/diff/80001/ui/views/layout/box_lay...
File ui/views/layout/box_layout.h (right):

https://codereview.chromium.org/2836313002/diff/80001/ui/views/layout/box_lay...
ui/views/layout/box_layout.h:73: bool collapse_margins_spacing);
On 2017/05/09 19:26:03, sky wrote:
> Merge this with other constructor and make collapse_margins_spacing have a
> default value.

I'm wondering if the default should be "true". It has a zero net-effect if there
are no margins set on the individual views.

https://codereview.chromium.org/2836313002/diff/80001/ui/views/layout/box_lay...
ui/views/layout/box_layout.h:127: struct ViewWrapper {
On 2017/05/09 19:26:03, sky wrote:
> Style guide says, "Use a struct only for passive objects that carry data;
> everything else is a class." Given you have a bunch of functions use a class
> here.

Sure thing.

This comes from my notion that "class" is for dynamically allocated objects (ie.
new & delete) and "struct" (or "record" in Delphi) is for stack-based or
inplace-allocated instances. Old habits and all :).

sky

https://codereview.chromium.org/2836313002/diff/80001/ui/views/layout/box_layout.cc File ui/views/layout/box_layout.cc (right): https://codereview.chromium.org/2836313002/diff/80001/ui/views/layout/box_layout.cc#newcode350 ui/views/layout/box_layout.cc:350: void BoxLayout::AdjustCrossAxisForMargin(const ViewWrapper& child, On 2017/05/09 20:07:06, kylix_rd wrote: ...

3 years, 7 months ago (2017-05-09 23:55:29 UTC) #31

https://codereview.chromium.org/2836313002/diff/80001/ui/views/layout/box_lay...
File ui/views/layout/box_layout.cc (right):

https://codereview.chromium.org/2836313002/diff/80001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:350: void
BoxLayout::AdjustCrossAxisForMargin(const ViewWrapper& child,
On 2017/05/09 20:07:06, kylix_rd wrote:
> On 2017/05/09 19:26:03, sky wrote:
> > I don't think this implements what I outlined in comment #6:
> > https://codereview.chromium.org/2836313002/#msg6 . My suggestion there is I
> > think the cross axis margins should be the max of across all children. I say
> > this as to do otherwise means the views aren't going to be aligned nicely on
> the
> > cross axis, they'll be staggered.
> 
> In all modes other than stretch, they'll be staggered anyway. If alignment is
> wanted, cross-axis margins should be set consistently. I was just maintaining
> that "feature" of the layout.

That makes it rather painful to set margins, in so far as you have to be sure to
set them consistently. In an ideal world it seems harmony would set default
margins based on the type (which I thought Peter mentioned the harmony specs
called for) and they would just work. By work I mean you would end up with nice
results, without having to iterate over children and reset the values to be
consistent.

Can you think of a reason why you would not want cross axis margins to
effectively be the same? From what I could see of earlier screenshots it seems
they wanted to use the max as I am asking for here.

https://codereview.chromium.org/2836313002/diff/80001/ui/views/layout/box_lay...
File ui/views/layout/box_layout.h (right):

https://codereview.chromium.org/2836313002/diff/80001/ui/views/layout/box_lay...
ui/views/layout/box_layout.h:73: bool collapse_margins_spacing);
On 2017/05/09 20:07:06, kylix_rd wrote:
> On 2017/05/09 19:26:03, sky wrote:
> > Merge this with other constructor and make collapse_margins_spacing have a
> > default value.
> 
> I'm wondering if the default should be "true". It has a zero net-effect if
there
> are no margins set on the individual views.

I think existing consumers should have to opt into the new behavior. I say that
based on a feeling that harmony would want to set default margins at view
creation time, but I could certainly be wrong there.

kylix_rd

As for the unification of the cross-axis margins, I'll think about that some more. My ...

3 years, 7 months ago (2017-05-10 15:30:37 UTC) #32

kylix_rd

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-10 15:31:14 UTC) #33

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2836313002/140001

3 years, 7 months ago (2017-05-10 15:32:12 UTC) #34

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-10 16:05:56 UTC) #35

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/450164)

3 years, 7 months ago (2017-05-10 16:05:57 UTC) #36

sky

Peter should weigh in on this, but again my thinking is that we will end ...

3 years, 7 months ago (2017-05-10 16:12:18 UTC) #37

Peter should weigh in on this, but again my thinking is that we will end up
in a place where margins are injected based on type and perhaps context. As
the margins are injected they may have different sizes (the mocks Peter
pointed out in the thread that lead to this patch had different cross axis
margins). In this scenario I think the expectation is you use the biggest
cross axis margin, otherwise the layout is not going to look good.

  -Scott

On Wed, May 10, 2017 at 8:30 AM, <kylixrd@chromium.org> wrote:

> As for the unification of the cross-axis margins, I'll think about that
> some
> more. My first thought is that if the cross-axis margins need to be
> consistent,
> then the user can keep the margins on that axis as 0 and then set the
> appropriate inside_border_xxxx_spacing. There is no reason that each side
> of
> margins must be the same.
>
> IMO, because the user can do something wrong with something in some
> instances,
> doesn't mean it shouldn't be allowed in other instances.
>
>
> https://codereview.chromium.org/2836313002/diff/80001/ui/
> views/examples/box_layout_example.cc
> File ui/views/examples/box_layout_example.cc (right):
>
> https://codereview.chromium.org/2836313002/diff/80001/ui/
> views/examples/box_layout_example.cc#newcode31
> ui/views/examples/box_layout_example.cc:31: class FullPanel : public
> View {
> On 2017/05/09 19:26:02, sky wrote:
> > Please add comments for classes. This name is confusing as I would
> naively
> > expect the children to be given the full bounds, which isn't the case.
> Also,
> > more often than not we use View in the name rather than panel.
>
> Done.
>
> https://codereview.chromium.org/2836313002/diff/80001/ui/
> views/examples/box_layout_example.cc#newcode36
> ui/views/examples/box_layout_example.cc:36: void Layout() override;
> On 2017/05/09 19:26:03, sky wrote:
> > Prefix with where override comes from.
>
> Done.
>
> https://codereview.chromium.org/2836313002/diff/80001/ui/
> views/examples/box_layout_example.cc#newcode44
> ui/views/examples/box_layout_example.cc:44: ChildPanel(BoxLayoutExample*
> example);
> On 2017/05/09 19:26:02, sky wrote:
> > explicit.
>
> Done.
>
> https://codereview.chromium.org/2836313002/diff/80001/ui/
> views/examples/box_layout_example.cc#newcode74
> ui/views/examples/box_layout_example.cc:74: class ExampleModel : public
> ui::ComboboxModel {
> On 2017/05/09 19:26:03, sky wrote:
> > Can you use ui/views/examples/example_combobox_model.h ?
>
> Yep. I didn't notice it was there.
>
> Done.
>
> https://codereview.chromium.org/2836313002/diff/80001/ui/
> views/examples/box_layout_example.h
> File ui/views/examples/box_layout_example.h (right):
>
> https://codereview.chromium.org/2836313002/diff/80001/ui/
> views/examples/box_layout_example.h#newcode25
> ui/views/examples/box_layout_example.h:25: class ViewListener {
> On 2017/05/09 19:26:03, sky wrote:
> > This name is overly generic. How about a more specific name such as
> > BoxLayoutExampleListener?
>
> Upon further review, this isn't necessary at this time.
>
> https://codereview.chromium.org/2836313002/diff/80001/ui/
> views/examples/box_layout_example.h#newcode63
> ui/views/examples/box_layout_example.h:63: void RefreshLayoutPanel();
> On 2017/05/09 19:26:03, sky wrote:
> > moooaaarrrr comments please!
>
> Done.
>
> https://codereview.chromium.org/2836313002/diff/80001/ui/
> views/layout/box_layout.h
> File ui/views/layout/box_layout.h (right):
>
> https://codereview.chromium.org/2836313002/diff/80001/ui/
> views/layout/box_layout.h#newcode73
> ui/views/layout/box_layout.h:73: bool collapse_margins_spacing);
> On 2017/05/09 19:26:03, sky wrote:
> > Merge this with other constructor and make collapse_margins_spacing
> have a
> > default value.
>
> Done.
>
> https://codereview.chromium.org/2836313002/diff/80001/ui/
> views/layout/box_layout.h#newcode127
> ui/views/layout/box_layout.h:127: struct ViewWrapper {
> On 2017/05/09 19:26:03, sky wrote:
> > Style guide says, "Use a struct only for passive objects that carry
> data;
> > everything else is a class." Given you have a bunch of functions use a
> class
> > here.
>
> Done.
>
> https://codereview.chromium.org/2836313002/diff/80001/ui/
> views/layout/box_layout.h#newcode235
> ui/views/layout/box_layout.h:235: bool collapse_margins_spacing_;
> On 2017/05/09 19:26:03, sky wrote:
> > const
>
> Done.
>
> https://codereview.chromium.org/2836313002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

kylix_rd

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-10 17:09:14 UTC) #39

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2836313002/160001

3 years, 7 months ago (2017-05-10 17:10:50 UTC) #40

Peter Kasting

It seems like there are generally two routes to go with most of the cross-axis ...

3 years, 7 months ago (2017-05-10 18:07:36 UTC) #41

It seems like there are generally two routes to go with most of the cross-axis
alignment values:

(1) Position/size the views against each other ignoring their margins; then tack
on the margins and fit the container around the result.
(2) Size the container based on overall view + margin sizes, then position/size
each view against the container.

For example, assume you have views like:

MMMM  VVVV  MMMM
VVVV        MMMM
VVVV        VVVV
VVVV
MMMM

(Here we have three horizontally-laid-out views VVVV, with vertical margins
MMMM.)

Here are the alignments that result from these different methods:

STRETCH (1):       STRETCH (2):
--------------     --------------
          MMMM     MMMM VVVV MMMM
MMMM      MMMM     VVVV VVVV MMMM
VVVV VVVV VVVV     VVVV VVVV VVVV
VVVV VVVV VVVV     VVVV VVVV VVVV
VVVV VVVV VVVV     MMMM VVVV VVVV
MMMM               --------------
--------------


START (1):         START (2):
--------------     --------------
          MMMM     MMMM VVVV MMMM
MMMM      MMMM     VVVV      MMMM
VVVV VVVV VVVV     VVVV      VVVV
VVVV               VVVV
VVVV               MMMM
MMMM               --------------
--------------


CENTER (1) or (2):*
--------------
MMMM      MMMM    
VVVV      MMMM
VVVV VVVV VVVV
VVVV
MMMM
--------------


END (1):           END (2):
--------------     --------------
MMMM               MMMM
VVVV      MMMM     VVVV
VVVV      MMMM     VVVV      MMMM
VVVV VVVV VVVV     VVVV      MMMM
MMMM               MMMM VVVV VVVV
--------------     --------------


*I think methods (1) and (2) always produce the same results for CENTER. 
However, there's a separate question: when top and bottom margins are unequal,
do we fit the container tightly around the views + margins, or do we potentially
expand its height as needed so the view centerline can be on the container
centerline?  Consider a simple two-view case for example:

Fit tightly:  Expand height:
---------     ---------
MMMM          MMMM
VVVV VVVV     VVVV VVVV
---------
              ---------


Looking at all these, I think there are cases where one might want each; but
overall I think route (1) makes more sense.

Does that answer the question?

kylix_rd

Based on Peter's highly detailed and epic ASCII art illustration, this patch implements (1). It ...

3 years, 7 months ago (2017-05-10 20:35:19 UTC) #42

kylix_rd

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-10 20:36:11 UTC) #43

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2836313002/180001

3 years, 7 months ago (2017-05-10 20:37:13 UTC) #44

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-10 22:23:08 UTC) #45

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/290657)

3 years, 7 months ago (2017-05-10 22:23:09 UTC) #46

kylix_rd

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-11 19:52:24 UTC) #47

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2836313002/200001

3 years, 7 months ago (2017-05-11 19:53:33 UTC) #48

kylix_rd

This includes some unit tests which will ensure margins are taken into account and/or collapsed ...

3 years, 7 months ago (2017-05-11 19:59:53 UTC) #49

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-11 20:49:17 UTC) #50

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/176308)

3 years, 7 months ago (2017-05-11 20:49:18 UTC) #51

sky

Peter, I stared at your ASCII art for a good ten minutes and couldn't make ...

3 years, 7 months ago (2017-05-12 14:03:10 UTC) #52

Peter,

I stared at your ASCII art for a good ten minutes and couldn't make heads
or tails of it. Specifically you said there are three horizontally laid out
views (VVVV), yet I see more:

MMMM VVVV MMMM
VVVV MMMM
VVVV VVVV
VVVV
MMMM

I don't understand what this means. If the views are laid out horizontally
how can you have two views in column two? No doubt I'm reading this wrong.

I feel like this would be a lot easier to resolve in front of a whiteboard.
I'm in Waterloo until Tuesday and back in the MTV office Wednesday. Could
this wait until then?

   -Scott


On Wed, May 10, 2017 at 11:07 AM, <pkasting@chromium.org> wrote:

> It seems like there are generally two routes to go with most of the
> cross-axis
> alignment values:
>
> (1) Position/size the views against each other ignoring their margins;
> then tack
> on the margins and fit the container around the result.
> (2) Size the container based on overall view + margin sizes, then
> position/size
> each view against the container.
>
> For example, assume you have views like:
>
> MMMM VVVV MMMM
> VVVV MMMM
> VVVV VVVV
> VVVV
> MMMM
>
> (Here we have three horizontally-laid-out views VVVV, with vertical margins
> MMMM.)
>
> Here are the alignments that result from these different methods:
>
> STRETCH (1): STRETCH (2):
> -------------- --------------
> MMMM MMMM VVVV MMMM
> MMMM MMMM VVVV VVVV MMMM
> VVVV VVVV VVVV VVVV VVVV VVVV
> VVVV VVVV VVVV VVVV VVVV VVVV
> VVVV VVVV VVVV MMMM VVVV VVVV
> MMMM --------------
> --------------
>
>
> START (1): START (2):
> -------------- --------------
> MMMM MMMM VVVV MMMM
> MMMM MMMM VVVV MMMM
> VVVV VVVV VVVV VVVV VVVV
> VVVV VVVV
> VVVV MMMM
> MMMM --------------
> --------------
>
>
> CENTER (1) or (2):*
> --------------
> MMMM MMMM
> VVVV MMMM
> VVVV VVVV VVVV
> VVVV
> MMMM
> --------------
>
>
> END (1): END (2):
> -------------- --------------
> MMMM MMMM
> VVVV MMMM VVVV
> VVVV MMMM VVVV MMMM
> VVVV VVVV VVVV VVVV MMMM
> MMMM MMMM VVVV VVVV
> -------------- --------------
>
>
> *I think methods (1) and (2) always produce the same results for CENTER.
> However, there's a separate question: when top and bottom margins are
> unequal,
> do we fit the container tightly around the views + margins, or do we
> potentially
> expand its height as needed so the view centerline can be on the container
> centerline? Consider a simple two-view case for example:
>
> Fit tightly: Expand height:
> --------- ---------
> MMMM MMMM
> VVVV VVVV VVVV VVVV
> ---------
> ---------
>
>
> Looking at all these, I think there are cases where one might want each;
> but
> overall I think route (1) makes more sense.
>
> Does that answer the question?
>
> https://codereview.chromium.org/2836313002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

kylix_rd

On 2017/05/12 14:03:10, sky wrote: > Peter, > > I stared at your ASCII art ...

3 years, 7 months ago (2017-05-12 15:25:53 UTC) #53

sky

On 2017/05/12 15:25:53, kylix_rd wrote: > On 2017/05/12 14:03:10, sky wrote: > > Peter, > ...

3 years, 7 months ago (2017-05-12 19:41:43 UTC) #54

On 2017/05/12 15:25:53, kylix_rd wrote:
> On 2017/05/12 14:03:10, sky wrote:
> > Peter,
> > 
> > I stared at your ASCII art for a good ten minutes and couldn't make heads
> > or tails of it. Specifically you said there are three horizontally laid out
> > views (VVVV), yet I see more:
> > 
> > MMMM VVVV MMMM
> > VVVV MMMM
> > VVVV VVVV
> > VVVV
> > MMMM
> > 
> > I don't understand what this means. If the views are laid out horizontally
> > how can you have two views in column two? No doubt I'm reading this wrong.
> > 
> > I feel like this would be a lot easier to resolve in front of a whiteboard.
> > I'm in Waterloo until Tuesday and back in the MTV office Wednesday. Could
> > this wait until then?
> > 
> >    -Scott
> 
> If you view it on the review page itself it looks fine. His post relied on the
> fixed pitch font used for the review app.

I'm advocating for something like 1, but not quite. In words the margins are
*always* fixed to the max of the margins of each view (along the cross axis).

START (1 and 3):   START (2):
--------------     --------------
          MMMM     MMMM VVVV MMMM
MMMM      MMMM     VVVV      MMMM
VVVV VVVV VVVV     VVVV      VVVV
VVVV               VVVV
VVVV               MMMM
MMMM               --------------
--------------


CENTER (1) or (2):* CENTER (3):
--------------    --------------
MMMM      MMMM              MMMM
VVVV      MMMM    MMMM      MMMM
VVVV VVVV VVVV    VVVV
VVVV              VVVV VVVV VVVV
MMMM              VVVV
--------------    MMMM
                  --------------

END (1):           END (2):        END (3):
--------------     --------------  --------------
MMMM               MMMM                      MMMM
VVVV      MMMM     VVVV            MMMM      MMMM
VVVV      MMMM     VVVV      MMMM  VVVV
VVVV VVVV VVVV     VVVV      MMMM  VVVV
MMMM               MMMM VVVV VVVV  VVVV VVVV VVVV
--------------     --------------  MMMM
                                   --------------

Why do this? I believe it produces the result best matching what a designer
would want and the most visually pleasing.

Peter Kasting

On 2017/05/12 19:41:43, sky wrote: > I'm advocating for something like 1, but not quite. ...

3 years, 7 months ago (2017-05-12 19:59:16 UTC) #55

On 2017/05/12 19:41:43, sky wrote:
> I'm advocating for something like 1, but not quite. In words the margins are
> *always* fixed to the max of the margins of each view (along the cross axis).
> 
> START (1 and 3):   START (2):
> --------------     --------------
>           MMMM     MMMM VVVV MMMM
> MMMM      MMMM     VVVV      MMMM
> VVVV VVVV VVVV     VVVV      VVVV
> VVVV               VVVV
> VVVV               MMMM
> MMMM               --------------
> --------------
> 
> 
> CENTER (1) or (2):* CENTER (3):
> --------------    --------------
> MMMM      MMMM              MMMM
> VVVV      MMMM    MMMM      MMMM
> VVVV VVVV VVVV    VVVV
> VVVV              VVVV VVVV VVVV
> MMMM              VVVV
> --------------    MMMM
>                   --------------
> 
> END (1):           END (2):        END (3):
> --------------     --------------  --------------
> MMMM               MMMM                      MMMM
> VVVV      MMMM     VVVV            MMMM      MMMM
> VVVV      MMMM     VVVV      MMMM  VVVV
> VVVV VVVV VVVV     VVVV      MMMM  VVVV
> MMMM               MMMM VVVV VVVV  VVVV VVVV VVVV
> --------------     --------------  MMMM
>                                    --------------
> 
> Why do this? I believe it produces the result best matching what a designer
> would want and the most visually pleasing.

Hmm.  I understand what you're asking for, but to me this would make less sense
and be less practically useful.

Basically, I think of a "margin" as saying "my view needs at least X space on
this side".  Route (1) alone already guarantees this.

Route (3) is basically saying, let's create a "margin row" above and below the
views, sized to the max margin heights.  Effectively, this is a way of saying
that view 3's margin dictates how much space is above view 1 _even when view 3
doesn't need that space_.  That seems strange.  And practically, this sort of
thing can be accomplished with gridlayout/nested layouts today much more easily
(I think) than route (1) can be accomplished today, so it adds less value to do
this.

I don't know if there are examples of margins in CSS or something that would
suggest how other developers might expect margins to behave; if so that'd be
another data point here.

sky

In earlier email threads I thought we wanted to head toward a place where views ...

3 years, 7 months ago (2017-05-14 14:04:25 UTC) #56

In earlier email threads I thought we wanted to head toward a place where
views automatically get margins at some point in their lifetime. The margin
is determined based on type and perhaps context. This comes from the email
thread "Adding a margin to a view." If you agree with this, then the place
creating the views (infobar code in your case), isn't explicitly setting
the margins. In such a scenario it's all too easy to end up with different
cross axis margins. Using different cross axis margins will result in a bad
layout. All of the examples I see at
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(...
have the same cross axis margin. This is why I think the cross axis margins
should be the max.

Do you disagree with the path I've outlined? Can you think of places where
you would actually want different cross axis margins?

I've outlined my reasoning for the route I think this code should go, but
at the end of the day you guys are actually using it. So, if my latest
comments aren't enough to convince you, then we go with route 1. I'll wait
for a response and then we'll take it from there.

  -Scott

On Fri, May 12, 2017 at 12:59 PM, <pkasting@chromium.org> wrote:

> On 2017/05/12 19:41:43, sky wrote:
> > I'm advocating for something like 1, but not quite. In words the margins
> are
> > *always* fixed to the max of the margins of each view (along the cross
> axis).
> >
> > START (1 and 3): START (2):
> > -------------- --------------
> > MMMM MMMM VVVV MMMM
> > MMMM MMMM VVVV MMMM
> > VVVV VVVV VVVV VVVV VVVV
> > VVVV VVVV
> > VVVV MMMM
> > MMMM --------------
> > --------------
> >
> >
> > CENTER (1) or (2):* CENTER (3):
> > -------------- --------------
> > MMMM MMMM MMMM
> > VVVV MMMM MMMM MMMM
> > VVVV VVVV VVVV VVVV
> > VVVV VVVV VVVV VVVV
> > MMMM VVVV
> > -------------- MMMM
> > --------------
> >
> > END (1): END (2): END (3):
> > -------------- -------------- --------------
> > MMMM MMMM MMMM
> > VVVV MMMM VVVV MMMM MMMM
> > VVVV MMMM VVVV MMMM VVVV
> > VVVV VVVV VVVV VVVV MMMM VVVV
> > MMMM MMMM VVVV VVVV VVVV VVVV VVVV
> > -------------- -------------- MMMM
> > --------------
> >
> > Why do this? I believe it produces the result best matching what a
> designer
> > would want and the most visually pleasing.
>
> Hmm. I understand what you're asking for, but to me this would make less
> sense
> and be less practically useful.
>
> Basically, I think of a "margin" as saying "my view needs at least X space
> on
> this side". Route (1) alone already guarantees this.
>
> Route (3) is basically saying, let's create a "margin row" above and below
> the
> views, sized to the max margin heights. Effectively, this is a way of
> saying
> that view 3's margin dictates how much space is above view 1 _even when
> view 3
> doesn't need that space_. That seems strange. And practically, this sort of
> thing can be accomplished with gridlayout/nested layouts today much more
> easily
> (I think) than route (1) can be accomplished today, so it adds less value
> to do
> this.
>
> I don't know if there are examples of margins in CSS or something that
> would
> suggest how other developers might expect margins to behave; if so that'd
> be
> another data point here.
>
> https://codereview.chromium.org/2836313002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Peter Kasting

On 2017/05/14 14:04:25, sky wrote: > In earlier email threads I thought we wanted to ...

3 years, 7 months ago (2017-05-15 17:06:29 UTC) #57

sky

(3) it is. Will review shortly. On Mon, May 15, 2017 at 10:06 AM, <pkasting@chromium.org> ...

3 years, 7 months ago (2017-05-15 18:18:17 UTC) #58

(3) it is. Will review shortly.

On Mon, May 15, 2017 at 10:06 AM, <pkasting@chromium.org> wrote:

> On 2017/05/14 14:04:25, sky wrote:
> > In earlier email threads I thought we wanted to head toward a place where
> > views automatically get margins at some point in their lifetime. The
> margin
> > is determined based on type and perhaps context. This comes from the
> email
> > thread "Adding a margin to a view." If you agree with this, then the
> place
> > creating the views (infobar code in your case), isn't explicitly setting
> > the margins. In such a scenario it's all too easy to end up with
> different
> > cross axis margins. Using different cross axis margins will result in a
> bad
> > layout. All of the examples I see at
> >
> https://folio.googleplex.com/chrome-ux-specs-and-sources/
> Chrome%20browser%20(MD)/Secondary%20UI%20Previews%
> 20and%20specs%20(exports)/Spec#%2FSPEC-secondary-UI-07a-
> layout.png%3Ff=hidden
> > have the same cross axis margin. This is why I think the cross axis
> margins
> > should be the max.
> >
> > Do you disagree with the path I've outlined? Can you think of places
> where
> > you would actually want different cross axis margins?
>
> I actually hope to implement the spec using different cross-axis margins
> with
> centered layout, and if I do so, then route (3) will break things.
> Consider the
> zoom bubble case in the "toasts with buttons" mock: if I stick a 12 pt top
> and
> bottom margin on the label and 8 pt top and bottom margins on the buttons,
> and
> then center, then route (1) and (2) give me what I want, but route (3)
> gives me
> 12 pt margins outside the buttons.
>
> I could choose to implement differently, but the goal of this
> implementation
> path was that the zoom bubble not need to care what the child margins are,
> and
> the children not need to know which kinds of siblings they have.
>
> https://codereview.chromium.org/2836313002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

sky

GAH! Sorry, that should be (1) it is. Will review shortly. On Mon, May 15, ...

3 years, 7 months ago (2017-05-15 18:18:34 UTC) #59

GAH! Sorry, that should be (1) it is. Will review shortly.

On Mon, May 15, 2017 at 11:18 AM, Scott Violet <sky@chromium.org> wrote:

> (3) it is. Will review shortly.
>
> On Mon, May 15, 2017 at 10:06 AM, <pkasting@chromium.org> wrote:
>
>> On 2017/05/14 14:04:25, sky wrote:
>> > In earlier email threads I thought we wanted to head toward a place
>> where
>> > views automatically get margins at some point in their lifetime. The
>> margin
>> > is determined based on type and perhaps context. This comes from the
>> email
>> > thread "Adding a margin to a view." If you agree with this, then the
>> place
>> > creating the views (infobar code in your case), isn't explicitly setting
>> > the margins. In such a scenario it's all too easy to end up with
>> different
>> > cross axis margins. Using different cross axis margins will result in a
>> bad
>> > layout. All of the examples I see at
>> >
>> https://folio.googleplex.com/chrome-ux-specs-and-sources/Chr
>> ome%20browser%20(MD)/Secondary%20UI%20Previews%20and%
>> 20specs%20(exports)/Spec#%2FSPEC-secondary-UI-07a-layout.png%3Ff=hidden
>> > have the same cross axis margin. This is why I think the cross axis
>> margins
>> > should be the max.
>> >
>> > Do you disagree with the path I've outlined? Can you think of places
>> where
>> > you would actually want different cross axis margins?
>>
>> I actually hope to implement the spec using different cross-axis margins
>> with
>> centered layout, and if I do so, then route (3) will break things.
>> Consider the
>> zoom bubble case in the "toasts with buttons" mock: if I stick a 12 pt
>> top and
>> bottom margin on the label and 8 pt top and bottom margins on the
>> buttons, and
>> then center, then route (1) and (2) give me what I want, but route (3)
>> gives me
>> 12 pt margins outside the buttons.
>>
>> I could choose to implement differently, but the goal of this
>> implementation
>> path was that the zoom bubble not need to care what the child margins
>> are, and
>> the children not need to know which kinds of siblings they have.
>>
>> https://codereview.chromium.org/2836313002/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

sky

On 2017/05/15 18:18:34, sky wrote: > GAH! Sorry, that should be (1) it is. Will ...

3 years, 7 months ago (2017-05-15 20:28:24 UTC) #60

On 2017/05/15 18:18:34, sky wrote:
> GAH! Sorry, that should be (1) it is. Will review shortly.
> 
> On Mon, May 15, 2017 at 11:18 AM, Scott Violet <mailto:sky@chromium.org>
wrote:
> 
> > (3) it is. Will review shortly.
> >
> > On Mon, May 15, 2017 at 10:06 AM, <mailto:pkasting@chromium.org> wrote:
> >
> >> On 2017/05/14 14:04:25, sky wrote:
> >> > In earlier email threads I thought we wanted to head toward a place
> >> where
> >> > views automatically get margins at some point in their lifetime. The
> >> margin
> >> > is determined based on type and perhaps context. This comes from the
> >> email
> >> > thread "Adding a margin to a view." If you agree with this, then the
> >> place
> >> > creating the views (infobar code in your case), isn't explicitly setting
> >> > the margins. In such a scenario it's all too easy to end up with
> >> different
> >> > cross axis margins. Using different cross axis margins will result in a
> >> bad
> >> > layout. All of the examples I see at
> >> >
> >> https://folio.googleplex.com/chrome-ux-specs-and-sources/Chr
> >> ome%20browser%20(MD)/Secondary%20UI%20Previews%20and%
> >> 20specs%20(exports)/Spec#%2FSPEC-secondary-UI-07a-layout.png%3Ff=hidden
> >> > have the same cross axis margin. This is why I think the cross axis
> >> margins
> >> > should be the max.
> >> >
> >> > Do you disagree with the path I've outlined? Can you think of places
> >> where
> >> > you would actually want different cross axis margins?
> >>
> >> I actually hope to implement the spec using different cross-axis margins
> >> with
> >> centered layout, and if I do so, then route (3) will break things.
> >> Consider the
> >> zoom bubble case in the "toasts with buttons" mock: if I stick a 12 pt
> >> top and
> >> bottom margin on the label and 8 pt top and bottom margins on the
> >> buttons, and
> >> then center, then route (1) and (2) give me what I want, but route (3)
> >> gives me
> >> 12 pt margins outside the buttons.
> >>
> >> I could choose to implement differently, but the goal of this
> >> implementation
> >> path was that the zoom bubble not need to care what the child margins
> >> are, and
> >> the children not need to know which kinds of siblings they have.
> >>
> >> https://codereview.chromium.org/2836313002/
> >>
> >
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

I started reviewing again, and realized I'm confused. I think Allen implemented
(3), which for the reasons I outlined SGTM, but it doesn't match what you want.

Looking back at your ascii art I realized something: Notice the following have
different heights:

START (1):
--------------
          MMMM
MMMM      MMMM
VVVV VVVV VVVV
VVVV
VVVV
MMMM
--------------

CENTER (1)
--------------
MMMM      MMMM
VVVV      MMMM
VVVV VVVV VVVV
VVVV
MMMM
--------------

In start the height is 6, but in center the height is 5. If use an algorithm
like the following to get the preferred height (which is something like Allen
has in the patch, although his is in two passes):

for (view in views) {
  margin_height = max(margin_height, view->margin->height());
  pref_height = max(pref_height, view->GetPreferredSize().height());
}
return margin_height + pref_height;

Then the height of what the 3 views you list is going to be 6. Why would center
be different?

But maybe you want an algorithm like:
for (view in views) {
  pref_height = max(pref_height, view->GetPreferredSize().height() +
view->margin().height());
}
return pref_height;

But if you do that, then the start case becomes:

MMMM VVVV MMMM
VVVV      MMMM
VVVV      VVVV
VVVV
MMMM

sky

I got part way through this before realizing what you implemented isn't what Peter wants. ...

3 years, 7 months ago (2017-05-15 20:29:10 UTC) #61

Peter Kasting

On 2017/05/15 20:28:24, sky wrote: > On 2017/05/15 18:18:34, sky wrote: > > GAH! Sorry, ...

3 years, 7 months ago (2017-05-15 21:15:33 UTC) #62

On 2017/05/15 20:28:24, sky wrote:
> On 2017/05/15 18:18:34, sky wrote:
> > GAH! Sorry, that should be (1) it is. Will review shortly.
> > 
> > On Mon, May 15, 2017 at 11:18 AM, Scott Violet <mailto:sky@chromium.org>
> wrote:
> > 
> > > (3) it is. Will review shortly.
> > >
> > > On Mon, May 15, 2017 at 10:06 AM, <mailto:pkasting@chromium.org> wrote:
> > >
> > >> On 2017/05/14 14:04:25, sky wrote:
> > >> > In earlier email threads I thought we wanted to head toward a place
> > >> where
> > >> > views automatically get margins at some point in their lifetime. The
> > >> margin
> > >> > is determined based on type and perhaps context. This comes from the
> > >> email
> > >> > thread "Adding a margin to a view." If you agree with this, then the
> > >> place
> > >> > creating the views (infobar code in your case), isn't explicitly
setting
> > >> > the margins. In such a scenario it's all too easy to end up with
> > >> different
> > >> > cross axis margins. Using different cross axis margins will result in a
> > >> bad
> > >> > layout. All of the examples I see at
> > >> >
> > >> https://folio.googleplex.com/chrome-ux-specs-and-sources/Chr
> > >> ome%20browser%20(MD)/Secondary%20UI%20Previews%20and%
> > >> 20specs%20(exports)/Spec#%2FSPEC-secondary-UI-07a-layout.png%3Ff=hidden
> > >> > have the same cross axis margin. This is why I think the cross axis
> > >> margins
> > >> > should be the max.
> > >> >
> > >> > Do you disagree with the path I've outlined? Can you think of places
> > >> where
> > >> > you would actually want different cross axis margins?
> > >>
> > >> I actually hope to implement the spec using different cross-axis margins
> > >> with
> > >> centered layout, and if I do so, then route (3) will break things.
> > >> Consider the
> > >> zoom bubble case in the "toasts with buttons" mock: if I stick a 12 pt
> > >> top and
> > >> bottom margin on the label and 8 pt top and bottom margins on the
> > >> buttons, and
> > >> then center, then route (1) and (2) give me what I want, but route (3)
> > >> gives me
> > >> 12 pt margins outside the buttons.
> > >>
> > >> I could choose to implement differently, but the goal of this
> > >> implementation
> > >> path was that the zoom bubble not need to care what the child margins
> > >> are, and
> > >> the children not need to know which kinds of siblings they have.
> > >>
> > >> https://codereview.chromium.org/2836313002/
> > >>
> > >
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> I started reviewing again, and realized I'm confused. I think Allen
implemented
> (3), which for the reasons I outlined SGTM, but it doesn't match what you
want.
> 
> Looking back at your ascii art I realized something: Notice the following have
> different heights:
> 
> START (1):
> --------------
>           MMMM
> MMMM      MMMM
> VVVV VVVV VVVV
> VVVV
> VVVV
> MMMM
> --------------
> 
> CENTER (1)
> --------------
> MMMM      MMMM
> VVVV      MMMM
> VVVV VVVV VVVV
> VVVV
> MMMM
> --------------
> 
> In start the height is 6, but in center the height is 5.

Correct.

> If use an algorithm
> like the following to get the preferred height (which is something like Allen
> has in the patch, although his is in two passes):
> 
> for (view in views) {
>   margin_height = max(margin_height, view->margin->height());
>   pref_height = max(pref_height, view->GetPreferredSize().height());
> }
> return margin_height + pref_height;
> 
> Then the height of what the 3 views you list is going to be 6.

I believe that's closest to what you'd want for (3), not (1) or (2).

> But maybe you want an algorithm like:
> for (view in views) {
>   pref_height = max(pref_height, view->GetPreferredSize().height() +
> view->margin().height());
> }
> return pref_height;

I believe that's more what you want for (2).

> But if you do that, then the start case becomes:
> 
> MMMM VVVV MMMM
> VVVV      MMMM
> VVVV      VVVV
> VVVV
> MMMM

Right, that was indeed my drawing for (2).

The way to compute height for (1) would be to compute it after both sizing and
positioning all the views relative to each other.  You can't easily compute the
container height without doing full layout.

The flow for (1) is basically:

* Size/position views against each other ignoring margins
* Attach margins to cross axis of views
* Draw a box around the outside of everything.  That's your preferred size

So for the center case, we would start with our three views:

VVVV VVVV VVVV
VVVV
VVVV

And we would center them against each other:

VVVV
VVVV VVVV VVVV
VVVV

(At this point, we've basically executed the pre-margin-support layout routine.)

Then we would attach the margins to each view:

MMMM      MMMM
VVVV      MMMM
VVVV VVVV VVVV
VVVV
MMMM

And then we would draw the container around them:

--------------
MMMM      MMMM
VVVV      MMMM
VVVV VVVV VVVV
VVVV
MMMM
--------------

And now we would know where the local 0 coordinate is (the top of the container)
and what the height is.

sky

On 2017/05/15 21:15:33, Peter Kasting wrote: > On 2017/05/15 20:28:24, sky wrote: > > On ...

3 years, 7 months ago (2017-05-15 21:44:53 UTC) #63

On 2017/05/15 21:15:33, Peter Kasting wrote:
> On 2017/05/15 20:28:24, sky wrote:
> > On 2017/05/15 18:18:34, sky wrote:
> > > GAH! Sorry, that should be (1) it is. Will review shortly.
> > > 
> > > On Mon, May 15, 2017 at 11:18 AM, Scott Violet <mailto:sky@chromium.org>
> > wrote:
> > > 
> > > > (3) it is. Will review shortly.
> > > >
> > > > On Mon, May 15, 2017 at 10:06 AM, <mailto:pkasting@chromium.org> wrote:
> > > >
> > > >> On 2017/05/14 14:04:25, sky wrote:
> > > >> > In earlier email threads I thought we wanted to head toward a place
> > > >> where
> > > >> > views automatically get margins at some point in their lifetime. The
> > > >> margin
> > > >> > is determined based on type and perhaps context. This comes from the
> > > >> email
> > > >> > thread "Adding a margin to a view." If you agree with this, then the
> > > >> place
> > > >> > creating the views (infobar code in your case), isn't explicitly
> setting
> > > >> > the margins. In such a scenario it's all too easy to end up with
> > > >> different
> > > >> > cross axis margins. Using different cross axis margins will result in
a
> > > >> bad
> > > >> > layout. All of the examples I see at
> > > >> >
> > > >> https://folio.googleplex.com/chrome-ux-specs-and-sources/Chr
> > > >> ome%20browser%20(MD)/Secondary%20UI%20Previews%20and%
> > > >> 20specs%20(exports)/Spec#%2FSPEC-secondary-UI-07a-layout.png%3Ff=hidden
> > > >> > have the same cross axis margin. This is why I think the cross axis
> > > >> margins
> > > >> > should be the max.
> > > >> >
> > > >> > Do you disagree with the path I've outlined? Can you think of places
> > > >> where
> > > >> > you would actually want different cross axis margins?
> > > >>
> > > >> I actually hope to implement the spec using different cross-axis
margins
> > > >> with
> > > >> centered layout, and if I do so, then route (3) will break things.
> > > >> Consider the
> > > >> zoom bubble case in the "toasts with buttons" mock: if I stick a 12 pt
> > > >> top and
> > > >> bottom margin on the label and 8 pt top and bottom margins on the
> > > >> buttons, and
> > > >> then center, then route (1) and (2) give me what I want, but route (3)
> > > >> gives me
> > > >> 12 pt margins outside the buttons.
> > > >>
> > > >> I could choose to implement differently, but the goal of this
> > > >> implementation
> > > >> path was that the zoom bubble not need to care what the child margins
> > > >> are, and
> > > >> the children not need to know which kinds of siblings they have.
> > > >>
> > > >> https://codereview.chromium.org/2836313002/
> > > >>
> > > >
> > > >
> > > 
> > > -- 
> > > You received this message because you are subscribed to the Google Groups
> > > "Chromium-reviews" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > 
> > I started reviewing again, and realized I'm confused. I think Allen
> implemented
> > (3), which for the reasons I outlined SGTM, but it doesn't match what you
> want.
> > 
> > Looking back at your ascii art I realized something: Notice the following
have
> > different heights:
> > 
> > START (1):
> > --------------
> >           MMMM
> > MMMM      MMMM
> > VVVV VVVV VVVV
> > VVVV
> > VVVV
> > MMMM
> > --------------
> > 
> > CENTER (1)
> > --------------
> > MMMM      MMMM
> > VVVV      MMMM
> > VVVV VVVV VVVV
> > VVVV
> > MMMM
> > --------------
> > 
> > In start the height is 6, but in center the height is 5.
> 
> Correct.
> 
> > If use an algorithm
> > like the following to get the preferred height (which is something like
Allen
> > has in the patch, although his is in two passes):
> > 
> > for (view in views) {
> >   margin_height = max(margin_height, view->margin->height());
> >   pref_height = max(pref_height, view->GetPreferredSize().height());
> > }
> > return margin_height + pref_height;
> > 
> > Then the height of what the 3 views you list is going to be 6.
> 
> I believe that's closest to what you'd want for (3), not (1) or (2).
> 
> > But maybe you want an algorithm like:
> > for (view in views) {
> >   pref_height = max(pref_height, view->GetPreferredSize().height() +
> > view->margin().height());
> > }
> > return pref_height;
> 
> I believe that's more what you want for (2).
> 
> > But if you do that, then the start case becomes:
> > 
> > MMMM VVVV MMMM
> > VVVV      MMMM
> > VVVV      VVVV
> > VVVV
> > MMMM
> 
> Right, that was indeed my drawing for (2).
> 
> The way to compute height for (1) would be to compute it after both sizing and
> positioning all the views relative to each other.  You can't easily compute
the
> container height without doing full layout.
> 
> The flow for (1) is basically:
> 
> * Size/position views against each other ignoring margins
> * Attach margins to cross axis of views
> * Draw a box around the outside of everything.  That's your preferred size
> 
> So for the center case, we would start with our three views:
> 
> VVVV VVVV VVVV
> VVVV
> VVVV
> 
> And we would center them against each other:
> 
> VVVV
> VVVV VVVV VVVV
> VVVV
> 
> (At this point, we've basically executed the pre-margin-support layout
routine.)
> 
> Then we would attach the margins to each view:

By attach margin is it something like you're subtracting the top-margin from the
y-coordinate in the last pass, accumulating the max margin, and then loop over
the views again offseting the max?

max_top = 0;
for (view in views) {
  max_top = max(max_top, view->margin().top());
  view->SetBounds(... view->bounds().y() - view->margin().top() ...);
}

And then you make a pass offsetting by max_top?
for (view in views) {
  view->SetBounds(... view->bounds().y() + max_top ...);
}

I'm not suggesting the actual code set the bounds multiple times, it's just
easiest illustrate.



> MMMM      MMMM
> VVVV      MMMM
> VVVV VVVV VVVV
> VVVV
> MMMM
> 
> And then we would draw the container around them:
> 
> --------------
> MMMM      MMMM
> VVVV      MMMM
> VVVV VVVV VVVV
> VVVV
> MMMM
> --------------
> 
> And now we would know where the local 0 coordinate is (the top of the
container)
> and what the height is.

Peter Kasting

On 2017/05/15 21:44:53, sky wrote: > On 2017/05/15 21:15:33, Peter Kasting wrote: > > Then ...

3 years, 7 months ago (2017-05-15 21:53:03 UTC) #64

kylix_rd

On 2017/05/15 21:53:03, Peter Kasting wrote: > On 2017/05/15 21:44:53, sky wrote: > > On ...

3 years, 7 months ago (2017-05-16 13:34:01 UTC) #65

kylix_rd

On 2017/05/16 13:34:01, kylix_rd wrote: > On 2017/05/15 21:53:03, Peter Kasting wrote: > > On ...

3 years, 7 months ago (2017-05-16 16:21:35 UTC) #66

On 2017/05/16 13:34:01, kylix_rd wrote:
> On 2017/05/15 21:53:03, Peter Kasting wrote:
> > On 2017/05/15 21:44:53, sky wrote:
> > > On 2017/05/15 21:15:33, Peter Kasting wrote:
> > > > Then we would attach the margins to each view:
> > > 
> > > By attach margin is it something like you're subtracting the top-margin
from
> > the
> > > y-coordinate in the last pass, accumulating the max margin, and then loop
> over
> > > the views again offseting the max?
> > > 
> > > max_top = 0;
> > > for (view in views) {
> > >   max_top = max(max_top, view->margin().top());
> > >   view->SetBounds(... view->bounds().y() - view->margin().top() ...);
> > > }
> > > 
> > > And then you make a pass offsetting by max_top?
> > > for (view in views) {
> > >   view->SetBounds(... view->bounds().y() + max_top ...);
> > > }
> > > 
> > > I'm not suggesting the actual code set the bounds multiple times, it's
just
> > > easiest illustrate.
> > 
> > Yes, I believe that would achieve what I'm describing.
> 
> Would it be helpful for you both to play with the BoxLayoutExample to get a
feel
> for how it works right now. I think once you actually see the behavior in
> practice, it will make much more sense both visually and operationally. If
> anything, it will give both of you a clearer picture of where tweaks and
> adjustments need to be made. For the most part, I think the code is doing
> exactly what is being described here. I can even make changes to the example
in
> order to better represent real-world scenarios. IMO, an interactive and
> operational test rig is more valuable than an in-person or virtual whiteboard
> discussion.
> 
> If this would expedite things, I even can provide a link to the
> views_examples_with_content_exe.exe executable file and the required DLLs so
you
> don't have to apply the patch and build locally.


I tried to get views_examples_with_content_exe.exe pulled out with it's required
libraries, but it crashes on startup.. I guess it needs more than merely the
dlls. Again, if this might be helpful, I'll continue to put this together by
doing a slimmed down non-component build into a separate output folder.

kylix_rd

On 2017/05/16 16:21:35, kylix_rd wrote: > On 2017/05/16 13:34:01, kylix_rd wrote: > > On 2017/05/15 ...

3 years, 7 months ago (2017-05-16 17:37:08 UTC) #67

On 2017/05/16 16:21:35, kylix_rd wrote:
> On 2017/05/16 13:34:01, kylix_rd wrote:
> > On 2017/05/15 21:53:03, Peter Kasting wrote:
> > > On 2017/05/15 21:44:53, sky wrote:
> > > > On 2017/05/15 21:15:33, Peter Kasting wrote:
> > > > > Then we would attach the margins to each view:
> > > > 
> > > > By attach margin is it something like you're subtracting the top-margin
> from
> > > the
> > > > y-coordinate in the last pass, accumulating the max margin, and then
loop
> > over
> > > > the views again offseting the max?
> > > > 
> > > > max_top = 0;
> > > > for (view in views) {
> > > >   max_top = max(max_top, view->margin().top());
> > > >   view->SetBounds(... view->bounds().y() - view->margin().top() ...);
> > > > }
> > > > 
> > > > And then you make a pass offsetting by max_top?
> > > > for (view in views) {
> > > >   view->SetBounds(... view->bounds().y() + max_top ...);
> > > > }
> > > > 
> > > > I'm not suggesting the actual code set the bounds multiple times, it's
> just
> > > > easiest illustrate.
> > > 
> > > Yes, I believe that would achieve what I'm describing.
> > 
> > Would it be helpful for you both to play with the BoxLayoutExample to get a
> feel
> > for how it works right now. I think once you actually see the behavior in
> > practice, it will make much more sense both visually and operationally. If
> > anything, it will give both of you a clearer picture of where tweaks and
> > adjustments need to be made. For the most part, I think the code is doing
> > exactly what is being described here. I can even make changes to the example
> in
> > order to better represent real-world scenarios. IMO, an interactive and
> > operational test rig is more valuable than an in-person or virtual
whiteboard
> > discussion.
> > 
> > If this would expedite things, I even can provide a link to the
> > views_examples_with_content_exe.exe executable file and the required DLLs so
> you
> > don't have to apply the patch and build locally.
> 
> 
> I tried to get views_examples_with_content_exe.exe pulled out with it's
required
> libraries, but it crashes on startup.. I guess it needs more than merely the
> dlls. Again, if this might be helpful, I'll continue to put this together by
> doing a slimmed down non-component build into a separate output folder.

https://drive.google.com/open?id=0B6LQg8CWo5bNX080Q19LYUJaUzg

This should be views_examples.zip. You should be able to unzip to a local folder
and run views_examples_with_content_exe. The first "example" is BoxLayout so it
should be ready to go right off.

kylix_rd

Still need to separate the view property into a separate file, and maybe add more ...

3 years, 7 months ago (2017-05-16 18:45:48 UTC) #68

Still need to separate the view property into a separate file, and maybe add
more ASCII-art examples to the .h file.

https://codereview.chromium.org/2836313002/diff/200001/ui/views/layout/box_la...
File ui/views/layout/box_layout.cc (right):

https://codereview.chromium.org/2836313002/diff/200001/ui/views/layout/box_la...
ui/views/layout/box_layout.cc:41: view_->SetBounds(bounds.x(), bounds.y(),
bounds.width(), bounds.height());
On 2017/05/15 20:29:09, sky wrote:
> view_->SetBoundsRect(bounds)

Done.

https://codereview.chromium.org/2836313002/diff/200001/ui/views/layout/box_la...
ui/views/layout/box_layout.cc:310: ? std::max(between_child_spacing_,
On 2017/05/15 20:29:09, sky wrote:
> optional: this code is nigh unreadable because of the excessive ternary
> operators. Peter may disagree as he uses the ternary operator a lot, so I'm
> leaving this optional. Specifically I would rather you use multiple 'ifs.' I'm
> ok with one ternary operator, but 3 ternary operators is not particularly
> readable.
> 
> Maybe checking if either view is null or !collapse_margins_spacing_ first
would
> help, e.g.
> if (!collapse_margins_spacing_ || !left.view() || !right.right())
>   return between_child_spacing_;

What? You don't appreciate my "mad ternary operator skillz" :)?

Yes, a little rework would make this more readable...

Done.

https://codereview.chromium.org/2836313002/diff/200001/ui/views/layout/box_la...
File ui/views/layout/box_layout.h (right):

https://codereview.chromium.org/2836313002/diff/200001/ui/views/layout/box_la...
ui/views/layout/box_layout.h:62: // whether or not spacing adjacent to view
margins are collapsed base on the
On 2017/05/15 20:29:09, sky wrote:
> How about: 'spacing between views along the main axis are collapsed to the max
> of the margins of the two adjacent views.' Please also add an example to help
> drive it home.

It's more than merely "adjacent views". The collapsing also happens with the
inside border spacing (aka. the internal padding). So it is the adjacent
"spacing", regardless of where that spacing is specified, that is collapsed.

I can certainly add an example... based on the ASCII art you and pkasting@ are
bantering about :).

https://codereview.chromium.org/2836313002/diff/200001/ui/views/layout/box_la...
ui/views/layout/box_layout.h:126: ViewWrapper(const ViewWrapper& wrapper) =
delete;
On 2017/05/15 20:29:09, sky wrote:
> Is there a reason to delete this constructor and not use DISALLOW that deletes
> more?

No, not really.

Done.

https://codereview.chromium.org/2836313002/diff/200001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/2836313002/diff/200001/ui/views/view.h#newcod...
ui/views/view.h:1705: VIEWS_EXPORT extern const ui::ClassProperty<gfx::Insets*>*
const kMarginsKey;
On 2017/05/15 20:29:10, sky wrote:
> Please move this into a separate file (I commented on this in an earlier patch
> with recommendations as to where to put it).

Oops. My bad. I completely missed that comment.

kylix_rd

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-17 14:05:07 UTC) #69

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2836313002/220001

3 years, 7 months ago (2017-05-17 14:05:24 UTC) #70

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-17 16:06:01 UTC) #71

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/457420)

3 years, 7 months ago (2017-05-17 16:06:02 UTC) #72

kylix_rd

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-17 16:26:29 UTC) #73

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2836313002/240001

3 years, 7 months ago (2017-05-17 16:27:06 UTC) #74

kylix_rd

New patches. Moved view properties to separate files. Fixed crash reported by pkasting@ in the ...

3 years, 7 months ago (2017-05-17 18:14:03 UTC) #75

kylix_rd

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-17 18:14:25 UTC) #76

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2836313002/260001

3 years, 7 months ago (2017-05-17 18:15:44 UTC) #77

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-17 23:10:52 UTC) #78

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/429621)

3 years, 7 months ago (2017-05-17 23:10:53 UTC) #79

kylix_rd

OK, this patch *should* get the functionality nailed down. In the non-collapsed margin mode, the ...

3 years, 7 months ago (2017-05-18 17:06:14 UTC) #80

kylix_rd

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-18 17:06:26 UTC) #81

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2836313002/280001

3 years, 7 months ago (2017-05-18 17:07:07 UTC) #82

Peter Kasting

On 2017/05/18 17:06:14, kylix_rd wrote: > OK, this patch *should* get the functionality nailed down. ...

3 years, 7 months ago (2017-05-18 17:10:46 UTC) #83

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-18 17:50:20 UTC) #84

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/429620)

3 years, 7 months ago (2017-05-18 17:50:21 UTC) #85

kylix_rd

On 2017/05/18 17:10:46, Peter Kasting wrote: > On 2017/05/18 17:06:14, kylix_rd wrote: > > OK, ...

3 years, 7 months ago (2017-05-18 19:16:16 UTC) #86

kylix_rd

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-18 19:16:37 UTC) #87

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2836313002/300001

3 years, 7 months ago (2017-05-18 19:17:53 UTC) #88

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-18 22:52:42 UTC) #89

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/448959)

3 years, 7 months ago (2017-05-18 22:52:43 UTC) #90

kylix_rd

On 2017/05/18 19:16:16, kylix_rd wrote: > On 2017/05/18 17:10:46, Peter Kasting wrote: > > On ...

3 years, 7 months ago (2017-05-19 18:53:17 UTC) #91

Peter Kasting

On 2017/05/19 18:53:17, kylix_rd wrote: > On 2017/05/18 19:16:16, kylix_rd wrote: > > On 2017/05/18 ...

3 years, 7 months ago (2017-05-19 19:02:21 UTC) #92

kylix_rd

On 2017/05/19 19:02:21, Peter Kasting wrote: > On 2017/05/19 18:53:17, kylix_rd wrote: > > On ...

3 years, 7 months ago (2017-05-19 19:25:02 UTC) #93

On 2017/05/19 19:02:21, Peter Kasting wrote:
> On 2017/05/19 18:53:17, kylix_rd wrote:
> > On 2017/05/18 19:16:16, kylix_rd wrote:
> > > On 2017/05/18 17:10:46, Peter Kasting wrote:
> > > > On 2017/05/18 17:06:14, kylix_rd wrote:
> > > > > OK, this patch *should* get the functionality nailed down. In the
> > > > non-collapsed
> > > > > margin mode, the margin along the cross-axis is the inside border
> spacing
> > +
> > > > the
> > > > > maximum margin between all views along the cross-axis sides. For
> example,
> > > the
> > > > > inside border spacing is 2 and view 1 has margins of 3 and view 2 has
> > > margins
> > > > of
> > > > > 4, all along the cross-axis. The overall margin will be 6 (inside
border
> > > > spacing
> > > > > 2 + max margin 4). This will ensure that the actual view boundaries
will
> > > align
> > > > > when the boundaries of the view encroach into that space.
> > > > 
> > > > Offhand, that sounds like Scott's route (3), which oversizes the margin.

> > > > Consider laying out these two views in centered mode:
> > > > 
> > > > MMMM MMMM
> > > > VVVV MMMM
> > > > VVVV VVVV
> > > > VVVV
> > > > 
> > > > Here the total margin above the taller view should only be 1, despite
the
> > > margin
> > > > of the second view being 2, because that's sufficient to guarantee both
> > views
> > > > their desired margin.
> > > 
> > > Hmmm... You're right. To get it to do this will take a little more work.
It
> > will
> > > affect GetPreferredSize() which has never taken into account centering
along
> > the
> > > cross axis.
> > 
> > I looked at this some more and it's going to take more work than I thought.
Is
> > this critical at this point? Can we finalize this patch and acknowledge the
> > current behavior either in a TODO or merely document it?
> 
> The behavior I think you're saying would take more work is the behavior I need
> for infobars, that's presumably why you're adding this to begin with.  So
uh...
> if you want your one consumer to use this, yes, it's critical?  :)

OK, I'll look into this a little deeper. I should be able come up with a
solution in the next day or so (early next week).

sky

On 2017/05/19 19:02:21, Peter Kasting wrote: > On 2017/05/19 18:53:17, kylix_rd wrote: > > On ...

3 years, 7 months ago (2017-05-19 22:06:48 UTC) #94

On 2017/05/19 19:02:21, Peter Kasting wrote:
> On 2017/05/19 18:53:17, kylix_rd wrote:
> > On 2017/05/18 19:16:16, kylix_rd wrote:
> > > On 2017/05/18 17:10:46, Peter Kasting wrote:
> > > > On 2017/05/18 17:06:14, kylix_rd wrote:
> > > > > OK, this patch *should* get the functionality nailed down. In the
> > > > non-collapsed
> > > > > margin mode, the margin along the cross-axis is the inside border
> spacing
> > +
> > > > the
> > > > > maximum margin between all views along the cross-axis sides. For
> example,
> > > the
> > > > > inside border spacing is 2 and view 1 has margins of 3 and view 2 has
> > > margins
> > > > of
> > > > > 4, all along the cross-axis. The overall margin will be 6 (inside
border
> > > > spacing
> > > > > 2 + max margin 4). This will ensure that the actual view boundaries
will
> > > align
> > > > > when the boundaries of the view encroach into that space.
> > > > 
> > > > Offhand, that sounds like Scott's route (3), which oversizes the margin.

> > > > Consider laying out these two views in centered mode:
> > > > 
> > > > MMMM MMMM
> > > > VVVV MMMM
> > > > VVVV VVVV
> > > > VVVV
> > > > 
> > > > Here the total margin above the taller view should only be 1, despite
the
> > > margin
> > > > of the second view being 2, because that's sufficient to guarantee both
> > views
> > > > their desired margin.
> > > 
> > > Hmmm... You're right. To get it to do this will take a little more work.
It
> > will
> > > affect GetPreferredSize() which has never taken into account centering
along
> > the
> > > cross axis.
> > 
> > I looked at this some more and it's going to take more work than I thought.
Is
> > this critical at this point? Can we finalize this patch and acknowledge the
> > current behavior either in a TODO or merely document it?
> 
> The behavior I think you're saying would take more work is the behavior I need
> for infobars, that's presumably why you're adding this to begin with.  So
uh...
> if you want your one consumer to use this, yes, it's critical?  :)

Peter, I encourage you to put up a patch for converting InfoBars to using
BoxLayout. I would hate for Allen to do all this work and then you realize there
is some reason InfoBars can't actually use BoxLayout... I think the chances of
that happening are slim, but it would be good to know sooner rather than later.

  -Scott

Peter Kasting

On 2017/05/19 22:06:48, sky wrote: > Peter, I encourage you to put up a patch ...

3 years, 7 months ago (2017-05-19 22:09:51 UTC) #95

kylix_rd

Ok, I will freely admit this was one of the most challenging patches I've done ...

3 years, 7 months ago (2017-05-23 20:51:39 UTC) #97

kylix_rd

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-24 14:53:26 UTC) #98

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2836313002/360001

3 years, 7 months ago (2017-05-24 14:53:50 UTC) #99

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-24 15:54:55 UTC) #100

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 7 months ago (2017-05-24 15:54:56 UTC) #101

sky

https://codereview.chromium.org/2836313002/diff/360001/ui/views/layout/box_layout.cc File ui/views/layout/box_layout.cc (right): https://codereview.chromium.org/2836313002/diff/360001/ui/views/layout/box_layout.cc#newcode30 ui/views/layout/box_layout.cc:30: } else if (orientation_ == Orientation::kHorizontal) { style guide ...

3 years, 7 months ago (2017-05-24 21:28:51 UTC) #102

kylix_rd

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-25 14:32:02 UTC) #103

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2836313002/380001

3 years, 7 months ago (2017-05-25 14:32:16 UTC) #104

kylix_rd

https://codereview.chromium.org/2836313002/diff/360001/ui/views/layout/box_layout.cc File ui/views/layout/box_layout.cc (right): https://codereview.chromium.org/2836313002/diff/360001/ui/views/layout/box_layout.cc#newcode30 ui/views/layout/box_layout.cc:30: } else if (orientation_ == Orientation::kHorizontal) { On 2017/05/24 ...

3 years, 7 months ago (2017-05-25 14:32:17 UTC) #105

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-25 16:43:24 UTC) #106

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 7 months ago (2017-05-25 16:43:25 UTC) #107

sky

https://codereview.chromium.org/2836313002/diff/380001/ui/views/layout/box_layout.cc File ui/views/layout/box_layout.cc (right): https://codereview.chromium.org/2836313002/diff/380001/ui/views/layout/box_layout.cc#newcode117 ui/views/layout/box_layout.cc:117: ViewWrapper next(this, NextVisibleView(i)); move after visible check. https://codereview.chromium.org/2836313002/diff/380001/ui/views/layout/box_layout.cc#newcode167 ui/views/layout/box_layout.cc:167: ...

3 years, 7 months ago (2017-05-25 19:16:52 UTC) #108

kylix_rd

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

3 years, 7 months ago (2017-05-25 20:09:44 UTC) #109

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2836313002/400001

3 years, 7 months ago (2017-05-25 20:10:16 UTC) #110

kylix_rd

leading/trailing is a *much* better name. Thanks. https://codereview.chromium.org/2836313002/diff/380001/ui/views/layout/box_layout.cc File ui/views/layout/box_layout.cc (right): https://codereview.chromium.org/2836313002/diff/380001/ui/views/layout/box_layout.cc#newcode117 ui/views/layout/box_layout.cc:117: ViewWrapper next(this, ...

3 years, 7 months ago (2017-05-25 20:11:20 UTC) #111

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-25 22:35:32 UTC) #112

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 7 months ago (2017-05-25 22:35:34 UTC) #113

sky

https://codereview.chromium.org/2836313002/diff/380001/ui/views/layout/box_layout.cc File ui/views/layout/box_layout.cc (right): https://codereview.chromium.org/2836313002/diff/380001/ui/views/layout/box_layout.cc#newcode493 ui/views/layout/box_layout.cc:493: gfx::Size non_child_size = NonChildSize(host_); On 2017/05/25 20:11:19, kylix_rd wrote: ...

3 years, 7 months ago (2017-05-25 22:37:27 UTC) #114

kylix_rd

In the immortal words of Bullwinkle Moose, "This time for sure!"... After correcting an issue ...

3 years, 6 months ago (2017-05-30 20:43:43 UTC) #115

sky

On 2017/05/30 20:43:43, kylix_rd wrote: > In the immortal words of Bullwinkle Moose, "This time ...

3 years, 6 months ago (2017-05-31 14:53:54 UTC) #116

On 2017/05/30 20:43:43, kylix_rd wrote:
> In the immortal words of Bullwinkle Moose, "This time for sure!"...
> 
> After correcting an issue with how GetPreferredSize() was calculating the
size,
> I also had to adjust how the layout works when the cross axis is centered.
> 
> The upshot of this is that regardless of whether or not the margins are
> collapsed, the behavior of the cross-axis remains the same. Another point is
> that the views are centered based on the full cross-axis size (sans the
insets).
> This can cause the view to be positioned off-center when the available bounds
> are sufficiently small and the margins are unbalanced across that axis. Clear
as
> mud?

Here's a unit test for the test case we talked about:

TEST_F(BoxLayoutTest, OverlappingCrossMargins) {
  {
    BoxLayout* layout = new BoxLayout(BoxLayout::kHorizontal, 0, 0, 0, false);
    host_->SetLayoutManager(layout);
    layout->set_cross_axis_alignment(BoxLayout::CROSS_AXIS_ALIGNMENT_END);
    View* v1 = new StaticSizedView(gfx::Size(20, 4));
    v1->SetProperty(kMarginsKey, new gfx::Insets(3, 0, 0, 0));
    host_->AddChildView(v1);
    View* v2 = new StaticSizedView(gfx::Size(20, 5));
    v2->SetProperty(kMarginsKey, new gfx::Insets(0, 0, 2, 0));
    host_->AddChildView(v2);

    EXPECT_EQ(9, layout->GetPreferredSize(host_.get()).height());
  }
  {
    BoxLayout* layout = new BoxLayout(BoxLayout::kHorizontal, 0, 0, 0, true);
    host_->SetLayoutManager(layout);
    layout->set_cross_axis_alignment(BoxLayout::CROSS_AXIS_ALIGNMENT_END);
    View* v1 = new StaticSizedView(gfx::Size(20, 4));
    v1->SetProperty(kMarginsKey, new gfx::Insets(3, 0, 0, 0));
    host_->AddChildView(v1);
    View* v2 = new StaticSizedView(gfx::Size(20, 5));
    v2->SetProperty(kMarginsKey, new gfx::Insets(0, 0, 2, 0));
    host_->AddChildView(v2);

    EXPECT_EQ(9, layout->GetPreferredSize(host_.get()).height());
  }
}

Unless I'm missing something the height with this patch is 10, not 9 as I would
expect. Am I missing something?

sky

Also, I did have one comment on the name below. Please correct me if I'm ...

3 years, 6 months ago (2017-05-31 15:25:15 UTC) #117

kylix_rd

https://codereview.chromium.org/2836313002/diff/420001/ui/views/layout/box_layout.h File ui/views/layout/box_layout.h (right): https://codereview.chromium.org/2836313002/diff/420001/ui/views/layout/box_layout.h#newcode63 ui/views/layout/box_layout.h:63: // |collapse_margins_spacing| parameter controls whether or not adjacent On ...

3 years, 6 months ago (2017-05-31 15:44:57 UTC) #118

sky

Fair enough. Please make this clear in the description. Thanks! On Wed, May 31, 2017 ...

3 years, 6 months ago (2017-05-31 15:49:13 UTC) #119

kylix_rd

On 2017/05/31 14:53:54, sky wrote: > On 2017/05/30 20:43:43, kylix_rd wrote: > > In the ...

3 years, 6 months ago (2017-05-31 17:02:22 UTC) #120

On 2017/05/31 14:53:54, sky wrote:
> On 2017/05/30 20:43:43, kylix_rd wrote:
> > In the immortal words of Bullwinkle Moose, "This time for sure!"...
> > 
> > After correcting an issue with how GetPreferredSize() was calculating the
> size,
> > I also had to adjust how the layout works when the cross axis is centered.
> > 
> > The upshot of this is that regardless of whether or not the margins are
> > collapsed, the behavior of the cross-axis remains the same. Another point is
> > that the views are centered based on the full cross-axis size (sans the
> insets).
> > This can cause the view to be positioned off-center when the available
bounds
> > are sufficiently small and the margins are unbalanced across that axis.
Clear
> as
> > mud?
> 
> Here's a unit test for the test case we talked about:
> 
> TEST_F(BoxLayoutTest, OverlappingCrossMargins) {
>   {
>     BoxLayout* layout = new BoxLayout(BoxLayout::kHorizontal, 0, 0, 0, false);
>     host_->SetLayoutManager(layout);
>     layout->set_cross_axis_alignment(BoxLayout::CROSS_AXIS_ALIGNMENT_END);
>     View* v1 = new StaticSizedView(gfx::Size(20, 4));
>     v1->SetProperty(kMarginsKey, new gfx::Insets(3, 0, 0, 0));
>     host_->AddChildView(v1);
>     View* v2 = new StaticSizedView(gfx::Size(20, 5));
>     v2->SetProperty(kMarginsKey, new gfx::Insets(0, 0, 2, 0));
>     host_->AddChildView(v2);
> 
>     EXPECT_EQ(9, layout->GetPreferredSize(host_.get()).height());
>   }
>   {
>     BoxLayout* layout = new BoxLayout(BoxLayout::kHorizontal, 0, 0, 0, true);
>     host_->SetLayoutManager(layout);
>     layout->set_cross_axis_alignment(BoxLayout::CROSS_AXIS_ALIGNMENT_END);
>     View* v1 = new StaticSizedView(gfx::Size(20, 4));
>     v1->SetProperty(kMarginsKey, new gfx::Insets(3, 0, 0, 0));
>     host_->AddChildView(v1);
>     View* v2 = new StaticSizedView(gfx::Size(20, 5));
>     v2->SetProperty(kMarginsKey, new gfx::Insets(0, 0, 2, 0));
>     host_->AddChildView(v2);
> 
>     EXPECT_EQ(9, layout->GetPreferredSize(host_.get()).height());
>   }
> }
> 
> Unless I'm missing something the height with this patch is 10, not 9 as I
would
> expect. Am I missing something?

What happens to view(20,5) as the vertical height shrinks? Does it remain 2 from
the upper edge and shrink along with view(20,4)? If sized down to 8, then
view(20,5) becomes (20,4) and view(20,4) becomes (20,3) and they would never
align their top edges. I thought that the margin along the cross axis edges
should be the max of all the views. That would keep (20,5) from violating that
constraint. That is why it is 10 instead of 9.

pkasting@? Am I understanding this correctly?

kylix_rd

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

3 years, 6 months ago (2017-05-31 19:07:42 UTC) #121

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2836313002/440001

3 years, 6 months ago (2017-05-31 19:08:41 UTC) #122

sky

On Wed, May 31, 2017 at 10:02 AM, <kylixrd@chromium.org> wrote: > On 2017/05/31 14:53:54, sky ...

3 years, 6 months ago (2017-05-31 19:53:11 UTC) #123

On Wed, May 31, 2017 at 10:02 AM, <kylixrd@chromium.org> wrote:

> On 2017/05/31 14:53:54, sky wrote:
> > On 2017/05/30 20:43:43, kylix_rd wrote:
> > > In the immortal words of Bullwinkle Moose, "This time for sure!"...
> > >
> > > After correcting an issue with how GetPreferredSize() was calculating
> the
> > size,
> > > I also had to adjust how the layout works when the cross axis is
> centered.
> > >
> > > The upshot of this is that regardless of whether or not the margins are
> > > collapsed, the behavior of the cross-axis remains the same. Another
> point is
> > > that the views are centered based on the full cross-axis size (sans the
> > insets).
> > > This can cause the view to be positioned off-center when the available
> bounds
> > > are sufficiently small and the margins are unbalanced across that axis.
> Clear
> > as
> > > mud?
> >
> > Here's a unit test for the test case we talked about:
> >
> > TEST_F(BoxLayoutTest, OverlappingCrossMargins) {
> > {
> > BoxLayout* layout = new BoxLayout(BoxLayout::kHorizontal, 0, 0, 0,
> false);
> > host_->SetLayoutManager(layout);
> > layout->set_cross_axis_alignment(BoxLayout::CROSS_AXIS_ALIGNMENT_END);
> > View* v1 = new StaticSizedView(gfx::Size(20, 4));
> > v1->SetProperty(kMarginsKey, new gfx::Insets(3, 0, 0, 0));
> > host_->AddChildView(v1);
> > View* v2 = new StaticSizedView(gfx::Size(20, 5));
> > v2->SetProperty(kMarginsKey, new gfx::Insets(0, 0, 2, 0));
> > host_->AddChildView(v2);
> >
> > EXPECT_EQ(9, layout->GetPreferredSize(host_.get()).height());
> > }
> > {
> > BoxLayout* layout = new BoxLayout(BoxLayout::kHorizontal, 0, 0, 0,
> true);
> > host_->SetLayoutManager(layout);
> > layout->set_cross_axis_alignment(BoxLayout::CROSS_AXIS_ALIGNMENT_END);
> > View* v1 = new StaticSizedView(gfx::Size(20, 4));
> > v1->SetProperty(kMarginsKey, new gfx::Insets(3, 0, 0, 0));
> > host_->AddChildView(v1);
> > View* v2 = new StaticSizedView(gfx::Size(20, 5));
> > v2->SetProperty(kMarginsKey, new gfx::Insets(0, 0, 2, 0));
> > host_->AddChildView(v2);
> >
> > EXPECT_EQ(9, layout->GetPreferredSize(host_.get()).height());
> > }
> > }
> >
> > Unless I'm missing something the height with this patch is 10, not 9 as I
> would
> > expect. Am I missing something?
>
> What happens to view(20,5) as the vertical height shrinks? Does it remain
> 2 from
> the upper edge and shrink along with view(20,4)? If sized down to 8, then
> view(20,5) becomes (20,4) and view(20,4) becomes (20,3) and they would
> never
> align their top edges. I thought that the margin along the cross axis edges
> should be the max of all the views.
>

If the margin along the cross axis is the max, then that's option (3),
which isn't what Peter wants.

What happens during resize depends upon the minimize size. I would treat
the margins as inflexible. Behavior when resized smaller than the minimum
is generally views extend outside the bounds of their parent, which most
often leads to clipping.


> That would keep (20,5) from violating that
> constraint. That is why it is 10 instead of 9.
>
> pkasting@? Am I understanding this correctly?
>
> https://codereview.chromium.org/2836313002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

kylix_rd

On 2017/05/31 19:53:11, sky wrote: > On Wed, May 31, 2017 at 10:02 AM, <mailto:kylixrd@chromium.org> ...

3 years, 6 months ago (2017-05-31 20:49:38 UTC) #124

On 2017/05/31 19:53:11, sky wrote:
> On Wed, May 31, 2017 at 10:02 AM, <mailto:kylixrd@chromium.org> wrote:
> 
> > On 2017/05/31 14:53:54, sky wrote:
> > > On 2017/05/30 20:43:43, kylix_rd wrote:
> > > > In the immortal words of Bullwinkle Moose, "This time for sure!"...
> > > >
> > > > After correcting an issue with how GetPreferredSize() was calculating
> > the
> > > size,
> > > > I also had to adjust how the layout works when the cross axis is
> > centered.
> > > >
> > > > The upshot of this is that regardless of whether or not the margins are
> > > > collapsed, the behavior of the cross-axis remains the same. Another
> > point is
> > > > that the views are centered based on the full cross-axis size (sans the
> > > insets).
> > > > This can cause the view to be positioned off-center when the available
> > bounds
> > > > are sufficiently small and the margins are unbalanced across that axis.
> > Clear
> > > as
> > > > mud?
> > >
> > > Here's a unit test for the test case we talked about:
> > >
> > > TEST_F(BoxLayoutTest, OverlappingCrossMargins) {
> > > {
> > > BoxLayout* layout = new BoxLayout(BoxLayout::kHorizontal, 0, 0, 0,
> > false);
> > > host_->SetLayoutManager(layout);
> > > layout->set_cross_axis_alignment(BoxLayout::CROSS_AXIS_ALIGNMENT_END);
> > > View* v1 = new StaticSizedView(gfx::Size(20, 4));
> > > v1->SetProperty(kMarginsKey, new gfx::Insets(3, 0, 0, 0));
> > > host_->AddChildView(v1);
> > > View* v2 = new StaticSizedView(gfx::Size(20, 5));
> > > v2->SetProperty(kMarginsKey, new gfx::Insets(0, 0, 2, 0));
> > > host_->AddChildView(v2);
> > >
> > > EXPECT_EQ(9, layout->GetPreferredSize(host_.get()).height());
> > > }
> > > {
> > > BoxLayout* layout = new BoxLayout(BoxLayout::kHorizontal, 0, 0, 0,
> > true);
> > > host_->SetLayoutManager(layout);
> > > layout->set_cross_axis_alignment(BoxLayout::CROSS_AXIS_ALIGNMENT_END);
> > > View* v1 = new StaticSizedView(gfx::Size(20, 4));
> > > v1->SetProperty(kMarginsKey, new gfx::Insets(3, 0, 0, 0));
> > > host_->AddChildView(v1);
> > > View* v2 = new StaticSizedView(gfx::Size(20, 5));
> > > v2->SetProperty(kMarginsKey, new gfx::Insets(0, 0, 2, 0));
> > > host_->AddChildView(v2);
> > >
> > > EXPECT_EQ(9, layout->GetPreferredSize(host_.get()).height());
> > > }
> > > }
> > >
> > > Unless I'm missing something the height with this patch is 10, not 9 as I
> > would
> > > expect. Am I missing something?
> >
> > What happens to view(20,5) as the vertical height shrinks? Does it remain
> > 2 from
> > the upper edge and shrink along with view(20,4)? If sized down to 8, then
> > view(20,5) becomes (20,4) and view(20,4) becomes (20,3) and they would
> > never
> > align their top edges. I thought that the margin along the cross axis edges
> > should be the max of all the views.
> >
> 
> If the margin along the cross axis is the max, then that's option (3),
> which isn't what Peter wants.
> 
> What happens during resize depends upon the minimize size. I would treat
> the margins as inflexible. Behavior when resized smaller than the minimum
> is generally views extend outside the bounds of their parent, which most
> often leads to clipping.
> 
> 
> > That would keep (20,5) from violating that
> > constraint. That is why it is 10 instead of 9.
> >
> > pkasting@? Am I understanding this correctly?
> >
> > https://codereview.chromium.org/2836313002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Pardon me while I wipe the egg off my face... I went back over the ASCII
diagrams again. I think I see where I missed it. In non-collapsed margin more,
the preferred size calculation *does* need to take into account the cross axis
alignment, especially for START and END. The margins will "collapse" to the max
of all the views along the common START or END edge... the margin on the
opposite edge will only affect that specific view. Currently GetPreferredSize()
collapses both margins along the cross axis regardless of the alignment.

Peter Kasting

On 2017/05/31 20:49:38, kylix_rd wrote: > Pardon me while I wipe the egg off my ...

3 years, 6 months ago (2017-05-31 20:53:51 UTC) #125

On 2017/05/31 20:49:38, kylix_rd wrote:
> Pardon me while I wipe the egg off my face... I went back over the ASCII
> diagrams again. I think I see where I missed it. In non-collapsed margin more,
> the preferred size calculation *does* need to take into account the cross axis
> alignment, especially for START and END. The margins will "collapse" to the
max
> of all the views along the common START or END edge... the margin on the
> opposite edge will only affect that specific view. Currently
GetPreferredSize()
> collapses both margins along the cross axis regardless of the alignment.

I think you are coming to the same place I am, but below is the response to your
earlier question I just finished writing, in case it adds clarity.

On 2017/05/31 17:02:22, kylix_rd wrote:
> On 2017/05/31 14:53:54, sky wrote:
> > On 2017/05/30 20:43:43, kylix_rd wrote:
> > > In the immortal words of Bullwinkle Moose, "This time for sure!"...
> > > 
> > > After correcting an issue with how GetPreferredSize() was calculating the
> > size,
> > > I also had to adjust how the layout works when the cross axis is centered.
> > > 
> > > The upshot of this is that regardless of whether or not the margins are
> > > collapsed, the behavior of the cross-axis remains the same. Another point
is
> > > that the views are centered based on the full cross-axis size (sans the
> > insets).
> > > This can cause the view to be positioned off-center when the available
> bounds
> > > are sufficiently small and the margins are unbalanced across that axis.
> Clear
> > as
> > > mud?
> > 
> > Here's a unit test for the test case we talked about:
> > 
> > TEST_F(BoxLayoutTest, OverlappingCrossMargins) {
> >   {
> >     BoxLayout* layout = new BoxLayout(BoxLayout::kHorizontal, 0, 0, 0,
false);
> >     host_->SetLayoutManager(layout);
> >     layout->set_cross_axis_alignment(BoxLayout::CROSS_AXIS_ALIGNMENT_END);
> >     View* v1 = new StaticSizedView(gfx::Size(20, 4));
> >     v1->SetProperty(kMarginsKey, new gfx::Insets(3, 0, 0, 0));
> >     host_->AddChildView(v1);
> >     View* v2 = new StaticSizedView(gfx::Size(20, 5));
> >     v2->SetProperty(kMarginsKey, new gfx::Insets(0, 0, 2, 0));
> >     host_->AddChildView(v2);
> > 
> >     EXPECT_EQ(9, layout->GetPreferredSize(host_.get()).height());
> >   }

So just so I'm clear, we have view 1, which has height 4 and a 3 DIP top margin,
end-aligned to view 2, which has height 5 and a 2 DIP margin.

I would expect this to get laid out like:

MMMM
MMMM
MMMM  VVVV
VVVV  VVVV
VVVV  VVVV
VVVV  VVVV
VVVV  VVVV
      MMMM
      MMMM

This would lead to a preferred container height of 9.

> What happens to view(20,5) as the vertical height shrinks? Does it remain 2
from
> the upper edge and shrink along with view(20,4)?

You mean, if the container is sized smaller than its preferred height, what
gives?  I think if you sized the container down from 9 to 7, you'd get this:

MMMM  VVVV
MMMM  VVVV
MMMM  VVVV
VVVV  VVVV
VVVV  VVVV
      MMMM
      MMMM

That is, we don't allow the margins to shrink, and we don't allow the bottom of
view 1 to move past the bottom of view 2 (which would violate end alignment); so
what has to give is view 1's height, which shrinks.

If you shrank the height below 7, then both views would start getting shorter,
until the left view has height 0.  At that point I don't think the container can
shrink further, but maybe if a view becomes size 0 its margin should be taken
out of the picture entirely, I dunno.

kylix_rd

On 2017/05/31 20:53:51, Peter Kasting wrote: > On 2017/05/31 20:49:38, kylix_rd wrote: > > Pardon ...

3 years, 6 months ago (2017-05-31 21:00:41 UTC) #126

On 2017/05/31 20:53:51, Peter Kasting wrote:
> On 2017/05/31 20:49:38, kylix_rd wrote:
> > Pardon me while I wipe the egg off my face... I went back over the ASCII
> > diagrams again. I think I see where I missed it. In non-collapsed margin
more,
> > the preferred size calculation *does* need to take into account the cross
axis
> > alignment, especially for START and END. The margins will "collapse" to the
> max
> > of all the views along the common START or END edge... the margin on the
> > opposite edge will only affect that specific view. Currently
> GetPreferredSize()
> > collapses both margins along the cross axis regardless of the alignment.
> 
> I think you are coming to the same place I am, but below is the response to
your
> earlier question I just finished writing, in case it adds clarity.
> 
> On 2017/05/31 17:02:22, kylix_rd wrote:
> > On 2017/05/31 14:53:54, sky wrote:
> > > On 2017/05/30 20:43:43, kylix_rd wrote:
> > > > In the immortal words of Bullwinkle Moose, "This time for sure!"...
> > > > 
> > > > After correcting an issue with how GetPreferredSize() was calculating
the
> > > size,
> > > > I also had to adjust how the layout works when the cross axis is
centered.
> > > > 
> > > > The upshot of this is that regardless of whether or not the margins are
> > > > collapsed, the behavior of the cross-axis remains the same. Another
point
> is
> > > > that the views are centered based on the full cross-axis size (sans the
> > > insets).
> > > > This can cause the view to be positioned off-center when the available
> > bounds
> > > > are sufficiently small and the margins are unbalanced across that axis.
> > Clear
> > > as
> > > > mud?
> > > 
> > > Here's a unit test for the test case we talked about:
> > > 
> > > TEST_F(BoxLayoutTest, OverlappingCrossMargins) {
> > >   {
> > >     BoxLayout* layout = new BoxLayout(BoxLayout::kHorizontal, 0, 0, 0,
> false);
> > >     host_->SetLayoutManager(layout);
> > >     layout->set_cross_axis_alignment(BoxLayout::CROSS_AXIS_ALIGNMENT_END);
> > >     View* v1 = new StaticSizedView(gfx::Size(20, 4));
> > >     v1->SetProperty(kMarginsKey, new gfx::Insets(3, 0, 0, 0));
> > >     host_->AddChildView(v1);
> > >     View* v2 = new StaticSizedView(gfx::Size(20, 5));
> > >     v2->SetProperty(kMarginsKey, new gfx::Insets(0, 0, 2, 0));
> > >     host_->AddChildView(v2);
> > > 
> > >     EXPECT_EQ(9, layout->GetPreferredSize(host_.get()).height());
> > >   }
> 
> So just so I'm clear, we have view 1, which has height 4 and a 3 DIP top
margin,
> end-aligned to view 2, which has height 5 and a 2 DIP margin.
> 
> I would expect this to get laid out like:
> 
> MMMM
> MMMM
> MMMM  VVVV
> VVVV  VVVV
> VVVV  VVVV
> VVVV  VVVV
> VVVV  VVVV
>       MMMM
>       MMMM
> 
> This would lead to a preferred container height of 9.
> 
> > What happens to view(20,5) as the vertical height shrinks? Does it remain 2
> from
> > the upper edge and shrink along with view(20,4)?
> 
> You mean, if the container is sized smaller than its preferred height, what
> gives?  I think if you sized the container down from 9 to 7, you'd get this:
> 
> MMMM  VVVV
> MMMM  VVVV
> MMMM  VVVV
> VVVV  VVVV
> VVVV  VVVV
>       MMMM
>       MMMM
> 
> That is, we don't allow the margins to shrink, and we don't allow the bottom
of
> view 1 to move past the bottom of view 2 (which would violate end alignment);
so
> what has to give is view 1's height, which shrinks.
> 
> If you shrank the height below 7, then both views would start getting shorter,
> until the left view has height 0.  At that point I don't think the container
can
> shrink further, but maybe if a view becomes size 0 its margin should be taken
> out of the picture entirely, I dunno.

In this instance the cross axis alignment being set to END would return 9.
However, setting it to START or CENTER would return 10, right?

Peter Kasting

On 2017/05/31 21:00:41, kylix_rd wrote: > In this instance the cross axis alignment being set ...

3 years, 6 months ago (2017-05-31 21:07:55 UTC) #127

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 6 months ago (2017-06-01 00:00:53 UTC) #128

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 6 months ago (2017-06-01 00:00:55 UTC) #129

kylix_rd

Now, when the cross axis alignment is START or END and collapse is false, only ...

3 years, 6 months ago (2017-06-01 16:06:11 UTC) #130

kylix_rd

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

3 years, 6 months ago (2017-06-01 16:07:05 UTC) #131

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2836313002/460001

3 years, 6 months ago (2017-06-01 16:07:21 UTC) #132

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 6 months ago (2017-06-01 17:38:57 UTC) #133

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 6 months ago (2017-06-01 17:38:58 UTC) #134

Peter Kasting

On 2017/06/01 16:06:11, kylix_rd wrote: > Now, when the cross axis alignment is START or ...

3 years, 6 months ago (2017-06-01 18:08:40 UTC) #135

kylix_rd

On 2017/06/01 18:08:40, Peter Kasting wrote: > On 2017/06/01 16:06:11, kylix_rd wrote: > > Now, ...

3 years, 6 months ago (2017-06-01 20:07:30 UTC) #136

Peter Kasting

On 2017/06/01 20:07:30, kylix_rd wrote: > On 2017/06/01 18:08:40, Peter Kasting wrote: > > On ...

3 years, 6 months ago (2017-06-01 20:25:48 UTC) #137

kylix_rd

On 2017/06/01 20:25:48, Peter Kasting wrote: > On 2017/06/01 20:07:30, kylix_rd wrote: > > On ...

3 years, 6 months ago (2017-06-01 20:28:29 UTC) #138

Peter Kasting

On 2017/06/01 20:28:29, kylix_rd wrote: > OK :)... this is almost going back to what ...

3 years, 6 months ago (2017-06-01 20:38:38 UTC) #139

kylix_rd

On 2017/06/01 20:38:38, Peter Kasting wrote: > On 2017/06/01 20:28:29, kylix_rd wrote: > > OK ...

3 years, 6 months ago (2017-06-01 20:41:05 UTC) #140

kylix_rd

On 2017/06/01 20:41:05, kylix_rd wrote: > On 2017/06/01 20:38:38, Peter Kasting wrote: > > On ...

3 years, 6 months ago (2017-06-02 16:21:35 UTC) #141

Peter Kasting

On 2017/06/02 16:21:35, kylix_rd wrote: > The more I think about this, I think the ...

3 years, 6 months ago (2017-06-02 17:40:07 UTC) #142

kylix_rd

On 2017/06/02 17:40:07, Peter Kasting wrote: > On 2017/06/02 16:21:35, kylix_rd wrote: > > The ...

3 years, 6 months ago (2017-06-02 17:52:17 UTC) #143

kylix_rd

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

3 years, 6 months ago (2017-06-02 18:24:31 UTC) #144

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2836313002/480001

3 years, 6 months ago (2017-06-02 18:25:09 UTC) #145

kylix_rd

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

3 years, 6 months ago (2017-06-02 19:17:03 UTC) #147

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2836313002/500001

3 years, 6 months ago (2017-06-02 19:17:55 UTC) #148

sky

Allen, Can you please clarify what state this patch is in? If it's not what ...

3 years, 6 months ago (2017-06-02 20:44:58 UTC) #149

kylix_rd

On 2017/06/02 20:44:58, sky wrote: > Allen, > > Can you please clarify what state ...

3 years, 6 months ago (2017-06-02 20:53:39 UTC) #150

On 2017/06/02 20:44:58, sky wrote:
> Allen,
> 
> Can you please clarify what state this patch is in? If it's not what Peter
> (the consumer) wants, I would prefer you fix it to the state that he is
> going to use it. Otherwise I'm going to have to keep reviewing this, and I,
> and I'm sure you, would like to move on!
> 
>   -Scott

The only thing right now is that in STRETCH or CENTER the view bounds on the
cross-axis will be visually aligned if they have different or
overlapping/unbalanced margins. There was a lot of emphasis on ensuring the view
edges would visually align in the cross-axis. This affects not only the layout
code but also the preferred size calculation.

This does kind of feel like it's been a bit of a moving target. I place much of
that on myself and some original misunderstandings of the requirements.
 
> On Fri, Jun 2, 2017 at 10:52 AM, <mailto:kylixrd@chromium.org> wrote:
> 
> > On 2017/06/02 17:40:07, Peter Kasting wrote:
> > > On 2017/06/02 16:21:35, kylix_rd wrote:
> > > > The more I think about this, I think the current behavior makes just
> > as much
> > > > sense as any other. In interest of finalizing this CL, we should go
> > with it
> > as
> > > > coded.
> > >
> > > I'm OK with landing something if you're planning on fixing it
> > afterwards, in
> > the
> > > interests of not having CLs drag on forever.
> >
> > That is certainly a big reason.
> >
> > > I don't agree with the contention that the current behavior makes as much
> > sense
> > > as any other and wouldn't need to be changed. I could be wrong on this
> > one.
> >
> > I'd also like to see how this plays out "in the real world". Especially,
> > I'd
> > like to know how you feel about this after playing around with the views
> > example
> > for a bit. Maybe you'll change your mind, maybe not. IMO, nothing beats
> > watching
> > the code in action.
> >
> > https://codereview.chromium.org/2836313002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 6 months ago (2017-06-02 21:02:38 UTC) #151

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 6 months ago (2017-06-02 21:02:39 UTC) #152

kylix_rd

Here's the current sitch'. For the cross axis, in START and END alignment mode, only ...

3 years, 6 months ago (2017-06-05 15:48:09 UTC) #153

sky

I had always thought collapsing is only useful along the main axis. Do we actually ...

3 years, 6 months ago (2017-06-05 15:51:03 UTC) #154

Peter Kasting

On 2017/06/05 15:51:03, sky wrote: > I had always thought collapsing is only useful along ...

3 years, 6 months ago (2017-06-05 15:57:10 UTC) #155

kylix_rd

On 2017/06/05 15:57:10, Peter Kasting wrote: > On 2017/06/05 15:51:03, sky wrote: > > I ...

3 years, 6 months ago (2017-06-05 16:29:39 UTC) #156

On 2017/06/05 15:57:10, Peter Kasting wrote:
> On 2017/06/05 15:51:03, sky wrote:
> > I had always thought collapsing is only useful along the main axis. Do we
> > actually care about collapsing along the cross axis? Peter, do you need
> > collapsing margins along the cross axis? This really only matters if the
> > view associated with the LayoutManager has a border.
> 
> It sounds as if Allen has implemented two different collapse bits
> (|collapse_margins_spacing_| to control collapsing against the border, plus
one
> other to control collapsing against another margin?).  The
> collapse-against-spacing bit affects both main and cross axis margins, which
> makes sense to me.  I read you as saying we may not care about this, but I
would
> say what's less clear to me is if we care about collapse-against-the-border at
> all; if we do, then it makes sense to do it on both axes.
> 
> It also sounds like the collapse-against-other-margins bit effectively does
> nothing for cross-axis margins, though I wasn't sure (because it's not clear
to
> me how anything would differ if "in START and END alignment mode, only the
> common edge margins are collapsed using the largest of all the views' margins"
> weren't true).  Which also sounds like what I was asking for?

The cross axis margins will only ever "touch" the |inside_border_insets_|. They
are only collapsed against the corresponding cross-axis margin when
|collapse_margins_spacing_| is true. When the alignment is START or END, the
margins aren't so much "collapsed" as much as the common view edges are aligned
at a point that is the largest of all the views margins along that edge. In the
case where |collapse_margins_spacing_| is true, then the edge corresponding to
the START or END alignment is aligned based on the max of the views' margins
along that edge and the |inside_border_insets_| same edge. 

Effectively, |collapse_margins_spacing_| *does* only control how
margins-against-spacing and margins-against-margins are handled.

Again, clear as mud?

sky

On Mon, Jun 5, 2017 at 9:29 AM, <kylixrd@chromium.org> wrote: > On 2017/06/05 15:57:10, Peter ...

3 years, 6 months ago (2017-06-05 17:24:37 UTC) #158

On Mon, Jun 5, 2017 at 9:29 AM, <kylixrd@chromium.org> wrote:

> On 2017/06/05 15:57:10, Peter Kasting wrote:
> > On 2017/06/05 15:51:03, sky wrote:
> > > I had always thought collapsing is only useful along the main axis. Do
> we
> > > actually care about collapsing along the cross axis? Peter, do you need
> > > collapsing margins along the cross axis? This really only matters if
> the
> > > view associated with the LayoutManager has a border.
> >
> > It sounds as if Allen has implemented two different collapse bits
> > (|collapse_margins_spacing_| to control collapsing against the border,
> plus
> one
> > other to control collapsing against another margin?). The
> > collapse-against-spacing bit affects both main and cross axis margins,
> which
> > makes sense to me. I read you as saying we may not care about this, but I
> would
> > say what's less clear to me is if we care about
> collapse-against-the-border at
> > all; if we do, then it makes sense to do it on both axes.
> >
> > It also sounds like the collapse-against-other-margins bit effectively
> does
> > nothing for cross-axis margins, though I wasn't sure (because it's not
> clear
> to
> > me how anything would differ if "in START and END alignment mode, only
> the
> > common edge margins are collapsed using the largest of all the views'
> margins"
> > weren't true). Which also sounds like what I was asking for?
>
> The cross axis margins will only ever "touch" the |inside_border_insets_|.
> They
> are only collapsed against the corresponding cross-axis margin when
> |collapse_margins_spacing_| is true. When the alignment is START or END,
> the
> margins aren't so much "collapsed" as much as the common view edges are
> aligned
> at a point that is the largest of all the views margins along that edge.
>

"common view edges are aligned at a point that is the largest of all the
views margins along that edge."

That sounds like (3). You're not using the max of the margins along the
cross axis, right? You can't be otherwise the test case I sent earlier
wouldn't work.

  -Scott

In the
> case where |collapse_margins_spacing_| is true, then the edge
> corresponding to
> the START or END alignment is aligned based on the max of the views'
> margins
> along that edge and the |inside_border_insets_| same edge.
>
> Effectively, |collapse_margins_spacing_| *does* only control how
> margins-against-spacing and margins-against-margins are handled.
>
> Again, clear as mud?
>
> https://codereview.chromium.org/2836313002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

sky

I got hung up on the docs and didn't make it to the actual implementation. ...

3 years, 6 months ago (2017-06-05 17:26:54 UTC) #159

kylix_rd

On 2017/06/05 17:24:37, sky wrote: > On Mon, Jun 5, 2017 at 9:29 AM, <mailto:kylixrd@chromium.org> ...

3 years, 6 months ago (2017-06-05 17:48:09 UTC) #160

On 2017/06/05 17:24:37, sky wrote:
> On Mon, Jun 5, 2017 at 9:29 AM, <mailto:kylixrd@chromium.org> wrote:
> 
> > On 2017/06/05 15:57:10, Peter Kasting wrote:
> > > On 2017/06/05 15:51:03, sky wrote:
> > > > I had always thought collapsing is only useful along the main axis. Do
> > we
> > > > actually care about collapsing along the cross axis? Peter, do you need
> > > > collapsing margins along the cross axis? This really only matters if
> > the
> > > > view associated with the LayoutManager has a border.
> > >
> > > It sounds as if Allen has implemented two different collapse bits
> > > (|collapse_margins_spacing_| to control collapsing against the border,
> > plus
> > one
> > > other to control collapsing against another margin?). The
> > > collapse-against-spacing bit affects both main and cross axis margins,
> > which
> > > makes sense to me. I read you as saying we may not care about this, but I
> > would
> > > say what's less clear to me is if we care about
> > collapse-against-the-border at
> > > all; if we do, then it makes sense to do it on both axes.
> > >
> > > It also sounds like the collapse-against-other-margins bit effectively
> > does
> > > nothing for cross-axis margins, though I wasn't sure (because it's not
> > clear
> > to
> > > me how anything would differ if "in START and END alignment mode, only
> > the
> > > common edge margins are collapsed using the largest of all the views'
> > margins"
> > > weren't true). Which also sounds like what I was asking for?
> >
> > The cross axis margins will only ever "touch" the |inside_border_insets_|.
> > They
> > are only collapsed against the corresponding cross-axis margin when
> > |collapse_margins_spacing_| is true. When the alignment is START or END,
> > the
> > margins aren't so much "collapsed" as much as the common view edges are
> > aligned
> > at a point that is the largest of all the views margins along that edge.
> >
> 
> "common view edges are aligned at a point that is the largest of all the
> views margins along that edge."
> 
> That sounds like (3). You're not using the max of the margins along the
> cross axis, right? You can't be otherwise the test case I sent earlier
> wouldn't work.

Only when the cross axis alignment is START or END. The view edges corresponding
to the START or the END, are always aligned. This requires that the margin along
that edge be the max among all the views along that edge. Otherwise, the edges
won't be aligned. The opposite edge will not necessarily be aligned as there is
no interaction with the adjacent views along the main axis. For STRETCH or
CENTER, neither of the cross-axis edges will align with the other views since
they, effectively, are aligning along their centers across the main axis.

The test case you sent does work, but only when the cross axis is END aligned.
If the alignment is changed to START, then the height would be 10. I'll take a
crack at the ASCII art to illustrate.
           _
MMMMM..... ^ 
MMMMM..... |   
MMMMM..... |
VVVVVVVVVV |  <--- top edges are aligned when cross axis alignment is START
VVVVVVVVVV |
VVVVVVVVVV 10
VVVVVVVVVV |
.....VVVVV |
.....MMMMM |
.....MMMMM v
           -

With cross-axis alignment set to END.
           _
MMMMM..... ^ 
MMMMM..... |   
MMMMMVVVVV |
VVVVVVVVVV |
VVVVVVVVVV 9
VVVVVVVVVV |
VVVVVVVVVV |  <--- bottom edges are aligned when cross axis alignment is END
.....MMMMM |
.....MMMMM v
           -

For CENTER and STRETCH, due to rounding, the height would be 10, like the START
version above. If the height of the second view changed to 6, it would still be
10.
           _
MMMMM..... ^ 
MMMMM..... |   
MMMMMVVVVV |
VVVVVVVVVV | 
VVVVVVVVVV |
VVVVVVVVVV 10 <--- vertical centers are aligned when cross axis alignment is
CENTER or STRETCH
VVVVVVVVVV |
.....VVVVV |
.....MMMMM |
.....MMMMM v
           -

>   -Scott
> 
> In the
> > case where |collapse_margins_spacing_| is true, then the edge
> > corresponding to
> > the START or END alignment is aligned based on the max of the views'
> > margins
> > along that edge and the |inside_border_insets_| same edge.
> >
> > Effectively, |collapse_margins_spacing_| *does* only control how
> > margins-against-spacing and margins-against-margins are handled.
> >
> > Again, clear as mud?
> >
> > https://codereview.chromium.org/2836313002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

kylix_rd

On 2017/06/05 18:04:43, Peter Kasting wrote: > That all looks correct to me. <Pops cork ...

3 years, 6 months ago (2017-06-05 18:17:08 UTC) #162

kylix_rd

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

3 years, 6 months ago (2017-06-05 20:07:59 UTC) #163

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2836313002/540001

3 years, 6 months ago (2017-06-05 20:08:28 UTC) #164

sky

On 2017/06/05 17:48:09, kylix_rd wrote: > On 2017/06/05 17:24:37, sky wrote: > > On Mon, ...

3 years, 6 months ago (2017-06-05 20:30:53 UTC) #165

On 2017/06/05 17:48:09, kylix_rd wrote:
> On 2017/06/05 17:24:37, sky wrote:
> > On Mon, Jun 5, 2017 at 9:29 AM, <mailto:kylixrd@chromium.org> wrote:
> > 
> > > On 2017/06/05 15:57:10, Peter Kasting wrote:
> > > > On 2017/06/05 15:51:03, sky wrote:
> > > > > I had always thought collapsing is only useful along the main axis. Do
> > > we
> > > > > actually care about collapsing along the cross axis? Peter, do you
need
> > > > > collapsing margins along the cross axis? This really only matters if
> > > the
> > > > > view associated with the LayoutManager has a border.
> > > >
> > > > It sounds as if Allen has implemented two different collapse bits
> > > > (|collapse_margins_spacing_| to control collapsing against the border,
> > > plus
> > > one
> > > > other to control collapsing against another margin?). The
> > > > collapse-against-spacing bit affects both main and cross axis margins,
> > > which
> > > > makes sense to me. I read you as saying we may not care about this, but
I
> > > would
> > > > say what's less clear to me is if we care about
> > > collapse-against-the-border at
> > > > all; if we do, then it makes sense to do it on both axes.
> > > >
> > > > It also sounds like the collapse-against-other-margins bit effectively
> > > does
> > > > nothing for cross-axis margins, though I wasn't sure (because it's not
> > > clear
> > > to
> > > > me how anything would differ if "in START and END alignment mode, only
> > > the
> > > > common edge margins are collapsed using the largest of all the views'
> > > margins"
> > > > weren't true). Which also sounds like what I was asking for?
> > >
> > > The cross axis margins will only ever "touch" the |inside_border_insets_|.
> > > They
> > > are only collapsed against the corresponding cross-axis margin when
> > > |collapse_margins_spacing_| is true. When the alignment is START or END,
> > > the
> > > margins aren't so much "collapsed" as much as the common view edges are
> > > aligned
> > > at a point that is the largest of all the views margins along that edge.
> > >
> > 
> > "common view edges are aligned at a point that is the largest of all the
> > views margins along that edge."
> > 
> > That sounds like (3). You're not using the max of the margins along the
> > cross axis, right? You can't be otherwise the test case I sent earlier
> > wouldn't work.
> 
> Only when the cross axis alignment is START or END. The view edges
corresponding
> to the START or the END, are always aligned. This requires that the margin
along
> that edge be the max among all the views along that edge. Otherwise, the edges
> won't be aligned. The opposite edge will not necessarily be aligned as there
is
> no interaction with the adjacent views along the main axis. For STRETCH or
> CENTER, neither of the cross-axis edges will align with the other views since
> they, effectively, are aligning along their centers across the main axis.
> 
> The test case you sent does work, but only when the cross axis is END aligned.
> If the alignment is changed to START, then the height would be 10. I'll take a
> crack at the ASCII art to illustrate.
>            _
> MMMMM..... ^ 
> MMMMM..... |   
> MMMMM..... |
> VVVVVVVVVV |  <--- top edges are aligned when cross axis alignment is START
> VVVVVVVVVV |
> VVVVVVVVVV 10
> VVVVVVVVVV |
> .....VVVVV |
> .....MMMMM |
> .....MMMMM v
>            -
> 
> With cross-axis alignment set to END.
>            _
> MMMMM..... ^ 
> MMMMM..... |   
> MMMMMVVVVV |
> VVVVVVVVVV |
> VVVVVVVVVV 9
> VVVVVVVVVV |
> VVVVVVVVVV |  <--- bottom edges are aligned when cross axis alignment is END
> .....MMMMM |
> .....MMMMM v
>            -
> 
> For CENTER and STRETCH, due to rounding, the height would be 10, like the
START
> version above. If the height of the second view changed to 6, it would still
be
> 10.
>            _
> MMMMM..... ^ 
> MMMMM..... |   
> MMMMMVVVVV |
> VVVVVVVVVV | 
> VVVVVVVVVV |
> VVVVVVVVVV 10 <--- vertical centers are aligned when cross axis alignment is
> CENTER or STRETCH
> VVVVVVVVVV |
> .....VVVVV |
> .....MMMMM |
> .....MMMMM v
>            -
> 
> >   -Scott
> > 
> > In the
> > > case where |collapse_margins_spacing_| is true, then the edge
> > > corresponding to
> > > the START or END alignment is aligned based on the max of the views'
> > > margins
> > > along that edge and the |inside_border_insets_| same edge.
> > >
> > > Effectively, |collapse_margins_spacing_| *does* only control how
> > > margins-against-spacing and margins-against-margins are handled.
> > >
> > > Again, clear as mud?
> > >
> > > https://codereview.chromium.org/2836313002/
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

That sounds right, but the description "common view edges are aligned at a point
that is the largest of all the views margins along that edge" implies on all
sides. Perhaps it's the 'common view edge' that is confusing?
I'll look at the patch shortly.

sky

https://codereview.chromium.org/2836313002/diff/540001/ui/views/layout/box_layout.cc File ui/views/layout/box_layout.cc (right): https://codereview.chromium.org/2836313002/diff/540001/ui/views/layout/box_layout.cc#newcode27 ui/views/layout/box_layout.cc:27: if (axis == HORIZONTAL_AXIS) use {} https://codereview.chromium.org/2836313002/diff/540001/ui/views/layout/box_layout.cc#newcode33 ui/views/layout/box_layout.cc:33: } ...

3 years, 6 months ago (2017-06-05 21:04:11 UTC) #166

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 6 months ago (2017-06-05 22:40:22 UTC) #167

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 6 months ago (2017-06-05 22:40:23 UTC) #168

kylix_rd

https://codereview.chromium.org/2836313002/diff/520001/ui/views/layout/box_layout.h File ui/views/layout/box_layout.h (right): https://codereview.chromium.org/2836313002/diff/520001/ui/views/layout/box_layout.h#newcode64 ui/views/layout/box_layout.h:64: // spacing/margins are collapsed base on the max of ...

3 years, 6 months ago (2017-06-06 18:22:35 UTC) #170

kylix_rd

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

3 years, 6 months ago (2017-06-06 18:22:42 UTC) #171

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2836313002/580001

3 years, 6 months ago (2017-06-06 18:23:22 UTC) #172

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 6 months ago (2017-06-06 20:21:36 UTC) #173

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 6 months ago (2017-06-06 20:21:38 UTC) #174

kylix_rd

On 2017/06/06 21:19:06, sky wrote: > LGTM - yay! WooHoo! Do we have another LGxM ...

3 years, 6 months ago (2017-06-07 17:40:47 UTC) #176

sky

You don't actually need one, but if you want him to verify this meets his ...

3 years, 6 months ago (2017-06-07 19:26:09 UTC) #177

kylix_rd

On 2017/06/07 19:26:09, sky wrote: > You don't actually need one, but if you want ...

3 years, 6 months ago (2017-06-07 20:04:46 UTC) #178

commit-bot: I haz the power

CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2836313002/580001

3 years, 6 months ago (2017-06-07 20:05:28 UTC) #180

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 6 months ago (2017-06-07 21:27:40 UTC) #181

commit-bot: I haz the power

Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/387580)

3 years, 6 months ago (2017-06-07 21:27:43 UTC) #182

commit-bot: I haz the power

CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2836313002/580001

3 years, 6 months ago (2017-06-08 13:15:41 UTC) #184

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 580001, "attempt_start_ts": 1496927724388700, "parent_rev": "b8edd52416f1e93bf2330543f5b888d2dadc9e14", "commit_rev": "3c489a82647c5e33a6e4fae55bc1a30d87c87d91"}

3 years, 6 months ago (2017-06-08 14:13:27 UTC) #185

commit-bot: I haz the power

Description was changed from ========== BoxLayout now suports per-view margins. Call BoxLayout::SetViewMargins(view, margins) to set ...

3 years, 6 months ago (2017-06-08 14:13:38 UTC) #186

commit-bot: I haz the power

3 years, 6 months ago (2017-06-08 14:13:41 UTC) #187

Message was sent while issue was closed.

Committed patchset #25 (id:580001) as
https://chromium.googlesource.com/chromium/src/+/3c489a82647c5e33a6e4fae55bc1...

Issue 2836313002: BoxLayout now suports per-view margins. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Clarifications, comments, and logic changes #

Patch Set 3 : BoxLayout now uses Margins property on the view #

Patch Set 4 : Added margin collapsing and a BoxLayout example #

Patch Set 5 : Refactor Combobox and Textfield creation into common functions #

Patch Set 6 : BoxLayoutExample complete #

Patch Set 7 : Comments, constant identifiers, removed ViewListener and ExampleModel #

Patch Set 8 : Adjust cross-axis margin to be the max of all margins or insets #

Patch Set 9 : Added unit tests. Fixed GetPreferredSize() calculations. Refactored some code. #

Patch Set 10 : Added example comment. Addressed other feedback. #

Patch Set 11 : Moved view properties to separate file. #

Patch Set 12 : Fixed crash in example and added ability to control size of child panels. #

Patch Set 13 : Uncollapsed margins in the cross-axis now align the views #

Patch Set 14 : Fixed BoxLayout for unit tests #

Patch Set 15 : Adjusted cross-axis calculations to better account for existing margins from other, larger peer vie… #

Patch Set 16 : Fix for BoxLayout unittests #

Patch Set 17 : Refactoring, style and formatting changes #

Patch Set 18 : More cleanup. #

Patch Set 19 : Better preferred size calculations and corrected layout for the cross axis. #

Patch Set 20 : Cross axis wasn't collapsing and didn't handle the leading/trailing edges correctly. Clarify what h… #

Patch Set 21 : When cross axis alignment is START or END, only collapse the anchoring edge when collapse_margins_s… #

Patch Set 22 : Remove unused local variable. #

Patch Set 23 : Don't collapse cross axis margins with adjacent margins in STRETCH or CENTER. #

Patch Set 24 : Fixed comment describing the BoxLayout behavior. #

Patch Set 25 : Merged with master. Removed cached orientation_ from ViewWrapper. #

Messages