On Thu, 1 Aug 2024 21:40:54 GMT, Andy Goryachev <ango...@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
