-
Notifications
You must be signed in to change notification settings - Fork 13
[#83] Rework access to preferences #83 #117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
ruspl-afed
commented
Jun 11, 2023
- Support unified way to work with both project and workspace preferences
- Provide UI to configure preferences in details
Support unified way to work with both project and worksapce preferences Provide UI to configure preferences in details Signed-off-by: Alexander Fedorov <[email protected]>
...se.cdt.lsp.clangd/OSGI-INF/org.eclipse.cdt.lsp.internal.clangd.ClangdConfigurationAccess.xml
Show resolved
Hide resolved
...ipse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/internal/clangd/ClangdPreferenceInitializer.java
Show resolved
Hide resolved
...pse.cdt.lsp.clangd/src/org/eclipse/cdt/lsp/internal/clangd/ClangdLanguageServerProvider.java
Show resolved
Hide resolved
...se.cdt.lsp.clangd/OSGI-INF/org.eclipse.cdt.lsp.internal.clangd.ClangdConfigurationAccess.xml
Show resolved
Hide resolved
...angd/src/org/eclipse/cdt/lsp/internal/clangd/editor/preferences/LspEditorPreferencePage.java
Show resolved
Hide resolved
|
||
public class LspEditorPropertiesPage extends PropertyPage { | ||
public final class LspEditorPropertiesPage extends PropertyPage { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be disabled or better hidden, when other ls provider is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, but currently we have nothing to distinguish one provider from another. Could be a a part of #81
load(); | ||
composite.setLayout(GridLayoutFactory.fillDefaults().numColumns(3).create()); | ||
composite.setLayoutData(GridDataFactory.fillDefaults().grab(true, false).create()); | ||
area = new ClangdConfigurationArea(composite, configuration.metadata()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All settings on project level make only sense, when we support one LS instance per project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH, I forgot what is blocking us from having LS instance per project. Is this still a limitation of LSP4E?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. There are two issues: To enable a single LS the boolean singleton
element in the org.eclipse.lsp4e.languageServer
extension point definition has to be set. To make this configurable, we could define a another language server which is a non-singleton. This solution needs an enable method for the ls definition, which is currently not implemented in the org.eclipse.lsp4e.languageServer
extension point, to distinguish between our singleton or non-singleton server.
Another approach would be to make the singleton
attribute settable via the org.eclipse.lsp4e.server.StreamConnectionProvider
or org.eclipse.lsp4j.services.LanguageServer
interface. This is preferred by me.
The other issue is the handling of external files. Which LS should be used for them? In most cases they have been opened by a project file via hyperlink. Here we have to track the origin file for each external file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @ruspl-afed Thank you very much. I am learning a lot!
Thank you for review @ghentschke ! |