bug#70958: 30.0.50; eglot-managed-mode hooks not called on shutdown

[João Távora]

merge 70958 70835
thanks

On Mon, May 27, 2024 at 3:32 PM Troy Brown <brownts@troybrown.dev> wrote:
>
> On Mon, May 27, 2024 at 10:09 AM João Távora <joaotavora@gmail.com> wrote:
> >
> > Bugs are only "legitimate" when they are harming someone somewhere.
> > This hook has been there for a number of years, and noone has complained
> > that I can remember. If you have a use for the on-shutdown, then it's
> > a bug.  It'd help to know about this use case. If you don't have a use,
> > it's just a doc bug, and patches welcome.

Actually I was wrong.  I have been recently warned of this exact
same issue.  I thought you and that person were the same.
bug#70835, which this bug is a duplicate of (so I've merged them,
hopefully)

> The use case is that I was experimenting with updating the
> buffer-local indent-region-function (and indirectly
> indent-line-function) to be based on eglot-format when the buffer was
> connected to the language server.  I was attempting to use the
> eglot-managed-mode-hook so I could update these variables when the
> Eglot buffer management changed.  Since the hook wasn't being called
> on shutdown it would still attempt to call eglot-format when it was no
> longer managing the buffer.  The workaround was to use a mode-specific
> function for indent-region-function and then having that call
> eglot-managed-p to determine if it should call eglot-format or
> something else (e.g., indent-relative).

Anyway, to your use case.  The "off" hook would solve your problem,
but not as well as your solution.  When setting variables, there's
no clean solution to the "undo problem", unless the variable in
question is a hook.  Think:

 var is originally X
 activate minor mode foo, saves var value of X, sets to Y,
 activate minor mode bar, that also sets var, saves Y, sets to Z
 deactivate foo, sets var to X
 deactivate bar, sets var to Y
 now both modes are inactive, variable is set to Y, in error

When the variable being affected is a hook with certain rules, this
problem doesn't exist.

Anyway, it's not a problem for Eglot to solve.  So given there is
also bug#70835 requesting the same, I think we can risk just running
eglot-managed-mode-hook like so let's try this patch:

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 6896baf30ce..2fab9e7f38b 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -2059,6 +2059,7 @@ eglot--managed-mode
     (when eglot--current-flymake-report-fn
       (eglot--report-to-flymake nil)
       (setq eglot--current-flymake-report-fn nil))
+    (run-hooks 'eglot-managed-mode-hook)
     (let ((server eglot--cached-server))
       (setq eglot--cached-server nil)
       (when server

There will possibly be people complaining we broke their configs,
so this might not be the end of the story. But it's reasonable to try it
since this is how  the documentation says it _should_ work and is
consistent with the normal minor-mode hooks.

João