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

Use serverInitiatedProgress instead of custom notifications #34

Closed
LDAP opened this issue Dec 12, 2020 · 5 comments
Closed

Use serverInitiatedProgress instead of custom notifications #34

LDAP opened this issue Dec 12, 2020 · 5 comments
Assignees
Labels
1-feature-request ✨ Issue type: Request for a desirable, nice-to-have feature 3-fixed Issue resolution: Issue has been fixed on the develop branch
Milestone

Comments

@LDAP
Copy link
Contributor

LDAP commented Dec 12, 2020

Is your feature request related to a problem? Please describe.
ltex-ls is using custom notifications for progress reporting.

Describe the solution you'd like
Replace it with the standard server initiated progress scheme of the LSP specification.
See microsoft/pyright#1268 for a analog discussion

ltex/progress, with progress 0.0 --> window/workDoneProgress/create + $/progress with kind begin
ltex/progress, with progress 1.0 --> $/progress with kind end

Additional context
Simplifies integration with non vscode-ltex clients.

@LDAP LDAP added the 1-feature-request ✨ Issue type: Request for a desirable, nice-to-have feature label Dec 12, 2020
@valentjn
Copy link
Owner

This is already done, just haven't pushed yet.

@valentjn valentjn self-assigned this Dec 12, 2020
@valentjn valentjn added this to the 9.0.0 milestone Dec 12, 2020
@valentjn valentjn added the 3-fixed Issue resolution: Issue has been fixed on the develop branch label Dec 12, 2020
@LDAP
Copy link
Contributor Author

LDAP commented Dec 13, 2020

I would like to reopen this issue (but I can't due to GitHub permissions).

I tested the 9.0.0 alpha and it seams that the specification for serverInitiatedProgress is not met.

I think currently the server is implementing clientInitiatedProgress insted. But in this case the progress should be initiated by LTEX-LS.

The specification requires the server to

  • Initiate progress reporting using the window/workDoneProgress/create request.
  • Report progress using the generic $/progress notification.

Additionaly,

  • the title parameter should only briefly inform about the kind of operation being performed. (e.g. Checking document).
  • The uri of the file should instead be part of the message parameter.
  • see here

@valentjn valentjn removed the 3-fixed Issue resolution: Issue has been fixed on the develop branch label Dec 13, 2020
@valentjn valentjn reopened this Dec 13, 2020
@valentjn
Copy link
Owner

The server didn't use client-initiated progress as the tokens used by the server notifications are not introduced by the client before the server notifications. Just the window/workDoneProgress/create request was missing for server-initiated progress. This is fixed now, as well as the progress title.

@valentjn valentjn added the 3-fixed Issue resolution: Issue has been fixed on the develop branch label Dec 25, 2020
@LDAP
Copy link
Contributor Author

LDAP commented Dec 27, 2020

Does work now with the LSP client for Sublime Text too.

@valentjn
Copy link
Owner

valentjn commented Jan 3, 2021

Fix released in 9.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1-feature-request ✨ Issue type: Request for a desirable, nice-to-have feature 3-fixed Issue resolution: Issue has been fixed on the develop branch
Projects
None yet
Development

No branches or pull requests

2 participants