GNU bug report logs - #61726
[PATCH] Eglot: Support positionEncoding capability

Previous Next

Full log

View this message in rfc822 format

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: João Távora <joaotavora <at> gmail.com>
Cc: 61726 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: bug#61726: [PATCH] Eglot: Support positionEncoding capability
Date: Fri, 24 Feb 2023 12:01:48 +0100

On Fri, 24 Feb 2023 at 10:20, João Távora wrote:

> The second thing I don't like is also due to the late-binding idea.
> This is a hotspot in Eglot, some of these functions are called
> many many times, for each LSP server interaction depending
> on how many document positions are exchanged (and they can
> be a lot).  I do remember benchmarking strategies at the time
> and seeing a perceptible difference.  Plus, this late-binding is
> really useless as a server will guaranteedly _not_ change its
> column-counting standard during the LSP session.

`eglot-lsp-abiding-column' allocates a new string!  I doubt that looking
up a few plists makes much of a difference compared to that.  But when
using the better offset counting styles, I think it might indeed make a
difference.  OTOH it might as well be premature optimization.

> Finally, here's a patch that doesn't use late-binding, doesn't
> introduce new strategies and supports "utf-32" and "utf-16"
> today.  As you can see, the patch is nearly trivial.

I'm fine with that way of doing things, but please respond to my concern
from the other message: do you really want to store a server capability
in a buffer-local variable?  What about your plans to support multiple
servers?

I suggest you to guard against future headaches.  We can store the
offset functions in two slots of the server class if you don't like to
traverse the capabilities plist each time.

> diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
> index eea8be6d1aa..ae8afa69651 100644
> --- a/lisp/progmodes/eglot.el
> +++ b/lisp/progmodes/eglot.el
> @@ -807,6 +807,7 @@ eglot-client-capabilities
>               :rangeFormatting    `(:dynamicRegistration :json-false)
>               :rename             `(:dynamicRegistration :json-false)
>               :inlayHint          `(:dynamicRegistration :json-false)
> +             :general            `(:positionEncodings ["utf-32" "utf-16"])
>               :publishDiagnostics (list :relatedInformation :json-false
>                                         ;; TODO: We can support
> :codeDescription after
>                                         ;; adding an appropriate UI to
> @@ -1789,6 +1790,9 @@ eglot--managed-mode
>        (add-hook 'eldoc-documentation-functions #'eglot-signature-eldoc-function
>                  nil t)
>        (eldoc-mode 1))
> +    (when (eq (eglot--server-capable :positionEncoding) "utf-16")
> +      (eglot--setq-saving eglot-move-to-column-function #'eglot-move-to-column)
> +      (eglot--setq-saving eglot-current-column-function
> #'eglot-current-column))
>      (cl-pushnew (current-buffer) (eglot--managed-buffers
> (eglot-current-server))))
>     (t
>      (remove-hook 'after-change-functions 'eglot--after-change t)
>
> As I said, enhancing this patch with a new pair of "current/move-to"
> functions that add in utf-8 support is acceptable.
>
> João

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.