search

eclipse-cdt / cdt-lsp

Eclipse CDT™ LSP Extensions for CDT
Eclipse Public License 2.0
23 stars 11 forks source link

Improve restarting mechanism for compile database #302

Closed ddscharfe closed 2 months ago

ddscharfe commented 2 months ago

In #23 we implemented a mechanism for restarting the language servers when the compile database changes (see org.eclipse.cdt.lsp.internal.clangd.editor.CompileCommandsMonitor).

The current implementation restarts all language servers if any open editor has a cpp file set as input, which is inside the same project as the changed compile database.

This works for text editors, but not for MultiPageEditorParts, which contain one or multiple language server based text editors. Additionally, if there are multiple language servers running, all language servers will always be restarted.

I think a better solution would be to just restart the language servers for the affected projects.

Note: AFAIK there is no API for getting the LanguageServerWrappers for an IProject in lsp4e. However by using LanguageServerProjectExecutor::forProject it is possible to get the LanguageServerWrappers via LanguageServerProjectExecutor::computeAll. @ghentschke do you know a better solution for this?

ghentschke commented 2 months ago

I am not sure if we need that, because currently we support only a single C/C++ language server instance. Or do you mean that all running language server and not only our will be restarted and there is no content type filter to prevent that?

ddscharfe commented 2 months ago

As far as I understand, this would be the case, because we restart all servers without filtering. The more important use case for me are MultiPageEditorParts with embedded text editors, which don't work with the current implementation.

ghentschke commented 2 months ago

I think a better solution would be to just restart the language servers for the affected projects.

Yes, that would be better.

Additionally, if there are multiple language servers running, all language servers will always be restarted.

We already restart only our own LS, because we are filtering for them in org.eclipse.cdt.lsp.util.LspUtils.getLanguageServers():

    public static Stream<LanguageServerWrapper> getLanguageServers() {
        return LanguageServiceAccessor.getStartedWrappers(null, true).stream()
                .filter(w -> "org.eclipse.cdt.lsp.server".equals(w.serverDefinition.id)); //$NON-NLS-1$
    }
ghentschke commented 2 months ago

Since we are using a single LS for the entire workspace I don't think we need to filter for LS for a given project.

ddscharfe commented 2 months ago

Additionally, if there are multiple language servers running, all language servers will always be restarted.

We already restart only our own LS, because we are filtering for them in org.eclipse.cdt.lsp.util.LspUtils.getLanguageServers():

Right, I overlooked this.

Since we are using a single LS for the entire workspace I don't think we need to filter for LS for a given project.

The main issue for me is that the current solution doesn't work for editors in a MultiPageEditorPart.

ghentschke commented 2 months ago

The main issue for me is that the current solution doesn't work for editors in a MultiPageEditorPart.

Okay, can you provide a PR for this?

ddscharfe commented 2 months ago

The main issue for me is that the current solution doesn't work for editors in a MultiPageEditorPart.

Okay, can you provide a PR for this?

Sure. What solution do you prefer: 1) Always restart the cdt language server(s) if any compile db changes. This would be the simplest solution. 2) Only restart the cdt language server(s) for the affected projects. I found a way to get the affected language server wrappers for a project, but it uses restricted API of lsp4e. I am also not sure if we should analyze the open text editors, which the current implementation does, or if we just restart the language server(s) regardless if any open editor is affected.

ghentschke commented 2 months ago

I am also not sure if we should analyze the open text editors, which the current implementation does, or if we just restart the language server(s) regardless if any open editor is affected.

I think an immediate restart would be fine, because a running LS implies that its used by an editor or any view provider like type hierarchy, fetching child elements of a translation unit in the project explorer view, ... So I think always restart the cdt language server(s) if any compile db changes would be fine.