On Tue, 1 Sep 2026 18:31:17 GMT, Andy Goryachev <[email protected]> wrote:
>> John Hendrikx has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Make getDrawingContext final
>
> modules/javafx.graphics/src/main/java/com/sun/prism/sw/SWDrawingContext.java
> line 258:
>
>> 256: @Override
>> 257: public void setGlobalAlpha(double alpha) {
>> 258: this.globalAlpha = Math.clamp(alpha, 0.0, 1.0);
>
> the clamping here violates the contract: "any valid double [value] can be
> set".
> the clamping should be done upon use.
Fixed
> modules/javafx.graphics/src/main/java/com/sun/prism/sw/SWDrawingContext.java
> line 547:
>
>> 545: double y1 = Math.floor(Math.min(y, y + h));
>> 546: double x2 = Math.ceil(Math.max(x, x + w));
>> 547: double y2 = Math.ceil(Math.max(y, y + h));
>
> the clip coordinates are rounded before transformation (L565), possibly
> repeatedly. later transformation will change the existing clip. should it
> be done at the last possible moment?
I've changed this to immediately transform the clip to device space. This has
the benefit that there is no repeated rounding anymore, and also that one can
now set a clip, and still do a rotation transform after setting the clip (the
other way around is still not allowed).
> modules/javafx.graphics/src/main/java/com/sun/prism/sw/SWDrawingContext.java
> line 987:
>
>> 985: applyStrokeParameters();
>> 986:
>> 987: graphics.setTransform(BaseTransform.IDENTITY_TRANSFORM);
>
> will applying the `IDENTITY_TRANSFORM` prevent scaling of stroke attributes
> like line width and dash lengths?
Yes, it would have. Everything is already in (transformed) device space, with
the exception of the stroke width and dash lengths. I've adjusted this now to
transform these if scale != 1.0
> modules/javafx.graphics/src/main/java/javafx/scene/image/WritableImage.java
> line 170:
>
>> 168: * @return the {@link DrawingContext} associated with this image,
>> never {@code null}
>> 169: */
>> 170: public DrawingContext getDrawingContext() {
>
> should this method be `final`?
Yes, it is the safer choice
> modules/javafx.graphics/src/main/java/javafx/scene/image/WritableImage.java
> line 175:
>
>> 173: if (context == null) {
>> 174: if (!(getWritablePlatformImage() instanceof
>> com.sun.prism.Image img)) {
>> 175: throw new IllegalStateException("platformImage must be
>> a prism image");
>
> 1. this exception must be documented
> 2. what is a "prism image" from the user perspective? could the message be
> rephrased to give the user a better understanding?
This exception can't happen, we only have prism images (`PlatformImage` is an
interface with 1 implementation) -- so I've removed it and just did a hard cast.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1969#discussion_r3932029146
PR Review Comment: https://git.openjdk.org/jfx/pull/1969#discussion_r3932013651
PR Review Comment: https://git.openjdk.org/jfx/pull/1969#discussion_r3932025945
PR Review Comment: https://git.openjdk.org/jfx/pull/1969#discussion_r3932054057
PR Review Comment: https://git.openjdk.org/jfx/pull/1969#discussion_r3932043539
