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 new client capability: positionEncodings #916

Closed
wants to merge 1 commit into from

Conversation

nemethf
Copy link
Collaborator

@nemethf nemethf commented Apr 9, 2022

This branched off from #915. It doesn't update NEWS.md as there's no user-visible changes. Unfortunately I'm not aware any server that implements this feature, so this is 100% untested. Also it adds another layer of indirection which has to have a small performance impact. The specification doesn't explain the encodings but this is quite informative: https://clangd.llvm.org/extensions.html#utf-8-offsets


Result in no user-visible changes. Default to codepoint based column
calculation, which is the most natural choice for Emacs.

  • eglot.el (eglot-client-capabilities): Add positionEncodings.
    (eglot-ignored-server-capabilities): Add positionEncoding.
    (eglot-negotiated-column, eglot-move-to-negotiated-column): New
    defuns.
    (eglot-current-column-function): Change to
    eglot-negotiated-column.
    (eglot-move-to-column-function): Change to
    eglot-move-to-negotiated-column.

Result in no user-visible changes.  Default to codepoint based column
calculation, which is the most natural choice for Emacs.

* eglot.el (eglot-client-capabilities): Add positionEncodings.
(eglot-ignored-server-capabilities): Add positionEncoding.
(eglot-negotiated-column, eglot-move-to-negotiated-column): New
defuns.
(eglot-current-column-function): Change to
eglot-negotiated-column.
(eglot-move-to-column-function): Change to
eglot-move-to-negotiated-column.
@nemethf
Copy link
Collaborator Author

nemethf commented Apr 9, 2022

I haven't realized but this feature is just a couple of days old. So maybe the PR shouldn't be merged until its final form is crystalized.

microsoft/language-server-protocol@f9c85d5

@joaotavora
Copy link
Owner

Also it adds another layer of indirection which has to have a small performance impact.

Yep it "has to have" a performance impact, but we don't know how big or small. Would have to be measured, but note that position calculation is likely a hotspot of Eglot.

Isn't it possible to check the capability in eglot--managed-mode and set the two variables locally for the buffer there?

For users currently overriding eglot-move-t-c-f and eglot-current-c-f in some major mode hook, we could check, in eglot--managed-mode if the value of the variable is equal to its default value before touching it. If it's not, then we leave it be. This wouldn't break existing customizations, I think.

Another option is to set the variable's values to :undecided and set them in eglot--managed-mode iff it's eq to that value.

@nemethf
Copy link
Collaborator Author

nemethf commented Nov 14, 2022

It seems rust-analyzer and ocaml-lsp now support this LSP feature, although I haven't experimented with them. (I cannot work on this PR at the moment, but wanted to add a note here about the news.)

@matklad
Copy link
Contributor

matklad commented Feb 13, 2023

I see that utf-32 is advertised by default. Am I correct that that would actually be the most convenient correct version for eglot? That is, that native Emacs offsets which don’t require any transcoding are UTF-32?

if that’s the case, I’ll add support for thst to rust-analyzer (IIRC, we support only utf-8 and utf-16, and I was under the impression that utf-8 is EMacs’ native encoding)

@matklad
Copy link
Contributor

matklad commented Feb 14, 2023

After rust-lang/rust-analyzer#14141, rust-analyzer implements all of utf-8, utf-16 and utf-32, which hopefully should simplify the testing!

@nemethf
Copy link
Collaborator Author

nemethf commented Feb 14, 2023

I see that utf-32 is advertised by default. Am I correct that that would actually be the most convenient correct version for eglot? That is, that native Emacs offsets which don’t require any transcoding are UTF-32?

Yes, they are. For example, point returns the number of code-points between the current cursor position and the beginning of the file being edited.

I’ll add support for thst to rust-analyzer

That'd be great. However, I'm not sure using utf-32 will result in a better overall performance. The specification has this to say about the issue:

Implementation considerations: since the conversion from one
encoding into another requires the content of the file / line the
conversion is best done where the file is read which is usually
on the server side.

At any rate, we will able to measure the effects of the encoding negotiations.

Thank you

@matklad
Copy link
Contributor

matklad commented Feb 14, 2023

ra PR merged!

@nemethf
Copy link
Collaborator Author

nemethf commented Feb 27, 2023

This is closed by commit "Eglot: support positionEncoding LSP capability (bug#61726)":
emacs-mirror/emacs@3e3e6d7

@nemethf nemethf closed this Feb 27, 2023
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.

3 participants