WIP: JDK-8186429 FXMLLoader setStaticLoad should be public API #44
Conversation
|
There is already a webrev in the issue with potential fix. Does it make this PR redundant? |
| @@ -39,7 +38,7 @@ | |||
| @SuppressWarnings("deprecation") | |||
| public void testStaticScriptLoad() throws IOException { | |||
| FXMLLoader fxmlLoader = new FXMLLoader(getClass().getResource("static_script_load.fxml")); | |||
| FXMLLoaderHelper.setStaticLoad(fxmlLoader, true); | |||
| fxmlLoader.setStaticLoad(true); | |||
There was a problem hiding this comment.
Since this is only called 2 times directly after the constructor it would make sense to add this directly as a param to the constructor. By doing so the field could be defined as final
There was a problem hiding this comment.
Sorry for the long gap :)
Are you trying to suggest to we can overload the constructor? Something similar to:
public FXMLLoader(boolean staticLoad) {
this.staticLoad = staticLoad;
}
Since FXMLoader already has a long list of constructors, do you think this would be a good idea?
There was a problem hiding this comment.
I do not recommend adding any more constructor parameters.
If you want to take this new feature forward, it will take a fair bit of work to specify what the effect of staticLoad should be on loading fxml files. It seems like a somewhat of a SceneBuilder-specific mode, but if it has more general use then it could be something to consider. Also, there will need to be several new tests to validate the behavior of various loading scenarios when using static load, including checking what the effects are of switching between modes for successive loads. It might seem simple to "just make this method public", but there is more to it than that.
I would also want @johanvos to weigh in, since Gluon now maintains SceneBuilder.
There was a problem hiding this comment.
Thank you for the pointers.
I will try to find if this feature has use-cases outside of Scene Builder.
| @@ -39,7 +38,7 @@ | |||
| @SuppressWarnings("deprecation") | |||
| public void testStaticScriptLoad() throws IOException { | |||
| FXMLLoader fxmlLoader = new FXMLLoader(getClass().getResource("static_script_load.fxml")); | |||
kevinrushforth
changed the title
|
The static load mode is also helpful if you want to use the FXMLLoader more as a parser. For example, we use it to verify that each fxml file in our project can be parsed successfully (as a unit test), and to extract all localized strings. In these cases, the static load mode is needed. |
kevinrushforth
changed the title
|
As announced in this message, the official This sandbox repository is being retired on 1-Oct-2019. You will need to migrate your WIP pull request to the openjdk/jfx repo if you want to continue working on it. Here are instructions for migrating your pull request. The updated CONTRIBUTING.md doc has additional information on how to proceed. Once you have done this, it would be helpful to add a comment with a pointer to the new PR. NOTE: since this is a feature / enhancement request it needs to be discussed on the openjfx-dev mailing list if you want it to be considered. The new openjdk/jfx repo will be open for pull requests on Wednesday, 2-Oct-2019. I will send email to the openjfx-dev mailing list announcing this. |
This is useful for SceneBuilder to open FXML files with event handler attributes. Currently, SceneBuilder for JDK 9+ uses reflection as a workaround.