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

Add window/progress #245

Closed
wants to merge 1 commit into from
Closed

Conversation

keegancsmith
Copy link

VS Code 1.12 includes an API for progress:

vscode.window.showProgress({
    location: vscode.ProgressLocation.Window,
    title: 'My long running operation'
}, async progress => {
    // Progress is shown while this function runs.
    // It can also return a promise which is then awaited
    progress.report({ message: 'Doing this' });
    await step1();

    progress.report({ message: 'Doing that' });
    await step2();
})

This proposal intends to make it possible for LSP to provide similar functionality.

The progress extension in vscode allows logging to source control and the window. However, we do not include that ability in this spec since:

@msftclas
Copy link

@keegancsmith,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla.microsoft.com.

It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
Microsoft Pull Request Bot

@msftclas
Copy link

@keegancsmith, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, Microsoft Pull Request Bot

@dbaeumer
Copy link
Member

@keegancsmith thanks for the PR. Actually adding this needs to be done under considering how other tools might implement something like this. The LSP client side is implemented in Eclipse, GNome builder, ...

I will mark this one as a dup of #70 which already contains quite some discussions around this topic.

@dbaeumer dbaeumer closed this May 17, 2017
@felixfbecker
Copy link

@dbaeumer Is there anything specific here that would not work with other editors? This follows the ideas in #70 pretty closely

@Xanewok
Copy link
Contributor

Xanewok commented Oct 4, 2017

@felixfbecker Do you think we could introduce this simple notification for starters, see how it goes and refine from there? I believe standarizing a simple progress protocol would incentivize more clients to implement at least some form of reporting the progress.
This could give more useful feedback on the issue, I think.

Currently the Rust Language Server uses custom rustDocument/diagnostics{Begin,End} notifications to report analysis build in progress and places a burden on the client to keep the 'running' diagnostics counter - total count of 0 means the build is done and all the information is up to date.
We were considering (rust-lang/rls#377) adding a bit more information (e.g. which dep is built, in addition to the main workspace package(s)), but as I said, right now it seems a bit ad hoc and we're mostly waiting for the LSP to standarize the feature.

@felixfbecker
Copy link

felixfbecker commented Oct 4, 2017

Sure, I think if multiple servers or clients want this it is reasonable to implement as an experimental/extension method and see if it picks up, then repropose it again. The way we did this with extensions at Sourcegraph was to prefix the methods with x, i.e. window/xprogress. Of course it's only valuable if we get at least one client to implement it too (we have a list of clients
at http://langserver.org/#implementations-client).

Is the PR in it's current form sufficient for your needs? Any other suggestions?

@Xanewok
Copy link
Contributor

Xanewok commented Oct 4, 2017

Regarding the current form, I think a more practical/consistent approach would be:

/**
-   * The title of the progress.
-   * This should be the same for all ProgressParams with the same id.
+   * Optional title of the progress.
+   * If unset, the previous title (if any) is still valid.
 */
-  title: string;
+  title?: string;

Seems redundant and/or hard to enforce passing the same title for a given id, but with the above change it'd be possible not to set any without introducing a new message - something I'm not sure is desired.

I think it's simple and self-contained enough in the current form as-is, I think it'd be nice to introduce it as an initial/experimental version.

@dbaeumer what are your thoughts?

@felixfbecker
Copy link

That makes a lot of sense to me. Wanna do a PR to the sourcegraph repo? We can have this live there as an unofficial spec extension for the time being

Xanewok added a commit to Xanewok/language-server-protocol that referenced this pull request Oct 4, 2017
Proposing this as of microsoft#245 (comment).
Existing discussion regarding the title: sourcegraph#21 (comment).
felixfbecker pushed a commit to sourcegraph/language-server-protocol that referenced this pull request Oct 4, 2017
Proposing this as of microsoft#245 (comment).
Existing discussion regarding the title: #21 (comment).
@dbaeumer
Copy link
Member

I added my comments here: microsoft/vscode-languageserver-node#261 (comment). I am open to where the discussion should happen but since a lively discussion happened in the PR I thought it is a good place to have it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants