[RFC PATCH 0/3] lib/cli/emacs: limit number of messages in search results

[RFC PATCH 0/3] lib/cli/emacs: limit number of messages in search results

[Jani Nikula]

Hi, here are a few RFC patches that add support for limiting the number of
messages in search results. The main goal was emacs; the lib/cli patches could
probably be better thought out to be more general.

The emacs interface was inspired by vc-print-log in Emacs vc.el. You'll get
buttons [Show 2X messages] and [Show unlimited messages] at the end of search
results to expand the results. You can use the 2X button repeatedly to increase
the limit for the buffer.

When the bulk of the work was done, I was told in IRC that patches exist
towards a similar goal:
 id:"87vctrq4or.fsf@hackervisions.org"
 id:"8739gyw0zh.fsf@opensourcematters.org"

My approach is slightly different. I think it's easier and better to do this in
lib rather than just in cli.

BR,
Jani.


Jani Nikula (3):
  lib: add support for limiting the number of search results
  cli: add support for limiting the number of search results
  emacs: support limiting the number of messages shown in search
    results

 emacs/notmuch-hello.el |   19 ++++++++++++++++---
 emacs/notmuch.el       |   43 ++++++++++++++++++++++++++++++++++++++-----
 lib/notmuch.h          |    3 +++
 lib/query.cc           |   26 ++++++++++++++++++++++++--
 notmuch-search.c       |   10 ++++++++++
 5 files changed, 91 insertions(+), 10 deletions(-)

-- 
1.7.5.4

[RFC PATCH 1/3] lib: add support for limiting the number of search results

[Jani Nikula]

Add a function to support limiting the number of messages in search
results. This is a fairly straightforward implementation just to support
the following patches. The proper design should probably support paging of
results (i.e. first give me results 0...49, then 50...99, etc.) That should
not be too difficult, as long as the library interface is properly thought
out.

Signed-off-by: Jani Nikula <jani@nikula.org>
---
 lib/notmuch.h |    3 +++
 lib/query.cc  |   26 ++++++++++++++++++++++++--
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/lib/notmuch.h b/lib/notmuch.h
index c4330e4..b5ef030 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -457,6 +457,9 @@ notmuch_query_set_sort (notmuch_query_t *query, notmuch_sort_t sort);
 notmuch_sort_t
 notmuch_query_get_sort (notmuch_query_t *query);
 
+void
+notmuch_query_set_maxitems (notmuch_query_t *query, unsigned int maxitems);
+
 /* Execute a query for threads, returning a notmuch_threads_t object
  * which can be used to iterate over the results. The returned threads
  * object is owned by the query and as such, will only be valid until
diff --git a/lib/query.cc b/lib/query.cc
index 6f02b04..04dfbc5 100644
--- a/lib/query.cc
+++ b/lib/query.cc
@@ -27,6 +27,7 @@ struct _notmuch_query {
     notmuch_database_t *notmuch;
     const char *query_string;
     notmuch_sort_t sort;
+    Xapian::doccount maxitems;
 };
 
 typedef struct _notmuch_mset_messages {
@@ -76,6 +77,8 @@ notmuch_query_create (notmuch_database_t *notmuch,
 
     query->sort = NOTMUCH_SORT_NEWEST_FIRST;
 
+    query->maxitems = 0;
+
     return query;
 }
 
@@ -97,6 +100,12 @@ notmuch_query_get_sort (notmuch_query_t *query)
     return query->sort;
 }
 
+void
+notmuch_query_set_maxitems(notmuch_query_t *query, unsigned int maxitems)
+{
+    query->maxitems = maxitems;
+}
+
 /* We end up having to call the destructors explicitly because we had
  * to use "placement new" in order to initialize C++ objects within a
  * block that we allocated with talloc. So C++ is making talloc
@@ -181,8 +190,21 @@ notmuch_query_search_messages (notmuch_query_t *query)
 
 	mset = enquire.get_mset (0, notmuch->xapian_db->get_doccount ());
 
-	messages->iterator = mset.begin ();
-	messages->iterator_end = mset.end ();
+	if (query->maxitems && query->maxitems < mset.size()) {
+	    if (query->sort == NOTMUCH_SORT_OLDEST_FIRST) {
+		/* Sort oldest first, but return the newest messages. */
+		messages->iterator = mset[mset.size() - query->maxitems];
+		messages->iterator_end = mset.end ();
+	    } else {
+		/* This path could be optimized by using maxitems in
+		 * enquire.get_mset(). */
+		messages->iterator = mset.begin ();
+		messages->iterator_end = mset[query->maxitems];
+	    }
+	} else {
+	    messages->iterator = mset.begin ();
+	    messages->iterator_end = mset.end ();
+	}
 
 	return &messages->base;
 
