Add Flex to views::BoxLayout.

Add Flex to views::BoxLayout.

This CL adds a per-view flex property to BoxLayout. The flex property functions
similarly to the CSS flexbox concept of flex, using the child view's preferred
size as the flex basis and then adding or removing space within each flexed
so that all views fit within the parent.

This CL also removes MAIN_AXIS_ALIGNMENT_FILL as it is superceded by
SetDefaultFlex().

BUG=386475
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285514

[sashab, 2014-07-01 06:52:04]

I'm thinking you should add a constructor that takes the default flex as a
parameter. That way, all those places you changed don't need to store the layout
at all - the constructor handles all the options they need.

https://codereview.chromium.org/360213002/diff/40001/ui/views/layout/box_layo...
File ui/views/layout/box_layout.h (right):

https://codereview.chromium.org/360213002/diff/40001/ui/views/layout/box_layo...
ui/views/layout/box_layout.h:44: // in-between the child views.
Is this TODO still needed? This can be emulated with all child views having a
flex of 1.

https://codereview.chromium.org/360213002/diff/40001/ui/views/layout/box_layo...
ui/views/layout/box_layout.h:80: void set_default_flex(int flex) { default_flex_
= flex; }
Is there a reason these methods use underscores and the other methods use camel
case?

https://codereview.chromium.org/360213002/diff/40001/ui/views/layout/box_layo...
ui/views/layout/box_layout.h:87: void SetFlexForViewAt(int index, int flex);
Can flex be negative? Maybe add 'The flex cannot be negative' and add a
DCHECK().

I also think you can make this comment a bit clearer:
Sets the flex weight for the view at |index|. Free space along the main axis is
distributed to views in the ratio of their flex weights. Similarly, if the views
will overflow the parent, space is subtracted in these ratios. A flex of 0 means
this view is not resized.

https://codereview.chromium.org/360213002/diff/40001/ui/views/layout/box_layo...
ui/views/layout/box_layout.h:100: // Returns the flex for the specified index.
Nit: 'Returns the flex *for the view*...'

https://codereview.chromium.org/360213002/diff/40001/ui/views/layout/box_layo...
ui/views/layout/box_layout.h:153: // The flex weight for views if none is set.
Add to this comment 'Defaults to 0.'

[calamity, 2014-07-01 08:19:44]

+benwells

I don't think it's worth adding a constructor with default flex in it since it's
a pretty niche use case. If we add a bunch of constructors, they should be for
important properties (like insets and spacing) and we don't want an int to get
lost among the parameters.

https://codereview.chromium.org/360213002/diff/40001/ui/views/layout/box_layo...
File ui/views/layout/box_layout.h (right):

https://codereview.chromium.org/360213002/diff/40001/ui/views/layout/box_layo...
ui/views/layout/box_layout.h:44: // in-between the child views.
On 2014/07/01 06:52:04, sasha_b wrote:
> Is this TODO still needed? This can be emulated with all child views having a
> flex of 1.

This TODO outlines spreading the blank space to the spaces between the views
rather than among the views.

https://codereview.chromium.org/360213002/diff/40001/ui/views/layout/box_layo...
ui/views/layout/box_layout.h:80: void set_default_flex(int flex) { default_flex_
= flex; }
On 2014/07/01 06:52:04, sasha_b wrote:
> Is there a reason these methods use underscores and the other methods use
camel
> case?

Setting a property verbatim can be inlined and unix_cased. Anything else is
CamelCased and in the .cc.

https://codereview.chromium.org/360213002/diff/40001/ui/views/layout/box_layo...
ui/views/layout/box_layout.h:87: void SetFlexForViewAt(int index, int flex);
On 2014/07/01 06:52:04, sasha_b wrote:
> Can flex be negative? Maybe add 'The flex cannot be negative' and add a
> DCHECK().
> 
> I also think you can make this comment a bit clearer:
> Sets the flex weight for the view at |index|. Free space along the main axis
is
> distributed to views in the ratio of their flex weights. Similarly, if the
views
> will overflow the parent, space is subtracted in these ratios. A flex of 0
means
> this view is not resized.

Done.

https://codereview.chromium.org/360213002/diff/40001/ui/views/layout/box_layo...
ui/views/layout/box_layout.h:100: // Returns the flex for the specified index.
On 2014/07/01 06:52:04, sasha_b wrote:
> Nit: 'Returns the flex *for the view*...'

