[PATCH] Make org-eldoc work with Emacs 28

[PATCH] Make org-eldoc work with Emacs 28

[James N. V. Cash]

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

Re: [PATCH] Make org-eldoc work with Emacs 28

[Nicolas Goaziou]

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

Re: [PATCH] Make org-eldoc work with Emacs 28

[James N. V. Cash]

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.

Re: [PATCH] Make org-eldoc work with Emacs 28

[Basil L. Contovounesios]

"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

Re: [PATCH] Make org-eldoc work with Emacs 28

[Kyle Meyer]

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.

Re: [PATCH] Make org-eldoc work with Emacs 28

[James N. V. Cash]

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.

[PATCH] org-eldoc: Fix compatibility with Emacs 28

[Kyle Meyer]

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

Re: [PATCH] org-eldoc: Fix compatibility with Emacs 28

[Basil L. Contovounesios]

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

Re: [PATCH] org-eldoc: Fix compatibility with Emacs 28

[Eric Abrahamsen]

"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?

Re: [PATCH] org-eldoc: Fix compatibility with Emacs 28

[Kyle Meyer]

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.

Re: [PATCH] org-eldoc: Fix compatibility with Emacs 28

[Joseph Mingrone]

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

Re: [PATCH] org-eldoc: Fix compatibility with Emacs 28

[Kyle Meyer]

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).