On Mon, 18 Aug 2025 20:42:20 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>>> `FXMLoader` uses the `@NamedArg` annotation to associate XML attribute >>> values with constructor arguments. >> >> I wonder if this should be explained in the @NamedArg javadoc. > >> > `FXMLoader` uses the `@NamedArg` annotation to associate XML attribute >> > values with constructor arguments. >> >> I wonder if this should be explained in the @NamedArg javadoc. > > It probably should, I never use FXML, but I should have guessed it was for > that purpose :) > @hjohn in case you missed my earlier comment: > > Noticed it does not close the input stream with a synchronous constructor (it > does with the async one). > > you can try it with the monkey tester from this branch > https://github.com/andy-goryachev-oracle/MonkeyTest/tree/image.from.stream > > or simply > > ``` > byte[] b = ImageTools.writePNG(src); > ByteArrayInputStream in = new ByteArrayInputStream(b) { > @Override > public void close() throws IOException { > System.out.println("Closed " + Thread.currentThread()); > } > }; > > // requires JDK-8361286: Allow enabling of background loading for > images loaded from an InputStream > Image im = new Image(in, loadInBackground); > imageView.setImage(im); > } catch (IOException e) { > e.printStackTrace(); > } > ``` A good question would be what the behavior should be. For the URL variants, since the Image class is creating the stream in both cases, it makes sense that it closes that stream. For `InputStream` parameters, the standard practice is to leave closing up to the caller, unless it is a wrapper (like `BufferedInputStream`). So, I think how the synchronous variant does it is probably the correct approach. However, when consuming the stream asynchronously, it is very hard for users to know when to safely close the stream as you basically handed ownership of the stream to another thread (you can't use the stream for anything while background loading is in progress...) The only way to know when the other thread is done with the stream is to look at the progress property. So, I think there are a few options: 1. Document that we'll be closing the stream in the background loading case; effectively, since you're giving this InputStream to be used by a different thread, the ownership of the stream transfers from the user to Image. 2. Document that you need to hook into the progress property to know when to close the stream (yikes!) 3. Don't provide the background loading option for `InputStream` (ie. don't integrate this PR, or only the documentation and clean-up bits) 4. Provide alternative API that takes `Supplier<InputStream>`, very powerful, and it would be clear who owns the stream and who closes it, and this would work for both sync/async. A prime motivation for allowing input streams for background loading was that the URL API is limited (you can't do authentication, use POST, or provide a body). In those cases you're forced to `InputStream` but then forego the option to load in the background, meaning that you can't have a progress indicator for large images, or show progressive images like with PJPEG. ------------- PR Comment: https://git.openjdk.org/jfx/pull/1875#issuecomment-3211366725
