[PATCH 0/7] emacs: JSON-based search cleanups

[PATCH 0/7] emacs: JSON-based search cleanups

[Austin Clements]

This series builds on the JSON-based search series [0] to clean up
several other aspects of search-mode.  It removes constraints on the
formatting of tags in the result line (you can even leave them out
entirely), it recolors lines when tags change, it adds supports for
multi-line result formats, and rendering big search buffers should be
less quadratic (it might even be linear).  Much of this derives from
having a single object representation of a result (the JSON plist) and
a simple method for rendering it to the buffer.

[0] 1341870162-17782-1-git-send-email-amdragon@mit.edu

[PATCH 1/7] emacs: Record thread search result object in a text property

[Austin Clements]

This also provides utility functions for working with this text
property that get its value, find its start, and find its end.
---
 emacs/notmuch.el |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index fabb7c0..ef18927 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -401,6 +401,32 @@ Complete list of currently available key bindings:
 	mode-name "notmuch-search")
   (setq buffer-read-only t))
 
+(defun notmuch-search-get-result (&optional pos)
+  "Return the result object for the thread at POS (or point).
+
+If there is no thread at POS (or point), returns nil."
+  (get-text-property (or pos (point)) 'notmuch-search-result))
+
+(defun notmuch-search-result-beginning (&optional pos)
+  "Return the point at the beginning of the thread at POS (or point).
+
+If there is no thread at POS (or point), returns nil."
+  (when (notmuch-search-get-result pos)
+    ;; We pass 1+point because previous-single-property-change starts
+    ;; searching one before the position we give it.
+    (previous-single-property-change (1+ (or pos (point)))
+				     'notmuch-search-result nil (point-min))))
+
+(defun notmuch-search-result-end (&optional pos)
+  "Return the point at the end of the thread at POS (or point).
+
+The returned point will be just after the newline character that
+ends the result line.  If there is no thread at POS (or point),
+returns nil"
+  (when (notmuch-search-get-result pos)
+    (next-single-property-change (or pos (point)) 'notmuch-search-result
+				 nil (point-max))))
+
 (defun notmuch-search-properties-in-region (property beg end)
   (save-excursion
     (let ((output nil)
@@ -736,6 +762,7 @@ non-authors is found, assume that all of the authors match."
 	  (notmuch-search-insert-field (car spec) (cdr spec) result))
 	(insert "\n")
 	(notmuch-search-color-line beg (point) (plist-get result :tags))
+	(put-text-property beg (point) 'notmuch-search-result result)
 	(put-text-property beg (point) 'notmuch-search-thread-id
 			   (concat "thread:" (plist-get result :thread)))
 	(put-text-property beg (point) 'notmuch-search-authors
-- 
1.7.10

[PATCH 2/7] emacs: Use text properties instead of overlays for tag coloring

[Austin Clements]

Previously, tag-based search result highlighting was done by creating
an overlay over each search result.  However, overlays have annoying
front- and rear-advancement semantics that make it difficult to
manipulate text at their boundaries, which the next patch will do.
They also have performance problems (creating an overlay is linear in
the number of overlays between point and the new overlay, making
highlighting a search buffer quadratic in the number of results).

Text properties have neither problem.  However, text properties make
it more difficult to apply multiple faces since, unlike with overlays,
a given character can only have a single 'face text property.  Hence,
we introduce a utility function that combines faces into any existing
'face text properties.

Using this utility function, it's straightforward to apply all of the
appropriate tag faces in notmuch-search-color-line.
---
 emacs/notmuch-lib.el |   15 +++++++++++++++
 emacs/notmuch.el     |   21 +++++++--------------
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index aa25513..30db58f 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -269,6 +269,21 @@ current buffer, if possible."
   (loop for (key value . rest) on plist by #'cddr
 	collect (cons (intern (substring (symbol-name key) 1)) value)))
 
+(defun notmuch-combine-face-text-property (start end face)
+  "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."
+
+  (let ((pos start))
+    (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))
+	(setq pos next)))))
+
 ;; Compatibility functions for versions of emacs before emacs 23.
 ;;
 ;; Both functions here were copied from emacs 23 with the following copyright:
diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index ef18927..82c148d 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -633,20 +633,13 @@ foreground and blue background."
 
 (defun notmuch-search-color-line (start end line-tag-list)
   "Colorize lines in `notmuch-show' based on tags."
-  ;; Create the overlay only if the message has tags which match one
-  ;; of those specified in `notmuch-search-line-faces'.
-  (let (overlay)
-    (mapc (lambda (elem)
-	    (let ((tag (car elem))
-		  (attributes (cdr elem)))
-	      (when (member tag line-tag-list)
-		(when (not overlay)
-		  (setq overlay (make-overlay start end)))
-		;; Merge the specified properties with any already
-		;; applied from an earlier match.
-		(overlay-put overlay 'face
-			     (append (overlay-get overlay 'face) attributes)))))
-	  notmuch-search-line-faces)))
+  (mapc (lambda (elem)
+	  (let ((tag (car elem))
+		(attributes (cdr elem)))
+	    (when (member tag line-tag-list)
+	      (notmuch-combine-face-text-property start end attributes))))
+	;; Reverse the list so earlier entries take precedence
+	(reverse notmuch-search-line-faces)))
 
 (defun notmuch-search-author-propertize (authors)
   "Split `authors' into matching and non-matching authors and
-- 
1.7.10

[PATCH 3/7] emacs: Update tags by rewriting the search result line in place

[Austin Clements]

Now that we keep the full thread result object, we can refresh a
result after any changes by simply deleting and reconstructing the
result line from scratch.

A convenient side-effect of this wholesale replacement is that search
now re-applies notmuch-search-line-faces when tags change.
---
 emacs/notmuch.el |   55 ++++++++++++++++++++++++++++++------------------------
 1 file changed, 31 insertions(+), 24 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 82c148d..2f83425 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -509,28 +509,12 @@ and will also appear in a buffer named \"*Notmuch errors*\"."
 	    (error (buffer-substring beg end))
 	    ))))))
 
-(defun notmuch-search-set-tags (tags)
-  (save-excursion
-    (end-of-line)
-    (re-search-backward "(")
-    (forward-char)
-    (let ((beg (point))
-	  (inhibit-read-only t))
-      (re-search-forward ")")
-      (backward-char)
-      (let ((end (point)))
-	(delete-region beg end)
-	(insert (propertize (mapconcat  'identity tags " ")
-			    'face 'notmuch-tag-face))))))
-
-(defun notmuch-search-get-tags ()
-  (save-excursion
-    (end-of-line)
-    (re-search-backward "(")
-    (let ((beg (+ (point) 1)))
-      (re-search-forward ")")
-      (let ((end (- (point) 1)))
-	(split-string (buffer-substring-no-properties beg end))))))
+(defun notmuch-search-set-tags (tags &optional pos)
+  (let ((new-result (plist-put (notmuch-search-get-result pos) :tags tags)))
+    (notmuch-search-update-result new-result pos)))
+
+(defun notmuch-search-get-tags (&optional pos)
+  (plist-get (notmuch-search-get-result pos) :tags))
 
 (defun notmuch-search-get-tags-region (beg end)
   (save-excursion
@@ -583,6 +567,29 @@ This function advances the next thread when finished."
   (notmuch-search-tag '("-inbox"))
   (notmuch-search-next-thread))
 
+(defun notmuch-search-update-result (result &optional pos)
+  "Update the result object of the current thread and redraw it."
+  (let ((start (notmuch-search-result-beginning pos))
+	(end (notmuch-search-result-end pos))
+	(init-point (point))
+	(inhibit-read-only t))
+    ;; Delete the current thread
+    (delete-region start end)
+    ;; Insert the updated thread
+    (notmuch-search-show-result result start)
+    ;; There may have been markers pointing into the text we just
+    ;; replaced.  For the most part, there's nothing we can do about
+    ;; this, but we can fix markers that were at point (which includes
+    ;; point itself and any save-excursions for which point hasn't
+    ;; moved) by re-inserting the text that should come before point
+    ;; before markers.
+    (when (and (>= init-point start) (<= init-point end))
+      (let* ((new-end (notmuch-search-result-end start))
+	     (new-point (if (= init-point end)
+			    new-end
+			  (min init-point (- new-end 1)))))
+	(insert-before-markers (delete-and-extract-region start new-point))))))
+
 (defun notmuch-search-process-sentinel (proc msg)
   "Add a message to let user know when \"notmuch search\" exits"
   (let ((buffer (process-buffer proc))
@@ -745,10 +752,10 @@ non-authors is found, assume that all of the authors match."
 			 (mapconcat 'identity (plist-get result :tags) " ")
 			 'font-lock-face 'notmuch-tag-face) ")")))))
 
-(defun notmuch-search-show-result (result)
+(defun notmuch-search-show-result (result &optional pos)
   ;; Ignore excluded matches
   (unless (= (plist-get result :matched) 0)
-    (let ((beg (point-max)))
+    (let ((beg (or pos (point-max))))
       (save-excursion
 	(goto-char beg)
 	(dolist (spec notmuch-search-result-format)
-- 
1.7.10

[PATCH 4/7] emacs: Use result text properties for search result iteration

[Austin Clements]

This simplifies the traversal of regions of results and eliminates the
need for save-excursions (which tend to get in the way of maintaining
point when we make changes to the buffer).  It also fixes some strange
corner cases in the old line-based code where results that bordered
the region but were not included in it could be affected by region
commands.  Coincidentally, this also essentially enables multi-line
search result formats; the only remaining non-multi-line-capable
functions are notmuch-search-{next,previous}-thread, which are only
used interactively.
---
 emacs/notmuch.el |   61 ++++++++++++++++++++++++++++++------------------------
 1 file changed, 34 insertions(+), 27 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 2f83425..3f72427 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -427,17 +427,33 @@ returns nil"
     (next-single-property-change (or pos (point)) 'notmuch-search-result
 				 nil (point-max))))
 
+(defmacro notmuch-search-do-results (beg end pos-sym &rest body)
+  "Invoke BODY for each result between BEG and END.
+
+POS-SYM will be bound to the point at the beginning of the
+current result."
+  (declare (indent 3))
+  (let ((end-sym (make-symbol "end"))
+	(first-sym (make-symbol "first")))
+    `(let ((,pos-sym (notmuch-search-result-beginning ,beg))
+	   ;; End must be a marker in case body changes the text
+	   (,end-sym (copy-marker ,end))
+	   ;; Make sure we examine one result, even if (= beg end)
+	   (,first-sym t))
+       ;; We have to be careful if the region extends beyond the
+       ;; results.  In this case, pos could be null or there could be
+       ;; no result at pos.
+       (while (and ,pos-sym (or (< ,pos-sym ,end-sym) ,first-sym))
+	 (when (notmuch-search-get-result ,pos-sym)
+	   ,@body)
+	 (setq ,pos-sym (notmuch-search-result-end ,pos-sym)
+	       ,first-sym nil)))))
+
 (defun notmuch-search-properties-in-region (property beg end)
-  (save-excursion
-    (let ((output nil)
-	  (last-line (line-number-at-pos end))
-	  (max-line (- (line-number-at-pos (point-max)) 2)))
-      (goto-char beg)
-      (beginning-of-line)
-      (while (<= (line-number-at-pos) (min last-line max-line))
-	(setq output (cons (get-text-property (point) property) output))
-	(forward-line 1))
-      output)))
+  (let (output)
+    (notmuch-search-do-results beg end pos
+      (push (get-text-property pos property) output))
+    output))
 
 (defun notmuch-search-find-thread-id ()
   "Return the thread for the current thread"
@@ -517,28 +533,19 @@ and will also appear in a buffer named \"*Notmuch errors*\"."
   (plist-get (notmuch-search-get-result pos) :tags))
 
 (defun notmuch-search-get-tags-region (beg end)
-  (save-excursion
-    (let ((output nil)
-	  (last-line (line-number-at-pos end))
-	  (max-line (- (line-number-at-pos (point-max)) 2)))
-      (goto-char beg)
-      (while (<= (line-number-at-pos) (min last-line max-line))
-	(setq output (append output (notmuch-search-get-tags)))
-	(forward-line 1))
-      output)))
+  (let (output)
+    (notmuch-search-do-results beg end pos
+      (setq output (append output (notmuch-search-get-tags pos))))
+    output))
 
 (defun notmuch-search-tag-region (beg end &optional tag-changes)
   "Change tags for threads in the given region."
   (let ((search-string (notmuch-search-find-thread-id-region-search beg end)))
     (setq tag-changes (funcall 'notmuch-tag search-string tag-changes))
-    (save-excursion
-      (let ((last-line (line-number-at-pos end))
-	    (max-line (- (line-number-at-pos (point-max)) 2)))
-	(goto-char beg)
-	(while (<= (line-number-at-pos) (min last-line max-line))
-	  (notmuch-search-set-tags
-	   (notmuch-update-tags (notmuch-search-get-tags) tag-changes))
-	  (forward-line))))))
+    (notmuch-search-do-results beg end pos
+      (notmuch-search-set-tags
+       (notmuch-update-tags (notmuch-search-get-tags pos) tag-changes)
+       pos))))
 
 (defun notmuch-search-tag (&optional tag-changes)
   "Change tags for the currently selected thread or region.
-- 
1.7.10

[PATCH 5/7] emacs: Replace other search text properties with result property

[Austin Clements]

Since the result object contains everything that the other text
properties recorded, we can remove the other text properties and
simply look in the plist of the appropriate result object.
---
 emacs/notmuch.el |   24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 3f72427..a5cf0dc 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -452,16 +452,18 @@ current result."
 (defun notmuch-search-properties-in-region (property beg end)
   (let (output)
     (notmuch-search-do-results beg end pos
-      (push (get-text-property pos property) output))
+      (push (plist-get (notmuch-search-get-result pos) property) output))
     output))
 
 (defun notmuch-search-find-thread-id ()
   "Return the thread for the current thread"
-  (get-text-property (point) 'notmuch-search-thread-id))
+  (let ((thread (plist-get (notmuch-search-get-result) :thread)))
+    (when thread (concat "thread:" thread))))
 
 (defun notmuch-search-find-thread-id-region (beg end)
   "Return a list of threads for the current region"
-  (notmuch-search-properties-in-region 'notmuch-search-thread-id beg end))
+  (mapcar (lambda (thread) (concat "thread:" thread))
+	  (notmuch-search-properties-in-region :thread beg end)))
 
 (defun notmuch-search-find-thread-id-region-search (beg end)
   "Return a search string for threads for the current region"
@@ -469,19 +471,19 @@ current result."
 
 (defun notmuch-search-find-authors ()
   "Return the authors for the current thread"
-  (get-text-property (point) 'notmuch-search-authors))
+  (plist-get (notmuch-search-get-result) :authors))
 
 (defun notmuch-search-find-authors-region (beg end)
   "Return a list of authors for the current region"
-  (notmuch-search-properties-in-region 'notmuch-search-authors beg end))
+  (notmuch-search-properties-in-region :authors beg end))
 
 (defun notmuch-search-find-subject ()
   "Return the subject for the current thread"
-  (get-text-property (point) 'notmuch-search-subject))
+  (plist-get (notmuch-search-get-result) :subject))
 
 (defun notmuch-search-find-subject-region (beg end)
   "Return a list of authors for the current region"
-  (notmuch-search-properties-in-region 'notmuch-search-subject beg end))
+  (notmuch-search-properties-in-region :subject beg end))
 
 (defun notmuch-search-show-thread ()
   "Display the currently selected thread."
@@ -769,13 +771,7 @@ non-authors is found, assume that all of the authors match."
 	  (notmuch-search-insert-field (car spec) (cdr spec) result))
 	(insert "\n")
 	(notmuch-search-color-line beg (point) (plist-get result :tags))
-	(put-text-property beg (point) 'notmuch-search-result result)
-	(put-text-property beg (point) 'notmuch-search-thread-id
-			   (concat "thread:" (plist-get result :thread)))
-	(put-text-property beg (point) 'notmuch-search-authors
-			   (plist-get result :authors))
-	(put-text-property beg (point) 'notmuch-search-subject
-			   (plist-get result :subject)))
+	(put-text-property beg (point) 'notmuch-search-result result))
       (when (string= (plist-get result :thread) notmuch-search-target-thread)
 	(setq notmuch-search-target-thread "found")
 	(goto-char beg)))))
-- 
1.7.10

[PATCH 6/7] emacs: Allow custom tags formatting

[Austin Clements]

Previously we ignored any notmuch-search-result-format customizations
for tag formatting because we needed to be able to parse back in the
result line and update the tags in place.  We no longer do either of
these things, so we can allow customization of this format.

(Coincidentally, previously we still allowed too much customization of
the tags format, since moving it earlier on the line or removing it
from the line would interfere with the tagging mechanism.  There is
now no problem with doing such things.)
---
 emacs/notmuch.el |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index a5cf0dc..f32cfb0 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -755,11 +755,9 @@ non-authors is found, assume that all of the authors match."
     (notmuch-search-insert-authors format-string (plist-get result :authors)))
 
    ((string-equal field "tags")
-    ;; Ignore format-string here because notmuch-search-set-tags
-    ;; depends on the format of this
-    (insert (concat "(" (propertize
-			 (mapconcat 'identity (plist-get result :tags) " ")
-			 'font-lock-face 'notmuch-tag-face) ")")))))
+    (let ((tags-str (mapconcat 'identity (plist-get result :tags) " ")))
+      (insert (propertize (format format-string tags-str)
+			  'face 'notmuch-tag-face))))))
 
 (defun notmuch-search-show-result (result &optional pos)
   ;; Ignore excluded matches
-- 
1.7.10

[PATCH 7/7] emacs: Fix navigation of multi-line search result formats

[Austin Clements]

At this point, the only remaining functions that don't support
multi-line search result formats are the thread navigation functions.
This patch fixes that by rewriting them in terms of
notmuch-search-result-{beginning,end}.
---
 emacs/notmuch.el |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index f32cfb0..2ece97d 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -287,18 +287,24 @@ For a mouse binding, return nil."
 (defun notmuch-search-next-thread ()
   "Select the next thread in the search results."
   (interactive)
-  (forward-line 1))
+  (when (notmuch-search-get-result (notmuch-search-result-end))
+    (goto-char (notmuch-search-result-end))))
 
 (defun notmuch-search-previous-thread ()
   "Select the previous thread in the search results."
   (interactive)
-  (forward-line -1))
+  (if (notmuch-search-get-result)
+      (unless (bobp)
+	(goto-char (notmuch-search-result-beginning (- (point) 1))))
+    ;; We must be past the end; jump to the last result
+    (notmuch-search-last-thread)))
 
 (defun notmuch-search-last-thread ()
   "Select the last thread in the search results."
   (interactive)
   (goto-char (point-max))
-  (forward-line -2))
+  (forward-line -2)
+  (goto-char (notmuch-search-result-beginning)))
 
 (defun notmuch-search-first-thread ()
   "Select the first thread in the search results."
-- 
1.7.10

Re: [PATCH 7/7] emacs: Fix navigation of multi-line search result formats

[Mark Walters]

On Fri, 13 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> At this point, the only remaining functions that don't support
> multi-line search result formats are the thread navigation functions.
> This patch fixes that by rewriting them in terms of
> notmuch-search-result-{beginning,end}.

This seems to subtly change the behaviour in the normal case of a single
line result. If point is not at the start of a search result then
notmuch-search-previous-thread moves to the start of the current thread
rather than the previous thread. Is that deliberate?

As far as I can see show uses p to move to the start of the previous
message so this behaviour is different from there as well. 

I think your choice may be the nicest one (and actually even more so in
show) but thought I would mention it anyway.

Best wishes 

Mark

> ---
>  emacs/notmuch.el |   12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index f32cfb0..2ece97d 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -287,18 +287,24 @@ For a mouse binding, return nil."
>  (defun notmuch-search-next-thread ()
>    "Select the next thread in the search results."
>    (interactive)
> -  (forward-line 1))
> +  (when (notmuch-search-get-result (notmuch-search-result-end))
> +    (goto-char (notmuch-search-result-end))))
>  
>  (defun notmuch-search-previous-thread ()
>    "Select the previous thread in the search results."
>    (interactive)
> -  (forward-line -1))
> +  (if (notmuch-search-get-result)
> +      (unless (bobp)
> +	(goto-char (notmuch-search-result-beginning (- (point) 1))))
> +    ;; We must be past the end; jump to the last result
> +    (notmuch-search-last-thread)))
>  
>  (defun notmuch-search-last-thread ()
>    "Select the last thread in the search results."
>    (interactive)
>    (goto-char (point-max))
> -  (forward-line -2))
> +  (forward-line -2)
> +  (goto-char (notmuch-search-result-beginning)))
>  
>  (defun notmuch-search-first-thread ()
>    "Select the first thread in the search results."
> -- 
> 1.7.10
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 7/7] emacs: Fix navigation of multi-line search result formats

[Austin Clements]

Quoth Mark Walters on Jul 13 at  6:21 pm:
> On Fri, 13 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> > At this point, the only remaining functions that don't support
> > multi-line search result formats are the thread navigation functions.
> > This patch fixes that by rewriting them in terms of
> > notmuch-search-result-{beginning,end}.
> 
> This seems to subtly change the behaviour in the normal case of a single
> line result. If point is not at the start of a search result then
> notmuch-search-previous-thread moves to the start of the current thread
> rather than the previous thread. Is that deliberate?
> 
> As far as I can see show uses p to move to the start of the previous
> message so this behaviour is different from there as well. 
> 
> I think your choice may be the nicest one (and actually even more so in
> show) but thought I would mention it anyway.

Oh, yes, that's true.  I suppose that choice was deliberate in the
sense that I wrote notmuch-search-previous-thread the way I thought it
should work without thinking very hard about how it originally worked.

I could rework it to keep the original behavior, or keep it this way
and document it in the commit message (and maybe NEWS).  Personally I
would prefer to keep it this way and update show's p binding to behave
similarly since I've always found show's p to be confusing.

> Best wishes 
> 
> Mark
> 
> > ---
> >  emacs/notmuch.el |   12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> > index f32cfb0..2ece97d 100644
> > --- a/emacs/notmuch.el
> > +++ b/emacs/notmuch.el
> > @@ -287,18 +287,24 @@ For a mouse binding, return nil."
> >  (defun notmuch-search-next-thread ()
> >    "Select the next thread in the search results."
> >    (interactive)
> > -  (forward-line 1))
> > +  (when (notmuch-search-get-result (notmuch-search-result-end))
> > +    (goto-char (notmuch-search-result-end))))
> >  
> >  (defun notmuch-search-previous-thread ()
> >    "Select the previous thread in the search results."
> >    (interactive)
> > -  (forward-line -1))
> > +  (if (notmuch-search-get-result)
> > +      (unless (bobp)
> > +	(goto-char (notmuch-search-result-beginning (- (point) 1))))
> > +    ;; We must be past the end; jump to the last result
> > +    (notmuch-search-last-thread)))
> >  
> >  (defun notmuch-search-last-thread ()
> >    "Select the last thread in the search results."
> >    (interactive)
> >    (goto-char (point-max))
> > -  (forward-line -2))
> > +  (forward-line -2)
> > +  (goto-char (notmuch-search-result-beginning)))
> >  
> >  (defun notmuch-search-first-thread ()
> >    "Select the first thread in the search results."

Re: [PATCH 3/7] emacs: Update tags by rewriting the search result line in place

[Mark Walters]

On Fri, 13 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> Now that we keep the full thread result object, we can refresh a
> result after any changes by simply deleting and reconstructing the
> result line from scratch.
>
> A convenient side-effect of this wholesale replacement is that search
> now re-applies notmuch-search-line-faces when tags change.
> ---
>  emacs/notmuch.el |   55 ++++++++++++++++++++++++++++++------------------------
>  1 file changed, 31 insertions(+), 24 deletions(-)
>
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 82c148d..2f83425 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -509,28 +509,12 @@ and will also appear in a buffer named \"*Notmuch errors*\"."
>  	    (error (buffer-substring beg end))
>  	    ))))))
>  
> -(defun notmuch-search-set-tags (tags)
> -  (save-excursion
> -    (end-of-line)
> -    (re-search-backward "(")
> -    (forward-char)
> -    (let ((beg (point))
> -	  (inhibit-read-only t))
> -      (re-search-forward ")")
> -      (backward-char)
> -      (let ((end (point)))
> -	(delete-region beg end)
> -	(insert (propertize (mapconcat  'identity tags " ")
> -			    'face 'notmuch-tag-face))))))
> -
> -(defun notmuch-search-get-tags ()
> -  (save-excursion
> -    (end-of-line)
> -    (re-search-backward "(")
> -    (let ((beg (+ (point) 1)))
> -      (re-search-forward ")")
> -      (let ((end (- (point) 1)))
> -	(split-string (buffer-substring-no-properties beg end))))))
> +(defun notmuch-search-set-tags (tags &optional pos)
> +  (let ((new-result (plist-put (notmuch-search-get-result pos) :tags tags)))
> +    (notmuch-search-update-result new-result pos)))
> +
> +(defun notmuch-search-get-tags (&optional pos)
> +  (plist-get (notmuch-search-get-result pos) :tags))
>  
>  (defun notmuch-search-get-tags-region (beg end)
>    (save-excursion
> @@ -583,6 +567,29 @@ This function advances the next thread when finished."
>    (notmuch-search-tag '("-inbox"))
>    (notmuch-search-next-thread))
>  
> +(defun notmuch-search-update-result (result &optional pos)
> +  "Update the result object of the current thread and redraw it."

I think this comment reads a little awkwardly. Maybe "Replace the result
object of the current thread by RESULT and redraw it"?

> +  (let ((start (notmuch-search-result-beginning pos))
> +	(end (notmuch-search-result-end pos))
> +	(init-point (point))
> +	(inhibit-read-only t))
> +    ;; Delete the current thread
> +    (delete-region start end)
> +    ;; Insert the updated thread
> +    (notmuch-search-show-result result start)
> +    ;; There may have been markers pointing into the text we just
> +    ;; replaced.  For the most part, there's nothing we can do about
> +    ;; this, but we can fix markers that were at point (which includes
> +    ;; point itself and any save-excursions for which point hasn't
> +    ;; moved) by re-inserting the text that should come before point
> +    ;; before markers.
> +    (when (and (>= init-point start) (<= init-point end))
> +      (let* ((new-end (notmuch-search-result-end start))
> +	     (new-point (if (= init-point end)
> +			    new-end
> +			  (min init-point (- new-end 1)))))
> +	(insert-before-markers (delete-and-extract-region start new-point))))))

I think this doesn't always do quite the right thing with multiline
results: the one that I can reproduce reliably is if you are on the
second line of the first result, and the first line of this result is
the first visible line in the window, and update it (eg by marking
unread) then the window scrolls so that the second line of the first
result is the first line visible in the window (ie the window scrolls up
one line)

> +
>  (defun notmuch-search-process-sentinel (proc msg)
>    "Add a message to let user know when \"notmuch search\" exits"
>    (let ((buffer (process-buffer proc))
> @@ -745,10 +752,10 @@ non-authors is found, assume that all of the authors match."
>  			 (mapconcat 'identity (plist-get result :tags) " ")
>  			 'font-lock-face 'notmuch-tag-face) ")")))))
>  
> -(defun notmuch-search-show-result (result)
> +(defun notmuch-search-show-result (result &optional pos)


Perhaps a documentation line? along the lines of Insert RESULT in buffer
at POS or the end of buffer.

Best wishes

Mark

>    ;; Ignore excluded matches
>    (unless (= (plist-get result :matched) 0)
> -    (let ((beg (point-max)))
> +    (let ((beg (or pos (point-max))))
>        (save-excursion
>  	(goto-char beg)
>  	(dolist (spec notmuch-search-result-format)
> -- 
> 1.7.10
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 2/7] emacs: Use text properties instead of overlays for tag coloring

[Mark Walters]

On Fri, 13 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> Previously, tag-based search result highlighting was done by creating
> an overlay over each search result.  However, overlays have annoying
> front- and rear-advancement semantics that make it difficult to
> manipulate text at their boundaries, which the next patch will do.
> They also have performance problems (creating an overlay is linear in
> the number of overlays between point and the new overlay, making
> highlighting a search buffer quadratic in the number of results).
>
> Text properties have neither problem.  However, text properties make
> it more difficult to apply multiple faces since, unlike with overlays,
> a given character can only have a single 'face text property.  Hence,
> we introduce a utility function that combines faces into any existing
> 'face text properties.
>
> Using this utility function, it's straightforward to apply all of the
> appropriate tag faces in notmuch-search-color-line.

I have some recollection of people talking about text properties as
opposed to overlays some time ago but saying one problem was you
couldn't do invisibility with text properties. Is that correct and is it
a concern? (Otherwise it all looks good)

Best wishes

Mark
> ---
>  emacs/notmuch-lib.el |   15 +++++++++++++++
>  emacs/notmuch.el     |   21 +++++++--------------
>  2 files changed, 22 insertions(+), 14 deletions(-)
>
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index aa25513..30db58f 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -269,6 +269,21 @@ current buffer, if possible."
>    (loop for (key value . rest) on plist by #'cddr
>  	collect (cons (intern (substring (symbol-name key) 1)) value)))
>  
> +(defun notmuch-combine-face-text-property (start end face)
> +  "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."
> +
> +  (let ((pos start))
> +    (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))
> +	(setq pos next)))))
> +
>  ;; Compatibility functions for versions of emacs before emacs 23.
>  ;;
>  ;; Both functions here were copied from emacs 23 with the following copyright:
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index ef18927..82c148d 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -633,20 +633,13 @@ foreground and blue background."
>  
>  (defun notmuch-search-color-line (start end line-tag-list)
>    "Colorize lines in `notmuch-show' based on tags."
> -  ;; Create the overlay only if the message has tags which match one
> -  ;; of those specified in `notmuch-search-line-faces'.
> -  (let (overlay)
> -    (mapc (lambda (elem)
> -	    (let ((tag (car elem))
> -		  (attributes (cdr elem)))
> -	      (when (member tag line-tag-list)
> -		(when (not overlay)
> -		  (setq overlay (make-overlay start end)))
> -		;; Merge the specified properties with any already
> -		;; applied from an earlier match.
> -		(overlay-put overlay 'face
> -			     (append (overlay-get overlay 'face) attributes)))))
> -	  notmuch-search-line-faces)))
> +  (mapc (lambda (elem)
> +	  (let ((tag (car elem))
> +		(attributes (cdr elem)))
> +	    (when (member tag line-tag-list)
> +	      (notmuch-combine-face-text-property start end attributes))))
> +	;; Reverse the list so earlier entries take precedence
> +	(reverse notmuch-search-line-faces)))
>  
>  (defun notmuch-search-author-propertize (authors)
>    "Split `authors' into matching and non-matching authors and
> -- 
> 1.7.10
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 7/7] emacs: Fix navigation of multi-line search result formats

[Mark Walters]

On Fri, 13 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> Quoth Mark Walters on Jul 13 at  6:21 pm:
>> On Fri, 13 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
>> > At this point, the only remaining functions that don't support
>> > multi-line search result formats are the thread navigation functions.
>> > This patch fixes that by rewriting them in terms of
>> > notmuch-search-result-{beginning,end}.
>> 
>> This seems to subtly change the behaviour in the normal case of a single
>> line result. If point is not at the start of a search result then
>> notmuch-search-previous-thread moves to the start of the current thread
>> rather than the previous thread. Is that deliberate?
>> 
>> As far as I can see show uses p to move to the start of the previous
>> message so this behaviour is different from there as well. 
>> 
>> I think your choice may be the nicest one (and actually even more so in
>> show) but thought I would mention it anyway.
>
> Oh, yes, that's true.  I suppose that choice was deliberate in the
> sense that I wrote notmuch-search-previous-thread the way I thought it
> should work without thinking very hard about how it originally worked.
>
> I could rework it to keep the original behavior, or keep it this way
> and document it in the commit message (and maybe NEWS).  Personally I
> would prefer to keep it this way and update show's p binding to behave
> similarly since I've always found show's p to be confusing.

I would be very happy with keeping the new way and updating show: I also
find the p behaviour in show confusing/annoying.

Best wishes

Mark



>
>> Best wishes 
>> 
>> Mark
>> 
>> > ---
>> >  emacs/notmuch.el |   12 +++++++++---
>> >  1 file changed, 9 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/emacs/notmuch.el b/emacs/notmuch.el
>> > index f32cfb0..2ece97d 100644
>> > --- a/emacs/notmuch.el
>> > +++ b/emacs/notmuch.el
>> > @@ -287,18 +287,24 @@ For a mouse binding, return nil."
>> >  (defun notmuch-search-next-thread ()
>> >    "Select the next thread in the search results."
>> >    (interactive)
>> > -  (forward-line 1))
>> > +  (when (notmuch-search-get-result (notmuch-search-result-end))
>> > +    (goto-char (notmuch-search-result-end))))
>> >  
>> >  (defun notmuch-search-previous-thread ()
>> >    "Select the previous thread in the search results."
>> >    (interactive)
>> > -  (forward-line -1))
>> > +  (if (notmuch-search-get-result)
>> > +      (unless (bobp)
>> > +	(goto-char (notmuch-search-result-beginning (- (point) 1))))
>> > +    ;; We must be past the end; jump to the last result
>> > +    (notmuch-search-last-thread)))
>> >  
>> >  (defun notmuch-search-last-thread ()
>> >    "Select the last thread in the search results."
>> >    (interactive)
>> >    (goto-char (point-max))
>> > -  (forward-line -2))
>> > +  (forward-line -2)
>> > +  (goto-char (notmuch-search-result-beginning)))
>> >  
>> >  (defun notmuch-search-first-thread ()
>> >    "Select the first thread in the search results."

Re: [PATCH 0/7] emacs: JSON-based search cleanups

[Mark Walters]

On Fri, 13 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> This series builds on the JSON-based search series [0] to clean up
> several other aspects of search-mode.  It removes constraints on the
> formatting of tags in the result line (you can even leave them out
> entirely), it recolors lines when tags change, it adds supports for
> multi-line result formats, and rendering big search buffers should be
> less quadratic (it might even be linear).  Much of this derives from
> having a single object representation of a result (the JSON plist) and
> a simple method for rendering it to the buffer.

Overall this series looks excellent. I have reviewed all the patches
except patch 4/7 (I will need to work out how macros work to review
that) and they seem fine modulo the small comments I have made.

One final comment: I couldn't persuade the defcustom search result to
take a newline symbol to test multiline results. It seems to escape
entries of \n, and I couldn't enter returns (and ^Q return didn't work
either).

It was easy by editing the option in my .emacs file and then seemed to
work as expected.

Best wishes

Mark
>
> [0] 1341870162-17782-1-git-send-email-amdragon@mit.edu
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 3/7] emacs: Update tags by rewriting the search result line in place

[Austin Clements]

Quoth Mark Walters on Jul 13 at  6:57 pm:
> On Fri, 13 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> > Now that we keep the full thread result object, we can refresh a
> > result after any changes by simply deleting and reconstructing the
> > result line from scratch.
> >
> > A convenient side-effect of this wholesale replacement is that search
> > now re-applies notmuch-search-line-faces when tags change.
> > ---
> >  emacs/notmuch.el |   55 ++++++++++++++++++++++++++++++------------------------
> >  1 file changed, 31 insertions(+), 24 deletions(-)
> >
> > diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> > index 82c148d..2f83425 100644
> > --- a/emacs/notmuch.el
> > +++ b/emacs/notmuch.el
> > @@ -509,28 +509,12 @@ and will also appear in a buffer named \"*Notmuch errors*\"."
> >  	    (error (buffer-substring beg end))
> >  	    ))))))
> >  
> > -(defun notmuch-search-set-tags (tags)
> > -  (save-excursion
> > -    (end-of-line)
> > -    (re-search-backward "(")
> > -    (forward-char)
> > -    (let ((beg (point))
> > -	  (inhibit-read-only t))
> > -      (re-search-forward ")")
> > -      (backward-char)
> > -      (let ((end (point)))
> > -	(delete-region beg end)
> > -	(insert (propertize (mapconcat  'identity tags " ")
> > -			    'face 'notmuch-tag-face))))))
> > -
> > -(defun notmuch-search-get-tags ()
> > -  (save-excursion
> > -    (end-of-line)
> > -    (re-search-backward "(")
> > -    (let ((beg (+ (point) 1)))
> > -      (re-search-forward ")")
> > -      (let ((end (- (point) 1)))
> > -	(split-string (buffer-substring-no-properties beg end))))))
> > +(defun notmuch-search-set-tags (tags &optional pos)
> > +  (let ((new-result (plist-put (notmuch-search-get-result pos) :tags tags)))
> > +    (notmuch-search-update-result new-result pos)))
> > +
> > +(defun notmuch-search-get-tags (&optional pos)
> > +  (plist-get (notmuch-search-get-result pos) :tags))
> >  
> >  (defun notmuch-search-get-tags-region (beg end)
> >    (save-excursion
> > @@ -583,6 +567,29 @@ This function advances the next thread when finished."
> >    (notmuch-search-tag '("-inbox"))
> >    (notmuch-search-next-thread))
> >  
> > +(defun notmuch-search-update-result (result &optional pos)
> > +  "Update the result object of the current thread and redraw it."
> 
> I think this comment reads a little awkwardly. Maybe "Replace the result
> object of the current thread by RESULT and redraw it"?

Better.  I should also say that it's the thread at POS (or point).

> > +  (let ((start (notmuch-search-result-beginning pos))
> > +	(end (notmuch-search-result-end pos))
> > +	(init-point (point))
> > +	(inhibit-read-only t))
> > +    ;; Delete the current thread
> > +    (delete-region start end)
> > +    ;; Insert the updated thread
> > +    (notmuch-search-show-result result start)
> > +    ;; There may have been markers pointing into the text we just
> > +    ;; replaced.  For the most part, there's nothing we can do about
> > +    ;; this, but we can fix markers that were at point (which includes
> > +    ;; point itself and any save-excursions for which point hasn't
> > +    ;; moved) by re-inserting the text that should come before point
> > +    ;; before markers.
> > +    (when (and (>= init-point start) (<= init-point end))
> > +      (let* ((new-end (notmuch-search-result-end start))
> > +	     (new-point (if (= init-point end)
> > +			    new-end
> > +			  (min init-point (- new-end 1)))))
> > +	(insert-before-markers (delete-and-extract-region start new-point))))))
> 
> I think this doesn't always do quite the right thing with multiline
> results: the one that I can reproduce reliably is if you are on the
> second line of the first result, and the first line of this result is
> the first visible line in the window, and update it (eg by marking
> unread) then the window scrolls so that the second line of the first
> result is the first line visible in the window (ie the window scrolls up
> one line)

Nice catch!  I really wish Emacs had a "replace text" function that
did sane things with markers and the window position and such; without
that anything I do is going to be finicky.  I'll see if saving and
restoring the window scroll position fixes this.

> > +
> >  (defun notmuch-search-process-sentinel (proc msg)
> >    "Add a message to let user know when \"notmuch search\" exits"
> >    (let ((buffer (process-buffer proc))
> > @@ -745,10 +752,10 @@ non-authors is found, assume that all of the authors match."
> >  			 (mapconcat 'identity (plist-get result :tags) " ")
> >  			 'font-lock-face 'notmuch-tag-face) ")")))))
> >  
> > -(defun notmuch-search-show-result (result)
> > +(defun notmuch-search-show-result (result &optional pos)
> 
> 
> Perhaps a documentation line? along the lines of Insert RESULT in buffer
> at POS or the end of buffer.

Good idea.

> Best wishes
> 
> Mark
> 
> >    ;; Ignore excluded matches
> >    (unless (= (plist-get result :matched) 0)
> > -    (let ((beg (point-max)))
> > +    (let ((beg (or pos (point-max))))
> >        (save-excursion
> >  	(goto-char beg)
> >  	(dolist (spec notmuch-search-result-format)

Re: [PATCH 2/7] emacs: Use text properties instead of overlays for tag coloring

[Austin Clements]

Quoth Mark Walters on Jul 13 at  6:59 pm:
> On Fri, 13 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> > Previously, tag-based search result highlighting was done by creating
> > an overlay over each search result.  However, overlays have annoying
> > front- and rear-advancement semantics that make it difficult to
> > manipulate text at their boundaries, which the next patch will do.
> > They also have performance problems (creating an overlay is linear in
> > the number of overlays between point and the new overlay, making
> > highlighting a search buffer quadratic in the number of results).
> >
> > Text properties have neither problem.  However, text properties make
> > it more difficult to apply multiple faces since, unlike with overlays,
> > a given character can only have a single 'face text property.  Hence,
> > we introduce a utility function that combines faces into any existing
> > 'face text properties.
> >
> > Using this utility function, it's straightforward to apply all of the
> > appropriate tag faces in notmuch-search-color-line.
> 
> I have some recollection of people talking about text properties as
> opposed to overlays some time ago but saying one problem was you
> couldn't do invisibility with text properties. Is that correct and is it
> a concern? (Otherwise it all looks good)

I believe the problem was that isearch can only automatically expand
invisible text that's marked invisible using overlays.  Regardless,
this shouldn't be an issue for this patch because invisibility and
faces are controlled by different text properties.  It also shouldn't
be a problem for the next patch because, while we do use overlays to
hide authors, that overlay should always be strictly within the search
result (even if it's at the end of the result format, we'll follow it
with a newline) and hence deleting the entire search result region
should also delete the author invisibility overlay.

> Best wishes
> 
> Mark
> > ---
> >  emacs/notmuch-lib.el |   15 +++++++++++++++
> >  emacs/notmuch.el     |   21 +++++++--------------
> >  2 files changed, 22 insertions(+), 14 deletions(-)
> >
> > diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> > index aa25513..30db58f 100644
> > --- a/emacs/notmuch-lib.el
> > +++ b/emacs/notmuch-lib.el
> > @@ -269,6 +269,21 @@ current buffer, if possible."
> >    (loop for (key value . rest) on plist by #'cddr
> >  	collect (cons (intern (substring (symbol-name key) 1)) value)))
> >  
> > +(defun notmuch-combine-face-text-property (start end face)
> > +  "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."
> > +
> > +  (let ((pos start))
> > +    (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))
> > +	(setq pos next)))))
> > +
> >  ;; Compatibility functions for versions of emacs before emacs 23.
> >  ;;
> >  ;; Both functions here were copied from emacs 23 with the following copyright:
> > diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> > index ef18927..82c148d 100644
> > --- a/emacs/notmuch.el
> > +++ b/emacs/notmuch.el
> > @@ -633,20 +633,13 @@ foreground and blue background."
> >  
> >  (defun notmuch-search-color-line (start end line-tag-list)
> >    "Colorize lines in `notmuch-show' based on tags."
> > -  ;; Create the overlay only if the message has tags which match one
> > -  ;; of those specified in `notmuch-search-line-faces'.
> > -  (let (overlay)
> > -    (mapc (lambda (elem)
> > -	    (let ((tag (car elem))
> > -		  (attributes (cdr elem)))
> > -	      (when (member tag line-tag-list)
> > -		(when (not overlay)
> > -		  (setq overlay (make-overlay start end)))
> > -		;; Merge the specified properties with any already
> > -		;; applied from an earlier match.
> > -		(overlay-put overlay 'face
> > -			     (append (overlay-get overlay 'face) attributes)))))
> > -	  notmuch-search-line-faces)))
> > +  (mapc (lambda (elem)
> > +	  (let ((tag (car elem))
> > +		(attributes (cdr elem)))
> > +	    (when (member tag line-tag-list)
> > +	      (notmuch-combine-face-text-property start end attributes))))
> > +	;; Reverse the list so earlier entries take precedence
> > +	(reverse notmuch-search-line-faces)))
> >  
> >  (defun notmuch-search-author-propertize (authors)
> >    "Split `authors' into matching and non-matching authors and

Re: [PATCH 0/7] emacs: JSON-based search cleanups

[Austin Clements]

Quoth Mark Walters on Jul 13 at  7:12 pm:
> On Fri, 13 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> > This series builds on the JSON-based search series [0] to clean up
> > several other aspects of search-mode.  It removes constraints on the
> > formatting of tags in the result line (you can even leave them out
> > entirely), it recolors lines when tags change, it adds supports for
> > multi-line result formats, and rendering big search buffers should be
> > less quadratic (it might even be linear).  Much of this derives from
> > having a single object representation of a result (the JSON plist) and
> > a simple method for rendering it to the buffer.
> 
> Overall this series looks excellent. I have reviewed all the patches
> except patch 4/7 (I will need to work out how macros work to review
> that) and they seem fine modulo the small comments I have made.
> 
> One final comment: I couldn't persuade the defcustom search result to
> take a newline symbol to test multiline results. It seems to escape
> entries of \n, and I couldn't enter returns (and ^Q return didn't work
> either).

C-q C-j worked for me.  Not the most obvious, certainly.

> It was easy by editing the option in my .emacs file and then seemed to
> work as expected.
> 
> Best wishes
> 
> Mark
> >
> > [0] 1341870162-17782-1-git-send-email-amdragon@mit.edu

[PATCH v2 0/7] emacs: JSON-based search cleanups

[Austin Clements]

This version updates the docstrings for notmuch-search-update-result
and notmuch-search-show-result and fixes the scrolling artifact in
notmuch-search-update-result.  I also updated the commit message of
patch 7 to mention that it changes the behavior of
notmuch-search-previous-thread.

The diff against v1 is below.

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 2ece97d..7302fa7 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -583,10 +583,12 @@ This function advances the next thread when finished."
   (notmuch-search-next-thread))
 
 (defun notmuch-search-update-result (result &optional pos)
-  "Update the result object of the current thread and redraw it."
+  "Replace the result object of the thread at POS (or point) by
+RESULT and redraw it."
   (let ((start (notmuch-search-result-beginning pos))
 	(end (notmuch-search-result-end pos))
 	(init-point (point))
+	(init-start (window-start))
 	(inhibit-read-only t))
     ;; Delete the current thread
     (delete-region start end)
@@ -603,7 +605,9 @@ This function advances the next thread when finished."
 	     (new-point (if (= init-point end)
 			    new-end
 			  (min init-point (- new-end 1)))))
-	(insert-before-markers (delete-and-extract-region start new-point))))))
+	(insert-before-markers (delete-and-extract-region start new-point))))
+    ;; We also may have shifted the window scroll.  Fix it.
+    (set-window-start (selected-window) init-start)))
 
 (defun notmuch-search-process-sentinel (proc msg)
   "Add a message to let user know when \"notmuch search\" exits"
@@ -766,6 +770,7 @@ non-authors is found, assume that all of the authors match."
 			  'face 'notmuch-tag-face))))))
 
 (defun notmuch-search-show-result (result &optional pos)
+  "Insert RESULT at POS or the end of the buffer if POS is null."
   ;; Ignore excluded matches
   (unless (= (plist-get result :matched) 0)
     (let ((beg (or pos (point-max))))

[PATCH v2 1/7] emacs: Record thread search result object in a text property

[Austin Clements]

This also provides utility functions for working with this text
property that get its value, find its start, and find its end.
---
 emacs/notmuch.el |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index fabb7c0..ef18927 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -401,6 +401,32 @@ Complete list of currently available key bindings:
 	mode-name "notmuch-search")
   (setq buffer-read-only t))
 
+(defun notmuch-search-get-result (&optional pos)
+  "Return the result object for the thread at POS (or point).
+
+If there is no thread at POS (or point), returns nil."
+  (get-text-property (or pos (point)) 'notmuch-search-result))
+
+(defun notmuch-search-result-beginning (&optional pos)
+  "Return the point at the beginning of the thread at POS (or point).
+
+If there is no thread at POS (or point), returns nil."
+  (when (notmuch-search-get-result pos)
+    ;; We pass 1+point because previous-single-property-change starts
+    ;; searching one before the position we give it.
+    (previous-single-property-change (1+ (or pos (point)))
+				     'notmuch-search-result nil (point-min))))
+
+(defun notmuch-search-result-end (&optional pos)
+  "Return the point at the end of the thread at POS (or point).
+
+The returned point will be just after the newline character that
+ends the result line.  If there is no thread at POS (or point),
+returns nil"
+  (when (notmuch-search-get-result pos)
+    (next-single-property-change (or pos (point)) 'notmuch-search-result
+				 nil (point-max))))
+
 (defun notmuch-search-properties-in-region (property beg end)
   (save-excursion
     (let ((output nil)
@@ -736,6 +762,7 @@ non-authors is found, assume that all of the authors match."
 	  (notmuch-search-insert-field (car spec) (cdr spec) result))
 	(insert "\n")
 	(notmuch-search-color-line beg (point) (plist-get result :tags))
+	(put-text-property beg (point) 'notmuch-search-result result)
 	(put-text-property beg (point) 'notmuch-search-thread-id
 			   (concat "thread:" (plist-get result :thread)))
 	(put-text-property beg (point) 'notmuch-search-authors
-- 
1.7.10

[PATCH v2 2/7] emacs: Use text properties instead of overlays for tag coloring

[Austin Clements]

Previously, tag-based search result highlighting was done by creating
an overlay over each search result.  However, overlays have annoying
front- and rear-advancement semantics that make it difficult to
manipulate text at their boundaries, which the next patch will do.
They also have performance problems (creating an overlay is linear in
the number of overlays between point and the new overlay, making
highlighting a search buffer quadratic in the number of results).

Text properties have neither problem.  However, text properties make
it more difficult to apply multiple faces since, unlike with overlays,
a given character can only have a single 'face text property.  Hence,
we introduce a utility function that combines faces into any existing
'face text properties.

Using this utility function, it's straightforward to apply all of the
appropriate tag faces in notmuch-search-color-line.
---
 emacs/notmuch-lib.el |   15 +++++++++++++++
 emacs/notmuch.el     |   21 +++++++--------------
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index aa25513..30db58f 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -269,6 +269,21 @@ current buffer, if possible."
   (loop for (key value . rest) on plist by #'cddr
 	collect (cons (intern (substring (symbol-name key) 1)) value)))
 
+(defun notmuch-combine-face-text-property (start end face)
+  "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."
+
+  (let ((pos start))
+    (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))
+	(setq pos next)))))
+
 ;; Compatibility functions for versions of emacs before emacs 23.
 ;;
 ;; Both functions here were copied from emacs 23 with the following copyright:
diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index ef18927..82c148d 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -633,20 +633,13 @@ foreground and blue background."
 
 (defun notmuch-search-color-line (start end line-tag-list)
   "Colorize lines in `notmuch-show' based on tags."
-  ;; Create the overlay only if the message has tags which match one
-  ;; of those specified in `notmuch-search-line-faces'.
-  (let (overlay)
-    (mapc (lambda (elem)
-	    (let ((tag (car elem))
-		  (attributes (cdr elem)))
-	      (when (member tag line-tag-list)
-		(when (not overlay)
-		  (setq overlay (make-overlay start end)))
-		;; Merge the specified properties with any already
-		;; applied from an earlier match.
-		(overlay-put overlay 'face
-			     (append (overlay-get overlay 'face) attributes)))))
-	  notmuch-search-line-faces)))
+  (mapc (lambda (elem)
+	  (let ((tag (car elem))
+		(attributes (cdr elem)))
+	    (when (member tag line-tag-list)
+	      (notmuch-combine-face-text-property start end attributes))))
+	;; Reverse the list so earlier entries take precedence
+	(reverse notmuch-search-line-faces)))
 
 (defun notmuch-search-author-propertize (authors)
   "Split `authors' into matching and non-matching authors and
-- 
1.7.10

[PATCH v2 3/7] emacs: Update tags by rewriting the search result line in place

[Austin Clements]

Now that we keep the full thread result object, we can refresh a
result after any changes by simply deleting and reconstructing the
result line from scratch.

A convenient side-effect of this wholesale replacement is that search
now re-applies notmuch-search-line-faces when tags change.
---
 emacs/notmuch.el |   60 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 36 insertions(+), 24 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 82c148d..8e1a769 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -509,28 +509,12 @@ and will also appear in a buffer named \"*Notmuch errors*\"."
 	    (error (buffer-substring beg end))
 	    ))))))
 
-(defun notmuch-search-set-tags (tags)
-  (save-excursion
-    (end-of-line)
-    (re-search-backward "(")
-    (forward-char)
-    (let ((beg (point))
-	  (inhibit-read-only t))
-      (re-search-forward ")")
-      (backward-char)
-      (let ((end (point)))
-	(delete-region beg end)
-	(insert (propertize (mapconcat  'identity tags " ")
-			    'face 'notmuch-tag-face))))))
-
-(defun notmuch-search-get-tags ()
-  (save-excursion
-    (end-of-line)
-    (re-search-backward "(")
-    (let ((beg (+ (point) 1)))
-      (re-search-forward ")")
-      (let ((end (- (point) 1)))
-	(split-string (buffer-substring-no-properties beg end))))))
+(defun notmuch-search-set-tags (tags &optional pos)
+  (let ((new-result (plist-put (notmuch-search-get-result pos) :tags tags)))
+    (notmuch-search-update-result new-result pos)))
+
+(defun notmuch-search-get-tags (&optional pos)
+  (plist-get (notmuch-search-get-result pos) :tags))
 
 (defun notmuch-search-get-tags-region (beg end)
   (save-excursion
@@ -583,6 +567,33 @@ This function advances the next thread when finished."
   (notmuch-search-tag '("-inbox"))
   (notmuch-search-next-thread))
 
+(defun notmuch-search-update-result (result &optional pos)
+  "Replace the result object of the thread at POS (or point) by
+RESULT and redraw it."
+  (let ((start (notmuch-search-result-beginning pos))
+	(end (notmuch-search-result-end pos))
+	(init-point (point))
+	(init-start (window-start))
+	(inhibit-read-only t))
+    ;; Delete the current thread
+    (delete-region start end)
+    ;; Insert the updated thread
+    (notmuch-search-show-result result start)
+    ;; There may have been markers pointing into the text we just
+    ;; replaced.  For the most part, there's nothing we can do about
+    ;; this, but we can fix markers that were at point (which includes
+    ;; point itself and any save-excursions for which point hasn't
+    ;; moved) by re-inserting the text that should come before point
+    ;; before markers.
+    (when (and (>= init-point start) (<= init-point end))
+      (let* ((new-end (notmuch-search-result-end start))
+	     (new-point (if (= init-point end)
+			    new-end
+			  (min init-point (- new-end 1)))))
+	(insert-before-markers (delete-and-extract-region start new-point))))
+    ;; We also may have shifted the window scroll.  Fix it.
+    (set-window-start (selected-window) init-start)))
+
 (defun notmuch-search-process-sentinel (proc msg)
   "Add a message to let user know when \"notmuch search\" exits"
   (let ((buffer (process-buffer proc))
@@ -745,10 +756,11 @@ non-authors is found, assume that all of the authors match."
 			 (mapconcat 'identity (plist-get result :tags) " ")
 			 'font-lock-face 'notmuch-tag-face) ")")))))
 
-(defun notmuch-search-show-result (result)
+(defun notmuch-search-show-result (result &optional pos)
+  "Insert RESULT at POS or the end of the buffer if POS is null."
   ;; Ignore excluded matches
   (unless (= (plist-get result :matched) 0)
-    (let ((beg (point-max)))
+    (let ((beg (or pos (point-max))))
       (save-excursion
 	(goto-char beg)
 	(dolist (spec notmuch-search-result-format)
-- 
1.7.10

[PATCH v2 4/7] emacs: Use result text properties for search result iteration

[Austin Clements]

This simplifies the traversal of regions of results and eliminates the
need for save-excursions (which tend to get in the way of maintaining
point when we make changes to the buffer).  It also fixes some strange
corner cases in the old line-based code where results that bordered
the region but were not included in it could be affected by region
commands.  Coincidentally, this also essentially enables multi-line
search result formats; the only remaining non-multi-line-capable
functions are notmuch-search-{next,previous}-thread, which are only
used interactively.
---
 emacs/notmuch.el |   61 ++++++++++++++++++++++++++++++------------------------
 1 file changed, 34 insertions(+), 27 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 8e1a769..92ba2c1 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -427,17 +427,33 @@ returns nil"
     (next-single-property-change (or pos (point)) 'notmuch-search-result
 				 nil (point-max))))
 
+(defmacro notmuch-search-do-results (beg end pos-sym &rest body)
+  "Invoke BODY for each result between BEG and END.
+
+POS-SYM will be bound to the point at the beginning of the
+current result."
+  (declare (indent 3))
+  (let ((end-sym (make-symbol "end"))
+	(first-sym (make-symbol "first")))
+    `(let ((,pos-sym (notmuch-search-result-beginning ,beg))
+	   ;; End must be a marker in case body changes the text
+	   (,end-sym (copy-marker ,end))
+	   ;; Make sure we examine one result, even if (= beg end)
+	   (,first-sym t))
+       ;; We have to be careful if the region extends beyond the
+       ;; results.  In this case, pos could be null or there could be
+       ;; no result at pos.
+       (while (and ,pos-sym (or (< ,pos-sym ,end-sym) ,first-sym))
+	 (when (notmuch-search-get-result ,pos-sym)
+	   ,@body)
+	 (setq ,pos-sym (notmuch-search-result-end ,pos-sym)
+	       ,first-sym nil)))))
+
 (defun notmuch-search-properties-in-region (property beg end)
-  (save-excursion
-    (let ((output nil)
-	  (last-line (line-number-at-pos end))
-	  (max-line (- (line-number-at-pos (point-max)) 2)))
-      (goto-char beg)
-      (beginning-of-line)
-      (while (<= (line-number-at-pos) (min last-line max-line))
-	(setq output (cons (get-text-property (point) property) output))
-	(forward-line 1))
-      output)))
+  (let (output)
+    (notmuch-search-do-results beg end pos
+      (push (get-text-property pos property) output))
+    output))
 
 (defun notmuch-search-find-thread-id ()
   "Return the thread for the current thread"
@@ -517,28 +533,19 @@ and will also appear in a buffer named \"*Notmuch errors*\"."
   (plist-get (notmuch-search-get-result pos) :tags))
 
 (defun notmuch-search-get-tags-region (beg end)
-  (save-excursion
-    (let ((output nil)
-	  (last-line (line-number-at-pos end))
-	  (max-line (- (line-number-at-pos (point-max)) 2)))
-      (goto-char beg)
-      (while (<= (line-number-at-pos) (min last-line max-line))
-	(setq output (append output (notmuch-search-get-tags)))
-	(forward-line 1))
-      output)))
+  (let (output)
+    (notmuch-search-do-results beg end pos
+      (setq output (append output (notmuch-search-get-tags pos))))
+    output))
 
 (defun notmuch-search-tag-region (beg end &optional tag-changes)
   "Change tags for threads in the given region."
   (let ((search-string (notmuch-search-find-thread-id-region-search beg end)))
     (setq tag-changes (funcall 'notmuch-tag search-string tag-changes))
-    (save-excursion
-      (let ((last-line (line-number-at-pos end))
-	    (max-line (- (line-number-at-pos (point-max)) 2)))
-	(goto-char beg)
-	(while (<= (line-number-at-pos) (min last-line max-line))
-	  (notmuch-search-set-tags
-	   (notmuch-update-tags (notmuch-search-get-tags) tag-changes))
-	  (forward-line))))))
+    (notmuch-search-do-results beg end pos
+      (notmuch-search-set-tags
+       (notmuch-update-tags (notmuch-search-get-tags pos) tag-changes)
+       pos))))
 
 (defun notmuch-search-tag (&optional tag-changes)
   "Change tags for the currently selected thread or region.
-- 
1.7.10

[PATCH v2 5/7] emacs: Replace other search text properties with result property

[Austin Clements]

Since the result object contains everything that the other text
properties recorded, we can remove the other text properties and
simply look in the plist of the appropriate result object.
---
 emacs/notmuch.el |   24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 92ba2c1..9e061b6 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -452,16 +452,18 @@ current result."
 (defun notmuch-search-properties-in-region (property beg end)
   (let (output)
     (notmuch-search-do-results beg end pos
-      (push (get-text-property pos property) output))
+      (push (plist-get (notmuch-search-get-result pos) property) output))
     output))
 
 (defun notmuch-search-find-thread-id ()
   "Return the thread for the current thread"
-  (get-text-property (point) 'notmuch-search-thread-id))
+  (let ((thread (plist-get (notmuch-search-get-result) :thread)))
+    (when thread (concat "thread:" thread))))
 
 (defun notmuch-search-find-thread-id-region (beg end)
   "Return a list of threads for the current region"
-  (notmuch-search-properties-in-region 'notmuch-search-thread-id beg end))
+  (mapcar (lambda (thread) (concat "thread:" thread))
+	  (notmuch-search-properties-in-region :thread beg end)))
 
 (defun notmuch-search-find-thread-id-region-search (beg end)
   "Return a search string for threads for the current region"
@@ -469,19 +471,19 @@ current result."
 
 (defun notmuch-search-find-authors ()
   "Return the authors for the current thread"
-  (get-text-property (point) 'notmuch-search-authors))
+  (plist-get (notmuch-search-get-result) :authors))
 
 (defun notmuch-search-find-authors-region (beg end)
   "Return a list of authors for the current region"
-  (notmuch-search-properties-in-region 'notmuch-search-authors beg end))
+  (notmuch-search-properties-in-region :authors beg end))
 
 (defun notmuch-search-find-subject ()
   "Return the subject for the current thread"
-  (get-text-property (point) 'notmuch-search-subject))
+  (plist-get (notmuch-search-get-result) :subject))
 
 (defun notmuch-search-find-subject-region (beg end)
   "Return a list of authors for the current region"
-  (notmuch-search-properties-in-region 'notmuch-search-subject beg end))
+  (notmuch-search-properties-in-region :subject beg end))
 
 (defun notmuch-search-show-thread ()
   "Display the currently selected thread."
@@ -774,13 +776,7 @@ non-authors is found, assume that all of the authors match."
 	  (notmuch-search-insert-field (car spec) (cdr spec) result))
 	(insert "\n")
 	(notmuch-search-color-line beg (point) (plist-get result :tags))
-	(put-text-property beg (point) 'notmuch-search-result result)
-	(put-text-property beg (point) 'notmuch-search-thread-id
-			   (concat "thread:" (plist-get result :thread)))
-	(put-text-property beg (point) 'notmuch-search-authors
-			   (plist-get result :authors))
-	(put-text-property beg (point) 'notmuch-search-subject
-			   (plist-get result :subject)))
+	(put-text-property beg (point) 'notmuch-search-result result))
       (when (string= (plist-get result :thread) notmuch-search-target-thread)
 	(setq notmuch-search-target-thread "found")
 	(goto-char beg)))))
-- 
1.7.10

[PATCH v2 6/7] emacs: Allow custom tags formatting

[Austin Clements]

Previously we ignored any notmuch-search-result-format customizations
for tag formatting because we needed to be able to parse back in the
result line and update the tags in place.  We no longer do either of
these things, so we can allow customization of this format.

(Coincidentally, previously we still allowed too much customization of
the tags format, since moving it earlier on the line or removing it
from the line would interfere with the tagging mechanism.  There is
now no problem with doing such things.)
---
 emacs/notmuch.el |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 9e061b6..d0a8021 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -759,11 +759,9 @@ non-authors is found, assume that all of the authors match."
     (notmuch-search-insert-authors format-string (plist-get result :authors)))
 
    ((string-equal field "tags")
-    ;; Ignore format-string here because notmuch-search-set-tags
-    ;; depends on the format of this
-    (insert (concat "(" (propertize
-			 (mapconcat 'identity (plist-get result :tags) " ")
-			 'font-lock-face 'notmuch-tag-face) ")")))))
+    (let ((tags-str (mapconcat 'identity (plist-get result :tags) " ")))
+      (insert (propertize (format format-string tags-str)
+			  'face 'notmuch-tag-face))))))
 
 (defun notmuch-search-show-result (result &optional pos)
   "Insert RESULT at POS or the end of the buffer if POS is null."
-- 
1.7.10

[PATCH v2 7/7] emacs: Fix navigation of multi-line search result formats

[Austin Clements]

At this point, the only remaining functions that don't support
multi-line search result formats are the thread navigation functions.
This patch fixes that by rewriting them in terms of
notmuch-search-result-{beginning,end}.

This changes the behavior of notmuch-search-previous-thread slightly
so that if point isn't at the beginning of a result, it first moves
point to the beginning of the result.
---
 emacs/notmuch.el |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index d0a8021..7302fa7 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -287,18 +287,24 @@ For a mouse binding, return nil."
 (defun notmuch-search-next-thread ()
   "Select the next thread in the search results."
   (interactive)
-  (forward-line 1))
+  (when (notmuch-search-get-result (notmuch-search-result-end))
+    (goto-char (notmuch-search-result-end))))
 
 (defun notmuch-search-previous-thread ()
   "Select the previous thread in the search results."
   (interactive)
-  (forward-line -1))
+  (if (notmuch-search-get-result)
+      (unless (bobp)
+	(goto-char (notmuch-search-result-beginning (- (point) 1))))
+    ;; We must be past the end; jump to the last result
+    (notmuch-search-last-thread)))
 
 (defun notmuch-search-last-thread ()
   "Select the last thread in the search results."
   (interactive)
   (goto-char (point-max))
-  (forward-line -2))
+  (forward-line -2)
+  (goto-char (notmuch-search-result-beginning)))
 
 (defun notmuch-search-first-thread ()
   "Select the first thread in the search results."
-- 
1.7.10

Re: [PATCH v2 0/7] emacs: JSON-based search cleanups

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 739 bytes --]

On Fri, Jul 13 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> This version updates the docstrings for notmuch-search-update-result
> and notmuch-search-show-result and fixes the scrolling artifact in
> notmuch-search-update-result.  I also updated the commit message of
> patch 7 to mention that it changes the behavior of
> notmuch-search-previous-thread.

Hey, Austin.  This is a really nice series.  Very nice cleanup, with a
lot of great side effects.  It definitely looks good to me, although
like Mark, I don't quite grok the defmacro in patch 4.  But this is only
my ignorance and not a comment on the patch at all.  I've also tested
and the series seems to work great.  I want to try some multi-line
search results now!

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH v2 4/7] emacs: Use result text properties for search result iteration

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 1910 bytes --]

On Fri, Jul 13 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> Coincidentally, this also essentially enables multi-line search result
> formats; the only remaining non-multi-line-capable functions are
> notmuch-search-{next,previous}-thread, which are only used
> interactively.

So I tried to experiment with multi-line search results with this patch
series, but it didn't work.  I tried adding a '\n' in one of the
formatter fields, but I got a series of errors for each result insertion
in the buffer.  What's the way to get multi-line to work?

> +(defmacro notmuch-search-do-results (beg end pos-sym &rest body)
> +  "Invoke BODY for each result between BEG and END.
> +
> +POS-SYM will be bound to the point at the beginning of the
> +current result."
> +  (declare (indent 3))
> +  (let ((end-sym (make-symbol "end"))
> +	(first-sym (make-symbol "first")))
> +    `(let ((,pos-sym (notmuch-search-result-beginning ,beg))
> +	   ;; End must be a marker in case body changes the text
> +	   (,end-sym (copy-marker ,end))
> +	   ;; Make sure we examine one result, even if (= beg end)
> +	   (,first-sym t))
> +       ;; We have to be careful if the region extends beyond the
> +       ;; results.  In this case, pos could be null or there could be
> +       ;; no result at pos.
> +       (while (and ,pos-sym (or (< ,pos-sym ,end-sym) ,first-sym))
> +	 (when (notmuch-search-get-result ,pos-sym)
> +	   ,@body)
> +	 (setq ,pos-sym (notmuch-search-result-end ,pos-sym)
> +	       ,first-sym nil)))))

Austin, can you explain why you use a defmacro here?  I'm honestly have
a hard time parsing what's going on here.  I understand in principle how
elisp macros work, but I don't see why it's needed here.

I'm also trying to understand what the commas are doing
(e.g. ",pos-sym").  Are they doing some sort of escaping?  

Some sophisticated elisp here!

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH v2 4/7] emacs: Use result text properties for search result iteration

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 175 bytes --]

> Some sophisticated elisp here!

Ah, Mark just pointed me to this message that has a good explanation of
some of these concepts:

id:"20120117230255.GW16740@mit.edu"

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH v2 4/7] emacs: Use result text properties for search result iteration

[Austin Clements]

Quoth Jameson Graef Rollins on Jul 14 at 12:31 pm:
> On Fri, Jul 13 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> > Coincidentally, this also essentially enables multi-line search result
> > formats; the only remaining non-multi-line-capable functions are
> > notmuch-search-{next,previous}-thread, which are only used
> > interactively.
> 
> So I tried to experiment with multi-line search results with this patch
> series, but it didn't work.  I tried adding a '\n' in one of the
> formatter fields, but I got a series of errors for each result insertion
> in the buffer.  What's the way to get multi-line to work?

That's strange.  What was the error?

I've been testing with
   (("date" . "%12s ")
    ("count" . "%-7s ")
    ("authors" . "%-20s ")
    ("subject" . "\n%s ")
    ("tags" . "(%s)"))
But maybe there are other cases it doesn't handle correctly?

> > +(defmacro notmuch-search-do-results (beg end pos-sym &rest body)
> > +  "Invoke BODY for each result between BEG and END.
> > +
> > +POS-SYM will be bound to the point at the beginning of the
> > +current result."
> > +  (declare (indent 3))
> > +  (let ((end-sym (make-symbol "end"))
> > +	(first-sym (make-symbol "first")))
> > +    `(let ((,pos-sym (notmuch-search-result-beginning ,beg))
> > +	   ;; End must be a marker in case body changes the text
> > +	   (,end-sym (copy-marker ,end))
> > +	   ;; Make sure we examine one result, even if (= beg end)
> > +	   (,first-sym t))
> > +       ;; We have to be careful if the region extends beyond the
> > +       ;; results.  In this case, pos could be null or there could be
> > +       ;; no result at pos.
> > +       (while (and ,pos-sym (or (< ,pos-sym ,end-sym) ,first-sym))
> > +	 (when (notmuch-search-get-result ,pos-sym)
> > +	   ,@body)
> > +	 (setq ,pos-sym (notmuch-search-result-end ,pos-sym)
> > +	       ,first-sym nil)))))
> 
> Austin, can you explain why you use a defmacro here?  I'm honestly have
> a hard time parsing what's going on here.  I understand in principle how
> elisp macros work, but I don't see why it's needed here.
> 
> I'm also trying to understand what the commas are doing
> (e.g. ",pos-sym").  Are they doing some sort of escaping?  
> 
> Some sophisticated elisp here!

I did this as a macro to parallel things like dolist and loop, I'll
try this out with a higher-order procedure and see if the results are
less opaque.

Re: [PATCH v2 4/7] emacs: Use result text properties for search result iteration

[Austin Clements]

Quoth myself on Jul 14 at  3:50 pm:
> Quoth Jameson Graef Rollins on Jul 14 at 12:31 pm:
> > On Fri, Jul 13 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> > > +(defmacro notmuch-search-do-results (beg end pos-sym &rest body)
> > > +  "Invoke BODY for each result between BEG and END.
> > > +
> > > +POS-SYM will be bound to the point at the beginning of the
> > > +current result."
> > > +  (declare (indent 3))
> > > +  (let ((end-sym (make-symbol "end"))
> > > +	(first-sym (make-symbol "first")))
> > > +    `(let ((,pos-sym (notmuch-search-result-beginning ,beg))
> > > +	   ;; End must be a marker in case body changes the text
> > > +	   (,end-sym (copy-marker ,end))
> > > +	   ;; Make sure we examine one result, even if (= beg end)
> > > +	   (,first-sym t))
> > > +       ;; We have to be careful if the region extends beyond the
> > > +       ;; results.  In this case, pos could be null or there could be
> > > +       ;; no result at pos.
> > > +       (while (and ,pos-sym (or (< ,pos-sym ,end-sym) ,first-sym))
> > > +	 (when (notmuch-search-get-result ,pos-sym)
> > > +	   ,@body)
> > > +	 (setq ,pos-sym (notmuch-search-result-end ,pos-sym)
> > > +	       ,first-sym nil)))))
> > 
> > Austin, can you explain why you use a defmacro here?  I'm honestly have
> > a hard time parsing what's going on here.  I understand in principle how
> > elisp macros work, but I don't see why it's needed here.
> > 
> > I'm also trying to understand what the commas are doing
> > (e.g. ",pos-sym").  Are they doing some sort of escaping?  
> > 
> > Some sophisticated elisp here!
> 
> I did this as a macro to parallel things like dolist and loop, I'll
> try this out with a higher-order procedure and see if the results are
> less opaque.

Here's what it looks like as a higher-order procedure and an example
use:

(defun notmuch-search-foreach-result (beg end function)
  "Invoke FUNCTION with the position of each result between BEG and END."

  (lexical-let ((pos (notmuch-search-result-beginning beg))
		;; End must be a marker in case function changes the
		;; text.
		(end (copy-marker end))
		;; Make sure we examine at least one result, even if
		;; (= beg end).
		(first t))
    ;; We have to be careful if the region extends beyond the results.
    ;; In this case, pos could be null or there could be no result at
    ;; pos.
    (while (and pos (or (< pos end) first))
      (when (notmuch-search-get-result pos)
	(apply function pos))
      (setq pos (notmuch-search-result-end pos)
	    first nil))))

(defun notmuch-search-properties-in-region (property beg end)
  (let (output)
    (notmuch-search-foreach-result
     beg end
     (lambda (pos)
       (push (plist-get (notmuch-search-get-result pos) property) output)))
    output))

Re: [PATCH v2 4/7] emacs: Use result text properties for search result iteration

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 1336 bytes --]

On Sat, Jul 14 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> That's strange.  What was the error?
>
> I've been testing with
>    (("date" . "%12s ")
>     ("count" . "%-7s ")
>     ("authors" . "%-20s ")
>     ("subject" . "\n%s ")
>     ("tags" . "(%s)"))
> But maybe there are other cases it doesn't handle correctly?

Hrm.  The error was:

  error in process filter: wrong type argument: wholenump, -13

However, that's in my test emacs setup.  It works fine in my normal
emacs session.  Lets assume this is just a problem with my setup that
I'll look into.

The other problem I seem to be running into is that the
customize-variable interface definitely doesn't handle newlines very
well.  If I try to add a '\n' in the interface it gets escaped, so I end
up with:

     ("subject" . "\\n%s ")

If I add the '\n' manually in my config, notmuch-search then interprets
the string correctly and adds newlines to the search results, but then
the customize-variable interface interprets the newline and adds a
newline the string field that kind makes the interface a little weird.
Is there "proper" way to add a newline to a string value in the
customize-variable interface that I'm not aware of?

In any event, this is in no way a blocker for this patch series.  This
is all totally tangential.  The patch series is great.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH v2 4/7] emacs: Use result text properties for search result iteration

[Mark Walters]

On Sat, 14 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> This simplifies the traversal of regions of results and eliminates the
> need for save-excursions (which tend to get in the way of maintaining
> point when we make changes to the buffer).  It also fixes some strange
> corner cases in the old line-based code where results that bordered
> the region but were not included in it could be affected by region
> commands.  Coincidentally, this also essentially enables multi-line
> search result formats; the only remaining non-multi-line-capable
> functions are notmuch-search-{next,previous}-thread, which are only
> used interactively.

Having understood how macros work (thanks to Austin's patient
explanations on irc) this looks fine. So +1 for the whole series.

Best wishes

Mark
> ---
>  emacs/notmuch.el |   61 ++++++++++++++++++++++++++++++------------------------
>  1 file changed, 34 insertions(+), 27 deletions(-)
>
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 8e1a769..92ba2c1 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -427,17 +427,33 @@ returns nil"
>      (next-single-property-change (or pos (point)) 'notmuch-search-result
>  				 nil (point-max))))
>  
> +(defmacro notmuch-search-do-results (beg end pos-sym &rest body)
> +  "Invoke BODY for each result between BEG and END.
> +
> +POS-SYM will be bound to the point at the beginning of the
> +current result."
> +  (declare (indent 3))
> +  (let ((end-sym (make-symbol "end"))
> +	(first-sym (make-symbol "first")))
> +    `(let ((,pos-sym (notmuch-search-result-beginning ,beg))
> +	   ;; End must be a marker in case body changes the text
> +	   (,end-sym (copy-marker ,end))
> +	   ;; Make sure we examine one result, even if (= beg end)
> +	   (,first-sym t))
> +       ;; We have to be careful if the region extends beyond the
> +       ;; results.  In this case, pos could be null or there could be
> +       ;; no result at pos.
> +       (while (and ,pos-sym (or (< ,pos-sym ,end-sym) ,first-sym))
> +	 (when (notmuch-search-get-result ,pos-sym)
> +	   ,@body)
> +	 (setq ,pos-sym (notmuch-search-result-end ,pos-sym)
> +	       ,first-sym nil)))))
> +
>  (defun notmuch-search-properties-in-region (property beg end)
> -  (save-excursion
> -    (let ((output nil)
> -	  (last-line (line-number-at-pos end))
> -	  (max-line (- (line-number-at-pos (point-max)) 2)))
> -      (goto-char beg)
> -      (beginning-of-line)
> -      (while (<= (line-number-at-pos) (min last-line max-line))
> -	(setq output (cons (get-text-property (point) property) output))
> -	(forward-line 1))
> -      output)))
> +  (let (output)
> +    (notmuch-search-do-results beg end pos
> +      (push (get-text-property pos property) output))
> +    output))
>  
>  (defun notmuch-search-find-thread-id ()
>    "Return the thread for the current thread"
> @@ -517,28 +533,19 @@ and will also appear in a buffer named \"*Notmuch errors*\"."
>    (plist-get (notmuch-search-get-result pos) :tags))
>  
>  (defun notmuch-search-get-tags-region (beg end)
> -  (save-excursion
> -    (let ((output nil)
> -	  (last-line (line-number-at-pos end))
> -	  (max-line (- (line-number-at-pos (point-max)) 2)))
> -      (goto-char beg)
> -      (while (<= (line-number-at-pos) (min last-line max-line))
> -	(setq output (append output (notmuch-search-get-tags)))
> -	(forward-line 1))
> -      output)))
> +  (let (output)
> +    (notmuch-search-do-results beg end pos
> +      (setq output (append output (notmuch-search-get-tags pos))))
> +    output))
>  
>  (defun notmuch-search-tag-region (beg end &optional tag-changes)
>    "Change tags for threads in the given region."
>    (let ((search-string (notmuch-search-find-thread-id-region-search beg end)))
>      (setq tag-changes (funcall 'notmuch-tag search-string tag-changes))
> -    (save-excursion
> -      (let ((last-line (line-number-at-pos end))
> -	    (max-line (- (line-number-at-pos (point-max)) 2)))
> -	(goto-char beg)
> -	(while (<= (line-number-at-pos) (min last-line max-line))
> -	  (notmuch-search-set-tags
> -	   (notmuch-update-tags (notmuch-search-get-tags) tag-changes))
> -	  (forward-line))))))
> +    (notmuch-search-do-results beg end pos
> +      (notmuch-search-set-tags
> +       (notmuch-update-tags (notmuch-search-get-tags pos) tag-changes)
> +       pos))))
>  
>  (defun notmuch-search-tag (&optional tag-changes)
>    "Change tags for the currently selected thread or region.
> -- 
> 1.7.10

Re: [PATCH v2 4/7] emacs: Use result text properties for search result iteration

[Austin Clements]

Quoth Jameson Graef Rollins on Jul 14 at  2:13 pm:
> On Sat, Jul 14 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> > That's strange.  What was the error?
> >
> > I've been testing with
> >    (("date" . "%12s ")
> >     ("count" . "%-7s ")
> >     ("authors" . "%-20s ")
> >     ("subject" . "\n%s ")
> >     ("tags" . "(%s)"))
> > But maybe there are other cases it doesn't handle correctly?
> 
> Hrm.  The error was:
> 
>   error in process filter: wrong type argument: wholenump, -13
> 
> However, that's in my test emacs setup.  It works fine in my normal
> emacs session.  Lets assume this is just a problem with my setup that
> I'll look into.

Interesting.  The "error in process filter" suggests that it is
happening somewhere in the notmuch code under the search process
filter, so it's probably some combination of something unusual in your
environment and this patch.  If you M-x toggle-debug-on-error, will it
drop you into the debugger when this happens?

> The other problem I seem to be running into is that the
> customize-variable interface definitely doesn't handle newlines very
> well.  If I try to add a '\n' in the interface it gets escaped, so I end
> up with:
> 
>      ("subject" . "\\n%s ")
> 
> If I add the '\n' manually in my config, notmuch-search then interprets
> the string correctly and adds newlines to the search results, but then
> the customize-variable interface interprets the newline and adds a
> newline the string field that kind makes the interface a little weird.
> Is there "proper" way to add a newline to a string value in the
> customize-variable interface that I'm not aware of?

I think this is just a sharp corner in customize and there's notmuch
we can do about it.  I believe the "proper" way to enter the newline
directly in customize is to hit C-q C-j (same way you get a raw
newline in any other buffer).  Given that I wouldn't expect anyone to
guess that, though, we could mention this in the defcustom's
docstring.

> In any event, this is in no way a blocker for this patch series.  This
> is all totally tangential.  The patch series is great.
> 
> jamie.

[PATCH v3 0/8] emacs: JSON-based search cleanups

[Austin Clements]

This version swaps out the notmuch-search-do-results macro for a
higher-order function, notmuch-search-foreach-result.  This requires
less squiting to understand and clearly distinguishes the arguments
passed in to the function from the arguments passed to the callback.
This version also updates the docstring for
notmuch-search-result-format to mention that multi-line result formats
work and how to enter them, and it adds a NEWS patch.

Diff from v2:

diff --git a/NEWS b/NEWS
index a1a6e93..7b33f0d 100644
--- a/NEWS
+++ b/NEWS
@@ -17,6 +17,20 @@ Maildir tag synchronization
 Emacs Interface
 ---------------
 
+Search results now get re-colored when tags are updated
+
+The formatting of tags in search results can now be customized
+
+  Previously, attempting to change the format of tags in
+  `notmuch-search-result-format` would usually break tagging from
+  search-mode.  We no longer make assumptions about the format.
+
+Multi-line search result formats are now supported
+
+  It is now possible to embed newlines in
+  `notmuch-search-result-format` to make individual search results
+  span multiple lines.
+
 Search now uses the JSON format internally
 
   This should address problems with unusual characters in authors and
diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 7302fa7..ec760dc 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -69,7 +69,13 @@
 	date, count, authors, subject, tags
 For example:
 	(setq notmuch-search-result-format \(\(\"authors\" . \"%-40s\"\)
-					     \(\"subject\" . \"%s\"\)\)\)"
+					     \(\"subject\" . \"%s\"\)\)\)
+Line breaks are permitted in format strings.  Note that a line
+break at the end of an \"authors\" field will get elided if the
+authors list is long; place it instead at the beginning of the
+following field.  To enter a line break when setting this
+variable with setq, use \\n.  To enter a line break in customize,
+press \\[quoted-insert] C-j."
   :type '(alist :key-type (string) :value-type (string))
   :group 'notmuch-search)
 
@@ -433,32 +439,39 @@ returns nil"
     (next-single-property-change (or pos (point)) 'notmuch-search-result
 				 nil (point-max))))
 
-(defmacro notmuch-search-do-results (beg end pos-sym &rest body)
-  "Invoke BODY for each result between BEG and END.
-
-POS-SYM will be bound to the point at the beginning of the
-current result."
-  (declare (indent 3))
-  (let ((end-sym (make-symbol "end"))
-	(first-sym (make-symbol "first")))
-    `(let ((,pos-sym (notmuch-search-result-beginning ,beg))
-	   ;; End must be a marker in case body changes the text
-	   (,end-sym (copy-marker ,end))
-	   ;; Make sure we examine one result, even if (= beg end)
-	   (,first-sym t))
-       ;; We have to be careful if the region extends beyond the
-       ;; results.  In this case, pos could be null or there could be
-       ;; no result at pos.
-       (while (and ,pos-sym (or (< ,pos-sym ,end-sym) ,first-sym))
-	 (when (notmuch-search-get-result ,pos-sym)
-	   ,@body)
-	 (setq ,pos-sym (notmuch-search-result-end ,pos-sym)
-	       ,first-sym nil)))))
+(defun notmuch-search-foreach-result (beg end function)
+  "Invoke FUNCTION for each result between BEG and END.
+
+FUNCTION should take one argument.  It will be applied to the
+character position of the beginning of each result that overlaps
+the region between points BEG and END.  As a special case, if (=
+BEG END), FUNCTION will be applied to the result containing point
+BEG."
+
+  (lexical-let ((pos (notmuch-search-result-beginning beg))
+		;; End must be a marker in case function changes the
+		;; text.
+		(end (copy-marker end))
+		;; Make sure we examine at least one result, even if
+		;; (= beg end).
+		(first t))
+    ;; We have to be careful if the region extends beyond the results.
+    ;; In this case, pos could be null or there could be no result at
+    ;; pos.
+    (while (and pos (or (< pos end) first))
+      (when (notmuch-search-get-result pos)
+	(funcall function pos))
+      (setq pos (notmuch-search-result-end pos)
+	    first nil))))
+;; Unindent the function argument of notmuch-search-foreach-result so
+;; the indentation of callers doesn't get out of hand.
+(put 'notmuch-search-foreach-result 'lisp-indent-function 2)
 
 (defun notmuch-search-properties-in-region (property beg end)
   (let (output)
-    (notmuch-search-do-results beg end pos
-      (push (plist-get (notmuch-search-get-result pos) property) output))
+    (notmuch-search-foreach-result beg end
+      (lambda (pos)
+	(push (plist-get (notmuch-search-get-result pos) property) output)))
     output))
 
 (defun notmuch-search-find-thread-id ()
@@ -542,18 +555,20 @@ and will also appear in a buffer named \"*Notmuch errors*\"."
 
 (defun notmuch-search-get-tags-region (beg end)
   (let (output)
-    (notmuch-search-do-results beg end pos
-      (setq output (append output (notmuch-search-get-tags pos))))
+    (notmuch-search-foreach-result beg end
+      (lambda (pos)
+	(setq output (append output (notmuch-search-get-tags pos)))))
     output))
 
 (defun notmuch-search-tag-region (beg end &optional tag-changes)
   "Change tags for threads in the given region."
   (let ((search-string (notmuch-search-find-thread-id-region-search beg end)))
     (setq tag-changes (funcall 'notmuch-tag search-string tag-changes))
-    (notmuch-search-do-results beg end pos
-      (notmuch-search-set-tags
-       (notmuch-update-tags (notmuch-search-get-tags pos) tag-changes)
-       pos))))
+    (notmuch-search-foreach-result beg end
+      (lambda (pos)
+	(notmuch-search-set-tags
+	 (notmuch-update-tags (notmuch-search-get-tags pos) tag-changes)
+	 pos)))))
 
 (defun notmuch-search-tag (&optional tag-changes)
   "Change tags for the currently selected thread or region.

[PATCH v3 1/8] emacs: Record thread search result object in a text property

[Austin Clements]

This also provides utility functions for working with this text
property that get its value, find its start, and find its end.
---
 emacs/notmuch.el |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index fabb7c0..ef18927 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -401,6 +401,32 @@ Complete list of currently available key bindings:
 	mode-name "notmuch-search")
   (setq buffer-read-only t))
 
+(defun notmuch-search-get-result (&optional pos)
+  "Return the result object for the thread at POS (or point).
+
+If there is no thread at POS (or point), returns nil."
+  (get-text-property (or pos (point)) 'notmuch-search-result))
+
+(defun notmuch-search-result-beginning (&optional pos)
+  "Return the point at the beginning of the thread at POS (or point).
+
+If there is no thread at POS (or point), returns nil."
+  (when (notmuch-search-get-result pos)
+    ;; We pass 1+point because previous-single-property-change starts
+    ;; searching one before the position we give it.
+    (previous-single-property-change (1+ (or pos (point)))
+				     'notmuch-search-result nil (point-min))))
+
+(defun notmuch-search-result-end (&optional pos)
+  "Return the point at the end of the thread at POS (or point).
+
+The returned point will be just after the newline character that
+ends the result line.  If there is no thread at POS (or point),
+returns nil"
+  (when (notmuch-search-get-result pos)
+    (next-single-property-change (or pos (point)) 'notmuch-search-result
+				 nil (point-max))))
+
 (defun notmuch-search-properties-in-region (property beg end)
   (save-excursion
     (let ((output nil)
@@ -736,6 +762,7 @@ non-authors is found, assume that all of the authors match."
 	  (notmuch-search-insert-field (car spec) (cdr spec) result))
 	(insert "\n")
 	(notmuch-search-color-line beg (point) (plist-get result :tags))
+	(put-text-property beg (point) 'notmuch-search-result result)
 	(put-text-property beg (point) 'notmuch-search-thread-id
 			   (concat "thread:" (plist-get result :thread)))
 	(put-text-property beg (point) 'notmuch-search-authors
-- 
1.7.10

[PATCH v3 2/8] emacs: Use text properties instead of overlays for tag coloring

[Austin Clements]

Previously, tag-based search result highlighting was done by creating
an overlay over each search result.  However, overlays have annoying
front- and rear-advancement semantics that make it difficult to
manipulate text at their boundaries, which the next patch will do.
They also have performance problems (creating an overlay is linear in
the number of overlays between point and the new overlay, making
highlighting a search buffer quadratic in the number of results).

Text properties have neither problem.  However, text properties make
it more difficult to apply multiple faces since, unlike with overlays,
a given character can only have a single 'face text property.  Hence,
we introduce a utility function that combines faces into any existing
'face text properties.

Using this utility function, it's straightforward to apply all of the
appropriate tag faces in notmuch-search-color-line.
---
 emacs/notmuch-lib.el |   15 +++++++++++++++
 emacs/notmuch.el     |   21 +++++++--------------
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index aa25513..30db58f 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -269,6 +269,21 @@ current buffer, if possible."
   (loop for (key value . rest) on plist by #'cddr
 	collect (cons (intern (substring (symbol-name key) 1)) value)))
 
+(defun notmuch-combine-face-text-property (start end face)
+  "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."
+
+  (let ((pos start))
+    (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))
+	(setq pos next)))))
+
 ;; Compatibility functions for versions of emacs before emacs 23.
 ;;
 ;; Both functions here were copied from emacs 23 with the following copyright:
diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index ef18927..82c148d 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -633,20 +633,13 @@ foreground and blue background."
 
 (defun notmuch-search-color-line (start end line-tag-list)
   "Colorize lines in `notmuch-show' based on tags."
-  ;; Create the overlay only if the message has tags which match one
-  ;; of those specified in `notmuch-search-line-faces'.
-  (let (overlay)
-    (mapc (lambda (elem)
-	    (let ((tag (car elem))
-		  (attributes (cdr elem)))
-	      (when (member tag line-tag-list)
-		(when (not overlay)
-		  (setq overlay (make-overlay start end)))
-		;; Merge the specified properties with any already
-		;; applied from an earlier match.
-		(overlay-put overlay 'face
-			     (append (overlay-get overlay 'face) attributes)))))
-	  notmuch-search-line-faces)))
+  (mapc (lambda (elem)
+	  (let ((tag (car elem))
+		(attributes (cdr elem)))
+	    (when (member tag line-tag-list)
+	      (notmuch-combine-face-text-property start end attributes))))
+	;; Reverse the list so earlier entries take precedence
+	(reverse notmuch-search-line-faces)))
 
 (defun notmuch-search-author-propertize (authors)
   "Split `authors' into matching and non-matching authors and
-- 
1.7.10

[PATCH v3 3/8] emacs: Update tags by rewriting the search result line in place

[Austin Clements]

Now that we keep the full thread result object, we can refresh a
result after any changes by simply deleting and reconstructing the
result line from scratch.

A convenient side-effect of this wholesale replacement is that search
now re-applies notmuch-search-line-faces when tags change.
---
 emacs/notmuch.el |   60 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 36 insertions(+), 24 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 82c148d..8e1a769 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -509,28 +509,12 @@ and will also appear in a buffer named \"*Notmuch errors*\"."
 	    (error (buffer-substring beg end))
 	    ))))))
 
-(defun notmuch-search-set-tags (tags)
-  (save-excursion
-    (end-of-line)
-    (re-search-backward "(")
-    (forward-char)
-    (let ((beg (point))
-	  (inhibit-read-only t))
-      (re-search-forward ")")
-      (backward-char)
-      (let ((end (point)))
-	(delete-region beg end)
-	(insert (propertize (mapconcat  'identity tags " ")
-			    'face 'notmuch-tag-face))))))
-
-(defun notmuch-search-get-tags ()
-  (save-excursion
-    (end-of-line)
-    (re-search-backward "(")
-    (let ((beg (+ (point) 1)))
-      (re-search-forward ")")
-      (let ((end (- (point) 1)))
-	(split-string (buffer-substring-no-properties beg end))))))
+(defun notmuch-search-set-tags (tags &optional pos)
+  (let ((new-result (plist-put (notmuch-search-get-result pos) :tags tags)))
+    (notmuch-search-update-result new-result pos)))
+
+(defun notmuch-search-get-tags (&optional pos)
+  (plist-get (notmuch-search-get-result pos) :tags))
 
 (defun notmuch-search-get-tags-region (beg end)
   (save-excursion
@@ -583,6 +567,33 @@ This function advances the next thread when finished."
   (notmuch-search-tag '("-inbox"))
   (notmuch-search-next-thread))
 
+(defun notmuch-search-update-result (result &optional pos)
+  "Replace the result object of the thread at POS (or point) by
+RESULT and redraw it."
+  (let ((start (notmuch-search-result-beginning pos))
+	(end (notmuch-search-result-end pos))
+	(init-point (point))
+	(init-start (window-start))
+	(inhibit-read-only t))
+    ;; Delete the current thread
+    (delete-region start end)
+    ;; Insert the updated thread
+    (notmuch-search-show-result result start)
+    ;; There may have been markers pointing into the text we just
+    ;; replaced.  For the most part, there's nothing we can do about
+    ;; this, but we can fix markers that were at point (which includes
+    ;; point itself and any save-excursions for which point hasn't
+    ;; moved) by re-inserting the text that should come before point
+    ;; before markers.
+    (when (and (>= init-point start) (<= init-point end))
+      (let* ((new-end (notmuch-search-result-end start))
+	     (new-point (if (= init-point end)
+			    new-end
+			  (min init-point (- new-end 1)))))
+	(insert-before-markers (delete-and-extract-region start new-point))))
+    ;; We also may have shifted the window scroll.  Fix it.
+    (set-window-start (selected-window) init-start)))
+
 (defun notmuch-search-process-sentinel (proc msg)
   "Add a message to let user know when \"notmuch search\" exits"
   (let ((buffer (process-buffer proc))
@@ -745,10 +756,11 @@ non-authors is found, assume that all of the authors match."
 			 (mapconcat 'identity (plist-get result :tags) " ")
 			 'font-lock-face 'notmuch-tag-face) ")")))))
 
-(defun notmuch-search-show-result (result)
+(defun notmuch-search-show-result (result &optional pos)
+  "Insert RESULT at POS or the end of the buffer if POS is null."
   ;; Ignore excluded matches
   (unless (= (plist-get result :matched) 0)
-    (let ((beg (point-max)))
+    (let ((beg (or pos (point-max))))
       (save-excursion
 	(goto-char beg)
 	(dolist (spec notmuch-search-result-format)
-- 
1.7.10

[PATCH v3 4/8] emacs: Use result text properties for search result iteration

[Austin Clements]

This simplifies the traversal of regions of results and eliminates the
need for save-excursions (which tend to get in the way of maintaining
point when we make changes to the buffer).  It also fixes some strange
corner cases in the old line-based code where results that bordered
the region but were not included in it could be affected by region
commands.  Coincidentally, this also essentially enables multi-line
search result formats; the only remaining non-multi-line-capable
functions are notmuch-search-{next,previous}-thread, which are only
used for interactive navigation.
---
 emacs/notmuch.el |   78 ++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 50 insertions(+), 28 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 8e1a769..49367e0 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -69,7 +69,13 @@
 	date, count, authors, subject, tags
 For example:
 	(setq notmuch-search-result-format \(\(\"authors\" . \"%-40s\"\)
-					     \(\"subject\" . \"%s\"\)\)\)"
+					     \(\"subject\" . \"%s\"\)\)\)
+Line breaks are permitted in format strings.  Note that a line
+break at the end of an \"authors\" field will get elided if the
+authors list is long; place it instead at the beginning of the
+following field.  To enter a line break when setting this
+variable with setq, use \\n.  To enter a line break in customize,
+press \\[quoted-insert] C-j."
   :type '(alist :key-type (string) :value-type (string))
   :group 'notmuch-search)
 
@@ -427,17 +433,40 @@ returns nil"
     (next-single-property-change (or pos (point)) 'notmuch-search-result
 				 nil (point-max))))
 
+(defun notmuch-search-foreach-result (beg end function)
+  "Invoke FUNCTION for each result between BEG and END.
+
+FUNCTION should take one argument.  It will be applied to the
+character position of the beginning of each result that overlaps
+the region between points BEG and END.  As a special case, if (=
+BEG END), FUNCTION will be applied to the result containing point
+BEG."
+
+  (lexical-let ((pos (notmuch-search-result-beginning beg))
+		;; End must be a marker in case function changes the
+		;; text.
+		(end (copy-marker end))
+		;; Make sure we examine at least one result, even if
+		;; (= beg end).
+		(first t))
+    ;; We have to be careful if the region extends beyond the results.
+    ;; In this case, pos could be null or there could be no result at
+    ;; pos.
+    (while (and pos (or (< pos end) first))
+      (when (notmuch-search-get-result pos)
+	(funcall function pos))
+      (setq pos (notmuch-search-result-end pos)
+	    first nil))))
+;; Unindent the function argument of notmuch-search-foreach-result so
+;; the indentation of callers doesn't get out of hand.
+(put 'notmuch-search-foreach-result 'lisp-indent-function 2)
+
 (defun notmuch-search-properties-in-region (property beg end)
-  (save-excursion
-    (let ((output nil)
-	  (last-line (line-number-at-pos end))
-	  (max-line (- (line-number-at-pos (point-max)) 2)))
-      (goto-char beg)
-      (beginning-of-line)
-      (while (<= (line-number-at-pos) (min last-line max-line))
-	(setq output (cons (get-text-property (point) property) output))
-	(forward-line 1))
-      output)))
+  (let (output)
+    (notmuch-search-foreach-result beg end
+      (lambda (pos)
+	(push (get-text-property pos property) output)))
+    output))
 
 (defun notmuch-search-find-thread-id ()
   "Return the thread for the current thread"
@@ -517,28 +546,21 @@ and will also appear in a buffer named \"*Notmuch errors*\"."
   (plist-get (notmuch-search-get-result pos) :tags))
 
 (defun notmuch-search-get-tags-region (beg end)
-  (save-excursion
-    (let ((output nil)
-	  (last-line (line-number-at-pos end))
-	  (max-line (- (line-number-at-pos (point-max)) 2)))
-      (goto-char beg)
-      (while (<= (line-number-at-pos) (min last-line max-line))
-	(setq output (append output (notmuch-search-get-tags)))
-	(forward-line 1))
-      output)))
+  (let (output)
+    (notmuch-search-foreach-result beg end
+      (lambda (pos)
+	(setq output (append output (notmuch-search-get-tags pos)))))
+    output))
 
 (defun notmuch-search-tag-region (beg end &optional tag-changes)
   "Change tags for threads in the given region."
   (let ((search-string (notmuch-search-find-thread-id-region-search beg end)))
     (setq tag-changes (funcall 'notmuch-tag search-string tag-changes))
-    (save-excursion
-      (let ((last-line (line-number-at-pos end))
-	    (max-line (- (line-number-at-pos (point-max)) 2)))
-	(goto-char beg)
-	(while (<= (line-number-at-pos) (min last-line max-line))
-	  (notmuch-search-set-tags
-	   (notmuch-update-tags (notmuch-search-get-tags) tag-changes))
-	  (forward-line))))))
+    (notmuch-search-foreach-result beg end
+      (lambda (pos)
+	(notmuch-search-set-tags
+	 (notmuch-update-tags (notmuch-search-get-tags pos) tag-changes)
+	 pos)))))
 
 (defun notmuch-search-tag (&optional tag-changes)
   "Change tags for the currently selected thread or region.
-- 
1.7.10

[PATCH v3 5/8] emacs: Replace other search text properties with result property

[Austin Clements]

Since the result object contains everything that the other text
properties recorded, we can remove the other text properties and
simply look in the plist of the appropriate result object.
---
 emacs/notmuch.el |   24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 49367e0..8590529 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -465,16 +465,18 @@ BEG."
   (let (output)
     (notmuch-search-foreach-result beg end
       (lambda (pos)
-	(push (get-text-property pos property) output)))
+	(push (plist-get (notmuch-search-get-result pos) property) output)))
     output))
 
 (defun notmuch-search-find-thread-id ()
   "Return the thread for the current thread"
-  (get-text-property (point) 'notmuch-search-thread-id))
+  (let ((thread (plist-get (notmuch-search-get-result) :thread)))
+    (when thread (concat "thread:" thread))))
 
 (defun notmuch-search-find-thread-id-region (beg end)
   "Return a list of threads for the current region"
-  (notmuch-search-properties-in-region 'notmuch-search-thread-id beg end))
+  (mapcar (lambda (thread) (concat "thread:" thread))
+	  (notmuch-search-properties-in-region :thread beg end)))
 
 (defun notmuch-search-find-thread-id-region-search (beg end)
   "Return a search string for threads for the current region"
@@ -482,19 +484,19 @@ BEG."
 
 (defun notmuch-search-find-authors ()
   "Return the authors for the current thread"
-  (get-text-property (point) 'notmuch-search-authors))
+  (plist-get (notmuch-search-get-result) :authors))
 
 (defun notmuch-search-find-authors-region (beg end)
   "Return a list of authors for the current region"
-  (notmuch-search-properties-in-region 'notmuch-search-authors beg end))
+  (notmuch-search-properties-in-region :authors beg end))
 
 (defun notmuch-search-find-subject ()
   "Return the subject for the current thread"
-  (get-text-property (point) 'notmuch-search-subject))
+  (plist-get (notmuch-search-get-result) :subject))
 
 (defun notmuch-search-find-subject-region (beg end)
   "Return a list of authors for the current region"
-  (notmuch-search-properties-in-region 'notmuch-search-subject beg end))
+  (notmuch-search-properties-in-region :subject beg end))
 
 (defun notmuch-search-show-thread ()
   "Display the currently selected thread."
@@ -789,13 +791,7 @@ non-authors is found, assume that all of the authors match."
 	  (notmuch-search-insert-field (car spec) (cdr spec) result))
 	(insert "\n")
 	(notmuch-search-color-line beg (point) (plist-get result :tags))
-	(put-text-property beg (point) 'notmuch-search-result result)
-	(put-text-property beg (point) 'notmuch-search-thread-id
-			   (concat "thread:" (plist-get result :thread)))
-	(put-text-property beg (point) 'notmuch-search-authors
-			   (plist-get result :authors))
-	(put-text-property beg (point) 'notmuch-search-subject
-			   (plist-get result :subject)))
+	(put-text-property beg (point) 'notmuch-search-result result))
       (when (string= (plist-get result :thread) notmuch-search-target-thread)
 	(setq notmuch-search-target-thread "found")
 	(goto-char beg)))))
-- 
1.7.10

[PATCH v3 6/8] emacs: Allow custom tags formatting

[Austin Clements]

Previously we ignored any notmuch-search-result-format customizations
for tag formatting because we needed to be able to parse back in the
result line and update the tags in place.  We no longer do either of
these things, so we can allow customization of this format.

(Coincidentally, previously we still allowed too much customization of
the tags format, since moving it earlier on the line or removing it
from the line would interfere with the tagging mechanism.  There is
now no problem with doing such things.)
---
 emacs/notmuch.el |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 8590529..86f59e0 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -774,11 +774,9 @@ non-authors is found, assume that all of the authors match."
     (notmuch-search-insert-authors format-string (plist-get result :authors)))
 
    ((string-equal field "tags")
-    ;; Ignore format-string here because notmuch-search-set-tags
-    ;; depends on the format of this
-    (insert (concat "(" (propertize
-			 (mapconcat 'identity (plist-get result :tags) " ")
-			 'font-lock-face 'notmuch-tag-face) ")")))))
+    (let ((tags-str (mapconcat 'identity (plist-get result :tags) " ")))
+      (insert (propertize (format format-string tags-str)
+			  'face 'notmuch-tag-face))))))
 
 (defun notmuch-search-show-result (result &optional pos)
   "Insert RESULT at POS or the end of the buffer if POS is null."
-- 
1.7.10

[PATCH v3 7/8] emacs: Fix navigation of multi-line search result formats

[Austin Clements]

At this point, the only remaining functions that don't support
multi-line search result formats are the thread navigation functions.
This patch fixes that by rewriting them in terms of
notmuch-search-result-{beginning,end}.

This changes the behavior of notmuch-search-previous-thread slightly
so that if point isn't at the beginning of a result, it first moves
point to the beginning of the result.
---
 emacs/notmuch.el |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 86f59e0..ec760dc 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -293,18 +293,24 @@ For a mouse binding, return nil."
 (defun notmuch-search-next-thread ()
   "Select the next thread in the search results."
   (interactive)
-  (forward-line 1))
+  (when (notmuch-search-get-result (notmuch-search-result-end))
+    (goto-char (notmuch-search-result-end))))
 
 (defun notmuch-search-previous-thread ()
   "Select the previous thread in the search results."
   (interactive)
-  (forward-line -1))
+  (if (notmuch-search-get-result)
+      (unless (bobp)
+	(goto-char (notmuch-search-result-beginning (- (point) 1))))
+    ;; We must be past the end; jump to the last result
+    (notmuch-search-last-thread)))
 
 (defun notmuch-search-last-thread ()
   "Select the last thread in the search results."
   (interactive)
   (goto-char (point-max))
-  (forward-line -2))
+  (forward-line -2)
+  (goto-char (notmuch-search-result-beginning)))
 
 (defun notmuch-search-first-thread ()
   "Select the first thread in the search results."
-- 
1.7.10

[PATCH v3 8/8] News for search cleanups

[Austin Clements]

---
 NEWS |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/NEWS b/NEWS
index a1a6e93..7b33f0d 100644
--- a/NEWS
+++ b/NEWS
@@ -17,6 +17,20 @@ Maildir tag synchronization
 Emacs Interface
 ---------------
 
+Search results now get re-colored when tags are updated
+
+The formatting of tags in search results can now be customized
+
+  Previously, attempting to change the format of tags in
+  `notmuch-search-result-format` would usually break tagging from
+  search-mode.  We no longer make assumptions about the format.
+
+Multi-line search result formats are now supported
+
+  It is now possible to embed newlines in
+  `notmuch-search-result-format` to make individual search results
+  span multiple lines.
+
 Search now uses the JSON format internally
 
   This should address problems with unusual characters in authors and
-- 
1.7.10

Re: [PATCH v2 4/7] emacs: Use result text properties for search result iteration

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 599 bytes --]

On Sat, Jul 14 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> I think this is just a sharp corner in customize and there's notmuch
> we can do about it.  I believe the "proper" way to enter the newline
> directly in customize is to hit C-q C-j (same way you get a raw
> newline in any other buffer).  Given that I wouldn't expect anyone to
> guess that, though, we could mention this in the defcustom's
> docstring.

Ah.  C-q C-j definitely does it.  Thanks, Austin.

I'll follow up with a patch that modifies the doc string, which can
maybe help make things clearer for the uninitiated.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH v3 0/8] emacs: JSON-based search cleanups

[Mark Walters]

On Sun, 15 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> This version swaps out the notmuch-search-do-results macro for a
> higher-order function, notmuch-search-foreach-result.  This requires
> less squiting to understand and clearly distinguishes the arguments
> passed in to the function from the arguments passed to the callback.
> This version also updates the docstring for
> notmuch-search-result-format to mention that multi-line result formats
> work and how to enter them, and it adds a NEWS patch.

Hi 

In essence this version looks good and, contrary to what I though
before, I do prefer this function version to the macro. In addition it
seems to work well in testing.

However, there are some problems with multiline search results (see
below) so I think we should either fix these or just downplay this new
functionality by, for example, removing the comments on newlines from
the defcustom and saying in NEWS that the feature is experimental/not
complete or similar. (NEWS could say how to enter newlines in the
defcustom)

With this minor comment on the documentation my criticisms should not
hold up this excellent series.

Multiline oddities and todo

There is still some strange scrolling when updating a result. One
example that seems reproducible is if the search format has trailing
newline and the cursor is at the start of that line it may jump to the
far right of the previous line (possibly scrolling the screen
horizontally) upon a tag change (eg +unread).

If the format has a newline at the start of the author field it
sometimes gets omitted from the output. (In addition to the known
problem at the end of the author field that Austin mentions in the
documentation for the defcustom).

Examples of "incompleteness": these are rather more personal but it
seems odd to me to highlight one line rather than one result in the
buffer. Similarly I would expect <up> <down> to scroll up or down by
one result rather than one line.

I have a draft patch which fixes these two and mostly fixes the
scrolling but it is bigger than I would like (and I haven't yet ironed
out/checked all corner cases). I will split it up and send to the list
so people can try it (but it is not intended as a submission
currently).

As it seems to be tricky to get fully right it might be preferable
just to change the documentation as suggested above and leave
multi-line to the adventurous.

Best wishes 

Mark

>
> Diff from v2:
>
> diff --git a/NEWS b/NEWS
> index a1a6e93..7b33f0d 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -17,6 +17,20 @@ Maildir tag synchronization
>  Emacs Interface
>  ---------------
>  
> +Search results now get re-colored when tags are updated
> +
> +The formatting of tags in search results can now be customized
> +
> +  Previously, attempting to change the format of tags in
> +  `notmuch-search-result-format` would usually break tagging from
> +  search-mode.  We no longer make assumptions about the format.
> +
> +Multi-line search result formats are now supported
> +
> +  It is now possible to embed newlines in
> +  `notmuch-search-result-format` to make individual search results
> +  span multiple lines.
> +
>  Search now uses the JSON format internally
>  
>    This should address problems with unusual characters in authors and
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 7302fa7..ec760dc 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -69,7 +69,13 @@
>  	date, count, authors, subject, tags
>  For example:
>  	(setq notmuch-search-result-format \(\(\"authors\" . \"%-40s\"\)
> -					     \(\"subject\" . \"%s\"\)\)\)"
> +					     \(\"subject\" . \"%s\"\)\)\)
> +Line breaks are permitted in format strings.  Note that a line
> +break at the end of an \"authors\" field will get elided if the
> +authors list is long; place it instead at the beginning of the
> +following field.  To enter a line break when setting this
> +variable with setq, use \\n.  To enter a line break in customize,
> +press \\[quoted-insert] C-j."
>    :type '(alist :key-type (string) :value-type (string))
>    :group 'notmuch-search)
>  
> @@ -433,32 +439,39 @@ returns nil"
>      (next-single-property-change (or pos (point)) 'notmuch-search-result
>  				 nil (point-max))))
>  
> -(defmacro notmuch-search-do-results (beg end pos-sym &rest body)
> -  "Invoke BODY for each result between BEG and END.
> -
> -POS-SYM will be bound to the point at the beginning of the
> -current result."
> -  (declare (indent 3))
> -  (let ((end-sym (make-symbol "end"))
> -	(first-sym (make-symbol "first")))
> -    `(let ((,pos-sym (notmuch-search-result-beginning ,beg))
> -	   ;; End must be a marker in case body changes the text
> -	   (,end-sym (copy-marker ,end))
> -	   ;; Make sure we examine one result, even if (= beg end)
> -	   (,first-sym t))
> -       ;; We have to be careful if the region extends beyond the
> -       ;; results.  In this case, pos could be null or there could be
> -       ;; no result at pos.
> -       (while (and ,pos-sym (or (< ,pos-sym ,end-sym) ,first-sym))
> -	 (when (notmuch-search-get-result ,pos-sym)
> -	   ,@body)
> -	 (setq ,pos-sym (notmuch-search-result-end ,pos-sym)
> -	       ,first-sym nil)))))
> +(defun notmuch-search-foreach-result (beg end function)
> +  "Invoke FUNCTION for each result between BEG and END.
> +
> +FUNCTION should take one argument.  It will be applied to the
> +character position of the beginning of each result that overlaps
> +the region between points BEG and END.  As a special case, if (=
> +BEG END), FUNCTION will be applied to the result containing point
> +BEG."
> +
> +  (lexical-let ((pos (notmuch-search-result-beginning beg))
> +		;; End must be a marker in case function changes the
> +		;; text.
> +		(end (copy-marker end))
> +		;; Make sure we examine at least one result, even if
> +		;; (= beg end).
> +		(first t))
> +    ;; We have to be careful if the region extends beyond the results.
> +    ;; In this case, pos could be null or there could be no result at
> +    ;; pos.
> +    (while (and pos (or (< pos end) first))
> +      (when (notmuch-search-get-result pos)
> +	(funcall function pos))
> +      (setq pos (notmuch-search-result-end pos)
> +	    first nil))))
> +;; Unindent the function argument of notmuch-search-foreach-result so
> +;; the indentation of callers doesn't get out of hand.
> +(put 'notmuch-search-foreach-result 'lisp-indent-function 2)
>  
>  (defun notmuch-search-properties-in-region (property beg end)
>    (let (output)
> -    (notmuch-search-do-results beg end pos
> -      (push (plist-get (notmuch-search-get-result pos) property) output))
> +    (notmuch-search-foreach-result beg end
> +      (lambda (pos)
> +	(push (plist-get (notmuch-search-get-result pos) property) output)))
>      output))
>  
>  (defun notmuch-search-find-thread-id ()
> @@ -542,18 +555,20 @@ and will also appear in a buffer named \"*Notmuch errors*\"."
>  
>  (defun notmuch-search-get-tags-region (beg end)
>    (let (output)
> -    (notmuch-search-do-results beg end pos
> -      (setq output (append output (notmuch-search-get-tags pos))))
> +    (notmuch-search-foreach-result beg end
> +      (lambda (pos)
> +	(setq output (append output (notmuch-search-get-tags pos)))))
>      output))
>  
>  (defun notmuch-search-tag-region (beg end &optional tag-changes)
>    "Change tags for threads in the given region."
>    (let ((search-string (notmuch-search-find-thread-id-region-search beg end)))
>      (setq tag-changes (funcall 'notmuch-tag search-string tag-changes))
> -    (notmuch-search-do-results beg end pos
> -      (notmuch-search-set-tags
> -       (notmuch-update-tags (notmuch-search-get-tags pos) tag-changes)
> -       pos))))
> +    (notmuch-search-foreach-result beg end
> +      (lambda (pos)
> +	(notmuch-search-set-tags
> +	 (notmuch-update-tags (notmuch-search-get-tags pos) tag-changes)
> +	 pos)))))
>  
>  (defun notmuch-search-tag (&optional tag-changes)
>    "Change tags for the currently selected thread or region.

Re: [PATCH v3 0/8] emacs: JSON-based search cleanups

[Mark Walters]

On Sun, 15 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> This version swaps out the notmuch-search-do-results macro for a
> higher-order function, notmuch-search-foreach-result.  This requires
> less squiting to understand and clearly distinguishes the arguments
> passed in to the function from the arguments passed to the callback.
> This version also updates the docstring for
> notmuch-search-result-format to mention that multi-line result formats
> work and how to enter them, and it adds a NEWS patch.

Hello

I am afraid I have found a minor (but reproducible) bug in the line
re-drawing even with single line results. Find a search result with a
partially elided author field and put the cursor after the ellipsis in
that line. Then update the tags. The result gets redrawn with ellipsis
written out in full. Re-redrawing with the cursor after the author field
redraws the line with the keeping the ellipsis written out in full,
whereas re-redrawing the line with cursor before the author field gets
it written with the correct ellipsis.

Best wishes

Mark
>
> Diff from v2:
>
> diff --git a/NEWS b/NEWS
> index a1a6e93..7b33f0d 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -17,6 +17,20 @@ Maildir tag synchronization
>  Emacs Interface
>  ---------------
>  
> +Search results now get re-colored when tags are updated
> +
> +The formatting of tags in search results can now be customized
> +
> +  Previously, attempting to change the format of tags in
> +  `notmuch-search-result-format` would usually break tagging from
> +  search-mode.  We no longer make assumptions about the format.
> +
> +Multi-line search result formats are now supported
> +
> +  It is now possible to embed newlines in
> +  `notmuch-search-result-format` to make individual search results
> +  span multiple lines.
> +
>  Search now uses the JSON format internally
>  
>    This should address problems with unusual characters in authors and
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 7302fa7..ec760dc 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -69,7 +69,13 @@
>  	date, count, authors, subject, tags
>  For example:
>  	(setq notmuch-search-result-format \(\(\"authors\" . \"%-40s\"\)
> -					     \(\"subject\" . \"%s\"\)\)\)"
> +					     \(\"subject\" . \"%s\"\)\)\)
> +Line breaks are permitted in format strings.  Note that a line
> +break at the end of an \"authors\" field will get elided if the
> +authors list is long; place it instead at the beginning of the
> +following field.  To enter a line break when setting this
> +variable with setq, use \\n.  To enter a line break in customize,
> +press \\[quoted-insert] C-j."
>    :type '(alist :key-type (string) :value-type (string))
>    :group 'notmuch-search)
>  
> @@ -433,32 +439,39 @@ returns nil"
>      (next-single-property-change (or pos (point)) 'notmuch-search-result
>  				 nil (point-max))))
>  
> -(defmacro notmuch-search-do-results (beg end pos-sym &rest body)
> -  "Invoke BODY for each result between BEG and END.
> -
> -POS-SYM will be bound to the point at the beginning of the
> -current result."
> -  (declare (indent 3))
> -  (let ((end-sym (make-symbol "end"))
> -	(first-sym (make-symbol "first")))
> -    `(let ((,pos-sym (notmuch-search-result-beginning ,beg))
> -	   ;; End must be a marker in case body changes the text
> -	   (,end-sym (copy-marker ,end))
> -	   ;; Make sure we examine one result, even if (= beg end)
> -	   (,first-sym t))
> -       ;; We have to be careful if the region extends beyond the
> -       ;; results.  In this case, pos could be null or there could be
> -       ;; no result at pos.
> -       (while (and ,pos-sym (or (< ,pos-sym ,end-sym) ,first-sym))
> -	 (when (notmuch-search-get-result ,pos-sym)
> -	   ,@body)
> -	 (setq ,pos-sym (notmuch-search-result-end ,pos-sym)
> -	       ,first-sym nil)))))
> +(defun notmuch-search-foreach-result (beg end function)
> +  "Invoke FUNCTION for each result between BEG and END.
> +
> +FUNCTION should take one argument.  It will be applied to the
> +character position of the beginning of each result that overlaps
> +the region between points BEG and END.  As a special case, if (=
> +BEG END), FUNCTION will be applied to the result containing point
> +BEG."
> +
> +  (lexical-let ((pos (notmuch-search-result-beginning beg))
> +		;; End must be a marker in case function changes the
> +		;; text.
> +		(end (copy-marker end))
> +		;; Make sure we examine at least one result, even if
> +		;; (= beg end).
> +		(first t))
> +    ;; We have to be careful if the region extends beyond the results.
> +    ;; In this case, pos could be null or there could be no result at
> +    ;; pos.
> +    (while (and pos (or (< pos end) first))
> +      (when (notmuch-search-get-result pos)
> +	(funcall function pos))
> +      (setq pos (notmuch-search-result-end pos)
> +	    first nil))))
> +;; Unindent the function argument of notmuch-search-foreach-result so
> +;; the indentation of callers doesn't get out of hand.
> +(put 'notmuch-search-foreach-result 'lisp-indent-function 2)
>  
>  (defun notmuch-search-properties-in-region (property beg end)
>    (let (output)
> -    (notmuch-search-do-results beg end pos
> -      (push (plist-get (notmuch-search-get-result pos) property) output))
> +    (notmuch-search-foreach-result beg end
> +      (lambda (pos)
> +	(push (plist-get (notmuch-search-get-result pos) property) output)))
>      output))
>  
>  (defun notmuch-search-find-thread-id ()
> @@ -542,18 +555,20 @@ and will also appear in a buffer named \"*Notmuch errors*\"."
>  
>  (defun notmuch-search-get-tags-region (beg end)
>    (let (output)
> -    (notmuch-search-do-results beg end pos
> -      (setq output (append output (notmuch-search-get-tags pos))))
> +    (notmuch-search-foreach-result beg end
> +      (lambda (pos)
> +	(setq output (append output (notmuch-search-get-tags pos)))))
>      output))
>  
>  (defun notmuch-search-tag-region (beg end &optional tag-changes)
>    "Change tags for threads in the given region."
>    (let ((search-string (notmuch-search-find-thread-id-region-search beg end)))
>      (setq tag-changes (funcall 'notmuch-tag search-string tag-changes))
> -    (notmuch-search-do-results beg end pos
> -      (notmuch-search-set-tags
> -       (notmuch-update-tags (notmuch-search-get-tags pos) tag-changes)
> -       pos))))
> +    (notmuch-search-foreach-result beg end
> +      (lambda (pos)
> +	(notmuch-search-set-tags
> +	 (notmuch-update-tags (notmuch-search-get-tags pos) tag-changes)
> +	 pos)))))
>  
>  (defun notmuch-search-tag (&optional tag-changes)
>    "Change tags for the currently selected thread or region.

Re: [PATCH v3 0/8] emacs: JSON-based search cleanups

[Austin Clements]

On Sun, 15 Jul 2012, Mark Walters <markwalters1009@gmail.com> wrote:
> On Sun, 15 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
>> This version swaps out the notmuch-search-do-results macro for a
>> higher-order function, notmuch-search-foreach-result.  This requires
>> less squiting to understand and clearly distinguishes the arguments
>> passed in to the function from the arguments passed to the callback.
>> This version also updates the docstring for
>> notmuch-search-result-format to mention that multi-line result formats
>> work and how to enter them, and it adds a NEWS patch.
>
> Hello
>
> I am afraid I have found a minor (but reproducible) bug in the line
> re-drawing even with single line results. Find a search result with a
> partially elided author field and put the cursor after the ellipsis in
> that line. Then update the tags. The result gets redrawn with ellipsis
> written out in full. Re-redrawing with the cursor after the author field
> redraws the line with the keeping the ellipsis written out in full,
> whereas re-redrawing the line with cursor before the author field gets
> it written with the correct ellipsis.

Arrrg, overlays.

I can think of two ways to fix this.  We could generate the author
elision overlays lazily (say, via jit-lock).  This would have the added
benefit of eliminating what I think is the last quadratic factor in
building search buffers, but there are much easier ways to do that.  Or,
I could scrap the insert-before-markers nonsense and manipulate point
directly in notmuch-search-update-result, with the caveat that the
little bit of support it had for doing sane things in some situations
involving save-excursions would be lost.  Given that we never call
notmuch-search-update-result inside a save-excursion (precisely because
I couldn't reliably hit the narrow window of situations it could handle
when there were save-excursions involved), I'd lean toward the latter
option.

Re: [PATCH v3 0/8] emacs: JSON-based search cleanups

[Mark Walters]

On Sun, 15 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> On Sun, 15 Jul 2012, Mark Walters <markwalters1009@gmail.com> wrote:
>> On Sun, 15 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
>>> This version swaps out the notmuch-search-do-results macro for a
>>> higher-order function, notmuch-search-foreach-result.  This requires
>>> less squiting to understand and clearly distinguishes the arguments
>>> passed in to the function from the arguments passed to the callback.
>>> This version also updates the docstring for
>>> notmuch-search-result-format to mention that multi-line result formats
>>> work and how to enter them, and it adds a NEWS patch.
>>
>> Hello
>>
>> I am afraid I have found a minor (but reproducible) bug in the line
>> re-drawing even with single line results. Find a search result with a
>> partially elided author field and put the cursor after the ellipsis in
>> that line. Then update the tags. The result gets redrawn with ellipsis
>> written out in full. Re-redrawing with the cursor after the author field
>> redraws the line with the keeping the ellipsis written out in full,
>> whereas re-redrawing the line with cursor before the author field gets
>> it written with the correct ellipsis.
>
> Arrrg, overlays.
>
> I can think of two ways to fix this.  We could generate the author
> elision overlays lazily (say, via jit-lock).  This would have the added
> benefit of eliminating what I think is the last quadratic factor in
> building search buffers, but there are much easier ways to do that.  Or,
> I could scrap the insert-before-markers nonsense and manipulate point
> directly in notmuch-search-update-result, with the caveat that the
> little bit of support it had for doing sane things in some situations
> involving save-excursions would be lost.  Given that we never call
> notmuch-search-update-result inside a save-excursion (precisely because
> I couldn't reliably hit the narrow window of situations it could handle
> when there were save-excursions involved), I'd lean toward the latter
> option.

I think I don't really know enough emacs/lisp to be able to say anything
sensible. I think manipulating point directly seems good because then I
think we can make sure things work in the multiline case too. 

I haven't yet worked out whether multiline is an amusing "oh look it works"
or genuinely useful thing. I am leaning towards the latter as do often
work on laptops with quite small screens and think I would like to see
more about a smaller number of results.

Best wishes

Mark

Re: [PATCH v3 0/8] emacs: JSON-based search cleanups

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 1512 bytes --]

On Sun, Jul 15 2012, Mark Walters <markwalters1009@gmail.com> wrote:
> However, there are some problems with multiline search results (see
> below) so I think we should either fix these or just downplay this new
> functionality by, for example, removing the comments on newlines from
> the defcustom and saying in NEWS that the feature is experimental/not
> complete or similar. (NEWS could say how to enter newlines in the
> defcustom)
>
> With this minor comment on the documentation my criticisms should not
> hold up this excellent series.

I agree that multi-line support needs a little bit more work before it's
ready for prime time.  I'll keep experimenting with it as well and see
if I can uncover any other issues.

But I definitely also agree that that work should *not* hold up this
patch series, as it adds plenty of other benefits.

> Examples of "incompleteness": these are rather more personal but it
> seems odd to me to highlight one line rather than one result in the
> buffer. Similarly I would expect <up> <down> to scroll up or down by
> one result rather than one line.

Highlighting the entire entry should definitely be fixed, since
otherwise it can be hard to see which of the other lines is associated
with the current entry.

I also notice that the author field munging causes some weird behaviors.
Certain formatter strings can cause it to break.  If we can get that
fixed it might be nice to have a similar functionality for the subject
field, which can also be really long.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

[PATCH v4] emacs: JSON-based search cleanups

[Austin Clements]

Ack.  I grabbed the wrong in-reply-to message ID when sending v4.
Please see id:"1342892232-5659-1-git-send-email-amdragon@mit.edu" for
v4.  Sorry about any confusion.