MacViews: support backgrounds for selected rows

ui/views/controls/tree/tree_view.cc

Index: ui/views/controls/tree/tree_view.cc
diff --git a/ui/views/controls/tree/tree_view.cc b/ui/views/controls/tree/tree_view.cc
index 70501f872adda1fe37dfcd84d55e308b35aa6071..70ab5be09b526659370f69e6ff9b7fd469835275 100644
--- a/ui/views/controls/tree/tree_view.cc
+++ b/ui/views/controls/tree/tree_view.cc
@@ -25,6 +25,7 @@
 #include "ui/views/controls/textfield/textfield.h"
 #include "ui/views/controls/tree/tree_view_controller.h"
 #include "ui/views/resources/grit/views_resources.h"
+#include "ui/views/style/platform_style.h"
 
 using ui::TreeModel;
 using ui::TreeModelNode;
@@ -68,6 +69,15 @@ ui::NativeTheme::ColorId text_color_id(bool has_focus, bool is_selected) {
   return ui::NativeTheme::kColorId_TreeText;
 }
 
+bool EventIsDoubleTapOrClick(const ui::LocatedEvent& event) {
+  if (event.type() == ui::ET_GESTURE_TAP) {
+    const auto& gesture = static_cast<const ui::GestureEvent&>(event);
+    return gesture.details().tap_count() == 2;

[tapted, 2016/06/13 07:03:47]

I think we can skip the `auto` and do

    return event.AsGestureEvent().details().tap_count() == 2;

[Elly Fong-Jones, 2016/06/14 17:21:27]

Done.

+  } else {

[tapted, 2016/06/13 07:03:48]

nit: "no else after return" - last item at
https://www.chromium.org/developers/coding-style#TOC-Code-formatting

[Elly Fong-Jones, 2016/06/14 17:21:27]

Done.

+    return event.flags() & ui::EF_IS_DOUBLE_CLICK;
+  }
+}
+
 }  // namespace
 
 TreeView::TreeView()
@@ -392,13 +402,7 @@ void TreeView::ShowContextMenu(const gfx::Point& p,
     // ContextMenuController) if over a node.
     gfx::Point local_point(p);
     ConvertPointFromScreen(this, &local_point);
-    int row = (local_point.y() - kVerticalInset) / row_height_;
-    int depth = 0;
-    InternalNode* node = GetNodeByRow(row, &depth);
-    if (!node)
-      return;
-    gfx::Rect bounds(GetBoundsForNodeImpl(node, row, depth));
-    if (!bounds.Contains(local_point))
+    if (!NodeAtPoint(local_point))

[tapted, 2016/06/13 07:03:47]

I think it would improve NodeAtPoint if it required a non-null depth - then we
just end up with an extra line here like `int ignored_depth;` rather than the
extra stuff in NodeAtPoint

[Elly Fong-Jones, 2016/06/14 17:21:27]

Done.

       return;
   }
   View::ShowContextMenu(p, source_type);
