Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2265053005:
[Remoting Android] Use Material Design's Ripple Expansion Function for Touch Feedback (Closed)
Description[Remoting Android] Use Material Design's Ripple Expansion Function for Touch Feedback
Currently the radius of the touch feedback animation is linear to time, making
it spend the same amount of time when showing the large circle as when showing
the smaller circle.
This CL changes the radius-time function to a decelerating function as the
material design spec suggested. This will make the animation looks more rapid
given the same duration.
This CL also increases the animation duration to 300ms, which is a balanced
value for showing both simple touch and press-and-hold feedback.
BUG=638317
Committed: https://crrev.com/cdc3c1395e8cc0795cf70d9fe552e12a1036285a
Cr-Commit-Position: refs/heads/master@{#413666}
Patch Set 1 #Patch Set 2 : Add comments #
Total comments: 6
Patch Set 3 : Reviewer's Feedback #
Total comments: 8
Patch Set 4 : Reviewer's Feedback #
Total comments: 2
Patch Set 5 : Reviewer's Feedback #Messages
Total messages: 25 (14 generated)
Description was changed from ========== [Remoting Android] Use Material Design Ripple Expansion for Touch Feedback Currently the radius of the touch feedback animation is linear to time, making it spend the same amount of time when showing the large circle as when showing the smaller circle. This CL changes the radius-time function to a decelerating function as the material design spec suggested. This will make the animation looks more rapid given the same duration. This CL also increases the animation duration to 300ms, which is a balanced value for showing both simple touch and press-and-hold feedback. BUG=638317 ========== to ========== [Remoting Android] Use Material Design's Ripple Expansion Function for Touch Feedback Currently the radius of the touch feedback animation is linear to time, making it spend the same amount of time when showing the large circle as when showing the smaller circle. This CL changes the radius-time function to a decelerating function as the material design spec suggested. This will make the animation looks more rapid given the same duration. This CL also increases the animation duration to 300ms, which is a balanced value for showing both simple touch and press-and-hold feedback. BUG=638317 ==========
yuweih@chromium.org changed reviewers: + joedow@chromium.org
ptal
https://codereview.chromium.org/2265053005/diff/20001/remoting/client/gl_curs... File remoting/client/gl_cursor_feedback.cc (right): https://codereview.chromium.org/2265053005/diff/20001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:20: const float kExpansionBase = 400.f; Perhaps there isn't a better name, but kExpansionBase isn't very descriptive until you read further down. Perhaps a comment for the related consts would help? https://codereview.chromium.org/2265053005/diff/20001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:21: const float kExpansionNormalization = (1.f - pow(kExpansionBase, -1)); I think this might be clearer to create an anonymous function which is given a progress value and returns the expansion coefficient. You could then use that to populate this value and the one on line 63. https://codereview.chromium.org/2265053005/diff/20001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:61: // Decelerating Expansion: (1 - 400^(-t)) / (1 - 400^(-1)) Can you update the comment a bit here? The comment above has hard-coded values (which would be out of date if someone changes the constant on line 20) I think it is better to document the intention instead of the effect. Are there min/max requirements got |progress|. Basically document the stuff that someone in the future (perhaps you) would need to know.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
ptal https://codereview.chromium.org/2265053005/diff/20001/remoting/client/gl_curs... File remoting/client/gl_cursor_feedback.cc (right): https://codereview.chromium.org/2265053005/diff/20001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:20: const float kExpansionBase = 400.f; On 2016/08/22 22:22:21, joedow wrote: > Perhaps there isn't a better name, but kExpansionBase isn't very descriptive > until you read further down. Perhaps a comment for the related consts would > help? Not sure what is the best description for this constant. It's basically just a part of the magical equation... Just moved the const inside the function to make it look clearer. https://codereview.chromium.org/2265053005/diff/20001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:21: const float kExpansionNormalization = (1.f - pow(kExpansionBase, -1)); On 2016/08/22 22:22:22, joedow wrote: > I think this might be clearer to create an anonymous function which is given a > progress value and returns the expansion coefficient. You could then use that > to populate this value and the one on line 63. Done. BTW nice timestamp, 22 22:22:22 :P https://codereview.chromium.org/2265053005/diff/20001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:61: // Decelerating Expansion: (1 - 400^(-t)) / (1 - 400^(-1)) On 2016/08/22 22:22:21, joedow wrote: > Can you update the comment a bit here? The comment above has hard-coded values > (which would be out of date if someone changes the constant on line 20) Just removed the comment here. It doesn't seem useful anyway since the code is self-explaining. > I think it is better to document the intention instead of the effect. Are there > min/max requirements got |progress|. Basically document the stuff that someone > in the future (perhaps you) would need to know. Done. (Above)
The CQ bit was checked by yuweih@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.
https://codereview.chromium.org/2265053005/diff/80001/remoting/client/gl_curs... File remoting/client/gl_cursor_feedback.cc (right): https://codereview.chromium.org/2265053005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:22: // progress: [0, 1], indicating the progress of the animation. params are referenced like this |progress|. It would be good to have a sentence above this stating what the function is used for. https://codereview.chromium.org/2265053005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:25: float GetExpansionCoeff(float progress) { Coeff should be spelled out (GetExpansionCoefficient) https://codereview.chromium.org/2265053005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:30: static const float expansion_base = 400.f; const values should use the same format as above (i.e. kExpansionBase) https://codereview.chromium.org/2265053005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:40: float GetAlphaCoeff(float progress) { I don't think this needs to be a separate function, it doesn't really add anything.
ptal https://codereview.chromium.org/2265053005/diff/80001/remoting/client/gl_curs... File remoting/client/gl_cursor_feedback.cc (right): https://codereview.chromium.org/2265053005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:22: // progress: [0, 1], indicating the progress of the animation. On 2016/08/23 01:27:20, joedow wrote: > params are referenced like this |progress|. It would be good to have a sentence > above this stating what the function is used for. Done. https://codereview.chromium.org/2265053005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:25: float GetExpansionCoeff(float progress) { On 2016/08/23 01:27:20, joedow wrote: > Coeff should be spelled out (GetExpansionCoefficient) Done. https://codereview.chromium.org/2265053005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:30: static const float expansion_base = 400.f; On 2016/08/23 01:27:20, joedow wrote: > const values should use the same format as above (i.e. kExpansionBase) Done. https://codereview.chromium.org/2265053005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:40: float GetAlphaCoeff(float progress) { On 2016/08/23 01:27:20, joedow wrote: > I don't think this needs to be a separate function, it doesn't really add > anything. Done.
lgtm https://codereview.chromium.org/2265053005/diff/100001/remoting/client/gl_cur... File remoting/client/gl_cursor_feedback.cc (right): https://codereview.chromium.org/2265053005/diff/100001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback.cc:86: layer_->Draw(1.f - progress); // linear fade-out. nit: move comment above the code.
The CQ bit was checked by yuweih@chromium.org
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 yuweih@chromium.org
https://codereview.chromium.org/2265053005/diff/100001/remoting/client/gl_cur... File remoting/client/gl_cursor_feedback.cc (right): https://codereview.chromium.org/2265053005/diff/100001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback.cc:86: layer_->Draw(1.f - progress); // linear fade-out. On 2016/08/23 01:58:42, joedow wrote: > nit: move comment above the code. Done.
The CQ bit was checked by yuweih@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from joedow@chromium.org Link to the patchset: https://codereview.chromium.org/2265053005/#ps120001 (title: "Reviewer's Feedback")
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 ========== [Remoting Android] Use Material Design's Ripple Expansion Function for Touch Feedback Currently the radius of the touch feedback animation is linear to time, making it spend the same amount of time when showing the large circle as when showing the smaller circle. This CL changes the radius-time function to a decelerating function as the material design spec suggested. This will make the animation looks more rapid given the same duration. This CL also increases the animation duration to 300ms, which is a balanced value for showing both simple touch and press-and-hold feedback. BUG=638317 ========== to ========== [Remoting Android] Use Material Design's Ripple Expansion Function for Touch Feedback Currently the radius of the touch feedback animation is linear to time, making it spend the same amount of time when showing the large circle as when showing the smaller circle. This CL changes the radius-time function to a decelerating function as the material design spec suggested. This will make the animation looks more rapid given the same duration. This CL also increases the animation duration to 300ms, which is a balanced value for showing both simple touch and press-and-hold feedback. BUG=638317 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Remoting Android] Use Material Design's Ripple Expansion Function for Touch Feedback Currently the radius of the touch feedback animation is linear to time, making it spend the same amount of time when showing the large circle as when showing the smaller circle. This CL changes the radius-time function to a decelerating function as the material design spec suggested. This will make the animation looks more rapid given the same duration. This CL also increases the animation duration to 300ms, which is a balanced value for showing both simple touch and press-and-hold feedback. BUG=638317 ========== to ========== [Remoting Android] Use Material Design's Ripple Expansion Function for Touch Feedback Currently the radius of the touch feedback animation is linear to time, making it spend the same amount of time when showing the large circle as when showing the smaller circle. This CL changes the radius-time function to a decelerating function as the material design spec suggested. This will make the animation looks more rapid given the same duration. This CL also increases the animation duration to 300ms, which is a balanced value for showing both simple touch and press-and-hold feedback. BUG=638317 Committed: https://crrev.com/cdc3c1395e8cc0795cf70d9fe552e12a1036285a Cr-Commit-Position: refs/heads/master@{#413666} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/cdc3c1395e8cc0795cf70d9fe552e12a1036285a Cr-Commit-Position: refs/heads/master@{#413666} |
