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

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

[Mauro Aranda]

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

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

[Mauro Aranda]

[-- Attachment #1: Type: text/plain, Size: 1081 bytes --]

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.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Patch --]
[-- Type: text/x-patch, Size: 1314 bytes --]

From 3c68014cded797add3daa4030917ab9de299f57d Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@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

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

[Basil L. Contovounesios]

Mauro Aranda <maurooaranda@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

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

[Mauro Aranda]

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

> Mauro Aranda <maurooaranda@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.

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

[Lars Ingebrigtsen]

Mauro Aranda <maurooaranda@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

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

[Mauro Aranda]

tags 45831 fixed
close 45831 28.1
quit


Lars Ingebrigtsen <larsi@gnus.org> writes:

> Mauro Aranda <maurooaranda@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.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 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