Support for 'xml/executeClientCommand' protocol extension #543

BoykoAlex · 2020-10-08T14:24:00Z

Support for handling xml/executeClientCommand request.
First attempt to find a matching command registered by one of the Language Servers.
Try to execute eclipse registered command if no LS command found.

A good chunk of client command handling logic (no LS registered command case) taken from LSP4E... Perhaps it'd make sense to make API for this in LSP4E...

Server side PR: eclipse-lemminx/lemminx#905

Signed-off-by: BoykoAlex <[email protected]>

mickaelistria · 2020-10-13T20:26:08Z

...ipse.wildwebdeveloper.xml/src/org/eclipse/wildwebdeveloper/xml/internal/CommandExecutor.java

@@ -0,0 +1,168 @@
+package org.eclipse.wildwebdeveloper.xml.internal;


Please add EPLv2 copyright header.

mickaelistria · 2020-10-13T20:30:11Z

...ipse.wildwebdeveloper.xml/src/org/eclipse/wildwebdeveloper/xml/internal/CommandExecutor.java

+	private static final String LSP_COMMAND_PARAMETER_TYPE_ID = "org.eclipse.lsp4e.commandParameterType"; //$NON-NLS-1$
+	private static final String LSP_PATH_PARAMETER_TYPE_ID = "org.eclipse.lsp4e.pathParameterType"; //$NON-NLS-1$
+
+	public CompletableFuture<Object> executeClientCommand(String id, Object... params) {


It looks like all that should be placed in LSP4E, and even that LSP4E does already have such code available. We should instead investigate a better way to expose the LSP4E command handling as API and consume it here.

Alex is a bit busy so I will continue to push this forwards. I just created a gerrit patch merging in this code with the lsp4e 'CommandExecutor'.

See: https://git.eclipse.org/r/c/lsp4e/lsp4e/+/170859

I suggest we start by discussion / resolving that patch via gerrit. And once that is done I can then amend this PR here to remove the obsolete wild web copy of CommandExecutor and use the one from lsp4e instead.

Note: you will see that my patch exports internal package. This is probably not what we want in the end. But I wanted to avoid moving the CommandExecutor class to another package as that would make it a lot harder to see what was changed in this class. So I think its better we review the change details first, and once we are agreed they are fine, we can then discuss / decide how to expose the api publically.

okay so I just updated the PR. The CommandExecutor has been removed and now using the lsp4e CommandExecutor instead.

mickaelistria · 2020-10-22T12:23:29Z

@BoykoAlex @kdvolder While I'm not fundamentally against that change, I believe thas such executeClientCommand could be standardized in LSP directly as I imagine plenty of use-cases for it, independently on the language. Is there already a request about it in LSP backlog? I'd like to make sure the idea is tracked at the best place before merging some local workarounds and extensions.

kdvolder · 2020-10-22T16:28:45Z

There's a couple of related LSP issue tickets:

Execute client side command microsoft/language-server-protocol#430
Clarify the intension of how commands are defined microsoft/language-server-protocol#432
What's the correct way for an LSP server to know whether a command is available on the client? microsoft/language-server-protocol#642

The first one is probably the most closely related, one part of what it needs/asks for is a mechanism for server to request client to execute a command.

The second one has a discussion around whether commands live in a 'shared/global' name space or whether the commands belong only to a specific client<->server session.

With a shared namespace one server could define a command and another one could use it (e.g. in code action, or also in a potential executeClientCommand request. With 'per session namespace' this wouldn't be possible.

The third one is about how server could know what commands 'exist' on the client. (I.e. kind of the inverse of how a server advertises commands as a capability so client can execute them).

I'm not sure how realistic it is to get this resolved in LSP protocol in the short term, which is why it may be best to move forward with custom protocol extensions such as 'xml/executeClientCommand' at this time. This is not ideal of course, but lacking consensus on how to handle it in the LSP itself, we may not have much choice.

kdvolder · 2020-10-22T16:58:14Z

BTW: Another option might be to have a 'executeClientCommand' provided by lsp4e. That way at least different eclipse based clients wouldn't all need to implement their own version of it.

mickaelistria · 2020-10-22T17:03:11Z

There's a couple of related LSP issue tickets

Interesting, however none seems easily actionable, and it looks that they are more brainstorming than actual concrete proposals. Can you please open one about "Introduce a client/executeCommand server->client operation" ? Since you have mostly implemented it for LemMinX already, this proposal might be easily actionable.
While this wouldn't answer every question asked, it would be a great starting point.

I'm not sure how realistic it is to get this resolved in LSP protocol in the short term,

I agree.

This is not ideal of course, but lacking consensus on how to handle it in the LSP itself, we may not have much choice.

I think that in parallel of just making things work here for XML, it's just time to open finer grain issues and maybe even PRs to the LSP spec to handle that.

BTW: Another option might be to have a 'executeClientCommand' provided by lsp4e

I already don't like a lot the idea of specific operations in Wild Web Developer, but I like much less the idea of placing them in LSP4E ;)
I may seem annoying here, but our goal isn't only to make 1 use-case work, it's also to improve the overall IDE domain and LSP directly. So I'm trying to enforce that before adding what I perceive as a workaround which is bringing back the N*M integration cost; we make a decent proposal in LSP to at least have some chances that this workaround can be replaced by something more standard and cost N+M in the future.

kdvolder · 2020-10-22T17:42:26Z

I already don't like a lot the idea of specific operations in Wild Web Developer, but I like much less the idea of placing them in LSP4E ;)

I understand, but... in a way putting into lsp4e is one small step closer to making it 'standardized'. So to me this would be better than having every individual Eclipse based client have their own xml/ExecuteClientCommand, sts/executeClientCommand, java/executeClientCommand etc. This is what the current PRs are driving towards. xml is now the third example (that I know of) where we want this kind of thing.

But I totally agree putting non-standard protocol extension in lsp4e vs putting it in xml, java and spring boot individually, neither is great. The best solution is this be standard part of lsp spec (and then it can be part of lsp4e as well as every other client library that cares to implement the spec faifully).

Anyhow, yes, I think it is probably time to create that LSP ticket to request a protocol extension and see what traction it gets. At least we can write up a bit more carefully what we think this extension should do, and then maybe we implement it in lsp4e accordingly.

I'll try and get round to this in the next couple of days.

kdvolder · 2020-11-06T00:15:39Z

I've made the requested change to use CommandExecutor from lsp4e. However, jenkins doesn't like it:

[ERROR] /home/jenkins/agent/workspace/Wildwebdeveloper_PR-543/org.eclipse.wildwebdeveloper.xml/src/org/eclipse/wildwebdeveloper/xml/internal/XmlLanguageClientImpl.java:[6] 
[ERROR] 	import org.eclipse.lsp4e.command.CommandExecutor;
[ERROR] 	       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[ERROR] The import org.eclipse.lsp4e.command.CommandExecutor cannot be resolved
[ERROR] /home/jenkins/agent/workspace/Wildwebdeveloper_PR-543/org.eclipse.wildwebdeveloper.xml/src/org/eclipse/wildwebdeveloper/xml/internal/XmlLanguageClientImpl.java:[17] 
[ERROR] 	return CommandExecutor.executeCommand(cmd, null, null);

Seems like it isn't there? I beleave that @mickaelistria did already merge that change into lsp4e. See: https://git.eclipse.org/r/c/lsp4e/lsp4e/+/170859

So... can someone help figure out why the build isn't happy?

Signed-off-by: Kris De Volder <[email protected]>

mickaelistria · 2020-11-06T09:41:37Z

Thanks Kris.
We'd need to move Wild Web Developer to the snapshots build of LSP4E for this to work. However, it's not really desirable to move to snapshots at the moment since we'd like to release Wild Web Developer soon-ish.
The best path forward seems to be that we 1st release LSP4E with this change; and then upgrade Wild Web Developer to this new LSP4E release so this method becomes visible.
Would that work for you? If so, please send a note to lsp4e-dev mailing-list to request for a release and we'll make it happen unless someone shares good reasons not to.

Note that another possibility would be that you manage to implement support for the client/executeCommand according to your LSP proposal directly in LSP4J and get it released soon; then we can work towards support for it in LSP4E, and in your language server and no change would then be required in Wild Web Developer.

kdvolder · 2020-11-06T17:45:24Z

However, it's not really desirable to move to snapshots at the moment since we'd like to release Wild Web Developer soon-ish.

Okay, no problem.

The best path forward seems to be that we 1st release LSP4E with this change; and then upgrade Wild Web Developer to this new LSP4E release so this method becomes visible.
Would that work for you?

Sure. I'll post a message to lsp4edev list shortly as you suggest.

Note that another possibility would be that you manage to implement support for the client/executeCommand according to your LSP proposal directly in LSP4J and get it released soon; then we can work towards support for it in LSP4E, and in your language server and no change would then be required in Wild Web Developer.

I was assuming we keep pursuing the LSP protocol PR independently in parallel. Also I don't see how it would solve the 'we don't want to use snapshot build right now' concern. We'd still need a change to lsp4e to adopt the new standard protocol and so also a new release/snapshot would be needed. I think. So it just adds more steps in between that need to be done before the release.

mickaelistria · 2020-11-06T17:51:57Z

So it just adds more steps in between that need to be done before the release.

Right, but those steps are more definitive and will be required anyway in a near future; while here is means we do a series of steps for the interim solution, then some steps for the final solution and then some steps to undo the interim. That greatly increase the cost of the feature implementation.
However, it's more up to you, as you're the main one involved in this effort ;)

kdvolder · 2020-11-06T17:54:23Z

Okay, wait, I think I didn't fully understand what you were saying at first. You suggesting we implement the 'client/executeCommand' message directly in lsp4j / lsp4e and then we don't even need this PR? Actually yes that makes sense to me.

Are you fine with doing it this way even without yet having a 'formal' acceptance of the LSP PR to become officially part of the protocol?

If so, yes, I agree that might actually be the best solution. But if we have to first work through LSP protocol acceptance, then maybe it takes too long and I think we should probably stick with the custom protocol instead.

mickaelistria · 2020-11-06T18:07:05Z

Are you fine with doing it this way even without yet having a 'formal' acceptance of the LSP PR to become officially part of the protocol?

I'd be fine from my LSP4E-centric perspective; however I think the LSP4J people may have some other (fair) considerations. I think it's more or less their call here.
Is there anything we can help with getting your PR for LSP merged sooner? That would remove a lot of doubt.

mickaelistria

There is a request to release LSP4E soon, with the CommandExecution class made public, so we'll probably merge it before 2020-12 if it still makes sense until then.
In the meantime please ensure that

.target file references the snapshots of TM4E (so CommandExecutor is visible)
the minimal version for org.eclipse.lsp4e in org.eclipse.wildwebdeveloper.xml is 0.10.0 (which does expose CommandExecutor)

kdvolder · 2020-11-12T18:30:01Z

the minimal version for org.eclipse.lsp4e in org.eclipse.wildwebdeveloper.xml is 0.10.0 (which does expose CommandExecutor)

Is that correct? Near as I can tell the 'master clone' of lsp4e in my workspace is version 0.13.4. So that should be one where we added 'CommandExecutor' as public api?

kdvolder · 2020-11-12T18:37:16Z

.target file references the snapshots of TM4E (so CommandExecutor is visible)

I don't understand. Did you mean LSP4E snapshot (instead of TM4E)? Earlier you said we should not depend on the snapshot so now I'm confused.

Signed-off-by: Kris De Volder <[email protected]>

mickaelistria · 2020-11-12T18:53:26Z

Sorry, you're right, it's LSP4E 0.13.4, I'm lost in too many topics these days... And keep in mind is not really a "public api" but more a temporary exposure of internal stuff until we completed the same story more elegantly ;)

kdvolder · 2020-11-12T18:54:50Z

@mickaelistria I've made the changes I think you wanted but not sure I understood what you wanted (see my confusion / questions above). Please have a look at the changes in last commit and let me know :-)

Link to latest commit for easy access/inspection: BoykoAlex@afdde86

mickaelistria

Can you please bump the version of the bundle to 0.10.0 ?

mickaelistria

Version in the pom.xml must be matching the version in MANIFEST.MF (except that .qualifier is to be replaced by -SNAPSHOT). You can try tocal builds with mvn verify to see such issues more immediately.
It's possible that some other features need similar change as a result of the bundle changing.

kdvolder · 2020-11-13T18:58:25Z

I allways forget to modify the pom on version bumps :-(

Thanks for the tip on mvn verify. Unfortunately it fails with some other error now:

ERROR] Failed to execute goal org.eclipse.tycho:tycho-packaging-plugin:2.0.0:package-plugin (default-package-plugin) on project org.eclipse.wildwebdeveloper: /home/kdvolder/git/wildwebdeveloper/org.eclipse.wildwebdeveloper/build.properties: bin.includes value(s) [package-lock.json, node_modules/] do not match any files. -> [Help 1]

Not really sure this is an actual problem or not, or just something about running the build locally on my machine. So I'll just push the change adn see what jenkins makes of it.

Signed-off-by: Kris De Volder <[email protected]>

mickaelistria · 2020-11-13T19:30:38Z

I've merged a commit that's partly conflicting here, adding 1 level of annoyance ;)
I'll take care of the final touch and will merge.

