On Fri, 2 Aug 2024 15:06:29 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> Michael Strauß has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - fix since tag
>> - adjust table styling
>
> modules/javafx.base/src/main/java/com/sun/javafx/UnmodifiableArrayList.java
> line 78:
>
>> 76:
>> 77: int index = 0;
>> 78: int numNonNullValues = 0;
>
> It seems to me, just one of these variables is sufficient. They both start
> at `0` and both are incremented at the same time.
Yes, I've removed one of the variables.
> modules/javafx.base/src/main/java/com/sun/javafx/UnmodifiableArrayList.java
> line 81:
>
>> 79:
>> 80: @SuppressWarnings("unchecked")
>> 81: T[] newValues = (T[])new Object[list.size()];
>
> With many `null`s this array may be too big. If that's intended, then ignore
> this comment.
That may be the case, but I think that's preferrable to requiring two
iterations of the list (one to figure out how many non-null values it contains,
the second to copy over the values). In practice, there shouldn't be any nulls
in most cases, as I don't know why someone would construct a `Background` or
`Border` with many nulls in there.
> modules/javafx.base/src/main/java/com/sun/javafx/UnmodifiableArrayList.java
> line 123:
>
>> 121: if (elements[i] != null) {
>> 122: newValues[j++] = elements[i];
>> 123: ++numNonNullValues;
>
> Both `j` and `numNonNullValues` are always the same. Could eliminate one.
You're right.
> modules/javafx.graphics/src/main/java/com/sun/javafx/scene/paint/PaintUtils.java
> line 102:
>
>> 100: source.getEndX(), source.getEndY(),
>> 101: source.isProportional(),
>> 102: source.getCycleMethod(),
>
> Should these be copied from the source? When the gradient is solid, they
> could be set to constants.
We can't interpolate between relative and absolute gradients (the interpolate
method lacks the context necessary to convert between the two), so we need to
copy these from source instead of always choosing one or the other.
> modules/javafx.graphics/src/main/java/com/sun/javafx/util/Utils.java line 183:
>
>> 181: * <li>If the intermediate list is shallow-equal to the first
>> list passed into the method (i.e. its
>> 182: * elements are references to the same objects), the
>> existing list is returned.
>> 183: * <li>If a new list is returned, it is unmodifiable.
>
> Well, it's documented, but I'd rather see this return an unmodifiable list in
> all cases (and not one of the input lists if it was determined to be
> modifiable). Perhaps ensure this is compatible with `List.copyOf` so that
> copies are only made when needed.
In practice, any list that is passed into this method is already unmodifiable.
This isn't a general-purpose method, it's tailored towards its usage for
`Border` and `Background` interpolation, and the optimization scheme that is
used in these classes.
> modules/javafx.graphics/src/main/java/com/sun/javafx/util/Utils.java line 197:
>
>> 195: if (secondList.isEmpty()) {
>> 196: return firstList.isEmpty() ? firstList : secondList;
>> 197: }
>
> This logic looks odd. If the second list is empty, we return the first list
> **if** it is empty (and so empty is returned), otherwise the second list
> (which is also empty). So, this is equivalent?
> Suggestion:
>
> if (secondList.isEmpty()) {
> return List.of(); // suggested here, as the two input lists are
> not guaranteed to be immutable
> }
The point of this excercise is to preferrably return existing instances, even
when they are empty. `Border` and `Background` can contain quite a lot of
objects, and often only one component is changed in a transition, while all of
the rest is left unchanged. We want to elide the allocation of new objects if
possible during transitions, which is why most classes check whether the result
of an interpolation happens to be the same instance as either the start value
or the end value.
> modules/javafx.graphics/src/main/java/javafx/css/StyleConverter.java line 268:
>
>> 266: * @since 24
>> 267: */
>> 268: public Map<CssMetaData<? extends Styleable, ?>, Object>
>> convertBack(T value) {
>
> I know the name `convert` wasn't all that great (it probably should have been
> `construct`, `parse` or `deserialize`), but I think `convertBack` is not that
> great either. Have you considered `toMetaData`, `revert`, `serialize` or
> `deconstruct`?
I considered `deconstruct` and `decompose`, but I wanted to stress that it's
the inverse of `convert`.
> modules/javafx.graphics/src/main/java/javafx/geometry/Insets.java line 117:
>
>> 115: *
>> 116: * @throws NullPointerException {@inheritDoc}
>> 117: * @since 23
>
> Should this be `24`?
Yes.
> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 9107:
>
>> 9105: // Make a copy of the list, because completing the timers
>> causes them to be removed
>> 9106: // from the list, which would result in a
>> ConcurrentModificationException.
>> 9107: for (TransitionTimer timer :
>> Map.copyOf(transitionTimers).values()) {
>
> If you only need a copy of the values, copying the whole map is a bit
> overkill. How about `List.copyOf(transitionTimers.values())` ? Lists are
> also much faster than maps for this purpose.
Good idea.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1702415018
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1702415036
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1702415039
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1702415068
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1702415084
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1702415079
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1702415105
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1702415114
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1702415138
