[PATCH v2] emacs: Add notmuch-show--build-queries.

[PATCH v2] emacs: Add notmuch-show--build-queries.

[Matt Armstrong]

This supercedes
id:1474003701-19831-1-git-send-email-marmstrong@google.com with a much
simpler patch.  My goal here is to make it easier to tweak
notmuch-show behavior without hacking on notmuch-show--build-buffer
itself, since it is responsible for a host of other tasks.

I'm still working (slowly) on coming up with a nice mental model for
the way notmuch's Emacs interface treats threads -vs- messages with
respect to "expansion" in show and tree results.  Although, to be
honest, wiht this patch and the following piece of advice,
notmuch-show buffers are doing what I want in all cases:

(defun my-notmuch-show--prepend-tag-unread (queries)
  "Prepend a tag:unread query to QUERIES.

Intended to be used as :filter-return advice on
`notmuch-show--build-queries'."
  (cons `(,notmuch-show-thread-id "and tag:unread") queries))

(advice-add 'notmuch-show--build-queries :filter-return
            #'my-notmuch-show--prepend-tag-unread)

[PATCH v2] Add notmuch-show--build-queries.

[Matt Armstrong]

notmuch-show--build-buffer now queries a list of queries built by the
former.  This simplifies the logic.  It also provides an easy place to
experiment with alternate sets of queries for given notmuch-show-*
variables (e.g. users can use advice-add to do so in a surgical way).
---
 emacs/notmuch-show.el | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index f2487ab..0727319 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1261,6 +1261,20 @@ matched."
 	(message "No messages matched the query!")
 	nil))))
 