Signed-off-by: Kris De Volder <[email protected]>

kdvolder · 2020-11-13T19:38:42Z

I've merged a commit that's partly conflicting here, adding 1 level of annoyance ;)
I'll take care of the final touch and will merge.

I noticed, pushed something to try and address that conflict already.

kdvolder · 2020-11-13T20:00:35Z

There a test fail now. I'd try running this test locally in my runtime workbench, but I'm not sure how to satisfy the dependency on:

org.eclipse.ui.tests.harness

So at the moment I can't get the test compiling / running.

mickaelistria · 2020-11-13T20:13:23Z

This is merged now. Thanks a lot Alex and Kris!

kdvolder · 2020-11-13T21:10:33Z

@mickaelistria Thanks a lot for shepherding this through :-)

mickaelistria · 2020-11-14T08:57:26Z

Thanks even more to you for keeping this change moving forward despite my numerous change requests!

kdvolder · 2020-11-21T00:49:48Z

Trying to 'consume' this in STS builds but we can't it seems because of:

Require-Bundle: 
 org.eclipse.wildwebdeveloper.xml;bundle-version="[0.9.0,0.10.0)",

In https://github.com/eclipse-m2e/m2e-core/blob/master/org.eclipse.m2e.editor.lemminx/META-INF/MANIFEST.MF#L13

