Use Event to render feedback animations.

[chromoting] Use Event to render feedback animations.

This change moves FeedbackAnimator into a standalone class, with a startAnimation static function to register a SelfRemovableParameterRunnable into onPaint event of DesktopView. After animation finishes, the SelfRemovableParameterRunnable will be removed from onPaint event automatically.

This change is part of Chromoting Android Client Refactor work. A design doc can
be found at https://goo.gl/MA6zjx.

BUG=615277
Committed: https://crrev.com/06d70696de0b8fac307927064bd36be2d486fe2a
Cr-Commit-Position: refs/heads/master@{#397619}

[Hzj_jie, 2016-06-01 03:23:19]

Patchset #2 (id:20001) has been deleted

[Hzj_jie, 2016-06-01 03:23:23]

Patchset #1 (id:1) has been deleted

[Hzj_jie, 2016-06-01 03:47:50]

Description was changed from

==========
Use Event to render feedback animations.

This change is part of Android Client Refactor work. A design doc can be found
at https://goo.gl/MA6zjx.

BUG=615277
==========

to

==========
Use Event to render feedback animations.

This change is part of Android Client Refactor work. A design doc can be found
at https://goo.gl/MA6zjx.

BUG=615277
==========

[Hzj_jie, 2016-06-01 03:47:50]

zijiehe@chromium.org changed reviewers:
+ lambroslambrou@chromium.org

[Lambros, 2016-06-01 23:51:37]

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right):

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:61: private
final Event.Raisable<EventParameters.Paint> mOnPaint = new Event.Raisable<>();
mOnPaint is a bit misleading. Maybe mOnPaintAnimation ?

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/Event.java (right):

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/Event.java:70: private static
final class SelfRemovableParameterRunnable<ParamT>
Unrelated cleanup - move to a separate CL ?

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/EventParameters.java
(right):

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/EventParameters.java:11:
public final class EventParameters {
These various EventParameters.* classes will not be
related to each other, so they should be in separate
files.

Perhaps use a Java package, or a common name
prefix such as
EventParametersFoo, EventParametersBar.
Or have EventParametersFoo be an inner class of
Foo.

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/EventParameters.java:18:
public Paint(Point cursorPosition, Canvas canvas, float scale) {
PaintAnimation?

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/FeedbackAnimator.java
(right):

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/FeedbackAnimator.java:14: /**
Helper class for displaying the press feedback animations. */
Is this class now single-threaded?

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/FeedbackAnimator.java:36: /**
Begins a new animation sequence at position (|x|, |y|). */
position |pos|.

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/FeedbackAnimator.java:40:
Preconditions.notNull(view);
Remove this?
If view == null, then view.onPaint() below will already throw a NPE, so this
check seems to add no value.

Checking pos != null is perhaps more useful (because 'pos' isn't immediately
dereferenced here). Though I'd prefer to avoid going overboard with
these null-checks everywhere.

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/FeedbackAnimator.java:78: //
Animation grows from 0 to |size|, and goes from fully opaque to transparent for
a
Blank line above comment.

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/TapGestureDetector.java
(right):

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/TapGestureDetector.java:1: //
Copyright 2013 The Chromium Authors. All rights reserved.
These bug-fixes and cleanups seem unrelated to this CL. Move them to another CL?

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/TapGestureDetector.java:107:
// Cause a long-press notification to be triggered after the timeout.
Blank line before comment.

[Lambros, 2016-06-02 00:01:04]

Just a general comment on CL descriptions:
Since we're not working directly on Chrome, it might be
useful to mention "chromoting" or "remoting" somewhere
in your CL descriptions.
In my own CLs, I've done this by adding [remoting android] or [remoting host] at
the beginning of the summary line. Other people have done similar things as
well.

[Hzj_jie, 2016-06-02 00:36:40]

Description was changed from

==========
Use Event to render feedback animations.

This change is part of Android Client Refactor work. A design doc can be found
at https://goo.gl/MA6zjx.

BUG=615277
==========

to

==========
[chromoting] Use Event to render feedback animations.

This change is part of Chromoting Android Client Refactor work. A design doc can
be found at https://goo.gl/MA6zjx.

BUG=615277
==========

[Hzj_jie, 2016-06-02 00:37:07]

On 2016/06/02 00:01:04, Lambros wrote:
> Just a general comment on CL descriptions:
> Since we're not working directly on Chrome, it might be
> useful to mention "chromoting" or "remoting" somewhere
> in your CL descriptions.
> In my own CLs, I've done this by adding [remoting android] or [remoting host]
at
> the beginning of the summary line. Other people have done similar things as
> well.

Thank you, updated. I am looking at your comments.

[Hzj_jie, 2016-06-02 01:06:30]

Description was changed from

==========
[chromoting] Use Event to render feedback animations.

This change is part of Chromoting Android Client Refactor work. A design doc can
be found at https://goo.gl/MA6zjx.

BUG=615277
==========

to

==========
[chromoting] Use Event to render feedback animations.

This change moves FeedbackAnimator into a standalone class, with a
startAnimation static function to register a SelfRemovableParameterRunnable into
onPaint event of DesktopView. After animation finishes, the
SelfRemovableParameterRunnable will be removed from onPaint event automatically.

This change is part of Chromoting Android Client Refactor work. A design doc can
be found at https://goo.gl/MA6zjx.

BUG=615277
==========

[Hzj_jie, 2016-06-02 02:21:37]

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right):

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:61: private
final Event.Raisable<EventParameters.Paint> mOnPaint = new Event.Raisable<>();
On 2016/06/01 23:51:36, Lambros wrote:
> mOnPaint is a bit misleading. Maybe mOnPaintAnimation ?

From my opinion, the event name represents 'happens at a certain time' instead
of 'happens for something'. So OnPaint expresses that this event will happen
during paint, but not for a specific usage. And in one of the coming changes, I
will also let AnimationJob listen to this event.

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/Event.java (right):

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/Event.java:70: private static
final class SelfRemovableParameterRunnable<ParamT>
On 2016/06/01 23:51:36, Lambros wrote:
> Unrelated cleanup - move to a separate CL ?

Sure.

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/EventParameters.java
(right):

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/EventParameters.java:11:
public final class EventParameters {
On 2016/06/01 23:51:36, Lambros wrote:
> These various EventParameters.* classes will not be
> related to each other, so they should be in separate
> files.
> 
> Perhaps use a Java package, or a common name
> prefix such as
> EventParametersFoo, EventParametersBar.
> Or have EventParametersFoo be an inner class of
> Foo.

During prototyping before, I found lots of events can share same or similar
event parameters, so a parameter is not directly binding to its consumers. But
using a new namespace in java is not so friendly, it does not provide 'searching
from current namespace' function. So I am OK to move them into individual files.

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/EventParameters.java:18:
public Paint(Point cursorPosition, Canvas canvas, float scale) {
On 2016/06/01 23:51:36, Lambros wrote:
> PaintAnimation?

Same reason as mOnPaint field and onPaint() function in DesktopView.

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/FeedbackAnimator.java
(right):

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/FeedbackAnimator.java:14: /**
Helper class for displaying the press feedback animations. */
On 2016/06/01 23:51:36, Lambros wrote:
> Is this class now single-threaded?

Yes, this class is not necessarily thread-safe. Since the onPaint event will
only be called by DesktopView.paint function.

This class is still thread-safe, though not necessary. All fields in this class
are final, if two run function calls concurrently, the instance will draw two
same circles on two (or one, depends on the caller) canvases.

I removed the comment about the thread-safety, simply because it's not a
must-have anymore.

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/FeedbackAnimator.java:36: /**
Begins a new animation sequence at position (|x|, |y|). */
On 2016/06/01 23:51:36, Lambros wrote:
> position |pos|.

Done.

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/FeedbackAnimator.java:40:
Preconditions.notNull(view);
On 2016/06/01 23:51:36, Lambros wrote:
> Remove this?
> If view == null, then view.onPaint() below will already throw a NPE, so this
> check seems to add no value.
> 
> Checking pos != null is perhaps more useful (because 'pos' isn't immediately
> dereferenced here). Though I'd prefer to avoid going overboard with
> these null-checks everywhere.

My fault, I recently wrote more C++ code than Java. :)

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/FeedbackAnimator.java:78: //
Animation grows from 0 to |size|, and goes from fully opaque to transparent for
a
On 2016/06/01 23:51:36, Lambros wrote:
> Blank line above comment.
> 

Done.

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/TapGestureDetector.java
(right):

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/TapGestureDetector.java:1: //
Copyright 2013 The Chromium Authors. All rights reserved.
On 2016/06/01 23:51:36, Lambros wrote:
> These bug-fixes and cleanups seem unrelated to this CL. Move them to another
CL?

Sure.

https://codereview.chromium.org/2023133002/diff/40001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/TapGestureDetector.java:107:
// Cause a long-press notification to be triggered after the timeout.
On 2016/06/01 23:51:37, Lambros wrote:
> Blank line before comment.

Done.

[Lambros, 2016-06-02 18:39:21]

lgtm

[Hzj_jie, 2016-06-02 21:14:32]

The CQ bit was checked by zijiehe@chromium.org

[commit-bot: I haz the power, 2016-06-02 21:15:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2023133002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2023133002/60001

[commit-bot: I haz the power, 2016-06-03 03:44:13]

Description was changed from

==========
[chromoting] Use Event to render feedback animations.

This change moves FeedbackAnimator into a standalone class, with a
startAnimation static function to register a SelfRemovableParameterRunnable into
onPaint event of DesktopView. After animation finishes, the
SelfRemovableParameterRunnable will be removed from onPaint event automatically.

This change is part of Chromoting Android Client Refactor work. A design doc can
be found at https://goo.gl/MA6zjx.

BUG=615277
==========

to

==========
[chromoting] Use Event to render feedback animations.

This change moves FeedbackAnimator into a standalone class, with a
startAnimation static function to register a SelfRemovableParameterRunnable into
onPaint event of DesktopView. After animation finishes, the
SelfRemovableParameterRunnable will be removed from onPaint event automatically.

This change is part of Chromoting Android Client Refactor work. A design doc can
be found at https://goo.gl/MA6zjx.

BUG=615277
==========

[commit-bot: I haz the power, 2016-06-03 03:44:14]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2016-06-03 03:46:16]

Description was changed from

==========
[chromoting] Use Event to render feedback animations.

This change moves FeedbackAnimator into a standalone class, with a
startAnimation static function to register a SelfRemovableParameterRunnable into
onPaint event of DesktopView. After animation finishes, the
SelfRemovableParameterRunnable will be removed from onPaint event automatically.

This change is part of Chromoting Android Client Refactor work. A design doc can
be found at https://goo.gl/MA6zjx.

BUG=615277
==========

to

==========
[chromoting] Use Event to render feedback animations.

This change moves FeedbackAnimator into a standalone class, with a
startAnimation static function to register a SelfRemovableParameterRunnable into
onPaint event of DesktopView. After animation finishes, the
SelfRemovableParameterRunnable will be removed from onPaint event automatically.

This change is part of Chromoting Android Client Refactor work. A design doc can
be found at https://goo.gl/MA6zjx.

BUG=615277

Committed: https://crrev.com/06d70696de0b8fac307927064bd36be2d486fe2a
Cr-Commit-Position: refs/heads/master@{#397619}
==========

[commit-bot: I haz the power, 2016-06-03 03:46:17]

Patchset 2 (id:??) landed as
https://crrev.com/06d70696de0b8fac307927064bd36be2d486fe2a
Cr-Commit-Position: refs/heads/master@{#397619}