Adding ripple effect for clicks on MD slider

ui/views/controls/md_slider.cc

Index: ui/views/controls/md_slider.cc
diff --git a/ui/views/controls/md_slider.cc b/ui/views/controls/md_slider.cc
index 1a58eaec55cd9ec966c6a5ea67eb260224616532..82190c57552d119d6abdb9b93ef4b00ab7bdb458 100644
--- a/ui/views/controls/md_slider.cc
+++ b/ui/views/controls/md_slider.cc
@@ -1,33 +1,44 @@
 // Copyright 2016 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 <iostream>

[bruthig, 2016/09/22 21:05:24]

nit: Remove

[yiyix, 2016/09/23 20:35:36]

Done.

 #include "ui/views/controls/md_slider.h"
 
 #include "third_party/skia/include/core/SkColor.h"
 #include "third_party/skia/include/core/SkPaint.h"
+#include "ui/gfx/animation/slide_animation.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/geometry/point.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/views/controls/slider.h"
 
+namespace gfx {
+class SlideAnimation;

[bruthig, 2016/09/22 21:05:24]

The forward declaration should be in the .h.

[yiyix, 2016/09/23 20:35:37]

sorry, i must have seen so many definition in the file and mistook it to the .h
file.

+}
+
 namespace views {
 
 // Color of slider at the active and the disabled state, respectively.
 const SkColor kActiveColor = SkColorSetARGB(0xFF, 0x42, 0x85, 0xF4);
 const SkColor kDisabledColor = SkColorSetARGB(0x42, 0x00, 0x00, 0x00);
+const SkColor kHighlightColor = SkColorSetARGB(0x4D, 0x42, 0x85, 0xF4);
 
 // The thickness of the slider.
 const int kLineThickness = 2;
 
-// The radius of the thumb of the slider.
-const int kThumbRadius = 6;
+// The radius of the thumb and the highlighted thumb of the slider,
+// respectively.
+const float kThumbRadius = 6.0;

[bruthig, 2016/09/22 21:05:24]

This is currently assigning a double literal, i.e. '6.0' to a float using an
implicit cast.  You should use the following format to assign a float literal
'6.0f' or more concise '6.f'.
Same applies below.

[yiyix, 2016/09/23 20:35:37]

is there any difference between these two casting?

[bruthig, 2016/09/23 22:24:39]

Nope, but I believe 6.f is the preferred way by owners tho.

+const float kThumbHighlightRadius = 10.0;
 
 // The stroke of the thumb when the slider is disabled.
 const int kThumbStroke = 2;
 
+// Duration of the thumb highlight growing effect animation.
+const int kSlideValueChangeDurationMS = 150;
+
 MdSlider::MdSlider(SliderListener* listener)
-    : Slider(listener), is_active_(true) {
+    : Slider(listener), is_active_(true), animating_thumb_value_(0.f) {
   SchedulePaint();
 }
 
@@ -37,17 +48,30 @@ void MdSlider::OnPaint(gfx::Canvas* canvas) {
   Slider::OnPaint(canvas);
 
   // Paint the slider.
-  const int thumb_size = kThumbRadius * 2;
   const gfx::Rect content = GetContentsBounds();
-  const int width = content.width() - thumb_size;
+  const int width = content.width() - kThumbRadius * 2;
   const int full = GetAnimatingValue() * width;
   const int empty = width - full;
   const int y = content.height() / 2 - kLineThickness / 2;
+  const int x = content.x() + full + kThumbRadius;
   canvas->FillRect(gfx::Rect(content.x(), y, full, kLineThickness),
                    is_active_ ? kActiveColor : kDisabledColor);
-  canvas->FillRect(
-      gfx::Rect(content.x() + full + thumb_size, y, empty, kLineThickness),
-      kDisabledColor);
+  canvas->FillRect(gfx::Rect(x + kThumbRadius, y, empty, kLineThickness),
+                   kDisabledColor);
+
+  gfx::Point center(x, content.height() / 2);

[bruthig, 2016/09/22 21:05:24]

nit: |thumb_center| would be more descriptive than just |center|.

[yiyix, 2016/09/23 20:35:36]

Done.

+
+  // Paint the thumb highlight if it exists.
+  if (is_active_ && animating_thumb_value_ > kThumbRadius) {
+    SkPaint highlight;
+    highlight.setColor(kHighlightColor);
+    highlight.setFlags(SkPaint::kAntiAlias_Flag);
+    float highlight_radius =
+        highlight_animation_.get() && highlight_animation_->is_animating()
+            ? animating_thumb_value_
+            : kThumbHighlightRadius;
+    canvas->DrawCircle(center, highlight_radius, highlight);

[bruthig, 2016/09/22 21:05:24]

You should just paint using the |animating_thumb_value_| here.  i.e. Don't take
into account the |highlight_animation_| above.  It is not necessary.

[yiyix, 2016/09/23 20:35:36]

Make sense, it always reflects the correct value no matter if the animation is
on or not.

+  }
 
   // Paint the thumb of the slider.
   SkPaint paint;
@@ -59,8 +83,8 @@ void MdSlider::OnPaint(gfx::Canvas* canvas) {
     paint.setStyle(SkPaint::kStroke_Style);
   }
   canvas->DrawCircle(
-      gfx::Point(content.x() + full + kThumbRadius, content.height() / 2),
-      is_active_ ? kThumbRadius : (kThumbRadius - kThumbStroke / 2), paint);
+      center, is_active_ ? kThumbRadius : (kThumbRadius - kThumbStroke / 2),
+      paint);
 }
 
 const char* MdSlider::GetClassName() const {
@@ -75,4 +99,27 @@ void MdSlider::UpdateState(bool control_on) {
 int MdSlider::GetThumbWidth() {
   return kThumbRadius * 2;
 }
+
+void MdSlider::SetHighlighted(bool focus) {
+  if (focus) {
+    highlight_animation_.reset(new gfx::SlideAnimation(this));

[bruthig, 2016/09/22 21:05:24]

By always creating a new |highlight_animation_| when |focus| is true, you will
get a jerky visual of the highlight disappearing if SetHighlight(true) is called
while a hide animation is in progress.
To avoid this you can make it so that a |highlight_animation_| is only created
if it is not currently null.  And if you want to cleanup the memory you can set
the |highlight_animation_| to null in an AnimationEnded() override.

[yiyix, 2016/09/23 20:35:36]

Done.

+    highlight_animation_->SetSlideDuration(kSlideValueChangeDurationMS);
+    highlight_animation_->Show();
+  } else {
+    highlight_animation_->Hide();

[bruthig, 2016/09/22 21:05:24]

You should probably be more robust here and ensure a |highlight_animation_|
instance exists.  Currently if someone creates a MdSlider and calls
SetHighlighted(false) immediately afterwards it will crash.
If you create a |highlight_animation_| instance whenever it is null, as
suggested above, it should fix this issue too.

[yiyix, 2016/09/23 20:35:36]

good catch!

+  }
+  SchedulePaint();

[bruthig, 2016/09/22 21:05:24]

You shouldn't need to call SchedulePaint() here.  It will be called from
AnimationProgressed().

[yiyix, 2016/09/23 20:35:36]

Done.

+}
+
+void MdSlider::AnimationProgressed(const gfx::Animation* animation) {
+  Slider::AnimationProgressed(animation);
+  if (animation == highlight_animation_.get()) {
+    float old_animating_thumb_value = animating_thumb_value_;
+    animating_thumb_value_ =
+        animation->CurrentValueBetween(kThumbRadius, kThumbHighlightRadius);
+    if (old_animating_thumb_value != animating_thumb_value_)
+      SchedulePaint();
+  }
+}
+
 }  // namespace views