On Fri, 2 Aug 2024 01:24:59 GMT, Michael Strauß <mstra...@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
