From: Jani Nikula <jani@nikula.org>
To: Daniel Schoepe <daniel@schoepe.org>, notmuch@notmuchmail.org
Subject: Re: [RFC PATCH 3/3] emacs: support limiting the number of messages
	shown in search results
In-Reply-To: <87pqhfk8vx.fsf@gilead.invalid>
References: <cover.1319833617.git.jani@nikula.org>
	<0290050284e4cb3a6f5ef0eb3582319f8d52ccf2.1319833617.git.jani@nikula.org>
	<87pqhfk8vx.fsf@gilead.invalid>
User-Agent: Notmuch/0.9-20-gc4362a8 (http://notmuchmail.org) Emacs/23.3.1
	(i686-pc-linux-gnu)
Date: Sat, 29 Oct 2011 23:28:14 +0300
Message-ID: <8762j7r19d.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: amdragon@mit.edu
Precedence: list

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.