Fixes related to face underlining

* lisp/faces.el (face-underline-p): Doc fix.  Handle :underline being
things other than `t' (a string, a list).
(face-inverse-video-p): Doc fix.
(set-face-underline): Rename it back from set-face-underline-p.
Doc fix.  Allow interactive input of values other than t.
(read-face-attribute): Apply formatting to :underline,
since like :box and :stipple it can take list values.

* doc/lispref/display.texi (Face Attributes): Fix :underline COLOR description.
(Attribute Functions): Update for set-face-underline rename.
Tweak descriptions of face-underline-p, face-inverse-video-p.

* etc/NEWS: Related edit.

6 files changed, 61 insertions, 27 deletions

diff --git a/doc/lispref/ChangeLog b/doc/lispref/ChangeLog
index d05f48b9f2d..68d7bee2b64 100644
--- a/doc/lispref/ChangeLog
+++ b/doc/lispref/ChangeLog
@@ -1,6 +1,8 @@
 2012-11-15  Glenn Morris  <rgm@gnu.org>
 
         * display.texi (Face Attributes): Fix :underline COLOR description.
+        (Attribute Functions): Update for set-face-underline rename.
+        Tweak descriptions of face-underline-p, face-inverse-video-p.
 
 2012-11-14  Glenn Morris  <rgm@gnu.org>
 
diff --git a/doc/lispref/display.texi b/doc/lispref/display.texi
index 6c77a9937d7..9fedd162da6 100644
--- a/doc/lispref/display.texi
+++ b/doc/lispref/display.texi
@@ -2403,7 +2403,7 @@ This sets the @code{:slant} attribute of @var{face} to @var{normal} if
 @var{italic-p} is @code{nil}, and to @var{italic} otherwise.
 @end defun
 
-@defun set-face-underline-p face underline &optional frame
+@defun set-face-underline face underline &optional frame
 This sets the @code{:underline} attribute of @var{face} to
 @var{underline}.
 @end defun
@@ -2466,12 +2466,16 @@ attribute of @var{face} is @code{italic} or @code{oblique}, and
 @code{nil} otherwise.
 @end defun
 
+@c Note the weasel words.  A face that inherits from an underlined
+@c face but does not specify :underline will return nil.
 @defun face-underline-p face &optional frame
-This function returns the @code{:underline} attribute of face @var{face}.
+This function returns non-@code{nil} if face @var{face} specifies
+a non-@code{nil} @code{:underline} attribute.
 @end defun
 
 @defun face-inverse-video-p face &optional frame
-This function returns the @code{:inverse-video} attribute of face @var{face}.
+This function returns non-@code{nil} if face @var{face} specifies
+a non-@code{nil} @code{:inverse-video} attribute.
 @end defun
 
 @node Displaying Faces
diff --git a/etc/NEWS b/etc/NEWS
index 84bedfbc257..b69240e081a 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -948,8 +948,8 @@ takes precedence over most other maps for a short while (normally one key).
 +++
 ** New fringe bitmap `exclamation-mark'.
 
++++
 ** Face underlining can now use a wave.
-See the "Face Attributes" section of the Elisp manual.
 
 ** The following functions and variables are obsolete:
 ---
diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index 01ccb886434..e53b667b2b2 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,5 +1,13 @@
 2012-11-15  Glenn Morris  <rgm@gnu.org>
 
+        * faces.el (face-underline-p): Doc fix.  Handle :underline being
+        things other than `t' (a string, a list).
+        (face-inverse-video-p): Doc fix.
+        (set-face-underline): Rename it back from set-face-underline-p.
+        Doc fix.  Allow interactive input of values other than t.
+        (read-face-attribute): Apply formatting to :underline,
+        since like :box and :stipple it can take list values.
+
         * term.el (ansi-term): Don't let C-x escape-char binding
         clobber the more standard C-c binding.  (Bug#12842)
 
diff --git a/lisp/faces.el b/lisp/faces.el
index f5ef88d08b0..d07c4d6f5a5 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -487,16 +487,21 @@ with the `default' face (which is always completely specified)."
 (defalias 'face-background-pixmap 'face-stipple)
 
 
