[#245] catch all exceptions during yaml parsing #246
Conversation
- Move showErrorMessage method from LSpUtils to ClangdConfigurationFileManager since it's only used here. Despite that we do not want to publish the LspUtils when making the org.eclipse.cdt.lsp.clangd package public API.
| var status = new Status(IStatus.ERROR, ClangdPlugin.PLUGIN_ID, e.getMessage()); | ||
| var projectLocation = project.getLocation().addTrailingSeparator().toPortableString(); | ||
| LspUtils.showErrorMessage(LspEditorUiMessages.CProjectChangeMonitor_yaml_scanner_error, | ||
| LspEditorUiMessages.CProjectChangeMonitor_yaml_scanner_error_message + projectLocation | ||
| showErrorDialog(LspEditorUiMessages.ClangdConfigurationFileManager_yaml_error, |
There was a problem hiding this comment.
But it should be a headless component implementation. Why does it show any dialogs at all?
There was a problem hiding this comment.
Since this is a component, you can try to bind org.eclipse.e4.core.services.statusreporter.StatusReporter and report through it.
There was a problem hiding this comment.
But it should be a headless component implementation. Why does it show any dialogs at all?
Users can edit the the .clangd file manually to add more options. If they add failures, it will be revealed when the .clangd file gets updated.
There was a problem hiding this comment.
Since this is a component, you can try to bind
org.eclipse.e4.core.services.statusreporter.StatusReporterand report through it.
yes. I'll check that. Thanks for the hint.
There was a problem hiding this comment.
Users can edit the the .clangd file manually to add more options. If they add failures, i 8000 t will be revealed when the .clangd file gets updated.
Maybe it's better to add a listener when the file gets edited and validate it (e.g. by loading it and showing an error dialog if the load fails).
There was a problem hiding this comment.
Users can edit the the .clangd file manually to add more options. If they add failures, it will be revealed when the .clangd file gets updated.
But this is different story. If user initiated all the chain from UI, we need to throw an exception from this method and show it to the user in UI layer. In this case you don't need more services, just throw
a3bec6d to
bbc9818
Compare
- Add unit test
bbc9818 to
3a6d1ac
Compare
| } | ||
|
|
||
| private static Shell getActiveShell() { | ||
| if (PlatformUI.getWorkbench() != null && PlatformUI.getWorkbench().getActiveWorkbenchWindow() != null) |
There was a problem hiding this comment.
PlatformUI.getWorkbench() != null
This code will not work as expected for not initialized workbench
But the issue is different: please avoid static calls from dynamic context, there will be no good. If you hrdly need workbench - please bind it as a service. However, I believe this service does not need UI, see my comment above.
There was a problem hiding this comment.
However, I believe this service does not need UI
When I think about it, I have to agree with you. Check if the user edits the .clangd file properly should be done somewhere else.
- remove error dialog since it does not belong to the ClangdConfigurationFileManager. The check, if the user edits the .clangd file properly should be done somewhere else.
- do not set database path in .clangd file with invalid yaml syntax. The user has to fix it first.
|
@ruspl-afed thank you for the very constructive review! |
Unfortunately there is no UI "call site" since it's an event handler call. That's why I opened #247 . |
- Move showErrorMessage method from LSpUtils to ClangdConfigurationFileManager since it's only used here. Despite that we do not want to publish the LspUtils when making the org.eclipse.cdt.lsp.clangd package public API.Do not set compilation database path in
.clangdfile with invalid yaml syntax.fixes #245