RFR: 8332895: Support interpolation for backgrounds and borders [v3]

Sat Aug 3 00:17:39 UTC 2024

On Fri, 2 Aug 2024 15:06:29 GMT, John Hendrikx <jhendrikx at 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