On Thu, 5 Sep 2024 00:42:30 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:
>
> changes per review
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 639:
> 637: var definitions = node.miscProperties != null ?
> node.miscProperties.transitionDefinitions : null;
> 638: if (definitions == null) {
> 639: definitions = node.new
> TransitionDefinitionCollection();
TransitionDefinitionCollection can be a static class, can it?
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 9043:
> 9041: }
> 9042:
> 9043: return result;
minor suggestion:
List<CssMetaData<? extends Styleable, ?>> subMetadata =
metadata.getSubProperties();
if (subMetadata != null) {
for (int i = 0, max = subMetadata.size(); i < max; ++i) {
result = collectTransitionTimers(property, result);
}
}
return result;
modules/javafx.graphics/src/main/java/javafx/scene/layout/BackgroundConverter.java
line 109:
> 107: image = img;
> 108: } else {
> 109: throw new
> IllegalArgumentException("convertedValues");
would it make sense to make this exception message more meaningful to help
diagnose the issue? for example, what is expected and what is encountered.
(this comment applies to every other converter)
modules/javafx.graphics/src/main/java/javafx/scene/layout/Border.java line 418:
> 416: * {@inheritDoc}
> 417: *
> 418: * @throws NullPointerException {@inheritDoc}
@kevinrushforth : is there a bug in javadoc that requires @throws repeated
here? without it, the base class @throws is not shown.
modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderStroke.java
line 54:
> 52: * <ul>
> 53: * <li>{@link Color} ↔ {@link LinearGradient}
> 54: * <li>{@link Color} ↔ {@link RadialGradient}
nice use of unicode `↔`
modules/javafx.graphics/src/test/java/test/javafx/css/StyleableProperty_transition_Test.java
line 198:
> 196: void setup() {
> 197: toolkit = (StubToolkit)Toolkit.getToolkit();
> 198: scene = new Scene(new Group());
should we `hide` stage in `@AfterEach` ?
probably applies to other tests
modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BackgroundConverterTest.java
line 51:
> 49: import static org.junit.jupiter.api.Assertions.*;
> 50:
> 51: public class BackgroundConverterTest {
this is minor, but applicable to other tests:
it would be nice to have a short basic description of the test, for human
consumption.
Example:
Tests BackgroundConverter functions
- from url
- convert from image
- equality of reconstructed objects
(this is just an example)
Also, should it also test `convertBack()` ?
modules/javafx.graphics/src/test/java/test/javafx/scene/paint/ColorTest.java
line 738:
> 736: @Test
> 737: public void testWebHsla3Param() {
> 738: assertThrows(IllegalArgumentException.class, () ->
> Color.web("hsla(240, 50%, 50%)"));
even though this change is unrelated to the feature, I agree with the decision
to make it in this PR. and its trivial, so unlikely to give us trouble merging.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1745865564
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1745878383
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1745709416
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1745778766
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1745822017
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1745835405
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1745840583
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1745846997
