On Tue, 8 Jul 2025 06:00:21 GMT, Prasanta Sadhukhan <psadhuk...@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. ------------- PR Comment: https://git.openjdk.org/jdk/pull/25767#issuecomment-3063379284
