Clean up eldoc handling to prepare better customizability

[ilohmar]

This is an early push to get feedback if I am moving into the right
direction. Please let me know if this is acceptable.

Basically, I would like some more custom options when/how to display
what part of hover/signature information. This area already has a
tradition of issues and feature requests, eg, PR #111 and, issue #117
etc.

In my understanding, based on eldoc documentation, eglot is currently
messing with eldoc's internals. I think that is unneccessary, hence
this PR.

As a result, there's also less global state, and the simpler code has
already helped clear the waters for some more changes that I am
currently trying locally.

[ilohmar]

BTW, I've assigned copyright for Emacs contributions to the FSF years
ago...

[ilohmar]

Not sure what that is supposed to mean.. I dir read the output and do not understand it. Notably, make check passes, that's what I cared about...

[joaotavora]

@AlexanderBarbosa, might have been a problem with the CI system.

@ilohmar make check is important, but it skips many tests when the user doesn't have the necessary LSP installed. A run that passes on Travis CI is more reliable, and a run that passes without skipping any tests even more so.

More importantly, while the PR looks good, I don't understand something important, because I've been out of touch for a while. Basically, what problem are you solving?

More specifically what didn't/did happen before the PR that does/doesn' happen now? Is any of this user-visible? Or is it exclusively developer-visible? Or is is a combination of both?

[ilohmar]

@joaotavora Sorry that I obviously did not make that clearer: So far, this should not have any user-visible effect, it is just a refactoring to treat the eldoc machinery in a nicer way.. I am doing this in a separate PR because I wanted to get this through before spending time on PRs for the customization of the hover info display that I have in mind.

In short, if you would say that this is not gonna happen, I would not waste time on preparing user-visible PRs based on it.. :)

[joaotavora]

@ilohmar a maintenance-friendly PR is always welcome regardless of the inclusion of later user-visible PR's, so you did well to separate (you just didn't explain exactly what the motivation was, but I understand now).

