RFR: 8311895: CSS Transitions [v2]

[John Hendrikx]

On Mon, 31 Jul 2023 18:07:52 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionDefinitionCssMetaData.java line 63:
>> 
>>> 61:     private static final Duration[] DURATION_ZERO = new Duration[] { Duration.ZERO };
>>> 62: 
>>> 63:     private static final Interpolator[] INTERPOLATOR_EASE = new Interpolator[] { InterpolatorConverter.CSS_EASE };
>> 
>> Careful, the array contents could still be modified
>
> That's true. However, `CssParser` is built around array-based sequences (see `StringConverter.SequenceConverter`), and changing these arrays to lists would seem out of place for this part of the codebase. What do you think?

It depends on how much you want guarantees that they're not changed.  I suppose it's not that big of an issue.  Changing to `List` is likely impossible (I think that part of the API is public in places), only other option than is to recreate the arrays if you want 100% certainty :)

>> modules/javafx.graphics/src/main/java/com/sun/javafx/css/TransitionTimer.java line 126:
>> 
>>> 124:      * @param forceStop if {@code true}, the timer is stopped unconditionally
>>> 125:      * @return {@code true} if the timer was cancelled or {@code timer} is {@code null},
>>> 126:      *         {@code false} otherwise
>> 
>> minor:
>> Suggestion:
>> 
>>      * @return {@code true} if the timer was cancelled, or {@code timer} is {@code null},
>>      *         otherwise {@code false}
>
> I don't see any real difference here, but then again I'm not a native speaker. I'll defer to public opinion.

I'm not a native either, but it flows a bit better IMHO.  I checked `String` class, they do it like that there as well.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1280672359
PR Review Comment: https://git.openjdk.org/jfx/pull/870#discussion_r1280673889