MacViews: support backgrounds for selected rows

ui/views/controls/tree/tree_view.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/views/controls/tree/tree_view.h"
 
 #include <algorithm>
 
 #include "base/i18n/rtl.h"
 #include "base/message_loop/message_loop.h"
 #include "ui/accessibility/ax_view_state.h"
 #include "ui/base/ime/input_method.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/events/event.h"
 #include "ui/events/keycodes/keyboard_codes.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/rect_conversions.h"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/skia_util.h"
 #include "ui/native_theme/native_theme.h"
 #include "ui/resources/grit/ui_resources.h"
 #include "ui/views/controls/prefix_selector.h"
 #include "ui/views/controls/scroll_view.h"
 #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;
 
 namespace views {
 
 // Insets around the view.
 static const int kHorizontalInset = 2;
 static const int kVerticalInset = 2;
 // Padding before/after the image.
@@ skipping 599 matching lines @@
 }
 
 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));

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

This should use GetBackgroundBoundsForNode() too, otherwise clicking on the row
(but not on the _text_) won't change the row selection. But of course we will
need to pass in `row` and `depth` so it does the right thing (or figure out some
other nice solution).

There's another call in TreeView::ShowContextMenu() which should probably be the
entire row as well, but I haven't checked a Cocoa TreeView to see if (a) it can
have context menus and (b) whether they are invoked outside the text region.

In fact, I think the *only* place we still want to use GetBoundsForNode[Impl] is
in TreeView::LayoutEditor() - and we might want to tweak that as well.

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

I reworked this function and ShowContextMenu to be more explicit that they are
doing hit-testing, by adding NodeAtPoint(), and added HitsNodeArrow() to test
for arrow hits. This function is now much easier to follow, I think.

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

Agree - nice refactoring!

     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());
@@ skipping 77 matching lines @@
                    -(empty_editor_size_.height() - font_list_.GetHeight()) / 2);
   // Give a little extra space for editing.
   row_bounds.set_width(row_bounds.width() + 50);
   editor_->SetBoundsRect(row_bounds);
   editor_->Layout();
 }
 
 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,
                          int min_row,
                          int max_row,
                          InternalNode* node,
                          int depth,
                          int* row) {
   if (*row >= max_row)
     return;
 
   if (*row >= min_row && *row < max_row)
     PaintRow(canvas, node, *row, depth);
   (*row)++;
   if (!node->is_expanded())
     return;
   depth++;
   for (int i = 0; i < node->child_count() && *row < max_row; ++i)
     PaintRows(canvas, min_row, max_row, node->GetChild(i), depth, row);
 }
 
 void TreeView::PaintRow(gfx::Canvas* canvas,
                         InternalNode* node,
                         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());
 
   // Paint the icon.
   gfx::ImageSkia icon;
   int icon_index = model_->GetIconIndex(node->model_node());
   if (icon_index != -1)
     icon = icons_[icon_index];
   else if (node == selected_node_)
     icon = open_icon_;
   else
     icon = closed_icon_;
   int icon_x = kArrowRegionSize + kImagePadding +
                (open_icon_.width() - icon.width()) / 2;
   if (base::i18n::IsRTL())
     icon_x = bounds.right() - icon_x - open_icon_.width();
   else
     icon_x += bounds.x();
   canvas->DrawImageInt(
       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,
                                   const gfx::Rect& node_bounds,
                                   bool expanded) {
   int center_x;
   if (base::i18n::IsRTL()) {
     center_x = node_bounds.right() - kArrowRegionSize +
                (kArrowRegionSize - 4) / 2;
   } else {
@@ skipping 30 matching lines @@
     return NULL;
   if (!parent_internal_node->loaded_children()) {
     if (create_type == DONT_CREATE_IF_NOT_LOADED)
       return NULL;
     LoadChildren(parent_internal_node);
   }
   return parent_internal_node->GetChild(
       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);
   return GetBoundsForNodeImpl(node, row, depth);
 }
 
 gfx::Rect TreeView::GetBoundsForNodeImpl(InternalNode* node,
                                          int row,
                                          int depth) {
   gfx::Rect rect(depth * kIndent + kHorizontalInset,
@@ skipping 166 matching lines @@
   if (!is_expanded_)
     return max_width;
   for (int i = 0; i < child_count(); ++i) {
     max_width = std::max(max_width,
                          GetChild(i)->GetMaxWidth(indent, depth + 1));
   }
   return max_width;
 }
 
 }  // namespace views