On Tue, 4 Mar 2025 06:51:54 GMT, Jeremy Wood <d...@openjdk.org> wrote:
> Previously ImageTypeSpecifier treated all TYPE_BYTE_INDEXED as if they were
> opaque.
>
> In this ticket's case: an ImageWriter received the wrong ImageTypeSpecifier
> and mistakenly indicated it *could* support a BufferedImage. But when the
> actual BufferedImage arrived (which was translucent), the ImageWriter threw
> an exception.
>
> Instead:
> Now the ImageTypeSpecifier accurately describes the given BufferedImage.
src/java.desktop/share/classes/javax/imageio/ImageTypeSpecifier.java line 1:
> 1: /*
Could you bump the copyright year?
src/java.desktop/share/classes/javax/imageio/ImageTypeSpecifier.java line 923:
> 921: int bufferedImageType = ((BufferedImage)image).getType();
> 922: if (bufferedImageType != BufferedImage.TYPE_CUSTOM &&
> 923: bufferedImageType != BufferedImage.TYPE_BYTE_INDEXED) {
Suggestion:
if (bufferedImageType != BufferedImage.TYPE_CUSTOM &&
bufferedImageType != BufferedImage.TYPE_BYTE_INDEXED) {
Usually, 8-space indentation used for wrapped continuation lines, if the
wrapped isn't aligned to a structure on the first line.
Personally, I'd move the `&&` operator to the wrapped line — it clearly
indicates it's a continuation. Yet neither style has been agreed upon.
test/jdk/javax/imageio/plugins/jpeg/JpegWriterWriteNonOpaqueIndexColorModelTest.java
line 33:
> 31:
> 32: import javax.imageio.ImageIO;
> 33: import java.awt.*;
Could you expand the wildcard imports?
test/jdk/javax/imageio/plugins/jpeg/JpegWriterWriteNonOpaqueIndexColorModelTest.java
line 63:
> 61: }
> 62: if (imageType == Transparency.BITMASK)
> 63: alpha[0] = 0;
Suggestion:
if (imageType == Transparency.BITMASK) {
alpha[0] = 0;
}
It's recommended to always use braces even when there's only one statement in
the block.
test/jdk/javax/imageio/plugins/jpeg/JpegWriterWriteNonOpaqueIndexColorModelTest.java
line 74:
> 72: boolean result = ImageIO.write(bi, "jpg", new
> ByteArrayOutputStream());
> 73: if (result != expectedWriteReturnValue)
> 74: throw new Error("ImageIO.write(..) returned " + result +
> " but we expected " + expectedWriteReturnValue);
Suggestion:
if (result != expectedWriteReturnValue) {
throw new Error("ImageIO.write(..) returned " + result + " but
we expected " + expectedWriteReturnValue);
}
test/jdk/javax/imageio/plugins/jpeg/JpegWriterWriteNonOpaqueIndexColorModelTest.java
line 77:
> 75: System.out.println("Tested passed");
> 76: return true;
> 77: } catch(Exception e) {
Suggestion:
} catch (Exception e) {
test/jdk/javax/imageio/plugins/jpeg/JpegWriterWriteNonOpaqueIndexColorModelTest.java
line 81:
> 79: e.printStackTrace();
> 80: return false;
> 81: }
Should an exception be allowed to escape? You throw an `Error` if the expected
return value doesn't match.
test/jdk/javax/imageio/plugins/jpeg/JpegWriterWriteNonOpaqueIndexColorModelTest.java
line 83:
> 81: }
> 82: }
> 83: }
Having a blank line in the end of a file is preferred.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23884#discussion_r1987265582
PR Review Comment: https://git.openjdk.org/jdk/pull/23884#discussion_r1987265134
PR Review Comment: https://git.openjdk.org/jdk/pull/23884#discussion_r1987268179
PR Review Comment: https://git.openjdk.org/jdk/pull/23884#discussion_r1987271648
PR Review Comment: https://git.openjdk.org/jdk/pull/23884#discussion_r1987275369
PR Review Comment: https://git.openjdk.org/jdk/pull/23884#discussion_r1987276141
PR Review Comment: https://git.openjdk.org/jdk/pull/23884#discussion_r1987278113
PR Review Comment: https://git.openjdk.org/jdk/pull/23884#discussion_r1987278977
