microsoft / vscode-languageserver-node

Language server protocol implementation for VSCode. This allows implementing language services in JS/TS running on node.js
MIT License
1.47k stars 324 forks source link

Support notification progress instead of only status bar #1567

Open DanTup opened 1 month ago

DanTup commented 1 month ago

While investigating a bug about progress notifications not showing during refactors, I found that this client always uses status bar notifications which are very subtle:

https://github.com/microsoft/vscode-languageserver-node/blob/0671381a338f496afdff98b43a2f2d38def468e7/client/src/common/progressPart.ts#L61-L62

While this is fine for something like background analysis, it's much less useful for something like an expensive refactor if a user drags a folder in explorer and it may take a few seconds to compute all the edits to update the imports. This would be better as a more visible notification.

@dbaeumer I'd be happy to implement this using middleware for my extension, but it's not clear if that's possible because this code lives inside ProgressPart.begin(). It would be nice to support it in the client though - maybe even the spec? I think the concept of a progress notification that is prominent because it is doing something important the user is explicitly waiting for could make sense (or, maybe the client could do it if the progress is tied to some kinds of requests, like executeCommand?).

dbaeumer commented 1 month ago

If you want to show progress in that scenario the progress should be initiated from the client side and then passed to the server since the progress is bound to a request. Server side progress in meant for "out of bound" progress.

I am not sure if this is currently easily possible in the middleware. But I am open to have such a feature in the language client.

DanTup commented 4 weeks ago

If you want to show progress in that scenario the progress should be initiated from the client side and then passed to the server since the progress is bound to a request

These commands are Refactors so they are being invoked by the client. I believe they're already tied to a progress token from the client, but the language client seems to just treat all progresses the same and use ProgressLocation.Window when it handles begin.

Part of me thinks it'd be nice if LSP had a prominent?: boolean or something on WorkDoneProgressBegin, but part of me also thinks that for Refactors maybe VS Code (or the language client) should handle this (so it can be consistent across languages). Probably a user that wants Dart's refactors to show a toast progress notification if they didn't complete in <1s would want the same for any other language?

I see that only Notification progresses in VS Code support cancellation too, so using that for Refactors also seems like it could be better to make it clearer how users could cancel if it's taking too long.

Any thoughts/opinions?

FMorschel commented 4 weeks ago

Probably a user that wants Dart's refactors to show a toast progress notification if they didn't complete in <1s

I already see a behaviour like this on renaming files, for example. It would be great if any and all refactors got the same treatment!

dbaeumer commented 4 weeks ago

These commands are Refactors so they are being invoked by the client. I believe they're already tied to a progress token from the client, but the language client seems to just treat all progresses the same and use ProgressLocation.Window when it handles begin.

VS Code never binds a progress token to a provider call / request. The only exception right now is the initialize request in LSP.

Because of that my proposal is that extensions can inject a progress token into requests and control the visualization of the progress.

DanTup commented 4 weeks ago

@dbaeumer I'm not sure I understand the suggestion. I'm not sure what difference injecting a token makes - it seems to just mean the progress is tied to that request rather than a server-created out-of-band progress (I don't think this makes any difference to me).

I presume we'd need to change what's happening in ProgressFeature.initialize's createHandler to create our own version of ProgressPart that could use a different kind of progress, though it's not clear to me how we could conditionally use the Notification style (I think ideally I'd like the server to control that, because it knows when the operation being invoked is an "expensive" one that the use may want a more obvious notification for).