On Fri, 29 Aug 2025 15:50:41 GMT, Andy Goryachev <[email protected]> wrote:
>> Michael Strauß has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains three additional
>> commits since the last revision:
>>
>> - Merge branch 'master' into feature/relaxed-interpolation
>> - javadoc
>> - Allow interpolation outside of range [0,1]
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/scene/layout/region/Margins.java
> line 92:
>
>> 90:
>> 91: return new Margins(
>> 92: Math.max(0, InterpolationUtils.interpolate(top,
>> endValue.top, t)),
>
> what is the reason for clamping this at 0, but not 1.0 ?
>
> also, why clamp here but not in `Insets` ?
`Margins` is used for border widths, which can't be negative according to
specification. However, they can be any positive size, therefore we don't clamp
here.
> modules/javafx.graphics/src/main/java/javafx/scene/paint/RadialGradient.java
> line 251:
>
>> 249: * @param cycleMethod cycle method applied to the gradient
>> 250: * @param stops the gradient's color specification
>> 251: * @throws IllegalArgumentException if {@code focusDistance} or
>> {@code radius} is negative
>
> can we use the `@since` tag applicable to `@throws` only?
> (also below)
I don't think this is possible.
> modules/javafx.graphics/src/main/java/javafx/scene/paint/RadialGradient.java
> line 613:
>
>> 611: if (distance < 0) {
>> 612: throw new IllegalArgumentException(
>> 613: "Invalid gradient specification: focus-distance
>> cannot be negative");
>
> why wrap here?
> also below
I removed the line break in this place and others.
> modules/javafx.graphics/src/test/java/test/javafx/scene/paint/RadialGradientTest.java
> line 302:
>
>> 300: } catch (IllegalArgumentException iae) {
>> 301: assertTrue(iae.getMessage().contains("focus-distance cannot
>> be negative"));
>> 302: }
>
> `RadialGradient` adds `checkInvariants` in three places, but only two tests
> are added.
> Should we add one more?
No, but I've removed the `checkInvariants()` call in the private constructor,
as it is only called from the `interpolate()` method and thus always operates
with valid gradients (no need to verify what has already been verified).
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1822#discussion_r2312127078
PR Review Comment: https://git.openjdk.org/jfx/pull/1822#discussion_r2312127935
PR Review Comment: https://git.openjdk.org/jfx/pull/1822#discussion_r2312127183
PR Review Comment: https://git.openjdk.org/jfx/pull/1822#discussion_r2312127492
