[PATCH] notmuch.el: add functionality to add or remove tags by region.

[PATCH] notmuch.el: add functionality to add or remove tags by region.

[Jesse Rosenthal]

This patch adds `-region' versions of the `notmuch-search-' commands to find properties. It also splits up  `notmuch-add/remove-tags' into both a `-thread' and a `-region' version. (This makes us modify `notmuch-search-archive-thread' to use the `notmuch-search-remove-tag-thread' function, instead of `notmuch-search-remove-tag', for consistency.) The add/remove-tag command called by pressing `+' or `-' will then choose accordingly, based on whether region is active.
---
 notmuch.el |   95 +++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 84 insertions(+), 11 deletions(-)

diff --git a/notmuch.el b/notmuch.el
index 97914f2..e333f24 100644
--- a/notmuch.el
+++ b/notmuch.el
@@ -1117,18 +1117,41 @@ Complete list of currently available key bindings:
   (set (make-local-variable 'font-lock-defaults)
          '(notmuch-search-font-lock-keywords t)))
 
+(defun notmuch-search-properties-in-region (property beg end)
+  (save-excursion
+    (let ((output nil)
+	  (last-line (line-number-at-pos end)))
+      (goto-char beg)
+      (beginning-of-line)
+      (while (<= (line-number-at-pos) last-line)
+	(setq output (cons (get-text-property (point) property) output))
+	(forward-line 1))
+      output)))
+
 (defun notmuch-search-find-thread-id ()
   "Return the thread for the current thread"
   (get-text-property (point) 'notmuch-search-thread-id))
 
+(defun notmuch-search-find-thread-id-region (beg end)
+  "Return a list of threads for the current region"
+  (notmuch-search-properties-in-region 'notmuch-search-thread-id beg end))
+
 (defun notmuch-search-find-authors ()
   "Return the authors for the current thread"
   (get-text-property (point) 'notmuch-search-authors))
 
+(defun notmuch-search-find-authors-region (beg end)
+  "Return a list of authors for the current region"
+  (notmuch-search-properties-in-region notmuch-search-authors beg end))
+
 (defun notmuch-search-find-subject ()
   "Return the subject for the current thread"
   (get-text-property (point) 'notmuch-search-subject))
 
+(defun notmuch-search-find-subject-region (beg end)
+  "Return a list of authors for the current region"
+  (notmuch-search-properties-in-region notmuch-search-subject beg end))
+
 (defun notmuch-search-show-thread ()
   "Display the currently selected thread."
   (interactive)
@@ -1173,6 +1196,14 @@ and will also appear in a buffer named \"*Notmuch errors*\"."
 	(delete-region beg end)
 	(insert (mapconcat  'identity tags " "))))))
 
+(defun notmuch-search-set-tags-region (tags beg end)
+  (save-excursion
+    (let ((last-line (line-number-at-pos end)))
+      (goto-char beg)
+      (while (<= (line-number-at-pos) last-line)
+	(notmuch-search-set-tags tags)
+	(forward-line)))))
+
 (defun notmuch-search-get-tags ()
   (save-excursion
     (end-of-line)
@@ -1182,32 +1213,74 @@ and will also appear in a buffer named \"*Notmuch errors*\"."
       (let ((end (- (point) 1)))
 	(split-string (buffer-substring beg end))))))
 
+(defun notmuch-search-get-tags-region (beg end)
+  (save-excursion
+    (let ((output nil)
+	  (last-line (line-number-at-pos end)))
+      (goto-char beg)
+      (while (<= (line-number-at-pos) last-line)
+	(setq output (append output (notmuch-search-get-tags)))
+	(forward-line 1))
+      output)))
+
+(defun notmuch-search-add-tag-thread (tag)
+  (notmuch-call-notmuch-process "tag" (concat "+" tag) (notmuch-search-find-thread-id))
+  (notmuch-search-set-tags (delete-dups (sort (cons tag (notmuch-search-get-tags)) 'string<))))
+
+(defun notmuch-search-add-tag-region (tag beg end)
+  (let ((search-id-string (mapconcat 'identity (notmuch-search-find-thread-id-region beg end) " or ")))
+    (notmuch-call-notmuch-process "tag" (concat "+" tag) search-id-string)
+    (notmuch-search-set-tags-region (delete-dups (sort (cons tag (notmuch-search-get-tags-region beg end)) 'string<)) beg end)))
+
+(defun notmuch-search-remove-tag-thread (tag)
+  (notmuch-call-notmuch-process "tag" (concat "-" tag) (notmuch-search-find-thread-id))
+  (notmuch-search-set-tags (delete tag (notmuch-search-get-tags))))
+
+(defun notmuch-search-remove-tag-region (tag beg end)
+  (let ((search-id-string (mapconcat 'identity (notmuch-search-find-thread-id-region beg end) " or ")))
+    (notmuch-call-notmuch-process "tag" (concat "-" tag) search-id-string)
+    (notmuch-search-set-tags-region (delete tag (notmuch-search-get-tags-region beg end)) beg end)))
+
 (defun notmuch-search-add-tag (tag)
-  "Add a tag to the currently selected thread.
+  "Add a tag to the currently selected thread or region.
 
-The tag is added to messages in the currently selected thread
-which match the current search terms."
+The tag is added to messages in the currently selected thread or
+region which match the current search terms."
   (interactive
    (list (notmuch-select-tag-with-completion "Tag to add: ")))
-  (notmuch-call-notmuch-process "tag" (concat "+" tag) (notmuch-search-find-thread-id))
-  (notmuch-search-set-tags (delete-dups (sort (cons tag (notmuch-search-get-tags)) 'string<))))
+  (save-excursion
+    (if (region-active-p)
+	(let* ((beg (region-beginning))
+	       (end (region-end)))
+	  (notmuch-search-add-tag-region tag beg end))
+      (notmuch-search-add-tag-thread tag))))
 
 (defun notmuch-search-remove-tag (tag)
-  "Remove a tag from the currently selected thread.
+  "Remove a tag from the currently selected thread or region.
 
 The tag is removed from messages in the currently selected thread
-which match the current search terms."
+or region which match the current search terms."
   (interactive
-   (list (notmuch-select-tag-with-completion "Tag to remove: " (notmuch-search-find-thread-id))))
-  (notmuch-call-notmuch-process "tag" (concat "-" tag) (notmuch-search-find-thread-id))
-  (notmuch-search-set-tags (delete tag (notmuch-search-get-tags))))
+   (list (notmuch-select-tag-with-completion 
+	  "Tag to remove: "
+	  (if (region-active-p) 
+	      (mapconcat 'identity 
+			 (notmuch-search-find-thread-id-region (region-beginning) (region-end))
+			 " ")
+	    (notmuch-search-find-thread-id)))))
+  (save-excursion
+    (if (region-active-p)
+	(let* ((beg (region-beginning))
+	       (end (region-end)))
+	  (notmuch-search-remove-tag-region tag beg end))
+      (notmuch-search-remove-tag-thread tag))))
 
 (defun notmuch-search-archive-thread ()
   "Archive the currently selected thread (remove its \"inbox\" tag).
 
 This function advances the next thread when finished."
   (interactive)
-  (notmuch-search-remove-tag "inbox")
+  (notmuch-search-remove-tag-thread "inbox")
   (forward-line))
 
 (defun notmuch-search-process-sentinel (proc msg)
-- 
1.6.5.3

Re: [PATCH] notmuch.el: add functionality to add or remove tags by region.

[Jesse Rosenthal]

Re: [PATCH] notmuch.el: add functionality to add or remove tags by region.

[Jameson Rollins]

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

On Wed, Jan 20, 2010 at 03:58:10PM -0500, Jesse Rosenthal wrote:
> This patch adds `-region' versions of the `notmuch-search-' commands to find properties. It also splits up  `notmuch-add/remove-tags' into both a `-thread' and a `-region' version. (This makes us modify `notmuch-search-archive-thread' to use the `notmuch-search-remove-tag-thread' function, instead of `notmuch-search-remove-tag', for consistency.) The add/remove-tag command called by pressing `+' or `-' will then choose accordingly, based on whether region is active.

Hi, Jesse.  Can you explain what exactly a "region" is?  It's not
clear from your log message or the patch itself.

jamie.

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

Re: [PATCH] notmuch.el: add functionality to add or remove tags by region.

[Jesse Rosenthal]

On Thu, 21 Jan 2010 10:23:24 -0500, Jameson Rollins <jrollins@finestructure.net> wrote:

Hi Jamie,

> Hi, Jesse.  Can you explain what exactly a "region" is?  It's not
> clear from your log message or the patch itself.

Region is emacs-speak for selected area (either click-and-drag or C-SPC
and then motion commands). Basically, this was meant to be an emacsy-way
of tagging a bunch of messages in a buffer at once.

Best,
Jesse

Re: [PATCH] notmuch.el: add functionality to add or remove tags by region.

[Jameson Rollins]

On Thu, Jan 21, 2010 at 10:35:34AM -0500, Jesse Rosenthal wrote:
> Region is emacs-speak for selected area (either click-and-drag or C-SPC
> and then motion commands). Basically, this was meant to be an emacsy-way
> of tagging a bunch of messages in a buffer at once.

I understand what a region is in emacs land, but I don't understand
how it translates to mail land.  Notmuch is not to my knowledge
capable of tagging a sub-region of a message, so in that sense it
might not work.  If you goal is to tag only a subset of messages in a
thread, then regions don't seem like the most natural way to do that,
since regions are non-disjoint.  Would it be better to use some sort
of tag/apply system ala mutt instead?

jamie.

Re: [PATCH] notmuch.el: add functionality to add or remove tags by region.

[Jesse Rosenthal]

On Thu, 21 Jan 2010 10:56:59 -0500, Jameson Rollins <jrollins@finestructure.net> wrote:
> Notmuch is not to my knowledge capable of tagging a sub-region of a
> message, so in that sense it might not work.  

This is only in notmuch-search mode, so it only operates on full
threads.

> Would it be better to use some sort of tag/apply system ala mutt
> instead?

That could work too. The main reason I implemented this is that, as an
emacs user, I kept intuitively trying to tag messages in this way, and
it wouldn't work.

I actually was thinking of combining regions and marking (let's call it
that to keep from getting confused with notmuch tags) -- i.e., have
region selection in search mode also mark messages. And then there could
also be a message-by-message way if you wanted to mark disjoint
messages.

Best,
Jesse

Re: [PATCH] notmuch.el: add functionality to add or remove tags by region.

[Jesse Rosenthal]

> ...have region selection in search mode also mark messages. And then
> there could also be a message-by-message way if you wanted to mark
> disjoint messages.

Sorry, I'm getting myself confused here. Not messages, *threads*.

[PATCH v2] notmuch.el: add functionality in notmuch search mode to add or remove tags by region

[Jesse Rosenthal]

