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

[Andy Goryachev]

On Fri, 2 Aug 2024 01:24:59 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> 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.

that's what I thought, thanks for clarification.

>> 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.

I see, so it only checks for hitting the start/end point.  thanks for clarification!

>> 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.
>
> Yes, I think that's good, as it will alert us to the impact of adding a new enum value. We would probably need to account for the new value here (there's no good default we can choose).

IllegalArgumentException?

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1701991167
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1702001112
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1702004500