+(defun notmuch-show--build-queries ()
+  "Return a list of queries to try for this search.
+
+If `notmuch-show-query-context` is not nil, the first query is
+the conjunction of it and `notmuch-show-thread-id`.  The next
+query is `notmuch-show-thread-id` alone, and serves as a fallback
+if the prior matches no messages."
+  (let* ((thread notmuch-show-thread-id)
+	 (context notmuch-show-query-context)
+	 (queries `((,thread))))
+    (if context
+	(setq queries (cons `(,thread "and (" ,context ")") queries)))
+    queries))
+
 (defun notmuch-show--build-buffer (&optional state)
   "Display messages matching the current buffer context.
 
@@ -1268,25 +1282,20 @@ Apply the previously saved STATE if supplied, otherwise show the
 first relevant message.
 
 If no messages match the query return NIL."
-  (let* ((basic-args (list notmuch-show-thread-id))
-	 (args (if notmuch-show-query-context
-		   (append (list "\'") basic-args
-			   (list "and (" notmuch-show-query-context ")\'"))
-		 (append (list "\'") basic-args (list "\'"))))
-	 (cli-args (cons "--exclude=false"
+  (let* ((cli-args (cons "--exclude=false"
 			 (when notmuch-show-elide-non-matching-messages
 			   (list "--entire-thread=false"))))
-
-	 (forest (or (notmuch-query-get-threads (append cli-args args))
-		     ;; If a query context reduced the number of
-		     ;; results to zero, try again without it.
-		     (and notmuch-show-query-context
-			  (notmuch-query-get-threads (append cli-args basic-args)))))
-
+	 (queries (notmuch-show--build-queries))
+	 (forest nil)
 	 ;; Must be reset every time we are going to start inserting
 	 ;; messages into the buffer.
 	 (notmuch-show-previous-subject ""))
-
+    ;; Use results from the first query that returns some.
+    (while (and (consp queries)
+		(not (setq forest
+			   (notmuch-query-get-threads
+			    `(,@cli-args "'" ,@(car queries) "'")))))
+      (setq queries (cdr queries)))
     (when forest
       (notmuch-show-insert-forest forest)
 
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH v2] Add notmuch-show--build-queries.

[Mark Walters]

On Tue, 11 Oct 2016, Matt Armstrong <marmstrong@google.com> wrote:
> notmuch-show--build-buffer now queries a list of queries built by the
> former.  This simplifies the logic.  It also provides an easy place to
> experiment with alternate sets of queries for given notmuch-show-*
> variables (e.g. users can use advice-add to do so in a surgical way).

Hi

I think I like the overall logic, but I do have some comments below.

> ---
>  emacs/notmuch-show.el | 37 +++++++++++++++++++++++--------------
>  1 file changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index f2487ab..0727319 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -1261,6 +1261,20 @@ matched."
>  	(message "No messages matched the query!")
>  	nil))))
>  
> +(defun notmuch-show--build-queries ()
> +  "Return a list of queries to try for this search.
> +
> +If `notmuch-show-query-context` is not nil, the first query is
> +the conjunction of it and `notmuch-show-thread-id`.  The next
> +query is `notmuch-show-thread-id` alone, and serves as a fallback
> +if the prior matches no messages."
> +  (let* ((thread notmuch-show-thread-id)
> +	 (context notmuch-show-query-context)
> +	 (queries `((,thread))))
> +    (if context
> +	(setq queries (cons `(,thread "and (" ,context ")") queries)))
> +    queries))

I think it would be better to keep closer to the existing style -- using
list and append. At least do that as a first patch and then do a change
as above with ` and , so we can review them separately. Note I find the
list append style much easier to read, and I am guessing someone else
did (perhaps cworth?) as they wrote it that way.

I think I would go for something like

(let* ((...
    queries))
  (push (list thread) queries)
  (if context (push (list thread "and (" context ")") queries))
  queries)

> +
>  (defun notmuch-show--build-buffer (&optional state)
>    "Display messages matching the current buffer context.
>  
> @@ -1268,25 +1282,20 @@ Apply the previously saved STATE if supplied, otherwise show the
>  first relevant message.
>  
>  If no messages match the query return NIL."
> -  (let* ((basic-args (list notmuch-show-thread-id))
> -	 (args (if notmuch-show-query-context
> -		   (append (list "\'") basic-args
> -			   (list "and (" notmuch-show-query-context ")\'"))
> -		 (append (list "\'") basic-args (list "\'"))))
> -	 (cli-args (cons "--exclude=false"
> +  (let* ((cli-args (cons "--exclude=false"
>  			 (when notmuch-show-elide-non-matching-messages
>  			   (list "--entire-thread=false"))))
> -
> -	 (forest (or (notmuch-query-get-threads (append cli-args args))
> -		     ;; If a query context reduced the number of
> -		     ;; results to zero, try again without it.
> -		     (and notmuch-show-query-context
> -			  (notmuch-query-get-threads (append cli-args basic-args)))))
> -
> +	 (queries (notmuch-show--build-queries))
> +	 (forest nil)
>  	 ;; Must be reset every time we are going to start inserting
>  	 ;; messages into the buffer.
>  	 (notmuch-show-previous-subject ""))
> -
> +    ;; Use results from the first query that returns some.
> +    (while (and (consp queries)
> +		(not (setq forest
> +			   (notmuch-query-get-threads
> +			    `(,@cli-args "'" ,@(car queries) "'")))))
> +      (setq queries (cdr queries)))

Similarly here I would avoid the ` , @ . I think I would also suggest
making pushing the setq outside the while condition. Something like

(while (and (consp queries)
            (not forest))
   (setq forest (notmuch-query-get-threads
                  (append cli-args (list "'") (car queries) (list "'"))))
            

Best wishes

Mark

Re: [PATCH v2] Add notmuch-show--build-queries.

[Matt Armstrong]

Mark, thanks for the review.  I agree with your comments.  I'll send
another patch.

As you might guess, I'm new to elisp.  Use of push makes some of this
fall out nicely.

For what it is worth, I did experiment with using loop to find the first
functional "forest".  There are several other calls to loop in this
file.  It was something like:

(let* ((cli-args (....))
       (forest (loop for query in (notmuch-show--build-queries)
                     for forest = (notmuch-query-get-threads
                                   (append cli-args
                                           (list "'") query (list "'")))
                     until forest
                     finally return forest))
      ...)
  ...)


I won't go that route in my next patch (I'm not convinced it is better).
But I also am not particularly invested in any one way to do this.



Mark Walters <markwalters1009@gmail.com> writes:

> On Tue, 11 Oct 2016, Matt Armstrong <marmstrong@google.com> wrote:
>> notmuch-show--build-buffer now queries a list of queries built by the
>> former.  This simplifies the logic.  It also provides an easy place to
>> experiment with alternate sets of queries for given notmuch-show-*
>> variables (e.g. users can use advice-add to do so in a surgical way).
>
> Hi
>
> I think I like the overall logic, but I do have some comments below.
>
>> ---
>>  emacs/notmuch-show.el | 37 +++++++++++++++++++++++--------------
>>  1 file changed, 23 insertions(+), 14 deletions(-)
>>
>> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
>> index f2487ab..0727319 100644
>> --- a/emacs/notmuch-show.el
>> +++ b/emacs/notmuch-show.el
>> @@ -1261,6 +1261,20 @@ matched."
>>  	(message "No messages matched the query!")
>>  	nil))))
>>  
>> +(defun notmuch-show--build-queries ()
>> +  "Return a list of queries to try for this search.
>> +
>> +If `notmuch-show-query-context` is not nil, the first query is
>> +the conjunction of it and `notmuch-show-thread-id`.  The next
>> +query is `notmuch-show-thread-id` alone, and serves as a fallback
>> +if the prior matches no messages."
>> +  (let* ((thread notmuch-show-thread-id)
>> +	 (context notmuch-show-query-context)
>> +	 (queries `((,thread))))
>> +    (if context
>> +	(setq queries (cons `(,thread "and (" ,context ")") queries)))
>> +    queries))
>
> I think it would be better to keep closer to the existing style -- using
> list and append. At least do that as a first patch and then do a change
> as above with ` and , so we can review them separately. Note I find the
> list append style much easier to read, and I am guessing someone else
> did (perhaps cworth?) as they wrote it that way.
>
> I think I would go for something like
>
> (let* ((...
>     queries))
>   (push (list thread) queries)
>   (if context (push (list thread "and (" context ")") queries))
>   queries)



>>  (defun notmuch-show--build-buffer (&optional state)
>>    "Display messages matching the current buffer context.
>>  
>> @@ -1268,25 +1282,20 @@ Apply the previously saved STATE if supplied, otherwise show the
>>  first relevant message.
>>  
>>  If no messages match the query return NIL."
>> -  (let* ((basic-args (list notmuch-show-thread-id))
>> -	 (args (if notmuch-show-query-context
>> -		   (append (list "\'") basic-args
>> -			   (list "and (" notmuch-show-query-context ")\'"))
>> -		 (append (list "\'") basic-args (list "\'"))))
>> -	 (cli-args (cons "--exclude=false"
>> +  (let* ((cli-args (cons "--exclude=false"
>>  			 (when notmuch-show-elide-non-matching-messages
>>  			   (list "--entire-thread=false"))))
>> -
>> -	 (forest (or (notmuch-query-get-threads (append cli-args args))
>> -		     ;; If a query context reduced the number of
>> -		     ;; results to zero, try again without it.
>> -		     (and notmuch-show-query-context
>> -			  (notmuch-query-get-threads (append cli-args basic-args)))))
>> -
>> +	 (queries (notmuch-show--build-queries))
>> +	 (forest nil)
>>  	 ;; Must be reset every time we are going to start inserting
>>  	 ;; messages into the buffer.
>>  	 (notmuch-show-previous-subject ""))
>> -
>> +    ;; Use results from the first query that returns some.
>> +    (while (and (consp queries)
>> +		(not (setq forest
>> +			   (notmuch-query-get-threads
>> +			    `(,@cli-args "'" ,@(car queries) "'")))))
>> +      (setq queries (cdr queries)))
>
> Similarly here I would avoid the ` , @ . I think I would also suggest
> making pushing the setq outside the while condition. Something like
>
> (while (and (consp queries)
>             (not forest))
>    (setq forest (notmuch-query-get-threads
>                   (append cli-args (list "'") (car queries) (list "'"))))