From: Mark Walters <markwalters1009@gmail.com>
To: Austin Clements <amdragon@MIT.EDU>, notmuch@notmuchmail.org
Subject: Re: [PATCH v3 0/8] emacs: JSON-based search cleanups
In-Reply-To: <1342306940-7499-1-git-send-email-amdragon@mit.edu>
References: <1342140319-19859-1-git-send-email-amdragon@mit.edu>
	<1342306940-7499-1-git-send-email-amdragon@mit.edu>
User-Agent: Notmuch/0.13.2+90~g84fa1ef (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Sun, 15 Jul 2012 09:34:50 +0100
Message-ID: <87fw8ttob9.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

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.