Skip to content
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

Rework access to preferences #83

Closed
ruspl-afed opened this issue May 17, 2023 · 14 comments
Closed

Rework access to preferences #83

ruspl-afed opened this issue May 17, 2023 · 14 comments
Assignees
Milestone

Comments

@ruspl-afed
Copy link
Member

Currently preferences are located in org.eclipse.cdt.lsp.clangd UI part, while some of them could be

So , the preferences should be:

  • split to have general and 'clangd' items
  • reworked from UI to headless manner
@ruspl-afed ruspl-afed self-assigned this May 17, 2023
ruspl-afed added a commit to ruspl-afed/eclipse-cdt-lsp that referenced this issue May 17, 2023
…editor.ui"

All except PLUGIN_ID itself are reworked
PLUGIN_ID rework will follow together with preference rework, see
eclipse-cdt#83

Required for eclipse-cdt#77
@ghentschke
Copy link
Contributor

ghentschke commented May 17, 2023

I would like to introduce a requirement for the enable preference:
As a vendor I would like to have the possibility to hide the "Prefer C/C++ Editor (LSP)" check box in the preferences and on project level, since I have my own enable logic.

Not sure if this is part of this issue. It may be helpful to keep in mind while working on this issue here.

ghentschke pushed a commit that referenced this issue May 17, 2023
All except PLUGIN_ID itself are reworked
PLUGIN_ID rework will follow together with preference rework, see
#83

Required for #77
@ruspl-afed
Copy link
Member Author

to hide the "Prefer C/C++ Editor (LSP)" check box in the preferences and on project level, since I have my own enable logic

Implementing "hide" and "own enable logic" are different aspects, and I can see two paths here:

  1. We can say that generis "Prefer LSP" is a prerequisite for all the further logic provided by vendor, in this case we don't need to hide this generic UI.
  2. We can try to make both UI and logic fully replaceable, this will need a bit more effort.

WDYT?

@ghentschke
Copy link
Contributor

ghentschke commented May 17, 2023

We can say that generis "Prefer LSP" is a prerequisite for all the further logic provided by vendor, in this case we don't need to hide this generic UI.

I don't like a general switch here, because as a vendor I don't want to give the user the possibility to disable LS based editor for some projects types.

We can try to make both UI and logic fully replaceable, this will need a bit more effort.

That would be great and makes the whole thing more flexible.

@ddscharfe
Copy link
Contributor

@ruspl-afed, @ghentschke, is any of you already working on this? I'd like to implement #81 which depends on this issue.

My quick attempt was to create my own ICLanguageServerProvider which derives from CdtCLanguageServerProvider and set its priority higher than the default one. However when the providers are created in the CLanguageServerRegistry, my provider gets created first and therefore sets the preferenceStore defaults, the default provider is created second and overwrites the preferenceStore defaults.

@ruspl-afed
Copy link
Member Author

, is any of you already working on this?

not yet, I was busy on Mylyn and on paid tasks

and set its priority higher than the default one

The idea of "priority" is questionable for me, especially in the form of enum - one can just declare highest and this is the end of the game for others.

my provider gets created first and therefore sets the preferenceStore defaults, the default provider is created second and overwrites the preferenceStore defaults.

yes, this is the one of key points to rework. As discussed above, there is no preferences to share between LS providers, every LS provider should configure itself.

@ghentschke
Copy link
Contributor

, is any of you already working on this?

me neither.

The idea of "priority" is questionable for me, especially in the form of enum - one can just declare highest and this is the end of the game for others.

That's the reason why I designed it this way. As a vendor I want to overwrite everybody else.

yes, this is the one of key points to rework. #83 (comment), there is no preferences to share between LS providers, every LS provider should configure itself.

I agree.

@ghentschke
Copy link
Contributor

@ruspl-afed could you provide a PR for this issue? If not I can do this, when I am done with #87

@ruspl-afed
Copy link
Member Author

That's the reason why I designed it this way. As a vendor I want to overwrite everybody else.

I see. If you need to restrict some functionality please consider Eclipse Passage, it was designed to solve this kind of problems.

@ruspl-afed could you provide a PR for this issue?

I plan to work on it in the coming days

@ghentschke
Copy link
Contributor

my provider gets created first and therefore sets the preferenceStore defaults, the default provider is created second and overwrites the preferenceStore defaults.

yes, this is the one of key points to rework. #83 (comment), there is no preferences to share between LS providers, every LS provider should configure itself.

That's why vendors should extend org.eclipse.cdt.lsp.server.DefaultCLanguageServerProvider. I should move this to the clangd plugin as well, since it is only applicable for clangd and rename it to DefaultClangdServerProvider. CdtCLanguageServerProvider should be tagged as final, since it should not be extended by vendors.

@ruspl-afed
Copy link
Member Author

If the type is for extension, can we consider to make it abstract and start its name from Base... instead of Default...?

@ruspl-afed
Copy link
Member Author

In progress...

@ruspl-afed
Copy link
Member Author

Have a progress with aligning workspace and project preferences storage and UI, but ICLanguageServerProvider API may need deeper rework to catch the "project context" in time, since currently it is not designed to understand the enclosing IProject and we are using hacks around property testers to understand at least something.
image

image

@ghentschke
Copy link
Contributor

Looks nice! Good job. Thank you @ruspl-afed

ruspl-afed added a commit to ruspl-afed/eclipse-cdt-lsp that referenced this issue Jun 11, 2023
Support unified way to work with both project and worksapce preferences
Provide UI to configure preferences in details

Signed-off-by: Alexander Fedorov <alexander.fedorov@arsysop.ru>
ruspl-afed added a commit to ruspl-afed/eclipse-cdt-lsp that referenced this issue Jun 11, 2023
Support unified way to work with both project and worksapce preferences
Provide UI to configure preferences in details

Signed-off-by: Alexander Fedorov <alexander.fedorov@arsysop.ru>
@ruspl-afed
Copy link
Member Author

done with #117

ruspl-afed added a commit that referenced this issue Jun 12, 2023
Support unified way to work with both project and worksapce preferences
Provide UI to configure preferences in details

Signed-off-by: Alexander Fedorov <alexander.fedorov@arsysop.ru>
@jonahgraham jonahgraham added this to the 1.0.0 milestone Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants