[notmuch] [PATCH] Support for deletion (patch included)
racin at free.fr
racin at free.fr
Thu Feb 25 07:51:59 PST 2010
Carl: The patch in the mail has problems; apparently I have to manually add scissorlines to the mail for it
to be processed by git-am. I thought this was automatically added. (I hate the git UI -- nothing is consistent,
concepts have different names, the definition of scissor lines is as precise as "A line that mainly consists of scissors (either ">8" or "8<") and perforation (dash "-") --, but I guess we can get used to it after a while...)
I'll send you a proper patch as soon as I can. Meanwhile, I'm sure you have comments on this updated patch!
Matthieu
À: "Carl Worth" <cworth at cworth.org>
Cc: notmuch at notmuchmail.org
Envoyé: Jeudi 25 Février 2010 00h00:04 GMT +00:00 GMT - Grande-Bretagne, Irlande, Portugal
Objet: Re: [notmuch] [PATCH] Support for deletion (patch included)
Hi Carl,
> 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).
>
OK, here it is (comments below). I had trouble splitting the patches into a patch series; I
found git add -p, but isn't there a better interface for selecting patches?
>From bdee9558d93bffb97c80632f522288e059deb7c2 Mon Sep 17 00:00:00 2001
From: Matthieu Lemerre <racin at racin.rez-gif.supelec.fr>
Date: Thu, 25 Feb 2010 00:24:24 +0100
Subject: [PATCH 1/2] Add and use notmuch-show-forall-in-thread macro
---
notmuch.el | 17 +++++++++++------
1 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/notmuch.el b/notmuch.el
index 6482170..5d7342a 100644
--- a/notmuch.el
+++ b/notmuch.el
@@ -321,17 +321,22 @@ pseudoheader summary"
(cons (notmuch-show-get-message-id) nil)))
(notmuch-show-set-tags (sort (set-difference tags toremove :test 'string=) 'string<))))))
-(defun notmuch-show-archive-thread-maybe-mark-read (markread)
- (save-excursion
+(defmacro notmuch-show-forall-in-thread (&rest body)
+ "Executes BODY with point in all messages of the current thread."
+ `(save-excursion
(goto-char (point-min))
(while (not (eobp))
- (if markread
- (notmuch-show-remove-tag "unread" "inbox")
- (notmuch-show-remove-tag "inbox"))
+ , at body
(if (not (eobp))
(forward-char))
(if (not (re-search-forward notmuch-show-message-begin-regexp nil t))
- (goto-char (point-max)))))
+ (goto-char (point-max))))))
+
+(defun notmuch-show-archive-thread-maybe-mark-read (markread)
+ (notmuch-show-forall-in-thread
+ (if markread
+ (notmuch-show-remove-tag "unread" "inbox")
+ (notmuch-show-remove-tag "inbox")))
(let ((parent-buffer notmuch-show-parent-buffer))
(kill-this-buffer)
(if parent-buffer
--
1.6.5
This first patch is helpful for factorizing out code. Basically, it allows to
apply a "message-only" command to all the thread.
>From 0073152e3fa7dd11d88de28e87eec7762cdbbbeb Mon Sep 17 00:00:00 2001
From: Matthieu Lemerre <racin at racin.rez-gif.supelec.fr>
Date: Thu, 25 Feb 2010 00:25:51 +0100
Subject: [PATCH 2/2] Add support for deletion in the emacs interface
---
notmuch.el | 56 +++++++++++++++++++++++++++++++++++++++++++++++++-------
1 files changed, 49 insertions(+), 7 deletions(-)
diff --git a/notmuch.el b/notmuch.el
index 5d7342a..0285573 100644
--- a/notmuch.el
+++ b/notmuch.el
@@ -92,6 +92,8 @@
(define-key map "x" 'notmuch-show-archive-thread-then-exit)
(define-key map "A" 'notmuch-show-mark-read-then-archive-thread)
(define-key map "a" 'notmuch-show-archive-thread)
+ (define-key map "d" 'notmuch-show-delete-thread)
+ (define-key map "D" 'notmuch-show-delete-message)
(define-key map "p" 'notmuch-show-previous-message)
(define-key map "N" 'notmuch-show-mark-read-then-next-open-message)
(define-key map "n" 'notmuch-show-next-message)
@@ -380,6 +382,23 @@ buffer."
(notmuch-show-archive-thread)
(kill-this-buffer))
+(defun notmuch-show-delete-message ()
+ "Delete current message (sets its deleted tag)."
+ (interactive)
+ (notmuch-show-add-tag "deleted"))
+
+(defun notmuch-show-delete-thread()
+ "Delete each message in thread."
+ (interactive)
+ (notmuch-show-forall-in-thread
+ (notmuch-show-delete-message)))
+
+(defun notmuch-show-delete-thread-and-exit()
+ "Delete each message in thread, then exit back to search results."
+ (interactive)
+ (notmuch-show-delete-thread)
+ (kill-this-buffer))
+
(defun notmuch-show-mark-read-then-archive-then-exit ()
"Remove unread tags from thread, then archive and exit to search results."
(interactive)
@@ -1227,6 +1246,7 @@ matching this search term are shown if non-nil. "
(define-key map [mouse-1] 'notmuch-search-show-thread)
(define-key map "*" 'notmuch-search-operate-all)
(define-key map "a" 'notmuch-search-archive-thread)
+ (define-key map "d" 'notmuch-search-delete-thread)
(define-key map "-" 'notmuch-search-remove-tag)
(define-key map "+" 'notmuch-search-add-tag)
(define-key map (kbd "RET") 'notmuch-search-show-thread)
@@ -1235,6 +1255,7 @@ matching this search term are shown if non-nil. "
(fset 'notmuch-search-mode-map notmuch-search-mode-map)
(defvar notmuch-search-query-string)
+(defvar notmuch-search-history nil)
(defvar notmuch-search-oldest-first t
"Show the oldest mail first in the search-mode")
@@ -1446,6 +1467,13 @@ This function advances the next thread when finished."
(notmuch-search-remove-tag "inbox")
(forward-line))
+(defun notmuch-search-delete-thread ()
+ "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)
"Add a message to let user know when \"notmuch search\" exits"
(let ((buffer (process-buffer proc))
@@ -1520,10 +1548,22 @@ characters as well as `_.+-'.
(append action-split (list notmuch-search-query-string) nil))))
;;;###autoload
-(defun notmuch-search (query &optional oldest-first)
- "Run \"notmuch search\" with the given query string and display results."
- (interactive "sNotmuch search: ")
- (let ((buffer (get-buffer-create (concat "*notmuch-search-" query "*"))))
+(defun notmuch-search (query &optional oldest-first include-deleted)
+ "Run \"notmuch search\" with the given query string and display results.
+
+With prefix argument, include deleted items.
+"
+ (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))))
+ (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 "*"))))
(switch-to-buffer buffer)
(notmuch-search-mode)
(set 'notmuch-search-query-string query)
@@ -1539,7 +1579,7 @@ characters as well as `_.+-'.
(let ((proc (start-process-shell-command
"notmuch-search" buffer notmuch-command "search"
(if oldest-first "--sort=oldest-first" "--sort=newest-first")
- (shell-quote-argument query))))
+ (shell-quote-argument real-query))))
(set-process-sentinel proc 'notmuch-search-process-sentinel)
(set-process-filter proc 'notmuch-search-process-filter))))
(run-hooks 'notmuch-search-hook)))
@@ -1587,7 +1627,7 @@ search."
Runs a new search matching only messages that match both the
current search results AND the additional query string provided."
- (interactive "sFilter search: ")
+ (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)))
@@ -1630,7 +1670,9 @@ current search results AND that are tagged with the given tag."
(fset 'notmuch-folder-mode-map notmuch-folder-mode-map)
-(defcustom notmuch-folders (quote (("inbox" . "tag:inbox") ("unread" . "tag:unread")))
+(defcustom notmuch-folders (quote (("inbox" . "tag:inbox")
+ ("unread" . "tag:unread")
+ ("deleted" . "tag:deleted")))
"List of searches for the notmuch folder view"
:type '(alist :key-type (string) :value-type (string))
:group 'notmuch)
--
1.6.5
This second patch is the reworked patch, after your comments.
> 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?
Did both. I have however a question: how to handle in the UI the
thread/message distinction? For now, I set "d" in notmuch-show buffer
to act as "delete the current thread", and "D" as "delete the current
message". I thought that as "d" meant in notmuch summary "delete the
current thread", the bindings would be more consistent. But now I
wonder if this may be counter intuitive, and if we should use "d" for
"delete current message" and "D" for "delete current thread". What do
people think?
It would be nice to have, in general a convenient way to handle the
thread/message distinction in the UI (e.g. for applying other tags).
>
> > +(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.
In fact I needed this; that way when you do "C-u s", the previous
search is shown, and to include deleted items you just have to do "C-u
s RET". So I made them in a single commit; but I can split them if
you really want to.
>
> > (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.
>
My bad. I did not see this in my review. Corrected this in the updated patch.
> And should we also archive the thread before removing the deleted tag?
I don't understand this. Do you mean: should we also archive the
thread before setting the deleted tag? I don't known why we should
bother to do it, as the tag is not show by the next results.
I would tend to think that archiving should be an explicit action,
that should call a hook. For instance, I'd like my mailflow to be like
this:
- get mail with offlineimap => a maildir directory, indexed by notmuch
- mails stay in inbox until I archive it. That way I can still access
it with IMAP.
- archiving puts an "archived" tag (and removes the inbox and unread
tags), and move the mail to another directory (or uses git to
archive the mail).
- deleting puts a deleted tag (but the messages stays inbox as long as
I do not explicitely remove this).
The main reason is that many mail servers only allow small quotas, so
you have to carefully select which mail should stay inbox (only the
mail that you need external access).
In brief, this mail flow needs to delete threads but not to archive
it. I think that we could provide hooks, so that you can archive
mails when you delete them if you like to.
By the way in my patch commentary, you said that you planned to change
the "inbox" and "unread" treatments to be more explicit. Do you still
plan to do that? I can maybe also provide a patch.
> > +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.
Hmm, that's what I did want to do initially. In that case, I think
that notmuch should be able to output parse trees from notmuch search
strings as JSON; this would help do the parsing in the various
implementations.
I also had this idea that may solve the "tag should be
orthogonal/command-line UI problem". We observe that "archived" is
contrary to "inbox", "read" contrary to "unread", "deleted" contrary
to "undeleted", and so on.
So it may be a good idea to have "pseudo-tags", that would help in the
interface (searching archived mails with search not tag:inbox is not
very intuitive...), as synonyms for "not tag:countrary-tag"
Then for each "pseudo-tag", notmuch could hard-code which is included
by default. Adding the other tags is easy: just add "and tag:deleted"
or "and tag:read" or "and tag:archived". Moreover, this would simplify
UI implementation as well, without requiring parsing of the strings.
Another, even simpler possibility would be to add a "--all" flag to
notmuch. Without it, it would omit some of the tags (like the deleted
ones), which would be convenient as a text interface. With it, it
would show all tags, which would be convenient for use with scripts
and UI, and mor in the "tags are orthogonal" spirit. Again, this would
not require to parse search strings, and imho would be much cleaner.
> > + (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.
The idea is that you do not have default search terms if you do
regular searchs. But if you want to include deleted items, then you
have the last search string. This way, you handle the common case when
you serach for a term, do not find it, then decide to try again with
deleted items.
Now, maybe the C-u interface is not such a good idea, and a toggle
button would be better. Don't know, but I find this interface
convenient.
>
> > + (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).
This is the way interactive work.
(defun notmuch-search (query &optional oldest-first include-deleted)
...
+ (interactive (let* ((prefix current-prefix-arg)
...
+ (list query nil prefix)))
The (list query nil prefix) at the end of the (interactive) form will
set query to query, nil to oldest-first, and prefix to
include-deleted.
>
> > @@ -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.
My bad, corrected that also.
Matthieu
_______________________________________________
notmuch mailing list
notmuch at notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
More information about the notmuch
mailing list