BoxLayout now suports per-view margins.

ui/views/layout/box_layout.cc

Index: ui/views/layout/box_layout.cc
diff --git a/ui/views/layout/box_layout.cc b/ui/views/layout/box_layout.cc
index 7e5abff4880af33a0b34583c3d0c7f127b7480c5..f2897f2225da3908613689d1f654e81f17d628c8 100644
--- a/ui/views/layout/box_layout.cc
+++ b/ui/views/layout/box_layout.cc
@@ -11,10 +11,49 @@
 
 namespace views {
 
+BoxLayout::ViewWrapper::ViewWrapper() : view_(nullptr), layout_(nullptr) {}
+
+BoxLayout::ViewWrapper::ViewWrapper(const BoxLayout* layout, View* view)
+    : view_(view), layout_(layout) {
+  gfx::Insets* margins = view_ ? view_->GetProperty(kMarginsKey) : nullptr;
+  if (margins)
+    margins_ = *margins;
+}
+
+BoxLayout::ViewWrapper::~ViewWrapper() {}
+
+int BoxLayout::ViewWrapper::GetHeightForWidth(int width) const {
+  return layout_->collapse_margins_spacing_
+             ? view_->GetHeightForWidth(width)
+             : view_->GetHeightForWidth(width - margins_.width()) +

[sky, 2017/05/09 19:26:03]

Make sure you don't go negative here.

[kylix_rd, 2017/05/10 20:35:19]

Done.

+                   margins_.height();
+}
+
+gfx::Size BoxLayout::ViewWrapper::GetPreferredSize() const {
+  gfx::Size preferred_size = view_->GetPreferredSize();
+  if (!layout_->collapse_margins_spacing_)
+    preferred_size.Enlarge(margins_.width(), margins_.height());
+  return preferred_size;
+}
+
+void BoxLayout::ViewWrapper::SetBoundsRect(const gfx::Rect& bounds) {
+  if (layout_->collapse_margins_spacing_)
+    view_->SetBounds(bounds.x(), bounds.y(), bounds.width(), bounds.height());
+  else
+    view_->SetBounds(bounds.x() + margins_.left(), bounds.y() + margins_.top(),

[sky, 2017/05/09 19:26:03]

optional: use {} around this if given the body spans multiple lines.

[kylix_rd, 2017/05/10 20:35:19]

Done.

+                     bounds.width() - margins_.width(),

[sky, 2017/05/09 19:26:03]

Do you need to protect against going negative?

[kylix_rd, 2017/05/10 20:35:19]

Done.

+                     bounds.height() - margins_.height());
+}
+
+bool BoxLayout::ViewWrapper::visible() const {
+  return view_->visible();
+}
+
 BoxLayout::BoxLayout(BoxLayout::Orientation orientation,
                      int inside_border_horizontal_spacing,
                      int inside_border_vertical_spacing,
-                     int between_child_spacing)
+                     int between_child_spacing,
+                     bool collapse_margins_spacing)
     : orientation_(orientation),
       inside_border_insets_(inside_border_vertical_spacing,
                             inside_border_horizontal_spacing),
