On Sun, 27 Jul 2025 07:08:47 GMT, Prasanta Sadhukhan <psadhuk...@openjdk.org>
wrote:
>> When trying to call 'icon.setImage(null);' where 'icon' is an instance of
>> ImageIcon, a null pointer exception is thrown at runtime.
>> The code tried to get the `id` for that image and instantiates
>> `MediaTracker` to associate the null image to that `id` and checks the
>> status of loading this null image, removes the null image from the tracker
>> and then tries to get the image width where it throws NPE as image is null.
>>
>> It's better to not go through all MediaTracker usage and bail out initially
>> itself for null image..
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> ArgType name change
At what point did we decide not to modify `Image.setImage` to accept `null` as
the parameter? It made perfect sense.
I wonder if it's possible to use JUnit a test framework in jtreg?
src/java.desktop/share/classes/javax/swing/ImageIcon.java line 64:
> 62: * are preloaded using MediaTracker to monitor the loaded state
> 63: * of the image.
> 64: * If the image source parameter to a constructor or method is non-null,
Suggestion:
* of the image.
*
* <p>
* If the image source parameter to a constructor or method is non-null,
I suggest putting the discussion on non-null parameters in its own paragraph
for ease of reading the javadoc.
src/java.desktop/share/classes/javax/swing/ImageIcon.java line 68:
> 66: * no exceptions will be thrown but the image will be unset,
> 67: * as it will have no dimensions and never be drawn, and
> 68: * {@code getImageLoadStatus()} will report {@see
> java.awt.MediaTracker#ERRORED}.
Suggestion:
* {@code getImageLoadStatus()} will report {@link
java.awt.MediaTracker#ERRORED}.
You shouldn't use `@see` here, it should either be `@code` or `@link`. I think
both references should use the same tag: either `@code` or `@link`; the latter
allows quickly navigating to the *method* or constant.
Perhaps, `@link` for the `getImageLoadStatus` method would work better: it
allows viewing the description of the method and its purpose; in its turn,
`getImageLoadStatus` references possible return values including
`MediaTracker.ERRORED` with `@see` tag.
src/java.desktop/share/classes/javax/swing/ImageIcon.java line 186:
> 184: * @param location the URL for the image
> 185: * @param description a brief textual description of the image
> 186: * @throws {@code NullPointerException} if (@code null) URL is
> passed.
Suggestion:
* @throws NullPointerException if a {@code null} URL is passed
The exception type is automatically rendered in monospaced font with a link to
its description.
You meant to use braces instead of parentheses.
Usually, there's no full stop for `@throws`.
Alternatively, specify the parameter name for more clarity:
Suggestion:
* @throws NullPointerException if {@code location} is {@code null}
src/java.desktop/share/classes/javax/swing/ImageIcon.java line 206:
> 204: * a string representation of the URL.
> 205: * @param location the URL for the image
> 206: * @throws {@code NullPointerException} if (@code null) URL is
> passed.
Suggestion:
* @throws NullPointerException if a {@code null} URL is passed
src/java.desktop/share/classes/javax/swing/ImageIcon.java line 217:
> 215: * @param image the image
> 216: * @param description a brief textual description of the image
> 217: * @throws {@code NullPointerException} if (@code null) Image is
> passed.
Suggestion:
* @throws NullPointerException if a {@code null} image is passed
Here, image shouldn't be capitalised, you mean it as the literal _“image”_
rather than a type.
I still prefer specifying the parameter name:
Suggestion:
* @throws NullPointerException if {@code image} is {@code null}
src/java.desktop/share/classes/javax/swing/ImageIcon.java line 229:
> 227: * then the string is used as the description of this icon.
> 228: * @param image the image
> 229: * @throws {@code NullPointerException} if (@code null) Image is
> passed.
Suggestion:
* @throws NullPointerException if {@code image} is {@code null}
src/java.desktop/share/classes/javax/swing/ImageIcon.java line 253:
> 251: * by the AWT Toolkit, such as GIF, JPEG, or (as of 1.3) PNG
> 252: * @param description a brief textual description of the image
> 253: * @throws {@code NullPointerException} if (@code null) imageData is
> passed.
Suggestion:
* @throws NullPointerException if {@code imageData} is {@code null}
src/java.desktop/share/classes/javax/swing/ImageIcon.java line 277:
> 275: * @param imageData an array of pixels in an image format supported
> by
> 276: * the AWT Toolkit, such as GIF, JPEG, or (as of 1.3) PNG
> 277: * @throws {@code NullPointerException} if (@code null) imageData is
> passed.
Suggestion:
* @throws NullPointerException if {@code imageData} is {@code null}
src/java.desktop/share/classes/javax/swing/ImageIcon.java line 382:
> 380: * Sets the image displayed by this icon.
> 381: * @param image the image
> 382: * @throws {@code NullPointerException} if (@code null) Image is
> passed.
Suggestion:
* @throws NullPointerException if {@code image} is {@code null}
test/jdk/javax/swing/ImageIcon/ImageIconTest.java line 39:
> 37: import javax.swing.ImageIcon;
> 38:
> 39: public class ImageIconTest {
Suggestion:
public class ImageIconNullParametersTest {
Be more specific?
test/jdk/javax/swing/ImageIcon/ImageIconTest.java line 52:
> 50: fos.write(invalidData);
> 51: }
> 52: String fileName = (new File(System.getProperty("test.src", "."),
> imgName)).getName();
Why do we need jumping over the hoops?
You're creating an image, put it into the current directory that defaults to
`scratch` directory in jtreg. The `scratch` directory is automatically cleaned
by jtreg between tests, thus you won't even leave files in the source tree of
JDK.
test/jdk/javax/swing/ImageIcon/ImageIconTest.java line 66:
> 64: new ImageIcon(s);
> 65: passed = true; // no exception expected for
> this case
> 66: break;
Does it make sense to create richer enum values which combine the expected
state?
-------------
Changes requested by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/25767#pullrequestreview-3064243188
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2237595098
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2237623088
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2237628892
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2237634552
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2237651047
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2237652775
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2237657503
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2237660636
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2237662773
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2237696278
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2237685414
PR Review Comment: https://git.openjdk.org/jdk/pull/25767#discussion_r2237693876
