[PATCH] Simplify "unread" tag handling in emacs UI.

[PATCH] Simplify "unread" tag handling in emacs UI.

[Jameson Rollins]

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

This patch is intended to greatly simplify the handling of the
"unread" tag in the emacs UI.  This patch adds a new function
'notmuch-show-mark-read', that removes the "unread" tag in
notmuch-show-mode.  This function is then executed as a
notmuch-show-hook, and by notmuch-show-next-message.  All of the
functions that explicitly marked messages as unread are removed or
renamed.

The idea here is that the user should never have to worry about
manipulating the "unread" tag.  The tag should persist until the user
actually views a message, at which point it should be automatically
removed.  This patch simplifies the notmuch.el quite a bit, removing a
lot of somewhat redundant functions, and reducing clutter in the name
and key-mapping space.
---
 notmuch.el |   95 +++++++++++++++++++----------------------------------------
 1 files changed, 31 insertions(+), 64 deletions(-)

diff --git a/notmuch.el b/notmuch.el
index 97914f2..2da3c06 100644
--- a/notmuch.el
+++ b/notmuch.el
@@ -69,15 +69,12 @@
     (define-key map "v" 'notmuch-show-view-all-mime-parts)
     (define-key map "-" 'notmuch-show-remove-tag)
     (define-key map "+" 'notmuch-show-add-tag)
-    (define-key map "X" 'notmuch-show-mark-read-then-archive-then-exit)
     (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 "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)
     (define-key map (kbd "DEL") 'notmuch-show-rewind)
-    (define-key map " " 'notmuch-show-advance-marking-read-and-archiving)
+    (define-key map " " 'notmuch-show-advance-and-archive)
     map)
   "Keymap for \"notmuch show\" buffers.")
 (fset 'notmuch-show-mode-map notmuch-show-mode-map)
@@ -226,13 +223,22 @@ Unlike builtin `previous-line' this version accepts no arguments."
 			 (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)
+(defun notmuch-show-archive-thread ()
+  "Archive 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)
   (save-excursion
     (goto-char (point-min))
     (while (not (eobp))
-      (if markread
-	  (notmuch-show-remove-tag "unread" "inbox")
-	(notmuch-show-remove-tag "inbox"))
+      (notmuch-show-remove-tag "inbox")
       (if (not (eobp))
 	  (forward-char))
       (if (not (re-search-forward notmuch-show-message-begin-regexp nil t))
@@ -245,47 +251,12 @@ Unlike builtin `previous-line' this version accepts no arguments."
 	  (forward-line)
 	  (notmuch-search-show-thread)))))
 
-(defun notmuch-show-mark-read-then-archive-thread ()
-  "Remove unread tags from thread, then archive and show next thread.
-
-Archive each message currently shown by removing the \"unread\"
-and \"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-thread-maybe-mark-read t))
-
-(defun notmuch-show-archive-thread ()
-  "Archive 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-thread-maybe-mark-read nil))
-
 (defun notmuch-show-archive-thread-then-exit ()
   "Archive each message in thread, then exit back to search results."
   (interactive)
   (notmuch-show-archive-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)
-  (notmuch-show-mark-read-then-archive-thread)
-  (kill-this-buffer))
-
 (defun notmuch-show-view-raw-message ()
   "View the raw email of the current message."
   (interactive)
@@ -440,7 +411,8 @@ Returns nil if already on the last message in the buffer."
     (while (point-invisible-p)
       (backward-char))
     (recenter 0)
-    nil))
+    nil)
+  (notmuch-show-mark-read))
 
 (defun notmuch-show-find-next-message ()
   "Returns the position of the next message in the buffer.
@@ -503,14 +475,8 @@ it."
       (notmuch-show-previous-message)
       (point))))
 
