-
Notifications
You must be signed in to change notification settings - Fork 54
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
Move findReferences in LSPEclipseUtils #282
Conversation
Here a draft PR in WWD side eclipse-wildwebdeveloper/wildwebdeveloper#942 which uses this utility class at https://github.com/eclipse/wildwebdeveloper/pull/942/files#diff-87df74520a640db8f0ba722477b478c6c3910e5ff9ceb22ad1f246c95b4a8655R34 |
To see the result with XML references in WWD, see eclipse-wildwebdeveloper/wildwebdeveloper#942 (comment) |
f87d3a7
to
0d0da5d
Compare
if (languageServers.isEmpty()) { | ||
return; | ||
} | ||
LanguageServer ls = languageServers.get(0); |
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.
I realize you are just moving the code, but is the general pattern not to loop over all LSs, why take here just the first one?
I'm not found of the idea of crowding LSPEclipseUtils with "functional" units; it's mostly a set of utils to translate LSP<->Eclipse elements and actions; not a util class that's meant at orchestrating the Language Servers. So I would rather see the |
It is exactly that I tried to explain here eclipse-wildwebdeveloper/wildwebdeveloper#644 (comment) in otherwords standardize some client command because I use a custom structure uri as first argument and line/character at the second arguments. If you wish to provide that in LSP4e it means other LSP server should use the same structure, it was the reason why I wanted to standardize those client command at microsoft/vscode-languageserver-node#495 (comment) |
OK, I didn't understand the grain of action. But now we're a bit forward and the goal and proposal is clearer, I think it's indeed worth adding the handlers in LSP4E for such standard structures and commands. |
IMHO I think LSP should specify standard command ID like I tried to define <extension
point="org.eclipse.lsp4e.languageServer">
<server>
...
</server>
<commandMapping
standardCommandId="lsp4e.showReferences"
commandId="xml.show.references"> |
@mickaelistria do you like the idea with commandMapping extension point? If you like it I could experiment it. |
I'm fine with LSP4E defining an Eclipse command or a handler for such common operation. |
I'm curious: does VSCode has the concept of generic and reusable commands? Doesn't it define a common findreference command? If yes, we could just have LSP4E adopting the same ids as VSCode and try to have lemminx using those common command ids. If not, I see 2 other good solutions:
My favorite is 1. |
I fear it will take some time to do that, and I need this feature for WWD.
It will work only with LemMinx and not with another LS language server which could not provide this feature.
Indeed vscode defines some commands https://vscode-docs.readthedocs.io/en/latest/extensionAPI/vscode-api-commands/ and for references it defines I think we could follow the same idea (I don't know if it is possible):
Once Platform will define a command mapping, WWD could use it and remove the |
LSP4E already defines a handler for org.eclipse.ui.genericeditor.findReferences command.
We can try that for the moment. Wild Web Developer can create a "delegating" handler for its specific command, using command and handler services to retrieve the org.eclipse.ui.gemericeditor.findReferences command/handler and implement enablement/execution by delegating to it. |
The initial target branch |
Move findReferences in LSPEclipseUtils
This PR provide the capability to move the search LSP references in the LSPEclipseUtils in order to use it in another plugin / component.
I need that to fix eclipse-wildwebdeveloper/wildwebdeveloper#644 in order to execute the LSP search references from CodeLens with XML language server.