[PATCH 0/2] notmuch-combine-face-text-property improvements

[PATCH 0/2] notmuch-combine-face-text-property improvements

[Austin Clements]

These two patches fix bugs in and improve on
notmuch-combine-face-text-property.  The bug fixed by the first patch
was found by Damien and the features added in the second patch should
help with notmuch-tagger.

(Sorry for the delay with these.  I've had them sitting in my tree for
a while with the intent to write some nice unit tests for them, but
real life got in the way and I probably won't have time to do anything
nontrivial for a while.)

[PATCH 1/2] emacs: Handle all face forms when combining faces

[Austin Clements]

Previously, notmuch-combine-face-text-property assumed that any
existing face properties of the modified text were already in face
list form.  This was true as long as it was the only function
manipulating faces (since it always produced a list form face), but if
anything else has manipulated the face, it was more likely to be
either a face name or a face plist.  It also didn't correctly handle
face lists as arguments, even though the doc string claimed it did.

This patch fixes notmuch-combine-face-text-property to handle all face
forms correctly by canonicalizing both the argument face and the
existing faces into list form.  This also means we can set the face to
a simpler non-list form if there's no existing face.
---
 emacs/notmuch-lib.el |   27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index d78bcf8..ad2816d 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -301,6 +301,16 @@ current buffer, if possible."
   (loop for (key value . rest) on plist by #'cddr
 	collect (cons (intern (substring (symbol-name key) 1)) value)))
 
+(defun notmuch-face-ensure-list-form (face)
+  "Return FACE in face list form.
+
+If FACE is already a face list, it will be returned as-is.  If
+FACE is a face name or face plist, it will be returned as a
+single element face list."
+  (if (and (listp face) (not (keywordp (car face))))
+      face
+    (list face)))
+
 (defun notmuch-combine-face-text-property (start end face)
   "Combine FACE into the 'face text property between START and END.
 
@@ -309,11 +319,20 @@ and END.  Attributes specified by FACE take precedence over
 existing attributes.  FACE must be a face name (a symbol or
 string), a property list of face attributes, or a list of these."
 
-  (let ((pos start))
+  ;; A face property can have three forms: a face name (a string or
+  ;; symbol), a property list, or a list of these two forms.  In the
+  ;; list case, the faces will be combined, with the earlier faces
+  ;; taking precedent.  Here we canonicalize everything to list form
+  ;; to make it easy to combine.
+  (let ((pos start)
+	(face-list (notmuch-face-ensure-list-form face)))
     (while (< pos end)
-      (let ((cur (get-text-property pos 'face))
-	    (next (next-single-property-change pos 'face nil end)))
-	(put-text-property pos next 'face (cons face cur))
+      (let* ((cur (get-text-property pos 'face))
+	     (cur-list (notmuch-face-ensure-list-form cur))
+	     (new (cond ((null cur-list) face)
+			(t (append face-list cur-list))))
+	     (next (next-single-property-change pos 'face nil end)))
+	(put-text-property pos next 'face new)
 	(setq pos next)))))
 
 (defun notmuch-logged-error (msg &optional extra)
-- 
1.7.10.4

[PATCH 2/2] emacs: Combine string faces and combine under existing faces

[Austin Clements]

This improves notmuch-combine-face-text-property to support both
applying faces to strings and to support combining the given face
under existing faces, rather than over.
---
 emacs/notmuch-lib.el |   21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index ad2816d..7c8816d 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -311,13 +311,16 @@ single element face list."
       face
     (list face)))
 
-(defun notmuch-combine-face-text-property (start end face)
+(defun notmuch-combine-face-text-property (start end face &optional below object)
   "Combine FACE into the 'face text property between START and END.
 
 This function combines FACE with any existing faces between START
-and END.  Attributes specified by FACE take precedence over
-existing attributes.  FACE must be a face name (a symbol or
-string), a property list of face attributes, or a list of these."
+and END in OBJECT (which defaults to the current buffer).
+Attributes specified by FACE take precedence over existing
+attributes unless BELOW is non-nil.  FACE must be a face name (a
+symbol or string), a property list of face attributes, or a list
+of these.  For convenience when applied to strings, this returns
+OBJECT."
 
   ;; A face property can have three forms: a face name (a string or
   ;; symbol), a property list, or a list of these two forms.  In the
@@ -327,13 +330,15 @@ string), a property list of face attributes, or a list of these."
   (let ((pos start)
 	(face-list (notmuch-face-ensure-list-form face)))
     (while (< pos end)
-      (let* ((cur (get-text-property pos 'face))
+      (let* ((cur (get-text-property pos 'face object))
 	     (cur-list (notmuch-face-ensure-list-form cur))
 	     (new (cond ((null cur-list) face)
+			(below (append cur-list face-list))
 			(t (append face-list cur-list))))
-	     (next (next-single-property-change pos 'face nil end)))
-	(put-text-property pos next 'face new)
-	(setq pos next)))))
+	     (next (next-single-property-change pos 'face object end)))
+	(put-text-property pos next 'face new object)
+	(setq pos next))))
+  object)
 
 (defun notmuch-logged-error (msg &optional extra)
   "Log MSG and EXTRA to *Notmuch errors* and signal MSG.
-- 
1.7.10.4

Re: [PATCH 0/2] notmuch-combine-face-text-property improvements

[Damien Cassou]

On Mon, Feb 4, 2013 at 10:37 PM, Austin Clements <amdragon@mit.edu> wrote:
> These two patches fix bugs in and improve on
> notmuch-combine-face-text-property.  The bug fixed by the first patch
> was found by Damien and the features added in the second patch should
> help with notmuch-tagger.
>
> (Sorry for the delay with these.  I've had them sitting in my tree for
> a while with the intent to write some nice unit tests for them, but
> real life got in the way and I probably won't have time to do anything
> nontrivial for a while.)


works for me, thank you Austin.

-- 
Damien Cassou
http://damiencassou.seasidehosting.st

"Success is the ability to go from one failure to another without
losing enthusiasm."
Winston Churchill