bug#36886: 26.2; (global-eldoc-mode -1) should disable eldoc completely

[Basil L. Contovounesios]

Noam Postavsky <npostavs@gmail.com> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> I actually don't understand why, when the globalized mode is ON, it
>> doesn't work in the minibuffer.  What am I missing?
>
> define-globalized-minor-mode only adds after-change-major-mode-hook,
> find-file-hook, and change-major-mode-hook.  AFAICT, the minibuffer
> stays in fundamental-mode, and doesn't visit a file so none of those are
> triggered.

The call to read-from-minibuffer in read--expression triggers
change-major-mode-hook and after-change-major-mode-hook.

These in turn call turn-on-eldoc-mode, which calls eldoc--supported-p,
which checks the value of eldoc-documentation-function.

The problem is that minibuffer-setup-hook, which sets
eldoc-documentation-function, has not yet run at this stage, so the
minibuffer is not yet considered suitable for enabling eldoc-mode.

> Possible patch attached (some additional manual updates +
> NEWS would be needed for define-globalized-minor-mode change).
>
> From d16b9c5dffb84c53bc6f55c4ca74fc8135d48d7f Mon Sep 17 00:00:00 2001
> From: Noam Postavsky <npostavs@gmail.com>
> Date: Sat, 3 Aug 2019 20:19:31 -0400
> Subject: [PATCH] Respect global-eldoc-mode in minibuffers (Bug#36886)
>
> * lisp/emacs-lisp/easy-mmode.el (define-globalized-minor-mode): Accept
> a BODY parameter.
> * lisp/emacs-lisp/eldoc.el (global-eldoc-mode): Add or remove
> eldoc-mode to eval-expression-minibuffer-setup-hook when enabling or
> disabling global-eldoc-mode.
> * lisp/simple.el (read--expression): Remove the no-longer-needed
> unconditional call to eldoc-mode.

Should this log entry mention bug#27202 as well?

[...]

> diff --git a/lisp/emacs-lisp/eldoc.el b/lisp/emacs-lisp/eldoc.el
> index 16b5863209..e5f1832de6 100644
> --- a/lisp/emacs-lisp/eldoc.el
> +++ b/lisp/emacs-lisp/eldoc.el
> @@ -207,7 +207,10 @@ eldoc-mode
>  (define-globalized-minor-mode global-eldoc-mode eldoc-mode turn-on-eldoc-mode
>    :group 'eldoc
>    :initialize 'custom-initialize-delay
> -  :init-value t)
> +  :init-value t
> +  (if global-eldoc-mode
> +      (add-hook 'eval-expression-minibuffer-setup-hook #'eldoc-mode)
> +    (remove-hook 'eval-expression-minibuffer-setup-hook #'eldoc-mode)))

This looks fine to me (the only alternative I can think of is creating a
new major mode specifically for read--expression), but shouldn't this
call to eldoc-mode be bundled in the same hook function with...

>  ;;;###autoload
>  (defun turn-on-eldoc-mode ()
> diff --git a/lisp/simple.el b/lisp/simple.el
> index e33709e8ad..f81400c073 100644
> --- a/lisp/simple.el
> +++ b/lisp/simple.el
> @@ -1583,7 +1583,6 @@ read--expression
>            ;; FIXME: call emacs-lisp-mode?
>            (add-function :before-until (local 'eldoc-documentation-function)
>                          #'elisp-eldoc-documentation-function)

...this advice?

> -          (eldoc-mode 1)
>            (add-hook 'completion-at-point-functions
>                      #'elisp-completion-at-point nil t)
>            (run-hooks 'eval-expression-minibuffer-setup-hook))

Thanks,

-- 
Basil