From: Aaron Ecay <aaronecay@gmail.com>
To: Jameson Graef Rollins <jrollins@finestructure.net>,
	Notmuch Mail <notmuch@notmuchmail.org>
Subject: Re: [PATCH 2/4] emacs: repurpose notmuch-show-archive-thread-internal
	function for general thread tagging
In-Reply-To: <1325975294-646-3-git-send-email-jrollins@finestructure.net>
References: <1325975294-646-1-git-send-email-jrollins@finestructure.net>
	<1325975294-646-2-git-send-email-jrollins@finestructure.net>
	<1325975294-646-3-git-send-email-jrollins@finestructure.net>
User-Agent: Notmuch/0.10.1+56~gd709fd6 (http://notmuchmail.org)
	Emacs/24.0.92.3 (i386-apple-darwin10.8.0)
Date: Sun, 08 Jan 2012 20:08:59 -0500
Message-ID: <m239bpk7h0.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: list

Jameson,

Some comments below:

On Sat,  7 Jan 2012 14:28:12 -0800, Jameson Graef Rollins <jrollins@finestr=
ucture.net> wrote:
> Instead of having a function that is only used for archiving a thread,
> we instead make it useful for any tagging operation.  The new
> function, notmuch-show-tag-thread-internal, now takes two more
> arguments, for the "sign" of the tagging operation ("-" or "+"), and
> the tag to be added or removed.  This will allow this function to be
> used for any generic thread tagging operation.
>=20
> The higher level functions that call this function are modified
> accordingly.
> ---
>  emacs/notmuch-show.el |   34 ++++++++++++++++++++--------------
>  1 files changed, 20 insertions(+), 14 deletions(-)
>=20
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 5502efd..1e16f05 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -1414,20 +1414,26 @@ argument, hide all of the messages."
>    (interactive)
>    (backward-button 1))
>=20=20
> -(defun notmuch-show-archive-thread-internal (show-next)
> +(defun notmuch-show-tag-thread-internal (sign tag show-next)

A couple of comments on the arguments:
- It would be good to make show-next &optional.  This will enable code
  to call the fn with only two arguments, and not showing next will be
  the default behavior.
- A more lispy way of specifying the sign would be to use a
  boolean.  Perhaps you could call this =E2=80=9Cremove=E2=80=9D; a value o=
f =E2=80=98t=E2=80=99 would
  remove the tag; =E2=80=98nil=E2=80=99 would add it.  Moving this argument=
 after =E2=80=98tag=E2=80=99
  and also making it &optional woudl allow this fn to be called with one
  arg to add a tag.  (Maybe this is too minimalist and API, however.)=20

>    ;; Remove the tag from the current set of messages.
>    (goto-char (point-min))
> -  (loop do (notmuch-show-remove-tag "inbox")
> -	until (not (notmuch-show-goto-message-next)))
> -  ;; Move to the next item in the search results, if any.
> -  (let ((parent-buffer notmuch-show-parent-buffer))
> -    (notmuch-kill-this-buffer)
> -    (if parent-buffer
> -	(progn
> -	  (switch-to-buffer parent-buffer)
> -	  (forward-line)
> -	  (if show-next
> -	      (notmuch-search-show-thread))))))
> +  (let ((tag-function))

No second set of parens is needed around tag-function.

> +    (cond
> +     ((string=3D sign "-")
> +      (setq tag-function 'notmuch-show-remove-tag))
> +     ((string=3D sign "+")
> +      (setq tag-function 'notmuch-show-add-tag)))
> +    (loop do (funcall tag-function tag)
> +	  until (not (notmuch-show-goto-message-next)))
> +    ;; Move to the next item in the search results, if any.

Does it make sense to separate the tagging and the movement?  It seems
plausible that some code somewhere might want to add/remove a tag from
all messages in the thread w/o changing the display.

> +    (let ((parent-buffer notmuch-show-parent-buffer))
> +      (notmuch-kill-this-buffer)
> +      (if parent-buffer
> +	  (progn
> +	    (switch-to-buffer parent-buffer)
> +	    (forward-line)
> +	    (if show-next
> +		(notmuch-search-show-thread)))))))

--=20
Aaron Ecay