When loading of FXML file fails, in some cases the error is not shown to the user #403
Conversation
…s during FXML loading.
…oad() method in case of unknown cause.
Oliver-Loeffler
force-pushed
the
issue-402
branch
from
42f17b1 to
f6d2a68
Compare
kit/src/test/resources/com/oracle/javafx/scenebuilder/kit/fxom/BrokenByUserData.fxml
Outdated
Show resolved
Hide resolved
| String message = t.getMessage(); | ||
| assertTrue(message.startsWith("javafx.fxml.LoadException:")); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we add a few more test cases to assert both known and unknown causes when loading a FXML? May be also assert a case where XMLStreamException is thrown.
There was a problem hiding this comment.
Yes, of course. This is a good point.
There was a problem hiding this comment.
Hi @Oliver-Loeffler,
Are you going to add some more test cases to move this PR forward?
There was a problem hiding this comment.
Yes, there will be an update with more tests.
There was a problem hiding this comment.
Hi @abhinayagarwal
There is a significant overlap with PR #405 hence I am waiting for your feedback regarding #405 before continuing here. When #405 becomes accepted, I'll rebase towards 405 and continue from there.
There was a problem hiding this comment.
There is also now a case for a SAXParseException which is wrapped as cause in a IOException. This is the case when it is attempted to load and incomplete or broken XML structure.
There was a problem hiding this comment.
@abhinayagarwal Eventually it worked and now I have a case where the XMLStreamException is provoked and handled.
Oliver-Loeffler
force-pushed
the
issue-402
branch
from
c698ac9 to
656f1f5
Compare
|
@AlmasB If you like we can start with this one. Would be glad if you'd have an eye on this one. We have many older (some newer) issues related to FXML/FXOM loading. This PR is reviewed by @abhinayagarwal . Goal here is at first, that Scene Builder no longer silently ignores exceptions. There is actually a mechanism which displays the Exception contents to the user but this was not triggered in past. So users complained about FXMLs which failed to load. |
| @@ -668,7 +668,7 @@ private void performOpenFiles(List<File> fxmlFiles, | |||
| hostWindow.loadFromFile(fxmlFile); | |||
| hostWindow.openWindow(); | |||
| } | |||
| } catch (IOException xx) { | |||
| } catch (RuntimeException | IOException xx) { | |||
There was a problem hiding this comment.
Is there a reason not to use Exception superclass here, i.e. why Runtime and IO only?
There was a problem hiding this comment.
Very good point, yes, you are absolutely right. This a leftover from searching what was happening. Catching any kind of exception is a better solution here. The map where exceptions are collected in is already suitable for all types of exceptions.
There was a problem hiding this comment.
If you wish to update this, happy to wait. Otherwise, it looks good to go.
| } | ||
|
|
||
| private void handleUnknownAndMissingCauses(Exception x) throws IOException { | ||
| throw new IOException(x); |
There was a problem hiding this comment.
I assume throwing this is fine as it will be caught by the SB general exception handler?
There was a problem hiding this comment.
Yes, this is handled in a proper way. The issue is shown to the user and a dump of the FXML file into a temp dir is performed. There is a proper exception handling downstream.
When loading of FXML file fails, in some cases the error is not shown to the user.
This is actually the case for #253.
The error detection in
SceneBuilderApponly worked for cases ofIOExceptions.This case is caused by a
NullPointerExceptioncoming fromFXMLLoaderinsideFXOMLoader.I cannot solve the issue #253 now but consider this behavior, that no error dialog is presented, as a bug.
Issue
Fixes #402
Related #327
Related #253
Related #54
(there are more very similar issues)
Progress