-(defun notmuch-show-mark-read-then-next-open-message ()
-  "Remove unread tag from this message, then advance to next open message."
-  (interactive)
-  (notmuch-show-remove-tag "unread")
-  (notmuch-show-next-open-message))
-
 (defun notmuch-show-rewind ()
-  "Backup through the thread, (reverse scrolling compared to \\[notmuch-show-advance-marking-read-and-archiving]).
+  "Backup through the thread, (reverse scrolling compared to \\[notmuch-show-advance-and-archive]).
 
 Specifically, if the beginning of the previous email is fewer
 than `window-height' lines from the current point, move to it
@@ -520,7 +486,7 @@ Otherwise, just scroll down a screenful of the current message.
 
 This command does not modify any message tags, (it does not undo
 any effects from previous calls to
-`notmuch-show-advance-marking-read-and-archiving'."
+`notmuch-show-advance-and-archive'."
   (interactive)
   (let ((previous (notmuch-show-find-previous-message)))
     (if (> (count-lines previous (point)) (- (window-height) next-screen-context-lines))
@@ -531,8 +497,8 @@ any effects from previous calls to
 	  (goto-char (window-start)))
       (notmuch-show-previous-message))))
 
-(defun notmuch-show-advance-marking-read-and-archiving ()
-  "Advance through thread, marking read and archiving.
+(defun notmuch-show-advance-and-archive ()
+  "Advance through thread and archive.
 
 This command is intended to be one of the simplest ways to
 process a thread of email. It does the following:
@@ -541,8 +507,7 @@ If the current message in the thread is not yet fully visible,
 scroll by a near screenful to read more of the message.
 
 Otherwise, (the end of the current message is already within the
-current window), remove the \"unread\" tag (if present) from the
-current message and advance to the next open message.
+current window), advance to the next open message.
 
 Finally, if there is no further message to advance to, and this
 last message is already read, then archive the entire current
@@ -555,7 +520,7 @@ which this thread was originally shown."
     (if (> next (window-end))
 	(scroll-up nil)
       (let ((last (notmuch-show-last-message-p)))
-	(notmuch-show-mark-read-then-next-open-message)
+	(notmuch-show-next-open-message)
 	(if last
 	    (notmuch-show-archive-thread))))))
 
@@ -878,17 +843,15 @@ pressing RET after positioning the cursor on a hidden part, (for
 which \\[notmuch-show-next-button] and \\[notmuch-show-previous-button] are helpful).
 
 Reading the thread sequentially is well-supported by pressing
-\\[notmuch-show-advance-marking-read-and-archiving]. This will scroll the current message (if necessary),
-advance to the next message, or advance to the next thread (if
-already on the last message of a thread). As each message is
-scrolled away its \"unread\" tag will be removed, and as each
-thread is scrolled away the \"inbox\" tag will be removed from
-each message in the thread.
+\\[notmuch-show-advance-and-archive]. This will
+scroll the current message (if necessary), advance to the next
+message, or advance to the next thread (if already on the last
+message of a thread).
 
 Other commands are available to read or manipulate the thread more
 selectively, (such as '\\[notmuch-show-next-message]' and '\\[notmuch-show-previous-message]' to advance to messages without
 removing any tags, and '\\[notmuch-show-archive-thread]' to archive an entire thread without
-scrolling through with \\[notmuch-show-advance-marking-read-and-archiving]).
+scrolling through with \\[notmuch-show-advance-and-archive]).
 
 You can add or remove arbitary tags from the current message with
 '\\[notmuch-show-add-tag]' or '\\[notmuch-show-remove-tag]'.
@@ -922,10 +885,14 @@ All currently available key bindings:
 
 ; Make show mode a bit prettier, highlighting URLs and using word wrap
 
+(defun notmuch-show-mark-read ()
+  (notmuch-show-remove-tag "unread"))
+
 (defun notmuch-show-pretty-hook ()
   (goto-address-mode 1)
   (visual-line-mode))
 
+(add-hook 'notmuch-show-hook 'notmuch-show-mark-read)
 (add-hook 'notmuch-show-hook 'notmuch-show-pretty-hook)
 (add-hook 'notmuch-search-hook
 	  (lambda()
-- 
1.6.5

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

Re: [PATCH] Simplify "unread" tag handling in emacs UI.

[Sebastian Spaeth]

On Tue, 19 Jan 2010 17:54:16 -0500, Jameson Rollins <jrollins@finestructure.net> wrote:
> This patch is intended to greatly simplify the handling of the
> "unread" tag in the emacs UI.  This patch adds a new function
> 'notmuch-show-mark-read', that removes the "unread" tag in
> notmuch-show-mode.  This function is then executed as a
> notmuch-show-hook, and by notmuch-show-next-message.  All of the
> functions that explicitly marked messages as unread are removed or
> renamed.

Hi Jameson,

I've been using this for quite some time now and I am pretty happy with
it. However, there is a problem with this approach as this renders the
function notmuch-show-next-unread-message useless. 

This proceeds to the next message, using
notmuch-show-next-message, which your patch marks automatically as read.

It then checks the unread status in order to decide whether to proceed
to the next again. So with your patch notmuch-show-next-unread-message
will skip through all messages in a thread thinking they are all read
(and actually marking all as read).

Sebastian

Re: [PATCH] Simplify "unread" tag handling in emacs UI.

[Jameson Rollins]

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

On Wed, 17 Feb 2010 14:33:11 +0100, "Sebastian Spaeth" <Sebastian@SSpaeth.de> wrote:
> I've been using this for quite some time now and I am pretty happy with
> it. However, there is a problem with this approach as this renders the
> function notmuch-show-next-unread-message useless. 
> 
> This proceeds to the next message, using
> notmuch-show-next-message, which your patch marks automatically as read.
> 
> It then checks the unread status in order to decide whether to proceed
> to the next again. So with your patch notmuch-show-next-unread-message
> will skip through all messages in a thread thinking they are all read
> (and actually marking all as read).

Hey, Sebastian.  I'm glad you've been finding the patch useful.  I'd be
happy to look at improving it, or of course accepting patch.  I was
hoping Carl might process these patch first, in which case it would be
easier to know what any improvements might look like.  I'll send a
separate email to ping Carl about that.

jamie.

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

Re: [PATCH] Simplify "unread" tag handling in emacs UI.

[Carl Worth]

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

On Wed, 17 Feb 2010 14:33:11 +0100, "Sebastian Spaeth" <Sebastian@SSpaeth.de> wrote:
> On Tue, 19 Jan 2010 17:54:16 -0500, Jameson Rollins <jrollins@finestructure.net> wrote:
> > This patch is intended to greatly simplify the handling of the
> > "unread" tag in the emacs UI.  This patch adds a new function
> > 'notmuch-show-mark-read', that removes the "unread" tag in
> > notmuch-show-mode.  This function is then executed as a
> > notmuch-show-hook, and by notmuch-show-next-message.  All of the
> > functions that explicitly marked messages as unread are removed or
> > renamed.

Hi Jameson,

Thanks for contributing the patch. This exact feature, (removing all
commands with "and mark read" in their names), has been on my todo list
for too long, and I'm anxious to remove it from that. But...

> It then checks the unread status in order to decide whether to proceed
> to the next again. So with your patch notmuch-show-next-unread-message
> will skip through all messages in a thread thinking they are all read
> (and actually marking all as read).

...that seems like a fatal bug in this script. Thanks for noting that
Sebastian.

I'm very interested in augmenting the test suite such that we could
write emacs-based tests that would capture this bug. It seems that with
the --batch, --load, and --funcall options it shouldn't be too hard to
write a modular function exercising emacs code and then execute it
no-interactively. Then we could either inspect the state of the database
to ensure the operation worked as desired, or else have the test
function write a buffer out to a file and test that the file has the
desired contents.

So that's something I'll be working on soon. And of course, I'll always
be glad to accept any help.

-Carl

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

Re: [PATCH] Simplify "unread" tag handling in emacs UI.

[Jameson Rollins]

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

On Wed, 24 Feb 2010 10:26:47 -0800, Carl Worth <cworth@cworth.org> wrote:
> On Wed, 17 Feb 2010 14:33:11 +0100, "Sebastian Spaeth" <Sebastian@SSpaeth.de> wrote:
> > On Tue, 19 Jan 2010 17:54:16 -0500, Jameson Rollins <jrollins@finestructure.net> wrote:
> > > This patch is intended to greatly simplify the handling of the
> > > "unread" tag in the emacs UI.  This patch adds a new function
> > > 'notmuch-show-mark-read', that removes the "unread" tag in
> > > notmuch-show-mode.  This function is then executed as a
> > > notmuch-show-hook, and by notmuch-show-next-message.  All of the
> > > functions that explicitly marked messages as unread are removed or
> > > renamed.

> Thanks for contributing the patch. This exact feature, (removing all
> commands with "and mark read" in their names), has been on my todo list
> for too long, and I'm anxious to remove it from that. But...
>
> > It then checks the unread status in order to decide whether to proceed
> > to the next again. So with your patch notmuch-show-next-unread-message
> > will skip through all messages in a thread thinking they are all read
> > (and actually marking all as read).
> 
> ...that seems like a fatal bug in this script. Thanks for noting that
> Sebastian.

I certainly don't see it as fatal, but it is something we should
resolve.  I think the simplification that the patch provides is worth
it.

I'm seeing the notmuch-show-next-unread-message as a non-interactive
function that's not currently called by any other functions, and is
therefore not being used.  Sebastian, are you using that in a private
function, or am I misreading the code?

jamie.

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

Re: [PATCH] Simplify "unread" tag handling in emacs UI.

[Sebastian Spaeth]

On Wed, 24 Feb 2010 14:20:27 -0500, Jameson Rollins <jrollins@finestructure.net> wrote:
> On Wed, 24 Feb 2010 10:26:47 -0800, Carl Worth <cworth@cworth.org> wrote:

> > > It then checks the unread status in order to decide whether to proceed
> > > to the next again. So with your patch notmuch-show-next-unread-message
> > > will skip through all messages in a thread thinking they are all read
> > > (and actually marking all as read).
> > 
> > ...that seems like a fatal bug in this script. Thanks for noting that
> > Sebastian.
> 
> I certainly don't see it as fatal, but it is something we should
> resolve.  I think the simplification that the patch provides is worth
> it.

Well, it is fatal in the sense that it makes that function useless. But
it's not as bad as it is currently unused. You will only notice it if you
make the "skip to next unread" (interactive) and bind it to a key (which
is what I have done). I still think it's worth taking this patch and fixing it then.

The solutions I can see are: 
- split into a "notmuch-show-next-message"
  and a notmuch-show-show-next-message (what a naming!) function. 
  One would merely skip to the next and one actually show and mark unread.
- Introduce a second argument option whether we should mark as unread or not.
- rework "notmuch-show-next-unread-message" to not make use of
  "notmuch-show-next-message" but e.g. to just use
  "notmuch-show-find-next-message" or so.

Does this make any sense?

> I'm seeing the notmuch-show-next-unread-message as a non-interactive
> function that's not currently called by any other functions, and is
> therefore not being used.  Sebastian, are you using that in a private
> function, or am I misreading the code?

Touché. As explained above. I made it interactive and bound it to 'n'.

Sebastian

Re: [PATCH] Simplify "unread" tag handling in emacs UI.

[Carl Worth]

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

On Thu, 25 Feb 2010 11:23:40 +0100, "Sebastian Spaeth" <Sebastian@SSpaeth.de> wrote:
> Well, it is fatal in the sense that it makes that function useless. But
> it's not as bad as it is currently unused.

Right. I didn't want to push something out with a known-broken function.

I would have rather the patch just remove the function in that case,
(but the fact that someone noticed the broken function meant that
someone actually *was* using it and removing it wouldn't be nice
either).

> I still think it's worth taking this patch and fixing it then.

Done and pushed out now.

> - split into a "notmuch-show-next-message"
>   and a notmuch-show-show-next-message (what a naming!) function. 
>   One would merely skip to the next and one actually show and mark
>   unread.

That's the approach I took, with naming of
notmuch-show-next-message-without-marking-read. And yes, the naming we
have for our emacs functions is awful. I really dislike "notmuch-show"
as a prefix because it puts a verb in the prefix, (where I'd instead
like to just have a verb for the actual functionality of the function).

But we have to have at least *something* to separate the search-results
and message-viewing modes, (since we have commands like "archive" in
both of them). The lack of nice scoping here is really awkward.

-Carl

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