This is 'pinning' us to version 9.0.0.

Support for 'xml/executeClientCommand' protocol extension

2e71167

Signed-off-by: BoykoAlex <[email protected]>

BoykoAlex mentioned this pull request Oct 8, 2020

Support for 'xml/executeClientCommand` access to server from extension eclipse-lemminx/lemminx#905

Merged

jabby requested a review from mickaelistria October 8, 2020 15:13

mickaelistria requested changes Oct 13, 2020

View reviewed changes

kdvolder mentioned this pull request Oct 28, 2020

Provide a client-side protocol message that allows server to request execution of a command microsoft/language-server-protocol#1117

Open

Remove CommandExecutor and use the one from lsp4e

22f9c78

Signed-off-by: Kris De Volder <[email protected]>

kdvolder force-pushed the spring-nsuri branch from 8219188 to 22f9c78 Compare November 6, 2020 00:18

kdvolder mentioned this pull request Nov 6, 2020

Add support for client/executeCommand request eclipse-lsp4j/lsp4j#468

Closed

mickaelistria requested changes Nov 12, 2020

View reviewed changes

Use lsp4e snapshot and set minimum lsp4e version

afdde86

Signed-off-by: Kris De Volder <[email protected]>

mickaelistria requested changes Nov 12, 2020

View reviewed changes

