bug#16483: 24.3.50; `read-face-name' is a mess now (regression)

bug#16483: 24.3.50; `read-face-name' is a mess now (regression)

[Drew Adams]

The doc string is a mess now.  Quite unclear, and incorrect in more than
one way.  And so is the code a mess.

1. The doc string no longer says what the behavior is if DEFAULT is nil.
In Emacs 24.3, it said this for that case:

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

What happens now?  No information about that.  See below for my guess.
Please tell users of the function what its behavior is.

2. The doc string says that DEFAULT is returned if the user enters the
empty string.  That is clearly wrong, at least when DEFAULT is a list of
faces or face names.  DEFAULT is not returned in such cases.  The most
that can be said in general is that DEFAULT _determines_ what is
returned for empty input - not that DEFAULT _is_ what is returned.

3. A list of symbols is not a list of face _names_.  A face is not a
face name.  Especially for functions like this, the doc should clearly
distinguish faces (symbols) from face names (strings).  All the more
so, because what is allowed and required as input for this function has
changed multiple times across Emacs versions.

4. The doc string does not allow for DEFAULT to be a face name, but that
case is supported by the code.  (And if it were not supported then this
would be another incompatible change from previous Emacs versions.)  If
it should not be supported then raise an error in that case.  If it should
be supported then correct the doc string.

5. The code is wrong, at least in this regard: If DEFAULT is a list of
strings (face names), and if MULTIPLE is nil, both of which are OK per
the doc string, then you get this:

