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

Experimental Incremental support #133

Closed
angelozerr opened this issue Sep 20, 2018 · 8 comments · Fixed by #359
Closed

Experimental Incremental support #133

angelozerr opened this issue Sep 20, 2018 · 8 comments · Fixed by #359
Assignees
Labels
enhancement New feature or request

Comments

@angelozerr
Copy link
Contributor

In large XML file, vscode like Eclipse freezes as soon as you type something. One of problem is because the didChange command send a big JSON content every time.

One idea that I have is to se the Incremental support on the server side, and didChange update the string of stored TextDocument.

I have started to implement this idea, but I think we should have a preferences like "experimentalIncrementalSupport" because I fear that there are some bugs.

@fbricon @NikolasKomonen what do you think about this experimentalIncrementalSupport preferences?

@fbricon
Copy link
Contributor

fbricon commented Sep 20, 2018

Yes, that'd be interesting, but I think experimental.incrementalSupport.enabled would be better.
Also, it'd be nice to finish getting proper schema validation/completion support before we start adding new fancy features.

angelozerr added a commit that referenced this issue Sep 21, 2018
JSON didChange request size when document changed. This incremental
support is only available with experimental.incrementalSupport.enabled
(see #133)
@angelozerr
Copy link
Contributor Author

Yes, that'd be interesting, but I think experimental.incrementalSupport.enabled would be better.

Thanks for your suggestion. I have done like this.

Also, it'd be nice to finish getting proper schema validation/completion support before we start adding new fancy features.

Yes sure, I'm working on this issue too and range formatting.

I close this issue, and please open a new issue if you find bugs with incremental support (by default, it's agaun Full and not incremental synch).

@fbricon
Copy link
Contributor

fbricon commented Sep 21, 2018

So I tried to test it in vscode but the way you implemented it, it's not a preference, but a client capability. In vscode we don't have an easy access to tweak available client capabilities and add experimental stuff.
The ClientCapabilities.experimental property is meant to add experimental feature of the protocol.

I think the experimental property you want needs to be part of the server configuration, i.e sent in the "settings" section of InitializeParams#getInitializationOptions(), so eventually ends up in XMLClientSettings

@fbricon fbricon reopened this Sep 21, 2018
@angelozerr
Copy link
Contributor Author

so eventually ends up in XMLClientSettings

Done.

@angelozerr
Copy link
Contributor Author

IMHO I think we should try to activate this incremental support to fix

The basic idea of incremental support is to update the XMLDocument text String content from DidChangeTextDocumentParams (which is filled only with text changed and not with the whole document in the case of activated incremental support ) -> see
https://github.com/angelozerr/lsp4xml/blob/a77306975b2312a1a219789a8d437d44064e59d8/org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/commons/TextDocuments.java#L86

We need to write a lot of test with call of update method:
https://github.com/angelozerr/lsp4xml/blob/a77306975b2312a1a219789a8d437d44064e59d8/org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/commons/TextDocument.java#L133

@NikolasKomonen
Copy link
Contributor

@angelozerr Is my following understanding of incremental support correct?

  1. The client will send the changed characters instead of the whole document

  2. The server will insert all of the changes into the current document

  3. The server will re-parse the whole document

So all incremental support is doing is reducing the size of the didChangeTextDocument json?

@fbricon
Copy link
Contributor

fbricon commented Apr 8, 2019

yes that's pretty much it

@angelozerr
Copy link
Contributor Author

@NikolasKomonen it's exactly that!

NikolasKomonen added a commit to NikolasKomonen/lsp4xml that referenced this issue Apr 24, 2019
Fixes eclipse#133

Removed the incremental setting, it is default and you can't change it

Signed-off-by: Nikolas <[email protected]>
NikolasKomonen added a commit to NikolasKomonen/lsp4xml that referenced this issue May 7, 2019
Fixes eclipse#133

Removed the incremental setting, it is default and you can't change it

Signed-off-by: Nikolas <[email protected]>
NikolasKomonen added a commit to NikolasKomonen/lsp4xml that referenced this issue May 16, 2019
Fixes eclipse#133

Removed the incremental setting, it is default and you can't change it

Signed-off-by: Nikolas <[email protected]>
NikolasKomonen added a commit to NikolasKomonen/lsp4xml that referenced this issue May 21, 2019
Fixes eclipse#133

Removed the incremental setting, it is default and you can't change it

Signed-off-by: Nikolas <[email protected]>
NikolasKomonen added a commit to NikolasKomonen/lsp4xml that referenced this issue May 29, 2019
Fixes eclipse#133

Removed the incremental setting, it is default and you can't change it

Signed-off-by: Nikolas <[email protected]>
NikolasKomonen added a commit to NikolasKomonen/lsp4xml that referenced this issue Jun 5, 2019
Fixes eclipse#133

Removed the incremental setting, it is default and you can't change it

Signed-off-by: Nikolas <[email protected]>
NikolasKomonen added a commit to NikolasKomonen/lsp4xml that referenced this issue Jun 5, 2019
Fixes eclipse#133

Removed the incremental setting, it is default and you can't change it

Signed-off-by: Nikolas <[email protected]>
NikolasKomonen added a commit to NikolasKomonen/lsp4xml that referenced this issue Jun 11, 2019
Fixes eclipse#133

Removed the incremental setting, it is default and you can't change it

Signed-off-by: Nikolas <[email protected]>
NikolasKomonen added a commit to NikolasKomonen/lsp4xml that referenced this issue Jun 11, 2019
Fixes eclipse#133

Incremental sync is off by default preference to turn on is:

"xml.experimental.incrementalSupport.enabled": true|false

Signed-off-by: Nikolas <[email protected]>
NikolasKomonen added a commit to NikolasKomonen/lsp4xml that referenced this issue Jun 14, 2019
Fixes eclipse#133

Incremental sync is off by default preference to turn on is:

"xml.experimental.incrementalSupport.enabled": true|false

Signed-off-by: Nikolas <[email protected]>
angelozerr pushed a commit that referenced this issue Jun 17, 2019
Fixes #133

Incremental sync is off by default preference to turn on is:

"xml.experimental.incrementalSupport.enabled": true|false

Signed-off-by: Nikolas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants