[ash-md] Makes Wi-Fi header row sticky when network list is scrolled

ash/common/system/tray/tray_details_view.cc

Index: ash/common/system/tray/tray_details_view.cc
diff --git a/ash/common/system/tray/tray_details_view.cc b/ash/common/system/tray/tray_details_view.cc
index 70ff14fd6b4feb2838251819af30b34515ef2cf2..83d6e63d4a41cc119662c4725994ff418296e286 100644
--- a/ash/common/system/tray/tray_details_view.cc
+++ b/ash/common/system/tray/tray_details_view.cc
@@ -32,6 +32,10 @@ const int kTitleRowPaddingBottom =
     kTitleRowVerticalPadding - kTitleRowSeparatorHeight;
 const SkColor kTitleRowSeparatorBorderColor = SkColorSetRGB(0xe0, 0xe0, 0xe0);
 
+const int kHeaderRowId = 1000;

[tdanderson, 2016/10/28 19:42:41]

Would it be safer to use -1 for |kHeaderRowId| ?

[varkha, 2016/11/02 02:03:20]

Done.

+const int kHeaderRowSeparatorThickness = 1;
+const SkColor kHeaderRowSeparatorColor = SkColorSetA(SK_ColorBLACK, 0x1F);
+
 // Special layout to overlap the separator for title row and the progress bar
 // which is displayed on it.
 // Expected children are a views::Separator and, optionally, a
@@ -65,7 +69,184 @@ class TitleRowSeparatorLayout : public views::LayoutManager {
   }
 };
 
