Recent read-face patch breaks `M-x customize-face'

Recent read-face patch breaks `M-x customize-face'

[Giorgos Keramidas]

Hi all,

I noticed that `M-x customize-face' triggers an error in a recent build
of Emacs trunk, and managed to track this backtrace:

Debugger entered--Lisp error: (wrong-type-argument sequencep 97)
  mapconcat((lambda (f) (if (symbolp f) (symbol-name f) f)) "all faces" ", ")
  (setq default (mapconcat (function (lambda (f) (if (symbolp f) (symbol-name f) f))) default ", "))
  (if default (setq default (mapconcat (function (lambda (f) (if (symbolp f) (symbol-name f) f))) defau$
  read-face-name("Customize face" "all faces" t)
  (list (read-face-name "Customize face" "all faces" t))
  call-interactively(customize-face record nil)
  command-execute(customize-face record)
  execute-extended-command(nil "customize-face")
  call-interactively(execute-extended-command nil nil)
  command-execute(execute-extended-command)

To the changes of this commit:

commit 605625d262e5e4bf996691027c755c65061492b1
Author: Roland Winkler <winkler@gnu.org>
Date:   Wed Apr 3 21:12:25 2013 -0500

    lisp/faces.el (read-face-name): Behave as promised by the docstring.

| diff --git a/lisp/ChangeLog b/lisp/ChangeLog
| index 2b86faf..7ee201a 100644
| --- a/lisp/ChangeLog
| +++ b/lisp/ChangeLog
| @@ -1,3 +1,9 @@
| +2013-04-04  Roland Winkler  <winkler@gnu.org>
| +
| +       * faces.el (read-face-name): Behave as promised by the docstring.
| +       Assume that arg default is a list of faces.
| +       (describe-face): Call read-face-name with list of default faces.
| +
|  2013-04-04  Thierry Volpiatto  <thierry.volpiatto@gmail.com>
|
|         * bookmark.el: Fix deletion of bookmarks (bug#13972).

Does customize-face depend on some bogus assumption that this commit
has now rendered broken??

Re: Recent read-face patch breaks `M-x customize-face'

[Giorgos Keramidas]

