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

client/executeCommand #1119

Open
wants to merge 2 commits into
base: gh-pages
Choose a base branch
from

Conversation

kdvolder
Copy link

See: #1117

/**
* The client supports message 'client/executeCommand'.
*/
supported: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that you're trying get something out the door but this needs many more options for clients. Given just this state of the PR a server doesn't know whether a command can succeed or not.

Just to give you some ideas:

  • the client should be able to communicate dynamically to the server about possible new commands that it knows about, similar to workspace/didChangeConfiguration.
  • It should be possible for the server to get a list of known command names from the client, similar to workspace/configuration

At this point the server can only do client/executeCommand and hope for the best.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should require more options at this point. The current proposal is good and there is room for evolution (such as listing commands) in the future. However, we don't really have a strong need to pass a static list of commands at that point, just like symmetrically, there is no strong need for clients to know in advance the list of server commands it can invoke.
The command pattern is often use for client or LS specific integrations, that we cannot really standardize in LSP because they're not generic enough.
I think it's better to mimic the current workspace/executeCommand action (and related capabilities), it seems more consistent.

At this point the server can only do client/executeCommand and hope for the best.

It's the same for workspace/executeCommand, and yet this is working well ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the same for workspace/executeCommand, and yet this is working well ;)

Not quite. If you're lucky, you can read a readme file explaining what the arguments of a command can be. If you're unlucky, you have to dig through the server source code to figure out what a command can do.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The configure-less way of invoking commands is via code actions, which works well, I agree.

Copy link
Author

@kdvolder kdvolder Oct 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not against adding more options in the future for things such as 'command discovery' or making the spec more precise about mechanism by which commands are made to 'exist' on the client side.

However, I wanted to explicitly avoid this becoming yet another ticket dragged out into a lengthy discussion with no actionable outcome.

To me these questions are somewhat orthogonal:

  1. how does a server request a client to execute a command
  2. how does a server know that a client can execute a specific (set of) commands
  3. how are client-side commands defined

So I explicitly wanted limit the scope of this ticket to be just about 1. The other questions can be answered independently and in fact there are already some tickets open with long discussions.

Given just this state of the PR a server doesn't know whether a command can succeed or not.

That is true, but at least we know how to request execution in a standardised way. I.e. we are answering question 1. So to me that is progress.

And we can assume an error comes back if the execution failed (I agree with your other comments that we can perhaps make some of the error cases more explicit in the spec).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries. I am just some guy maintaining a client and weary of opening pandora's box with client/executeCommand. We must have a follow-up PR that defines how command discovery works, and we must standardize some "common" commands.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really disagree with you. I just don't want to go there yet. Yes, I could imagine some options added to capability object in the future such as 'commands: [...]' for a list of 'statically' known commands. Or a property 'commandDisovery: boolean' that indicates there is some extra protocol for command discovery. etc. But this sort of thing could be arbitrarily complex and with lots of people wanting something a little different from it. So I just don't think we should/can tackle all of these at once right now. The more we can separate this into smaller questions/issues, the more chance we have of actually making progress in small increments.

@rwols
Copy link
Contributor

rwols commented Oct 29, 2020

If this ultimately also leads to standardizing commands like "run auto-complete" and "commit completion" in a client-neutral way then I'm okay with this.

@mickaelistria
Copy link

If this ultimately also leads to standardizing commands like "run auto-complete" and "commit completion" in a client-neutral way then I'm okay with this.

I don't get the use-case here, the client is supposed to orchestrate that directly with LSP operations, why would command be useful for that?
If we can identify some generally useful server side commands that we're ready to spec, then it's better to just specify them as proper LSP operation, with proper parameters definition and so on than leaving it in a pot-pourri of command ids without finer specification.

@rwols
Copy link
Contributor

rwols commented Oct 29, 2020

I don't get the use-case here, the client is supposed to orchestrate that directly with LSP operations, why would command be useful for that?

There are servers that put "command": "editor.action.triggerSuggest" in some of their completion items. This means "show the auto-complete widget". But it's not standardized.

If we can identify some generally useful server side commands that we're ready to spec, then it's better to just specify them as proper LSP operation, with proper parameters definition and so on than leaving it in a pot-pourri of command ids without finer specification.

For sure :)

@dbaeumer
Copy link
Member

@kdvolder thanks for the PR. Besides updating the specification we need a reference implementation somewhere as well. This can be VS Code, Eclipse or any other client. Is someone willing to do that as well?

@kdvolder
Copy link
Author

This can be VS Code, Eclipse or any other client. Is someone willing to do that as well?

I was planning on doing it yes, just need to find the time :-)

@dbaeumer
Copy link
Member

@kdvolder great.

@radeksimko
Copy link
Contributor

I appreciate the work being done here and aim to ship something.

Do you mind describing some concrete use cases this is aiming to solve?

I ran into at least two use cases myself where I had the need to execute client-side commands:

  • open completion prompt
  • show references

In both cases these are commands that would benefit from being linked directly from their relevant methods - i.e. CompletionItem.command or CodeLens.command.

Executing a separate standalone method would require either extra roundtrip where the server implements some kind of a proxy server-side command for any client-side command, or somehow entirely bypass the existing workflow.

Are there some other use cases this is meant to address?


I agree with @rwols here that for server to not know what commands are available on the client it makes it practically impossible to provide a decent UX in cases I outlined above. Imagine that server implements code lens to show reference counts, user clicks to "5 references", linking to editor.action.showReferences and the client passes it back to the server because it doesn't implement it. The server can't do anything about it so the click ends up as no-op in the UI. That doesn't seem ideal? It would seem better if the server could just suppress the code lens entirely and avoid showing the link in the first place.

@radeksimko
Copy link
Contributor

I just found a use case for this feature. I assume it would be useful for updating some UI elements when the server deems it necessary.

The client may pull certain details (such as language/dependency versions or constraints, or some sort of hierarchy) about the codebase via custom command, but these details may get outdated as the user works on the codebase. It would therefore be useful for the server to notify the client when such details change.

I suppose that is the reason why these refresh commands exist anyway

@dbaeumer
Copy link
Member

Any progress on this PR?

@dbaeumer dbaeumer added the info-needed Issue requires more information from poster label Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
info-needed Issue requires more information from poster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants