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

Send workspace/applyEdit to client for auto-close of tags #324

Open
mickaelistria opened this issue Mar 12, 2019 · 12 comments
Open

Send workspace/applyEdit to client for auto-close of tags #324

mickaelistria opened this issue Mar 12, 2019 · 12 comments
Assignees
Labels
blockedByLSP If this issue requires changes to the LSP to be completed enhancement New feature or request

Comments

@mickaelistria
Copy link
Contributor

Auto-edits like closing tags as user types should be implemented using the standard workspace/applyEdit command.
Basically, the LS would listen to documentChange (so far, so good), and when it is detected that an opening tag was closed (like <hello>, it should send the workspace/applyEdit command to add the </hello> tag at the right location).

@fbricon
Copy link
Contributor

fbricon commented Mar 12, 2019

I think we can probably switch to using workspace/applyEdit easily, but it won't be sent on documentChange for the moment.
So far, the whole document is sent to the server (an experimental incremental mode exists but is largely untested), detecting the precise location where a tag needs closing requires a more precise range.

@mickaelistria
Copy link
Contributor Author

detecting the precise location where a tag needs closing requires a more precise range.

I think the LS can try to compare the previous document state with the current one on documentChange to sort out the smaller grain change can trigger an auto-edit. But at this point, I guess it's the same difficulty than implementing incremental mode...

@fbricon
Copy link
Contributor

fbricon commented Mar 12, 2019

I'd be more inclined to (ab)use the completion request instead

@mickaelistria
Copy link
Contributor Author

That would be already a nice way to provide a good chunk of value.

@NikolasKomonen NikolasKomonen added this to the v0.6.0 milestone Apr 9, 2019
@NikolasKomonen NikolasKomonen added the blockedByLSP If this issue requires changes to the LSP to be completed label May 21, 2019
@NikolasKomonen
Copy link
Contributor

This issue is implemented by https://github.com/NikolasKomonen/lsp4xml/tree/autoCompletionMigration
but because the LSP does not allow snippets in applyEdit's it will not be merged.

@NikolasKomonen NikolasKomonen removed this from the v0.6.0 milestone May 21, 2019
@mickaelistria
Copy link
Contributor Author

This issue is marked as "blocked by LSP", but in reality, isn't it more blocked by lack of support for incremental changes in lsp4xml?

@NikolasKomonen
Copy link
Contributor

NikolasKomonen commented Aug 7, 2019

@mickaelistria
I started working on this issue, but using the applyEdit method didn't allow for selecting the cursor position (using a $0 ) and it always put the cursor at the end of the insert. Overall the experience is not as good as the current implementation I've found.

@mickaelistria
Copy link
Contributor Author

Ok, so this is microsoft/language-server-protocol#724 , isn't it?

@NikolasKomonen
Copy link
Contributor

yup, not sure if it's being worked on currently or is just in the backlog. I created a similar issue previously but it's been overridden by microsoft/language-server-protocol#724, which should be the one to follow for any updates.

@mickaelistria
Copy link
Contributor Author

Wouldn't https://microsoft.github.io/language-server-protocol/specification#textDocument_onTypeFormatting be the right thing to use here?
Actually, I'm not sure clients would be expected to change the cursor position.

@angelozerr
Copy link
Contributor

Wouldn't https://microsoft.github.io/language-server-protocol/specification#textDocument_onTypeFormatting be the right thing to use here?

We need to experiment it for XML. But for HTML language server it will require to do the same thing if it works

@moabson
Copy link

moabson commented Jul 2, 2021

What is the currently status for this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blockedByLSP If this issue requires changes to the LSP to be completed enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants