Clean up layout for holding space bubble

Remove GridLayout manager for the holding_space_item_chips_container
in favor of a simple custom layout. The GridLayout manager seems to be
unwieldy when trying to remove items.

Clean up hard-coded padding values.

Bug: 1129321
Change-Id: I065c4b5c8acc9822f6a6385660a50a0a77ee98a1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2415280
Commit-Queue: Ahmed Mehfooz <amehfooz@chromium.org>
Reviewed-by: David Black <dmblack@google.com>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809072}

ash/public/cpp/holding_space/holding_space_constants.h

@@ -22,15 +22,21 @@ enum HoldingSpaceCommandId {
 
 // Appearance.
 constexpr gfx::Insets kHoldingSpaceContainerPadding(16);
-constexpr int kHoldingSpaceContainerSeparation = 8;
+constexpr int kHoldingSpaceContainerChildSpacing = 16;
+constexpr int kHoldingSpaceContainerSpacing = 8;
 constexpr gfx::Insets kHoldingSpaceChipPadding(8);
 constexpr int kHoldingSpaceChipChildSpacing = 8;
 constexpr int kHoldingSpaceChipCornerRadius = 8;
+constexpr int kHoldingSpaceChipHeight = 40;
 constexpr int kHoldingSpaceChipIconSize = 24;
-constexpr int kHoldingSpaceColumnPadding = 8;
+constexpr int kHoldingSpaceChipWidth = 160;
+constexpr int kHoldingSpaceChipsPerRow = 2;
+constexpr int kHoldingSpaceColumnSpacing = 8;
 constexpr int kHoldingSpaceColumnWidth = 160;
-constexpr int kHoldingSpaceRowPadding = 8;
+constexpr int kHoldingSpaceRowSpacing = 8;
+constexpr int kHoldingSpaceScreenshotSpacing = 8;
 constexpr gfx::Size kHoldingSpaceScreenshotSize(104, 80);
+constexpr gfx::Insets kHoldingSpaceScreenshotsContainerPadding(8, 0);
 
 // View IDs.
 constexpr int kHoldingSpacePinnedFilesContainerId = 1;

ash/system/holding_space/holding_space_item_chip_view.cc

@@ -37,6 +37,8 @@ HoldingSpaceItemChipView::HoldingSpaceItemChipView(const HoldingSpaceItem* item)
       views::BoxLayout::Orientation::kHorizontal,
       gfx::Insets(kHoldingSpaceChipPadding), kHoldingSpaceChipChildSpacing));
 
+  SetPreferredSize(gfx::Size(kHoldingSpaceChipWidth, kHoldingSpaceChipHeight));
+
   image_ =
       AddChildView(std::make_unique<tray::RoundedImageView>(kTrayItemSize / 2));
 

ash/system/holding_space/holding_space_item_chips_container.cc

@@ -7,22 +7,108 @@
 #include "ash/public/cpp/holding_space/holding_space_constants.h"
 #include "ash/public/cpp/holding_space/holding_space_item.h"
 #include "ash/system/holding_space/holding_space_item_chip_view.h"