Regarding the test there seems to be something potentially related to your PR in test nr 10, "hover-after-completions". I suspect this because I see
(...truncated. Full help is in `*eglot-help for exit*')
in the test log. It might be that your code is inadvertently waiting for user input.

[ilohmar]

I saw that, but could not make any sense of it at first.. Think I am getting it now. Not easy to test with the LSP dependencies.

It appears that the docs of python's sys.exit are long and displayed in a separate buffer only. With the PR, such code is no longer in the eldoc--message-function, eldoc-message is not called, hence eldoc-last-message is never set (which is what the code is waiting for).

The thing is.. I believe this is correct, eldoc-last-message should not be set in this case. I will try to verify that and config the test scenario such that it always uses eldoc-message.

[joaotavora]

Not easy to test with the LSP dependencies.

And yet, this is what Eglot has to work with in the end...

[ilohmar]

Sure, it's just hard to separate whether it's a test of the environment, the language server, or of eglot itself. Case in point: When I use my local pyls language server, all of eglot's test with it fail :( Wrong virtual environment or whatever.. I am just happy that I am not currently working with Python anymore.

[joaotavora]

Sure, it's just hard to separate whether it's a test of the environment, the language server, or of eglot itself.

Sorry, I disagree. They are all tests of eglot: given this environment and that particular fully-compliant LSP server (the so-called fixture), eglot has to behave so and so. If it doesn't, it's either a failure or a miscoded test.

That said, I agree it's suboptimal that these kinds of tests require users to install additional dependencies. Maybe Eglot could come bundled with a LSP server simulator, solving the situation. But it doesn't so this is better than no integration tests at all.

[ilohmar]

It was an off-handed comment, sorry for that. Testing methodology is, understandably, a sensitive issue for many people, myself included. I did not and do not want to sidetrack this issue and will refrain from further comments on tests.

[joaotavora]

It was an off-handed comment, sorry for that.

No need to apologize. I was just clearing something up: I'm not testing the server (I could be, but I'm not) I can explain better if you think it's worth it.

Running the test suite is important (and it evidenced a problem in this case), but if running the full test suite is indeed inconvenient (which it completely it is, because of the server dependencies), I can run it for you, that's okay too!

And thanks for working on this.

[ilohmar]

All's well. Is there anything else for me to do or do you just need some time to review the PR before (hopefully) merging?

[joaotavora]

All's well. Is there anything else for me to do or do you just need some time to review the PR before (hopefully) merging?

Changing a test code for a refactoring raises some (minor) alarms for me. I need time to review what exactly is going on here. There is an Eglot extension somewhere, actually an eldoc extension motivated by Eglot, but completely independent from it. That extensions shows eldoc in a separate window. I wanted to make sure it still works with Eglot. I also wanted to check with emacs-devel that calling eldoc-message is still the recommended way for async backends (like Eglot) to display messages.

[ilohmar]

The async question w.r.t. eldoc is very interesting, indeed, I did not want to touch that for the time being.

As for the test code: This is getting to the core of what I am working on. It just ensures that we actually use eldoc's facilities when we test for it.

Here are some more thoughts why I think that it is correct that the help buffer display does not pass through eldoc; this also relates to the eldoc extension you are mentioning, and I am starting to see things more clearly (I think):

1. eldoc (to me, and judging by its docstrings) is meant to provide unintrusive, single-line (I am working on customization for this..) feedback for the context around point, in the minibuffer by default. That is, eg, how the emacs-lisp-mode setup does it. I am trying not to change any of this.

2. There is a want/need for more extensive "electric" documentation. This is why there is the help-buffer of eglot with the associated settings. I regard the "eglot help-buffer display" as a new and additional "doc display mechanism". I quite enjoy it, but I also think it is unnecessarily coupled to the eldoc machinery. This PR tries to loosen that coupling, and only pass info to eldoc that conforms to (my current understanding of) its docstrings. That is the only reason why the test has been adjusted.

3. In the generic eglot--update-doc, we can add customization settings for how any doc info is displayed. For example, I would like to always have a single-line echo area output, /independent/ of whether I show lots of info in the help buffer. This I plan to do in follow-up PRs.

4. In the long term, eldoc might adopt or gain similar additions for displaying longer information, or always use the eldoc box extension etc. It should be simple to accomodate such user customizations, because we now have a single place where information is routed to different displaying facilities (before it gets to eldoc).

I hope this is useful and not too long-winded. Please let me know if I can help things along in any other way.

[joaotavora]

quite enjoy it, but I also think it is unnecessarily coupled to the eldoc machinery. This PR tries to loosen that coupling, and only pass info to eldoc that conforms to (my current understanding of) its docstrings. That is the only reason why the test has been adjusted.

This makes sense.

In the long term, eldoc might adopt or gain similar additions for displaying longer information, or always use the eldoc box extension etc. It should be simple to accomodate such user customizations, because we now have a single place where information is routed to different displaying facilities (before it gets to eldoc).

This makes sense too.

I like your reasoning and you should basically copy-paste your post to emacs-devel and start a discussion there.

But, in the meantime, can we be sure not to have broken https://github.com/casouri/eldoc-box?

If the answer is "no, we haven't" then I will try to merge this asap so you can propose your other PR's. If it is "yes, a little", then we'll see what we can do: perhaps we can adjust eldoc-box somewhat and still merge your PR.

[ilohmar]

I like your reasoning and you should basically copy-paste your post to emacs-devel and start a discussion there.

Well, that's a different project (for me) that I will not get to in the near future..

But, in the meantime, can we be sure not to have broken https://github.com/casouri/eldoc-box?

If the answer is "no, we haven't" then I will try to merge this asap so you can propose your other PR's. If it is "yes, a little", then we'll see what we can do: perhaps we can adjust eldoc-box somewhat and still merge your PR.

It is somewhat unfortunate to couple these questions, but I will try it later. From a glance at the code, I expect that eldoc-box will continue to work if one sets eglot-put-doc-in-help-buffer to nil (as for the test). Again, that would be correct, it just means that eglot always uses the eldoc facilities, in order to "tunnel" information to eldoc-box.

It mostly boils down to separating what is displayed from where/how it is displayed.

[joaotavora]

From a glance at the code, I expect that eldoc-box will continue to work if one sets eglot-put-doc-in-help-buffer to nil (as for the test)

And does it work currently (without this PR) regardless if one sets eglot-put-doc-in-help-buffer or not?

[ilohmar]

Sure, b/c right now, eglot pushes everything through eldoc via the :before-until function eglot--eldoc-message.

"Works" is a somewhat vague term here.. :) Under all the various circumstances, eldoc-box shows what eldoc gets. But eldoc currently gets quite some stuff that IMO it should not get.

[ilohmar]

Sorry, that was misleading: eldoc currently always uses eldoc-message, regardless of whether eldoc is the intended recipient. Therefore, all of this gets to eldoc-box as well.

[joaotavora]

Sorry, that was misleading: eldoc currently always uses eldoc-message, regardless of whether eldoc is the intended recipient. Therefore, all of this gets to eldoc-box as well.

OK, I understand. So in summary: there is an impact, but you think its advantages outweigh the disadvantages.

I think I agree: when eglot-put-doc-in-help-buffer is t, things don't go through eldoc at all.

[ilohmar]

Yes, that's one way of putting it. And if/when eldoc is changed to accommodate larger documentation and/or multiple ways of displaying it (help buffer, childframe..), it will be easy to adapt (basically just replacing eglot--update-doc by eldoc-message itself).

Would it help to add a README entry in eldoc-box regarding setting eglot-put-doc-in-help-buffer to nil? I could raise an issue there.

[ilohmar]

Just want to check back, because it's been a while..

What's the status on the PR? Are you contemplating the various diifferent options, or are you waiting on me to do sth? I don't intend to be annoying, probably you're just busy with other things :)

[joaotavora]

probably you're just busy with other things :)

Sorry, yes

[joaotavora]

I've pushed another commit to this branch with some minor corrections:

1. Added a comment explaining the test protection
2. Replaced the contorted unless/when structure with an ìf`

Please review and squash the 3 commits in one, use the body of this last commit's message in the squashed commit .

[ilohmar]

Excellent. I guess you mean to squash all commits of this PR into one and force-push, which I'll do. I'll also fix a typo in the comment (should refer to eldoc-last-message) and the commit message, that's it.

[joaotavora]

Merged it. Sorry for the delay.

[joaotavora]

Looking forward for your next contributions by the way, which I hope to review more diligently...

[casouri]

I just came across this discussion. So what this PR does is to filter out some long docs and only push short doc to eldoc, am I correct?

[casouri]

Also I like the idea of keeping eldoc under one line and separate more extensive docs out. A few major mode currently shows a lot of stuff in minibuffer and could use this separation improvement. As I see it, the extensive doc should be called out on demand to a pop up buffer or optionally a child frame, because when programming, we don’t really need doc for every function all the time. Therefore another separate mechanism could be added instead of extending eldoc, which is the opposite - always on for every function all the time.

Btw, I didn’t see any related discussion on Emacs-devel, have I missed it?

[casouri]

As I see it, the extensive doc should be called out on demand to a pop up buffer or optionally a child frame

Actually, that sound like Emacs help to me. Maybe it can be made generic for languages/major modes?

[ilohmar]

Hi, sorry, I did not post this to emacs-devel yet, b/c I did not find the time to think about what else is wanted and how to achieve it. I agree with your last but one message, and this was the core of this PR. If the user sets eglot-put-doc-in-help-buffer to nil, eldoc-box should continue to work as before, for the time being.

[ilohmar]

As I see it, the extensive doc should be called out on demand to a pop up buffer or optionally a child frame

Actually, that sound like Emacs help to me. Maybe it can be made generic for languages/major modes?

It should definitely be generic, and something like (or exactly) Emacs help. On my todo list is a PR to add options to eglot--update-doc (for where to put what info), and one target could be eldoc-box, another an explicit help buffer that eglot itself populates etc... Whenever I get around to it :)

[casouri]

As for generic, I actually mean extending emacs help or create another similar facility. We don’t need to limit ourself to eglot. 😉

[joaotavora]

I actually mean extending emacs help or create another similar facility. We don’t need to limit ourself to eglot. 😉

Three thumbs up! 👍 👍 👍

[casouri]

Here are my thoughts:

First there are problems in the current state of documentation in many major modes. Specifically those who use eldoc to display extensive docs. It makes eldoc jump up and down. When the major mode truncates the doc, users can't see the whole documentation. I think people can confirm this by experience, but if not I can collect some proves. Other major modes use their own documentation facility and are naturally inconsistent across major modes.

I think it's safe to say many major modes requires some type of documentation facility to show documentation for a variable/function/class etc. And eldoc doesn't fit that role because it is intended for some quick information around point all the time automatically, e.g., function signature and one-line summary of function, etc. OTOH, (extensive) documentation doesn't need to be displayed all the time and is often too long to fit in the minibuffer. Most editors/IDE (including Emacs help) provide documentation by letting users call out documentation manually.

Now, given that Emacs already has a fantastic help system for Elisp, it would be nice if we can extend existing Emacs help so other major modes can take advantage of the nice features of it. This will reduce code and bring consistency, just like comint.el. Also, making the help system generic can let people easily add new features and all the major modes immediately get the extension for free. (An example of such extension is helpful.el.)

---

I have a vague idea of what such a help system could be:

1. a uniform entry point, e.g., describe-function/variable/class/symbol/etc
2. several containers to select from, e.g., popup buffer, childframe.
3. each major mode defines a backend to populate documentation buffer however they like.

Upon call, the help system sets up a container and lets major mode backend do its thing, async or not. The container can have some universal bindings by a help-minor-mode.

What do you think?

[casouri]

It's probably better to write a POC and just start using it myself.

https://github.com/casouri/ghelp