In Emacs 28, eldoc now passes in a callback to the documentation functions. This breaks org-eldoc as it currently is, as org-eldoc-documentation-function gets called with the wrong number of arguments. This patch makes it continue to work by setting the new variable eldoc-documentation-strategy, which puts eldoc in "backwards-compatability" mode. From 5c04048c0d1ed3f80c7dd3e6477e12fc8e760675 Mon Sep 17 00:00:00 2001 From: "James N. V. Cash" <james.nvc@gmail.com> Date: Fri, 10 Jul 2020 11:56:23 -0400 Subject: [PATCH] Make org-eldoc work with Emacs 28's new eldoc API Still using backward-compatability to use the old style of function. The new way is to make the documentation function take a callback, but this approach means the fewest changes. --- contrib/lisp/org-eldoc.el | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/contrib/lisp/org-eldoc.el b/contrib/lisp/org-eldoc.el index 72b10a1fb..060674b24 100644 --- a/contrib/lisp/org-eldoc.el +++ b/contrib/lisp/org-eldoc.el @@ -161,11 +161,15 @@ (defun org-eldoc-load () "Set up org-eldoc documentation function." (interactive) - (if (boundp 'eldoc-documentation-functions) - (add-hook 'eldoc-documentation-functions - #'org-eldoc-documentation-function nil t) - (setq-local eldoc-documentation-function - #'org-eldoc-documentation-function))) + (cond + ((boundp 'eldoc-documentation-strategy) + (setq-local eldoc-documentation-strategy + #'org-eldoc-documentation-function)) + ((boundp 'eldoc-documentation-functions) + (add-hook 'eldoc-documentation-functions + #'org-eldoc-documentation-function nil t)) + (t (setq-local eldoc-documentation-function + #'org-eldoc-documentation-function)))) ;;;###autoload (add-hook 'org-mode-hook #'org-eldoc-load) -- 2.17.1
Hello,
"James N. V. Cash" <james.nvc@gmail.com> writes:
> In Emacs 28, eldoc now passes in a callback to the documentation
> functions. This breaks org-eldoc as it currently is, as
> org-eldoc-documentation-function gets called with the wrong number of
> arguments.
>
> This patch makes it continue to work by setting the new variable
> eldoc-documentation-strategy, which puts eldoc in
> "backwards-compatability" mode.
Thank you.
Do we need another variable for that? Could
org-eldoc-documentation-funicton catch wrong-number-of-arguments error,
and try another call instead?
Regards,
--
Nicolas Goaziou
Nicolas Goaziou <mail@nicolasgoaziou.fr> writes: > Do we need another variable for that? Could > org-eldoc-documentation-function catch wrong-number-of-arguments error, > and try another call instead? I believe that the issue is org-eldoc-documentation-function itself is called with the wrong number of arguments; the new behaviour of eldoc-mode is to call the documentation functions with a callback argument, that the documentation function invokes to return its value, unless the strategy is a function. From the docstring for eldoc-documentation-strategy: > ... > For backward compatibility to the "old" protocol, this variable > can also be set to a function that returns nil or a doc string, > depending whether or not there is documentation to display at > all. Another approach would be to, I suppose, make org-eldoc-documentation-function take an optional callback argument (defaulting to identity) and either wrap the whole body of the function in a funcall of that function or pass the callback to all the functions that org-eldoc-documentation-function calls.
"James N. V. Cash" <james.nvc@gmail.com> writes: > This patch makes it continue to work by setting the new variable > eldoc-documentation-strategy, which puts eldoc in > "backwards-compatability" mode. How involved would it be to make org-eldoc work in non-"backwards-compatibility" mode? > From 5c04048c0d1ed3f80c7dd3e6477e12fc8e760675 Mon Sep 17 00:00:00 2001 > From: "James N. V. Cash" <james.nvc@gmail.com> > Date: Fri, 10 Jul 2020 11:56:23 -0400 > Subject: [PATCH] Make org-eldoc work with Emacs 28's new eldoc API > > Still using backward-compatability to use the old style of > function. The new way is to make the documentation function take a > callback, but this approach means the fewest changes. > --- > contrib/lisp/org-eldoc.el | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/contrib/lisp/org-eldoc.el b/contrib/lisp/org-eldoc.el > index 72b10a1fb..060674b24 100644 > --- a/contrib/lisp/org-eldoc.el > +++ b/contrib/lisp/org-eldoc.el > @@ -161,11 +161,15 @@ > (defun org-eldoc-load () > "Set up org-eldoc documentation function." > (interactive) > - (if (boundp 'eldoc-documentation-functions) > - (add-hook 'eldoc-documentation-functions > - #'org-eldoc-documentation-function nil t) > - (setq-local eldoc-documentation-function > - #'org-eldoc-documentation-function))) > + (cond > + ((boundp 'eldoc-documentation-strategy) > + (setq-local eldoc-documentation-strategy > + #'org-eldoc-documentation-function)) > + ((boundp 'eldoc-documentation-functions) > + (add-hook 'eldoc-documentation-functions > + #'org-eldoc-documentation-function nil t)) Both eldoc-documentation-strategy and eldoc-documentation-functions are new in Emacs 28, so if one is defined, then so is the other. More importantly, functions added to eldoc-documentation-functions must take at least one argument, so org-eldoc-documentation-function is not a suitable function in its current state. > + (t (setq-local eldoc-documentation-function > + #'org-eldoc-documentation-function)))) Thanks, -- Basil
Basil L. Contovounesios writes: > "James N. V. Cash" <james.nvc@gmail.com> writes: > >> This patch makes it continue to work by setting the new variable >> eldoc-documentation-strategy, which puts eldoc in >> "backwards-compatability" mode. > > How involved would it be to make org-eldoc work in > non-"backwards-compatibility" mode? I think we can do that, while still supporting Org's minimum Emacs version, by following python.el. Here's what it does: (with-no-warnings ;; supress warnings about eldoc-documentation-function being obsolete (if (null eldoc-documentation-function) ;; Emacs<25 (set (make-local-variable 'eldoc-documentation-function) #'python-eldoc-function) (if (boundp 'eldoc-documentation-functions) (add-hook 'eldoc-documentation-functions #'python-eldoc-function nil t) (add-function :before-until (local 'eldoc-documentation-function) #'python-eldoc-function)))) And then... >> - (if (boundp 'eldoc-documentation-functions) >> - (add-hook 'eldoc-documentation-functions >> - #'org-eldoc-documentation-function nil t) >> - (setq-local eldoc-documentation-function >> - #'org-eldoc-documentation-function))) >> + (cond >> + ((boundp 'eldoc-documentation-strategy) >> + (setq-local eldoc-documentation-strategy >> + #'org-eldoc-documentation-function)) >> + ((boundp 'eldoc-documentation-functions) >> + (add-hook 'eldoc-documentation-functions >> + #'org-eldoc-documentation-function nil t)) > > Both eldoc-documentation-strategy and eldoc-documentation-functions are > new in Emacs 28, so if one is defined, then so is the other. > > More importantly, functions added to eldoc-documentation-functions must > take at least one argument, so org-eldoc-documentation-function is not a > suitable function in its current state. ... org-eldoc-documentation-function's signature could be changed to (&rest _ignored), like python-eldoc-function's.
Kyle Meyer <kyle@kyleam.com> writes:
> Basil L. Contovounesios writes:
>> How involved would it be to make org-eldoc work in
>> non-"backwards-compatibility" mode?
>
> I think we can do that, while still supporting Org's minimum Emacs
> version, by following python.el. Here's what it does:
> ...
>
> ... org-eldoc-documentation-function's signature could be changed to
> (&rest _ignored), like python-eldoc-function's.
This makes the most sense to me; I missed that the default documentation
strategy also allows the function to ignore the callback & just return a
docstring directly.
James N. V. Cash writes: > Kyle Meyer <kyle@kyleam.com> writes: > >> Basil L. Contovounesios writes: >>> How involved would it be to make org-eldoc work in >>> non-"backwards-compatibility" mode? >> >> I think we can do that, while still supporting Org's minimum Emacs >> version, by following python.el. Here's what it does: >> ... >> >> ... org-eldoc-documentation-function's signature could be changed to >> (&rest _ignored), like python-eldoc-function's. > > This makes the most sense to me; I missed that the default documentation > strategy also allows the function to ignore the callback & just return a > docstring directly. All right, thanks. Here's that in patch form. I briefly tested with Emacs 26, 27, and 28, and things seemed to work fine (though I'm not an org-eldoc user). I'll plan to apply it in a day or two unless there are objections. -- >8 -- Subject: [PATCH] org-eldoc: Fix compatibility with Emacs 28 * contrib/lisp/org-eldoc.el (org-eldoc-documentation-function): Accept and ignore additional arguments for compatibility with Emacs 28. (org-eldoc-load): Use add-function to register org-eldoc-documentation-function for Emacs versions 25 through 27, as documented in eldoc-documentation-function. See Emacs's fd020a2931 (eldoc: modify `eldoc-documentation-function' using `add-function', 2014-12-05) and c0fcbd2c11 (Expose ElDoc functions in a hook (Bug#28257), 2020-02-25) for more information on the Emacs 25 and Emacs 28 changes, respectively. --- contrib/lisp/org-eldoc.el | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/contrib/lisp/org-eldoc.el b/contrib/lisp/org-eldoc.el index 72b10a1fb..b89eb0918 100644 --- a/contrib/lisp/org-eldoc.el +++ b/contrib/lisp/org-eldoc.el @@ -127,7 +127,7 @@ (declare-function css-eldoc-function "css-eldoc" ()) (declare-function php-eldoc-function "php-eldoc" ()) (declare-function go-eldoc--documentation-function "go-eldoc" ()) -(defun org-eldoc-documentation-function () +(defun org-eldoc-documentation-function (&rest _ignored) "Return breadcrumbs when on a headline, args for src block header-line, calls other documentation functions depending on lang when inside src body." (or @@ -161,11 +161,17 @@ (defun org-eldoc-documentation-function () (defun org-eldoc-load () "Set up org-eldoc documentation function." (interactive) - (if (boundp 'eldoc-documentation-functions) - (add-hook 'eldoc-documentation-functions - #'org-eldoc-documentation-function nil t) - (setq-local eldoc-documentation-function - #'org-eldoc-documentation-function))) + ;; This approach is taken from python.el. + (with-no-warnings + (if (null eldoc-documentation-function) + ;; Emacs<25 + (setq-local eldoc-documentation-function + #'org-eldoc-documentation-function) + (if (boundp 'eldoc-documentation-functions) + (add-hook 'eldoc-documentation-functions + #'org-eldoc-documentation-function nil t) + (add-function :before-until (local 'eldoc-documentation-function) + #'org-eldoc-documentation-function))))) ;;;###autoload (add-hook 'org-mode-hook #'org-eldoc-load) base-commit: e62ca4a1bf576a2c498f47536d3f12cd698e3ac0 -- 2.27.0
Kyle Meyer <kyle@kyleam.com> writes: > All right, thanks. Here's that in patch form. I briefly tested with > Emacs 26, 27, and 28, and things seemed to work fine (though I'm not an > org-eldoc user). I'm not either, but it seems to get pulled in automatically when org-plus-contrib is installed - that's how I noticed the errors in Org buffers. [...] > @@ -161,11 +161,17 @@ (defun org-eldoc-documentation-function () > (defun org-eldoc-load () > "Set up org-eldoc documentation function." > (interactive) > - (if (boundp 'eldoc-documentation-functions) > - (add-hook 'eldoc-documentation-functions > - #'org-eldoc-documentation-function nil t) > - (setq-local eldoc-documentation-function > - #'org-eldoc-documentation-function))) > + ;; This approach is taken from python.el. > + (with-no-warnings > + (if (null eldoc-documentation-function) > + ;; Emacs<25 > + (setq-local eldoc-documentation-function > + #'org-eldoc-documentation-function) > + (if (boundp 'eldoc-documentation-functions) > + (add-hook 'eldoc-documentation-functions > + #'org-eldoc-documentation-function nil t) > + (add-function :before-until (local 'eldoc-documentation-function) > + #'org-eldoc-documentation-function))))) LGTM. My only aesthetic nit would be to replace the nested if with a flat cond, but that's entirely up to you. Thanks, -- Basil
On Fri, 2020-07-17 at 01:41, Kyle Meyer <kyle@kyleam.com> wrote: > All right, thanks. Here's that in patch form. I briefly tested with > Emacs 26, 27, and 28, and things seemed to work fine (though I'm not an > org-eldoc user). > I'll plan to apply it in a day or two unless there are objections. So far so good here. It fixes the errors running 28.0.50 (85eaa83 from 2020-07-16) and the latest org-mode-contrib. Thank you, Joe
"Basil L. Contovounesios" <contovob@tcd.ie> writes:
> Kyle Meyer <kyle@kyleam.com> writes:
>
>> All right, thanks. Here's that in patch form. I briefly tested with
>> Emacs 26, 27, and 28, and things seemed to work fine (though I'm not an
>> org-eldoc user).
>
> I'm not either, but it seems to get pulled in automatically when
> org-plus-contrib is installed - that's how I noticed the errors in Org
> buffers.
I was also trying to figure out how to shut it off, and there were no
obvious knobs to do so. Given that this gets installed implicitly, I
think maybe it shouldn't be turned on by default. Isn't this what
`org-modules' is for?
Basil L. Contovounesios writes:
>> @@ -161,11 +161,17 @@ (defun org-eldoc-documentation-function ()
>> (defun org-eldoc-load ()
>> "Set up org-eldoc documentation function."
>> (interactive)
>> - (if (boundp 'eldoc-documentation-functions)
>> - (add-hook 'eldoc-documentation-functions
>> - #'org-eldoc-documentation-function nil t)
>> - (setq-local eldoc-documentation-function
>> - #'org-eldoc-documentation-function)))
>> + ;; This approach is taken from python.el.
>> + (with-no-warnings
>> + (if (null eldoc-documentation-function)
>> + ;; Emacs<25
>> + (setq-local eldoc-documentation-function
>> + #'org-eldoc-documentation-function)
>> + (if (boundp 'eldoc-documentation-functions)
>> + (add-hook 'eldoc-documentation-functions
>> + #'org-eldoc-documentation-function nil t)
>> + (add-function :before-until (local 'eldoc-documentation-function)
>> + #'org-eldoc-documentation-function)))))
>
> LGTM. My only aesthetic nit would be to replace the nested if with a
> flat cond, but that's entirely up to you.
I agree with your preference, though it didn't cross my mind when I was
lazily copying over from python.el. Will update.
Thanks.
Joseph Mingrone writes: > On Fri, 2020-07-17 at 01:41, Kyle Meyer <kyle@kyleam.com> wrote: [...] >> I'll plan to apply it in a day or two unless there are objections. > > So far so good here. It fixes the errors running 28.0.50 (85eaa83 from > 2020-07-16) and the latest org-mode-contrib. Thanks for testing. Pushed (b2b587387).