views::Separator cleanup.

ui/views/controls/separator.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/separator.h"
 
 #include "ui/accessibility/ax_node_data.h"
 #include "ui/gfx/canvas.h"
 #include "ui/native_theme/native_theme.h"
 
 namespace views {
 
 // static
 const char Separator::kViewClassName[] = "Separator";
 
-// The separator size in pixels.
-const int kSeparatorSize = 1;
+// static
+const int Separator::kThickness = 1;
 
-Separator::Separator(Orientation orientation)
-    : orientation_(orientation),
-      color_overridden_(false),
-      size_(kSeparatorSize) {
-  SetColorFromNativeTheme();
-}
+Separator::Separator(Orientation orientation) : orientation_(orientation) {}
 
-Separator::~Separator() {
-}
+Separator::~Separator() {}
 
 void Separator::SetColor(SkColor color) {
-  color_ = color;
-  color_overridden_ = true;
+  overridden_color_ = color;
   SchedulePaint();
 }
 
-void Separator::SetPreferredSize(int size) {
-  if (size != size_) {
-    size_ = size;
+void Separator::SetPreferredLength(int length) {
+  if (length != length_) {
+    length_ = length;
     PreferredSizeChanged();
   }
 }
 
-void Separator::SetColorFromNativeTheme() {
-  color_ = GetNativeTheme()->GetSystemColor(
-      ui::NativeTheme::kColorId_SeparatorColor);
-}
-
 ////////////////////////////////////////////////////////////////////////////////
 // Separator, View overrides:
 
 gfx::Size Separator::GetPreferredSize() const {
-  gfx::Size size =
-      orientation_ == HORIZONTAL ? gfx::Size(1, size_) : gfx::Size(size_, 1);
+  gfx::Size size = orientation_ == HORIZONTAL ? gfx::Size(length_, kThickness)

[tdanderson, 2017/02/06 21:19:06]

You're switching the meaning of HORIZONTAL vs VERTICAL but this CL doesn't
update every call site where a Separator is instantiated.

[Evan Stade, 2017/02/06 21:39:17]

The meaning of horizontal and vertical are the same. I've changed the meaning of
"SetPreferredSize/Length" and only two places call that function, one of which
is going away soon.

Probably I could just rip out that function and the orientation enum altogether
(just always request 1x1 because it's almost always the layout that sets the
length appropriately).

[sky, 2017/02/07 19:02:49]

I see where you are going. I find the change to separator here confusing. When
you create a separator you have an explicit orientation in mind. Is there some
use case I'm not seeing where you know you want a separator but don't know the
orientation?

[Evan Stade, 2017/02/07 21:39:50]

I looked into this but it's not convenient to actually remove this function and
rely on borders for the tray separators.
No, there is not a case where you have a separator but you don't know the
orientation. The issue is that the orientation is totally ignored unless you
also call SetPreferredSize. Of all the places that create separators, only one
uses SetPreferredSize. For n - 1 instantiations, the orientation param is
redundant (with the layout manager) and irrelevant (you could change it to the
other orientation with no change in behavior). For the last instantiation ---
the one that does use SetPreferredSize --- the current code passes the wrong
orientation because SetPreferredSize controls the wrong dimension for that
code's purposes.

tl;dr n - 1 callsites are passing an irrelevant param and 1 callsite is passing
the wrong value, currently.

+                                              : gfx::Size(kThickness, length_);
   gfx::Insets insets = GetInsets();
   size.Enlarge(insets.width(), insets.height());
   return size;
 }
 
 void Separator::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   node_data->role = ui::AX_ROLE_SPLITTER;
 }
 
 void Separator::OnPaint(gfx::Canvas* canvas) {
-  canvas->FillRect(GetContentsBounds(), color_);
-}
+  SkColor color = overridden_color_
+                      ? *overridden_color_
+                      : GetNativeTheme()->GetSystemColor(
+                            ui::NativeTheme::kColorId_SeparatorColor);
 
-void Separator::OnNativeThemeChanged(const ui::NativeTheme* theme) {
-  if (!color_overridden_)
-    SetColorFromNativeTheme();
+  // The separator fills its bounds, but avoid filling partial pixels.

[sky, 2017/02/03 23:49:45]

Are you sure this is the right thing? As views don't necessarily align with
pixels this won't necessarily look nice. I think you should just fillrect.

[Evan Stade, 2017/02/06 16:31:26]

yes. Before/after screenshot posted to bug. If the view is not pixel aligned it
still might not look nice. But with FillRect, it looks bad pixel aligned or not.

[sky, 2017/02/06 17:59:21]

Do we need to turn on aa?

[Evan Stade, 2017/02/06 19:03:29]

We want an integral number of pixels. AA would give us blurry lines.

[sky, 2017/02/06 22:42:55]

I think we're going in circles. We really need views to position and size views
to pixel boundaries. Otherwise we'll continue having non-intuitive code like
this.

[Evan Stade, 2017/02/07 16:57:54]

Yea, but I thought that was a long way off from becoming reality. At this point
is there a way to specify layout size in pixels? Is there another solution here
I'm missing to get a consistent and sharp separator?

+  float dsf = canvas->UndoDeviceScaleFactor();
+  gfx::RectF contents = gfx::ScaleRect(gfx::RectF(GetContentsBounds()), dsf);
+  canvas->FillRect(gfx::ToEnclosedRect(contents), color);
 }
 
 const char* Separator::GetClassName() const {
   return kViewClassName;
 }
 
 }  // namespace views