From: Carl Worth <cworth@cworth.org>
To: Matthieu Lemerre <racin@free.fr>, notmuch@notmuchmail.org
Subject: Re: [PATCH] Support for deletion (patch included)
Date: Wed, 24 Feb 2010 10:49:16 -0800 [thread overview]
Message-ID: <87tyt6wjtf.fsf@yoom.home.cworth.org> (raw)
In-Reply-To: <87y6l7144e.fsf@free.fr>
[-- 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 --]
next prev parent reply other threads:[~2010-02-24 18:49 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-12-13 11:54 [PATCH] Support for deletion (patch included) Matthieu Lemerre
2010-02-24 18:49 ` Carl Worth [this message]
2010-02-25 0:00 ` racin
2010-02-25 10:49 ` Sebastian Spaeth
2010-03-01 9:09 ` Michal Sojka
[not found] <829811857.5353531267112884804.JavaMail.root@zimbra1-e1.priv.proxad.net>
2010-02-25 15:51 ` racin
[not found] <1427711643.5760731267266834921.JavaMail.root@zimbra1-e1.priv.proxad.net>
2010-02-27 10:34 ` racin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://notmuchmail.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87tyt6wjtf.fsf@yoom.home.cworth.org \
--to=cworth@cworth.org \
--cc=notmuch@notmuchmail.org \
--cc=racin@free.fr \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://yhetil.org/notmuch.git/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).