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)? > > 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. Documented all NPE behavour and added class-level statement. Updated test to verify it. getImageLoadStatus() does return 4 which is ERRORED for invalid image, updated test reflects that. ------------- PR Comment: https://git.openjdk.org/jdk/pull/25767#issuecomment-3066744236
