Allow users to tweak *only* eldoc-documentation-functions

[bigodel]

In the body of eglot--managed-mode, the following

eglot/eglot.el

Line 1956 in db91d58

(unless (eglot--stay-out-of-p 'eldoc)

should actually be

 (unless (eglot--stay-out-of-p 'eldoc-documentation-functions)

that way we capture both eglot and eglot-documentation-functions in eglot-stay-out-of i.e. if you want to manage ElDoc as a whole, use eglot in eglot-stay-out-of; although if you only to manage a particular ElDoc variable (in my use case I want to have Eglot set eldoc-documentation-strategy but not the documentation functions) you can use that instead.

We'd also need to move the call to (eglot-mode 1) outside of that unless, but it could (and should) remain as

  (unless (eglot--stay-out-of-p 'eldoc)

(or even (eglot--stay-out-of-p 'eldoc-mode)?)

[joaotavora]

I more or less get the idea, but I find your proposal confusing. It would be easier to understand with a patch (and hopefully to Emacs's lisp/progmodes/eglot (mirrored here for example: https://github.com/emacs-mirror/emacs/blob/master/lisp/progmodes/eglot.el)

use eglot in eglot-stay-out-of;

You must mean "eldoc in eglot-stay-out-of", surely.

[bigodel]

I more or less get the idea, but I find your proposal confusing. It would be easier to understand with a patch (and hopefully to Emacs's lisp/progmodes/eglot (mirrored here for example: https://github.com/emacs-mirror/emacs/blob/master/lisp/progmodes/eglot.el)

Sorry, I should've provided at least a diff. I'll do that in the next couple of hours, but I'm not at the computer right now.

You must mean "eldoc in eglot-stay-out-of", surely.

Yep, my ADHD brain gets those mixed up all the time (I've even thought some weird behaviours to be a bug only to find out I mixed eglot for eldoc or vice-versa haha).

[bigodel]

Here's a diff of the proposed change:

--- eglot.el	2024-05-08 19:12:54.755392314 -0300
+++ eglot-proposal.el	2024-05-08 19:15:16.176628500 -0300
@@ -1968,12 +1968,12 @@
       (add-function :before-until (local 'imenu-create-index-function)
                     #'eglot-imenu))
     (unless (eglot--stay-out-of-p 'flymake) (flymake-mode 1))
-    (unless (eglot--stay-out-of-p 'eldoc)
+    (unless (eglot--stay-out-of-p 'eldoc-documentation-functions)
       (add-hook 'eldoc-documentation-functions #'eglot-hover-eldoc-function
                 nil t)
       (add-hook 'eldoc-documentation-functions #'eglot-signature-eldoc-function
-                nil t)
-      (eldoc-mode 1))
+                nil t))
+    (unless (eglot--stay-out-of-p 'eldoc) (eldoc-mode 1))
     (cl-pushnew (current-buffer) (eglot--managed-buffers (eglot-current-server))))
    (t
     (remove-hook 'after-change-functions #'eglot--after-change t)

Instead of managing eldoc-documentation-functions unless the symbol eldoc is in eglot-stay-out-of, we let the user choose whether to manage the documentation functions itself, but allowing Eglot to enable eldoc-mode, just like it is done with eldoc-documentation-strategy. This way, if eglot-stay-out-of is '(eldoc-documentation-functions), we'd still enable eldoc-mode, but wouldn't do anything regarding documentation functions, whereas if the value of the variable is '(eldoc) Eglot would refrain from doing anything regarding ElDoc.

Is that clearer? I can provide a patch in the mailing list, but since I have other proposed ideas I'd rather discuss all of them and propose a single patch with all of the changes.

[joaotavora]

I've given you my input.

[bigodel]

I've given you my input.

So even with my reasoning you still don't think it makes sense? Even considering it's a behaviour that works for one of the ElDoc variables but not the other? Don't you think that's inconsistent?

Also, could you please check whether what I reported in this but report is indeed a bug or not? Because your solution wouldn't work with the current behaviour of eglot-managed-mode-hook, given that it only triggers when the mode is activated and not when it is deactivated, hence I couldn't tear down my Eglot-specific ElDoc configs. But even if that's fixed I still think allowing for staying out of eldoc-documentation-functions only is worth it because it 1. consistent with other ElDoc variables configured by Eglot, 2. tear down of other ElDoc variables is done correctly, even resetting the variable to a previous buffer-local value if it was set that way (without having the user create a variable to store its previous value, just using Eglot's internals), 3. this change would still work with previous version and 4. would only add 1 line of code

[joaotavora]

That bug is a confirmed regression, it will be fixed soon. But my suggestion is to set e-d-strategy to e-d-compose globally and you don't need the fix.

[bigodel]

But my suggestion is to set e-d-strategy to e-d-compose globally and you don't need the fix.

I don't like that as my global value for this variable, I only like it when using Eglot.

[bigodel]

but in summary this use case isn't with the complexity and subtlety to Eglot configuration, in my opinion

I don't understand why this use case isn't, but eldoc-documentation-strategy is. If I have (setq eglot-stay-out-of '(eldoc-documentation-strategy)), I can have Eglot not touch that variable, but still enable eldoc-mode and set e-d-functions. In fact, any variable configured with eglot--setq-saving will behave like that e.g. flymake-diagnostic-functions.

I only proposed such a change because I was caught off guard when I expected (setq eglot-stay-out-of '(eldoc-documentation-functions)) to behave like e-d-strategy but it didn't. The current behaviour is inconsistent and might lead to more confusion for other users.

Please do keep in mind that I'm just trying to understand why the hesitancy on introducing such a small change that, in my honest opinion, only makes eglot-stay-out-of's behaviour more consistent with little to no code and no breaking change.