I think this is disingenuous as it implies the flex weight is linked to the
view. Also updated comment above to reflect that this is not the case.

https://codereview.chromium.org/360213002/diff/40001/ui/views/layout/box_layo...
ui/views/layout/box_layout.h:153: // The flex weight for views if none is set.
On 2014/07/01 06:52:04, sasha_b wrote:
> Add to this comment 'Defaults to 0.'

Done.

[sashab, 2014-07-02 01:09:35]

Hmm, I'm starting to think maybe this change should actually be in a subclass of
BoxLayout, maybe FlexBoxLayout, where the flex actually defaults to 1. If you're
going to place a horizontal layout with equally stretched views, a FlexBoxLayout
is actually what you want, not a BoxLayout with a default flex of 1 (which is
kind of a hack on BoxLayout when you think about it).

Just my thoughts, though, I'm happy to do whatever you think is best. But maybe
the two separate classes is the way to go.

[calamity, 2014-07-02 03:20:58]

On 2014/07/02 01:09:35, sasha_b wrote:
> Hmm, I'm starting to think maybe this change should actually be in a subclass
of
> BoxLayout, maybe FlexBoxLayout, where the flex actually defaults to 1. If
you're
> going to place a horizontal layout with equally stretched views, a
FlexBoxLayout
> is actually what you want, not a BoxLayout with a default flex of 1 (which is
> kind of a hack on BoxLayout when you think about it).
> 
> Just my thoughts, though, I'm happy to do whatever you think is best. But
maybe
> the two separate classes is the way to go.

My preference is to be as close to the CSS flexbox api as possible since it
gives us an already thought-out api for the whole concept. The display: flex
property captures the entire flexible box concept and says that if you don't
want flex, don't specify a flex. A CSS example of default flex is at
http://jsfiddle.net/9WZCM/3/ . set_default_flex is really just a convenience to
deal with the fact that we have no selectors. There is no separate display type
for a default flexy flexbox or a nonflexible box layout.

I'm not sure what we'd get with a new LayoutManager except different default
settings. We'd still want to have set_default_flex around because you may want
to have things double size by default (http://jsfiddle.net/9WZCM/4/).

Is the problem just that the name BoxLayout doesn't mention flex (note that
webkit's old flexbox implementation was called -webkit-box. See
http://css-tricks.com/old-flexbox-and-new-flexbox/)? Would changing the name to
FlexBoxLayout or FlexLayout help at all?

[sashab, 2014-07-02 06:39:32]

After offline discussion I am happy keeping it as one class. We certainly don't
want a FlexLayout passed around as a BoxLayout.

[benwells, 2014-07-02 08:02:26]

https://codereview.chromium.org/360213002/diff/60001/ui/views/layout/box_layo...
File ui/views/layout/box_layout.cc (right):

https://codereview.chromium.org/360213002/diff/60001/ui/views/layout/box_layo...
ui/views/layout/box_layout.cc:31: flex_map_[index] = flex_weight;
Nit: check flex_weight >= 0

https://codereview.chromium.org/360213002/diff/60001/ui/views/layout/box_layo...
ui/views/layout/box_layout.cc:67: padding_unit = free_space / flex_sum;
As discussed maybe there is a simpler way to do this with integer arithmetic.

https://codereview.chromium.org/360213002/diff/60001/ui/views/layout/box_layo...
ui/views/layout/box_layout.cc:118: current_padding += remainder_change /
flex_sum;
shouldn't this be += (current_remainder + remainder_change) / flex_sum?

https://codereview.chromium.org/360213002/diff/60001/ui/views/layout/box_layo...
ui/views/layout/box_layout.cc:121: if (std::abs(current_remainder) > flex_sum /
2)
reminder: you said this should be >= when we were discussing it.

[calamity, 2014-07-03 01:54:28]

https://codereview.chromium.org/360213002/diff/60001/ui/views/layout/box_layo...
File ui/views/layout/box_layout.cc (right):

https://codereview.chromium.org/360213002/diff/60001/ui/views/layout/box_layo...
ui/views/layout/box_layout.cc:31: flex_map_[index] = flex_weight;
On 2014/07/02 08:02:26, benwells wrote:
> Nit: check flex_weight >= 0

Done.

https://codereview.chromium.org/360213002/diff/60001/ui/views/layout/box_layo...
ui/views/layout/box_layout.cc:67: padding_unit = free_space / flex_sum;
On 2014/07/02 08:02:26, benwells wrote:
> As discussed maybe there is a simpler way to do this with integer arithmetic.

Done.

https://codereview.chromium.org/360213002/diff/60001/ui/views/layout/box_layo...
ui/views/layout/box_layout.cc:118: current_padding += remainder_change /
flex_sum;
On 2014/07/02 08:02:26, benwells wrote:
> shouldn't this be += (current_remainder + remainder_change) / flex_sum?

I don't believe so. The current remainder is factored into the rounding below,
after current remainder is % flex_sum. Here, we are just accounting for the
information that gets lost in that mod. Either way, this has all been changed as
per your suggestion.

https://codereview.chromium.org/360213002/diff/60001/ui/views/layout/box_layo...
ui/views/layout/box_layout.cc:121: if (std::abs(current_remainder) > flex_sum /
2)
On 2014/07/02 08:02:26, benwells wrote:
> reminder: you said this should be >= when we were discussing it.

Done. Also had to move the / 2 to the other side to avoid a bad division case.

[benwells, 2014-07-03 07:29:59]

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

https://codereview.chromium.org/360213002/diff/100001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:51: int main_free_space = 0;
Nit: move this to just before it is used (line 65 i think)

https://codereview.chromium.org/360213002/diff/100001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:65: if (num_visible) {
Can you return here if !num_visible?

https://codereview.chromium.org/360213002/diff/100001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:119: if (std::abs(current_remainder) * 2 >=
flex_sum)
It feels magical that this works and doesn't ever allocate an extra pixel / too
few pixels. I want to work through some examples and figure out the magic.

