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