default for read-face-name

default for read-face-name

[Drew Adams]

`read-face-name' accepts an optional arg STRING-DESCRIBING-DEFAULT, which is
used only if (ahem) no real defaulting can be done (!) - a catch-all description
such as "all faces" is passed and added to the prompt.

Normally, however, default faces are picked up, from all of these sources (and
then STRING-DESCRIBING-DEFAULT is ignored):

1. face symbols named at point (using thing-at-point)
2. a `read-face-name' text property on the char at point
3. a `face' text property on the char at point

I think we should also let callers specify a default face to use: Change
STRING-DESCRIBING-DEFAULT to DEFAULT, and give it precedence over the other ways
of defaulting. If the calling code knows the best default, let it easily DTRT.

We could add DEFAULT as an additional arg, but the current
STRING-DESCRIBING-DEFAULT is pretty useless anyway, as the doc string suggests:
"you can omit it".  So I'd suggest just replacing it with a new DEFAULT arg.

Reason for a DEFAULT arg: Sometimes a significant default face is known to the
calling code, and it should be possible to specify it (pass it to the user).

For example, if a face is saved somewhere as the current face to use for
something, and the code calls `read-face-name' to let you enter a different face
to override that, it can be helpful to provide the old face as the default, in
order to: (a) show you what it is and (b) let you edit it to a similar face name
(or confirm its use by hitting RET).

The current design pretty much assumes that such a "current" face would in fact
be used already at point, so the `face' property would pick it up.  But that is
not always the case - it is a bad assumption.

And it seems silly to have to temporarily add a text property (`read-face-name')
at point, just to be able to provide a proper default.  That kind of thing can
make sense for a static display such as `list-faces-display' but it is silly as
a general way to pass a default value to the function.

In sum, a read function deserves a default, and it is reasonable to be able to
provide the default as an arg.

The code change needed would be straightforward.  And there seem to be only a
couple of places in the existing source files where STRING-DESCRIBING-DEFAULT is
used.  My guess is that the change I'm suggesting would not have a negative
impact.

WDOT?

Re: default for read-face-name

[Juri Linkov]

> WDOT?

Please see how STRING-DESCRIBING-DEFAULT is used in `customize-face'
where "all faces" means that nil returned from `read-face-name'
forces it to use all faces.

We could keep this functionality and also allow specifying the default
face like you proposed.  It is possible to distinguish two cases
by checking if STRING-DESCRIBING-DEFAULT is a string (to be used in prompt)
or a face (for the default value).

-- 
Juri Linkov
http://www.jurta.org/emacs/

RE: default for read-face-name

[Drew Adams]

> Please see how STRING-DESCRIBING-DEFAULT is used in `customize-face'
> where "all faces" means that nil returned from `read-face-name'
> forces it to use all faces.

I don't see how that is a problem.  Is such an implementation required?  Can't
`customize-face' easily be made to use all faces without such a hack?

> We could keep this functionality and also allow specifying the default
> face like you proposed.  It is possible to distinguish two cases
> by checking if STRING-DESCRIBING-DEFAULT is a string (to be 
> used in prompt) or a face (for the default value).

