GNU bug report logs - #45831
28.0.50; list-colors-display callback arg needs to evaluate to a function?

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 45831 in the body.
You can then email your comments to 45831 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 28.0.50; list-colors-display callback arg needs to evaluate to a
 function?
Date: Tue, 12 Jan 2021 20:27:28 -0300

Starting from emacs -Q:

1. Eval the following:
(list-colors-display nil nil
		     (let ((cbuf (current-buffer)))
		       (lambda (color)
			 (when (buffer-live-p cbuf)
			   (message "Picked color %s for buffer %s"
				    color (buffer-name cbuf))))))

2. Hit RET on any color.  I hit RET in the first one, which is black.

3. Get the following error:
Symbol’s function definition is void: closure


Docstring of list-colors-display says:
If the optional argument CALLBACK is non-nil, it should be a
function to call each time the user types RET or clicks on a
color.  The function should accept a single argument, the color name.

But I'm passing a function and it is erroring out.


If instead the CALLBACK argument is a form that evaluates to a function,
like in:
(list-colors-display nil nil
		     (let ((cbuf (current-buffer)))
		       `(function ,(lambda (color)
				    (when (buffer-live-p cbuf)
				      (message "Picked color %s for buffer %s"
					       color (buffer-name cbuf)))))))

it succeeds with: Picked color black for buffer *scratch*


This is not what is described in the docstring, but I think the code
should be fixed to allow the argument to be like the one provided in
step 1.


In GNU Emacs 28.0.50 (build 4, x86_64-pc-linux-gnu, GTK+ Version 3.22.30, cairo version 1.15.10)
 of 2021-01-12 built on tbb-desktop
Repository revision: c734ba68623279d814e857ddc536421a08c38f34
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12008000
System Description: Ubuntu 18.04.5 LTS

Configured features:
CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG
LIBSELINUX LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG RSVG SOUND THREADS
TIFF TOOLKIT_SCROLL_BARS X11 XDBE XIM XPM GTK3 ZLIB

Important settings:
  value of $LC_MONETARY: es_AR.UTF-8
  value of $LC_NUMERIC: es_AR.UTF-8
  value of $LC_TIME: es_AR.UTF-8
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
rfc822 mml mml-sec epa derived epg epg-config gnus-util rmail
rmail-loaddefs auth-source cl-seq eieio eieio-core cl-macs
eieio-loaddefs password-cache json map mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047
rfc2045 ietf-drums mm-util mail-prsvr mail-utils cl-print debug
backtrace find-func time-date subr-x cl-extra shortdoc
text-property-search seq byte-opt gv bytecomp byte-compile cconv
help-fns radix-tree color help-mode easymenu cl-loaddefs cl-lib
iso-transl tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel term/x-win x-win term/common-win x-dnd tool-bar
dnd fontset image regexp-opt fringe tabulated-list replace newcomment
text-mode elisp-mode lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core term/tty-colors frame minibuffer cl-generic
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray
cl-preloaded nadvice button loaddefs faces cus-face macroexp files
window text-properties overlay sha1 md5 base64 format env code-pages
mule custom widget hashtable-print-readable backquote threads dbusbind
inotify dynamic-setting system-font-setting font-render-setting cairo
move-toolbar gtk x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 113889 8725)
 (symbols 48 7333 1)
 (strings 32 21356 2142)
 (string-bytes 1 682695)
 (vectors 16 12605)
 (vector-slots 8 175429 11483)
 (floats 8 158 307)
 (intervals 56 4536 253)
 (buffers 984 16))

Message #8 received at 45831 <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: 45831 <at> debbugs.gnu.org
Subject: Re: bug#45831: 28.0.50; list-colors-display callback arg needs to
 evaluate to a function?
Date: Tue, 12 Jan 2021 20:59:29 -0300

[Message part 1 (text/plain, inline)]

tags 45831 patch
quit


Backward-compatible change attached.  With this change, all of the
following work:

(list-colors-display nil nil
		     (let ((cbuf (current-buffer)))
		       (lambda (color)
			 (when (buffer-live-p cbuf)
			   (message "Picked color %s for buffer %s"
				    color (buffer-name cbuf))))))

(list-colors-display nil nil
		     (let ((cbuf (current-buffer)))
		       `(function ,(lambda (color)
				    (when (buffer-live-p cbuf)
				      (message "Picked color %s for buffer %s"
					       color (buffer-name cbuf)))))))

