BoxLayout now suports per-view margins.

ui/views/layout/box_layout.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/views/layout/box_layout.h"
 
 #include <algorithm>
 
 #include "ui/gfx/geometry/rect.h"
 #include "ui/views/view.h"
 
 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),
       between_child_spacing_(between_child_spacing),
       main_axis_alignment_(MAIN_AXIS_ALIGNMENT_START),
       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() {
 }
 
 void BoxLayout::SetFlexForView(const View* view, int flex_weight) {
   DCHECK(host_);
   DCHECK(view);
   DCHECK_EQ(host_, view->parent());
   DCHECK_GE(flex_weight, 0);
   flex_map_[view] = flex_weight;
 }
 
 void BoxLayout::ClearFlexForView(const View* view) {
   DCHECK(view);
   flex_map_.erase(view);
 }
 
 void BoxLayout::SetDefaultFlex(int default_flex) {
   DCHECK_GE(default_flex, 0);
   default_flex_ = 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;
   }
 
   if (!num_visible)
     return;
 
   total_main_axis_size -= between_child_spacing_;
   // Free space can be negative indicating that the views want to overflow.
   int main_free_space = MainAxisSize(child_area) - total_main_axis_size;
@@ skipping 20 matching lines @@
     gfx::Rect new_child_area(child_area);
     SetMainAxisPosition(position, &new_child_area);
     SetMainAxisSize(size, &new_child_area);
     child_area.Intersect(new_child_area);
   }
 
   int main_position = MainAxisPosition(child_area);
   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
     // the cross axis size because child Views may calculate their cross axis
     // size based on their main axis size. See https://crbug.com/682266.
 
     // Calculate cross axis size.
     gfx::Rect bounds(child_area);
     SetMainAxisPosition(main_position, &bounds);
     if (cross_axis_alignment_ != CROSS_AXIS_ALIGNMENT_STRETCH) {
       int free_space = CrossAxisSize(bounds) - CrossAxisSizeForView(child);
       int position = CrossAxisPosition(bounds);
       if (cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_CENTER) {
         position += free_space / 2;
       } else if (cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_END) {
         position += free_space;
       }
       SetCrossAxisPosition(position, &bounds);
       SetCrossAxisSize(CrossAxisSizeForView(child), &bounds);
     }
 
     // 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;
       // Use the current remainder to round to the nearest pixel.
       if (std::abs(rem) * 2 >= flex_sum)
         current_padding += main_free_space > 0 ? 1 : -1;
       total_padding += current_padding;
     }
 
     // Set main axis size.
     // TODO(bruthig): Use the allocated width to determine the cross axis size.
     // 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.
   if (flex_sum)
     DCHECK_EQ(total_padding, main_free_space);
 }
 
 gfx::Size BoxLayout::GetPreferredSize(const View* host) const {
   DCHECK_EQ(host_, host);
   // 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_);
   }
 
   return GetPreferredSizeForChildWidth(host, width);
 }
 
 int BoxLayout::GetPreferredHeightForWidth(const View* host, int width) const {
   DCHECK_EQ(host_, host);
   int child_width = width - NonChildSize(host).width();
   return GetPreferredSizeForChildWidth(host, child_width).height();
 }
 
 void BoxLayout::Installed(View* host) {
   DCHECK(!host_);
   host_ = host;
 }
 
 void BoxLayout::ViewRemoved(View* host, View* view) {
   ClearFlexForView(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_;
 
   return it->second;
 }
 
 int BoxLayout::MainAxisSize(const gfx::Rect& rect) const {
   return orientation_ == kHorizontal ? rect.width() : rect.height();
 }
 
@@ skipping 30 matching lines @@
     rect->set_height(size);
 }
 
 void BoxLayout::SetCrossAxisPosition(int position, gfx::Rect* rect) const {
   if (orientation_ == kVertical)
     rect->set_x(position);
   else
     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) {
     // Horizontal layouts ignore |child_area_width|, meaning they mimic the
     // 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;
     }
 
     child_area_bounds.set_width(child_area_width);
     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());
 }
 
 gfx::Size BoxLayout::NonChildSize(const View* host) const {
   gfx::Insets insets(host->GetInsets());
   return gfx::Size(insets.width() + inside_border_insets_.width(),
                    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