kdvolder force-pushed the spring-nsuri branch from 61439ed to 8514f2f Compare November 13, 2020 19:01

Bump version of xml bundle to 0.10.0

6079efd

Signed-off-by: Kris De Volder <[email protected]>

kdvolder force-pushed the spring-nsuri branch from 8514f2f to 6079efd Compare November 13, 2020 19:03

kdvolder added 2 commits November 13, 2020 11:31

Bump xml feature version

207c6b6

Signed-off-by: Kris De Volder <[email protected]>

Merge remote-tracking branch 'origin/master' into spring-nsuri

9df9edf

Signed-off-by: Kris De Volder <[email protected]>

mickaelistria closed this Nov 13, 2020

kdvolder mentioned this pull request Nov 21, 2020

Relax version constraint on wwd eclipse-m2e/m2e-core#35

Closed

martinlippert mentioned this pull request Jan 4, 2021

finish work on project-related XSD namespace resolution for XML language server spring-projects/sts4#581

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for 'xml/executeClientCommand' protocol extension #543

Support for 'xml/executeClientCommand' protocol extension #543

BoykoAlex commented Oct 8, 2020 •

edited

Loading

mickaelistria Oct 13, 2020

mickaelistria Oct 13, 2020

kdvolder Oct 15, 2020

kdvolder Oct 16, 2020

