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.
The code changes look fine except for one thing I noticed (see below), which
might lead to an unintended side effect.
Also, I had one suggestion for a javadoc comment block.
modules/javafx.graphics/src/main/java/javafx/animation/Animation.java line 332:
> 331: }
> 332: if (isNearZero(newRate)) {
> 333: if (isRunning()) {
The code from here to the end of the method used to be in an `else` block of
the `if (isRunningEmbedded())` test. It
will now be run even if that test is true. Was this intended, and if so, why?
modules/javafx.graphics/src/main/java/javafx/animation/Animation.java line 402:
> 401: */
> 402: private void doSetCurrentRate(double value) {
> 403: if (currentRate != null || !areNearEqual(value,
> DEFAULT_CURRENT_RATE)) {
The above javadoc comment block appears after the javadoc comments for the
private `currentRate` property variable and
before the public get/property methods for `currentRate`. This seems fragile
(although the API docs pick up the right
one), so I recommend making this an ordinary block comment (i.e., `/*` rather
than `/**`). Either that or move it below
the get/property methods (and add `@param value`).
-------------
PR: https://git.openjdk.java.net/jfx/pull/196
