[#394] Add clangd console view for log data #395
Conversation
- the logging can be enabled in the clangd preference page fixes eclipse-cdt#394
because at startup we have no project info available. At startup time we need to know if the stderr should be piped to the console.
| // destroy LS process first, to prevent a write operation on a already closed output stream: | ||
| super.stop(); | ||
| // then close output stream. | ||
| // TODO: we need to wait here until the process has been terminated: |
There was a problem hiding this comment.
We need to fix this in LSP4E. See eclipse-lsp4e/lsp4e#1194
There was a problem hiding this comment.
Sorry for the late review @ghentschke , I was out of connection for some time. Please have a look at my comments
| @@ -15,7 +15,7 @@ | |||
| import org.eclipse.core.resources.IProject; | |||
|
|
|||
| /** | |||
| * @since 2.1 | |||
There was a problem hiding this comment.
Are you sure this increment was necessary?
There was a problem hiding this comment.
Yes, otherwise it hadn't build
There was a problem hiding this comment.
Very strange. Does this mean that we had this interface for 2.1 already and now we need to redeclare the same interface for 2.2 as well? Or do we have a baseline configuration issue?
There was a problem hiding this comment.
Or do we have a baseline configuration issue?
Yes it didn't build because of that:
I was wondering as well and hadn't had the time for further investigations.
| .getExtensionPoint(EXTENSION_ID).getConfigurationElements()) { | ||
| if (LOGGER_ELEMENT.equals(configurationElement.getName())) { | ||
| return Optional | ||
| .ofNullable((ILogProvider) getInstanceFromExtension(configurationElement, ILogProvider.class)); |
There was a problem hiding this comment.
If we have multiple extensions, then the actual log provider becomes difficult to predict. Do we really need to introduce extension point for logger?
There was a problem hiding this comment.
You're having a point here. My thought was that this extension point is not intended to be used by external bundles as well (if so, we can still refactor) and I favor extension point mechanism over OSGi services.
There was a problem hiding this comment.
..but when I think about the use case, a service provider would may be better?
There was a problem hiding this comment.
I understand that you would prefer to have extension point here. But unfortunately, we are unlikely to be able to do two things:
- declare extension point as private
- declare extension point as
singleton
But before searching for way how to solve these issues I need to ask: do we already have a need to replace your default implementation of console by something else?
There was a problem hiding this comment.
do we already have a need to replace your default implementation of console by something else?
The use case I can think about is a vendor who want to connect their own console to the clangd stderr output.
But I am currently thinking about a refactoring of this part, because I want to allow that the data from clangd stderr could be written onto several output streams in the AsyncStreamPipe.
Despite the use case above, I had the idea of a temporary parsing of stderr for clang-format failures when a user wants to format a source file. (See #400)
bundles/org.eclipse.cdt.lsp/src/org/eclipse/cdt/lsp/internal/server/AsyncStreamPipe.java
Show resolved
Hide resolved
fixes #394