Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2335513002:
Adding ripple effect for clicks on MD slider (Closed)
DescriptionAdding ripple effect for clicks on MD slider
The MD slider is currently hiding behind ash-md flag. When the flag is
set to experimental, a highlight effect will be showing for clicking or
touching on MD slider.
TEST=MANUAL
- test if the highlight shows when users click, click and drag or touch
the md slider.
BUG=631775
Committed: https://crrev.com/df1235e312d117abcc12d062de6d74d314080349
Cr-Commit-Position: refs/heads/master@{#422814}
Patch Set 1 #
Total comments: 6
Patch Set 2 : adding animation #
Total comments: 33
Patch Set 3 : address comments + hiding animation #Patch Set 4 : imporve animation #
Total comments: 25
Patch Set 5 : improve animation in slider and address comments #
Total comments: 20
Patch Set 6 : nits #
Total comments: 16
Patch Set 7 : address comments #
Total comments: 13
Patch Set 8 : address comments #
Total comments: 10
Patch Set 9 : impl #Patch Set 10 : add disallow_copy_and_assign to testslider #
Messages
Total messages: 52 (24 generated)
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== impl BUG= ========== to ========== Adding ripple effect for clicking on MD slider BUG=631775 ==========
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Adding ripple effect for clicking on MD slider BUG=631775 ========== to ========== Adding ripple effect for clicks on MD slider The MD slider is currently hiding behind ash-md flag. When the flag is set to experimental, a ripple effect will be showing for clicking or touching on MD slider. TEST=MANUAL - test if the ripple show when users click, click and drag or touch the md slider. BUG=631775 ==========
yiyix@chromium.org changed reviewers: + tdanderson@chromium.org
yiyix@chromium.org changed reviewers: + bruthig@chromium.org - tdanderson@chromium.org
@bruthig, could you please take a look? Thanks.
https://codereview.chromium.org/2335513002/diff/20001/ui/views/controls/md_sl... File ui/views/controls/md_slider.cc (right): https://codereview.chromium.org/2335513002/diff/20001/ui/views/controls/md_sl... ui/views/controls/md_slider.cc:19: const SkColor kRippleColor = SkColorSetARGB(0x4D, 0x42, 0x85, 0xF4); I'd recommend using a more generic, and hopefully more intuitive, kHighlightColor instead of 'Ripple'. Applies elsewhere too. https://codereview.chromium.org/2335513002/diff/20001/ui/views/controls/md_sl... ui/views/controls/md_slider.cc:42: is_pressed_ = true; If you replace |is_pressed_| with an int |thumb_ripple_radius_| you should be able to add the follow-up animation quite easily. https://codereview.chromium.org/2335513002/diff/20001/ui/views/controls/md_sl... File ui/views/controls/md_slider.h (right): https://codereview.chromium.org/2335513002/diff/20001/ui/views/controls/md_sl... ui/views/controls/md_slider.h:35: bool OnMousePressed(const ui::MouseEvent& event) override; What about touch and keyboard interactions? I assume we would want the highlight for touch for sure but don't know about keyboard. It might make sense to re-use the Slider::PrepareForMove() method as MousePressed() and OnGestureEvent() already call this.
https://codereview.chromium.org/2335513002/diff/20001/ui/views/controls/md_sl... File ui/views/controls/md_slider.cc (right): https://codereview.chromium.org/2335513002/diff/20001/ui/views/controls/md_sl... ui/views/controls/md_slider.cc:19: const SkColor kRippleColor = SkColorSetARGB(0x4D, 0x42, 0x85, 0xF4); On 2016/09/12 17:16:48, bruthig wrote: > I'd recommend using a more generic, and hopefully more intuitive, > kHighlightColor instead of 'Ripple'. Applies elsewhere too. Done. https://codereview.chromium.org/2335513002/diff/20001/ui/views/controls/md_sl... ui/views/controls/md_slider.cc:42: is_pressed_ = true; On 2016/09/12 17:16:48, bruthig wrote: > If you replace |is_pressed_| with an int |thumb_ripple_radius_| you should be > able to add the follow-up animation quite easily. Thanks, I think so.. https://codereview.chromium.org/2335513002/diff/20001/ui/views/controls/md_sl... File ui/views/controls/md_slider.h (right): https://codereview.chromium.org/2335513002/diff/20001/ui/views/controls/md_sl... ui/views/controls/md_slider.h:35: bool OnMousePressed(const ui::MouseEvent& event) override; On 2016/09/12 17:16:48, bruthig wrote: > What about touch and keyboard interactions? I assume we would want the > highlight for touch for sure but don't know about keyboard. > > It might make sense to re-use the Slider::PrepareForMove() method as > MousePressed() and OnGestureEvent() already call this. Instead of override functions, I created a new function called SetFocus, which will be set when the slider is selected. Please take a look at this new approach.
On 2016/09/20 20:25:46, yiyix wrote: > https://codereview.chromium.org/2335513002/diff/20001/ui/views/controls/md_sl... > File ui/views/controls/md_slider.cc (right): > > https://codereview.chromium.org/2335513002/diff/20001/ui/views/controls/md_sl... > ui/views/controls/md_slider.cc:19: const SkColor kRippleColor = > SkColorSetARGB(0x4D, 0x42, 0x85, 0xF4); > On 2016/09/12 17:16:48, bruthig wrote: > > I'd recommend using a more generic, and hopefully more intuitive, > > kHighlightColor instead of 'Ripple'. Applies elsewhere too. > > Done. > > https://codereview.chromium.org/2335513002/diff/20001/ui/views/controls/md_sl... > ui/views/controls/md_slider.cc:42: is_pressed_ = true; > On 2016/09/12 17:16:48, bruthig wrote: > > If you replace |is_pressed_| with an int |thumb_ripple_radius_| you should be > > able to add the follow-up animation quite easily. > > Thanks, I think so.. > > https://codereview.chromium.org/2335513002/diff/20001/ui/views/controls/md_sl... > File ui/views/controls/md_slider.h (right): > > https://codereview.chromium.org/2335513002/diff/20001/ui/views/controls/md_sl... > ui/views/controls/md_slider.h:35: bool OnMousePressed(const ui::MouseEvent& > event) override; > On 2016/09/12 17:16:48, bruthig wrote: > > What about touch and keyboard interactions? I assume we would want the > > highlight for touch for sure but don't know about keyboard. > > > > It might make sense to re-use the Slider::PrepareForMove() method as > > MousePressed() and OnGestureEvent() already call this. > Instead of override functions, I created a new function called SetFocus, which > will be set when the slider is selected. Please take a look at this new > approach. Finished adding animation, @bruthig, could you take a first look at this new approach?
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/md_sl... File ui/views/controls/md_slider.cc (right): https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/md_sl... ui/views/controls/md_slider.cc:34: // Duration of the animation. Be more specific here. i.e. What 'animation'? https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/md_sl... ui/views/controls/md_slider.cc:73: if (is_active_ && value_ > 0) { Is it an intentional decision to paint the highlight on top of the thumb? With the disclaimer that I haven't seen it, I feel like it would look better if the highlight was painted behind the thumb. i.e. move this block above the thumb painting on line 69 https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/md_sl... ui/views/controls/md_slider.cc:78: float val = Using |highlight_radius| variable name would be more appropriate than |val|. https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/md_sl... ui/views/controls/md_slider.cc:79: highlight_animation_.get() && highlight_animation_->is_animating() It would be less confusing if the painting of the highlight wasn't dependent on these three different variables: |value_|, |highlight_animation_|, and |animating_thumb_value_|. You should be able to collapse these 3 conditions into a single |highlight_radius_| value. It would then be the AnimationProgressed() callback's responsibility to set this |highlight_radius_| value. It might make sense to discuss this one offline. https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/md_sl... ui/views/controls/md_slider.cc:85: gfx::Point(content.x() + full + kThumbRadius, content.height() / 2), Consider computing the thumb center point once and reusing that value on line 70 as well as here. https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/md_sl... ui/views/controls/md_slider.cc:111: value_ = 0; I think it would be more visually pleasing if the highlight animated away when disappearing. You should be able to use the SlideAnimation::Hide() to do this. https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/md_sl... ui/views/controls/md_slider.cc:118: animating_thumb_value_ = This should only update |animating_thumb_value_| if |animation| == |highlight_animation_|. And it should call SchedulePaint() when |animating_thumb_value_| is actually updated. https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/md_sl... File ui/views/controls/md_slider.h (right): https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/md_sl... ui/views/controls/md_slider.h:9: #include "ui/gfx/animation/animation_delegate.h" nit: You shouldn't need to include this since slider.h already does. https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/md_sl... ui/views/controls/md_slider.h:46: float value_; The variable name |highlight_radius_| would be more appropriate/descriptive than the generic |value_|. https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/md_sl... ui/views/controls/md_slider.h:48: std::unique_ptr<gfx::SlideAnimation> highlight_animation_; nit: You should forward declare SlideAnimation. https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/slide... File ui/views/controls/slider.cc (right): https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/slide... ui/views/controls/slider.cc:182: SetFocus(true); Instead of calling SetFocus() from OnMousePressed(), OnMouseReleased(), OnGestureEvent(), I think you should move it to OnSliderDragStarted() and OnSliderDragEnded(). https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/slide... ui/views/controls/slider.cc:250: animating_value_ = animation->CurrentValueBetween(animating_value_, value_); This should only update |animating_value_| if |animation| == |move_animation_|. And it should only call SchedulePaint() when |animating_value_| is updated. https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/slider.h File ui/views/controls/slider.h (right): https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/slide... ui/views/controls/slider.h:75: virtual void SetFocus(bool focus){}; Move definition to .cc file. https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/slide... ui/views/controls/slider.h:75: virtual void SetFocus(bool focus){}; The 'focus' concept is already defined on View and this 'focus' means something a little different. Instead of overloading the term might I suggest renaming SetFocus() to SetHighlighted(). https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/slide... ui/views/controls/slider.h:77: void OnMouseReleased(const ui::MouseEvent& event) override; OnMousePressed() and OnMouseReleased() should be grouped with the other similar overrides on line 91. https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/slide... ui/views/controls/slider.h:79: protected: nit: Remove, protected is already declared on line 74. https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/slide... ui/views/controls/slider.h:80: explicit Slider(SliderListener* listener); Style guide specifies constructors should be declared before functions.
https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/md_sl... File ui/views/controls/md_slider.cc (right): https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/md_sl... ui/views/controls/md_slider.cc:34: // Duration of the animation. On 2016/09/21 00:27:45, bruthig wrote: > Be more specific here. i.e. What 'animation'? Sorry, I was playing with slide animation and linear animation, i was keep changing and keep rolling back changes. Sorry to give you something incomplete to review. https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/md_sl... ui/views/controls/md_slider.cc:73: if (is_active_ && value_ > 0) { On 2016/09/21 00:27:46, bruthig wrote: > Is it an intentional decision to paint the highlight on top of the thumb? With > the disclaimer that I haven't seen it, I feel like it would look better if the > highlight was painted behind the thumb. i.e. move this block above the thumb > painting on line 69 The highlight and the thumb are not over each other, they are exclusive. I did some math on the size of stroke and the size of the thumb, so that they are exclusive. https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/md_sl... ui/views/controls/md_slider.cc:78: float val = On 2016/09/21 00:27:45, bruthig wrote: > Using |highlight_radius| variable name would be more appropriate than |val|. Done. https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/md_sl... ui/views/controls/md_slider.cc:79: highlight_animation_.get() && highlight_animation_->is_animating() On 2016/09/21 00:27:45, bruthig wrote: > It would be less confusing if the painting of the highlight wasn't dependent on > these three different variables: |value_|, |highlight_animation_|, and > |animating_thumb_value_|. You should be able to collapse these 3 conditions > into a single |highlight_radius_| value. It would then be the > AnimationProgressed() callback's responsibility to set this |highlight_radius_| > value. It might make sense to discuss this one offline. I don't really get it, the highlight is dependent on |kThumbRadius| (fixed val) and |highlight_radius| (variable val) only; and |highlight_radius| is determined by animation. As the stroke value changes, I need to change the circle radius, so the highlight effect stays separately from the thumb drawing. we can talk about this computation offline. https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/md_sl... ui/views/controls/md_slider.cc:85: gfx::Point(content.x() + full + kThumbRadius, content.height() / 2), On 2016/09/21 00:27:45, bruthig wrote: > Consider computing the thumb center point once and reusing that value on line 70 > as well as here. good point. https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/md_sl... ui/views/controls/md_slider.cc:111: value_ = 0; On 2016/09/21 00:27:45, bruthig wrote: > I think it would be more visually pleasing if the highlight animated away when > disappearing. You should be able to use the SlideAnimation::Hide() to do this. The animation was not really specified in the design doc, I was thinkking let sebastien play with it and give some comments. I have added the hiding animation. https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/md_sl... ui/views/controls/md_slider.cc:118: animating_thumb_value_ = On 2016/09/21 00:27:45, bruthig wrote: > This should only update |animating_thumb_value_| if |animation| == > |highlight_animation_|. And it should call SchedulePaint() when > |animating_thumb_value_| is actually updated. Done. https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/md_sl... File ui/views/controls/md_slider.h (right): https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/md_sl... ui/views/controls/md_slider.h:9: #include "ui/gfx/animation/animation_delegate.h" On 2016/09/21 00:27:46, bruthig wrote: > nit: You shouldn't need to include this since slider.h already does. Done. https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/md_sl... ui/views/controls/md_slider.h:46: float value_; On 2016/09/21 00:27:46, bruthig wrote: > The variable name |highlight_radius_| would be more appropriate/descriptive than > the generic |value_|. Done. https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/md_sl... ui/views/controls/md_slider.h:48: std::unique_ptr<gfx::SlideAnimation> highlight_animation_; On 2016/09/21 00:27:46, bruthig wrote: > nit: You should forward declare SlideAnimation. Done. https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/slide... File ui/views/controls/slider.cc (right): https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/slide... ui/views/controls/slider.cc:182: SetFocus(true); On 2016/09/21 00:27:46, bruthig wrote: > Instead of calling SetFocus() from OnMousePressed(), OnMouseReleased(), > OnGestureEvent(), I think you should move it to OnSliderDragStarted() and > OnSliderDragEnded(). good call!! https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/slide... ui/views/controls/slider.cc:250: animating_value_ = animation->CurrentValueBetween(animating_value_, value_); On 2016/09/21 00:27:46, bruthig wrote: > This should only update |animating_value_| if |animation| == |move_animation_|. > And it should only call SchedulePaint() when |animating_value_| is updated. Done. https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/slider.h File ui/views/controls/slider.h (right): https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/slide... ui/views/controls/slider.h:75: virtual void SetFocus(bool focus){}; On 2016/09/21 00:27:46, bruthig wrote: > The 'focus' concept is already defined on View and this 'focus' means something > a little different. Instead of overloading the term might I suggest renaming > SetFocus() to SetHighlighted(). Done. https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/slide... ui/views/controls/slider.h:77: void OnMouseReleased(const ui::MouseEvent& event) override; On 2016/09/21 00:27:46, bruthig wrote: > OnMousePressed() and OnMouseReleased() should be grouped with the other similar > overrides on line 91. Done. https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/slide... ui/views/controls/slider.h:79: protected: On 2016/09/21 00:27:46, bruthig wrote: > nit: Remove, protected is already declared on line 74. Sorry again, It is not a fully finish version when i sent it out... I was asking for a first look to look at the idea of the animation implementation. https://codereview.chromium.org/2335513002/diff/60001/ui/views/controls/slide... ui/views/controls/slider.h:80: explicit Slider(SliderListener* listener); On 2016/09/21 00:27:46, bruthig wrote: > Style guide specifies constructors should be declared before functions. Done.
@bruthig, thank you so much for suggesting me to use the hiding animation and show me how to do it. I really like the version I have now~! Could you please take another review on this code change?
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
It's looking pretty good overall. Just a few suggestions to polish it up a bit :) https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/md_s... File ui/views/controls/md_slider.cc (right): https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:4: #include <iostream> nit: Remove https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:16: class SlideAnimation; The forward declaration should be in the .h. https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:31: const float kThumbRadius = 6.0; 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. https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:62: gfx::Point center(x, content.height() / 2); nit: |thumb_center| would be more descriptive than just |center|. https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:73: canvas->DrawCircle(center, highlight_radius, highlight); 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. https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:105: highlight_animation_.reset(new gfx::SlideAnimation(this)); 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. https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:109: highlight_animation_->Hide(); 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. https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:111: SchedulePaint(); You shouldn't need to call SchedulePaint() here. It will be called from AnimationProgressed(). https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/md_s... File ui/views/controls/md_slider.h (right): https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/md_s... ui/views/controls/md_slider.h:42: float animating_thumb_value_; Can you use a more descriptive name than |animating_thumb_value_|? e.g. |thumb_highlight_radius_| https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/slid... File ui/views/controls/slider.cc (right): https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/slid... ui/views/controls/slider.cc:180: return; nit: Remove https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/slid... ui/views/controls/slider.cc:250: if (move_animation_.get() && (animation == move_animation_.get())) { nit: Checking that |move_animation_| here is redundant. I think it's safe to assume |animation| will never be null. https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/slid... File ui/views/controls/slider.h (right): https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/slid... ui/views/controls/slider.h:76: virtual void SetHighlighted(bool focus); nit: Rename |focus| param to be consistent with function name. Applies elsewhere too.
Patchset #5 (id:120001) has been deleted
yiyix@chromium.org changed reviewers: + sadrul@chromium.org, varkha@chromium.org
@varkha and @sadrul, could you please take a look at this change? thank you https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/md_s... File ui/views/controls/md_slider.cc (right): https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:4: #include <iostream> On 2016/09/22 21:05:24, bruthig wrote: > nit: Remove Done. https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:16: class SlideAnimation; On 2016/09/22 21:05:24, bruthig wrote: > The forward declaration should be in the .h. sorry, i must have seen so many definition in the file and mistook it to the .h file. https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:31: const float kThumbRadius = 6.0; On 2016/09/22 21:05:24, bruthig wrote: > 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. is there any difference between these two casting? https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:62: gfx::Point center(x, content.height() / 2); On 2016/09/22 21:05:24, bruthig wrote: > nit: |thumb_center| would be more descriptive than just |center|. Done. https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:73: canvas->DrawCircle(center, highlight_radius, highlight); On 2016/09/22 21:05:24, bruthig wrote: > 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. Make sense, it always reflects the correct value no matter if the animation is on or not. https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:105: highlight_animation_.reset(new gfx::SlideAnimation(this)); On 2016/09/22 21:05:24, bruthig wrote: > 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. Done. https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:109: highlight_animation_->Hide(); On 2016/09/22 21:05:24, bruthig wrote: > 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. good catch! https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:111: SchedulePaint(); On 2016/09/22 21:05:24, bruthig wrote: > You shouldn't need to call SchedulePaint() here. It will be called from > AnimationProgressed(). Done. https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/md_s... File ui/views/controls/md_slider.h (right): https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/md_s... ui/views/controls/md_slider.h:42: float animating_thumb_value_; On 2016/09/22 21:05:24, bruthig wrote: > Can you use a more descriptive name than |animating_thumb_value_|? e.g. > |thumb_highlight_radius_| sure, i also noticed that i forget to change the description above. I am now animating the radius instead. https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/slid... File ui/views/controls/slider.cc (right): https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/slid... ui/views/controls/slider.cc:180: return; On 2016/09/22 21:05:24, bruthig wrote: > nit: Remove Highlight needs a default implementation, so non_md_slider does not need to implement this function. I originally have "virtual void SetHighlighted(bool focus){};" and you asked me to move it to cc file. So does it mean that i should have it in the .h file? https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/slid... File ui/views/controls/slider.h (right): https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/slid... ui/views/controls/slider.h:76: virtual void SetHighlighted(bool focus); On 2016/09/22 21:05:24, bruthig wrote: > nit: Rename |focus| param to be consistent with function name. Applies > elsewhere too. Done.
yiyix@chromium.org changed reviewers: + tdanderson@chromium.org
Description was changed from ========== Adding ripple effect for clicks on MD slider The MD slider is currently hiding behind ash-md flag. When the flag is set to experimental, a ripple effect will be showing for clicking or touching on MD slider. TEST=MANUAL - test if the ripple show when users click, click and drag or touch the md slider. BUG=631775 ========== to ========== Adding ripple effect for clicks on MD slider The MD slider is currently hiding behind ash-md flag. When the flag is set to experimental, a highlight effect will be showing for clicking or touching on MD slider. TEST=MANUAL - test if the highlight shows when users click, click and drag or touch the md slider. BUG=631775 ==========
A few nits, otherwise lgtm. I defer to Ben/Sadrul for a more detailed look though. https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/md_s... File ui/views/controls/md_slider.cc (right): https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:118: !highlight_animation_->IsShowing()) nit: include {} since boolean condition spans > 1 line https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/slid... File ui/views/controls/slider.cc (right): https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/slid... ui/views/controls/slider.cc:249: if (animation == move_animation_.get() && !move_animation_->IsShowing()) { nit: no {} https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/slid... File ui/views/controls/slider.h (right): https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/slid... ui/views/controls/slider.h:76: virtual void SetHighlighted(bool is_highlighted) {} nit: Move the default (no-op) implementation {} into the .cc
https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/md_s... File ui/views/controls/md_slider.cc (right): https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:118: !highlight_animation_->IsShowing()) On 2016/09/23 20:55:50, tdanderson wrote: > nit: include {} since boolean condition spans > 1 line Done. https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/slid... File ui/views/controls/slider.cc (right): https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/slid... ui/views/controls/slider.cc:249: if (animation == move_animation_.get() && !move_animation_->IsShowing()) { On 2016/09/23 20:55:50, tdanderson wrote: > nit: no {} Done. https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/slid... File ui/views/controls/slider.h (right): https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/slid... ui/views/controls/slider.h:76: virtual void SetHighlighted(bool is_highlighted) {} On 2016/09/23 20:55:50, tdanderson wrote: > nit: Move the default (no-op) implementation {} into the .cc Done.
https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/md_s... File ui/views/controls/md_slider.cc (right): https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:31: const float kThumbRadius = 6.0; On 2016/09/23 20:35:37, yiyix wrote: > On 2016/09/22 21:05:24, bruthig wrote: > > 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. > > is there any difference between these two casting? Nope, but I believe 6.f is the preferred way by owners tho. https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/slid... File ui/views/controls/slider.cc (right): https://codereview.chromium.org/2335513002/diff/100001/ui/views/controls/slid... ui/views/controls/slider.cc:180: return; On 2016/09/23 20:35:37, yiyix wrote: > On 2016/09/22 21:05:24, bruthig wrote: > > nit: Remove > > Highlight needs a default implementation, so non_md_slider does not need to > implement this function. I originally have "virtual void SetHighlighted(bool > focus){};" and you asked me to move it to cc file. So does it mean that i should > have it in the .h file? I was just asking you to remove the return; statement. https://codereview.chromium.org/2335513002/diff/160001/ui/views/controls/md_s... File ui/views/controls/md_slider.cc (right): https://codereview.chromium.org/2335513002/diff/160001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:99: highlight_animation_->SetSlideDuration(kSlideHighlightChangeDurationMs); nit: The duration can be set when constructing the |highlight_animation_| above. https://codereview.chromium.org/2335513002/diff/160001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:118: !highlight_animation_->IsShowing()) { FTR I am not suggesting to remove the second clause here (like I did in Slider::AnimationEnded()), because the |highlight_animation_| is in almost all cases going to be animated to hidden and it makes sense to only destroy it when it does because of this. https://codereview.chromium.org/2335513002/diff/160001/ui/views/controls/md_s... File ui/views/controls/md_slider.h (right): https://codereview.chromium.org/2335513002/diff/160001/ui/views/controls/md_s... ui/views/controls/md_slider.h:45: std::unique_ptr<gfx::SlideAnimation> highlight_animation_; nit: You should forward declare SlideAnimation above. The only reason this isn't currently a compile time error is because ui/views/controls/slider.h is already forward declaring it, but you shouldn't be relying on that because slider.h should be able to remove it without creating an error in this .h file. https://codereview.chromium.org/2335513002/diff/160001/ui/views/controls/slid... File ui/views/controls/slider.cc (right): https://codereview.chromium.org/2335513002/diff/160001/ui/views/controls/slid... ui/views/controls/slider.cc:75: nit: no newline required. the braces can be on the same line. Running 'git cl format' might do this for you ;) https://codereview.chromium.org/2335513002/diff/160001/ui/views/controls/slid... ui/views/controls/slider.cc:101: if (!move_animation_.get() || !move_animation_->is_animating()) { This new condition confuses me a little, what is its purpose? https://codereview.chromium.org/2335513002/diff/160001/ui/views/controls/slid... ui/views/controls/slider.cc:248: if (animation && animation != move_animation_.get()) nit: I think it's safe to assume |animation| is not null here. i.e. drop the first clause. If it were possible for |animation| to be null I don't think you want to be calling SchedulePaint() anyway right? https://codereview.chromium.org/2335513002/diff/160001/ui/views/controls/slid... ui/views/controls/slider.cc:254: if (animation == move_animation_.get() && !move_animation_->IsShowing()) From what I can see Hide() will never be used on the |move_animation_|. Thus !move_animation_->IsShowing() will always be false and you will never be destroying the |move_animation_|. You should be able to safely drop the second clause here. https://codereview.chromium.org/2335513002/diff/160001/ui/views/controls/slid... File ui/views/controls/slider.h (right): https://codereview.chromium.org/2335513002/diff/160001/ui/views/controls/slid... ui/views/controls/slider.h:76: virtual void SetHighlighted(bool is_highlighted); nit: Can you group this together with the other virtual function below? It makes it easier to see what things can be extended if there are closer together.
https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/md_s... File ui/views/controls/md_slider.cc (right): https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:20: const SkColor kHighlightColor = SkColorSetARGB(0x4D, 0x42, 0x85, 0xF4); Can this be a derivative of kActiveColor? https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:54: is_active_ ? kActiveColor : kDisabledColor); nit: you can cache the color and use it below. https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/md_s... File ui/views/controls/md_slider.h (right): https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/md_s... ui/views/controls/md_slider.h:16: // MaterialDesignSlider as the default slider implementation. (crbug.com/614453) nit: MdSlider to match the class name. https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/slid... File ui/views/controls/slider.cc (right): https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/slid... ui/views/controls/slider.cc:96: if (!move_animation_.get() || !move_animation_->is_animating()) { I suspect you could remove ".get()". https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/slid... ui/views/controls/slider.cc:172: return move_animation_.get() && move_animation_->is_animating() See if you don't need ".get()". https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/slid... ui/views/controls/slider.cc:244: return; So you are no longer doing exponential approximation to the target? https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/slid... File ui/views/controls/slider.h (right): https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/slid... ui/views/controls/slider.h:85: void AnimationEnded(const gfx::Animation* animation) override; nit: methods here and in .cc should follow the order of declarations of ancestors in line 49.
Thank you all the detailed comments, again. @bruthig and varkha, Could you please take another look? https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/md_s... File ui/views/controls/md_slider.cc (right): https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:20: const SkColor kHighlightColor = SkColorSetARGB(0x4D, 0x42, 0x85, 0xF4); On 2016/09/23 22:30:25, varkha wrote: > Can this be a derivative of kActiveColor? I now try to save the alpha value instead and create the highlight color from the kActiveColor now. https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:54: is_active_ ? kActiveColor : kDisabledColor); On 2016/09/23 22:30:25, varkha wrote: > nit: you can cache the color and use it below. Sure. thanks. https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/md_s... File ui/views/controls/md_slider.h (right): https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/md_s... ui/views/controls/md_slider.h:16: // MaterialDesignSlider as the default slider implementation. (crbug.com/614453) On 2016/09/23 22:30:25, varkha wrote: > nit: MdSlider to match the class name. Done. https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/slid... File ui/views/controls/slider.cc (right): https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/slid... ui/views/controls/slider.cc:96: if (!move_animation_.get() || !move_animation_->is_animating()) { On 2016/09/23 22:30:25, varkha wrote: > I suspect you could remove ".get()". right, i missed this after our discussion yesterday. https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/slid... ui/views/controls/slider.cc:172: return move_animation_.get() && move_animation_->is_animating() On 2016/09/23 22:30:25, varkha wrote: > See if you don't need ".get()". Done. https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/slid... ui/views/controls/slider.cc:244: return; On 2016/09/23 22:30:25, varkha wrote: > So you are no longer doing exponential approximation to the target? No, I like the default SlideAnimation behavior https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/slid... File ui/views/controls/slider.h (right): https://codereview.chromium.org/2335513002/diff/140001/ui/views/controls/slid... ui/views/controls/slider.h:85: void AnimationEnded(const gfx::Animation* animation) override; On 2016/09/23 22:30:25, varkha wrote: > nit: methods here and in .cc should follow the order of declarations of > ancestors in line 49. Done. https://codereview.chromium.org/2335513002/diff/160001/ui/views/controls/md_s... File ui/views/controls/md_slider.cc (right): https://codereview.chromium.org/2335513002/diff/160001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:99: highlight_animation_->SetSlideDuration(kSlideHighlightChangeDurationMs); On 2016/09/23 22:24:39, bruthig (OOO Sep 23-28) wrote: > nit: The duration can be set when constructing the |highlight_animation_| above. Done. https://codereview.chromium.org/2335513002/diff/160001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:118: !highlight_animation_->IsShowing()) { On 2016/09/23 22:24:39, bruthig (OOO Sep 23-28) wrote: > FTR I am not suggesting to remove the second clause here (like I did in > Slider::AnimationEnded()), because the |highlight_animation_| is in almost all > cases going to be animated to hidden and it makes sense to only destroy it when > it does because of this. In all normal use cases, the hiding animation will be called after the showing animation. I will keep it as it is. https://codereview.chromium.org/2335513002/diff/160001/ui/views/controls/md_s... File ui/views/controls/md_slider.h (right): https://codereview.chromium.org/2335513002/diff/160001/ui/views/controls/md_s... ui/views/controls/md_slider.h:45: std::unique_ptr<gfx::SlideAnimation> highlight_animation_; On 2016/09/23 22:24:39, bruthig (OOO Sep 23-28) wrote: > nit: You should forward declare SlideAnimation above. The only reason this > isn't currently a compile time error is because ui/views/controls/slider.h is > already forward declaring it, but you shouldn't be relying on that because > slider.h should be able to remove it without creating an error in this .h file. Done. https://codereview.chromium.org/2335513002/diff/160001/ui/views/controls/slid... File ui/views/controls/slider.cc (right): https://codereview.chromium.org/2335513002/diff/160001/ui/views/controls/slid... ui/views/controls/slider.cc:75: On 2016/09/23 22:24:39, bruthig (OOO Sep 23-28) wrote: > nit: no newline required. the braces can be on the same line. Running 'git cl > format' might do this for you ;) Done. format did not fix this.... unfortunately. https://codereview.chromium.org/2335513002/diff/160001/ui/views/controls/slid... ui/views/controls/slider.cc:101: if (!move_animation_.get() || !move_animation_->is_animating()) { On 2016/09/23 22:24:39, bruthig (OOO Sep 23-28) wrote: > This new condition confuses me a little, what is its purpose? This is because I had !move_animation_->is_animating() then reset the animation in my animationEnded functions. I was imagining that the move_animation_ is reset when this condition holds. As we can safely remove this reset in animationEnded, I can safely remove this second condition. https://codereview.chromium.org/2335513002/diff/160001/ui/views/controls/slid... ui/views/controls/slider.cc:248: if (animation && animation != move_animation_.get()) On 2016/09/23 22:24:39, bruthig (OOO Sep 23-28) wrote: > nit: I think it's safe to assume |animation| is not null here. i.e. drop the > first clause. If it were possible for |animation| to be null I don't think you > want to be calling SchedulePaint() anyway right? I think you mean "if it were possible for |animation| to be null, I think you want to be calling SchedulePaint() anyways" :P I agree with you, i want to paint the new state. https://codereview.chromium.org/2335513002/diff/160001/ui/views/controls/slid... ui/views/controls/slider.cc:254: if (animation == move_animation_.get() && !move_animation_->IsShowing()) On 2016/09/23 22:24:39, bruthig (OOO Sep 23-28) wrote: > From what I can see Hide() will never be used on the |move_animation_|. Thus > !move_animation_->IsShowing() will always be false and you will never be > destroying the |move_animation_|. You should be able to safely drop the second > clause here. You right, I never hide anything in this class https://codereview.chromium.org/2335513002/diff/160001/ui/views/controls/slid... File ui/views/controls/slider.h (right): https://codereview.chromium.org/2335513002/diff/160001/ui/views/controls/slid... ui/views/controls/slider.h:76: virtual void SetHighlighted(bool is_highlighted); On 2016/09/23 22:24:39, bruthig (OOO Sep 23-28) wrote: > nit: Can you group this together with the other virtual function below? It > makes it easier to see what things can be extended if there are closer together. i will group is with getAnimatingValue().
lgtm with nits. https://codereview.chromium.org/2335513002/diff/180001/ui/views/controls/md_s... File ui/views/controls/md_slider.cc (right): https://codereview.chromium.org/2335513002/diff/180001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:101: highlight_animation_->SetSlideDuration(kSlideHighlightChangeDurationMs); nit: Since it is never changed, the duration can be only set once (under if). https://codereview.chromium.org/2335513002/diff/180001/ui/views/controls/md_s... File ui/views/controls/md_slider.h (right): https://codereview.chromium.org/2335513002/diff/180001/ui/views/controls/md_s... ui/views/controls/md_slider.h:46: // Animating value of the current radius size of the thumb's highlight. nit: current radius size -> current radius https://codereview.chromium.org/2335513002/diff/180001/ui/views/controls/slid... File ui/views/controls/slider.cc (right): https://codereview.chromium.org/2335513002/diff/180001/ui/views/controls/slid... ui/views/controls/slider.cc:242: } nit: I think it would be easier to read this class if you order the methods as in the .h file. Sorry we haven't noticed this before. https://codereview.chromium.org/2335513002/diff/180001/ui/views/controls/slid... File ui/views/controls/slider.h (right): https://codereview.chromium.org/2335513002/diff/180001/ui/views/controls/slid... ui/views/controls/slider.h:81: // false to hide the effect. nit: maybe just "Shows or hides the highlight on the slider thumb. The default implantation does nothing.". https://codereview.chromium.org/2335513002/diff/180001/ui/views/controls/slid... ui/views/controls/slider.h:84: virtual int GetThumbWidth() = 0; nit: doc?
https://codereview.chromium.org/2335513002/diff/180001/ui/views/controls/md_s... File ui/views/controls/md_slider.cc (right): https://codereview.chromium.org/2335513002/diff/180001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:122: } Call back into Slider::AnimationEnded() as well?
@sadrul, bruthig: could you please take another look? https://codereview.chromium.org/2335513002/diff/180001/ui/views/controls/md_s... File ui/views/controls/md_slider.cc (right): https://codereview.chromium.org/2335513002/diff/180001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:101: highlight_animation_->SetSlideDuration(kSlideHighlightChangeDurationMs); On 2016/09/24 13:59:18, varkha wrote: > nit: Since it is never changed, the duration can be only set once (under if). Done. https://codereview.chromium.org/2335513002/diff/180001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:122: } On 2016/09/26 19:54:16, sadrul wrote: > Call back into Slider::AnimationEnded() as well? Done. https://codereview.chromium.org/2335513002/diff/180001/ui/views/controls/md_s... File ui/views/controls/md_slider.h (right): https://codereview.chromium.org/2335513002/diff/180001/ui/views/controls/md_s... ui/views/controls/md_slider.h:46: // Animating value of the current radius size of the thumb's highlight. On 2016/09/24 13:59:18, varkha wrote: > nit: current radius size -> current radius Done. https://codereview.chromium.org/2335513002/diff/180001/ui/views/controls/slid... File ui/views/controls/slider.cc (right): https://codereview.chromium.org/2335513002/diff/180001/ui/views/controls/slid... ui/views/controls/slider.cc:242: } On 2016/09/24 13:59:18, varkha wrote: > nit: I think it would be easier to read this class if you order the methods as > in the .h file. Sorry we haven't noticed this before. Done. https://codereview.chromium.org/2335513002/diff/180001/ui/views/controls/slid... File ui/views/controls/slider.h (right): https://codereview.chromium.org/2335513002/diff/180001/ui/views/controls/slid... ui/views/controls/slider.h:81: // false to hide the effect. On 2016/09/24 13:59:18, varkha wrote: > nit: maybe just "Shows or hides the highlight on the slider thumb. The default > implantation does nothing.". I like that it points out the default implementation is empty. https://codereview.chromium.org/2335513002/diff/180001/ui/views/controls/slid... ui/views/controls/slider.h:84: virtual int GetThumbWidth() = 0; On 2016/09/24 13:59:18, varkha wrote: > nit: doc? Done.
https://codereview.chromium.org/2335513002/diff/200001/ui/views/controls/md_s... File ui/views/controls/md_slider.cc (right): https://codereview.chromium.org/2335513002/diff/200001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:20: const U8CPU kHighlightColorAlpha = 0x4D; I don't see U8CPU type used anywhere else in chrome. Use uint8_t instead. https://codereview.chromium.org/2335513002/diff/200001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:100: highlight_animation_.reset(new gfx::SlideAnimation(this)); This can probably have an early return if (!is_highlighted) (i.e. if not highlighted, no point creating the animation at all) https://codereview.chromium.org/2335513002/diff/200001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:120: Slider::AnimationEnded(animation); In AnimationProgressed(), you call back into Slider only if animation is not the highlight animation. Should you do the same here? (i.e. callback only for non-highlight animation?) https://codereview.chromium.org/2335513002/diff/200001/ui/views/controls/md_s... File ui/views/controls/md_slider.h (right): https://codereview.chromium.org/2335513002/diff/200001/ui/views/controls/md_s... ui/views/controls/md_slider.h:49: std::unique_ptr<gfx::SlideAnimation> highlight_animation_; DISALLOW_COPY_AND_ASSIGN
https://codereview.chromium.org/2335513002/diff/180001/ui/views/controls/slid... File ui/views/controls/slider.cc (right): https://codereview.chromium.org/2335513002/diff/180001/ui/views/controls/slid... ui/views/controls/slider.cc:242: } On 2016/09/29 19:09:39, yiyix wrote: > On 2016/09/24 13:59:18, varkha wrote: > > nit: I think it would be easier to read this class if you order the methods as > > in the .h file. Sorry we haven't noticed this before. > > Done. In the future it would make it easier as a reviewer to do this in a follow-up CL or as the very last patch after you have all your lgtms. https://codereview.chromium.org/2335513002/diff/200001/ui/views/controls/slid... File ui/views/controls/slider.cc (right): https://codereview.chromium.org/2335513002/diff/200001/ui/views/controls/slid... ui/views/controls/slider.cc:97: SchedulePaint(); nit: You should be consistent with what you do here and in AnimationEnded(). i.e. don't rely on the implementation detail that MdSlider::AnimationProgressed() doesn't call this.
https://codereview.chromium.org/2335513002/diff/200001/ui/views/controls/md_s... File ui/views/controls/md_slider.cc (right): https://codereview.chromium.org/2335513002/diff/200001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:20: const U8CPU kHighlightColorAlpha = 0x4D; On 2016/09/30 02:53:36, sadrul wrote: > I don't see U8CPU type used anywhere else in chrome. Use uint8_t instead. Done. https://codereview.chromium.org/2335513002/diff/200001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:100: highlight_animation_.reset(new gfx::SlideAnimation(this)); On 2016/09/30 02:53:36, sadrul wrote: > This can probably have an early return if (!is_highlighted) (i.e. if not > highlighted, no point creating the animation at all) That's true. updated. https://codereview.chromium.org/2335513002/diff/200001/ui/views/controls/md_s... ui/views/controls/md_slider.cc:120: Slider::AnimationEnded(animation); On 2016/09/30 02:53:36, sadrul wrote: > In AnimationProgressed(), you call back into Slider only if animation is not the > highlight animation. Should you do the same here? (i.e. callback only for > non-highlight animation?) Done. https://codereview.chromium.org/2335513002/diff/200001/ui/views/controls/md_s... File ui/views/controls/md_slider.h (right): https://codereview.chromium.org/2335513002/diff/200001/ui/views/controls/md_s... ui/views/controls/md_slider.h:49: std::unique_ptr<gfx::SlideAnimation> highlight_animation_; On 2016/09/30 02:53:36, sadrul wrote: > DISALLOW_COPY_AND_ASSIGN Thanks, i also added it to the NonMdSlider class. https://codereview.chromium.org/2335513002/diff/200001/ui/views/controls/slid... File ui/views/controls/slider.cc (right): https://codereview.chromium.org/2335513002/diff/200001/ui/views/controls/slid... ui/views/controls/slider.cc:97: SchedulePaint(); On 2016/09/30 21:56:18, bruthig wrote: > nit: You should be consistent with what you do here and in AnimationEnded(). > i.e. don't rely on the implementation detail that > MdSlider::AnimationProgressed() doesn't call this. Done.
lgtm
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was checked by yiyix@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org, varkha@chromium.org, bruthig@chromium.org Link to the patchset: https://codereview.chromium.org/2335513002/#ps240001 (title: "add disallow_copy_and_assign to testslider")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Adding ripple effect for clicks on MD slider The MD slider is currently hiding behind ash-md flag. When the flag is set to experimental, a highlight effect will be showing for clicking or touching on MD slider. TEST=MANUAL - test if the highlight shows when users click, click and drag or touch the md slider. BUG=631775 ========== to ========== Adding ripple effect for clicks on MD slider The MD slider is currently hiding behind ash-md flag. When the flag is set to experimental, a highlight effect will be showing for clicking or touching on MD slider. TEST=MANUAL - test if the highlight shows when users click, click and drag or touch the md slider. BUG=631775 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Adding ripple effect for clicks on MD slider The MD slider is currently hiding behind ash-md flag. When the flag is set to experimental, a highlight effect will be showing for clicking or touching on MD slider. TEST=MANUAL - test if the highlight shows when users click, click and drag or touch the md slider. BUG=631775 ========== to ========== Adding ripple effect for clicks on MD slider The MD slider is currently hiding behind ash-md flag. When the flag is set to experimental, a highlight effect will be showing for clicking or touching on MD slider. TEST=MANUAL - test if the highlight shows when users click, click and drag or touch the md slider. BUG=631775 Committed: https://crrev.com/df1235e312d117abcc12d062de6d74d314080349 Cr-Commit-Position: refs/heads/master@{#422814} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/df1235e312d117abcc12d062de6d74d314080349 Cr-Commit-Position: refs/heads/master@{#422814} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
