BoxLayout: Added set_collapse_when_hidden(), like views::Label.

BoxLayout: Added set_collapse_when_hidden(), like views::Label.

When this is set to true, GetPreferredSize returns 0x0 if the host is
invisible. This allows you to set a BoxLayout-controlled view to
invisible and have it properly disappear (instead of taking up space).
The default behaviour is unchanged.

One day, this should be available on all views, but for now,
generalizing it to BoxLayout is better than nothing.

BUG=548551

[Matt Giuca, 2015-10-28 08:08:37]

mgiuca@chromium.org changed reviewers:
+ sadrul@chromium.org

[sadrul, 2015-10-29 17:57:29]

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

https://codereview.chromium.org/1422703004/diff/1/ui/views/layout/box_layout....
ui/views/layout/box_layout.cc:169: continue;
Most layout-managers currently explicitly check for child view's visibility
(like BoxLayout does here). Is there a good reason to change this?

[Matt Giuca, 2015-11-05 05:36:58]

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

https://codereview.chromium.org/1422703004/diff/1/ui/views/layout/box_layout....
ui/views/layout/box_layout.cc:169: continue;
On 2015/10/29 17:57:29, sadrul wrote:
> Most layout-managers currently explicitly check for child view's visibility
> (like BoxLayout does here). Is there a good reason to change this?

I think you're right about this. But let's break it down.

Essentially, I'm doing this because GridLayout *does not* respect its children's
visibility (and instead requires the child to have GetPreferredSize return 0 or
it allocates space for them).

It looks like BoxLayout *does* respect its children's visibility, and that's the
"right" thing to do. (yes?)

Now while that's going on, Label has collapse_when_hidden(), seemingly to get
around the GridLayout issue. But you're suggesting that Label shouldn't have
this feature just to support GridLayout; instead GridLayout should respect its
child view's visibility?

Have I got everything right so far?

If that's the case, can we fix GridLayout? The steps would be:
1. Add an option to GridLayout collapse_children_when_hidden to assume children
have a size of 0x0 if they are invisible.
2. Update all code using Label::collapse_when_hidden to use
GridLayout::collapse_children_when_hidden instead.
3. Remove Label::collapse_when_hidden.
4. (potentially) Remove GridLayout::collapse_when_hidden and make this the
default behaviour.

Or we could just skip to Step 4 and test thoroughly --- it isn't likely to break
much.

In the meantime though, I might just update my particular view
(ExclusiveAccessBubbleView) to use BoxLayout instead so that I'm not blocked on
this.

[Matt Giuca, 2015-11-06 05:44:48]

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

https://codereview.chromium.org/1422703004/diff/1/ui/views/layout/box_layout....
ui/views/layout/box_layout.cc:169: continue;
On 2015/11/05 05:36:58, Matt Giuca wrote:
> On 2015/10/29 17:57:29, sadrul wrote:
> > Most layout-managers currently explicitly check for child view's visibility
> > (like BoxLayout does here). Is there a good reason to change this?
> 
> I think you're right about this. But let's break it down.
> 
> Essentially, I'm doing this because GridLayout *does not* respect its
children's
> visibility (and instead requires the child to have GetPreferredSize return 0
or
> it allocates space for them).
> 
> It looks like BoxLayout *does* respect its children's visibility, and that's
the
> "right" thing to do. (yes?)
> 
> Now while that's going on, Label has collapse_when_hidden(), seemingly to get
> around the GridLayout issue. But you're suggesting that Label shouldn't have
> this feature just to support GridLayout; instead GridLayout should respect its
> child view's visibility?
> 
> Have I got everything right so far?
> 
> If that's the case, can we fix GridLayout? The steps would be:
> 1. Add an option to GridLayout collapse_children_when_hidden to assume
children
> have a size of 0x0 if they are invisible.
> 2. Update all code using Label::collapse_when_hidden to use
> GridLayout::collapse_children_when_hidden instead.
> 3. Remove Label::collapse_when_hidden.
> 4. (potentially) Remove GridLayout::collapse_when_hidden and make this the
> default behaviour.
> 
> Or we could just skip to Step 4 and test thoroughly --- it isn't likely to
break
> much.
> 
> In the meantime though, I might just update my particular view
> (ExclusiveAccessBubbleView) to use BoxLayout instead so that I'm not blocked
on
> this.

Note: Just mailed out https://codereview.chromium.org/1410413007 which frees up
my dependency on GridLayout (so I no longer need this CL).

Keeping it open so we can finish this discussion, then I will close this since I
agree with you that propagating the collapse_when_hidden hack from Label is the
wrong thing to do.

[Matt Giuca, 2015-11-09 02:54:26]

Description was changed from

==========
BoxLayout: Added set_collapse_when_hidden(), like views::Label.

When this is set to true, GetPreferredSize returns 0x0 if the host is
invisible. This allows you to set a BoxLayout-controlled view to
invisible and have it properly disappear (instead of taking up space).
The default behaviour is unchanged.

One day, this should be available on all views, but for now,
generalizing it to BoxLayout is better than nothing.

BUG=548551
==========

to

==========
BoxLayout: Added set_collapse_when_hidden(), like views::Label.

When this is set to true, GetPreferredSize returns 0x0 if the host is
invisible. This allows you to set a BoxLayout-controlled view to
invisible and have it properly disappear (instead of taking up space).
The default behaviour is unchanged.

One day, this should be available on all views, but for now,
generalizing it to BoxLayout is better than nothing.

BUG=548551
==========