From: Mark Walters <markwalters1009@gmail.com>
To: Jameson Graef Rollins <jrollins@finestructure.net>,
	Notmuch Mail <notmuch@notmuchmail.org>
Subject: Re: [PATCH 5/8] emacs: modify search tag functions to use new
	notmuch-tag interface
In-Reply-To: <1333845338-22960-6-git-send-email-jrollins@finestructure.net>
References: <1333354853-25729-1-git-send-email-jrollins@finestructure.net>
	<1333845338-22960-1-git-send-email-jrollins@finestructure.net>
	<1333845338-22960-2-git-send-email-jrollins@finestructure.net>
	<1333845338-22960-3-git-send-email-jrollins@finestructure.net>
	<1333845338-22960-4-git-send-email-jrollins@finestructure.net>
	<1333845338-22960-5-git-send-email-jrollins@finestructure.net>
	<1333845338-22960-6-git-send-email-jrollins@finestructure.net>
User-Agent: Notmuch/0.12+111~g5c30f66 (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Mon, 09 Apr 2012 19:39:43 +0100
Message-ID: <871unwn40g.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

On Sun, 08 Apr 2012, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> The main change here is to modify argument parsing so as to not force
> tag-changes to be a list, and to let notmuch-tag handle prompting the
> user when required.  doc strings are also updated and cleaned up.
> ---
>  emacs/notmuch.el |   36 +++++++++++++-----------------------
>  1 files changed, 13 insertions(+), 23 deletions(-)
>
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 9aec96d..a03a526 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -523,13 +523,10 @@ and will also appear in a buffer named \"*Notmuch errors*\"."
>  See `notmuch-search-tag-region' for details."
>    (apply 'notmuch-search-tag-region (point) (point) tag-changes))
>  
> -(defun notmuch-search-tag-region (beg end &rest tag-changes)
> -  "Change tags for threads in the given region.
> -
> -TAGS is a list of tag operations for `notmuch-tag'.  The tags are
> -added or removed for all threads in the region from BEG to END."
> +(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)))
> -    (apply 'notmuch-tag search-string tag-changes)
> +    (setq tag-changes (funcall 'notmuch-tag search-string tag-changes))
>      (save-excursion
>        (let ((last-line (line-number-at-pos end))
>  	    (max-line (- (line-number-at-pos (point-max)) 2)))
> @@ -539,14 +536,14 @@ added or removed for all threads in the region from BEG to END."
>  	   (notmuch-update-tags (notmuch-search-get-tags) tag-changes))
>  	  (forward-line))))))
>  
> -(defun notmuch-search-tag (&optional initial-input)
> -  "Change tags for the currently selected thread or region."
> +(defun notmuch-search-tag (&optional tag-changes)
> +  "Change tags for the currently selected thread or region.
> +
> +See `notmuch-tag' for information on the format of TAG-CHANGES."
>    (interactive)
>    (let* ((beg (if (region-active-p) (region-beginning) (point)))
> -	 (end (if (region-active-p) (region-end) (point)))
> -	 (search-string (notmuch-search-find-thread-id-region-search beg end))
> -	 (tags (notmuch-read-tag-changes initial-input search-string)))
> -    (apply 'notmuch-search-tag-region beg end tags)))
> +	 (end (if (region-active-p) (region-end) (point))))
> +    (funcall 'notmuch-search-tag-region beg end tag-changes)))
>  
>  (defun notmuch-search-add-tag ()
>    "Same as `notmuch-search-tag' but sets initial input to '+'."
> @@ -790,18 +787,11 @@ non-authors is found, assume that all of the authors match."
>  	      (goto-char found-target)))
>        (delete-process proc))))
>  
> -(defun notmuch-search-tag-all (&rest tag-changes)
> -  "Add/remove tags from all matching messages.
> +(defun notmuch-search-tag-all (&optional tag-changes)
> +  "Add/remove tags from all messages in current search buffer.
>  
> -This command adds or removes tags from all messages matching the
> -current search terms. When called interactively, this command
> -will prompt for tags to be added or removed. Tags prefixed with
> -'+' will be added and tags prefixed with '-' will be removed.
> -
> -Each character of the tag name may consist of alphanumeric
> -characters as well as `_.+-'.
> -"
> -  (interactive (notmuch-read-tag-changes))
> +See `notmuch-tag' for information on the format of TAG-CHANGES."
> +  (interactive)
>    (apply 'notmuch-tag notmuch-search-query-string tag-changes))

My only comment here is that I wonder about the tag-all command. I can
see that this is a performance win over applying tag region to the whole
buffer but is it another place where we have a race with incoming mail
(ie we tag everything that now matches rather than everything in the
buffer).

A second advantage with the tag-region form is that it automatically
does excludes. Currently notmuch-tag does not do excludes as it looked
very hairy to implement correctly: thus the tag-all command could spend
a lot of time tagging excluded messages.

OTOH this seems to be a "feature" of the existing code so a "fix" is not
required for this series.

(Note I would not consider this a full review as my lisp is not good
enough; eg I am unsure  why the change from apply to funcall was
needed).

Best wishes

Mark

>  
>  (defun notmuch-search-buffer-title (query)
> -- 
> 1.7.9.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch