Date: Fri, 4 Nov 2011 16:54:15 -0400
From: Austin Clements <amdragon@MIT.EDU>
To: Jani Nikula <jani@nikula.org>
Subject: Re: [RFC PATCH v2 3/3] emacs: support limiting the number of
	results shown in search results
Message-ID: <20111104205415.GJ22935@mit.edu>
References: <cover.1320093940.git.jani@nikula.org>
	<0b937eb1103dbb24142a783eff599a7cb08f195e.1320093940.git.jani@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To:
 <0b937eb1103dbb24142a783eff599a7cb08f195e.1320093940.git.jani@nikula.org>
User-Agent: Mutt/1.5.20 (2009-06-14)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth Jani Nikula on Oct 31 at 11:18 pm:
> Add support for limiting the maximum number of results initially displayed
> in search results. When enabled, the search results will contain push
> buttons to double the number of results displayed or to show unlimited
> results.
> 
> The approach is inspired by vc-print-log in Emacs vc.el.
> 
> Signed-off-by: Jani Nikula <jani@nikula.org>
> ---
>  emacs/notmuch-hello.el |   17 ++++++++++++--
>  emacs/notmuch.el       |   53 +++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 62 insertions(+), 8 deletions(-)
> 
> diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
> index 65fde75..4ba13e3 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 maxitems target-thread target-line continuation))
>  (declare-function notmuch-poll "notmuch" ())
>  
>  (defvar notmuch-hello-search-bar-marker nil
> @@ -37,6 +37,17 @@
>    :type 'integer
>    :group 'notmuch)
>  
> +(defcustom notmuch-search-maxitems 0
> +  "The maximum number of results to show in search results.
> +
> +This variables controls the maximum number of results to
> +initially show in search results. Set to 0 to not limit the
> +number of results. If non-zero, the search results will contain
> +push buttons to double the number (can be repeated) or show
> +unlimited number of results."
> +  :type 'integer
> +  :group 'notmuch)
> +

It would be more lispy to use nil to indicate no limit, rather than 0.
The type could be something like
  (choice (const   :tag "Unlimited" nil)
          (integer :tag "Limit"))

>  (defcustom notmuch-show-empty-saved-searches nil
>    "Should saved searches with no messages be listed?"
>    :type 'boolean
> @@ -151,7 +162,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 notmuch-search-maxitems nil nil #'notmuch-hello-search-continuation)))

It might make more sense of notmuch-search to use the value of
notmuch-search-maxitems directly, rather than feeding it in
everywhere, especially since it's buffer-local.  If there's really a
place that needs to override it, it would be easy to let-bind the
variable there.

>  
>  (defun notmuch-hello-add-saved-search (widget)
>    (interactive)
> @@ -200,7 +211,7 @@ diagonal."
>  (defun notmuch-hello-widget-search (widget &rest ignore)
>    (notmuch-search (widget-get widget
>  			      :notmuch-search-terms)
> -		  notmuch-search-oldest-first
> +		  notmuch-search-oldest-first notmuch-search-maxitems
>  		  nil nil #'notmuch-hello-search-continuation))
>  
>  (defun notmuch-saved-search-count (search)
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index f11ec24..741ebe1 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))))

Huh.  I thought widgets automatically had local keymaps, but maybe
that's only buttons (insert-button type buttons, that is).  A local
keymap is probably the right way to do this regardless.  Or use
buttons instead of widgets.

>      (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,11 @@ 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 (and notmuch-search-maxitems
> +				 (< 0 notmuch-search-maxitems)
> +				 (< notmuch-search-maxitems
> +				    (count-lines (point-min) (point-max))))
> +			    (notmuch-search-setup-buttons))
>  			(insert "\n")
>  			(if (and atbob
>  				 (not (string= notmuch-search-target-thread "found")))
> @@ -883,7 +897,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 maxitems target-thread target-line continuation)
>    "Run \"notmuch search\" with the given query string and display results.
>  
>  The optional parameters are used as follows:
> @@ -899,6 +913,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 +931,11 @@ The optional parameters are used as follows:
>  		     (if oldest-first
>  			 "--sort=oldest-first"
>  		       "--sort=newest-first")
> +		     (if (and maxitems (< 0 maxitems))
> +			 (if oldest-first
> +			     (format "--first=-%d" maxitems)
> +			   (format "--maxitems=%d" maxitems))
> +		       "")
>  		     query)))
>  	  (set-process-sentinel proc 'notmuch-search-process-sentinel)
>  	  (set-process-filter proc 'notmuch-search-process-filter))))
> @@ -932,13 +952,36 @@ 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 maxitems target-thread target-line continuation)
>      (goto-char (point-min))))
>  
> +(defun notmuch-search-double-results (&rest ignore)
> +  (if notmuch-search-maxitems
> +      (set 'notmuch-search-maxitems (* 2 notmuch-search-maxitems)))
> +  (notmuch-search-refresh-view))
> +
> +(defun notmuch-search-unlimited-results (&rest ignore)
> +  (set 'notmuch-search-maxitems nil)
> +  (notmuch-search-refresh-view))

I would recommend setq instead of set, except in the weird local
places where `set' is clearly the prevailing style.

> +
> +(defun notmuch-search-setup-buttons ()
> +  (widget-insert "    ")
> +  (widget-create 'push-button
> +		 :notify 'notmuch-search-double-results
> +		 :help-echo "Double the number of results shown"
> +		 "Show 2X results")
> +  (widget-insert "    ")
> +  (widget-create 'push-button
> +		 :notify 'notmuch-search-unlimited-results
> +		 :help-echo "Show all search results"
> +		 "Show unlimited results")
> +  (widget-setup))
> +

If you're limiting from the bottom, would it make more sense for these
widgets to be placed at the top of the buffer, since that's the
direction they'll expand the results?

>  (defcustom notmuch-poll-script ""
>    "An external script to incorporate new mail into the notmuch database.
>  
> @@ -997,7 +1040,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 +1049,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 ()

-- 
Austin Clements                                      MIT/'06/PhD/CSAIL
amdragon@mit.edu                           http://web.mit.edu/amdragon
       Somewhere in the dream we call reality you will find me,
              searching for the reality we call dreams.