Possibility to show full first line of hover info #459
Conversation
|
This can probably fix #117 |
|
:-) nice job 👏 👏 👏 I think the idea is good, but it doens't follow the semantics of the |
Do some refactoring to join similar logic in eglot-doc-too-large-for-echo-area and eglot--truncate-string. * eglot.el (eglot-doc-too-large-for-echo-area): Now returns the number of lines available. (eglot--truncate-string): New helper. (eglot--first-line-of-doc, eglot--top-lines-of-doc): Remove. (eglot--update-doc): Use new helpers.
|
Hi @muffinmad , I did some adjustments to this PR, to join some duplicated logic, improving efficiency, but hopefully keeping the behaviour you want. I'm in a hurry, so I couldn't even test, but it shouldn't be very far off. Have a look when you can. |
|
Alongside with failed newly added test following error occurred. from datetime import datetimeWith point at the end of that line: |
|
Nice, thanks for the test and the simple recipe. I'll look at this asap |
|
There seems to be a bug in edit: perhaps all lines (and not just the last) should truncate width? |
|
Fredrik Bergroth <notifications@github.com> writes:
There seems to be a bug in eglot--truncate-string if there is no trailing newline:
(eglot--truncate-string "aa\nbb" 2) ;; => "aabb"
thanks!
|
Do some refactoring to join similar logic in eglot-doc-too-large-for-echo-area and eglot--truncate-string. * eglot.el (eglot-doc-too-large-for-echo-area): Now returns the number of lines available. (eglot--truncate-string): New helper. (eglot--first-line-of-doc, eglot--top-lines-of-doc): Remove. (eglot--update-doc): Use new helpers.
b8ada54 to
b54e91a
Compare
|
Hi @muffinmad and @fbergroth. I fixed both problems, and enhanced the test a little bit. @muffinmad if this works like you wanted I was thinking of squashing this to a commit and pushing with you as the Co-author. Or the other way around, you the author me co-author. Whichever you prefer. |
eglot.el
Outdated
| (t | ||
| (eldoc-message (eglot--first-line-of-doc string))))) | ||
| ;; Can't (yet?) honour non-t non-nil values if this var |
There was a problem hiding this comment.
Should this read "of this var"?
eglot.el
Outdated
| (string &optional (height max-mini-window-height)) | ||
| "Return non-nil if STRING won't fit in echo area of height HEIGHT. | ||
| HEIGHT defaults to `max-mini-window-height' (which see) and is | ||
| interpret like that variable. If non-nil, the return value is |
Do some refactoring to join similar logic in eglot-doc-too-large-for-echo-area and eglot--truncate-string. * eglot.el (eglot-doc-too-large-for-echo-area): Now returns the number of lines available. (eglot--truncate-string): New helper. (eglot--first-line-of-doc, eglot--top-lines-of-doc): Remove. (eglot--update-doc): Use new helpers.
b54e91a to
ef3e023
Compare
Please take a look at the differences between the original PR. All tests are done with (setq eglot-put-doc-in-help-buffer nil)
(modify-frame-parameters nil '((width . 70) (height . 20)))1. No problem here, the first line of the hover info truncated to fame width is shown. 2. First line of the hover info truncated to frame width is show. Can we show full first line here?
New PR version is on the left 3. First lines of the hover info are hidden.
New PR version is on the left
I'm totally fine with the first option. It's turned to be your PR now ;-) |
Yes, I'm sorry. Though I didnt' erase your commit (dont' lose it yoruslef). Anyway I'm not going to push until this does exactly what you want. What matters is the behaviour, and then we make the implementation as tight as possible. |
…ain) Co-authored-by: Andreii Kolomoiets <andreyk.mad@gmail.com> Also do some refactoring to join similar logic in eglot-doc-too-large-for-echo-area and eglot--truncate-string. * eglot.el (eglot-doc-too-large-for-echo-area): Now returns the number of lines available. (eglot--truncate-string): New helper. (eglot--first-line-of-doc, eglot--top-lines-of-doc): Remove. (eglot--update-doc): Use new helpers. * eglot-tests.el (hover-multiline-doc-locus): New test
ef3e023 to
9dc0f3f
Compare
|
OK, this should be fixed (both problems you pointed out in 2 and 3). Have a last look and I'll push tomorrow to close this. And thanks very much! |
Case number 2 works as expected now, but for 3 the first lines of the hover info are still hidden: (modify-frame-parameters nil '((width . 70)))
(setq eldoc-echo-area-use-multiline-p t)
|
|
I think this third problem should be something solved in |
|
In the meantime, I've pushed this version to master, since it's much nicer than what we had before. |
|
So far we've been leaving that responsibility to Another thing to try is binding |
|
Indeed, I tried that, but I think something in eldoc itself will then call
eldoc-message again with the last recorded message, but without the binding.
Anyway this is just a middle point before actually fixing the need for
eldoc clients to call eldoc-message directly.
…On Mon, May 25, 2020, 12:48 Dmitry Gutov ***@***.***> wrote:
So far we've been leaving that responsibility to
eglot-documentation-function (binding message-truncate-lines to t). That
doesn't need complex math.
Another thing to try is binding resize-mini-windows to nil, I think.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#459 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAC6PQ35SSAH5UXWUJEAP7DRTJLJ5ANCNFSM4M2X7DSA>
.
|
Ah, hm, my bad. Indeed, you can't set the binding this way. But looking at ElDoc's sources, have you tried the |
|
Gotta love the "they are Legion" comment in eldoc.el 🤣 |
|
`eldoc-documentation-callback` is a function, not a variable? If so, quite
a strange name for a function.
Feel free to use something better.
Then what's the difference between that and calling `eldoc-message` directly?
[ I have a feeling of déjà vu! ]
`eldoc-message` (despite the absence of a "--") is an
internal function. We could "redirect it", but I think at this point we
might as well define a new function to be used for the call back.
If in the end it can be merged with `eldoc-message`, then that's fine as well.
In any case, there may be differences, especially now with the *s*
which means that we need to combine the output from various functions
before we can pass it to `eldoc-message`.
I guess just the return value of the things eglot is going to put in
`eldoc-documentation-functions`. I'd rather use the variable though,
it gives us more leeway for handling quirky cases later on, like
properly supporting the `eldoc-documentation-compose` case.
But that should all be hidden behind `eldoc-documentation-callback`
(the functions placed on the *s* shouldn't have to worry about that).
By the way, this is unrelated: _some_ handling of
`eldoc-echo-blablabla-multiline` is already done by eldoc.el directly
I see. Docstrings should be upgraded.
Quite likely. I think there's a chance that the code should be changed
as well (and not necessarily to reflect the doc, but to provide a more
sane/regular/predictable behavior).
> Hmm... I'm beginning to get the impression that you missed
> commit c0fcbd2c119b8418855f0931aceefbef717c5e53 `master`:
Yes, as I said before i missed it. It makes things more complicated.
FWIW, while I think the idea behind the change is right, I'm beginning
to suspect that a different approach might be needed (since we will need
to combine synchronous outputs with asynchronously outputs, so I suspect
we won't be able to hide all that within
`eldoc-documentation-function`).
FWIW, the current code also has a bit of a backward compatibility
problem with packages that set `eldoc-documentation-function` instead of
`eldoc-documentation-functions`. So this might be an opportunity to fix
this backward compatibility as well before it's "anointed" in the
Emacs-28 release.
Stefan
|
|
> So I think a much simpler API is to have `eldoc-documentation-function*s*`
return a special value meaning "will call you later" and that later call
is to a global function `eldoc-documentation-callback`.
Why doesn't it return some "future" or "promise" value instead? So that
Eldoc sets up all the necessary callbacks itself.
Fine by me
Stefan
|
Oh sure, but if we can't give each member of those functions a different Anyway, the current patch I'm preparing doens't suppport the async case and |
|
> But that should all be hidden behind `eldoc-documentation-callback`
> (the functions placed on the *s* shouldn't have to worry about that).
Oh sure, but if we can't give each member of those functions a different
callback function, then it's harder to combine the results sanely. In hindsight
it was bad to make elements `eldoc-documentation-functions` argless functions.
I'm not sure what information they'd need to do their job.
OK, let's say we use futures.
So we say that `eldoc-documentation-functions` can return futures.
The futures API I'd imagine it here could look like:
(cl-defstruct (eldoc-future
(conc-name eldoc-future--)
(:constructor eldoc-future-make ()))
(value eldoc-future--unset)
callback)
(defun eldoc-future-set (f v)
(cl-assert (eq (eldoc-future--value f) 'eldoc-future--unset))
(setf (eldoc-future--value f) v)
(when (eldoc-future--callback f)
(funcall (eldoc-future--callback f) v)))
(defun eldoc-future-set-callback (f c)
(cl-assert (null (eldoc-future--callback f)))
(setf (eldoc-future--callback f) c)
(unless (eq (eldoc-future--value f) 'eldoc-future--unset)
(funcall c (eldoc-future--value f))))
So the backend starts by calling `eldoc-future-make` and returning
that object. Then eldoc calls `eldoc-future-set-callback` on it, which
sets the callback. Then in a process-filter/sentinel the backend calls
`eldoc-future-set` on it, thus running the callback.
The same interface would work for synchronous backends, where they call
`eldoc-future-set` before returning the object.
Then we write a `eldoc-future-do` macro that turns
(eldoc-future-let ((v FOO))
BAR)
into
(eldoc-future-set-callback FOO (lambda (v) BAR))
You could have followed a similar path to `flymake-diagnostic-functions` for
what is a completely analogous problem.
You mean we'd pass the callback function as an argument?
That's a valid option as well, indeed.
I think what I like about futures is that it opens up the possibility to
extend the API a bit more easily. E.g. adding the possibility to abort
a future.
Maybe you still can, this API hasn't really been out for longer than
a few weeks in GNU ELPA, I doubt anyone is using it.
I definitely consider this API as "not yet released".
Stefan
|
|
I don't have anything against this futuristic ambitious future stuff, but I definitely don't have time to discuss it, and this problem needs some fixing in the short term (both here, in SLY, and probably also SLIME). So I'm going to propose something much closer to what
In that case I would simplify my patch more and remove the cat 0001-Better-handle-asynchronously-produced-eldoc-docstrin.patch master ⬆ ✭ ◼
From cd6ef1b12ea120773cbe239157f06eca93a932cd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <joaotavora@gmail.com>
Date: Mon, 25 May 2020 16:39:40 +0100
Subject: [PATCH] Better handle asynchronously produced eldoc docstrings
* lisp/emacs-lisp/eldoc.el (eldoc-documentation-functions):
Overhaul docstring.
(eldoc-documentation-compose, eldoc-documentation-default): Handle
non-nil, non-string values of elements of
eldoc-documentation-functions. Use eldoc--handle-multiline.
(eldoc-print-current-symbol-info): Honour non-nil, non-string
values returned by eldoc-documentation-callback.
(eldoc--handle-multiline): New helper.
(Version): Bump to 1.1.0.
---
lisp/emacs-lisp/eldoc.el | 73 ++++++++++++++++++++++++++++++----------
1 file changed, 56 insertions(+), 17 deletions(-)
diff --git a/lisp/emacs-lisp/eldoc.el b/lisp/emacs-lisp/eldoc.el
index ef5dbf8103..288f617822 100644
--- a/lisp/emacs-lisp/eldoc.el
+++ b/lisp/emacs-lisp/eldoc.el
@@ -5,7 +5,7 @@
;; Author: Noah Friedman <friedman@splode.com>
;; Keywords: extensions
;; Created: 1995-10-06
-;; Version: 1.0.0
+;; Version: 1.1.0
;; Package-Requires: ((emacs "26.3"))
;; This is a GNU ELPA :core package. Avoid functionality that is not
@@ -338,12 +338,26 @@ eldoc-display-message-no-interference-p
(defvar eldoc-documentation-functions nil
- "Hook for functions to call to return doc string.
-Each function should accept no arguments and return a one-line
-string for displaying doc about a function etc. appropriate to
-the context around point. It should return nil if there's no doc
-appropriate for the context. Typically doc is returned if point
-is on a function-like name or in its arg list.
+ "Hook of functions that produce doc strings.
+Each hook function should accept no arguments and decide whether
+to display a doc short string about the context around point. If
+the decision and the doc string can be produced quickly, the hook
+function should immediately return the doc string, or nil if
+there's no doc appropriate for the context. Otherwise, if its
+computation is expensive or can't be performed directly, the hook
+function should save the value bound to
+`eldoc-documentation-callback', and arrange for that callback
+function to be asynchronously called at a later time, passing it
+either nil or the desired doc string. The hook function should
+then return a non-nil, non-string value.
+
+A current limitation of the asynchronous case is that it is only
+guaranteed to work correctly if the value of
+`eldoc-documentation-function' (notice the singular) is
+`eldoc-documentation-default'.
+
+Typically doc is returned if point is on a function-like name or
+in its arg list.
Major modes should modify this hook locally, for example:
(add-hook \\='eldoc-documentation-functions #\\='foo-mode-eldoc nil t)
@@ -351,14 +365,18 @@ eldoc-documentation-functions
taken into account if the major mode specific function does not
return any documentation.")
+(defun eldoc--handle-multiline (res)
+ "Helper for handling a bit of `eldoc-echo-area-use-multiline-p'."
+ (if eldoc-echo-area-use-multiline-p res
+ (truncate-string-to-width
+ res (1- (window-width (minibuffer-window))))))
+
(defun eldoc-documentation-default ()
"Show first doc string for item at point.
Default value for `eldoc-documentation-function'."
(let ((res (run-hook-with-args-until-success 'eldoc-documentation-functions)))
- (when res
- (if eldoc-echo-area-use-multiline-p res
- (truncate-string-to-width
- res (1- (window-width (minibuffer-window))))))))
+ (cond ((stringp res) (eldoc--handle-multiline res))
+ (t res))))
(defun eldoc-documentation-compose ()
"Show multiple doc string results at once.
@@ -368,13 +386,11 @@ eldoc-documentation-compose
'eldoc-documentation-functions
(lambda (f)
(let ((str (funcall f)))
- (when str (push str res))
+ (when (stringp str) (push str res))
nil)))
(when res
(setq res (mapconcat #'identity (nreverse res) ", "))
- (if eldoc-echo-area-use-multiline-p res
- (truncate-string-to-width
- res (1- (window-width (minibuffer-window))))))))
+ (eldoc--handle-multiline res))))
(defcustom eldoc-documentation-function #'eldoc-documentation-default
"Function to call to return doc string.
@@ -408,6 +424,12 @@ eldoc--supported-p
;; there's some eldoc support in the current buffer.
(local-variable-p 'eldoc-documentation-function))))
+;; this variable should be unbound, but that confuses
+;; `describe-symbol' for some reason.
+(defvar eldoc-documentation-callback nil
+ "Dynamically bound. Accessible to `eldoc-documentation-functions'.
+See that function for details.")
+
(defun eldoc-print-current-symbol-info ()
"Print the text produced by `eldoc-documentation-function'."
;; This is run from post-command-hook or some idle timer thing,
@@ -417,11 +439,28 @@ eldoc-print-current-symbol-info
;; Erase the last message if we won't display a new one.
(when eldoc-last-message
(eldoc-message nil))
- (let ((non-essential t))
+ (let ((non-essential t)
+ (buffer (current-buffer)))
;; Only keep looking for the info as long as the user hasn't
;; requested our attention. This also locally disables inhibit-quit.
(while-no-input
- (eldoc-message (funcall eldoc-documentation-function)))))))
+ (let*
+ ((waiting-for-callback nil)
+ (eldoc-documentation-callback
+ (lambda (string)
+ (with-current-buffer buffer
+ ;; JT@2020-05-25: Currently, we expect one single
+ ;; docstring from the client, we silently swallow
+ ;; anything the client unexpectedly gives us,
+ ;; including updates. This could change.
+ (when waiting-for-callback
+ (eldoc-message (eldoc--handle-multiline string))
+ (setq waiting-for-callback nil)))))
+ (res
+ (funcall eldoc-documentation-function)))
+ (cond ((stringp res) (eldoc-message res))
+ (res (setq waiting-for-callback t))
+ (t (eldoc-message nil)))))))))
;; If the entire line cannot fit in the echo area, the symbol name may be
;; truncated or eliminated entirely from the output to make room for the |
|
The docstring contained an error: it should read: "is only guaranteed to work if the value is eldoc-documentation-default" |
I disagree. The "futures" solution will most likely have argless functions. So the ad-hoc solution should use them too, then the API could be extended/swapper very easily.
Naturally. There are other places they could be used in, too. |
Yup. Or add an "errback" as well, in which ElDoc will display/log in the execution error using some of its mechanisms. |
Well the future isn't here yet, is it? When it does come, it's a question of telling elements of Anyway, as much as I am convinced of the soundness of Stefan's proposal on futures (it is a reasonably easy concept, though i suspect a bit niche still), I would consider the possibility that we might be over-engineering it here, because I can't think of enormous advantages of futures to Eldoc specifically:
Curiously, applying the reasoning to Flymake yields opposite results: Flymake does have a compelling use case for those two things.
|
I just showed an ad-hoc API that would work very close to it.
Why do this ugly thing when we can do it better from the start?
It would most of the time abort an HTTP request, not local computation. |
It's not ugly at all. Just as ugly as multiple semantics for return values. And if you want to do the future thing now, be my guest. I await your implementation.
Go for it. But why go for an ad-hoc API when...
How do you abort those? I.e. where you would be checking for the abort flag in the future object? |
|
Two more points:
|
OK.
It's about equal to complexity of your patch. But if we can get the futures in sooner rather than later, that'd be better.
By calling
You probably know where to look for its documentation, but OK. Please wait for the alternative patch. |
And exactly when will that code do that in the HTTP request scenario that you presented? |
|
Perhaps Eldoc will do that in |
|
Or, with |
That problem is already solved by
By the time that could be useful the presumed HTTP request is already well on its way and you're waiting for a filter or a sentinel call. In this scenario, the only place where is might make sense to check
I seriously doubt that, BTW. To "get the futures in", i.e. to get a futures library in Emacs, you have to, well, develop a futures library. A good one, presumably. That's a new file, and a new section in manual. Not to mention a few weeks of bikeshedding (at best) with comparisons to existing implementations such as https://github.com/skeeto/emacs-aio (which, by the way, looks very good). To hold up this needed fix while we wait for that to resolve seems nonsensical (and, I suppose, a bit paradigmatic of how the reddit crowd sees emacs development). I say: let's develop a good futures library and use it as much as we can. It'll be your new baby. But babies need to gestate, so let's not rush it. In the meantime, let's fix this actual problem that Eglot is facing. The |
|
You set up the callback inside
Depending on the implementation, it could be parsing a huge JSON or whatever. And some other implementations could terminate the network connection (if they expect the server on the other side to handle it well), or kill an external process. |
I don't understand. As soon as the response arrives, of course. As soon as the idle timer fires, you'll have made the request, there's no stopping it.There's also no point in checking
For eldoc? Nah. For diagnostics, maybe, indeed we get very large diagnostic responses in Eglot. But if you really really think that aborting is a killer feature here, I can add a
Again, the only chance they have to do so in the process filter or sentinel. So by then you'll have wasted the effort. |
And is Emacs idle at that time?
Yes, true. Only having done a very cursory look at the code, I imagined it doing a sit-for loop until the response arrives. I can see now this is not the case.
This is not only for Eglot, though.
No idea, really.
Not sure why you think so. Anyway, check out https://debbugs.gnu.org/41531#8. |
Doesn't matter, by the time you can check if input is pending, you'll have bothered the server and Emacs will have slurped a good chunk of its response into memory.
Yes, but what "function signature at point" server produces "huge JSON" then?
Because that's the way Emacs works! The only effort you can save is heavy lisp processing in the process filter. And what I'm telling you is that the effort is reasonably small in the eldoc case.
Thanks. Doesn't look bad. For a poor man's async primitive, I prefer my version, which matches what flymake.el and url.el (and possibly also websocket.el) do. But of course I can live with your patch, so I'll let Stefan or someone else decide . Let's continue in the Emacs bug tracker. |
OK, another question: if the result still valid?
No idea, a hypothetical one. It's an open API, not everybody is or will be using LSP until the end of time. And it doesn't really have to be huge. A saving is a saving.
You can certainly kill the external process outside of it. Saving on CPU expenses in general.
So even the code savings didn't convince you? Both in eldoc.el, and likely in doc functions as well.
Using a global var is so Emacs 23. |
|
Continued the rest of the conversation in the emacs bug tracker link you posted.
|
…oc (again) Co-authored-by: Andreii Kolomoiets <andreyk.mad@gmail.com> Also do some refactoring to join similar logic in eglot-doc-too-large-for-echo-area and eglot--truncate-string. * eglot.el (eglot-doc-too-large-for-echo-area): Now returns the number of lines available. (eglot--truncate-string): New helper. (eglot--first-line-of-doc, eglot--top-lines-of-doc): Remove. (eglot--update-doc): Use new helpers. * eglot-tests.el (hover-multiline-doc-locus): New test
…oc (again) Co-authored-by: Andreii Kolomoiets <andreyk.mad@gmail.com> Also do some refactoring to join similar logic in eglot-doc-too-large-for-echo-area and eglot--truncate-string. * eglot.el (eglot-doc-too-large-for-echo-area): Now returns the number of lines available. (eglot--truncate-string): New helper. (eglot--first-line-of-doc, eglot--top-lines-of-doc): Remove. (eglot--update-doc): Use new helpers. * eglot-tests.el (hover-multiline-doc-locus): New test
Co-authored-by: Andreii Kolomoiets <andreyk.mad@gmail.com> Also do some refactoring to join similar logic in eglot-doc-too-large-for-echo-area and eglot--truncate-string. * eglot.el (eglot-doc-too-large-for-echo-area): Now returns the number of lines available. (eglot--truncate-string): New helper. (eglot--first-line-of-doc, eglot--top-lines-of-doc): Remove. (eglot--update-doc): Use new helpers. * eglot-tests.el (hover-multiline-doc-locus): New test #459: joaotavora/eglot#459
Co-authored-by: Andreii Kolomoiets <andreyk.mad@gmail.com> Also do some refactoring to join similar logic in eglot-doc-too-large-for-echo-area and eglot--truncate-string. * eglot.el (eglot-doc-too-large-for-echo-area): Now returns the number of lines available. (eglot--truncate-string): New helper. (eglot--first-line-of-doc, eglot--top-lines-of-doc): Remove. (eglot--update-doc): Use new helpers. * eglot-tests.el (hover-multiline-doc-locus): New test GitHub-reference: close joaotavora/eglot#459
Let's continue our journey to perfect eldoc message! :)
This PR provides the possibility to show a full first line of the hover/signature info.
Current behavior
eldoc-echo-area-use-multiline-pvaluenil"Hey, where is the rest of the signature info? I just finished with the tenth arg of the func and want to know what the eleventh one is about!"
"Why, Eglot, why?"
Proposed behavior
eldoc-echo-area-use-multiline-pvaluenil"The echo area occupy only one line and doesn't break my shiny windows layout. Whoo-hoo!"
t"I can see as much hover info as the echo area can fit. Very informative! Awesome!"
"OMG this is the most convenient way to display hover and (especially) signature info! Great job, Eglot!"
Bonus
Honor
eglot--messageprefix length when truncating message.(setq eglot-put-doc-in-help-buffer t)The echo area content:
Current behavior
Proposed behavior
Thanks!