-#include "ash/system/tray/tray_constants.h"
-#include "ui/views/layout/grid_layout.h"
+#include "base/debug/stack_trace.h"
+#include "ui/views/layout/layout_manager_base.h"
+#include "ui/views/layout/proposed_layout.h"
 
 namespace ash {
 
+namespace {
+
+// Need a custom grid layout to facilitate removal of views from the grid,
+// which can change the number of rows required. views::GridLayout makes this
+// case difficult.
+class SimpleGridLayout : public views::LayoutManagerBase {
+ public:
+  SimpleGridLayout(int column_count, int column_spacing, int row_spacing)
+      : column_count_(column_count),
+        column_spacing_(column_spacing),
+        row_spacing_(row_spacing) {}
+
+  gfx::Size GetChildPreferredSize() const {
+    if (cached_child_preferred_size_)
+      return *cached_child_preferred_size_;
+
+    if (!host_view()->children().size())
+      return gfx::Size();
+
+    cached_child_preferred_size_ =
+        host_view()->children()[0]->GetPreferredSize();
+    return *cached_child_preferred_size_;
+  }
+
+  gfx::Size CalculatePreferredSize() const {
+    int total_children = 0;
+    for (auto* child : host_view()->children()) {
+      if (IsChildIncludedInLayout(child))
+        total_children++;
+    }
+    // Equivalent to `ceil(children().size() / column_count_)`.
+    int number_of_rows = (total_children + column_count_ - 1) / column_count_;
+
+    if (!number_of_rows)
+      return gfx::Size();
+
+    // SimpleGridLayout assumes all children have identical sizes.
+    int child_height = GetChildPreferredSize().height();
+    int child_width = GetChildPreferredSize().width();
+
+    int height =
+        (number_of_rows * (row_spacing_ + child_height)) - row_spacing_;
+    int width =
+        (column_count_ * (child_width + column_spacing_)) - column_spacing_;
+
+    return gfx::Size(width, height);
+  }
+
+  views::ProposedLayout CalculateProposedLayout(
+      const views::SizeBounds& size_bounds) const override {
+    views::ProposedLayout proposed_layout;
+
+    if (size_bounds.is_fully_bounded()) {
+      proposed_layout.host_size =
+          gfx::Size(size_bounds.width().value(), size_bounds.height().value());
+    } else {
+      proposed_layout.host_size = CalculatePreferredSize();
+    }
+
+    gfx::Size size = GetChildPreferredSize();
+    int row = 0;
+    int col = 0;
+    for (auto* child : host_view()->children()) {
+      if (!IsChildIncludedInLayout(child))
+        continue;
+
+      int x = (col * (column_spacing_ + size.width()));
+      int y = (row * (row_spacing_ + size.height()));
+      proposed_layout.child_layouts.push_back(
+          views::ChildLayout{child,
+                             child->GetVisible(),
+                             gfx::Rect(x, y, size.width(), size.height()),
+                             {size.width(), size.height()}});
+
+      ++col;
+      if (col % column_count_ == 0) {
+        ++row;
+        col = 0;
+      }
+    }
+    return proposed_layout;
+  }
+
+ private:
+  mutable base::Optional<gfx::Size> cached_child_preferred_size_;
+
+  const int column_count_;
+  const int column_spacing_;
+  const int row_spacing_;
+};
+}  // namespace
+
 HoldingSpaceItemChipsContainer::HoldingSpaceItemChipsContainer() {
-  layout_ = SetLayoutManager(std::make_unique<views::GridLayout>());
-  column_set_ = layout_->AddColumnSet(0);
-
-  column_set_->AddColumn(
-      views::GridLayout::Alignment::FILL, views::GridLayout::Alignment::FILL, 0,
-      views::GridLayout::ColumnSize::kFixed, kHoldingSpaceColumnWidth, 0);
-  column_set_->AddPaddingColumn(0, kHoldingSpaceColumnPadding);
-  column_set_->AddColumn(
-      views::GridLayout::Alignment::FILL, views::GridLayout::Alignment::FILL, 0,
-      views::GridLayout::ColumnSize::kFixed, kHoldingSpaceColumnWidth, 0);
+  SetLayoutManager(std::make_unique<SimpleGridLayout>(
+      kHoldingSpaceChipsPerRow, kHoldingSpaceColumnSpacing,
+      kHoldingSpaceRowSpacing));
 }
 
 HoldingSpaceItemChipsContainer::~HoldingSpaceItemChipsContainer() = default;
@@ -31,10 +117,4 @@ const char* HoldingSpaceItemChipsContainer::GetClassName() const {
   return "HoldingSpaceItemChipsContainer";
 }
 
-void HoldingSpaceItemChipsContainer::AddItemChip(const HoldingSpaceItem* item) {
-  if ((children().size() % 2) == 0)
-    layout_->StartRowWithPadding(0, 0, 0, kHoldingSpaceRowPadding);
-  layout_->AddView(std::make_unique<HoldingSpaceItemChipView>(item));
-}
-
 }  // namespace ash
\ No newline at end of file

ash/system/holding_space/holding_space_item_chips_container.h

@@ -8,15 +8,8 @@
 #include "ash/ash_export.h"
 #include "ui/views/view.h"
 
-namespace views {
-class ColumnSet;
-class GridLayout;
-}  // namespace views
-
 namespace ash {
 
-class HoldingSpaceItem;
-
 // A container view which automatically arranges item chips into a 2 column
 // grid.
 class HoldingSpaceItemChipsContainer : public views::View {
@@ -30,12 +23,6 @@ class HoldingSpaceItemChipsContainer : public views::View {
 
   // views::View:
   const char* GetClassName() const override;
-
-  void AddItemChip(const HoldingSpaceItem* item);
-
- private:
-  views::GridLayout* layout_ = nullptr;
-  views::ColumnSet* column_set_ = nullptr;
 };
 
 }  // namespace ash

ash/system/holding_space/holding_space_tray.cc

@@ -157,7 +157,7 @@ void HoldingSpaceTray::ShowBubble(bool show_by_click) {
   auto* separator =
       bubble_view->AddChildView(std::make_unique<views::Separator>());
   separator->SetBorder(views::CreateEmptyBorder(
-      gfx::Insets(kHoldingSpaceContainerSeparation, 0, 0, 0)));
+      gfx::Insets(kHoldingSpaceContainerSpacing, 0, 0, 0)));
 
   recent_files_container_ =
       bubble_view->AddChildView(std::make_unique<RecentFilesContainer>());