-- 
1.7.5.4

[RFC PATCH 2/3] cli: add support for limiting the number of search results

[Jani Nikula]

Add command line parameter --maxitems=N to notmuch search to limit the
number of displayed messages to N.

These two are equal:

$ notmuch search --output=messages --sort=newest-first --maxitems=10 SEARCH
$ notmuch search --output=messages --sort=newest-first SEARCH | head

As are these:

$ notmuch search --output=messages --sort=oldest-first --maxitems=10 SEARCH
$ notmuch search --output=messages --sort=oldest-first SEARCH | tail

Note that N refers to the maximum amount of messages, even for
--output=threads.

Signed-off-by: Jani Nikula <jani@nikula.org>
---
 notmuch-search.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/notmuch-search.c b/notmuch-search.c
index 6f04d9a..a3a6475 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -394,6 +394,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
     const search_format_t *format = &format_text;
     int i, ret;
     output_t output = OUTPUT_SUMMARY;
+    unsigned int maxitems = 0;
 
     argc--; argv++; /* skip subcommand argument */
 
@@ -412,6 +413,14 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
 		fprintf (stderr, "Invalid value for --sort: %s\n", opt);
 		return 1;
 	    }
+	} else if (STRNCMP_LITERAL (argv[i], "--maxitems=") == 0) {
+	    const char *p;
+	    opt = argv[i] + sizeof ("--maxitems=") - 1;
+	    maxitems = strtoul(opt, &p, 10);
+	    if (*opt == '\0' || p == opt || *p != '\0') {
+		fprintf (stderr, "Invalid value for --maxitems: %s\n", opt);
+		return 1;
+	    }
 	} else if (STRNCMP_LITERAL (argv[i], "--format=") == 0) {
 	    opt = argv[i] + sizeof ("--format=") - 1;
 	    if (strcmp (opt, "text") == 0) {
@@ -473,6 +482,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
     }
 
     notmuch_query_set_sort (query, sort);
+    notmuch_query_set_maxitems (query, maxitems);
 
     switch (output) {
     default:
-- 
1.7.5.4

[RFC PATCH 3/3] emacs: support limiting the number of messages shown in search results

[Jani Nikula]

Add support for limiting the maximum number of messages initially displayed
in search results. When enabled, the search results will contain push
buttons to double the number of messages displayed or to show unlimited
messages.

The approach is inspired by vc-print-log in Emacs vc.el.

Signed-off-by: Jani Nikula <jani@nikula.org>
---
 emacs/notmuch-hello.el |   19 ++++++++++++++++---
 emacs/notmuch.el       |   43 ++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 54 insertions(+), 8 deletions(-)

diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
index 65fde75..16eecb6 100644
--- a/emacs/notmuch-hello.el
+++ b/emacs/notmuch-hello.el
@@ -26,7 +26,7 @@
 (require 'notmuch-lib)
 (require 'notmuch-mua)
 
-(declare-function notmuch-search "notmuch" (query &optional oldest-first target-thread target-line continuation))
+(declare-function notmuch-search "notmuch" (query &optional oldest-first target-thread target-line continuation maxitems))
 (declare-function notmuch-poll "notmuch" ())
 
 (defvar notmuch-hello-search-bar-marker nil
@@ -37,6 +37,18 @@
   :type 'integer
   :group 'notmuch)
 
+(defcustom notmuch-search-maxitems 100
+  "The maximum number of messages to show in search results.
+
+This variables controls the maximum number of messages to
+initially show in search results. Set to 0 to unlimited. If
+non-zero, the search results will contain push buttons to double
+the number (can be repeated) or show unlimited number of
+messages. Note that this controls the number of messages; in a
+threaded view this is not the number of lines to show."
+  :type 'integer
+  :group 'notmuch)
+
 (defcustom notmuch-show-empty-saved-searches nil
   "Should saved searches with no messages be listed?"
   :type 'boolean
@@ -151,7 +163,7 @@ Typically \",\" in the US and UK and \".\" in Europe."
 (defun notmuch-hello-search (search)
   (let ((search (notmuch-hello-trim search)))
     (notmuch-hello-remember-search search)
-    (notmuch-search search notmuch-search-oldest-first nil nil #'notmuch-hello-search-continuation)))
+    (notmuch-search search notmuch-search-oldest-first nil nil #'notmuch-hello-search-continuation notmuch-search-maxitems)))
 
 (defun notmuch-hello-add-saved-search (widget)
   (interactive)
@@ -201,7 +213,8 @@ diagonal."
   (notmuch-search (widget-get widget
 			      :notmuch-search-terms)
 		  notmuch-search-oldest-first
-		  nil nil #'notmuch-hello-search-continuation))
+		  nil nil #'notmuch-hello-search-continuation
+		  notmuch-search-maxitems))
 
 (defun notmuch-saved-search-count (search)
   (car (process-lines notmuch-command "count" search)))
diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index f11ec24..bb805da 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -193,6 +193,7 @@ For a mouse binding, return nil."
 
 (defvar notmuch-search-mode-map
   (let ((map (make-sparse-keymap)))
+    (set-keymap-parent map widget-keymap)
     (define-key map "?" 'notmuch-help)
     (define-key map "q" 'notmuch-search-quit)
     (define-key map "x" 'notmuch-search-quit)
@@ -217,7 +218,13 @@ For a mouse binding, return nil."
     (define-key map "a" 'notmuch-search-archive-thread)
     (define-key map "-" 'notmuch-search-remove-tag)
     (define-key map "+" 'notmuch-search-add-tag)
-    (define-key map (kbd "RET") 'notmuch-search-show-thread)
+    ; Some hackery to allow RET both on buttons and messages. There's probably a
+    ; better way to do this...
+    (define-key map (kbd "RET") '(lambda (pos)
+				   (interactive "@d")
+				   (if (get-char-property pos 'button)
+				       (widget-button-press pos)
+				     (notmuch-search-show-thread))))
     (define-key map (kbd "M-RET") 'notmuch-search-show-thread-crypto-switch)
     map)
   "Keymap for \"notmuch search\" buffers.")
@@ -239,6 +246,7 @@ For a mouse binding, return nil."
 (defvar notmuch-search-target-thread)
 (defvar notmuch-search-target-line)
 (defvar notmuch-search-continuation)
+(defvar notmuch-search-maxitems)
 
 (defvar notmuch-search-disjunctive-regexp      "\\<[oO][rR]\\>")
 
@@ -373,6 +381,7 @@ Complete list of currently available key bindings:
   (make-local-variable 'notmuch-search-oldest-first)
   (make-local-variable 'notmuch-search-target-thread)
   (make-local-variable 'notmuch-search-target-line)
+  (make-local-variable 'notmuch-search-maxitems)
   (set (make-local-variable 'notmuch-search-continuation) nil)
   (set (make-local-variable 'scroll-preserve-screen-position) t)
   (add-to-invisibility-spec 'notmuch-search)
@@ -633,6 +642,8 @@ This function advances the next thread when finished."
 			(insert "End of search results.")
 			(if (not (= exit-status 0))
 			    (insert (format " (process returned %d)" exit-status)))
+			(if notmuch-search-maxitems
+			    (notmuch-search-setup-buttons))
 			(insert "\n")
 			(if (and atbob
 				 (not (string= notmuch-search-target-thread "found")))
@@ -883,7 +894,7 @@ characters as well as `_.+-'.
 	  )))
 
 ;;;###autoload
-(defun notmuch-search (query &optional oldest-first target-thread target-line continuation)
+(defun notmuch-search (query &optional oldest-first target-thread target-line continuation maxitems)
   "Run \"notmuch search\" with the given query string and display results.
 
 The optional parameters are used as follows:
@@ -899,6 +910,7 @@ The optional parameters are used as follows:
     (notmuch-search-mode)
     (set 'notmuch-search-query-string query)
     (set 'notmuch-search-oldest-first oldest-first)
+    (set 'notmuch-search-maxitems maxitems)
     (set 'notmuch-search-target-thread target-thread)
     (set 'notmuch-search-target-line target-line)
     (set 'notmuch-search-continuation continuation)
@@ -916,6 +928,8 @@ The optional parameters are used as follows:
 		     (if oldest-first
 			 "--sort=oldest-first"
 		       "--sort=newest-first")
+		     (if maxitems
+			 (format "--maxitems=%d" maxitems))
 		     query)))
 	  (set-process-sentinel proc 'notmuch-search-process-sentinel)
 	  (set-process-filter proc 'notmuch-search-process-filter))))
@@ -932,13 +946,32 @@ same relative position within the new buffer."
   (interactive)
   (let ((target-line (line-number-at-pos))
 	(oldest-first notmuch-search-oldest-first)
+	(maxitems notmuch-search-maxitems)
 	(target-thread (notmuch-search-find-thread-id))
 	(query notmuch-search-query-string)
 	(continuation notmuch-search-continuation))
     (notmuch-kill-this-buffer)
-    (notmuch-search query oldest-first target-thread target-line continuation)
+    (notmuch-search query oldest-first target-thread target-line continuation maxitems)
     (goto-char (point-min))))
 
+(defun notmuch-search-setup-buttons ()
+  (widget-insert "    ")
+  (widget-create 'push-button
+		 :notify (lambda (&rest ignore)
+			   (set 'notmuch-search-maxitems
+				(* 2 notmuch-search-maxitems))
+			   (notmuch-search-refresh-view))
+		 :help-echo "Double the number of messages shown"
+		 "Show 2X messages")
+  (widget-insert "    ")
+  (widget-create 'push-button
+		 :notify (lambda (&rest ignore)
+			   (set 'notmuch-search-maxitems 0)
+			   (notmuch-search-refresh-view))
+		 :help-echo "Show all search results"
+		 "Show unlimited messages")
+  (widget-setup))
+
 (defcustom notmuch-poll-script ""
   "An external script to incorporate new mail into the notmuch database.
 
@@ -997,7 +1030,7 @@ current search results AND the additional query string provided."
 			 query)))
     (notmuch-search (if (string= notmuch-search-query-string "*")
 			grouped-query
-		      (concat notmuch-search-query-string " and " grouped-query)) notmuch-search-oldest-first)))
+		      (concat notmuch-search-query-string " and " grouped-query)) notmuch-search-oldest-first notmuch-search-maxitems)))
 
 (defun notmuch-search-filter-by-tag (tag)
   "Filter the current search results based on a single tag.
@@ -1006,7 +1039,7 @@ Runs a new search matching only messages that match both the
 current search results AND that are tagged with the given tag."
   (interactive
    (list (notmuch-select-tag-with-completion "Filter by tag: ")))
-  (notmuch-search (concat notmuch-search-query-string " and tag:" tag) notmuch-search-oldest-first))
+  (notmuch-search (concat notmuch-search-query-string " and tag:" tag) notmuch-search-oldest-first notmuch-search-maxitems))
 
 ;;;###autoload
 (defun notmuch ()
-- 
1.7.5.4

Re: [RFC PATCH 3/3] emacs: support limiting the number of messages shown in search results

[Daniel Schoepe]

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

On Fri, 28 Oct 2011 23:59:31 +0300, Jani Nikula <jani@nikula.org> wrote:
> Add support for limiting the maximum number of messages initially displayed
> in search results. When enabled, the search results will contain push
> buttons to double the number of messages displayed or to show unlimited
> messages.

Nice patch, as it not only makes searches with a lot of results easier
to use on slower machines/hard drives, but I also find that seeing only
a few dozen threads in the buffer looks more "orderly" to me, compared
to a buffer with hundreds of lines.

A few comments about the code:

> @@ -373,6 +381,7 @@ Complete list of currently available key bindings:
>    (make-local-variable 'notmuch-search-oldest-first)
>    (make-local-variable 'notmuch-search-target-thread)
>    (make-local-variable 'notmuch-search-target-line)
> +  (make-local-variable 'notmuch-search-maxitems)
>    (set (make-local-variable 'notmuch-search-continuation) nil)
>    (set (make-local-variable 'scroll-preserve-screen-position) t)
>    (add-to-invisibility-spec 'notmuch-search)
> @@ -633,6 +642,8 @@ This function advances the next thread when finished."
>  			(insert "End of search results.")
>  			(if (not (= exit-status 0))
>  			    (insert (format " (process returned %d)" exit-status)))
> +			(if notmuch-search-maxitems
> +			    (notmuch-search-setup-buttons))

As discussed on IRC, this causes `notmuch-search' to fail if the
maxitems argument is nil.

> +(defun notmuch-search-setup-buttons ()
> +  (widget-insert "    ")
> +  (widget-create 'push-button
> +		 :notify (lambda (&rest ignore)
> +			   (set 'notmuch-search-maxitems
> +				(* 2 notmuch-search-maxitems))
> +			   (notmuch-search-refresh-view))
> +		 :help-echo "Double the number of messages shown"
> +		 "Show 2X messages")
> +  (widget-insert "    ")
> +  (widget-create 'push-button
> +		 :notify (lambda (&rest ignore)
> +			   (set 'notmuch-search-maxitems 0)
> +			   (notmuch-search-refresh-view))
> +		 :help-echo "Show all search results"
> +		 "Show unlimited messages")
> +  (widget-setup))

I think these notify-actions should be separate functions to make it
easier to bind them to keys.

> +
>  (defcustom notmuch-poll-script ""
>    "An external script to incorporate new mail into the notmuch database.
>  
> @@ -997,7 +1030,7 @@ current search results AND the additional query string provided."
>  			 query)))
>      (notmuch-search (if (string= notmuch-search-query-string "*")
>  			grouped-query
> -		      (concat notmuch-search-query-string " and " grouped-query)) notmuch-search-oldest-first)))
> +		      (concat notmuch-search-query-string " and "
>  			 grouped-query)) notmuch-search-oldest-first
>  			 notmuch-search-maxitems)))

This causes notmuch-search-filter to fail (repeatedly), since `notmuch-search'
expects a TARGET-THREAD (or nil) as its third parameter, but is given
`notmuch-search-maxitems' instead.

>  
>  (defun notmuch-search-filter-by-tag (tag)
>    "Filter the current search results based on a single tag.
> @@ -1006,7 +1039,7 @@ Runs a new search matching only messages that match both the
>  current search results AND that are tagged with the given tag."
>    (interactive
>     (list (notmuch-select-tag-with-completion "Filter by tag: ")))
> -  (notmuch-search (concat notmuch-search-query-string " and tag:" tag) notmuch-search-oldest-first))
> +  (notmuch-search (concat notmuch-search-query-string " and tag:"
>    tag) notmuch-search-oldest-first notmuch-search-maxitems))

Same here.

Cheers,
Daniel

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

Re: [RFC PATCH 2/3] cli: add support for limiting the number of search results

[Daniel Schoepe]

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

On Fri, 28 Oct 2011 23:59:30 +0300, Jani Nikula <jani@nikula.org> wrote:
> @@ -412,6 +413,14 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>  		fprintf (stderr, "Invalid value for --sort: %s\n", opt);
>  		return 1;
>  	    }
> +	} else if (STRNCMP_LITERAL (argv[i], "--maxitems=") == 0) {
> +	    const char *p;
> +	    opt = argv[i] + sizeof ("--maxitems=") - 1;
> +	    maxitems = strtoul(opt, &p, 10);

p should be of type `char *', not `const char *', as it will be
modified by strtoul. (Otherwise, gcc will produce a warning about this).

Cheers,
Daniel

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

Re: [RFC PATCH 2/3] cli: add support for limiting the number of search results

[Jani Nikula]

On Sat, 29 Oct 2011 19:30:50 +0200, Daniel Schoepe <daniel@schoepe.org> wrote:
> On Fri, 28 Oct 2011 23:59:30 +0300, Jani Nikula <jani@nikula.org> wrote:
> > @@ -412,6 +413,14 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
> >  		fprintf (stderr, "Invalid value for --sort: %s\n", opt);
> >  		return 1;
> >  	    }
> > +	} else if (STRNCMP_LITERAL (argv[i], "--maxitems=") == 0) {
> > +	    const char *p;
> > +	    opt = argv[i] + sizeof ("--maxitems=") - 1;
> > +	    maxitems = strtoul(opt, &p, 10);
> 
> p should be of type `char *', not `const char *', as it will be
> modified by strtoul. (Otherwise, gcc will produce a warning about this).

strtoul() won't touch the data pointed to by p (it only modifies p), so
in that sense it could be const, but you're right in that it really
should be 'char *', just for a more complicated reason. Thanks for
making me look it up: http://c-faq.com/ansi/constmismatch.html (not the
best of explanations, perhaps, but gives an idea why the 2nd parameter
of strtoul() can't be 'const char **').

BR,
Jani.

Re: [RFC PATCH 2/3] cli: add support for limiting the number of search results

[Daniel Schoepe]

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

On Sat, 29 Oct 2011 23:08:04 +0300, Jani Nikula <jani@nikula.org> wrote:
> strtoul() won't touch the data pointed to by p (it only modifies p), so
> in that sense it could be const, but you're right in that it really
> should be 'char *', just for a more complicated reason. Thanks for
> making me look it up: http://c-faq.com/ansi/constmismatch.html (not the
> best of explanations, perhaps, but gives an idea why the 2nd parameter
> of strtoul() can't be 'const char **').

Oh, right, thanks for the clarification. As I often do in situations
such as this, I confused "const char *" and "char const *".

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

Re: [RFC PATCH 3/3] emacs: support limiting the number of messages shown in search results

[Jani Nikula]

On Sat, 29 Oct 2011 19:25:22 +0200, Daniel Schoepe <daniel@schoepe.org> wrote:
> On Fri, 28 Oct 2011 23:59:31 +0300, Jani Nikula <jani@nikula.org> wrote:
> > Add support for limiting the maximum number of messages initially displayed
> > in search results. When enabled, the search results will contain push
> > buttons to double the number of messages displayed or to show unlimited
> > messages.
> 
> Nice patch, as it not only makes searches with a lot of results easier
> to use on slower machines/hard drives, but I also find that seeing only
> a few dozen threads in the buffer looks more "orderly" to me, compared
> to a buffer with hundreds of lines.

Thanks, I agree. Though having read your review below, it's not such a
nice patch after all. :)

Also, having chatted with amdragon on IRC, patches 1 and 2 should be
thought out better. Perhaps changing the lib is not the way to go after
all, even if that would help other lib users in paging the results. But
the whole thing was a quick proof of concept (hence "RFC") to get to see
how this patch would work out, and I'm glad you liked it.

> A few comments about the code:
> 
> > @@ -373,6 +381,7 @@ Complete list of currently available key bindings:
> >    (make-local-variable 'notmuch-search-oldest-first)
> >    (make-local-variable 'notmuch-search-target-thread)
> >    (make-local-variable 'notmuch-search-target-line)
> > +  (make-local-variable 'notmuch-search-maxitems)
> >    (set (make-local-variable 'notmuch-search-continuation) nil)
> >    (set (make-local-variable 'scroll-preserve-screen-position) t)
> >    (add-to-invisibility-spec 'notmuch-search)
> > @@ -633,6 +642,8 @@ This function advances the next thread when finished."
> >  			(insert "End of search results.")
> >  			(if (not (= exit-status 0))
> >  			    (insert (format " (process returned %d)" exit-status)))
> > +			(if notmuch-search-maxitems
> > +			    (notmuch-search-setup-buttons))
> 
> As discussed on IRC, this causes `notmuch-search' to fail if the
> maxitems argument is nil.

And as you pointed out, the parameter is optional so it must accept nil.

> > +(defun notmuch-search-setup-buttons ()
> > +  (widget-insert "    ")
> > +  (widget-create 'push-button
> > +		 :notify (lambda (&rest ignore)
> > +			   (set 'notmuch-search-maxitems
> > +				(* 2 notmuch-search-maxitems))
> > +			   (notmuch-search-refresh-view))
> > +		 :help-echo "Double the number of messages shown"
> > +		 "Show 2X messages")
> > +  (widget-insert "    ")
> > +  (widget-create 'push-button
> > +		 :notify (lambda (&rest ignore)
> > +			   (set 'notmuch-search-maxitems 0)
> > +			   (notmuch-search-refresh-view))
> > +		 :help-echo "Show all search results"
> > +		 "Show unlimited messages")
> > +  (widget-setup))
> 
> I think these notify-actions should be separate functions to make it
> easier to bind them to keys.

It's obvious now that you say it! :)

> > +
> >  (defcustom notmuch-poll-script ""
> >    "An external script to incorporate new mail into the notmuch database.
> >  
> > @@ -997,7 +1030,7 @@ current search results AND the additional query string provided."
> >  			 query)))
> >      (notmuch-search (if (string= notmuch-search-query-string "*")
> >  			grouped-query
> > -		      (concat notmuch-search-query-string " and " grouped-query)) notmuch-search-oldest-first)))
> > +		      (concat notmuch-search-query-string " and "
> >  			 grouped-query)) notmuch-search-oldest-first
> >  			 notmuch-search-maxitems)))
> 
> This causes notmuch-search-filter to fail (repeatedly), since `notmuch-search'
> expects a TARGET-THREAD (or nil) as its third parameter, but is given
> `notmuch-search-maxitems' instead.

Oh, yes, totally broken.

> >  
> >  (defun notmuch-search-filter-by-tag (tag)
> >    "Filter the current search results based on a single tag.
> > @@ -1006,7 +1039,7 @@ Runs a new search matching only messages that match both the
> >  current search results AND that are tagged with the given tag."
> >    (interactive
> >     (list (notmuch-select-tag-with-completion "Filter by tag: ")))
> > -  (notmuch-search (concat notmuch-search-query-string " and tag:" tag) notmuch-search-oldest-first))
> > +  (notmuch-search (concat notmuch-search-query-string " and tag:"
> >    tag) notmuch-search-oldest-first notmuch-search-maxitems))
> 
> Same here.

Yup.

Many thanks for the review. I'll fix these for myself so I might send a
v2 just as well.


BR,
Jani.