bug#62687: [PATCH] Eglot: eglot--sig-info: Show SigInfo Docs if Markup; fix regex for highlighting; etc

[Mekeor Melire <mekeor@posteo.de>]

[-- Attachment #1: Type: text/plain, Size: 4886 bytes --]

2023-04-10 08:14 joaotavora@gmail.com:

> If you mean the names of the formal parameters of a given 
> function

No, I did not mean those. Sorry if I wasn't clear enough about 
this.

> If you mean the local variables names in the Elisp code, then 
> please let's not touch them.

Alright.

> You told me my last commit introduced another problem, so I just 
> want to understand what that new problem is.

> 2. Explain what the problem is in terms of user-visible 
> behaviour. Like "I see 'fooey: foo factor' and I would like to 
> see 'barey: foo factor'"

In my opinion, it makes most sense to discuss the problems of this 
function by the order of its lines of code. Since the first lines 
introduce variables, I had decided to discuss their naming first.

Also, I never claimed that the problem, that your last commit 
introduced, was related to user-visible behavior.

Decide yourself how you want to proceed. I hope this still counts 
as "single threaded" to you, since I don't request you to do 
something about all of these things at the same time, but I just 
want you to decide how we go on. Please, just choose one single 
path to take and continue it together. Don't reply to all of these 
things. Otherwise, everything will be chaotic (because there are 
interdependencies) and our conversation won't be single-threaded 
at all.

1. If you are interested in non-semantical code changes and 
specifically about indentation of eglot.el: The indentation of all 
lines of eglot--sig-info, beginning at the following line, needs 
to be adjusted by two spaces to the left: 
https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/progmodes/eglot.el?h=e33c0a549153fa3894f3b5e9c5e42ce07a1a68c7#n3129

2. If you are interested in semantical code changes that are not 
related to user-visible behavior but rather efficiency, one such 
problem is: For highlighting the active parameter in the formals 
list, we don't need to compare the index of each parameter to the 
known index of the active parameter. Instead, we can just access 
it, as it was done before the last commit on eglot.el. A 
minimalist patch for this issue is attached.

3.: (That's the rest of this email.)

If you are only interested in user-visible behavior: I'd like to 
discuss the visibility of documentation inside the echo-area.

Generally, I agree that we should keep the echoed message short in 
order to avoid flooding and distraction from the actual code that 
the user is editing. But at the same time, the documentation that 
LSP provides is one of its key features. So, there is a trade-off 
between keeping the echoed message short and quickly accessible 
useful documentation.

I guess, there are many different approaches to this trade-off, 
including the following:

A: We keep it the way it is. That is, we keep Eglot echoing only 
the SignatureInformation label, and there within, highlighting the 
active parameter, if possible.

B: We make Eglot echo the SignatureInformation label (where 
ParameterInformation label is highlighted, if possible) and 
SignatureInformation documentation and ParameterInformation 
documentation.

C: We make this configurable. In the simplest case, the 
configuration-variable would be a boolean to turn echoed-docs on 
or off. Alternatively, a more complex configuration-variable would 
be a list of symbols out of 'signature-information-label 
'signature-information-documentation and 
'parameter-information-documentation, which would also allow to 
specify the order of these.

D: We introduce a new function, similar to 
eglot-signature-eldoc-function, which will (leverage Eldoc in 
order to) not only echo the SigInfo-label, but also SigInfo- and 
ParamInfo-doc. This function could be bound to a key.

Personally, I'd vote for B (echo docs by default) together with C 
(add a configuration-variable). And I strongly vote against A 
(keep as-is).

Maybe you are interested in how other LSP-clients solve this 
problem. Personally, I only use Emacs+Eglot, but I did some 
research:

α: lsp-mode (for Emacs) seems to use Eldoc by default and echoes 
both the SigInfo-label and some docs. (I'm not sure if SigInfo- 
and/or ParamInfo-docs.) lsp-mode also offers a variable to turn 
docs off: lsp-signature-render-documentation. 
https://emacs-lsp.github.io/lsp-mode/tutorials/how-to-turn-off/ 
(13.)

β: lsp-ui, an addon for lsp-mode, show the label and both docs for 
the thing at point in a child-frame. 
https://emacs-lsp.github.io/lsp-ui/#lsp-ui-doc

γ: Neovim offers the function vim.lsp.buf.signature_help which is 
often bound to C-k but can also be configured to be called on the 
thing when/where the cursor holds. It will show SigInfo-label, a 
horizontal line, SigInfo- and finally ParamInfo-doc. A screenshot 
of this is attached.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 62687-2023-04-10-no-loop-index-comparison-for-active-param.patch --]
[-- Type: text/x-patch, Size: 3149 bytes --]

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 3f00281e155..247ebaf42da 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -3133,30 +3133,34 @@ for which LSP on-type-formatting should be requested."
         (when-let (doc (and (not briefp) sigdoc (eglot--format-markup sigdoc)))
           (goto-char (point-max))
           (insert "\n" (replace-regexp-in-string "^" "  " doc)))
+      ;; Perhaps highlight active parameter in the formals list
+      (let
+        ((active-param (or sig-active activeParameter)))
+        (when (and active-param (< -1 active-param (length parameters)))
+          (save-excursion
+            (goto-char (point-min))
+            (pcase-let
+              ((`(,beg ,end)
+                 (eglot--dbind ((ParameterInformation)
+                                 ((:label parlabel)))
+                   (aref parameters active-param)
+                   (if (stringp parlabel)
+                     (let ((case-fold-search nil))
+                       (and (search-forward parlabel (line-end-position) t)
+                         (list (match-beginning 0) (match-end 0))))
+                     (mapcar #'1+ (append parlabel nil))))))
+              (if (and beg end)
+                (add-face-text-property
+                  beg end
+                  'eldoc-highlight-function-argument)))))
         ;; Now to the parameters
         (cl-loop
-         with active-param = (or sig-active activeParameter)
          for i from 0 for parameter across parameters do
          (eglot--dbind ((ParameterInformation)
                         ((:label parlabel))
                         ((:documentation pardoc)))
              parameter
-           ;; ...perhaps highlight it in the formals list
-           (when (and (eq i active-param))
-             (save-excursion
-               (goto-char (point-min))
-               (pcase-let
-                   ((`(,beg ,end)
-                     (if (stringp parlabel)
-                         (let ((case-fold-search nil))
-                           (and (search-forward parlabel (line-end-position) t)
-                                (list (match-beginning 0) (match-end 0))))
-                       (mapcar #'1+ (append parlabel nil)))))
-                 (if (and beg end)
-                     (add-face-text-property
-                      beg end
-                      'eldoc-highlight-function-argument)))))
-           ;; ...and/or maybe add its doc on a line by its own.
+           ;; Maybe add its doc on a line by its own.
            (let (fpardoc)
              (when (and pardoc (not briefp)
                         (not (string-empty-p
@@ -3166,7 +3170,7 @@ for which LSP on-type-formatting should be requested."
                         (if (stringp parlabel) parlabel
                           (apply #'substring siglabel (mapcar #'1+ parlabel)))
                         'face (and (eq i active-param) 'eldoc-highlight-function-argument))
-                       ": " fpardoc)))))
+                       ": " fpardoc))))))
       (buffer-string))))
 
 (defun eglot-signature-eldoc-function (cb)

[-- Attachment #3: neovim-lsp-signature-help.png --]
[-- Type: image/png, Size: 54666 bytes --]