add delete keybinding

add delete keybinding

[Dirk Hohndel]

Straight forward addition to the Emacs UI. The 'd' keybinding
is implemented very similar to the 'a' keybinding - it only
adds a +deleted tag as well. This tag is used by notmuchsync
to delete (-p "prune") files in the mailstore.

I'm sending this mostly as an RFC - I use this and like it, but
people seem to have strong feelings as to how they want to deal 
with deleting email (or for some people, how they don't want to
do that at all).

/D

[PATCH] Add 'd'elete keybinding to search and show views

[Dirk Hohndel]

This is a variation of the 'a'rchive binding - it additionally sets
the "deleted" tag (which notmuchsync uses to trigger pruning of files)

Signed-off-by: Dirk Hohndel <hohndel@infradead.org>
---
 emacs/notmuch-show.el |   26 ++++++++++++++++++++++----
 emacs/notmuch.el      |   10 ++++++++++
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 916b39e..45d1abe 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -520,6 +520,7 @@ function is used. "
 	(define-key map "+" 'notmuch-show-add-tag)
 	(define-key map "x" 'notmuch-show-archive-thread-then-exit)
 	(define-key map "a" 'notmuch-show-archive-thread)
+	(define-key map "d" 'notmuch-show-delete-thread)
 	(define-key map "N" 'notmuch-show-next-message)
 	(define-key map "P" 'notmuch-show-previous-message)
 	(define-key map "n" 'notmuch-show-next-open-message)
@@ -910,10 +911,13 @@ to stdout or stderr will appear in the *Messages* buffer."
   (interactive)
   (backward-button 1))
 
-(defun notmuch-show-archive-thread-internal (show-next)
+(defun notmuch-show-archive-or-delete-thread-internal (show-next delete)
   ;; Remove the tag from the current set of messages.
   (goto-char (point-min))
-  (loop do (notmuch-show-remove-tag "inbox")
+  (loop do (progn
+	     (notmuch-show-remove-tag "inbox")
+	     (if delete
+		 (notmuch-show-add-tag "deleted")))
 	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))
@@ -925,6 +929,20 @@ to stdout or stderr will appear in the *Messages* buffer."
 	  (if show-next
 	      (notmuch-search-show-thread))))))
 
+(defun notmuch-show-delete-thread ()
+  "Delete each message in thread, then show next thread from search.
+
+Archive each message currently shown by removing the \"inbox\"
+tag from each. Then kill this buffer and show the next thread
+from the search from which this thread was originally shown.
+
+Note: This command is safe from any race condition of new messages
+being delivered to the same thread. It does not archive the
+entire thread, but only the messages shown in the current
+buffer."
+  (interactive)
+  (notmuch-show-archive-or-delete-thread-internal t t))
+
 (defun notmuch-show-archive-thread ()
   "Archive each message in thread, then show next thread from search.
 
@@ -937,12 +955,12 @@ being delivered to the same thread. It does not archive the
 entire thread, but only the messages shown in the current
 buffer."
   (interactive)
-  (notmuch-show-archive-thread-internal t))
+  (notmuch-show-archive-or-delete-thread-internal t nil))
 
 (defun notmuch-show-archive-thread-then-exit ()
   "Archive each message in thread, then exit back to search results."
   (interactive)
-  (notmuch-show-archive-thread-internal nil))
+  (notmuch-show-archive-or-delete-thread-internal nil nil))
 
 (defun notmuch-show-do-stash (text)
   (kill-new text)
diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index f135bca..372f940 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -254,6 +254,7 @@ For a mouse binding, return 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)
@@ -551,6 +552,15 @@ This function advances the next thread when finished."
   (notmuch-search-remove-tag-thread "inbox")
   (forward-line))
 
+(defun notmuch-search-delete-thread ()
+  "Delete the currently selected thread (remove its \"inbox\" tag and add \"deleted\" tag).
+
+This function advances the next thread when finished."
+  (interactive)
+  (notmuch-search-remove-tag-thread "inbox")
+  (notmuch-search-add-tag-thread "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))
-- 
1.6.6.1

Re: [PATCH] Add 'd'elete keybinding to search and show views

[Jesse Rosenthal]

Hi Dirk,

On Wed, 21 Apr 2010 16:16:03 -0700, Dirk Hohndel <hohndel@infradead.org> wrote:
> +	     (if delete
> +		 (notmuch-show-add-tag "deleted")))

The one thing I would suggest is adding a level of customizability,
instead of hardcoding in "deleted".
i.e.:
 
  (defvar notmuch-delete-tag "deleted")

and then:

  (if delete
      (notmuch-show-add-tag notmuch-delete-tag))


The less we force specific tags on users, it seems to me, the better.

Best,
Jesse

Re: [PATCH] Add 'd'elete keybinding to search and show views

[Jameson Rollins]

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

On Wed, 21 Apr 2010 19:25:43 -0400, Jesse Rosenthal <jrosenthal@jhu.edu> wrote:
> The one thing I would suggest is adding a level of customizability,
> instead of hardcoding in "deleted".
> i.e.:
>  
>   (defvar notmuch-delete-tag "deleted")
> 
> and then:
> 
>   (if delete
>       (notmuch-show-add-tag notmuch-delete-tag))
> 
> 
> The less we force specific tags on users, it seems to me, the better.

Hey, Jesse.  There's been some discussion on this, and I think Carl's
position (sorry if I'm putting words in your mouth, Carl) was to just
settle on the tag "deleted" for this concept (of deleted messages) and
stick with it.  I tend to agree with that position.  I think it's better
to just pick a tag for this and stick with it, and "deleted" is as good
as any.  If we allow customization on this level then I think things are
going to get confusing and harder to support down the line.

Jesse, can you think of a reason why it would really be better to allow
customization for this?

jamie.

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

Re: [PATCH] Add 'd'elete keybinding to search and show views

[Jesse Rosenthal]

Hey Jamie,

> There's been some discussion on this, and I think Carl's
> position (sorry if I'm putting words in your mouth, Carl) was to just
> settle on the tag "deleted" for this concept (of deleted messages) and
> stick with it.  

If it's been thought through and there's some consensus, I'll
defer. It's not something that I feel all that strongly about.

> Jesse, can you think of a reason why it would really be better to allow
> customization for this?

No really convincing reasons. I guess I was mainly going by the vague
intuition that this level of configurability just somehow seems more
emacs-y. And that emacs users (me, I guess) might expect it from an
emacs app. 

Also, I guess I'm just wary of any hard-coding in of tags.  Since we
can't foresee the future, or other people's specific email usage
patterns. It's less about this one tag, and more about having any
literal tags in the code. And, I'll be the first to admit, more a
general wariness than a thought-out position.

But, as I said, I'll gladly defer to people who have thought this
through more thoroughly (and, for that matter, to people who would use
this feature).

Best,
Jesse

Re: [PATCH] Add 'd'elete keybinding to search and show views

[Jameson Rollins]

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

On Wed, 21 Apr 2010 16:16:02 -0700, Dirk Hohndel <hohndel@infradead.org> wrote:
> Straight forward addition to the Emacs UI. The 'd' keybinding
> is implemented very similar to the 'a' keybinding - it only
> adds a +deleted tag as well. This tag is used by notmuchsync
> to delete (-p "prune") files in the mailstore.
> 
> I'm sending this mostly as an RFC - I use this and like it, but
> people seem to have strong feelings as to how they want to deal 
> with deleting email (or for some people, how they don't want to
> do that at all).

Hey, Dirk.  I'm definitely on board with this idea, and have in fact
been doing exactly the same thing with my personal customizations as you
propose (including using the 'd' key).

I only have a couple of nit-picky comments about your proposed
implementation:

On Wed, 21 Apr 2010 16:16:03 -0700, Dirk Hohndel <hohndel@infradead.org> wrote:
> -(defun notmuch-show-archive-thread-internal (show-next)
> +(defun notmuch-show-archive-or-delete-thread-internal (show-next delete)
>    ;; Remove the tag from the current set of messages.
>    (goto-char (point-min))
> -  (loop do (notmuch-show-remove-tag "inbox")
> +  (loop do (progn
> +	     (notmuch-show-remove-tag "inbox")
> +	     (if delete
> +		 (notmuch-show-add-tag "deleted")))
>  	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))
> @@ -925,6 +929,20 @@ to stdout or stderr will appear in the *Messages* buffer."
>  	  (if show-next
>  	      (notmuch-search-show-thread))))))

I'm not sure I like the idea of piggybacking on the
notmuch-show-archive-thread-internal function.  Why not just make a new
separate notmuch-show-delete-thread-internal function?  I think it makes
the code clearer and easier to parse for the calls to these functions
(otherwise it's a little unclear what "t t" and "nil nil" and so on
mean).

> +(defun notmuch-search-delete-thread ()
> +  "Delete the currently selected thread (remove its \"inbox\" tag and add \"deleted\" tag).
> +
> +This function advances the next thread when finished."
> +  (interactive)
> +  (notmuch-search-remove-tag-thread "inbox")
> +  (notmuch-search-add-tag-thread "deleted")
> +  (forward-line))

I'm also not a fan of these functions (notmuch-search-delete-thread and
notmuch-show-delete-thread) removing the "inbox" tag.  Just because I
want to mark a messages as deleted doesn't mean that I want to archive
it.  I would really like to keep these concepts distinct if possible.

jamie.

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

Re: [PATCH] Add 'd'elete keybinding to search and show views

[Dirk Hohndel]

On Wed, 21 Apr 2010 19:32:39 -0400, Jameson Rollins <jrollins@finestructure.net> wrote:
> On Wed, 21 Apr 2010 16:16:02 -0700, Dirk Hohndel <hohndel@infradead.org> wrote:
> > Straight forward addition to the Emacs UI. The 'd' keybinding
> > is implemented very similar to the 'a' keybinding - it only
> > adds a +deleted tag as well. This tag is used by notmuchsync
> > to delete (-p "prune") files in the mailstore.
> > 
> > I'm sending this mostly as an RFC - I use this and like it, but
> > people seem to have strong feelings as to how they want to deal 
> > with deleting email (or for some people, how they don't want to
> > do that at all).
> 
> Hey, Dirk.  I'm definitely on board with this idea, and have in fact
> been doing exactly the same thing with my personal customizations as you
> propose (including using the 'd' key).

Great. 
 
> I only have a couple of nit-picky comments about your proposed
> implementation:

That's why I sent this out...
 
> On Wed, 21 Apr 2010 16:16:03 -0700, Dirk Hohndel <hohndel@infradead.org> wrote:
> > -(defun notmuch-show-archive-thread-internal (show-next)
> > +(defun notmuch-show-archive-or-delete-thread-internal (show-next delete)
> >    ;; Remove the tag from the current set of messages.
> >    (goto-char (point-min))
> > -  (loop do (notmuch-show-remove-tag "inbox")
> > +  (loop do (progn
> > +	     (notmuch-show-remove-tag "inbox")
> > +	     (if delete
> > +		 (notmuch-show-add-tag "deleted")))
> >  	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))
> > @@ -925,6 +929,20 @@ to stdout or stderr will appear in the *Messages* buffer."
> >  	  (if show-next
> >  	      (notmuch-search-show-thread))))))
> 
> I'm not sure I like the idea of piggybacking on the
> notmuch-show-archive-thread-internal function.  Why not just make a new
> separate notmuch-show-delete-thread-internal function?  I think it makes
> the code clearer and easier to parse for the calls to these functions
> (otherwise it's a little unclear what "t t" and "nil nil" and so on
> mean).

It's the C programmer in me who hates code duplication. This way, if the
algorithm for walking the mails in the thread changes, you only fix it
once. But I see your point about weird options... in C I'd have
constants named THREAD_DELETE and THREAD_ARCHIVE_ONLY that I'd pass
around...

> 
> > +(defun notmuch-search-delete-thread ()
> > +  "Delete the currently selected thread (remove its \"inbox\" tag and add \"deleted\" tag).
> > +
> > +This function advances the next thread when finished."
> > +  (interactive)
> > +  (notmuch-search-remove-tag-thread "inbox")
> > +  (notmuch-search-add-tag-thread "deleted")
> > +  (forward-line))
> 
> I'm also not a fan of these functions (notmuch-search-delete-thread and
> notmuch-show-delete-thread) removing the "inbox" tag.  Just because I
> want to mark a messages as deleted doesn't mean that I want to archive
> it.  I would really like to keep these concepts distinct if possible.

Well, that would take away a big reason for adding this -
convenience. In the end it's user experience design. The question is
(based on expected behavior) - do you expect a deleted email to stay
visible in your inbox. I don't - and I know that many people do.

So while I agree with the other conclusion in the thread that "deleted"
as tag for deleted messages doesn't have to be configurable - maybe this
feature wants to be configurable.

notmuch-archive-deleted or something like that

/D

-- 
Dirk Hohndel
Intel Open Source Technology Center

Re: [PATCH] Add 'd'elete keybinding to search and show views

[Jameson Rollins]

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

On Wed, 21 Apr 2010 17:20:41 -0700, Dirk Hohndel <hohndel@infradead.org> wrote:
> Well, that would take away a big reason for adding this -
> convenience. In the end it's user experience design. The question is
> (based on expected behavior) - do you expect a deleted email to stay
> visible in your inbox. I don't - and I know that many people do.

I actually have my default "inbox" set to be the search "tag:inbox and
not tag:delete", so it's true that in general I don't usually want to
see the deleted messages.  However, I think the right solution is to
have the default view be "inbox and not deleted", instead of using the
delete key to remove inbox tags.  I just feel like I want to have
fine-grained control over tagging.  By the same token, I don't want
hitting the space bar to archive messages when all I really want to do
is advance to the next message.

jamie.

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

Re: add delete keybinding

[Carl Worth]

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

On Wed, 21 Apr 2010 16:16:02 -0700, Dirk Hohndel <hohndel@infradead.org> wrote:
> I'm sending this mostly as an RFC - I use this and like it, but
> people seem to have strong feelings as to how they want to deal 
> with deleting email (or for some people, how they don't want to
> do that at all).

I like the idea of adding the deleted tag with a 'd' keybinding.

I also agree with the comments that suggest that this should be
independent of the archiving operation. (That is, 'd' should just add
the deleted tag and do nothing else.)

With sup, there was the idea to not make the user add "add not
tag:deleted" to all searches. The way that worked was basically that sup
would append "and not tag:deleted" to all searches except for those that
already had "tag:deleted" in them. This allows deleted messages to give
every appearance of being deleted, (they would not show up in searches),
but a user *could* still find them[*] by explicitly saying "and
tag:deleted" in the search.

I don't think it makes sense to add a delete keybinding without some
support along the lines of what's described above.

It does seem out of character for the library or even the command-line
to do interpretation of tag names and munging of search strings like
this. So I think all of the above should be implemented within emacs
code. It's the emacs code that is adding the "deleted" tag so it should
be interpreting its behavior as well.

-Carl

[*] Until the user did some sort of external expunge operation that
actually deleted the files, of course.

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