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