On Tue, 15 Oct 2024 21:35:14 GMT, John Hendrikx <[email protected]> wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review changes
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/iio/ImageStorage.java
> line 201:
>
>> 199: return null;
>> 200: }
>> 201: }
>
> Can we document why this is done? If I had to guess: loading
> `XImageLoaderFactory`, even though it is present in this module, may fail
> loading dependent classes (`ImageIO`).
I've added a javadoc paragraph with more details.
> modules/javafx.graphics/src/main/java/com/sun/javafx/iio/javax/XImageLoader.java
> line 217:
>
>> 215: }
>> 216:
>> 217: return ByteBuffer.wrap(Arrays.copyOf(data, size - offset));
>
> This doesn't look correct. If `offset` isn't zero, then dropping the last
> few bytes seems incorrect. It should start the copy from `offset`?
>
> I would think this should be: `ByteBuffer.wrap(data, offset, size - offset)`
> (avoids copy but may waste some space).
>
> Or if you still want to make a copy, then `Arrays.copyOfRange(data, offset,
> size)`.
>
> Note: I have the impression that calling `getData` means the bank won't be
> modified anymore, so there may not be a need to copy it.
Good catch, I think it is sufficient to use `ByteBuffer.wrap(data, offset, size
- offset)` in all cases.
> modules/javafx.graphics/src/main/java/com/sun/javafx/image/PixelUtils.java
> line 215:
>
>> 213: getB2BConverter(PixelGetter<ByteBuffer> src,
>> PixelSetter<ByteBuffer> dst)
>> 214: {
>> 215: if (src == ByteRgba.getter) {
>
> minor: I'd be in favor of fixing this formatting. I know it means well, but
> it really isn't improving readability. Perhaps it is more of a job for a
> `switch` or `Map` lookup.
The code style in this area is very... special. However, I'm hesitant to do a
wholesale reformatting here. This will pollute the Git change log with not all
that much benefits.
> modules/javafx.graphics/src/main/java/javafx/scene/image/Image.java line 75:
>
>> 73: * <li><a href="http://www.libpng.org/pub/png/spec/";>PNG</a></li>
>> 74: * </ul>
>> 75: * For all other formats, JavaFX uses the {@link javax.imageio Java
>> Image I/O API} on supported platforms.
>
> This doesn't look like a valid javadoc link; in Eclipse at least it doesn't
> do anything. Perhaps:
>
> Suggestion:
>
> * For all other formats, JavaFX uses the {@link javax.imageio.ImageIO Java
> Image I/O API} on supported platforms.
Must be an Eclipse problem. IntelliJ and the generated Javadocs correctly point
to the `javax.imageio` package documentation.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1803167608
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1803168475
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1803171651
PR Review Comment: https://git.openjdk.org/jfx/pull/1593#discussion_r1803167495