(list-colors-display nil nil
		     (let ((cbuf (current-buffer)))
		       `(lambda (color)
			  (when (buffer-live-p ,cbuf)
			    (message "Picked color %s for buffer %s"
				     color (buffer-name ,cbuf))))))

(list-colors-display nil nil
		     (let ((cbuf (current-buffer)))
		       #'(lambda (color)
			  (when (buffer-live-p cbuf)
			    (message "Picked color %s for buffer %s"
				     color (buffer-name cbuf))))))


Feel free to drop the 2nd cond clause if backward-compatibility is not
deemed too important here.

[0001-Fix-list-colors-print-handling-of-callback-arg.patch (text/x-patch, inline)]

From 3c68014cded797add3daa4030917ab9de299f57d Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda <at> gmail.com>
Date: Tue, 12 Jan 2021 20:41:49 -0300
Subject: [PATCH] Fix list-colors-print handling of callback arg

* lisp/facemenu.el (list-colors-print): Keeping
backward-compatibility, don't fail when passed a closure object as
CALLBACK.  (Bug#45831)
---
 lisp/facemenu.el | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/lisp/facemenu.el b/lisp/facemenu.el
index 2609397b0d..dc5f8f46ab 100644
--- a/lisp/facemenu.el
+++ b/lisp/facemenu.el
@@ -606,9 +606,14 @@ list-colors-display
 
 (defun list-colors-print (list &optional callback)
   (let ((callback-fn
-	 (if callback
-	     `(lambda (button)
-		(funcall ,callback (button-get button 'color-name))))))
+         ;; Expect CALLBACK to be a function, but allow it to be a form that
+         ;; evaluates to a function, for backward-compatibility.  (Bug#45831)
+         (cond ((functionp callback)
+                (lambda (button)
+                  (funcall callback (button-get button 'color-name))))
+               (callback
+                `(lambda (button)
+                  (funcall ,callback (button-get button 'color-name)))))))
     (dolist (color list)
       (if (consp color)
 	  (if (cdr color)
-- 
2.29.2

Message #13 received at 45831 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Mauro Aranda <maurooaranda <at> gmail.com>
Cc: 45831 <at> debbugs.gnu.org
Subject: Re: bug#45831: 28.0.50; list-colors-display callback arg needs to
 evaluate to a function?
Date: Fri, 15 Jan 2021 22:04:39 +0000

Mauro Aranda <maurooaranda <at> gmail.com> writes:

>  (defun list-colors-print (list &optional callback)
>    (let ((callback-fn
> -	 (if callback
> -	     `(lambda (button)
> -		(funcall ,callback (button-get button 'color-name))))))
> +         ;; Expect CALLBACK to be a function, but allow it to be a form that
> +         ;; evaluates to a function, for backward-compatibility.  (Bug#45831)
> +         (cond ((functionp callback)
> +                (lambda (button)
> +                  (funcall callback (button-get button 'color-name))))
> +               (callback
> +                `(lambda (button)
> +                  (funcall ,callback (button-get button 'color-name)))))))

Why not a single evaluated closure, e.g. like the following?

  (let ((callback-fn
         (when callback
           ;; Expect CALLBACK to be a function, but allow it to be a form that
           ;; evaluates to a function, for backward-compatibility (bug#45831).
           (or (functionp callback)
               (setq callback (eval callback lexical-binding)))
           (lambda (button)
             (funcall callback (button-get button 'color-name))))))
    ...)

Thanks,

-- 
Basil

Message #16 received at 45831 <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: 45831 <at> debbugs.gnu.org
Subject: Re: bug#45831: 28.0.50; list-colors-display callback arg needs to
 evaluate to a function?
Date: Sat, 16 Jan 2021 08:42:19 -0300

"Basil L. Contovounesios" <contovob <at> tcd.ie> writes:

> Mauro Aranda <maurooaranda <at> gmail.com> writes:
>
>>  (defun list-colors-print (list &optional callback)
>>    (let ((callback-fn
>> -	 (if callback
>> -	     `(lambda (button)
>> -		(funcall ,callback (button-get button 'color-name))))))
>> +         ;; Expect CALLBACK to be a function, but allow it to be a form that
>> +         ;; evaluates to a function, for backward-compatibility.  (Bug#45831)
>> +         (cond ((functionp callback)
>> +                (lambda (button)
>> +                  (funcall callback (button-get button 'color-name))))
>> +               (callback
>> +                `(lambda (button)
>> +                  (funcall ,callback (button-get button 'color-name)))))))
>
> Why not a single evaluated closure, e.g. like the following?
>
>   (let ((callback-fn
>          (when callback
>            ;; Expect CALLBACK to be a function, but allow it to be a form that
>            ;; evaluates to a function, for backward-compatibility (bug#45831).
>            (or (functionp callback)
>                (setq callback (eval callback lexical-binding)))
>            (lambda (button)
>              (funcall callback (button-get button 'color-name))))))
>     ...)

Just because I didn't want to change the original code too much, in case
someone thought the backward-compatible change wasn't necessary.  But of
course, your sample code looks better.

> Thanks,

Thanks for taking a look.

Message #19 received at 45831 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Mauro Aranda <maurooaranda <at> gmail.com>
Cc: 45831 <at> debbugs.gnu.org
Subject: Re: bug#45831: 28.0.50; list-colors-display callback arg needs to
 evaluate to a function?
Date: Tue, 19 Jan 2021 07:42:18 +0100

Mauro Aranda <maurooaranda <at> gmail.com> writes:

> Backward-compatible change attached.  With this change, all of the
> following work:

Looks good to me; go ahead and push.

Basil's version was shorter, but wasn't as straightforward (and besides,
it had an `eval', which always makes me suspicious. :))

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #22 received at 45831 <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 45831 <at> debbugs.gnu.org
Subject: Re: bug#45831: 28.0.50; list-colors-display callback arg needs to
 evaluate to a function?
Date: Tue, 19 Jan 2021 09:28:15 -0300

tags 45831 fixed
close 45831 28.1
quit


Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Mauro Aranda <maurooaranda <at> gmail.com> writes:
>
>> Backward-compatible change attached.  With this change, all of the
>> following work:
>
> Looks good to me; go ahead and push.
>
> Basil's version was shorter, but wasn't as straightforward (and besides,
> it had an `eval', which always makes me suspicious. :))

Thanks; I've pushed my patch.  If someone wants to tweak it to make it
prettier, I don't object.

Message #33 received at 45831 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Mauro Aranda <maurooaranda <at> gmail.com>
Cc: 45831 <at> debbugs.gnu.org
Subject: Re: bug#45831: 28.0.50; list-colors-display callback arg needs to
 evaluate to a function?
Date: Tue, 25 May 2021 15:27:22 -0400

> It seems like this message and the other one you sent to this bug
> address didn't reach the bugtracker, perhaps because the bug is archived
> (I didn't have the time to read the docs about that).  Would you mind
> resending them, but unarchiving the bug first?

Surprisingly I didn't get any message back warning me about the bug
being archived, but indeed it seems it was archived.
I just unarchived it, but I don't think my messages were important
enough to deserve a resend.

>>> Feel free to drop the 2nd cond clause if backward-compatibility is not
>>> deemed too important here.
>> It seems it's only backward compatible with a calling convention that
>> was never documented, so it's probably not necessary, indeed.
> Either way, I do agree with you that it's probably not necessary, but I
> wanted the patch to be simple so the bug reported would get fixed
> quickly.  That part is done, so feel free to drop that sneaky backquote ;-).

Wise strategy,


        Stefan

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.