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..9ea5abd278d479fb67db29346396dee26ce57d2d 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;
@@ -742,7 +743,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 +770,12 @@ void TreeView::PaintRow(gfx::Canvas* canvas,
                         int row,
                         int depth) {
   gfx::Rect bounds(GetBoundsForNodeImpl(node, row, depth));
+  const SkColor bg_color = GetNativeTheme()->GetSystemColor(

[tapted, 2016/06/09 07:13:37]

bg_color -> selected_row_bg_color

[Elly Fong-Jones, 2016/06/09 20:24:37]

Done.

+      text_background_color_id(HasFocus()));

[tapted, 2016/06/09 07:13:37]

I think HasFocus() -> HasFocus() || editing_

Otherwise when editing the field editor gets focus, which makes the background
goes grey. Cocoa retains the background color for this case

[Elly Fong-Jones, 2016/06/09 20:24:37]

Done.

+
+  // Paint the row background.
+  if (PlatformStyle::kTreeViewSelectsEntireRow && selected_node_ == node)
+    canvas->FillRect(GetBackgroundBoundsForNode(node), bg_color);
 
   if (model_->GetChildCount(node->model_node()))
     PaintExpandControl(canvas, bounds, node->is_expanded());
@@ -792,29 +799,33 @@ 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());
+  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 text background. In edit mode, the selected node is a separate
+  // editing control, so it does not need a background painted here.
+  if ((!editing_ || selected_node_ != node) &&
+      !PlatformStyle::kTreeViewSelectsEntireRow) {
     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);
+        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.

[tapted, 2016/06/09 07:13:37]

Should this still be inside `if ((!editing_ || selected_node_ != node)`? (i.e.
regardless of kTreeViewSelectsEntireRow). Otherwise, we're painting text "under"
the Textfield control being used for editing. Something neater than
nesting/repeating the `if` statements might be, here,

// No need to paint the text on the row being edited.
if (editing_ && selected_node == node)
  return;

In fact, this can go up on line 801, so that the `if` statement on line 809 only
has to check `if (!PlatformStyle::kTreeViewSelectsEntireRow)`

[Elly Fong-Jones, 2016/06/09 20:24:37]

Done.

+  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 +876,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));

[tapted, 2016/06/09 07:13:38]

This rect we want is much simpler - e.g. |depth| is ignored completely, and we
don't need to mirror anything. I think we should just repeat the y/height
calculations from GetBoundsForNodeImpl

[Elly Fong-Jones, 2016/06/09 20:24:37]

The reason this uses GetBoundsForNode() and then adjusts is so that the
background isn't the entire row on builds where kTreeViewSelectsEntireRow is
false, since the background is used to decide what needs to be repainted in
SchedulePaintForNode().

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

Yeah - I think it would look like


  if (!PlatformStyle::kTreeViewSelectsEntireRow)
    return GetTextBoundsForNode(node);

  int row, ignored_depth;
  row = GetRowForInternalNode(node, &ignored_depth);
  return gfx::Rect(bounds.x(), row * row_height_ + kVerticalInset,
                   bounds().width(), row_height_);

[tapted, 2016/06/15 03:17:49]

ping? any thoughts on this?

[Elly Fong-Jones, 2016/06/16 14:42:34]

Oop, sorry, missed it. Done.

+  if (PlatformStyle::kTreeViewSelectsEntireRow) {
+    row_bounds.set_x(bounds().x());
+    row_bounds.set_width(bounds().width());
+  }
+  return row_bounds;
+}
+
 gfx::Rect TreeView::GetBoundsForNode(InternalNode* node) {
   int row, depth;
   row = GetRowForInternalNode(node, &depth);