On 2013-04-05 19:50, Giorgos Keramidas <gkeramidas@gmail.com> wrote:
> commit 605625d262e5e4bf996691027c755c65061492b1
> Author: Roland Winkler <winkler@gnu.org>
> Date:   Wed Apr 3 21:12:25 2013 -0500
>
>     lisp/faces.el (read-face-name): Behave as promised by the docstring.
>
> | diff --git a/lisp/ChangeLog b/lisp/ChangeLog
> | index 2b86faf..7ee201a 100644
> | --- a/lisp/ChangeLog
> | +++ b/lisp/ChangeLog
> | @@ -1,3 +1,9 @@
> | +2013-04-04  Roland Winkler  <winkler@gnu.org>
> | +
> | +       * faces.el (read-face-name): Behave as promised by the docstring.
> | +       Assume that arg default is a list of faces.
> | +       (describe-face): Call read-face-name with list of default faces.
> | +
> |  2013-04-04  Thierry Volpiatto  <thierry.volpiatto@gmail.com>
> |
> |         * bookmark.el: Fix deletion of bookmarks (bug#13972).
>
> Does customize-face depend on some bogus assumption that this commit
> has now rendered broken??

Verified.  Simply reverting lisp/faces.el to its state before this
patch, unbreaks `M-x customize-face'.

Re: Recent read-face patch breaks `M-x customize-face'

[Roland Winkler]

On Fri Apr 5 2013 Giorgos Keramidas wrote:
> Does customize-face depend on some bogus assumption that this
> commit has now rendered broken??

The patch of read-face-name made this function behave as promised by
its docstring.  This says that the arg DEFAULT should be a list of
faces.  (Previously, quite to the contrary, the actual code of
read-face-name assumed that DEFAULT should be a single string.)

When I installed the patch, I somehow overlooked that customize-face
calls read-face-name with DEFAULT having the value "all faces".
This is certainly not a very meaningful value to begin with.
So I guess the best fix is to replace "all faces" by nil.

Or am I once again overlooking something?

Roland

Re: Recent read-face patch breaks `M-x customize-face'

[Roland Winkler]

On Fri Apr 5 2013 Roland Winkler wrote:
> So I guess the best fix is to replace "all faces" by nil.

...both for customize-face and customize-face-other-window.

Re: Recent read-face patch breaks `M-x customize-face'

[Giorgos Keramidas]

On 2013-04-05 14:00, Roland Winkler <winkler@gnu.org> wrote:
> On Fri Apr 5 2013 Roland Winkler wrote:
> > So I guess the best fix is to replace "all faces" by nil.
>
> ...both for customize-face and customize-face-other-window.

Correct.  This fixes `M-x customize-face' in trunk.  Thanks!

===========================================================================
diff --git a/lisp/cus-edit.el b/lisp/cus-edit.el
index d19e2de..b82c67b 100644
--- a/lisp/cus-edit.el
+++ b/lisp/cus-edit.el
@@ -1319,7 +1319,7 @@ If OTHER-WINDOW is non-nil, display in another window.
 
 Interactively, when point is on text which has a face specified,
 suggest to customize that face, if it's customizable."
-  (interactive (list (read-face-name "Customize face" "all faces" t)))
+  (interactive (list (read-face-name "Customize face" nil t)))
   (if (member face '(nil ""))
       (setq face (face-list)))
   (if (and (listp face) (null (cdr face)))
@@ -1350,7 +1350,7 @@ If FACE is actually a face-alias, customize the face it is aliased to.
 
 Interactively, when point is on text which has a face specified,
 suggest to customize that face, if it's customizable."
-  (interactive (list (read-face-name "Customize face" "all faces" t)))
+  (interactive (list (read-face-name "Customize face" nil t)))
   (customize-face face t))
 
 (defalias 'customize-customized 'customize-unsaved)
===========================================================================

Re: Recent read-face patch breaks `M-x customize-face'

[Stefan Monnier]

> When I installed the patch, I somehow overlooked that customize-face
> calls read-face-name with DEFAULT having the value "all faces".
> This is certainly not a very meaningful value to begin with.
> So I guess the best fix is to replace "all faces" by nil.

It would be safer to simply turn any non-list value of `default' into
a one-element list.


        Stefan

Re: Recent read-face patch breaks `M-x customize-face'

[Roland Winkler]

On Fri Apr 5 2013 Stefan Monnier wrote:
> It would be safer to simply turn any non-list value of `default' into
> a one-element list.

Makes sense.  Yet the problem here is that customize-face calls
read-face-name with DEFAULT having the bogus value "all faces".
So customize-face should still be fixed.

Re: Recent read-face patch breaks `M-x customize-face'

[Stefan Monnier]

>> It would be safer to simply turn any non-list value of `default' into
>> a one-element list.
> Makes sense.  Yet the problem here is that customize-face calls
> read-face-name with DEFAULT having the bogus value "all faces".
> So customize-face should still be fixed.

Just for the record: the reason for this "all faces" argument is so that
M-x customize-face prompts with "Customize face (default `all faces'):"


        Stefan

Re: Recent read-face patch breaks `M-x customize-face'

[Γιώργος Κεραμίδας]

On Sat, 06 Apr 2013 10:02:52 -0400, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>>> It would be safer to simply turn any non-list value of `default' into
>>> a one-element list.
>> Makes sense.  Yet the problem here is that customize-face calls
>> read-face-name with DEFAULT having the bogus value "all faces".
>> So customize-face should still be fixed.
>
> Just for the record: the reason for this "all faces" argument is so that
> M-x customize-face prompts with "Customize face (default `all faces'):"

Ah, that's where that message came from!  I see now that no message
about (default `xxx') is displayed at the M-x prompt.  Maybe we have to
change read-face-name a bit.

Right now the doc string of read-face-name says things in two places
that when combined together seem to imply that what customize-face
expects is ok:

    (read-face-name PROMPT &optional DEFAULT MULTIPLE)

    Read one or more face names, defaulting to the face(s) at point.
    PROMPT should be a prompt string; it should not end in a space or
    a colon.

  : The optional argument DEFAULT specifies the default face name(s)
  : to return if the user just types RET.  If its value is non-nil,
  : it should be a list of face names (symbols or strings); in that case,
  : the default return value is the `car' of DEFAULT (if the argument
  : MULTIPLE is non-nil), or DEFAULT (if MULTIPLE is nil).  See below
  : for the meaning of MULTIPLE.

    If DEFAULT is nil, the list of default face names is taken from
    the symbol at point and the `read-face-name' property of the text at point,
    or, if that is nil, from the `face' property of the text at point.

    This function uses `completing-read-multiple' with "[ \t]*,[ \t]*"
    as the separator regexp.  Thus, the user may enter multiple face
  : names, separated by commas.  The optional argument MULTIPLE
  : specifies the form of the return value.  If MULTIPLE is non-nil,
  : return a list of face names; if the user entered just one face
  : name, the return value would be a list of one face name.
  : Otherwise, return a single face name; if the user entered more
  : than one face name, return only the first one.

If we read together the two marked parts they seem to translate to what
customize-face expects, that when DEFAULT is nil and, at the same time,
MULTIPLE is non-nil, a list of all faces is returned.

What `read-face-name' does when DEFAULT is non-nil makes, of course,
sense: display the current value of DEFAULT in the prompt.  But maybe we
should change it to also work correctly if MULTIPLE is non-nil and
DEFAULT is not the name of a symbol.

What was failing was this part of `read-face-name':

  : ;; If we only want one, and the default is more than one,
  : ;; discard the unwanted ones now.
  : (if (and default (not multiple))
  :     (setq default (list (car default))))
  :
  : (if default
  :     (setq default (mapconcat (lambda (f)
  :                                (if (symbolp f) (symbol-name f) f))
  :                              default ", ")))

Maybe we just need to account for the case of:

    (if (and multiple default (not (symbolp default)) (not (consp default)))
      just-use-default-value-as-prompt)

Because what happened here was that `default' was passed as "all faces",
and mapconcat happily tried to filter the string through the lambda
function, resulting in a list of character values, resulting in a call
like this:

  (mapconcat (lambda (x) x)
    (list 97 108 108 32 102 97 99 101 115)
    ", ")

Which of course throws a traceback, when mapconcat tries to look at the
character value `97' as a sequence to concatenate it with the rest.

Re: Recent read-face patch breaks `M-x customize-face'

[Roland Winkler]

On Sat Apr 6 2013 Stefan Monnier wrote:
> Just for the record: the reason for this "all faces" argument is so that
> M-x customize-face prompts with "Customize face (default `all faces'):"

...This is the previous behavior that any string value of DEFAULT
(unless it gets magically overridden by a "face at point") gets
displayed as default in the prompt. But if this value of DEFAULT is
not a face, it is ignored in the end, that is, read-face-name
returns nil. Instead of "all faces", one could use also "Hello
World" to get the same final result.

This behavior of read-face-name appears a bit of a kludge to me,
though I understand the rationale for this in the context of
customize-face. But at least such behavior needs to be documented in
the doc string of read-face-name.

I'd prefer if customize-face called read-face-name with DEFAULT
being nil and a prompt that said something like

  "Customize face (use `' for `all faces')"

But this doesn't work with the current code of read-face-name that
may replace a DEFAULT value of nil by a "face at point".
Removing this magic from read-face-name would require in turn that
commands such as make-face-bold call face-at-point for a reasonable
value of DEFAULT. In my humble opinion, this appears to be the
cleanest solution.

Roland

Re: Recent read-face patch breaks `M-x customize-face'

[Stefan Monnier]

> This behavior of read-face-name appears a bit of a kludge to me,

Agreed.

> Removing this magic from read-face-name would require in turn that
> commands such as make-face-bold call face-at-point for a reasonable
> value of DEFAULT. In my humble opinion, this appears to be the
> cleanest solution.

I think the for sake of backward compatibility, it is best to just
preserve the kludge.


        Stefan

Re: Recent read-face patch breaks `M-x customize-face'

[Roland Winkler]

On Sat Apr 6 2013 Stefan Monnier wrote:
> > Removing this magic from read-face-name would require in turn that
> > commands such as make-face-bold call face-at-point for a reasonable
> > value of DEFAULT. In my humble opinion, this appears to be the
> > cleanest solution.
> 
> I think the for sake of backward compatibility, it is best to just
> preserve the kludge.

The command customize-face is actually more subtle than I had
realized previously. It calls read-face-name with DEFAULT being
"all faces". But read-face-name used to ignore and override this
value when there was a "face at point". So the value of DEFAULT was
really   (or (face-at-point) "all faces").  From a user perspective,
this behavior is certainly quite reasonable. But for the old
implementation it was crucial that read-face-name would replace
"all faces" by (face-at-point).  This overriding of the value of
DEFAULT was rather weird behavior and I'd like to avoid this if
somehow possible.

Backward compatibilty should not break existing code. But under this
umbrella it must be possible to modify and improve the code.

So backward compatiblity requires that the arg DEFAULT of
read-face-name may be a list of faces (as the docstring says). But a
symbol (representing a single face) or a string (which may represent
a single face, a comma-separated list of faces, or some "bogus face"
such as "all faces") should be accepted, too. But as long as the prompt
of read-face-name tells the user the actual value of DEFAULT, this
will not break anybody's existing code. - Did I overlook something here?

Roland

Re: Recent read-face patch breaks `M-x customize-face'

[Roland Winkler]

On Sat Apr 6 2013 Stefan Monnier wrote:
> Just for the record: the reason for this "all faces" argument is so that
> M-x customize-face prompts with "Customize face (default `all faces'):"

This should now be fixed (revision 112273).

Roland