On Fri, 24 Apr 2020 00:58:30 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
> Mostly refactoring in preparation of the upcoming fixes. The changes might
> look like a lot, but it's mostly rearranging
> of methods. Summery of changes:
> ### Animation
> * Added `isNearZero` and `areNearEqual` methods that deal with `EPSILON`
> checks.
> * Added `isStopped`, `isRunning` and `isPaused` convenience methods.
> * Added `runHandler` method to deal with running the handler.
> * Moved methods to be grouped closer to where they are used rather than by
> visibility.
> * Removed the static import for `TickCalculation`.
> * Various small subjective readability changes.
> * Behavioral changes: switching `autoReverse` and `onFinished` properties
> from "Simple" to "Base" properties; and lazily
> initializing the `cuePoints` map.
>
> ### Clip Envelopes
> * Added `MultiLoopClipEnvelope` as an intermediate parent for infinite and
> finite clip envelopes.
> * Rearranged methods order to be consistent.
> * Replaced the `checkBounds` method with a new overload of `Utils.clamp`.
> * Renamed `pos` to `cyclePos`.
> * Extracted common methods: `changedDirection` and `ticksRateChange`
> * Added internal documentation.
>
> Also corrected a typo in `TicksCalculation` and added an explanation for an
> animation test.
Overall looks good to me, Added few minor comments.
modules/javafx.graphics/src/main/java/com/sun/scenario/animation/shared/MultiLoopClipEnvelope.java
line 48:
> 47:
> 48: protected boolean autoReverse() {
> 49: return autoReverse;
I would suggest the name to be `isAutoReverese`
modules/javafx.graphics/src/main/java/com/sun/scenario/animation/shared/ClipEnvelope.java
line 46:
> 45: */
> 46: public abstract class ClipEnvelope {
> 47:
I think the removal of line 46 was unintended change.
modules/javafx.graphics/src/main/java/javafx/animation/Animation.java line 142:
> 141: }
> 142:
> 143: /*
Both these methods seem like they belong to Utils.java. If moved to Utils.java
then the all the calls
`(Math.abs(currentRate - rate) < EPSILON)` can be changed to use these methods
from Utils.java. If we move these
methods then, Utils.java also needs to declare `static final double EPSILON =
1e-12;` and the EPSILON here can be
initialized as `private static final double EPSILON = Utils.EPSILON;`
modules/javafx.graphics/src/main/java/com/sun/scenario/animation/shared/MultiLoopClipEnvelope.java
line 61:
> 60:
> 61: protected boolean changedDirection(double newRate) {
> 62: return newRate * rate < 0;
I would recommend to name as `isDirectionChanged`
modules/javafx.graphics/src/main/java/com/sun/scenario/animation/shared/SingleLoopClipEnvelope.java
line 102:
> 101: long ticksChange = Math.round(currentTick * currentRate);
> 102: ticks = Utils.clamp(0, deltaTicks + ticksChange, cycleTicks);
> 103: AnimationAccessor.getDefault().playTo(animation, ticks,
> cycleTicks);
This could remain unchanged. The `ticksChange` value is not really used
elsewhere here.
modules/javafx.graphics/src/main/java/com/sun/scenario/animation/shared/MultiLoopClipEnvelope.java
line 59:
> 58: return Math.round((ticks - deltaTicks) * Math.abs(newRate /
> rate));
> 59: }
> 60:
This is similar to `ClipEnvelope.ticksRateChange()` with a minor difference.
Can this be removed from here and
`ClipEnvelope.ticksRateChange()` be reused ?
-------------
Changes requested by arapte (Reviewer).
PR: https://git.openjdk.java.net/jfx/pull/196