This patch adds `-region' versions of the `notmuch-search-' commands to find
properties. It also splits up  `notmuch-add/remove-tags' into both a
`-thread' and a `-region' version. (This makes us modify
`notmuch-search-archive-thread' to use the
`notmuch-search-remove-tag-thread' function, instead of
`notmuch-search-remove-tag', for consistency.) The add/remove-tag command
called by pressing `+' or `-' will then choose accordingly, based on whether
region is active.

This version fixes a couple of errors in the first version, which led to
incorrect marking of some tags in the search view (though the actual
tagging was still correct). It's also based on current master.

I'm not sure any more if region selection is actually the correct way to
do this, or if a mutt-style message-marking method would be better. But
I didn't want a buggy incorrect version out there.
---
 notmuch.el |   98 +++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 87 insertions(+), 11 deletions(-)

diff --git a/notmuch.el b/notmuch.el
index 0f4ea10..7d9a82f 100644
--- a/notmuch.el
+++ b/notmuch.el
@@ -1227,18 +1227,41 @@ Complete list of currently available key bindings:
   (set (make-local-variable 'font-lock-defaults)
          '(notmuch-search-font-lock-keywords t)))
 
+(defun notmuch-search-properties-in-region (property beg end)
+  (save-excursion
+    (let ((output nil)
+	  (last-line (line-number-at-pos end)))
+      (goto-char beg)
+      (beginning-of-line)
+      (while (<= (line-number-at-pos) last-line)
+	(setq output (cons (get-text-property (point) property) output))
+	(forward-line 1))
+      output)))
+
 (defun notmuch-search-find-thread-id ()
   "Return the thread for the current thread"
   (get-text-property (point) 'notmuch-search-thread-id))
 
+(defun notmuch-search-find-thread-id-region (beg end)
+  "Return a list of threads for the current region"
+  (notmuch-search-properties-in-region 'notmuch-search-thread-id beg end))
+
 (defun notmuch-search-find-authors ()
   "Return the authors for the current thread"
   (get-text-property (point) 'notmuch-search-authors))
 
+(defun notmuch-search-find-authors-region (beg end)
+  "Return a list of authors for the current region"
+  (notmuch-search-properties-in-region 'notmuch-search-authors beg end))
+
 (defun notmuch-search-find-subject ()
   "Return the subject for the current thread"
   (get-text-property (point) 'notmuch-search-subject))
 
+(defun notmuch-search-find-subject-region (beg end)
+  "Return a list of authors for the current region"
+  (notmuch-search-properties-in-region 'notmuch-search-subject beg end))
+
 (defun notmuch-search-show-thread ()
   "Display the currently selected thread."
   (interactive)
@@ -1292,32 +1315,85 @@ and will also appear in a buffer named \"*Notmuch errors*\"."
       (let ((end (- (point) 1)))
 	(split-string (buffer-substring beg end))))))
 
+(defun notmuch-search-get-tags-region (beg end)
+  (save-excursion
+    (let ((output nil)
+	  (last-line (line-number-at-pos end)))
+      (goto-char beg)
+      (while (<= (line-number-at-pos) last-line)
+	(setq output (append output (notmuch-search-get-tags)))
+	(forward-line 1))
+      output)))
+
+(defun notmuch-search-add-tag-thread (tag)
+  (notmuch-call-notmuch-process "tag" (concat "+" tag) (notmuch-search-find-thread-id))
+  (notmuch-search-set-tags (delete-dups (sort (cons tag (notmuch-search-get-tags)) 'string<))))
+
+(defun notmuch-search-add-tag-region (tag beg end)
+  (let ((search-id-string (mapconcat 'identity (notmuch-search-find-thread-id-region beg end) " or ")))
+    (notmuch-call-notmuch-process "tag" (concat "+" tag) search-id-string)
+    (save-excursion
+      (let ((last-line (line-number-at-pos end)))
+	(goto-char beg)
+	(while (<= (line-number-at-pos) last-line)
+	  (notmuch-search-set-tags (delete-dups (sort (cons tag (notmuch-search-get-tags)) 'string<)))
+	  (forward-line))))))
+
+(defun notmuch-search-remove-tag-thread (tag)
+  (notmuch-call-notmuch-process "tag" (concat "-" tag) (notmuch-search-find-thread-id))
+  (notmuch-search-set-tags (delete tag (notmuch-search-get-tags))))
+
+(defun notmuch-search-remove-tag-region (tag beg end)
+  (let ((search-id-string (mapconcat 'identity (notmuch-search-find-thread-id-region beg end) " or ")))
+    (notmuch-call-notmuch-process "tag" (concat "-" tag) search-id-string)
+    (save-excursion
+      (let ((last-line (line-number-at-pos end)))
+	(goto-char beg)
+	(while (<= (line-number-at-pos) last-line)
+	  (notmuch-search-set-tags (delete tag (notmuch-search-get-tags)))
+	  (forward-line))))))
+
+
 (defun notmuch-search-add-tag (tag)
-  "Add a tag to the currently selected thread.
+  "Add a tag to the currently selected thread or region.
 
-The tag is added to messages in the currently selected thread
-which match the current search terms."
+The tag is added to messages in the currently selected thread or
+region which match the current search terms."
   (interactive
    (list (notmuch-select-tag-with-completion "Tag to add: ")))
-  (notmuch-call-notmuch-process "tag" (concat "+" tag) (notmuch-search-find-thread-id))
-  (notmuch-search-set-tags (delete-dups (sort (cons tag (notmuch-search-get-tags)) 'string<))))
+  (save-excursion
+    (if (region-active-p)
+	(let* ((beg (region-beginning))
+	       (end (region-end)))
+	  (notmuch-search-add-tag-region tag beg end))
+      (notmuch-search-add-tag-thread tag))))
 
 (defun notmuch-search-remove-tag (tag)
-  "Remove a tag from the currently selected thread.
+  "Remove a tag from the currently selected thread or region.
 
 The tag is removed from messages in the currently selected thread
-which match the current search terms."
+or region which match the current search terms."
   (interactive
-   (list (notmuch-select-tag-with-completion "Tag to remove: " (notmuch-search-find-thread-id))))
-  (notmuch-call-notmuch-process "tag" (concat "-" tag) (notmuch-search-find-thread-id))
-  (notmuch-search-set-tags (delete tag (notmuch-search-get-tags))))
+   (list (notmuch-select-tag-with-completion
+	  "Tag to remove: "
+	  (if (region-active-p)
+	      (mapconcat 'identity
+			 (notmuch-search-find-thread-id-region (region-beginning) (region-end))
+			 " ")
+	    (notmuch-search-find-thread-id)))))
+  (save-excursion
+    (if (region-active-p)
+	(let* ((beg (region-beginning))
+	       (end (region-end)))
+	  (notmuch-search-remove-tag-region tag beg end))
+      (notmuch-search-remove-tag-thread tag))))
 
 (defun notmuch-search-archive-thread ()
   "Archive the currently selected thread (remove its \"inbox\" tag).
 
 This function advances the next thread when finished."
   (interactive)
-  (notmuch-search-remove-tag "inbox")
+  (notmuch-search-remove-tag-thread "inbox")
   (forward-line))
 
 (defun notmuch-search-process-sentinel (proc msg)
-- 
1.6.3.3

[PATCH] notmuch.el: bind 'd' to new function notmuch-search-delete-thread-or-region

[Sebastian Spaeth]

Create a new function notmuch-search-delete-thread-or region which does exactly what its name implies.

Hitting 'd' will delete the current thread (or multiple threads if you marked a region). Deleting means adding tag 'delete' and removing tags 'unread' and 'inbox' in this case.

This patch requires Jesse's patch from mail id:87sk90ragj.fsf@jhu.edu.

Signed-off-by: Sebastian Spaeth <Sebastian@SSpaeth.de>
---
 notmuch.el |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/notmuch.el b/notmuch.el
index 7d9a82f..ac07ff3 100644
--- a/notmuch.el
+++ b/notmuch.el
@@ -1101,6 +1101,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-or-region)
     (define-key map "-" 'notmuch-search-remove-tag)
     (define-key map "+" 'notmuch-search-add-tag)
     (define-key map (kbd "RET") 'notmuch-search-show-thread)
@@ -1396,6 +1397,16 @@ This function advances the next thread when finished."
   (notmuch-search-remove-tag-thread "inbox")
   (forward-line))
 
+(defun notmuch-search-delete-thread-or-region ()
+  "Delete the currently selected thread (tag \"+delete -inbox -unread\").
+
+This function advances the next thread when finished."
+  (interactive)
+  (notmuch-search-add-tag "delete")
+  (notmuch-search-remove-tag "inbox")
+  (notmuch-search-remove-tag "unread")
+  (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.3.3

Re: notmuch.el: bind 'd' to new function notmuch-search-delete-thread-or-region

[anarcat]

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

I can confirm this patch works for me.

I think this would be a great addition to notmuch, and I could add it
directly to Debian's 0.6 package install.

A.

On Wed, Feb 17, 2010 at 02:12:26AM -0000, Sebastian Spaeth wrote:
> Create a new function notmuch-search-delete-thread-or region which does exactly what its name implies.
> 
> Hitting 'd' will delete the current thread (or multiple threads if you marked a region). Deleting means adding tag 'delete' and removing tags 'unread' and 'inbox' in this case.

-- 
Antoine Beaupré +++ Réseau Koumbit Networks +++ +1.514.387.6262 #208
--------------------------------------------------------------------
À force de ne jamais réfléchir, on a un bonheur stupide
                        - Jean Cocteau

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

Re: notmuch.el: bind 'd' to new function notmuch-search-delete-thread-or-region

[Matthieu Lemerre]

On Fri, 15 Jul 2011 00:11:29 -0400, anarcat <anarcat@koumbit.org> wrote:
Non-text part: multipart/mixed
Non-text part: multipart/signed
> I can confirm this patch works for me.
> 
> I think this would be a great addition to notmuch, and I could add it
> directly to Debian's 0.6 package install.
> 

I also strongly in favor for the addition of this patch, having a
similar one in my own branch.

Matthieu

[PATCH 1/2] add notmuch keybinding 'd'

[Antoine Beaupré]

It adds a tag 'deleted' and removes the tags 'inbox' and 'unread'. It
works in show as well as in search mode

Based on previous work by: Sebastian Spaeth <Sebastian@SSpaeth.de>

Signed-off-by: Antoine Beaupré <anarcat@koumbit.org>
---
 emacs/notmuch-show.el |    8 ++++++++
 emacs/notmuch.el      |   11 +++++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index f96743b..c83b992 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -875,6 +875,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)
 	(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)
@@ -1297,6 +1298,13 @@ the result."
 	     (mapcar (lambda (s) (concat "-" s)) toremove))
       (notmuch-show-set-tags new-tags))))
 
+(defun notmuch-show-delete ()
+  "Delete current mail (tag +deleted -unread -inbox)."
+  (interactive)
+  (notmuch-show-add-tag "deleted")
+  (notmuch-show-remove-tag "unread")
+  (notmuch-show-remove-tag "inbox"))
+
 (defun notmuch-show-toggle-headers ()
   "Toggle the visibility of the current message headers."
   (interactive)
diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index f11ec24..f6fb07b 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -215,6 +215,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-or-region)
     (define-key map "-" 'notmuch-search-remove-tag)
     (define-key map "+" 'notmuch-search-add-tag)
     (define-key map (kbd "RET") 'notmuch-search-show-thread)
@@ -611,6 +612,16 @@ This function advances the next thread when finished."
   "Data that has not yet been processed.")
 (make-variable-buffer-local 'notmuch-search-process-filter-data)
 
+(defun notmuch-search-delete-thread-or-region ()
+  "Delete the currently selected thread (tag \"+deleted -inbox -unread\").
+
+This function advances the next thread when finished."
+  (interactive)
+  (notmuch-search-add-tag "deleted")
+  (notmuch-search-remove-tag "inbox")
+  (notmuch-search-remove-tag "unread")
+  (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.7.5.4

Re: [PATCH 1/2] add notmuch keybinding 'd'

[David Edmondson]

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

On Sat, 16 Jul 2011 14:39:59 -0400, Antoine Beaupré <anarcat@koumbit.org> wrote:
> It adds a tag 'deleted' and removes the tags 'inbox' and 'unread'. It
> works in show as well as in search mode

Various people have asked for a keybinding to add a 'delete' tag. Is
this version the right one to choose?

It seems sane to me (though I won't use it).

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

Re: [PATCH 1/2] add notmuch keybinding 'd'

[Jani Nikula]

On Tue, 03 Jan 2012 14:56:50 +0000, David Edmondson <dme@dme.org> wrote:
> On Sat, 16 Jul 2011 14:39:59 -0400, Antoine Beaupré <anarcat@koumbit.org> wrote:
> > It adds a tag 'deleted' and removes the tags 'inbox' and 'unread'. It
> > works in show as well as in search mode
> 
> Various people have asked for a keybinding to add a 'delete' tag. Is
> this version the right one to choose?

The code's there, and talk is cheap... but I'd rather like to see a
solution that would make it easy for users to add arbitrary tagging
operations to key bindings, instead of a fixed binding for "deleted".

That way perhaps adding a default 'd' for "deleted" wouldn't feel so
special either.

> It seems sane to me (though I won't use it).

I wouldn't use it either, and I don't use "deleted" tag in the first
place. And even if I used it, I'd still like to keep the distinction
between "deleted after reading" and "deleted unread", which this patch
loses by removing the "unread" tag.


BR,
Jani.

Re: [PATCH 1/2] add notmuch keybinding 'd'

[Jameson Graef Rollins]

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

On Tue, 03 Jan 2012 21:57:22 +0200, Jani Nikula <jani@nikula.org> wrote:
> The code's there, and talk is cheap... but I'd rather like to see a
> solution that would make it easy for users to add arbitrary tagging
> operations to key bindings, instead of a fixed binding for "deleted".

It's already easy for users to add arbitrary key bindings.  It's emacs,
and the whole point of which is that it's infinitely extensible [0]:

(define-key notmuch-show-mode-map "d"
  (lambda ()
    "Delete current message and advance to next message."
    (interactive)
    (notmuch-show-add-tag "delete")
    (notmuch-show-next-open-message-or-pop)))

There really can't be an "easier" solution to add a key binding other
than just adding the above to your .emacs, and I really don't think it
would be worth it to try to make one.

The fact that it's already so easy to customize the key bindings is why
I've been reluctant (or even against) getting into protracted
discussions about what the default key bindings should be.  Everyone is
going to want a different behavior, and since it's easy enough to
customize, just let users define what they want.

> I wouldn't use it either, and I don't use "deleted" tag in the first
> place. And even if I used it, I'd still like to keep the distinction
> between "deleted after reading" and "deleted unread", which this patch
> loses by removing the "unread" tag.

Beyond what I just said about letting everyone customize things
themselves, I agree that I don't personally want other tags added or
removed when I "delete" a message.  The key binding above is actually
the one I use.

jamie.

[0] http://notmuchmail.org/emacstips/#index6h2

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

Re: [PATCH 1/2] add notmuch keybinding 'd'

[Jani Nikula]

On Tue, 03 Jan 2012 13:35:23 -0800, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> There really can't be an "easier" solution to add a key binding other
> than just adding the above to your .emacs, and I really don't think it
> would be worth it to try to make one.

I guess you're right. Thanks for the example too.

> The fact that it's already so easy to customize the key bindings is why
> I've been reluctant (or even against) getting into protracted
> discussions about what the default key bindings should be.  Everyone is
> going to want a different behavior, and since it's easy enough to
> customize, just let users define what they want.

This is what I was basically after too: provide elementary functions
that make it easy for users to do what they want. (Just in this case
nothing more is really needed.)

As a side note, GNU Global has an interesting approach of not binding
any keys, unless you have done:
(setq gtags-suggested-key-mapping t)


BR,
Jani.

Re: [PATCH 1/2] add notmuch keybinding 'd'

[Antoine Beaupré]

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

On Tue, 03 Jan 2012 21:57:22 +0200, Jani Nikula <jani@nikula.org> wrote:
> I wouldn't use it either, and I don't use "deleted" tag in the first
> place. And even if I used it, I'd still like to keep the distinction
> between "deleted after reading" and "deleted unread", which this patch
> loses by removing the "unread" tag.

I have been using this for months now.

But honestly, I don't care much anymore: the hard part is not the tag,
it's what you do with it after (hint: just remove the damn file).

Most patches I have submitted here haven't been accepted and I have to
painfully reroll my own packages every time there's a new release, which
has been a very frustrating experience. To see such a trivial patch
obsoleted tops it.

For the curious, those (other) patches are:

 * lib: add 'safe' setting for flags
 * lib: Add back the synchronization of 'T' flag with deleted tag
 * run notmuch-hello-mode-hook at the end of the hello mode setup

Most of those do not change the current behavior, and I have been
running them for more than 4 months.

I'd very much like to get help to get this in... 

A.

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

Re: [PATCH 1/2] add notmuch keybinding 'd'

[Jameson Graef Rollins]

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

On Fri, 06 Jan 2012 15:10:57 -0500, Antoine Beaupré <anarcat@koumbit.org> wrote:
> Most patches I have submitted here haven't been accepted and I have to
> painfully reroll my own packages every time there's a new release, which
> has been a very frustrating experience. To see such a trivial patch
> obsoleted tops it.

Don't be too over dramatic, Antoine.  A lot of patches have sat fallow
for a long time, and a lot of us maintain our own stuff on top what's in
master.

There's also been discussion on this particular topic since long before
you showed up.  cworth pushed back on adding a delete key until we had a
more integrated solution.  I think the same thing is happening here.

I'm going to take a crack at an implementation of this functionality
this weekend that I think will satisfy most of out needs.

jamie.

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

Re: [PATCH 1/2] add notmuch keybinding 'd'

[David Bremner]

On Fri, 06 Jan 2012 15:10:57 -0500, Antoine Beaupré <anarcat@koumbit.org> wrote:
> On Tue, 03 Jan 2012 21:57:22 +0200, Jani Nikula <jani@nikula.org> wrote:

>  * lib: add 'safe' setting for flags
>  * lib: Add back the synchronization of 'T' flag with deleted tag
>  * run notmuch-hello-mode-hook at the end of the hello mode setup

> 
> Most of those do not change the current behavior, and I have been
> running them for more than 4 months.
> 
> I'd very much like to get help to get this in... 

Hi Antoine;

I understand your frustration; it's not very motivating to feel
ignored. Over the last few months we have been working to develop a
patch review process for notmuch [1], but as you can see from [2] there
is still a backlog of patches that have not been reviewed, the two lib
patches you mention among them. More reviewers are always welcome ;).

David

[1]: http://notmuchmail.org/nmbug/
[2]: http://nmbug.tethera.net/status/

Re: [PATCH 1/2] add notmuch keybinding 'd'

[Antoine Beaupré]

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

On Fri, 06 Jan 2012 16:51:51 -0400, David Bremner <david@tethera.net> wrote:
> On Fri, 06 Jan 2012 15:10:57 -0500, Antoine Beaupré <anarcat@koumbit.org> wrote:
> > On Tue, 03 Jan 2012 21:57:22 +0200, Jani Nikula <jani@nikula.org> wrote:
> 
> >  * lib: add 'safe' setting for flags
> >  * lib: Add back the synchronization of 'T' flag with deleted tag
> >  * run notmuch-hello-mode-hook at the end of the hello mode setup
> 
> > 
> > Most of those do not change the current behavior, and I have been
> > running them for more than 4 months.
> > 
> > I'd very much like to get help to get this in... 
> 
> Hi Antoine;
> 
> I understand your frustration; it's not very motivating to feel
> ignored. Over the last few months we have been working to develop a
> patch review process for notmuch [1], but as you can see from [2] there
> is still a backlog of patches that have not been reviewed, the two lib
> patches you mention among them. More reviewers are always welcome ;).

Hi,

Thanks for the nice words.

I am sorry I whined on the list like this. :) I guess I was a bit tired
and disappointed no progress had been done on those patches, but I
understand how hard it is to keep up with the crazy flood on the list.

> [1]: http://notmuchmail.org/nmbug/
> [2]: http://nmbug.tethera.net/status/

This is a great initiative!

I am glad to see my patches still sitting there at least! ;) And I am
also happy to see Jamie's patches for the delete key there, if I had
more time I would actually go through and review them, unfortunately, I
have very little time to review stuff that, honestly, "just works" for
me right now. ;)

Sorry again for the outburst, keep up the good work!

A.

-- 
La guerre, c'est le massacre d'hommes qui ne se connaissent pas,
au profit d'hommes qui se connaissent mais ne se massacreront pas.
                        - Paul Valéry

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

Re: [PATCH 1/2] add notmuch keybinding 'd'

[Jani Nikula]

Hi Antoine -

On Fri, 06 Jan 2012 15:10:57 -0500, Antoine Beaupré <anarcat@koumbit.org> wrote:
> On Tue, 03 Jan 2012 21:57:22 +0200, Jani Nikula <jani@nikula.org> wrote:
> > I wouldn't use it either, and I don't use "deleted" tag in the first
> > place. And even if I used it, I'd still like to keep the distinction
> > between "deleted after reading" and "deleted unread", which this patch
> > loses by removing the "unread" tag.
> 
> I have been using this for months now.
> 
> But honestly, I don't care much anymore: the hard part is not the tag,
> it's what you do with it after (hint: just remove the damn file).
> 
> Most patches I have submitted here haven't been accepted and I have to
> painfully reroll my own packages every time there's a new release, which
> has been a very frustrating experience. To see such a trivial patch
> obsoleted tops it.

This patch, while trivial, fixes a specific use case for you, according
to your preferences. I was hoping for something more generic, and
Jameson showed how this can be done in one's .emacs without adding new
code (or carrying your own patches) at all. If this approach does not
work for you, please let us know why!

> For the curious, those (other) patches are:
> 
>  * lib: add 'safe' setting for flags
>  * lib: Add back the synchronization of 'T' flag with deleted tag
>  * run notmuch-hello-mode-hook at the end of the hello mode setup
> 
> Most of those do not change the current behavior, and I have been
> running them for more than 4 months.

I haven't looked at the patches, but please understand that not changing
the current behaviour is not a sufficient reason for inclusion. Once in,
people will start using the features, which will need to be maintained
and supported potentially for a very long time. Once in, it's not easy
to take features out. Especially so for lib. So we need to be careful.

Tests have also become an increasingly important part in getting any
non-trivial changes in. It's not enough that they work perfectly now; we
need to be able to make sure later changes won't break them.

> I'd very much like to get help to get this in... 

I'll try to find a moment to review your other patches.

I've also had my moments of frustration, but things have really improved
significantly since you've sent those patches. David is doing an
excellent job. Be persistent!


Kind regards,
Jani.

Re: [PATCH 1/2] add notmuch keybinding 'd'

[Tomi Ollila]

On Fri, 06 Jan 2012 15:10:57 -0500, Antoine Beaupré <anarcat@koumbit.org> wrote:
> 
> For the curious, those (other) patches are:
> 
>  * lib: add 'safe' setting for flags
>  * lib: Add back the synchronization of 'T' flag with deleted tag
>  * run notmuch-hello-mode-hook at the end of the hello mode setup

To add other replies, there are now notmuch-hello-mode-hook and
notmuch-hello-refresh-hook impmelented.

> Most of those do not change the current behavior, and I have been
> running them for more than 4 months.
> 
> I'd very much like to get help to get this in... 
> 
> A.

Tomi

Re: [PATCH 1/2] add notmuch keybinding 'd'

[David Edmondson]

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

On Tue, 03 Jan 2012 14:56:50 +0000, David Edmondson <dme@dme.org> wrote:
> On Sat, 16 Jul 2011 14:39:59 -0400, Antoine Beaupré <anarcat@koumbit.org> wrote:
> > It adds a tag 'deleted' and removes the tags 'inbox' and 'unread'. It
> > works in show as well as in search mode
> 
> Various people have asked for a keybinding to add a 'delete' tag. Is
> this version the right one to choose?

No-one has spoken up in favour of this particular change (and the removal
of "unread" seems questionable) so I plan to mark it 'obsolete'. If
anyone would like to bring it back, please post an updated version of
the patch and argue for it.

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

Re: [PATCH 1/2] add notmuch keybinding 'd'

[Antoine Beaupré]

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

On Wed, 04 Jan 2012 08:52:13 +0000, David Edmondson <dme@dme.org> wrote:
> On Tue, 03 Jan 2012 14:56:50 +0000, David Edmondson <dme@dme.org> wrote:
> > On Sat, 16 Jul 2011 14:39:59 -0400, Antoine Beaupré <anarcat@koumbit.org> wrote:
> > > It adds a tag 'deleted' and removes the tags 'inbox' and 'unread'. It
> > > works in show as well as in search mode
> > 
> > Various people have asked for a keybinding to add a 'delete' tag. Is
> > this version the right one to choose?
> 
> No-one has spoken up in favour of this particular change (and the removal
> of "unread" seems questionable) so I plan to mark it 'obsolete'. If
> anyone would like to bring it back, please post an updated version of
> the patch and argue for it.

I do think this is superseded by this:

id:"1325975294-646-4-git-send-email-jrollins@finestructure.net"

Thanks again Jamie. :)

A.

-- 
La nature n'a créé ni maîtres ni esclaves
Je ne veux ni donner ni recevoir de lois.
                        - Denis Diderot

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

Re: [PATCH v2] notmuch.el: add functionality in notmuch search mode to add or remove tags by region

[Carl Worth]

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

On Tue, 16 Feb 2010 19:07:40 -0500, Jesse Rosenthal <jrosenthal@jhu.edu> wrote:
> This patch adds `-region' versions of the `notmuch-search-' commands to find
> properties. It also splits up  `notmuch-add/remove-tags' into both a
> `-thread' and a `-region' version. (This makes us modify
> `notmuch-search-archive-thread' to use the
> `notmuch-search-remove-tag-thread' function, instead of
> `notmuch-search-remove-tag', for consistency.) The add/remove-tag command
> called by pressing `+' or `-' will then choose accordingly, based on whether
> region is active.
> 
> This version fixes a couple of errors in the first version, which led to
> incorrect marking of some tags in the search view (though the actual
> tagging was still correct). It's also based on current master.
> 
> I'm not sure any more if region selection is actually the correct way to
> do this, or if a mutt-style message-marking method would be better. But
> I didn't want a buggy incorrect version out there.

I think this feature is very useful, and that the region is definitely
an appropriate way to implement it, (doing region-based operations is
very natural for emacs users). Mutt-style marking could be implemented
as well, but that would be separate I think.

I tested this patch a bit and added one small cleanup to the
documentation (see below).

I also don't like the duplication of code in
notmuch-search-add-tag-thread and notmuch-search-add-tag-region, (and
the same in the remove case). Fortunately, I think this easy to avoid by
simply making notmuch-search-add-tag-thread call:

       (notmuch-search-add-tag-region tag (point) (point))

and the same in the remove case.

But I haven't pushed this patch yet for a flaw in the case of selecting
beyond the last thread, (such as selecting to the line that includes the
"End of search results). If I try an operation on that line, it will act
like it works, (it displays the new tags by all threads), but then a
refresh makes them all disappear again. Presumably the "notmuch tag"
command is failing, this isn't being noticed, and the code marches on to
update the representation of the tags.

And presumably the notmuch tag is failing because no thread ID is found
for the last line, so the mapconcat call is sticking an extra " or "
onto the end of the search string. And Xapian doesn't like that:

	$ notmuch search tag:inbox or
	A Xapian exception occurred performing query: Syntax: <expression> OR <expression>
        Query string was: tag:inbox or

Things behave even worse if I make the region be the entire buffer,
(including the last blank line). Then the commands hang. I got nervous
that this was then adding "or or" and trying to add/remove a tag to
every message containing the word "or". But I haven't looked closely.

If we can fix this bug, then I'd like to apply this patch. I'd even like
to fix the "*" binding to be implemented in terms of something like
these functions so that we could get visual updates of the tag state
From that command. (But that will take some reworking of the interface
as the current implementation can't support multiple tags added/removed
as * current expects).

I'd appreciate any help fixing up the patch.

-Carl

From fec5622add1a4e9f305c16e96143439ee22a5c58 Mon Sep 17 00:00:00 2001
From: Carl Worth <cworth@cworth.org>
Date: Wed, 7 Apr 2010 13:15:27 -0700
Subject: [PATCH] emacs: Correct the documentation for notmuch-search-add-tag (and -remove-tag)

These commands act on all messages in the thread, not simply those
that match the search. (There are use case for both behaviors, but the
documentation must match the behavior that's actually implemented).
---
 emacs/notmuch.el |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 64f3e3d..517c53a 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -532,12 +532,11 @@ and will also appear in a buffer named \"*Notmuch errors*\"."
 	  (notmuch-search-set-tags (delete tag (notmuch-search-get-tags)))
 	  (forward-line))))))
 
-
 (defun notmuch-search-add-tag (tag)
   "Add a tag to the currently selected thread or region.
 
-The tag is added to messages in the currently selected thread or
-region which match the current search terms."
+The tag is added to all messages in the currently selected thread
+or threads in the current region."
   (interactive
    (list (notmuch-select-tag-with-completion "Tag to add: ")))
   (save-excursion
@@ -550,8 +549,8 @@ region which match the current search terms."
 (defun notmuch-search-remove-tag (tag)
   "Remove a tag from the currently selected thread or region.
 
-The tag is removed from all messages in the currently selected thread
-or region which match the current search terms."
+The tag is removed from all messages in the currently selected
+thread or threads in the current region."
   (interactive
    (list (notmuch-select-tag-with-completion
 	  "Tag to remove: "
-- 
1.7.0


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

Re: [PATCH v2] notmuch.el: add functionality in notmuch search mode to add or remove tags by region

[Jesse Rosenthal]

On Wed, 07 Apr 2010 14:10:38 -0700, Carl Worth <cworth@cworth.org> wrote:
> On Tue, 16 Feb 2010 19:07:40 -0500, Jesse Rosenthal <jrosenthal@jhu.edu> wrote:
> I think this feature is very useful, and that the region is definitely
> an appropriate way to implement it, (doing region-based operations is
> very natural for emacs users). Mutt-style marking could be implemented
> as well, but that would be separate I think.

Great -- never sure if my intuitive use corresponds with that of others.

> I also don't like the duplication of code in
> notmuch-search-add-tag-thread and notmuch-search-add-tag-region, (and
> the same in the remove case). Fortunately, I think this easy to avoid by
> simply making notmuch-search-add-tag-thread call:
> 
>        (notmuch-search-add-tag-region tag (point) (point))
> 
> and the same in the remove case.

Good idea.

> 
> But I haven't pushed this patch yet for a flaw in the case of selecting
> beyond the last thread, (such as selecting to the line that includes the
> "End of search results). If I try an operation on that line, it will act
> like it works, (it displays the new tags by all threads), but then a
> refresh makes them all disappear again. Presumably the "notmuch tag"
> command is failing, this isn't being noticed, and the code marches on to
> update the representation of the tags.

Good point, and shouldn't be too hard to fix. It seems like it could be
done either (a) at the lisp level (filtering out the blank entries), or
(b) at the literal buffer level (having a step that sets point-max for
the region appropriately). The choice (a) seems clearer in an abstract
way, but has the downside of needing to be repeated twice (both for
database tagging, and for rewriting the tags on the screen). I'll play
with both and see which is clearer.

Best,
Jesse

Re: [PATCH v2] notmuch.el: add functionality in notmuch search mode to add or remove tags by region

[Carl Worth]

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

On Fri, 09 Apr 2010 10:11:00 -0400, Jesse Rosenthal <jrosenthal@jhu.edu> wrote:
> I'll play with both and see which is clearer.

Any progress here, Jesse?

I'd like to release notmuch 0.2 soon and would prefer not to revert this
feature, (but I also don't really want to ship it with this known bug).

-Carl

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

Re: [PATCH v2] notmuch.el: add functionality in notmuch search mode to add or remove tags by region

[Jesse Rosenthal]

Sorry -- just got back into town from some business travel. I'll be
working on it this afternoon, and will let you know by this evening
where I get with it.

Best,
Jesse

On Tue, 13 Apr 2010 10:55:55 -0700, Carl Worth <cworth@cworth.org> wrote:
> On Fri, 09 Apr 2010 10:11:00 -0400, Jesse Rosenthal <jrosenthal@jhu.edu> wrote:
> > I'll play with both and see which is clearer.
> 
> Any progress here, Jesse?
> 
> I'd like to release notmuch 0.2 soon and would prefer not to revert this
> feature, (but I also don't really want to ship it with this known bug).
> 
> -Carl
Non-text part: application/pgp-signature

[PATCH] Fix bug, and clean up code duplication, in adding or removing tag by region.

[Jesse Rosenthal]

There was a bug in notmuch-search-{add,remove}-tag-region, which would
not behave correctly if the region went beyond the last message. Now,
instead of simply iterating to the last line of the region, these
functions will iterate to the minimum of the last line of the region
and the last possible line, i.e.

(- (line-number-at-pos (point-max)) 2)

Also clean up code duplication, as per Carl's suggestion, by making
notmuch-search-{add/remove}-tag-thread a special case of the -region
commands, where the region in question is between (point) and (point).
---
 emacs/notmuch.el |   26 ++++++++++++++------------
 1 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index 517c53a..be09f42 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -399,10 +399,11 @@ Complete list of currently available key bindings:
 (defun notmuch-search-properties-in-region (property beg end)
   (save-excursion
     (let ((output nil)
-	  (last-line (line-number-at-pos end)))
+	  (last-line (line-number-at-pos end))
+	  (max-line (- (line-number-at-pos (point-max)) 2)))
       (goto-char beg)
       (beginning-of-line)
-      (while (<= (line-number-at-pos) last-line)
+      (while (<= (line-number-at-pos) (min last-line max-line))
 	(setq output (cons (get-text-property (point) property) output))
 	(forward-line 1))
       output)))
@@ -497,38 +498,39 @@ and will also appear in a buffer named \"*Notmuch errors*\"."
 (defun notmuch-search-get-tags-region (beg end)
   (save-excursion
     (let ((output nil)
-	  (last-line (line-number-at-pos end)))
+	  (last-line (line-number-at-pos end))
+	  (max-line (- (line-number-at-pos (point-max)) 2)))
       (goto-char beg)
-      (while (<= (line-number-at-pos) last-line)
+      (while (<= (line-number-at-pos) (min last-line max-line))
 	(setq output (append output (notmuch-search-get-tags)))
 	(forward-line 1))
       output)))
 
 (defun notmuch-search-add-tag-thread (tag)
-  (notmuch-call-notmuch-process "tag" (concat "+" tag) (notmuch-search-find-thread-id))
-  (notmuch-search-set-tags (delete-dups (sort (cons tag (notmuch-search-get-tags)) 'string<))))
+  (notmuch-search-add-tag-region tag (point) (point)))
 
 (defun notmuch-search-add-tag-region (tag beg end)
   (let ((search-id-string (mapconcat 'identity (notmuch-search-find-thread-id-region beg end) " or ")))
     (notmuch-call-notmuch-process "tag" (concat "+" tag) search-id-string)
     (save-excursion
-      (let ((last-line (line-number-at-pos end)))
+      (let ((last-line (line-number-at-pos end))
+	    (max-line (- (line-number-at-pos (point-max)) 2)))
 	(goto-char beg)
-	(while (<= (line-number-at-pos) last-line)
+	(while (<= (line-number-at-pos) (min last-line max-line))
 	  (notmuch-search-set-tags (delete-dups (sort (cons tag (notmuch-search-get-tags)) 'string<)))
 	  (forward-line))))))
 
 (defun notmuch-search-remove-tag-thread (tag)
-  (notmuch-call-notmuch-process "tag" (concat "-" tag) (notmuch-search-find-thread-id))
-  (notmuch-search-set-tags (delete tag (notmuch-search-get-tags))))
+  (notmuch-search-remove-tag-region tag (point) (point)))
 
 (defun notmuch-search-remove-tag-region (tag beg end)
   (let ((search-id-string (mapconcat 'identity (notmuch-search-find-thread-id-region beg end) " or ")))
     (notmuch-call-notmuch-process "tag" (concat "-" tag) search-id-string)
     (save-excursion
-      (let ((last-line (line-number-at-pos end)))
+      (let ((last-line (line-number-at-pos end))
+	    (max-line (- (line-number-at-pos (point-max)) 2)))
 	(goto-char beg)
-	(while (<= (line-number-at-pos) last-line)
+	(while (<= (line-number-at-pos) (min last-line max-line))
 	  (notmuch-search-set-tags (delete tag (notmuch-search-get-tags)))
 	  (forward-line))))))
 
-- 
1.6.5.3

Re: [PATCH] Fix bug, and clean up code duplication, in adding or removing tag by region.

[Carl Worth]

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

On Tue, 13 Apr 2010 14:47:19 -0400, Jesse Rosenthal <jrosenthal@jhu.edu> wrote:
> There was a bug in notmuch-search-{add,remove}-tag-region, which would
> not behave correctly if the region went beyond the last message. Now,
> instead of simply iterating to the last line of the region, these
> functions will iterate to the minimum of the last line of the region
> and the last possible line, i.e.

Thanks, Jesse!

I tested this and it works great. 

> (- (line-number-at-pos (point-max)) 2)

The only real problem I see with this approach is that it's fragile in
depending on the buffer having exactly 2 lines of non-thread text at the
end. I can easily see myself wanting to remove the "End of Search
Results" line at the end of the buffer. And if I do that, this code will
break, (tag manipulations will miss the last message).

A more robust fix would check for the ability to read a thread
ID. So making a single function such as
notmuch-search-find-last-line-with-thread-id or so would do the trick
here.

> Also clean up code duplication, as per Carl's suggestion, by making
> notmuch-search-{add/remove}-tag-thread a special case of the -region
> commands, where the region in question is between (point) and (point).

A very nice change as well. My internal alarm on "also" in a commit
message fired, so I took advantage of "git add -p" and "git rebase -i"
to split this portion into a separate commit.

All pushed now.

-Carl

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

Re: [PATCH] Fix bug, and clean up code duplication, in adding or removing tag by region.

[Jesse Rosenthal]

On Wed, 14 Apr 2010 10:14:46 -0700, Carl Worth <cworth@cworth.org> wrote:
> The only real problem I see with this approach is that it's fragile in
> depending on the buffer having exactly 2 lines of non-thread text at the
> end. I can easily see myself wanting to remove the "End of Search
> Results" line at the end of the buffer. And if I do that, this code will
> break, (tag manipulations will miss the last message).

I was a bit worried about that myself. I hard-coded the 2 based on the
hard-coding in `notmuch-search-last-thread' (">"), which currently goes
to the bottom and then goes up two lines. But it seems like if there
were a more robust approach, it could be used in both.

> A more robust fix would check for the ability to read a thread
> ID. So making a single function such as
> notmuch-search-find-last-line-with-thread-id or so would do the trick
> here.

It occurs to me that the best way to do this would probably be to go to
point-max, and then (forward-line -1) until we hit a thread-id. That way
we wouldn't have to work all the way down long search indexes. I'll try
to code that up for the next release, and then have
notmuch-search-last-thread use it, as well as the region functions.

Best,
Jesse

Re: [PATCH] Fix bug, and clean up code duplication, in adding or removing tag by region.

[Mark Anderson]

On Wed, 14 Apr 2010 12:50:50 -0500, Jesse Rosenthal <jrosenthal@jhu.edu> wrote:
> It occurs to me that the best way to do this would probably be to go to
> point-max, and then (forward-line -1) until we hit a thread-id. That way
> we wouldn't have to work all the way down long search indexes. I'll try
> to code that up for the next release, and then have
> notmuch-search-last-thread use it, as well as the region functions.

This sounds great, just be careful if this command is run before the
buffer has completed loading, as you could be in the middle of a search
instead of the end.

AFAIK, with the "asynchronous" buffer loading, there's no guarantee that
point-max is the end of the search until the other thread has exited.

Again, my lisp-fu is very poor, but just a concern I see.

-Mark