[Emacs-diffs] /srv/bzr/emacs/emacs-24 r110873: Fixes related to face underlining

[Glenn Morris]

------------------------------------------------------------
revno: 110873
committer: Glenn Morris <address@hidden>
branch nick: emacs-24
timestamp: Wed 2012-11-14 23:30:46 -0800
message:
  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.
modified:
  doc/lispref/ChangeLog
  doc/lispref/display.texi
  etc/NEWS
  lisp/ChangeLog
  lisp/faces.el
  src/xfaces.c

=== modified file 'doc/lispref/ChangeLog'
--- a/doc/lispref/ChangeLog     2012-11-15 06:13:46 +0000
+++ b/doc/lispref/ChangeLog     2012-11-15 07:30:46 +0000
@@ -1,6 +1,8 @@
 2012-11-15  Glenn Morris  <address@hidden>
 
        * 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  <address@hidden>
 

=== modified file 'doc/lispref/display.texi'
--- a/doc/lispref/display.texi  2012-11-15 06:13:46 +0000
+++ b/doc/lispref/display.texi  2012-11-15 07:30:46 +0000
@@ -2403,7 +2403,7 @@
 @var{italic-p} is @code{nil}, and to @var{italic} otherwise.
 @end defun
 
address@hidden set-face-underline-p face underline &optional frame
address@hidden 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 @@
 @code{nil} otherwise.
 @end defun
 
address@hidden Note the weasel words.  A face that inherits from an underlined
address@hidden 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 address@hidden if face @var{face} specifies
+a address@hidden @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 address@hidden if face @var{face} specifies
+a address@hidden @code{:inverse-video} attribute.
 @end defun
 
 @node Displaying Faces

=== modified file 'etc/NEWS'
--- a/etc/NEWS  2012-11-14 08:45:50 +0000
+++ b/etc/NEWS  2012-11-15 07:30:46 +0000
@@ -948,8 +948,8 @@
 +++
 ** 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:
 ---

=== modified file 'lisp/ChangeLog'
--- a/lisp/ChangeLog    2012-11-15 06:17:56 +0000
+++ b/lisp/ChangeLog    2012-11-15 07:30:46 +0000
@@ -1,5 +1,13 @@
 2012-11-15  Glenn Morris  <address@hidden>
 
+       * 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)
 

=== modified file 'lisp/faces.el'
--- a/lisp/faces.el     2012-10-23 02:39:13 +0000
+++ b/lisp/faces.el     2012-11-15 07:30:46 +0000
@@ -487,16 +487,21 @@
 (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 @@
   (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 @@
   (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 @@
           (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 @@
     ;; 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 @@
     ;; 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" ())

=== modified file 'src/xfaces.c'
--- a/src/xfaces.c      2012-10-23 02:39:13 +0000
+++ b/src/xfaces.c      2012-11-15 07:30:46 +0000
@@ -2906,6 +2906,12 @@
           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 @@
       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;