RFR: 8260342: FXMLLoader fails to load a sub layout using fx:include with the resources attribute [v2]
Ajit Ghaisas
aghaisas at openjdk.org
Tue Aug 1 10:09:52 UTC 2023
On Wed, 21 Jun 2023 13:34:28 GMT, Guillaume Tâche <duke at openjdk.org> wrote:
>> This fixes ResourceBundle loading by calling `ResourceBundle.getBundle(value, Locale.getDefault())` when the loader resources are null or their classloader is null.
>> Apparently the original author of the bug report said they would submit a pull request, but it seems like it never happened.
>
> Guillaume Tâche has updated the pull request incrementally with one additional commit since the last revision:
>
> Empty commit
Overall the fix and tests look fine.
I have a couple of comments on the fix.
modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java line 1101:
> 1099: loadListener.readInternalAttribute(localName, value);
> 1100: }
> 1101: final ResourceBundle loaderResources = FXMLLoader.this.resources;
Why to have this local temporary `loaderResources`?
We can simply use `this.resources` for readability.
modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java line 1106:
> 1104: } else {
> 1105: final ClassLoader cl = loaderResources.getClass().getClassLoader();
> 1106: resources = cl == null ?
Add a bracket around `cl == null`
-------------
Changes requested by aghaisas (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/1158#pullrequestreview-1556626578
PR Review Comment: https://git.openjdk.org/jfx/pull/1158#discussion_r1280399572
PR Review Comment: https://git.openjdk.org/jfx/pull/1158#discussion_r1280400336
More information about the openjfx-dev
mailing list