On Fri, 11 Jul 2025 18:50:45 GMT, Phil Race <p...@openjdk.org> wrote:
>>> I think we need to have the "whole picture" tested to make sure everything >>> does as we expect. It isn't so easy to tell even from looking at the source >>> of ImageIcon. Then spec'ed so developers know what to expect. When I wrote >>> a little test, I see null args do generate NPEs but invalid args don't >>> result in null images Toolkit images are installed, even if they can't be >>> used because the source isn't valid. They are "effectively" null, but not >>> "actually" null. We should at least specify the null arg behaviours and try >>> to say something about invalid image data. And as a result, I am now >>> flipping to just documenting the setImage(null) NPE The protected method >>> loadImage probably needs to say it too. Invalid image data, maybe we can >>> cover that in a class level statement. Note: I'd like to see every >>> constructor tested with both null and invalid image data. >> >> I tested with null image/imagedata and invalid imagedata and it seems only >> these constructors throw NPE (current JDK) for null image (invalid image >> doesnt throw any exception) >> >> ImageIcon(image, desc) >> ImageIcon(image) >> ImageIcon(byte[], desc) >> ImageIcon(byte[]) >> >> Current PR checks for null in ImageIcon(image) so it will not throw NPE but >> ImageIcon(byte[]) will still throw.. >> Should we handle those in separate PR and let this only be for >> setImage(null)? > >> > I think we need to have the "whole picture" tested to make sure everything >> > does as we expect. It isn't so easy to tell even from looking at the >> > source of ImageIcon. Then spec'ed so developers know what to expect. When >> > I wrote a little test, I see null args do generate NPEs but invalid args >> > don't result in null images Toolkit images are installed, even if they >> > can't be used because the source isn't valid. They are "effectively" null, >> > but not "actually" null. We should at least specify the null arg >> > behaviours and try to say something about invalid image data. And as a >> > result, I am now flipping to just documenting the setImage(null) NPE The >> > protected method loadImage probably needs to say it too. Invalid image >> > data, maybe we can cover that in a class level statement. Note: I'd like >> > to see every constructor tested with both null and invalid image data. >> >> I tested with null image/imagedata and invalid imagedata and it seems only >> these constructors throw NPE (current JDK) for null image (invalid image >> doesnt throw any exception) >> >> ImageIcon(image, desc) ImageIcon(image) ImageIcon(byte[], desc) >> ImageIcon(byte[]) >> >> Current PR checks for null in ImageIcon(image) so it will not throw NPE but >> ImageIcon(byte[]) will still throw.. Should we handle those in separate PR >> and let this only be for setImage(null)? > > You told me (off-line) that null URL also throws NPE and I see that too. > > This class is a bit of a mess of accidental behaviours and scant > documentation. > > Let's document all the NPE behaviours and include a test that verifies it. > > And invalid data like "new byte[0]" or null for a file name, or pointing to > something that isn't an image file, or an invalid URL .. etc .. results in an > Image instance being present, but it will never be completed so can never be > drawn. > > I think we need a class level statement like > "If the image source parameter to a constructor is non-null, but does not > reference valid accessible image data, no exceptions will be thrown but the > image will be 'effectively' null, as it will have no dimensions and never be > drawn, and > getImageLoadStatus() will report ERRORED" - you should verify that last part > but I think it is right. > > We probably should add the similar text on setImage(). > > We can also test the invalid image data cases too. @prrace I guess we need to change the bug summary to maybe "Clarify ImageIcon constructors and setImage method for null image parameters and invalid image data" or do you have anything in mind? ------------- PR Comment: https://git.openjdk.org/jdk/pull/25767#issuecomment-3116269718