@@ -640,39 +644,20 @@ bool TreeView::OnClickOrTap(const ui::LocatedEvent& event) {
   CommitEdit();
   RequestFocus();
 
-  int row = (event.y() - kVerticalInset) / row_height_;
-  int depth = 0;
-  InternalNode* node = GetNodeByRow(row, &depth);
-  if (node) {
-    gfx::Rect bounds(GetBoundsForNodeImpl(node, row, depth));
-    if (bounds.Contains(event.location())) {
-      int relative_x = event.x() - bounds.x();
-      if (base::i18n::IsRTL())
-        relative_x = bounds.width() - relative_x;
-      if (relative_x < kArrowRegionSize &&
-          model_->GetChildCount(node->model_node())) {
-        if (node->is_expanded())
-          Collapse(node->model_node());
-        else
-          Expand(node->model_node());
-      } else if (relative_x > kArrowRegionSize) {
-        SetSelectedNode(node->model_node());
-        bool should_toggle = false;
-        if (event.type() == ui::ET_GESTURE_TAP) {
-          const ui::GestureEvent& gesture =
-              static_cast<const ui::GestureEvent&>(event);
-          should_toggle = gesture.details().tap_count() == 2;
-        } else {
-          should_toggle = (event.flags() & ui::EF_IS_DOUBLE_CLICK) != 0;
-        }
-        if (should_toggle) {
-          if (node->is_expanded())
-            Collapse(node->model_node());
-          else
-            Expand(node->model_node());
-        }
-      }
-    }
+  int depth;
+  InternalNode* node = NodeAtPoint(event.location(), &depth);
+  if (!node)
+    return true;
+
+  bool hits_arrow = HitsNodeArrow(node, depth, event.location());
+  if (!hits_arrow)
+    SetSelectedNode(node->model_node());
+
+  if (hits_arrow || EventIsDoubleTapOrClick(event)) {
+    if (node->is_expanded())
+      Collapse(node->model_node());
+    else
+      Expand(node->model_node());
   }
   return true;
 }
@@ -742,7 +727,7 @@ void TreeView::LayoutEditor() {
 void TreeView::SchedulePaintForNode(InternalNode* node) {
   if (!node)
     return;  // Explicitly allow NULL to be passed in.
-  SchedulePaintInRect(GetBoundsForNode(node));
+  SchedulePaintInRect(GetBackgroundBoundsForNode(node));
 }
 
 void TreeView::PaintRows(gfx::Canvas* canvas,
@@ -769,6 +754,12 @@ void TreeView::PaintRow(gfx::Canvas* canvas,
                         int row,
                         int depth) {
   gfx::Rect bounds(GetBoundsForNodeImpl(node, row, depth));
+  const SkColor selected_row_bg_color = GetNativeTheme()->GetSystemColor(
+      text_background_color_id(HasFocus() || editing_));
+
+  // Paint the row background.
+  if (PlatformStyle::kTreeViewSelectsEntireRow && selected_node_ == node)
+    canvas->FillRect(GetBackgroundBoundsForNode(node), selected_row_bg_color);
 
   if (model_->GetChildCount(node->model_node()))
     PaintExpandControl(canvas, bounds, node->is_expanded());

[tapted, 2016/06/13 07:03:47]

We probably need to convert this to a vector icon so we can control the color
it's painted, but that's good for a follow-up.

[Elly Fong-Jones, 2016/06/14 17:21:27]

Acknowledged.

@@ -792,29 +783,34 @@ void TreeView::PaintRow(gfx::Canvas* canvas,
       icon, icon_x,
       bounds.y() + (bounds.height() - icon.height()) / 2);
 
-  if (!editing_ || node != selected_node_) {
-    gfx::Rect text_bounds(bounds.x() + text_offset_, bounds.y(),
-                          bounds.width() - text_offset_, bounds.height());
-    if (base::i18n::IsRTL())
-      text_bounds.set_x(bounds.x());
-    if (node == selected_node_) {
-      const SkColor bg_color = GetNativeTheme()->GetSystemColor(
-          text_background_color_id(HasFocus()));
-      canvas->FillRect(text_bounds, bg_color);
-      if (HasFocus())
-        canvas->DrawFocusRect(text_bounds);
-    }
-    const ui::NativeTheme::ColorId color_id =
-        text_color_id(HasFocus(), node == selected_node_);
-    const gfx::Rect internal_bounds(
-        text_bounds.x() + kTextHorizontalPadding,
-        text_bounds.y() + kTextVerticalPadding,
-        text_bounds.width() - kTextHorizontalPadding * 2,
-        text_bounds.height() - kTextVerticalPadding * 2);
-    canvas->DrawStringRect(node->model_node()->GetTitle(), font_list_,
-                           GetNativeTheme()->GetSystemColor(color_id),
-                           internal_bounds);
+  // Paint the text background and text. In edit mode, the selected node is a
+  // separate editing control, so it does not need to be painted here.
+  if (editing_ && selected_node_ == node)
+    return;
+
+  gfx::Rect text_bounds(bounds.x() + text_offset_, bounds.y(),
+                        bounds.width() - text_offset_, bounds.height());
+  if (base::i18n::IsRTL())
+    text_bounds.set_x(bounds.x());
+
+  // Paint the background on the selected row.
+  if (!PlatformStyle::kTreeViewSelectsEntireRow && node == selected_node_) {
+    canvas->FillRect(text_bounds, selected_row_bg_color);
+    if (HasFocus())
+      canvas->DrawFocusRect(text_bounds);
   }
+
+  // Paint the text.
+  const ui::NativeTheme::ColorId color_id =
+      text_color_id(HasFocus(), node == selected_node_);
+  const gfx::Rect internal_bounds(
+      text_bounds.x() + kTextHorizontalPadding,
+      text_bounds.y() + kTextVerticalPadding,
+      text_bounds.width() - kTextHorizontalPadding * 2,
+      text_bounds.height() - kTextVerticalPadding * 2);
+  canvas->DrawStringRect(node->model_node()->GetTitle(), font_list_,
+                         GetNativeTheme()->GetSystemColor(color_id),
+                         internal_bounds);
 }
 
 void TreeView::PaintExpandControl(gfx::Canvas* canvas,
@@ -865,6 +861,15 @@ TreeView::InternalNode* TreeView::GetInternalNodeForModelNode(
       model_->GetIndexOf(parent_internal_node->model_node(), model_node));
 }
 
+gfx::Rect TreeView::GetBackgroundBoundsForNode(InternalNode* node) {
+  gfx::Rect row_bounds(GetBoundsForNode(node));
+  if (PlatformStyle::kTreeViewSelectsEntireRow) {
+    row_bounds.set_x(bounds().x());
+    row_bounds.set_width(bounds().width());
+  }
+  return row_bounds;
+}
+
 gfx::Rect TreeView::GetBoundsForNode(InternalNode* node) {

[tapted, 2016/06/13 07:03:47]

perhaps we should rename these to GetTextBoundsForNode[Impl]?

[Elly Fong-Jones, 2016/06/14 17:21:27]

Done.

   int row, depth;
   row = GetRowForInternalNode(node, &depth);
@@ -1011,6 +1016,44 @@ PrefixSelector* TreeView::GetPrefixSelector() {
   return selector_.get();
 }
 
+TreeView::InternalNode* TreeView::NodeAtPoint(const gfx::Point& point,
+                                              int* depth) {
+  int row = (point.y() - kVerticalInset) / row_height_;
+  int local_depth = 0;
+  InternalNode* node = GetNodeByRow(row, &local_depth);
+  if (!node)
+    return nullptr;
+
+  // If the entire row gets a selected background, clicking anywhere in the row
+  // serves to hit this node.
+  if (PlatformStyle::kTreeViewSelectsEntireRow) {
+    if (depth)
+      *depth = local_depth;
+    return node;
+  }
+  gfx::Rect bounds(GetBoundsForNodeImpl(node, row, local_depth));
+  if (bounds.Contains(point)) {
+    if (depth)
+      *depth = local_depth;
+    return node;
+  }
+  return nullptr;
+}
+
+bool TreeView::HitsNodeArrow(InternalNode* node,
+                             int depth,
+                             const gfx::Point& point) {
+  if (!model_->GetChildCount(node->model_node()))
+    return false;

[tapted, 2016/06/13 07:03:48]

nit: blank line after

[Elly Fong-Jones, 2016/06/14 17:21:27]

Done.

+  gfx::Rect bounds(GetBackgroundBoundsForNode(node));
+  int arrow_dx = depth * kIndent + kHorizontalInset;
+  gfx::Rect arrow_bounds(bounds.x() + arrow_dx, bounds.y(), kArrowRegionSize,

[tapted, 2016/06/13 07:03:48]

I don't think we can use bounds.x() here -- GetBackgroundBoundsForNode() returns
different a different x value for kTreeViewSelectsEntireRow, but the arrow
should still be in the same place, regardless of kTreeViewSelectsEntireRow

(it seems ok on mac, so maybe this has regressed on other platforms)

It's possible the fix is just `bounds.x()` -> 'bounds().x()` but that's a bit
too subtle. - We should rename the |bounds| local variable to row_bounds.

Or, perhaps best, just use 

  int row, depth;
  row = GetRowForInternalNode(node, &depth);
  int arrow_dx = ..
  gfx::Rect arrow_bounds(/* x calc */, row * row_height_ + kVerticalInset,
                         kArrowRegionSize, row_height_);
  

i.e. don't pass in |depth| to the function -- GetBackgroundBoundsForNode is
currently calculating it anyway.

[Elly Fong-Jones, 2016/06/14 17:21:27]

Done.

+                         bounds.height());
+  if (base::i18n::IsRTL())
+    arrow_bounds.set_x(bounds.width() - arrow_dx - kArrowRegionSize);
+  return arrow_bounds.Contains(point);
+}
+
 // InternalNode ----------------------------------------------------------------
 
 TreeView::InternalNode::InternalNode()