On Wed, 31 Jul 2024 19:34:12 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> This PR completes the CSS Transitions story (see #870) by adding
>> interpolation support for backgrounds and borders, making them targetable by
>> transitions.
>>
>> `Background` and `Border` objects are deeply immutable, but not
>> interpolatable. Consider the following `Background`, which describes the
>> background of a `Region`:
>>
>>
>> Background {
>> fills = [
>> BackgroundFill {
>> fill = Color.RED
>> }
>> ]
>> }
>>
>>
>> Since backgrounds are deeply immutable, changing the region's background to
>> another color requires the construction of a new `Background`, containing a
>> new `BackgroundFill`, containing the new `Color`.
>>
>> Animating the background color using a CSS transition therefore requires the
>> entire Background object graph to be interpolatable in order to generate
>> intermediate backgrounds.
>>
>> More specifically, the following types will now implement `Interpolatable`.
>>
>> - `Insets`
>> - `Background`
>> - `BackgroundFill`
>> - `BackgroundImage`
>> - `BackgroundPosition`
>> - `BackgroundSize`
>> - `Border`
>> - `BorderImage`
>> - `BorderStroke`
>> - `BorderWidths`
>> - `CornerRadii`
>> - `Stop`
>> - `Paint` and all of its subclasses
>> - `Margins` (internal type)
>> - `BorderImageSlices` (internal type)
>>
>> ## Interpolation of composite objects
>>
>> As of now, only `Color`, `Point2D`, and `Point3D` are interpolatable. Each
>> of these classes is an aggregate of `double` values, which are combined
>> using linear interpolation. However, many of the new interpolatable classes
>> comprise of not only `double` values, but a whole range of other types. This
>> requires us to more precisely define what we mean by "interpolation".
>>
>> Mirroring the CSS specification, the `Interpolatable` interface defines
>> several types of component interpolation:
>>
>> | Interpolation type | Description |
>> |---|---|
>> | default | Component types that implement `Interpolatable` are interpolated
>> by calling the `interpolate(Object, double)}` method. |
>> | linear | Two components are combined by linear interpolation such that `t
>> = 0` produces the start value, and `t = 1` produces the end value. This
>> interpolation type is usually applicable for numeric components. |
>> | discrete | If two components cannot be meaningfully combined, the
>> intermediate component value is equal to the start value for `t < 0.5` and
>> equal to the end value for `t >= 0.5`. |
>> | pairwise | Two lists are combined by pairwise interpolation. If the start
>> list has fewer elements than the target list, the...
>
> Michael Strauß has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fixed a bug
Not a complete review, but the first batch of comments (I stopped at Stop, will
continue). I think it's better to send smaller batches than one giant one (and
I am afraid losing the comments due to some GH glitch).
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;">`
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?
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) {
...
}
modules/javafx.graphics/src/main/java/javafx/css/StyleableLongProperty.java
line 136:
> 134: @Override
> 135: public void onUpdate(double progress) {
> 136: set(progress < 1 ? startValue + (long)((endValue -
> startValue) * progress) : endValue);
minor: should it use (long)Math.round() ?
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?
modules/javafx.graphics/src/main/java/javafx/css/StyleableObjectProperty.java
line 210:
> 208: // the running transition by adding its mediator to the
> newly created transition controller
> 209: // that manages the hover->base transition. In this way, the
> new transition controller will
> 210: // continue to aggregate the effects of the pre-existing
> transition.
TODO
add a unit test?
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
modules/javafx.graphics/src/main/java/javafx/css/StyleableObjectProperty.java
line 312:
> 310: // refers to the reversing controller, and not to this
> controller. We need to be careful to only
> 311: // clear references to this controller.
> 312: if (controller == this) {
is it guaranteed that the reference will be cleared eventually?
TODO
is there a unit test?
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?
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?
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
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 9184:
> 9182: *
> 9183: * @param metadata the CSS metadata of the property
> 9184: * @param result the map into which results are stored, should
> be {@code null}
same comment, `could be null`?
modules/javafx.graphics/src/main/java/javafx/scene/layout/Background.java line
301:
> 299: boolean opaqueFill = false;
> 300:
> 301: // We need to iterate over all of the supplied elements in the
> fills list.
TODO
check if there is a unit test for this
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()?
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
modules/javafx.graphics/src/main/java/javafx/scene/layout/BackgroundPosition.java
line 266:
> 264: Side verticalSide, double verticalPosition,
> boolean verticalAsPercentage) {
> 265: return this.horizontalSide == horizontalSide
> 266: && this.horizontalPosition == horizontalPosition
so here `==` seems to be ok since we are comparing enums and primitive values,
but...
should we account for floating point errors that might appear during
interpolation?
i.e.
isClose(this.horizontalPosition, horizontalPosition) &&
where
private static boolean isClose(double a, double b) {
return Math.abs(a - b) < EPSILON; // 0.00001 or something like that should be
ok for screen coordinates
}
modules/javafx.graphics/src/main/java/javafx/scene/layout/BackgroundSize.java
line 258:
> 256: return this.width == width
> 257: && this.height == height
> 258: && this.widthAsPercentage == widthAsPercentage
same comment - use isClose() for doubles?
modules/javafx.graphics/src/main/java/javafx/scene/layout/Border.java line 331:
> 329:
> 330: for (int i = 0, max = strokes.size(); i < max; i++) {
> 331: final BorderStroke stroke = strokes.get(i);
just curious: do we really need `final` keyword for local variables after java8?
modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderConverter.java
line 270:
> 268: case ROUND -> BackgroundRepeat.ROUND;
> 269: case SPACE -> BackgroundRepeat.SPACE;
> 270: case STRETCH -> BackgroundRepeat.NO_REPEAT;
what do you think of adding default: case?
this code will not compile if BorderRepeat ever acquires a new value.
modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderConverter.java
line 279:
> 277: case ROUND -> BorderRepeat.ROUND;
> 278: case SPACE -> BorderRepeat.SPACE;
> 279: case NO_REPEAT -> BorderRepeat.STRETCH;
same here // "non-comprehensive switch expression over evolved enum problem"
modules/javafx.graphics/src/main/java/javafx/scene/paint/Color.java line 1956:
> 1954:
> 1955: // If both instances are equal, return this instance to prevent
> the creation of numerous small objects.
> 1956: if (t <= 0.0 || equals(endValue)) return this;
This question might be outside of the current PR, but I think the naive color
interpolation algorithm is a bad choice here (L1960).
See for example
https://www.alanzucconi.com/2016/01/06/colour-interpolation/
The question that is relevant to this PR is whether we should enable the user
to specify the interpolation algorithm.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1522#pullrequestreview-2213806821
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1700900660
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1700902994
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1700911902
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1700915376
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1700918183
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1700925072
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1700933990
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1700936434
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1700942231
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1700956059
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1700960388
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1700962134
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1700967627
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1700988085
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1700990366
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1700995142
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1700997569
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1700999656
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1701010560
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1701011685
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1701018016