https://codereview.chromium.org/360213002/diff/100001/ui/views/layout/box_lay...
File ui/views/layout/box_layout_unittest.cc (right):

https://codereview.chromium.org/360213002/diff/100001/ui/views/layout/box_lay...
ui/views/layout/box_layout_unittest.cc:387: for (size_t i = 0; i <
main_alignments.size(); ++i) {
What is this loop for? It doesn't seem to do anything except repeat code three
times identically. (or maybe I'm blind and can't see the change ...).

[calamity, 2014-07-03 08:31:24]

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

https://codereview.chromium.org/360213002/diff/100001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:51: int main_free_space = 0;
On 2014/07/03 07:29:59, benwells wrote:
> Nit: move this to just before it is used (line 65 i think)

Done.

https://codereview.chromium.org/360213002/diff/100001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:65: if (num_visible) {
On 2014/07/03 07:29:59, benwells wrote:
> Can you return here if !num_visible?

Done.

https://codereview.chromium.org/360213002/diff/100001/ui/views/layout/box_lay...
File ui/views/layout/box_layout_unittest.cc (right):

https://codereview.chromium.org/360213002/diff/100001/ui/views/layout/box_lay...
ui/views/layout/box_layout_unittest.cc:387: for (size_t i = 0; i <
main_alignments.size(); ++i) {
On 2014/07/03 07:29:59, benwells wrote:
> What is this loop for? It doesn't seem to do anything except repeat code three
> times identically. (or maybe I'm blind and can't see the change ...).

Oh wups. This needs to set the alignment. The idea was to enforce that the 3
existing values of main alignment don't affect the view when flex is set. We
could get rid of this I guess.

[benwells, 2014-07-04 07:27:57]

As I won't be here next week feel free to send this on to an owner.

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

https://codereview.chromium.org/360213002/diff/120001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:63: total_main_axis_size -=
between_child_spacing_;
Nit: this line can move after the if (!num_visible) check.

https://codereview.chromium.org/360213002/diff/120001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:121: current_remainder %= flex_sum;
As discussed today I'm worried this will leak pixels and the rounding isn't
always going to account for it. The rounding might also use extra pixels
sometimes.

This could be tested easily by DCHECKING that all the allocation padding sums to
the available padding and then laying out a bunch of scenarios.

[calamity, 2014-07-07 05:11:18]

Since I reworked the padding code, it'd be good to get you to take another pass.

Thanks.

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

https://codereview.chromium.org/360213002/diff/120001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:63: total_main_axis_size -=
between_child_spacing_;
On 2014/07/04 07:27:56, benwells wrote:
> Nit: this line can move after the if (!num_visible) check.

Done.

https://codereview.chromium.org/360213002/diff/120001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:121: current_remainder %= flex_sum;
On 2014/07/04 07:27:56, benwells wrote:
> As discussed today I'm worried this will leak pixels and the rounding isn't
> always going to account for it. The rounding might also use extra pixels
> sometimes.
> 
> This could be tested easily by DCHECKING that all the allocation padding sums
to
> the available padding and then laying out a bunch of scenarios.

Indeed it does. Reworked algorithm.

[benwells, 2014-07-18 02:52:25]

just one comment....

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

https://codereview.chromium.org/360213002/diff/200001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:141: DCHECK_LE(MainAxisPosition(child_area) +
MainAxisSize(child_area),
I think DCHECK_EQ(total_padding, main_free_space) would be clearer. I'm not sure
what this DCHECK is testing.

[calamity, 2014-07-18 03:20:03]

+sky for OWNERS (particularly ui/views)

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

https://codereview.chromium.org/360213002/diff/200001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:141: DCHECK_LE(MainAxisPosition(child_area) +
MainAxisSize(child_area),
On 2014/07/18 02:52:25, benwells wrote:
> I think DCHECK_EQ(total_padding, main_free_space) would be clearer. I'm not
sure
> what this DCHECK is testing.

Done.

[benwells, 2014-07-18 04:17:17]

On 2014/07/18 03:20:03, calamity wrote:
> +sky for OWNERS (particularly ui/views)
> 
>
https://codereview.chromium.org/360213002/diff/200001/ui/views/layout/box_lay...
> File ui/views/layout/box_layout.cc (right):
> 
>
https://codereview.chromium.org/360213002/diff/200001/ui/views/layout/box_lay...
> ui/views/layout/box_layout.cc:141: DCHECK_LE(MainAxisPosition(child_area) +
> MainAxisSize(child_area),
> On 2014/07/18 02:52:25, benwells wrote:
> > I think DCHECK_EQ(total_padding, main_free_space) would be clearer. I'm not
> sure
> > what this DCHECK is testing.
> 
> Done.

lgtm then :)

[sky, 2014-07-18 15:54:00]

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

https://codereview.chromium.org/360213002/diff/220001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:31: DCHECK(index >= 0);
DCHECK_GE for all these.

https://codereview.chromium.org/360213002/diff/220001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:51: int total_main_axis_size = 0;
This code needs some comments as to what each of the loops are doing.

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

https://codereview.chromium.org/360213002/diff/220001/ui/views/layout/box_lay...
ui/views/layout/box_layout.h:89: void SetFlexForViewAt(int index, int flex);
Using indices like this is fragile and I suspect prone to unexpected conditions
(eg you set this for a view but a view and a new view gets inserted before the
view). I think this would make more sense to be View based.

https://codereview.chromium.org/360213002/diff/220001/ui/views/layout/box_lay...
ui/views/layout/box_layout.h:106: int GetFlexForViewAt(int index);
const

[calamity, 2014-07-22 05:35:15]

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

https://codereview.chromium.org/360213002/diff/220001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:31: DCHECK(index >= 0);
On 2014/07/18 15:54:00, sky wrote:
> DCHECK_GE for all these.

Done.

https://codereview.chromium.org/360213002/diff/220001/ui/views/layout/box_lay...
ui/views/layout/box_layout.cc:51: int total_main_axis_size = 0;
On 2014/07/18 15:54:00, sky wrote:
> This code needs some comments as to what each of the loops are doing.

Done.

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

https://codereview.chromium.org/360213002/diff/220001/ui/views/layout/box_lay...
ui/views/layout/box_layout.h:89: void SetFlexForViewAt(int index, int flex);
On 2014/07/18 15:54:00, sky wrote:
> Using indices like this is fragile and I suspect prone to unexpected
conditions
> (eg you set this for a view but a view and a new view gets inserted before the
> view). I think this would make more sense to be View based.

Done.

https://codereview.chromium.org/360213002/diff/220001/ui/views/layout/box_lay...
ui/views/layout/box_layout.h:106: int GetFlexForViewAt(int index);
On 2014/07/18 15:54:00, sky wrote:
> const

Done.

[calamity, 2014-07-22 05:58:46]

Hmm. Actually, I'm a bit concerned that this implementation will have dangling
pointers in the map which is probably the reason I used indices. This could
cause a flake where a new view is given an old view's address causing weird
layout if not cleared properly..

I don't think there's a good way for a layout manager to observe a view for
destruction.. GridLayout also sort of has this problem but is a far more static
layout that you wouldn't expect to be deleting views from.

Is it sufficient to just add a warning that clients should clear the flex when
deleting views?

[sky, 2014-07-22 16:56:14]

On 2014/07/22 05:58:46, calamity wrote:
> Hmm. Actually, I'm a bit concerned that this implementation will have dangling
> pointers in the map which is probably the reason I used indices. This could
> cause a flake where a new view is given an old view's address causing weird
> layout if not cleared properly..
> 
> I don't think there's a good way for a layout manager to observe a view for
> destruction.. GridLayout also sort of has this problem but is a far more
static
> layout that you wouldn't expect to be deleting views from.
> 
> Is it sufficient to just add a warning that clients should clear the flex when
> deleting views?

LayoutManager::ViewRemoved? Is it not working correctly?

[calamity, 2014-07-23 05:06:39]

On 2014/07/22 16:56:14, sky wrote:
> On 2014/07/22 05:58:46, calamity wrote:
> > Hmm. Actually, I'm a bit concerned that this implementation will have
dangling
> > pointers in the map which is probably the reason I used indices. This could
> > cause a flake where a new view is given an old view's address causing weird
> > layout if not cleared properly..
> > 
> > I don't think there's a good way for a layout manager to observe a view for
> > destruction.. GridLayout also sort of has this problem but is a far more
> static
> > layout that you wouldn't expect to be deleting views from.
> > 
> > Is it sufficient to just add a warning that clients should clear the flex
when
> > deleting views?
> 
> LayoutManager::ViewRemoved? Is it not working correctly?

Ah, didn't see that. Cool. Next problem: Since ViewRemoved depends on the
LayoutManager being installed to a host, I feel like BoxLayout should now only
support laying out the host that it's installed to (like GridLayout). So it
would DCHECK(!host_) on Installed and DCHECK_EQ(host_, host) in Layout().

This means the only way to use BoxLayout is to set it as a LayoutManager of a
view. I don't think anything uses BoxLayout in a different way (except the
unittests).

WDYT?

[sky, 2014-07-23 15:49:08]

SGTM. If the tests aren't using BoxLayout installed on a view I would
update the test.

On Tue, Jul 22, 2014 at 10:06 PM,  <calamity@chromium.org> wrote:
> On 2014/07/22 16:56:14, sky wrote:
>>
>> On 2014/07/22 05:58:46, calamity wrote:
>> > Hmm. Actually, I'm a bit concerned that this implementation will have
>
> dangling
>>
>> > pointers in the map which is probably the reason I used indices. This
>> > could
>> > cause a flake where a new view is given an old view's address causing
>> > weird
>> > layout if not cleared properly..
>> >
>> > I don't think there's a good way for a layout manager to observe a view
>> > for
>> > destruction.. GridLayout also sort of has this problem but is a far more
>> static
>> > layout that you wouldn't expect to be deleting views from.
>> >
>> > Is it sufficient to just add a warning that clients should clear the
>> > flex
>
> when
>>
>> > deleting views?
>
>
>> LayoutManager::ViewRemoved? Is it not working correctly?
>
>
> Ah, didn't see that. Cool. Next problem: Since ViewRemoved depends on the
> LayoutManager being installed to a host, I feel like BoxLayout should now
> only
> support laying out the host that it's installed to (like GridLayout). So it
> would DCHECK(!host_) on Installed and DCHECK_EQ(host_, host) in Layout().
>
> This means the only way to use BoxLayout is to set it as a LayoutManager of
> a
> view. I don't think anything uses BoxLayout in a different way (except the
> unittests).
>
> WDYT?
>
> https://codereview.chromium.org/360213002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[calamity, 2014-07-24 07:36:01]

Done.

[sky, 2014-07-24 16:57:17]

LGTM

[calamity, 2014-07-25 04:16:01]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-07-25 04:18:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/calamity@chromium.org/360213002/320001

[commit-bot: I haz the power, 2014-07-25 07:05:49]

Change committed as 285514