I would prefer to have two separate parameters, if you insist on retaining
STRING-DESCRIBING-DEFAULT as it is (for `customize-face').  In that case, please
just add DEFAULT as a normal default value: a face name.

I disagree with having a default value for completion be other than a string.
In the case of `read-face-name', the DEFAULT arg should be a face name.
Completion accepts a string (strings) as the default (defaults) and returns a
string.  Let's stick to that approach.

You might argue in favor of allowing a face so as to allow a face spec - or even
a list of faces (symbols or specs) that are to be combined.  I disagree with
such complexity here.  I would prefer a simple DEFAULT: a face name (string).

Re: default for read-face-name

[Juri Linkov]

> I don't see how that is a problem.  Is such an implementation required?  Can't
> `customize-face' easily be made to use all faces without such a hack?

This is easy to do using the garbage-in/garbage-out principle.
When code will call `read-face-name' with the DEFAULT arg "all faces"
and the user types RET, it will return "all faces" as is.  Then
`customize-face' will compare the returned value with the string
"all faces".

> I would prefer to have two separate parameters, if you insist on
> retaining STRING-DESCRIBING-DEFAULT as it is (for `customize-face').
> In that case, please just add DEFAULT as a normal default value:
> a face name.

This patch changes STRING-DESCRIBING-DEFAULT to a normal default value:

=== modified file 'lisp/faces.el'
--- lisp/faces.el	2010-03-24 00:17:31 +0000
+++ lisp/faces.el	2010-06-25 19:51:14 +0000
@@ -915,13 +915,13 @@ (defun invert-face (face &optional frame
 ;;; Interactively modifying faces.
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
-(defun read-face-name (prompt &optional string-describing-default multiple)
+(defun read-face-name (prompt &optional default multiple)
   "Read a face, defaulting to the face or faces on the char after point.
 If it has the property `read-face-name', that overrides the `face' property.
 PROMPT should be a string that describes what the caller will do with the face;
 it should not end in a space.
-STRING-DESCRIBING-DEFAULT should describe what default the caller will use if
-the user just types RET; you can omit it.
+The optional argument DEFAULT provides the value to display in the
+minibuffer prompt that is returned if the user just types RET.
 If MULTIPLE is non-nil, return a list of faces (possibly only one).
 Otherwise, return a single face."
   (let ((faceprop (or (get-char-property (point) 'read-face-name)
@@ -960,10 +960,10 @@ (defun read-face-name (prompt &optional 
     (let* ((input
 	    ;; Read the input.
 	    (completing-read-multiple
-	     (if (or faces string-describing-default)
+	     (if (or faces default)
 		 (format "%s (default %s): " prompt
 			 (if faces (mapconcat 'symbol-name faces ",")
-			   string-describing-default))
+			   default))
 	       (format "%s: " prompt))
 	     (completion-table-in-turn nonaliasfaces aliasfaces)
 	     nil t nil 'face-name-history
@@ -971,7 +971,7 @@ (defun read-face-name (prompt &optional 
 	   ;; Canonicalize the output.
 	   (output
 	    (cond ((or (equal input "") (equal input '("")))
-		   faces)
+		   (or faces default))
 		  ((stringp input)
 		   (mapcar 'intern (split-string input ", *" t)))
 		  ((listp input)

=== modified file 'lisp/cus-edit.el'
--- lisp/cus-edit.el	2010-04-20 18:52:07 +0000
+++ lisp/cus-edit.el	2010-06-25 19:51:40 +0000
@@ -1289,7 +1289,7 @@ (defun customize-face (&optional face)
 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)))
-  (if (member face '(nil ""))
+  (if (member face '(nil "" "all faces"))
       (setq face (face-list)))
   (if (and (listp face) (null (cdr face)))
       (setq face (car face)))

-- 
Juri Linkov
http://www.jurta.org/emacs/

RE: default for read-face-name

[Drew Adams]

> This patch changes STRING-DESCRIBING-DEFAULT to a normal 
> default value:

Thank you.  I didn't test it, but I trust it DTRT.

Re: default for read-face-name

[Juri Linkov]

>> This patch changes STRING-DESCRIBING-DEFAULT to a normal
>> default value:
>
> Thank you.  I didn't test it, but I trust it DTRT.

I found another place that uses `read-face-name':
`describe-face' calls

  (read-face-name "Describe face" "= `default' face" t)

so later it should compare `face' with `"= `default' face"' like

    (unless (and face (not (equal face "= `default' face")))
      (setq face 'default))

If this is ugly, we could call `read-face-name' with the real face name
(i.e. `default'), not with a string describing default:

  (read-face-name "Describe face" 'default t)

But in this case, the prompt is less descriptive:

  Describe face (default default):

Compare it with the current prompt:

  Describe face (default = `default' face):

-- 
Juri Linkov
http://www.jurta.org/emacs/

RE: default for read-face-name

[Drew Adams]

> I found another place that uses `read-face-name':
> `describe-face' calls
>   (read-face-name "Describe face" "= `default' face" t)
> so later it should compare `face' with `"= `default' face"' like
>     (unless (and face (not (equal face "= `default' face")))
>       (setq face 'default))
> 
> If this is ugly, we could call `read-face-name' with the real 
> face name (i.e. `default'), not with a string describing default:
>   (read-face-name "Describe face" 'default t)
> But in this case, the prompt is less descriptive:
>   Describe face (default default):
> Compare it with the current prompt:
>   Describe face (default = `default' face):

Either is OK by me.  Whether the code is a bit ugly is less important here than
serving the user.  (That ugliness does not represent fragility or imply a
maintenance headache.)

FWIW, anytime the default value is put into the prompt it should probably be
quoted: `...'.  At the very least, that increases clarity in cases like this and
when the value is a string with spaces (or a trailing colon or...).

In this case that would give "Describe face (default `default'):" which seems
clear in the context of choosing a face name.  That fix is probably a more
general than just for `read-face-name', however.

Re: default for read-face-name

[Juri Linkov]

> FWIW, anytime the default value is put into the prompt it should probably be
> quoted: `...'.  At the very least, that increases clarity in cases like this and
> when the value is a string with spaces (or a trailing colon or...).
>
> In this case that would give "Describe face (default `default'):" which seems
> clear in the context of choosing a face name.

I agree that the default value quoted in the prompt is better.
So with the patch below we get:

  Describe face (default `default'):

  Customize face (default `all faces'):

=== modified file 'lisp/faces.el'
--- lisp/faces.el	2010-03-24 00:17:31 +0000
+++ lisp/faces.el	2010-06-30 08:07:21 +0000
@@ -915,13 +915,14 @@ (defun invert-face (face &optional frame
 ;;; Interactively modifying faces.
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
-(defun read-face-name (prompt &optional string-describing-default multiple)
+(defun read-face-name (prompt &optional default multiple)
   "Read a face, defaulting to the face or faces on the char after point.
 If it has the property `read-face-name', that overrides the `face' property.
 PROMPT should be a string that describes what the caller will do with the face;
 it should not end in a space.
-STRING-DESCRIBING-DEFAULT should describe what default the caller will use if
-the user just types RET; you can omit it.
+The optional argument DEFAULT provides the value to display in the
+minibuffer prompt that is returned if the user just types RET
+unless DEFAULT is a string (in which case nil is returned).
 If MULTIPLE is non-nil, return a list of faces (possibly only one).
 Otherwise, return a single face."
   (let ((faceprop (or (get-char-property (point) 'read-face-name)
@@ -960,10 +961,10 @@ (defun read-face-name (prompt &optional 
     (let* ((input
 	    ;; Read the input.
 	    (completing-read-multiple
-	     (if (or faces string-describing-default)
-		 (format "%s (default %s): " prompt
+	     (if (or faces default)
+		 (format "%s (default `%s'): " prompt
 			 (if faces (mapconcat 'symbol-name faces ",")
-			   string-describing-default))
+			   default))
 	       (format "%s: " prompt))
 	     (completion-table-in-turn nonaliasfaces aliasfaces)
 	     nil t nil 'face-name-history
@@ -971,7 +972,7 @@ (defun read-face-name (prompt &optional 
 	   ;; Canonicalize the output.
 	   (output
 	    (cond ((or (equal input "") (equal input '("")))
-		   faces)
+		   (or faces (unless (stringp default) default)))
 		  ((stringp input)
 		   (mapcar 'intern (split-string input ", *" t)))
 		  ((listp input)
@@ -1334,7 +1335,7 @@ (defun describe-face (face &optional fra
 If the optional argument FRAME is given, report on face FACE in that frame.
 If FRAME is t, report on the defaults for face FACE (for new frames).
 If FRAME is omitted or nil, use the selected frame."
-  (interactive (list (read-face-name "Describe face" "= `default' face" t)))
+  (interactive (list (read-face-name "Describe face" 'default t)))
   (let* ((attrs '((:family . "Family")
 		  (:foundry . "Foundry")
 		  (:width . "Width")

-- 
Juri Linkov
http://www.jurta.org/emacs/