-
Notifications
You must be signed in to change notification settings - Fork 221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: enable Scene Builder to load FXML with unresolved imports (#120, #281, #733) #576
base: master
Are you sure you want to change the base?
Conversation
unresolved imports. There is also a new option (preserve unresolved imports) which also allows to keep the imports in saved FXML files.
8f48abb
to
85723a2
Compare
…of FXML with unresolved imports' as it exactly does the 2nd.
This one no longer works as it used to. I'll review and rework accordingly. Loading FXML files with unresolvable imports works. Saving does not work anymore. I'll also add example FXMLs and tests for the FXOM. |
…nresolvable types declared as import.
…eleting elements. Element browsing is not working.
…e reviewed and identified.
…repeated exceptions.
@abhinayagarwal or @AlmasB Could you please review this one? The idea is:
Interesting would be your opinion to see if this approach is helpful and maintainable. Thanks! |
Hi @abhinayagarwal, do you have any chance to review this one again? |
1da507e
to
cb6e16e
Compare
28ac0dc
to
73dddef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good! Great contribution.
I've left a few comments.
Note also that you might need to disable the Preview
option if there are unresolved imports, otherwise it fails with a RTE:
java.lang.RuntimeException: Bug in PreviewWindowController::requestUpdate
at [email protected]/com.oracle.javafx.scenebuilder.kit.preview.PreviewWindowController$1.lambda$run$1(PreviewWindowController.java:269)
public static FXOMDocumentSwitch[] combined(FXOMDocumentSwitch[] existingSwitches, | ||
FXOMDocumentSwitch newSwitch) { | ||
Set<FXOMDocumentSwitch> options = EnumSet.of(FXOMDocumentSwitch.NORMALIZED); | ||
options.addAll(Set.of(existingSwitches)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or else, newSwitch
is unused, and you are missing options.add(newSwitch);
?
if (toggle) { | ||
return new FXOMDocumentSwitch[] {this}; | ||
} else { | ||
return new FXOMDocumentSwitch[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe use new FXOMDocumentSwitch[] {}
?
*/ | ||
public static FXOMDocumentSwitch[] combined(FXOMDocumentSwitch[] existingSwitches, | ||
FXOMDocumentSwitch newSwitch) { | ||
Set<FXOMDocumentSwitch> options = EnumSet.of(FXOMDocumentSwitch.NORMALIZED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you include NORMALIZED
here?
In EditorController you call: FXOMDocumentSwitch.combined(switches, FXOMDocumentSwitch.NORMALIZED);
Do you mean Set<FXOMDocumentSwitch> options = EnumSet.of(newSwitch);
?
|
||
private boolean hasControlsFromExternalPlugin; | ||
|
||
private List<Class<?>> initialDeclaredClasses; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unneeded extra line
this.glue = new GlueDocument(fxmlText); | ||
this.location = location; | ||
this.classLoader = classLoader; | ||
this.resources = resources; | ||
initialDeclaredClasses = new ArrayList<>(); | ||
this.initialDeclaredClasses = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unneeded use of this
here
for (Entry<String,Pattern> entry : typesToRemove.entrySet()) { | ||
var pattern = entry.getValue().matcher(lineToTest); | ||
if (pattern.matches()) { | ||
LOGGER.log(Level.INFO, "Import to ignore: {0}", lineToTest); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lower to Level.FINE
?
if (Platform.isFxApplicationThread()) { | ||
return callable.call(); | ||
} else { | ||
Platform.runLater(()->task.run()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Platform.runLater(task);
@@ -281,14 +282,61 @@ public void that_missing_imports_during_defines_resolution_cause_exception() thr | |||
assertEquals(IllegalStateException.class, t.getClass()); | |||
assertTrue(t.getMessage().contains("Bug in FXOMRefresher")); | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unneeded extra line
()->waitFor(()->new FXOMDocument(fxmlText, location, classLoader, resources, switches))); | ||
|
||
if (t.getCause() != null) { | ||
t = t.getCause(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused, missing an assertion?
errorDialog.setDebugInfo(I18N.getString("alert.open.failure.unresolved.imports.advice", | ||
allMissing, missingTypes.size())); | ||
errorDialog.setTitle(I18N.getString("alert.title.open") + ": " + fxmlFile.getName()); | ||
errorDialog.showAndWait(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: add a button to the dialog with an Open FXML/JAR Manager
option, so the user can add the missing dependency.
Thank you for the feedback @jperedadnr, I'll look into and rework accordingly. |
This fix allows Scene Builder to load FXML files with unresolved imports.
There is also a new option (keep unresolved imports) which also allows to keep the imports in saved FXML files. This can be toggled in Scene Builder preferences dialog.
If disabled, Scene Builder will effectively omit those unresolved imports and remove them from the updated FXML to be written.
If enabled, Scene Builder will keep track of unresolved imports and write them (unmodified) into the updated FXML. The import remains as it was declared in the original FXML file. Optimization is not applied to unresolved imports.
Of course, one cannot make use of unresolved imports and edit these controls. But, one can at least open an FXML file, edit and rework it in parts. Also, Scene Builder will inform the user that there is something wrong with the imports - it will not silently ignore the issue. This is quite helpful with foreign projects where users only want to look into the files and get a feeling of how the UI would look like.
This PR also increments Maven-Surefire version from 3.0.0-M5 to 3.5.0 in order to mitigate module system related errors during test.
Issue
Fixes #120
Fixes #281
Fixes #733 (added on 2024-09-29)
Progress