Re: [PATCH] Support for deletion (patch included)

[Carl Worth <cworth@cworth.org>]

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

On Sun, 13 Dec 2009 12:54:09 +0100, Matthieu Lemerre <racin@free.fr> wrote:
> I forgot the attachment..

Hi Matthieu,

This is a very interesting patch. Thanks for contributing it.

Could you also write a commit message describing what the patch does?
The easiest way for me to apply that would be if you would create a git
commit, then run "git format-patch origin/master" and mail the resulting
files, (the "git send-email" command can be used here, or you can insert
the files into a mail-composition buffer and modify them as needed).

A couple of minor comments on the patch:

>      (define-key map "a" 'notmuch-search-archive-thread)
> +    (define-key map "d" 'notmuch-search-mark-as-deleted)

For consistency, let's name this notmuch-search-delete-thread.

And we'll probably want a notmuch-show-delete-message function as well,
no?

> +(defvar notmuch-search-history nil)

Excellent! I've wanted custom search history for a while, and just
didn't know how to do it with (interactive "s"). It looks easy enough
with read-string as you're doing here. But this is independent
functionality, so would be preferred as an independent patch/commit.

>    (forward-line))
>  
> +
> +(defun notmuch-search-mark-as-deleted ()
> +  "Mark the currently selected thread as deleted (set its \"deleted\" tag).
> +This function advances the next thread when finished."
> +  (interactive)
> +  (notmuch-search-add-tag "deleted")
> +  (forward-line))
> +
> +
>  (defun notmuch-search-process-sentinel (proc msg)

Watch that extra whitespace. The convention is a single line of
whitespace between each function.

And should we also archive the thread before removing the deleted tag?

> +With prefix argument, include deleted items.

That's a pretty good interface I think.

Another approach would be to do something like what sup does---that
would be to scan the search terms and if it contains "tag:deleted" and
all then don't prepend the "not tag:deleted and" to the search
string. The assumption there is that if the user is explicitly
mentioning the deleted tag, then we should just rely on the user to
explicitly describe how the tag should be treated.

That's perhaps not an unreasonable heuristic, and might be done even in
addition to the prefix-argument approach. But that could be an
additional commit, and I won't require it.

> +  (interactive (let* ((prefix current-prefix-arg)
> +		      (query (if prefix
> +				 (read-string "Notmuch search (including deleted): "
> +					      notmuch-search-query-string
> +					      'notmuch-search-history)
> +			       (read-string "Notmuch search: " nil
> +					    'notmuch-search-history))))

Why is the second (initial-input) argument non-nil in one case, but nil
in the other? The documentation for `read-string' says the argument is
deprecated and should be nil in all new code.

> +		 (list query nil prefix)))
> +  (let ((real-query (if include-deleted query 
> +		      (concat "not tag:deleted and (" query ")")))
> +	(buffer (get-buffer-create (concat "*notmuch-search-" query
> "*"))))

Does the include-deleted case actually work? I don't see anything in the
code that sets this variable. (I'm just reviewing here--I haven't tested
it manually).

> @@ -1351,7 +1374,6 @@ search."
>  
>  Runs a new search matching only messages that match both the
>  current search results AND the additional query string provided."
> -  (interactive "sFilter search: ")
>    (let ((grouped-query (if (string-match-p notmuch-search-disjunctive-regexp query) (concat "( " query " )") query)))
>      (notmuch-search (concat notmuch-search-query-string " and " grouped-query) notmuch-search-oldest-first)))

Is this just an accidental chunk in the patch? I don't see why this
function should become non-interactive now.

Thanks again,

-Carl

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