unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / Atom feed
* bug#45831: 28.0.50; list-colors-display callback arg needs to evaluate to a function?
@ 2021-01-12 23:27 Mauro Aranda
  2021-01-12 23:59 ` Mauro Aranda
  0 siblings, 1 reply; 6+ messages in thread
From: Mauro Aranda @ 2021-01-12 23:27 UTC (permalink / raw)
  To: 45831

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





^ permalink raw reply	[flat|nested] 6+ messages in thread

* bug#45831: 28.0.50; list-colors-display callback arg needs to evaluate to a function?
  2021-01-12 23:27 bug#45831: 28.0.50; list-colors-display callback arg needs to evaluate to a function? Mauro Aranda
@ 2021-01-12 23:59 ` Mauro Aranda
  2021-01-15 22:04   ` Basil L. Contovounesios
  2021-01-19  6:42   ` Lars Ingebrigtsen
  0 siblings, 2 replies; 6+ messages in thread
From: Mauro Aranda @ 2021-01-12 23:59 UTC (permalink / raw)
  To: 45831

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* bug#45831: 28.0.50; list-colors-display callback arg needs to evaluate to a function?
  2021-01-12 23:59 ` Mauro Aranda
@ 2021-01-15 22:04   ` Basil L. Contovounesios
  2021-01-16 11:42     ` Mauro Aranda
  2021-01-19  6:42   ` Lars Ingebrigtsen
  1 sibling, 1 reply; 6+ messages in thread
From: Basil L. Contovounesios @ 2021-01-15 22:04 UTC (permalink / raw)
  To: Mauro Aranda; +Cc: 45831

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





^ permalink raw reply	[flat|nested] 6+ messages in thread

* bug#45831: 28.0.50; list-colors-display callback arg needs to evaluate to a function?
  2021-01-15 22:04   ` Basil L. Contovounesios
@ 2021-01-16 11:42     ` Mauro Aranda
  0 siblings, 0 replies; 6+ messages in thread
From: Mauro Aranda @ 2021-01-16 11:42 UTC (permalink / raw)
  To: Basil L. Contovounesios; +Cc: 45831

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





^ permalink raw reply	[flat|nested] 6+ messages in thread

* bug#45831: 28.0.50; list-colors-display callback arg needs to evaluate to a function?
  2021-01-12 23:59 ` Mauro Aranda
  2021-01-15 22:04   ` Basil L. Contovounesios
@ 2021-01-19  6:42   ` Lars Ingebrigtsen
  2021-01-19 12:28     ` Mauro Aranda
  1 sibling, 1 reply; 6+ messages in thread
From: Lars Ingebrigtsen @ 2021-01-19  6:42 UTC (permalink / raw)
  To: Mauro Aranda; +Cc: 45831

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





^ permalink raw reply	[flat|nested] 6+ messages in thread

* bug#45831: 28.0.50; list-colors-display callback arg needs to evaluate to a function?
  2021-01-19  6:42   ` Lars Ingebrigtsen
@ 2021-01-19 12:28     ` Mauro Aranda
  0 siblings, 0 replies; 6+ messages in thread
From: Mauro Aranda @ 2021-01-19 12:28 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: 45831

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.





^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-01-19 12:28 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-12 23:27 bug#45831: 28.0.50; list-colors-display callback arg needs to evaluate to a function? Mauro Aranda
2021-01-12 23:59 ` Mauro Aranda
2021-01-15 22:04   ` Basil L. Contovounesios
2021-01-16 11:42     ` Mauro Aranda
2021-01-19  6:42   ` Lars Ingebrigtsen
2021-01-19 12:28     ` Mauro Aranda

unofficial mirror of bug-gnu-emacs@gnu.org 

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://yhetil.org/emacs-bugs/0 emacs-bugs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 emacs-bugs emacs-bugs/ https://yhetil.org/emacs-bugs \
		bug-gnu-emacs@gnu.org
	public-inbox-index emacs-bugs

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.yhetil.org/yhetil.emacs.bugs
	nntp://news.gmane.io/gmane.emacs.bugs


AGPL code for this site: git clone http://ou63pmih66umazou.onion/public-inbox.git