@@ -23,8 +62,18 @@ BoxLayout::BoxLayout(BoxLayout::Orientation orientation,
       cross_axis_alignment_(CROSS_AXIS_ALIGNMENT_STRETCH),
       default_flex_(0),
       minimum_cross_axis_size_(0),
-      host_(NULL) {
-}
+      collapse_margins_spacing_(collapse_margins_spacing),
+      host_(NULL) {}
+
+BoxLayout::BoxLayout(BoxLayout::Orientation orientation,
+                     int inside_border_horizontal_spacing,
+                     int inside_border_vertical_spacing,
+                     int between_child_spacing)
+    : BoxLayout(orientation,
+                inside_border_horizontal_spacing,
+                inside_border_vertical_spacing,
+                between_child_spacing,
+                false) {}
 
 BoxLayout::~BoxLayout() {
 }
@@ -49,23 +98,25 @@ void BoxLayout::SetDefaultFlex(int default_flex) {
 
 void BoxLayout::Layout(View* host) {
   DCHECK_EQ(host_, host);
-  gfx::Rect child_area(host->GetLocalBounds());
-  child_area.Inset(host->GetInsets());
+  gfx::Rect child_area(host->GetContentsBounds());
   child_area.Inset(inside_border_insets_);
+  AdjustMainAxisForMargin(&child_area);
 
   int total_main_axis_size = 0;
   int num_visible = 0;
   int flex_sum = 0;
   // Calculate the total size of children in the main axis.
   for (int i = 0; i < host->child_count(); ++i) {
-    View* child = host->child_at(i);
-    if (!child->visible())
+    ViewWrapper child(this, host->child_at(i));
+    ViewWrapper next(this, NextVisibleView(host, i));
+    if (!child.visible())
       continue;
-    int flex = GetFlexForView(child);
+    int flex = GetFlexForView(child.view());
     int child_main_axis_size = MainAxisSizeForView(child, child_area.width());
     if (child_main_axis_size == 0 && flex == 0)
       continue;
-    total_main_axis_size += child_main_axis_size + between_child_spacing_;
+    total_main_axis_size +=
+        child_main_axis_size + MainAxisMarginBetweenViews(child, next);
     ++num_visible;
     flex_sum += flex;
   }
@@ -106,8 +157,9 @@ void BoxLayout::Layout(View* host) {
   int total_padding = 0;
   int current_flex = 0;
   for (int i = 0; i < host->child_count(); ++i) {
-    View* child = host->child_at(i);
-    if (!child->visible())
+    ViewWrapper child(this, host->child_at(i));
+    ViewWrapper next(this, NextVisibleView(host, i));
+    if (!child.visible())
       continue;
 
     // TODO(bruthig): Fix this. The main axis should be calculated before
@@ -131,8 +183,8 @@ void BoxLayout::Layout(View* host) {
 
     // Calculate flex padding.
     int current_padding = 0;
-    if (GetFlexForView(child) > 0) {
-      current_flex += GetFlexForView(child);
+    if (GetFlexForView(child.view()) > 0) {
+      current_flex += GetFlexForView(child.view());
       int quot = (main_free_space * current_flex) / flex_sum;
       int rem = (main_free_space * current_flex) % flex_sum;
       current_padding = quot - total_padding;
@@ -147,12 +199,14 @@ void BoxLayout::Layout(View* host) {
     // See https://crbug.com/682266.
     int child_main_axis_size = MainAxisSizeForView(child, child_area.width());
     SetMainAxisSize(child_main_axis_size + current_padding, &bounds);
-    if (MainAxisSize(bounds) > 0 || GetFlexForView(child) > 0)
-      main_position += MainAxisSize(bounds) + between_child_spacing_;
+    if (MainAxisSize(bounds) > 0 || GetFlexForView(child.view()) > 0)
+      main_position +=
+          MainAxisSize(bounds) + MainAxisMarginBetweenViews(child, next);
 
     // Clamp child view bounds to |child_area|.
     bounds.Intersect(child_area);
-    child->SetBoundsRect(bounds);
+    AdjustCrossAxisForMargin(child, child_area, &bounds);
+    child.SetBoundsRect(bounds);
   }
 
   // Flex views should have grown/shrunk to consume all free space.
@@ -165,12 +219,12 @@ gfx::Size BoxLayout::GetPreferredSize(const View* host) const {
   // Calculate the child views' preferred width.
   int width = 0;
   if (orientation_ == kVertical) {
-    for (int i = 0; i < host->child_count(); ++i) {
-      const View* child = host->child_at(i);
-      if (!child->visible())
+    for (int i = 0; i < host_->child_count(); ++i) {
+      const ViewWrapper child(this, host_->child_at(i));
+      if (!child.visible())
         continue;
 
-      width = std::max(width, child->GetPreferredSize().width());
+      width = std::max(width, child.GetPreferredSize().width());
     }
     width = std::max(width, minimum_cross_axis_size_);
   }
@@ -194,7 +248,7 @@ void BoxLayout::ViewRemoved(View* host, View* view) {
 }
 
 int BoxLayout::GetFlexForView(const View* view) const {
-  std::map<const View*, int>::const_iterator it = flex_map_.find(view);
+  FlexMap::const_iterator it = flex_map_.find(view);
   if (it == flex_map_.end())
     return default_flex_;
 
@@ -245,26 +299,97 @@ void BoxLayout::SetCrossAxisPosition(int position, gfx::Rect* rect) const {
     rect->set_y(position);
 }
 
-int BoxLayout::MainAxisSizeForView(const View* view,
+int BoxLayout::MainAxisSizeForView(const ViewWrapper& view,
                                    int child_area_width) const {
   return orientation_ == kHorizontal
-             ? view->GetPreferredSize().width()
-             : view->GetHeightForWidth(cross_axis_alignment_ ==
-                                               CROSS_AXIS_ALIGNMENT_STRETCH
-                                           ? child_area_width
-                                           : view->GetPreferredSize().width());
+             ? view.GetPreferredSize().width()
+             : view.GetHeightForWidth(cross_axis_alignment_ ==
+                                              CROSS_AXIS_ALIGNMENT_STRETCH
+                                          ? child_area_width
+                                          : view.GetPreferredSize().width());
 }
 
-int BoxLayout::CrossAxisSizeForView(const View* view) const {
+int BoxLayout::MainAxisMarginBetweenViews(const ViewWrapper& left,
+                                          const ViewWrapper& right) const {
+  return orientation_ == Orientation::kHorizontal
+             ? (collapse_margins_spacing_ && left.view() && right.view()
+                    ? std::max(between_child_spacing_,
+                               std::max(left.margins().right(),
+                                        right.margins().left()))
+                    : between_child_spacing_)
+             : (collapse_margins_spacing_ && left.view() && right.view()
+                    ? std::max(between_child_spacing_,
+                               std::max(left.margins().bottom(),
+                                        right.margins().top()))
+                    : between_child_spacing_);
+}
+
+void BoxLayout::AdjustMainAxisForMargin(gfx::Rect* rect) const {
+  if (collapse_margins_spacing_) {

[sky, 2017/05/09 19:26:03]

Generally we prefer early return than lots of nesting.

[kylix_rd, 2017/05/09 20:07:06]

"if" block gone in subsequent patch.

+    const ViewWrapper first(this, FirstVisibleView(host_));
+    const ViewWrapper last(this, LastVisibleView(host_));
+    if (!(first.view() && last.view()))
+      return;
+    if (orientation_ == Orientation::kHorizontal) {
+      rect->set_x(rect->x() + std::max(inside_border_insets_.left(),
+                                       first.margins().left()));
+      rect->set_width((rect->right() - std::max(inside_border_insets_.right(),

[sky, 2017/05/09 19:26:03]

Make sure you don't end up with a negative width (or height below).

[kylix_rd, 2017/05/10 20:35:19]

Done.

+                                                last.margins().right())) -
+                      rect->x());
+    } else {
+      rect->set_y(rect->y() +
+                  std::max(inside_border_insets_.top(), first.margins().top()));
+      rect->set_height(
+          (rect->bottom() -
+           std::max(inside_border_insets_.bottom(), last.margins().bottom())) -
+          rect->y());
+    }
+  }
+}
+
+void BoxLayout::AdjustCrossAxisForMargin(const ViewWrapper& child,

[sky, 2017/05/09 19:26:03]

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.

[kylix_rd, 2017/05/09 20:07:06]

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.

[sky, 2017/05/09 23:55:29]

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.

+                                         const gfx::Rect& client,
+                                         gfx::Rect* rect) const {
+  if (collapse_margins_spacing_) {
+    if (orientation_ == Orientation::kVertical) {
+      rect->SetByBounds(
+          rect->x() > client.x()
+              ? rect->x()
+              : rect->x() + std::max(inside_border_insets_.left(),
+                                     child.margins().left()),
+          rect->y(),
+          rect->right() < client.right()
+              ? rect->right()
+              : rect->right() - std::max(inside_border_insets_.right(),
+                                         child.margins().right()),
+          rect->bottom());
+    } else {
+      rect->SetByBounds(
+          rect->x(),
+          rect->y() > client.y()
+              ? rect->y()
+              : rect->y() + std::max(inside_border_insets_.top(),
+                                     child.margins().top()),
+          rect->right(),
+          rect->bottom() < client.bottom()
+              ? rect->bottom()
+              : rect->bottom() - std::max(inside_border_insets_.bottom(),
+                                          child.margins().bottom()));
+    }
+  }
+}
+
+int BoxLayout::CrossAxisSizeForView(const ViewWrapper& view) const {
   // TODO(bruthig): For horizontal case use the available width and not the
   // preferred width. See https://crbug.com/682266.
   return orientation_ == kVertical
-             ? view->GetPreferredSize().width()
-             : view->GetHeightForWidth(view->GetPreferredSize().width());
+             ? view.GetPreferredSize().width()
+             : view.GetHeightForWidth(view.GetPreferredSize().width());
 }
 
 gfx::Size BoxLayout::GetPreferredSizeForChildWidth(const View* host,
                                                    int child_area_width) const {
+  DCHECK_EQ(host, host_);
   gfx::Rect child_area_bounds;
 
   if (orientation_ == kHorizontal) {
@@ -272,34 +397,36 @@ gfx::Size BoxLayout::GetPreferredSizeForChildWidth(const View* host,
     // default behavior of GridLayout::GetPreferredHeightForWidth().
     // TODO(estade|bruthig): Fix this See // https://crbug.com/682266.
     int position = 0;
-    for (int i = 0; i < host->child_count(); ++i) {
-      const View* child = host->child_at(i);
-      if (!child->visible())
+    for (int i = 0; i < host_->child_count(); ++i) {
+      const ViewWrapper child(this, host_->child_at(i));
+      const ViewWrapper next(this, NextVisibleView(host, i));
+      if (!child.visible())
         continue;
 
-      gfx::Size size(child->GetPreferredSize());
+      gfx::Size size(child.GetPreferredSize());
       if (size.IsEmpty())
         continue;
 
       gfx::Rect child_bounds(position, 0, size.width(), size.height());
       child_area_bounds.Union(child_bounds);
-      position += size.width() + between_child_spacing_;
+      position += size.width() + MainAxisMarginBetweenViews(child, next);
     }
     child_area_bounds.set_height(
         std::max(child_area_bounds.height(), minimum_cross_axis_size_));
   } else {
     int height = 0;
-    for (int i = 0; i < host->child_count(); ++i) {
-      const View* child = host->child_at(i);
-      if (!child->visible())
+    for (int i = 0; i < host_->child_count(); ++i) {
+      const ViewWrapper child(this, host_->child_at(i));
+      const ViewWrapper next(this, NextVisibleView(host, i));
+      if (!child.visible())
         continue;
 
       // Use the child area width for getting the height if the child is
       // supposed to stretch. Use its preferred size otherwise.
       int extra_height = MainAxisSizeForView(child, child_area_width);
       // Only add |between_child_spacing_| if this is not the only child.
-      if (height != 0 && extra_height > 0)
-        height += between_child_spacing_;
+      if (next.view() && extra_height > 0)
+        height += MainAxisMarginBetweenViews(child, next);
       height += extra_height;
     }
 
@@ -307,7 +434,7 @@ gfx::Size BoxLayout::GetPreferredSizeForChildWidth(const View* host,
     child_area_bounds.set_height(height);
   }
 
-  gfx::Size non_child_size = NonChildSize(host);
+  gfx::Size non_child_size = NonChildSize(host_);
   return gfx::Size(child_area_bounds.width() + non_child_size.width(),
                    child_area_bounds.height() + non_child_size.height());
 }
@@ -318,4 +445,28 @@ gfx::Size BoxLayout::NonChildSize(const View* host) const {
                    insets.height() + inside_border_insets_.height());
 }
 
+View* BoxLayout::NextVisibleView(View* host, int index) const {
+  DCHECK_EQ(host_, host);

[sky, 2017/05/09 19:26:03]

Why bother with passing in host when host_ is already a member? Same comment for
the other functions.

[kylix_rd, 2017/05/09 20:07:06]

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

+  for (int i = index + 1; i < host->child_count(); ++i) {
+    View* result = host->child_at(i);
+    if (result->visible())
+      return result;
+  }
+  return nullptr;
+}
+
+View* BoxLayout::FirstVisibleView(View* host) const {
+  return NextVisibleView(host, -1);
+}
+
+View* BoxLayout::LastVisibleView(View* host) const {
+  DCHECK_EQ(host_, host);
+  for (int i = host->child_count() - 1; i >= 0; --i) {
+    View* result = host->child_at(i);
+    if (result->visible())
+      return result;
+  }
+  return nullptr;
+}
+
 }  // namespace views