Debugger entered--Lisp error: (wrong-type-argument symbolp "font-lock-comme=
nt-face")
* symbol-name("font-lock-comment-face")
* (cond ((symbolp default) (symbol-name default)) (multiple (mapconcat (fun=
ction (lambda (f) (if (symbolp f) (symbol-name f) f))) default ", ")) (t (s=
ymbol-name (car default))))
* (setq default (cond ((symbolp default) (symbol-name default)) (multiple (=
mapconcat (function (lambda (f) (if (symbolp f) (symbol-name f) f))) defaul=
t ", ")) (t (symbol-name (car default)))))
* (if (and default (not (stringp default))) (setq default (cond ((symbolp d=
efault) (symbol-name default)) (multiple (mapconcat (function (lambda (f) (=
if ... ... f))) default ", ")) (t (symbol-name (car default))))))
...
* read-face-name("face: " ("font-lock-comment-face" "highlight") nil)

6. If raising an error for #5 is the correct behavior and the doc is wrong
about allowing DEFAULT to be a list of face names, then here is a proposed
doc correction, assuming I understand the behavior right (which is not sure):

 If non-nil, DEFAULT should be a face (a symbol), a face name (a
 string) or a list of faces (symbols).

 DEFAULT determines what is returned if the user just hits `RET' (empty
 input), as follows:

  If DEFAULT is nil then return nil.
  If DEFAULT is a single face, then return its name.
  If DEFAULT is a list of faces, then:

    If MULTIPLE is nil, return the name of the first face in the list.
    If MULTIPLE is non-nil, return DEFAULT.

7. The doc string should not mention `completing-read-multiple' or the
separator regexp.  Anyway, the code uses the value of `crm-separator';
it does not use the literal regexp mentioned in the doc.  The doc should
just say something like this (to be appended to that suggested in #6):

 If MULTIPLE is non-nil, read multiple face names and return them as a
 list.  If MULTIPLE is nil, read and return a single face name.

8. What happened to the useful defaulting of previous Emacs versions?
Yes, I know why you made the change, but now any existing code that uses
`read-face-name' is broken if it depends on `r-f-n' to provide such
defaulting.  Too bad.

Please consider: There is more Elisp code in the world than just what is
distributed by Emacs Dev.  `read-face-name' has been and continues to be
a poster child of how not to evolve code.  It has morphed in incompatible
ways from version to version.  The right way to do what you wanted to do
for Emacs 24.4 would have been to go ahead and define `face-at-point',
but to *use* it in `read-face-name', so that that function continues to
provide the expected defaulting when DEFAULT is nil:

  (unless default (setq default  (face-at-point)))

I somehow doubt that that part of the regression will be fixed, but
perhaps some of the other points above have a chance of being addressed.

In GNU Emacs 24.3.50.1 (i686-pc-mingw32)
 of 2014-01-07 on ODIEONE
Bzr revision: 115916 bzg@gnu.org-20140107233629-du2solx6tmxnx0np
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --prefix=3D/c/Devel/emacs/binary --enable-checking=3Dyes,glyphs
 'CFLAGS=3D-O0 -g3' LDFLAGS=3D-Lc:/Devel/emacs/lib
 CPPFLAGS=3D-Ic:/Devel/emacs/include'

bug#16483: 24.3.50; `read-face-name' is a mess now (regression)

[Lars Ingebrigtsen]

Drew Adams <drew.adams@oracle.com> writes:

> The doc string is a mess now.  Quite unclear, and incorrect in more than
> one way.  And so is the code a mess.
>
> 1. The doc string no longer says what the behavior is if DEFAULT is nil.
> In Emacs 24.3, it said this for that case:
>
>  If DEFAULT is nil, the list of default face names is taken from
>  the `read-face-name' property of the text at point, or, if that
>  is nil, from the `face' property of the text at point.
>
> What happens now?  No information about that.  See below for my guess.
> Please tell users of the function what its behavior is.

As far as I can see, if DEFAULT is nil, there is no list of default face
names?

> 2. The doc string says that DEFAULT is returned if the user enters the
> empty string.  That is clearly wrong, at least when DEFAULT is a list of
> faces or face names.  DEFAULT is not returned in such cases.  The most
> that can be said in general is that DEFAULT _determines_ what is
> returned for empty input - not that DEFAULT _is_ what is returned.

I've now made the doc string more correct, if more difficult to parse.

> 5. The code is wrong, at least in this regard: If DEFAULT is a list of
> strings (face names), and if MULTIPLE is nil, both of which are OK per
> the doc string, then you get this:
>
> Debugger entered--Lisp error: (wrong-type-argument symbolp "font-lock-comme=
> nt-face")
> * symbol-name("font-lock-comment-face")

I've fixed that now.

> 8. What happened to the useful defaulting of previous Emacs versions?
> Yes, I know why you made the change, but now any existing code that uses
> `read-face-name' is broken if it depends on `r-f-n' to provide such
> defaulting.  Too bad.
>
> Please consider: There is more Elisp code in the world than just what is
> distributed by Emacs Dev.  `read-face-name' has been and continues to be
> a poster child of how not to evolve code.  It has morphed in incompatible
> ways from version to version.  The right way to do what you wanted to do
> for Emacs 24.4 would have been to go ahead and define `face-at-point',
> but to *use* it in `read-face-name', so that that function continues to
> provide the expected defaulting when DEFAULT is nil:
>
>   (unless default (setq default  (face-at-point)))
>
> I somehow doubt that that part of the regression will be fixed, but
> perhaps some of the other points above have a chance of being addressed.

Yes, that's outside the scope of read-face-name, I think.

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

bug#16483: 24.3.50; `read-face-name' is a mess now (regression)

[Drew Adams]

> > 1. The doc string no longer says what the behavior is if DEFAULT is nil.
> > In Emacs 24.3, it said this for that case:
> >
> >  If DEFAULT is nil, the list of default face names is taken from
> >  the `read-face-name' property of the text at point, or, if that
> >  is nil, from the `face' property of the text at point.
> >
> > What happens now?  No information about that.  See below for my guess.
> > Please tell users of the function what its behavior is.
> 
> As far as I can see, if DEFAULT is nil, there is no list of default face
> names?

Yes, and so no default value is put in the prompt.

Note: This handling of the prompt and the default value(s) for
this function have a long and torturous history.  This stuff
has been changed a zillion times, with each time being something
arcane and incomprehensible for users (IMHO).

It is simpler (better) now.  This item (#1) can be closed: if
DEFAULT is nil now, the only effect it has is on the return
value (`nil'). 

> > 2. The doc string says that DEFAULT is returned if the user enters the
> > empty string.  That is clearly wrong, at least when DEFAULT is a list of
> > faces or face names.  DEFAULT is not returned in such cases.  The most
> > that can be said in general is that DEFAULT _determines_ what is
> > returned for empty input - not that DEFAULT _is_ what is returned.
> 
> I've now made the doc string more correct, if more difficult to parse.

I trust it must be better.  It can always be improved more
in the future if not completely satisfactory.  Thx.

> > 8. What happened to the useful defaulting of previous Emacs versions?
> > Yes, I know why you made the change, but now any existing code that uses
> > `read-face-name' is broken if it depends on `r-f-n' to provide such
> > defaulting.  Too bad.
> >
> > Please consider: There is more Elisp code in the world than just what is
> > distributed by Emacs Dev.  `read-face-name' has been and continues to be
> > a poster child of how not to evolve code.  It has morphed in incompatible
> > ways from version to version.  The right way to do what you wanted to do
> > for Emacs 24.4 would have been to go ahead and define `face-at-point',
> > but to *use* it in `read-face-name', so that that function continues to
> > provide the expected defaulting when DEFAULT is nil:
> >
> >   (unless default (setq default  (face-at-point)))
> >
> > I somehow doubt that that part of the regression will be fixed, but
> > perhaps some of the other points above have a chance of being addressed.
> 
> Yes, that's outside the scope of read-face-name, I think.