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

Support notification progress instead of only status bar #1567

Open
DanTup opened this issue Oct 9, 2024 · 5 comments
Open

Support notification progress instead of only status bar #1567

DanTup opened this issue Oct 9, 2024 · 5 comments
Labels
info-needed Issue requires more information from poster

Comments

@DanTup
Copy link
Contributor

DanTup commented Oct 9, 2024

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:

// Since we don't use commands this will be a silent window progress with a hidden notification.
void Window.withProgress<void>({ location: ProgressLocation.Window, cancellable: params.cancellable, title: params.title}, async (progress, cancellationToken) => {

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
Copy link
Member

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.

@dbaeumer dbaeumer added the info-needed Issue requires more information from poster label Oct 11, 2024
@DanTup
Copy link
Contributor Author

DanTup commented Oct 14, 2024

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
Copy link

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
Copy link
Member

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
Copy link
Contributor Author

DanTup commented Oct 15, 2024

@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).

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

No branches or pull requests

3 participants