fix: enable Scene Builder to load FXML with unresolved imports (#120, #281, #733) #576
Conversation
unresolved imports. There is also a new option (preserve unresolved imports) which also allows to keep the imports in saved FXML files.
Oliver-Loeffler
force-pushed
the
issue-120
branch
from
8f48abb to
85723a2
Compare
…of FXML with unresolved imports' as it exactly does the 2nd.
Oliver-Loeffler
changed the title
Oliver-Loeffler
changed the title
|
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? |
Oliver-Loeffler
force-pushed
the
issue-120
branch
from
1da507e to
cb6e16e
Compare
Oliver-Loeffler
changed the title
Oliver-Loeffler
force-pushed
the
issue-120
branch
from
28ac0dc to
73dddef
Compare
There was a problem hiding this comment.
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 com.gluonhq.scenebuilder.kit@24.0.0-SNAPSHOT/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.
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.
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.
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; | ||
|
|
| 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.
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); |
| if (Platform.isFxApplicationThread()) { | ||
| return callable.call(); | ||
| } else { | ||
| Platform.runLater(()->task.run()); |
There was a problem hiding this comment.
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")); | |||
| } | |||
|
|
|||
| ()->waitFor(()->new FXOMDocument(fxmlText, location, classLoader, resources, switches))); | ||
|
|
||
| if (t.getCause() != null) { | ||
| t = t.getCause(); |
There was a problem hiding this comment.
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.
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