[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..7786b1dbabd91c2b749c1b63fa9cb2567a3a9ded 100644
--- a/ash/common/system/tray/tray_details_view.cc
+++ b/ash/common/system/tray/tray_details_view.cc
@@ -16,10 +16,141 @@
 #include "ui/views/controls/scroll_view.h"
 #include "ui/views/controls/separator.h"
 #include "ui/views/layout/box_layout.h"
+#include "ui/views/view_targeter.h"
+#include "ui/views/view_targeter_delegate.h"
 
 namespace ash {
 namespace {
 
+const int kHeaderRowId = -1;

[Evan Stade, 2016/11/02 13:41:27]

Should this be exposed in tray_details_view.h?

[varkha, 2016/11/02 22:42:34]

I've moved it to tray_constants.h instead.

+const int kHeaderRowSeparatorThickness = 1;
+const SkColor kHeaderRowSeparatorColor = SkColorSetA(SK_ColorBLACK, 0x1F);
+
+// 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

[Evan Stade, 2016/11/02 13:41:27]

nit: until the next one "pushes" it up

[varkha, 2016/11/02 22:42:34]

Done.

+// 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 views::ViewTargeterDelegate {
+ public:
+  ScrollContentsView(ash::TrayDetailsView* tray_details_view) {

[Evan Stade, 2016/11/02 13:41:27]

doesn't look like you need the param

[varkha, 2016/11/02 22:42:34]

Yes, I've removed it but merging brought it back. Thanks!

+    SetEventTargeter(base::MakeUnique<views::ViewTargeter>(this));
+  }
+  ~ScrollContentsView() override {}
+
+ protected:
+  // views::View:
+  void OnBoundsChanged(const gfx::Rect& previous_bounds) override {
+    PositionHeaderRows();

[Evan Stade, 2016/11/02 23:12:25]

why not InvalidateLayout()?

[varkha, 2016/11/03 02:32:57]

I thought this would be more expensive. Now I am only effectively repositioning
one view and keeping the rest of the layout.

+  }
+
+  void PaintChildren(const ui::PaintContext& context) override {
+    for (int i = 0; i < child_count(); ++i) {
+      if (child_at(i)->id() != kHeaderRowId && !child_at(i)->layer())
+        child_at(i)->Paint(context);
+    }
+    // Paint header rows above other children in Z-order.
+    for (auto& header : headers_) {
+      if (!header.view->layer())
+        header.view->Paint(context);
+    }
+  }
+
+  void Layout() override {
+    views::View::Layout();
+    headers_.clear();
+    for (int i = 0; i < child_count(); ++i) {
+      views::View* view = child_at(i);
+      if (view->id() == kHeaderRowId)
+        headers_.push_back(Header(view));

[Evan Stade, 2016/11/02 13:41:27]

seems like you could/should keep this list up to date in ViewHierarchyChanged?

[varkha, 2016/11/02 22:42:34]

<Header> list is not just keeping the list of headers but also maintains between
the calls to Layout the "original" offset so that a child row can be painted
sticky or get re-positioned back to its original location. So I could maintain
the membership in the list in hierarchy changes and only update the offset in
Layout() but I am not convinced that this would be simpler or more efficient
given that I would need to use another container or repeatedly call find before
adding to the vector.

+    }
+    PositionHeaderRows();
+  }
+
+  void ViewHierarchyChanged(
+      const ViewHierarchyChangedDetails& details) override {
+    if (!details.is_add && details.parent == this) {
+      auto header_it = std::find_if(headers_.begin(), headers_.end(),
+                                    [details](const Header& header) {
+                                      return header.view == details.child;
+                                    });
+      if (header_it != headers_.end())
+        headers_.erase(header_it);

[Evan Stade, 2016/11/02 13:41:27]

isn't there a std::remove_if?

[varkha, 2016/11/02 22:42:34]

Yes, effectively generalizing this for the case when the elements are not unique
in container. Done.

+    }
+  }
+
+  View* TargetForRect(View* root, const gfx::Rect& rect) override {
+    // Give header rows first dibs on events.
+    for (auto& header : headers_) {
+      views::View* view = header.view;
+      gfx::Rect local_to_header = rect;
+      local_to_header.Offset(-view->x(), -view->y());
+      if (ViewTargeterDelegate::DoesIntersectRect(view, local_to_header))
+        return ViewTargeterDelegate::TargetForRect(view, local_to_header);
+    }
+    return ViewTargeterDelegate::TargetForRect(root, rect);
+  }
+
+ private:
+  class Header {

[Evan Stade, 2016/11/02 13:41:27]

struct? not sure what this is buying us either way at this point. Can we just
call View::y() as needed?

[varkha, 2016/11/02 22:42:34]

I need this to have an offset saved in Layout and still available when
OnBoundsChanged is called some time later. This is what Header struct is doing. 

[Evan Stade, 2016/11/02 23:12:25]

this is not at all obvious from the code. This warrants better docs and better
naming (of both the class and the offset variable).

[varkha, 2016/11/03 02:32:57]

Done. I've also moved a bit more into this so this I think is now a legitimate
class.

+   public:
+    Header(views::View* header) : view(header), offset(header->bounds().y()) {}

[Evan Stade, 2016/11/02 13:41:27]

you can just do header->y()

[varkha, 2016/11/02 22:42:34]

Done.

+
+    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) {

[Evan Stade, 2016/11/02 13:41:27]

nit: s/ShowHeaderSticky/DecorateAsSticky/

or SetStickyAppearance
or SetLooksSticky

or something. "Show" sounds a lot like you're making something visible (like
Widget::Show).

[varkha, 2016/11/02 22:42:34]

I like your first suggestion the most. Done.

+    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.
+  void PositionHeaderRows() {
+    const int scroll_offset = -bounds().y();
+    Header* previous_header = nullptr;
+    for (auto& header : headers_) {

[Evan Stade, 2016/11/02 23:12:25]

I feel like this loop could be simpler if you iterated backwards, so instead of
making a bunch of headers sticky then undoing, you only reposition at most one
header. This also (I believe) obviates the need for the Header struct.

[varkha, 2016/11/03 02:32:57]

Done (although I am still not sure how you would eliminate the need to keep
track of the original offset (unless you force Layout() to happen on every
scroll increment which I tried to avoid).

+      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();

[Evan Stade, 2016/11/02 23:12:25]

I don't see the purpose in reusing header_bounds here.

[varkha, 2016/11/03 02:32:57]

Done.

+          header_bounds.set_y(previous_header->offset);
+          previous_header->view->SetBoundsRect(header_bounds);

[Evan Stade, 2016/11/02 23:12:25]

this stanza looks equivalent to

  previous_header->view->SetY(previous_header->offset)

[varkha, 2016/11/03 02:32:57]

Done. Here and elsewhere.

+          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);
+};
+
 // Constants for the title row in material design.
 const int kTitleRowVerticalPadding = 4;
 const int kTitleRowSeparatorBorderHeight = 1;
@@ -65,8 +196,6 @@ class TitleRowSeparatorLayout : public views::LayoutManager {
   }
 };
 
-}  // namespace
-
 class ScrollSeparator : public views::View {
  public:
   ScrollSeparator() {}
@@ -74,17 +203,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);

[Evan Stade, 2016/11/02 13:41:27]

aren't we already in ash

[varkha, 2016/11/02 22:42:34]

Done.

   }
   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 +225,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 +237,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,7 +313,7 @@ void TrayDetailsView::CreateTitleRow(int string_id) {
 
 void TrayDetailsView::CreateScrollableList() {
   DCHECK(!scroller_);
-  scroll_content_ = new views::View;
+  scroll_content_ = new ScrollContentsView(this);
   scroll_content_->SetLayoutManager(

[Evan Stade, 2016/11/02 13:41:27]

perhaps this belongs in ScrollContentsView's ctor now?

[varkha, 2016/11/02 22:42:34]

Done.

       new views::BoxLayout(views::BoxLayout::kVertical, 0, 0, 1));
   scroller_ = new FixedSizedScrollView;