-}  // namespace
+// Variation of a BoxLayout that keeps header rows at the end of children_ in
+// order to stack them above the other rows while maintaining the order (group
+// headers first, the rest of the group rows in their original order).
+// To use this layout tag header rows with set_id(kHeaderRowId) and tag groups
+// including the header row with SetGroup(group_id).
+class HeaderListLayout : public views::BoxLayout {
+ public:
+  HeaderListLayout() : views::BoxLayout(views::BoxLayout::kVertical, 0, 0, 1) {}
+  ~HeaderListLayout() override {}
+
+ protected:
+  // views::BoxLayout:
+  void LayoutChildren(views::View* host,
+                      const gfx::Rect& child_area,
+                      int main_free_space,
+                      int flex_sum,
+                      int main_position,
+                      int current_flex,
+                      int* total_padding) override {
+    for (int i = 0, count = host->child_count(); i < count; ++i) {

[tdanderson, 2016/10/28 19:42:41]

I would find LayoutChildren() to be much more readable if you included
documentation above each loop (i.e., lines 91 and 105) as you have already done
for lines 100 and 123.

[varkha, 2016/11/02 02:03:20]

Acknowledged.

+      views::View* child = host->child_at(i);
+      if (child->id() == kHeaderRowId) {

[Evan Stade, 2016/10/28 17:15:40]

I think we discussed an alternative where you would add the headers via a
method, e.g.

void HeaderListLayout::AddViewAsHeader(views::View*)

that would remove the need for this step (also, the way you're currently using
IDs is technically in contradiction of the documentation, "ID should be unique
within the subtree", although it probably doesn't actually matter much).

[varkha, 2016/11/01 00:51:06]

I've tried to add setting header rows explicitly. A few rough edges there but it
is possible. Before I go with cleaning this up, can you take a look and see if
that is what you had in mind? It is a bit more coupling as the child row views
will need to know about the scroll contents view and its type but avoids using
id().

+        auto header = std::find(headers_.begin(), headers_.end(), child);
+        if (header == headers_.end())
+          headers_.push_back(Header(child));
+      }
+    }
+    // Ensure that the header rows are stacked at the end of |host|'s children.
+    for (const auto& header : headers_)
+      host->ReorderChildView(header.view, -1);
+
+    auto headers(headers_);
+    int group_id = -1;
+    for (int i = 0, count = host->child_count(); i < count; ++i) {
+      views::View* child = host->child_at(i);
+      if (!child->visible() || child->id() == kHeaderRowId)
+        continue;
+      if (child->GetGroup() != group_id) {
+        // Find a header for the new group and place it above the group.
+        group_id = child->GetGroup();
+        auto header = std::find(headers.begin(), headers.end(), group_id);
+        if (header != headers.end()) {
+          LayoutChild(header->view, child_area, main_free_space, flex_sum,
+                      &main_position, &current_flex, total_padding);
+          headers.erase(header);
+        }
+      }
+      LayoutChild(child, child_area, main_free_space, flex_sum, &main_position,
+                  &current_flex, total_padding);
+    }
+    // In the end layout unclaimed headers.

[tdanderson, 2016/10/28 19:42:41]

nit on wording: just "Layout unclaimed headers" or  "Layout remaining headers".

[varkha, 2016/11/02 02:03:20]

Acknowledged.

+    for (const auto& header : headers) {
+      LayoutChild(header.view, child_area, main_free_space, flex_sum,
+                  &main_position, &current_flex, total_padding);
+    }
+  }
+
+  void ViewRemoved(views::View* host, views::View* view) override {
+    views::BoxLayout::ViewRemoved(host, view);
+    auto header = std::find(headers_.begin(), headers_.end(), view);
+    if (header != headers_.end())
+      headers_.erase(header);
+  }
+
+ private:
+  class Header {

[Evan Stade, 2016/10/28 17:15:40]

somewhat confusing that you have to classes both named Header

[varkha, 2016/11/02 02:03:20]

Acknowledged.

+   public:
+    explicit Header(views::View* header) : view(header) {}
+    bool operator==(views::View* other) { return view == other; }
+    bool operator==(int group_id) { return view->GetGroup() == group_id; }
+
+    views::View* view;
+  };
+
+  // Header child views that stick to the top of visible viewport when scrolled.
+  std::vector<Header> headers_;
+
+  DISALLOW_COPY_AND_ASSIGN(HeaderListLayout);
+};
+
+// A view that is used as ScrollView contents. It supports designating some of
+// the children as sticky header rows. The sticky header rows are not scrolled
+// above the top of the visible viewport and are painted above other children.
+// To indicate that a child is a sticky header row use set_id(kHeaderRowId).
+class ScrollContentsView : public views::View {
+ public:
+  ScrollContentsView() {}
+  ~ScrollContentsView() override {}
+
+ protected:
+  // views::View.

[tdanderson, 2016/10/28 19:42:41]

nit: views::View:

[varkha, 2016/11/02 02:03:21]

Done.

+  void Layout() override {
+    views::View::Layout();
+    headers_.clear();
+    for (int i = 0, count = child_count(); i < count; ++i) {

[tdanderson, 2016/10/28 19:42:41]

just (int i = 0; i < child_count(); ++i) ? here and elsewhere in this class

[varkha, 2016/11/02 02:03:20]

Done.

+      views::View* header = child_at(i);

[tdanderson, 2016/10/28 19:42:41]

nit: consider naming as |view| instead since you haven't yet determined if it's
a header.

[varkha, 2016/11/02 02:03:21]

Done.

+      if (header->id() == kHeaderRowId)
+        headers_.push_back(Header(header));
+    }
+    ScrollChildren();
+  }
+
+  void OnBoundsChanged(const gfx::Rect& previous_bounds) override {
+    ScrollChildren();
+  }
+
+  void ViewHierarchyChanged(
+      const ViewHierarchyChangedDetails& details) override {
+    if (!details.is_add && details.parent == this) {
+      auto header = std::find(headers_.begin(), headers_.end(), details.child);
+      if (header != headers_.end())
+        headers_.erase(header);

[tdanderson, 2016/10/28 19:42:41]

Shouldn't you call ScrollChildren() after erasing |header|?

[varkha, 2016/11/02 02:03:20]

I thought you would get a Layout() call when this happens and you still have any
children left. When there are no children left we should be fine as well.

+    }
+  }
+
+  const char* GetClassName() const override { return "ScrollContentsView"; }
+
+ private:
+  class Header {
+   public:
+    explicit Header(views::View* header)
+        : view(header), offset(header->bounds().y()) {}
+    bool operator==(views::View* other) { return view == other; }
+
+    views::View* view;
+    int offset;
+  };
+
+  // Sets decorations on a header row to indicate whether it is sticky.
+  static void ShowHeaderSticky(views::View* header, bool show_sticky) {
+    if (show_sticky) {
+      header->SetBorder(views::Border::CreateSolidSidedBorder(
+          0, 0, kHeaderRowSeparatorThickness, 0, kHeaderRowSeparatorColor));
+    } else {
+      header->SetBorder(views::Border::CreateSolidSidedBorder(
+          kHeaderRowSeparatorThickness, 0, 0, 0, kHeaderRowSeparatorColor));
+    }
+  }
+
+  // Adjusts y-position of header rows allowing one or two rows to stick to the
+  // top of the visible viewport.

[Evan Stade, 2016/10/28 17:15:40]

do you have a use case for multiple sticky headers yet?

[varkha, 2016/11/02 02:03:20]

Yes, Cellular networks will have same UI with a sticky header row.

+  void ScrollChildren() {

[tdanderson, 2016/10/28 19:42:41]

Consider a more descriptive name for this, such as PositionHeaderRows()?

[varkha, 2016/11/02 02:03:20]

Done.

+    const int scroll_offset = -bounds().y();
+    Header* previous_header = nullptr;
+    for (auto& header : headers_) {
+      gfx::Rect header_bounds = header.view->bounds();
+      if (scroll_offset > header.offset) {
+        header_bounds.set_y(scroll_offset);
+        header.view->SetBoundsRect(header_bounds);
+        ShowHeaderSticky(header.view, true);
+        header.view->Layout();
+        header.view->SchedulePaint();
+        if (previous_header) {
+          header_bounds = previous_header->view->bounds();
+          header_bounds.set_y(previous_header->offset);
+          previous_header->view->SetBoundsRect(header_bounds);
+          ShowHeaderSticky(previous_header->view, false);
+        }
+        previous_header = &header;
+      } else if (previous_header &&
+                 header_bounds.y() < previous_header->view->bounds().bottom()) {
+        gfx::Rect previous_header_bounds = previous_header->view->bounds();
+        previous_header_bounds.set_y(header_bounds.y() -
+                                     previous_header->view->bounds().height());
+        previous_header->view->SetBoundsRect(previous_header_bounds);
+        ShowHeaderSticky(previous_header->view, false);
+        ShowHeaderSticky(header.view, false);
+      } else {
+        ShowHeaderSticky(header.view, false);
+      }
+    }
+  }
+
+  // Header child views that stick to the top of visible viewport when scrolled.
+  std::vector<Header> headers_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScrollContentsView);
+};
 
 class ScrollSeparator : public views::View {
  public:
@@ -74,17 +255,20 @@ class ScrollSeparator : public views::View {
   ~ScrollSeparator() override {}
 
  private:
-  // Overriden from views::View.
+  // views::View.
   void OnPaint(gfx::Canvas* canvas) override {
-    canvas->FillRect(gfx::Rect(0, height() / 2, width(), 1), kBorderLightColor);
+    canvas->FillRect(gfx::Rect(0, height() / 2, width(), 1),
+                     ash::kBorderLightColor);
   }
   gfx::Size GetPreferredSize() const override {
-    return gfx::Size(1, kTrayPopupScrollSeparatorHeight);
+    return gfx::Size(1, ash::kTrayPopupScrollSeparatorHeight);
   }
 
   DISALLOW_COPY_AND_ASSIGN(ScrollSeparator);
 };
 
+}  // namespace
+
 class ScrollBorder : public views::Border {
  public:
   ScrollBorder() {}
@@ -93,7 +277,7 @@ class ScrollBorder : public views::Border {
   void set_visible(bool visible) { visible_ = visible; }
 
  private:
-  // Overridden from views::Border.
+  // views::Border.
   void Paint(const views::View& view, gfx::Canvas* canvas) override {
     if (!visible_)
       return;
@@ -105,7 +289,7 @@ class ScrollBorder : public views::Border {
 
   gfx::Size GetMinimumSize() const override { return gfx::Size(0, 1); }
 
-  bool visible_;
+  bool visible_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(ScrollBorder);
 };
@@ -181,9 +365,8 @@ void TrayDetailsView::CreateTitleRow(int string_id) {
 
 void TrayDetailsView::CreateScrollableList() {
   DCHECK(!scroller_);
-  scroll_content_ = new views::View;
-  scroll_content_->SetLayoutManager(
-      new views::BoxLayout(views::BoxLayout::kVertical, 0, 0, 1));
+  scroll_content_ = new ScrollContentsView();
+  scroll_content_->SetLayoutManager(new HeaderListLayout());
   scroller_ = new FixedSizedScrollView;
   scroller_->SetContentsView(scroll_content_);