On Mon, 31 Jul 2023 18:07:52 GMT, Michael Strauß <mstra...@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
