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

[João Távora]

On Sat, May 25, 2024 at 8:53 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Troy Brown <brownts@troybrown.dev>
> > Date: Wed, 15 May 2024 08:38:28 -0400
> >
> > The documentation for eglot-managed-mode-hook indicates that the hook
> > is run after Eglot has started/stopped managing a buffer.  I was
> > trying to use this to perform setup/teardown of functionality when
> > this happened, but it appears the registered hooks are never called on
> > shutdown.  The following is a little test that can be run in a buffer
> > which has LSP support, to demonstrate the problem.  My expectation is
> > that "Buffer not managed" is output and the my-eglot-hook-var is
> > changed to 'not-managed when a shutdown occurs.
> >
> > --8<---------------cut here---------------start------------->8---
> > (defun my-eglot-test ()
> >   (defun my-eglot-hook ()
> >     (message "my-eglot-hook invoked")
> >     (if (eglot-managed-p)
> >         (progn
> >           (message "Buffer is managed")
> >           (setq-local my-eglot-hook-var 'managed))
> >       (message "Buffer not managed")
> >       (setq-local my-eglot-hook-var 'not-managed)))
> >   (add-hook 'eglot-managed-mode-hook #'my-eglot-hook)
> >   (setq-local my-eglot-hook-var 'initial)
> >   (cl-assert (not (eglot-managed-p)))
> >   (cl-assert (eq my-eglot-hook-var 'initial))
> >   (call-interactively #'eglot)
> >   (cl-assert (eglot-managed-p))
> >   (cl-assert (eq my-eglot-hook-var 'managed))
> >   (sleep-for 3) ; wait for server connection
> >   (call-interactively #'eglot-shutdown)
> >   (cl-assert (not (eglot-managed-p)))
> >   (cl-assert (eq my-eglot-hook-var 'not-managed)))
> > --8<---------------cut here---------------end--------------->8---
>
> João, any comments or suggestions?

eglot-managed-mode-hook is an abnormal minor mode hook because there
is no eglot-managed-mode minor mode, there is only eglot--managed-mode
which is a "--" definition on purpose (that does have the normal hooks
of course).

So when e-m-m-hook was added it was made to run only on "turn on"
because that's where it was most needed.  We can try changing Eglot
to also run it on "turn off", but that is a backward incompatible
change.

Alternatively, we can add a new eglot-managed-mode-off-hook.

Whatever the decision, there is the additional question of _when_ to
run the "off".  Maybe it's simple to  decide, but at least the the
"on" e-m -m-hook is specially  designed to run after some
LSP communication has taken  place,  which is of the  reasons
eglot--managed-mode-hook wasn't  suitable.  So maybe the "off"
should run in a similarly careful symmetrical position.

Finally, Troy can probably also make use of the internal
eglot--managed-mode-hook.  It's a "--", unsupported and dangerous
in theory, in practice it should be OK.

I'm CCing Felicián and Stefan for suggestions.

João