kdvolder Nov 6, 2020 •

edited

Loading

mickaelistria commented Oct 22, 2020

kdvolder commented Oct 22, 2020 •

edited

Loading

kdvolder commented Oct 22, 2020

mickaelistria commented Oct 22, 2020

kdvolder commented Oct 22, 2020

kdvolder commented Nov 6, 2020

mickaelistria commented Nov 6, 2020

kdvolder commented Nov 6, 2020

mickaelistria commented Nov 6, 2020

kdvolder commented Nov 6, 2020 •

edited

Loading

mickaelistria commented Nov 6, 2020

mickaelistria left a comment

kdvolder commented Nov 12, 2020 •

edited

Loading

kdvolder commented Nov 12, 2020

mickaelistria commented Nov 12, 2020

kdvolder commented Nov 12, 2020 •

edited

Loading

mickaelistria left a comment

mickaelistria left a comment

kdvolder commented Nov 13, 2020

mickaelistria commented Nov 13, 2020

kdvolder commented Nov 13, 2020

kdvolder commented Nov 13, 2020

mickaelistria commented Nov 13, 2020

kdvolder commented Nov 13, 2020

mickaelistria commented Nov 14, 2020

kdvolder commented Nov 21, 2020 •

edited

Loading

		@@ -0,0 +1,168 @@
		package org.eclipse.wildwebdeveloper.xml.internal;

Support for 'xml/executeClientCommand' protocol extension #543

Support for 'xml/executeClientCommand' protocol extension #543

Conversation

BoykoAlex commented Oct 8, 2020 • edited Loading

mickaelistria Oct 13, 2020

Choose a reason for hiding this comment

mickaelistria Oct 13, 2020

Choose a reason for hiding this comment

kdvolder Oct 15, 2020

Choose a reason for hiding this comment

kdvolder Oct 16, 2020

Choose a reason for hiding this comment

kdvolder Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

mickaelistria commented Oct 22, 2020

kdvolder commented Oct 22, 2020 • edited Loading

kdvolder commented Oct 22, 2020

mickaelistria commented Oct 22, 2020

kdvolder commented Oct 22, 2020

kdvolder commented Nov 6, 2020

mickaelistria commented Nov 6, 2020

kdvolder commented Nov 6, 2020

mickaelistria commented Nov 6, 2020

kdvolder commented Nov 6, 2020 • edited Loading

mickaelistria commented Nov 6, 2020

mickaelistria left a comment

Choose a reason for hiding this comment

kdvolder commented Nov 12, 2020 • edited Loading

kdvolder commented Nov 12, 2020

mickaelistria commented Nov 12, 2020

kdvolder commented Nov 12, 2020 • edited Loading

mickaelistria left a comment

Choose a reason for hiding this comment

mickaelistria left a comment

Choose a reason for hiding this comment

kdvolder commented Nov 13, 2020

mickaelistria commented Nov 13, 2020

kdvolder commented Nov 13, 2020

kdvolder commented Nov 13, 2020

mickaelistria commented Nov 13, 2020

kdvolder commented Nov 13, 2020

mickaelistria commented Nov 14, 2020

kdvolder commented Nov 21, 2020 • edited Loading

BoykoAlex commented Oct 8, 2020 •

edited

Loading

kdvolder Nov 6, 2020 •

edited

Loading

kdvolder commented Oct 22, 2020 •

edited

Loading

kdvolder commented Nov 6, 2020 •

edited

Loading

kdvolder commented Nov 12, 2020 •

edited

Loading

kdvolder commented Nov 12, 2020 •

edited

Loading

kdvolder commented Nov 21, 2020 •

edited

Loading