+;; FIXME all of these -p functions ignore inheritance (cf face-stipple).
+;; Ie, a face that inherits from an underlined face but does not
+;; specify :underline will return nil.
+;; So these functions don't actually tell you anything about how the
+;; face will _appear_.  So not very useful IMO.
 (defun face-underline-p (face &optional frame)
- "Return non-nil if FACE is underlined.
+ "Return non-nil if FACE specifies a non-nil underlining.
 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."
- (eq (face-attribute face :underline frame) t))
+ (not (memq (face-attribute face :underline frame) '(unspecified nil))))
 
 
 (defun face-inverse-video-p (face &optional frame)
- "Return non-nil if FACE is in inverse video on FRAME.
+ "Return non-nil if FACE specifies a non-nil inverse-video.
 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."
@@ -837,21 +842,24 @@ and DATA is a string, containing the raw bits of the bitmap."
   (set-face-attribute face frame :stipple (or stipple 'unspecified)))
 
 
-(defun set-face-underline-p (face underline &optional frame)
+(defun set-face-underline (face underline &optional frame)
   "Specify whether face FACE is underlined.
 UNDERLINE nil means FACE explicitly doesn't underline.
-UNDERLINE non-nil means FACE explicitly does underlining
-with the same of the foreground color.
-If UNDERLINE is a string, underline with the color named UNDERLINE.
+UNDERLINE t means FACE underlines with its foreground color.
+If UNDERLINE is a string, underline with that color.
+
+UNDERLINE may also be a list of the form (:color COLOR :style STYLE),
+where COLOR is a string or `foreground-color', and STYLE is either
+`line' or `wave'.  :color may be omitted, which means to use the
+foreground color.  :style may be omitted, which means to use a line.
+
 FRAME nil or not specified means change face on all frames.
 Use `set-face-attribute' to ``unspecify'' underlining."
-  (interactive
-   (let ((list (read-face-and-attribute :underline)))
-     (list (car list) (eq (car (cdr list)) t))))
+  (interactive (read-face-and-attribute :underline))
   (set-face-attribute face frame :underline underline))
 
-(define-obsolete-function-alias 'set-face-underline
-                                'set-face-underline-p "22.1")
+(define-obsolete-function-alias 'set-face-underline-p
+                                'set-face-underline "24.3")
 
 
 (defun set-face-inverse-video-p (face inverse-video-p &optional frame)
@@ -866,6 +874,9 @@ Use `set-face-attribute' to ``unspecify'' the inverse video attribute."
   (set-face-attribute face frame :inverse-video inverse-video-p))
 
 
+;; The -p suffix is a hostage to fortune.  What if we want to extend
+;; this to allow more than boolean options?  Exactly this happened
+;; to set-face-underline-p.
 (defun set-face-bold-p (face bold-p &optional frame)
   "Specify whether face FACE is bold.
 BOLD-P non-nil means FACE should explicitly display bold.
@@ -1114,6 +1125,9 @@ name of the attribute for prompting.  Value is the new attribute value."
            (string-to-number new-value)))))
 
 
+;; FIXME this does allow you to enter the list forms of :box,
+;; :stipple, or :underline, because face-valid-attribute-values does
+;; not return those forms.
 (defun read-face-attribute (face attribute &optional frame)
   "Interactively read a new value for FACE's ATTRIBUTE.
 Optional argument FRAME nil or unspecified means read an attribute value
@@ -1125,12 +1139,11 @@ of a global face.  Value is the new attribute value."
     ;; Represent complex attribute values as strings by printing them
     ;; out.  Stipple can be a vector; (WIDTH HEIGHT DATA).  Box can be
     ;; a list `(:width WIDTH :color COLOR)' or `(:width WIDTH :shadow
-    ;; SHADOW)'.
-    (when (and (or (eq attribute :stipple)
-                   (eq attribute :box))
-               (or (consp old-value)
-                   (vectorp old-value)))
-      (setq old-value (prin1-to-string old-value)))
+    ;; SHADOW)'.  Underline can be `(:color COLOR :style STYLE)'.
+    (and (memq attribute '(:box :stipple :underline))
+         (or (consp old-value)
+             (vectorp old-value))
+         (setq old-value (prin1-to-string old-value)))
     (cond ((listp valid)
            (let ((default
                    (or (car (rassoc old-value valid))
@@ -1160,11 +1173,10 @@ of a global face.  Value is the new attribute value."
     ;; Convert stipple and box value text we read back to a list or
     ;; vector if it looks like one.  This makes the assumption that a
     ;; pixmap file name won't start with an open-paren.
-    (when (and (or (eq attribute :stipple)
-                   (eq attribute :box))
-               (stringp new-value)
-               (string-match "^[[(]" new-value))
-      (setq new-value (read new-value)))
+    (and (memq attribute '(:stipple :box :underline))
+         (stringp new-value)
+         (string-match "^[[(]" new-value)
+         (setq new-value (read new-value)))
     new-value))
 
 (declare-function fontset-list "fontset.c" ())
diff --git a/src/xfaces.c b/src/xfaces.c
index 221387c4b6d..5eda6dca6da 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -2906,6 +2906,12 @@ FRAME 0 means change the face on all frames, and change the default
           Lisp_Object key, val, list;
 
           list = value;
+          /* FIXME?  This errs on the side of acceptance.  Eg it accepts:
+               (defface foo '((t :underline 'foo) "doc")
+             Maybe this is intentional, maybe it isn't.
+             Non-nil symbols other than t are not documented as being valid.
+             Eg compare with inverse-video, which explicitly rejects them.
+          */
           valid_p = 1;
 
           while (!NILP (CAR_SAFE(list)))
@@ -5727,6 +5733,8 @@ realize_x_face (struct face_cache *cache, Lisp_Object attrs[LFACE_VECTOR_SIZE])
       face->underline_defaulted_p = 1;
       face->underline_type = FACE_UNDER_LINE;
 
+      /* FIXME?  This is also not robust about checking the precise form.
+         See comments in Finternal_set_lisp_face_attribute.  */
       while (CONSP (underline))
         {
           Lisp_Object keyword, value;