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

[João Távora]

By the way Troy, can you show your indent-region-function based
on eglot-format? I once tried this and it didn't work very well.  With the
clangd server.  Would like to see your attempt.

João

On Mon, May 27, 2024 at 4:45 PM João Távora <joaotavora@gmail.com> wrote:
>
> 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



-- 
João Távora