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

Fri Aug 2 01:28:42 UTC 2024

On Thu, 1 Aug 2024 21:40:54 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> Michael Strauß has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   fixed a bug
>
> modules/javafx.graphics/src/main/java/javafx/animation/Interpolatable.java line 37:
> 
>> 35:  *     <caption><b>Interpolation types</b></caption>
>> 36:  *     <tbody>
>> 37:  *         <tr><td><a id="default" style="white-space: nowrap">default</a></td>
> 
> very minor: is it possible to top-align the first column content instead of center?
> 
> something like
> `<td style="vertical-align: top;">`

Sure.

> modules/javafx.graphics/src/main/java/javafx/animation/Interpolatable.java line 57:
> 
>> 55:  *             </td>
>> 56:  *         </tr>
>> 57:  *         <tr><td style="white-space: nowrap">(see prose)</td>
> 
> "see prose" - could you be more specific?  maybe give an example?

An example would be `LinearGradient.stops`, but this is rather arbitrary. This table is not exhaustive, and the last line is meant to mention that it's not exhaustive. I'm not sure how an example helps, as every example will probably be different.

> modules/javafx.graphics/src/main/java/javafx/css/StyleableDoubleProperty.java line 157:
> 
>> 155:         @Override
>> 156:         public boolean updateReversingAdjustedStartValue(TransitionMediator existingMediator) {
>> 157:             var mediator = (TransitionMediatorImpl)existingMediator;
> 
> minor: should we use an instanceof pattern here, or do we want to throw ClassCastException?
> (here and elsewhere)
> 
> example:
> 
> if(existingMediator instanceof TransitionMediatorImpl mediator) {
> ...
> }

A reversing mediator must be the same type, otherwise something is seriously broken. I prefer a `ClassCastException` to highlight that this is very unexpected.

> modules/javafx.graphics/src/main/java/javafx/css/StyleableObjectProperty.java line 124:
> 
>> 122:             // value of the existing transition. This scenario can sometimes happen when a CSS value
>> 123:             // is redundantly applied, which would cause unexpected animations if we allowed the new
>> 124:             // transition to interrupt the existing transition.
> 
> TODO
> is there a unit test for this scenario?

`StyleableProperty_transition_Test.testRedundantTransitionIsDiscarded`

> modules/javafx.graphics/src/main/java/javafx/css/StyleableObjectProperty.java line 273:
> 
>> 271: 
>> 272:     private StyleOrigin origin = null;
>> 273:     private TransitionController<T> controller = null;
> 
> minor: `= null` is unnecessary

I agree. Not sure if I should remove it from all of the StyleableProperty implementations, as the `origin = null` is already there.

> modules/javafx.graphics/src/main/java/javafx/css/StyleableObjectProperty.java line 465:
> 
>> 463:                 if (mediators.get(i) == mediator) {
>> 464:                     mediators.remove(i);
>> 465:                     break;
> 
> how do we ensure there are no duplicates in the `mediators` list?

The only mediators that are added are new ones (L418) or ones that don't come from this controller (L430). This applies to all controllers, so there can never be a duplicate.

> modules/javafx.graphics/src/main/java/javafx/css/StyleableObjectProperty.java line 538:
> 
>> 536:                 } else if (startValue instanceof Interpolatable && endValue instanceof Interpolatable) {
>> 537:                     value = ((Interpolatable<U>)startValue).interpolate(endValue, progress);
>> 538:                 } else {
> 
> minor: does this if-else block ordered by the expected frequency?

Most of the time, we get an array series from CSS, so the common case is at the top.

> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 9010:
> 
>> 9008:          *
>> 9009:          * @param property the {@code StyleableProperty}
>> 9010:          * @param result the map into which results are stored, should be {@code null}
> 
> should be or could be?
> see L9032

_Should_ be `null` to get the non-allocating behavior if nothing is found, but _could_ also be non-null if we want to provide our own output map.

> modules/javafx.graphics/src/main/java/javafx/scene/layout/BackgroundFill.java line 157:
> 
>> 155:         return this.fill == fill
>> 156:             && this.radii == radii
>> 157:             && this.insets == insets;
> 
> are you sure these don't need equals()?

This is an internal optimization, see the comment at L135. Since we control the implementation of all types here, we _know_ that these types will return the existing instance if the intermediate value is equal to either the start value or the end value. Using `equals()` here is not necessary, and is not required for correctness.

> modules/javafx.graphics/src/main/java/javafx/scene/layout/BackgroundImage.java line 212:
> 
>> 210:             && this.repeatY == repeatY
>> 211:             && this.position == position
>> 212:             && this.size == size;
> 
> same comment on equals().
> 
> 'size' comes from interpolate() on L178, so there is no guarantee as of its identity

See my other comment. We _know_ that `BackgroundSize` returns existing instances if the intermediate value would be equal.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1701068408
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1701068436
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1701068449
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1701068468
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1701068503
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1701068540
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1701068594
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1701068658
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1701068704
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1701068718