ash/system/holding_space/pinned_files_container.cc

@@ -26,7 +26,8 @@ PinnedFilesContainer::PinnedFilesContainer() {
   SetID(kHoldingSpacePinnedFilesContainerId);
 
   SetLayoutManager(std::make_unique<views::BoxLayout>(
-      views::BoxLayout::Orientation::kVertical, kHoldingSpaceContainerPadding));
+      views::BoxLayout::Orientation::kVertical, kHoldingSpaceContainerPadding,
+      kHoldingSpaceContainerChildSpacing));
 
   auto* title_label = AddChildView(std::make_unique<views::Label>(
       l10n_util::GetStringUTF16(IDS_ASH_HOLDING_SPACE_PINNED_TITLE)));
@@ -42,8 +43,10 @@ PinnedFilesContainer::PinnedFilesContainer() {
   // TODO(crbug.com/1125254): Populate containers if and when holding space
   // model is attached, below is a temporary solution.
   for (const auto& item : HoldingSpaceController::Get()->model()->items()) {
-    if (item->type() == HoldingSpaceItem::Type::kPinnedFile)
-      item_chips_container_->AddItemChip(item.get());
+    if (item->type() == HoldingSpaceItem::Type::kPinnedFile) {
+      item_chips_container_->AddChildView(
+          std::make_unique<HoldingSpaceItemChipView>(item.get()));
+    }
   }
 }
 

ash/system/holding_space/recent_files_container.cc

@@ -9,6 +9,7 @@
 #include "ash/public/cpp/holding_space/holding_space_item.h"
 #include "ash/public/cpp/holding_space/holding_space_model.h"
 #include "ash/strings/grit/ash_strings.h"
+#include "ash/system/holding_space/holding_space_item_chip_view.h"
 #include "ash/system/holding_space/holding_space_item_chips_container.h"
 #include "ash/system/holding_space/holding_space_item_screenshot_view.h"
 #include "ash/system/tray/tray_constants.h"
@@ -25,7 +26,9 @@ RecentFilesContainer::RecentFilesContainer() {
   SetID(kHoldingSpaceRecentFilesContainerId);
 
   SetLayoutManager(std::make_unique<views::BoxLayout>(
-      views::BoxLayout::Orientation::kVertical, kHoldingSpaceContainerPadding));
+      views::BoxLayout::Orientation::kVertical, kHoldingSpaceContainerPadding,
+      kHoldingSpaceContainerChildSpacing));
+
   auto setup_layered_child = [](views::View* child) {
     child->SetPaintToLayer();
     child->layer()->SetFillsBoundsOpaquely(false);
@@ -40,8 +43,9 @@ RecentFilesContainer::RecentFilesContainer() {
 
   screenshots_container_ = AddChildView(std::make_unique<views::View>());
   screenshots_container_->SetLayoutManager(std::make_unique<views::BoxLayout>(
-      views::BoxLayout::Orientation::kHorizontal, gfx::Insets(16, 0, 24, 0),
-      8));
+      views::BoxLayout::Orientation::kHorizontal,
+      kHoldingSpaceScreenshotsContainerPadding,
+      kHoldingSpaceScreenshotSpacing));
 
   // TODO(crbug.com/1125254): Populate containers if and when holding space
   // model is attached, below is a temporary solution.
@@ -63,8 +67,10 @@ RecentFilesContainer::RecentFilesContainer() {
   // TODO(crbug.com/1125254): Populate containers if and when holding space
   // model is attached, below is a temporary solution.
   for (const auto& item : HoldingSpaceController::Get()->model()->items()) {
-    if (item->type() == HoldingSpaceItem::Type::kDownload)
-      recent_downloads_container_->AddItemChip(item.get());
+    if (item->type() == HoldingSpaceItem::Type::kDownload) {
+      recent_downloads_container_->AddChildView(
+          std::make_unique<HoldingSpaceItemChipView>(item.get()));
+    }
   }
 }