Skip to content
This repository has been archived by the owner on Jun 24, 2021. It is now read-only.

WIP: JDK-8186429 FXMLLoader setStaticLoad should be public API #44

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

WIP: JDK-8186429 FXMLLoader setStaticLoad should be public API #44

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 0 additions & 59 deletions modules/javafx.fxml/src/main/java/com/sun/javafx/fxml/FXMLLoaderHelper.java
View file
Open in desktop

This file was deleted.

25 changes: 9 additions & 16 deletions modules/javafx.fxml/src/main/java/javafx/fxml/FXMLLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@
import com.sun.javafx.fxml.expression.ExpressionValue;
import com.sun.javafx.fxml.expression.KeyPath;
import static com.sun.javafx.FXPermissions.MODIFY_FXML_CLASS_LOADER_PERMISSION;
import com.sun.javafx.fxml.FXMLLoaderHelper;
import com.sun.javafx.fxml.MethodHelper;
import java.net.MalformedURLException;
import java.security.AccessController;
Expand Down Expand Up @@ -312,7 +311,7 @@ public void processPropertyAttribute(Attribute attribute) throws IOException {
throw constructLoadException("Cannot bind to builder property.");
}

if (!isStaticLoad()) {
if (!staticLoad) {
value = value.substring(BINDING_EXPRESSION_PREFIX.length(),
value.length() - 1);
expression = Expression.valueOf(value);
Expand Down Expand Up @@ -2052,13 +2051,6 @@ public String run() {
return System.getProperty("javafx.version");
}
});

FXMLLoaderHelper.setFXMLLoaderAccessor(new FXMLLoaderHelper.FXMLLoaderAccessor() {
@Override
public void setStaticLoad(FXMLLoader fxmlLoader, boolean staticLoad) {
fxmlLoader.setStaticLoad(staticLoad);
}
});
}

/**
Expand Down Expand Up @@ -2378,21 +2370,22 @@ public void setClassLoader(ClassLoader classLoader) {
clearImports();
}

/*
/**
* Returns the static load flag.
*
* @since 11
*/
boolean isStaticLoad() {
// SB-dependency: RT-21226 has been filed to track this
public final boolean isStaticLoad() {
return staticLoad;
}

/*
/**
* Sets the static load flag.
*
* @param staticLoad
* @param staticLoad The value for the static load flag
* @since 11
*/
void setStaticLoad(boolean staticLoad) {
// SB-dependency: RT-21226 has been filed to track this
public final void setStaticLoad(boolean staticLoad) {
this.staticLoad = staticLoad;
}

Expand Down
3 changes: 1 addition & 2 deletions modules/javafx.fxml/src/test/java/test/javafx/fxml/FXMLLoader_ScriptTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
* questions.
*/

import com.sun.javafx.fxml.FXMLLoaderHelper;
import org.junit.Test;

import java.io.IOException;
Expand All @@ -39,7 +38,7 @@ public class FXMLLoader_ScriptTest {
@SuppressWarnings("deprecation")
public void testStaticScriptLoad() throws IOException {
FXMLLoader fxmlLoader = new FXMLLoader(getClass().getResource("static_script_load.fxml"));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final FXMLLoader fxmlLoader

FXMLLoaderHelper.setStaticLoad(fxmlLoader, true);
fxmlLoader.setStaticLoad(true);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the pointers.

I will try to find if this feature has use-cases outside of Scene Builder.

AtomicBoolean scriptCalled = new AtomicBoolean();
AtomicBoolean scriptEndCalled = new AtomicBoolean();
fxmlLoader.setLoadListener(new LoadListener() {
Expand Down
3 changes: 1 addition & 2 deletions modules/javafx.fxml/src/test/java/test/javafx/fxml/RT_18218Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@

package test.javafx.fxml;

import com.sun.javafx.fxml.FXMLLoaderHelper;
import java.io.IOException;
import java.util.Map;
import javafx.fxml.FXMLLoader;
Expand All @@ -39,7 +38,7 @@ public class RT_18218Test {
@SuppressWarnings({"unchecked", "deprecation"})
public void testStaticScriptLoad() throws IOException {
FXMLLoader fxmlLoader = new FXMLLoader(getClass().getResource("rt_18218.fxml"));
FXMLLoaderHelper.setStaticLoad(fxmlLoader, true);
fxmlLoader.setStaticLoad(true);
fxmlLoader.setLoadListener(new LoadListener() {
private String unknownStaticPropertyElementName = null;

Expand Down