[PATCH WIP v2 0/5] emacs: show: redesign unread/read logic

[PATCH WIP v2 0/5] emacs: show: redesign unread/read logic

[Mark Walters]

This is further wip of the message at
id:1385285551-5158-1-git-send-email-markwalters1009@gmail.com
see there for some discussion of the design.

This series is still definitely wip: one reason for posting is so
people can play with different strategies for marking read
easily. (As WIP tree's unread handling is broken and the tests need updating.)

The series consists of three parts: the first 4 patches add the notion
of seen: this means the user has seen the message but the message has
typically not been marked read yet. The seen messages are marked read
when the user quits the show buffer unless the user quits with
prefix-arg quit. In all cases an informative message is shown.

The fifth patch adds a psot-command-hook stub for updating the seen
status. This seems a natural place to do the update as it means
however the user navugates around the buffer (eg next-message or
page-down etc) the update gets done.

This is intended to be an easy place for other people to try out their
own mark read strategies.

The final patch implements something pretty close to what I would like
for marking seen/read. A message is deemed seen provided the user has
seen the top of the message, and has seen either the bottom of the
message or a point at least some customisable number of lines into the
message. The customisable number of lines can either be a fixed number
e.g. 20, or a number depending on the height of the current window
e.g. the default is 3/4 of the window height.

The idea is a message seen if the user has seen the entire message, or
enough of it they have to have noticed it. The figure of 3/4 also
means that the notmuch commands like next-message which place the top
of the message at the top of the window automatically mark the message
seen as either the whole message or at least one window full must be
visible.

I would be very grateful for any comments on whether this behaves as
people would expect, what they would want instead etc

Best wishes

Mark





Mark Walters (6):
  emacs: show: add a function to test for unread
  emacs: show: add some seen helpers
  emacs: show: sync seen to read on quit and refresh
  emacs: show: use the `seen' interface
  emacs: show: add an update seen function to post-command-hook
  emacs: show: make `seen' mean user viewed whole message

 emacs/notmuch-show.el |  163 ++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 148 insertions(+), 15 deletions(-)

-- 
1.7.9.1

[PATCH WIP v2 1/6] emacs: show: add a function to test for unread

[Mark Walters]

This adds a function which tests for unread. A message is deemed
unread if applying `notmuch-show-mark-read-tags' would change the
message's tags.
---
 emacs/notmuch-show.el |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 784644c..a135e79 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1510,6 +1510,17 @@ current thread."
   "Are the headers of the current message visible?"
   (notmuch-show-get-prop :headers-visible))
 
+(defun notmuch-show-unread-p ()
+  "Return t if current message is unread.
+
+Returns t unless applying `notmuch-show-mark-read-tags' would be
+a no-op"
+  (when notmuch-show-mark-read-tags
+    (let* ((current-tags (notmuch-show-get-tags))
+	   (tag-changes (notmuch-tag-change-list notmuch-show-mark-read-tags))
+	   (new-tags (notmuch-update-tags current-tags tag-changes)))
+      (not (equal current-tags new-tags)))))
+
 (put 'notmuch-show-mark-read 'notmuch-prefix-doc
      "Mark the current message as unread.")
 (defun notmuch-show-mark-read (&optional unread)
-- 
1.7.9.1

[PATCH WIP v2 2/6] emacs: show: add some seen helpers

[Mark Walters]

We separate out "seeing" a message from marking it read. The intention
is only to mark read at certain times (eg quitting a show buffer). We
use the term seen to mean we have seen the message (for some
definition of seen) in the current buffer, but have not (typically)
marked the message read yet.

This adds helper functions to test/set the seen property and a
function to tag all seen messages read. The last of these tells the
user how many messages have been marked read.  It also takes an
argument that tells the function not to do any marking.  This last use
case displays a messages saying "Not marking messages read" and keeps
the logic of what to do when the user does not want to sync seen to
read in one place.
---
 emacs/notmuch-show.el |   34 ++++++++++++++++++++++++++++++++++
 1 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index a135e79..c63d295 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1536,6 +1536,40 @@ marked as unread, i.e. the tag changes in
     (apply 'notmuch-show-tag-message
 	   (notmuch-tag-change-list notmuch-show-mark-read-tags unread))))
 
+(defun notmuch-show-seen-p ()
+  "Return t if current message has been seen."
+  (notmuch-show-get-prop :seen))
+
+(defun notmuch-show-mark-seen ()
+  "Mark current message seen."
+  (notmuch-show-set-prop :seen t))
+
+(defun notmuch-show-mark-all-seen-read (&optional not-mark)
+  "Mark read all messages that have been seen in this buffer.
+
+If NOT-MARK then do not mark the messages read, and tell the user
+we are not marking them."
+  (if not-mark
+      (message "Not marking messages read")
+    (let ((messages-to-mark-read))
+      ;; We get a list of all message to tag read. A list means that
+      ;; we can tag all the messages in one tag operation rather than
+      ;; needing one per read message.
+      (notmuch-show-mapc
+       (lambda ()
+	 (when (and (notmuch-show-seen-p) (notmuch-show-unread-p))
+	   (push (notmuch-show-get-message-id) messages-to-mark-read))))
+      (when messages-to-mark-read
+	(notmuch-tag (mapconcat #'identity messages-to-mark-read " ")
+		     (notmuch-tag-change-list notmuch-show-mark-read-tags)))
+      (let ((count (length messages-to-mark-read)))
+	(cond ((> count 1)
+	       (message "Marked %s messages read" count))
+	      ((= count 1)
+	       (message "Marked one message read"))
+	      ((= count 0)
+	       (message "No messages marked read")))))))
+
 ;; Functions for getting attributes of several messages in the current
 ;; thread.
 
-- 
1.7.9.1

[PATCH WIP v2 3/6] emacs: show: sync seen to read on quit and refresh

[Mark Walters]

This makes notmuch show buffers mark all seen messages as read when
quitting or refreshing the show buffer, accept in the case these are
called with a prefix argument.

This slightly overloads the prefix-argument to refresh: as it tells
refresh not to save state either. This is probably OK as it says "make
the buffer look like the database".

Note, at this point the seen functionality is not yet used: that is
nothing yet marks a message seen.
---
 emacs/notmuch-show.el |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index c63d295..05f649b 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1213,6 +1213,11 @@ preferences. If invoked with a prefix argument (or RESET-STATE is
 non-nil) then the state of the buffer (open/closed messages) is
 reset based on the original query."
   (interactive "P")
+  ;; Do not mark seen messages read if we are resetting state. The
+  ;; idea is that resetting state is asking for the view to be reset
+  ;; to the current state of the database.
+  (notmuch-show-mark-all-seen-read reset-state)
+
   (let ((inhibit-read-only t)
 	(state (unless reset-state
 		 (notmuch-show-capture-state))))
@@ -1258,6 +1263,8 @@ reset based on the original query."
 (defvar notmuch-show-mode-map
       (let ((map (make-sparse-keymap)))
 	(set-keymap-parent map notmuch-common-keymap)
+	;; the following overrides the common-keymap quit
+	(define-key map [remap notmuch-kill-this-buffer] 'notmuch-show-quit-and-mark-read)
 	(define-key map "Z" 'notmuch-tree-from-show-current-query)
 	(define-key map (kbd "<C-tab>") 'widget-backward)
 	(define-key map (kbd "M-TAB") 'notmuch-show-previous-button)
@@ -1570,6 +1577,14 @@ we are not marking them."
 	      ((= count 0)
 	       (message "No messages marked read")))))))
 
+(put 'notmuch-show-quit-and-mark-read 'notmuch-prefix-doc
+     "... without marking seen messages read.")
+(defun notmuch-show-quit-and-mark-read (&optional not-mark)
+  "Kill the current buffer marking seen messages read."
+  (interactive "P")
+  (notmuch-show-mark-all-seen-read not-mark)
+  (notmuch-kill-this-buffer))
+
 ;; Functions for getting attributes of several messages in the current
 ;; thread.
 
-- 
1.7.9.1

[PATCH WIP v2 4/6] emacs: show: use the `seen' interface

[Mark Walters]

This converts the existing logic to use the seen/read distinction. At
this point the logic for deciding whether to mark seen/read is
unchanged, but the actual tagging read is now deferred to
quit/refresh.
---
 emacs/notmuch-show.el |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 05f649b..261c2e3 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1721,7 +1721,7 @@ thread, navigate to the next thread in the parent search buffer."
   (interactive "P")
   (if (notmuch-show-goto-message-next)
       (progn
-	(notmuch-show-mark-read)
+	(notmuch-show-mark-seen)
 	(notmuch-show-message-adjust))
     (if pop-at-end
 	(notmuch-show-next-thread)
@@ -1733,7 +1733,7 @@ thread, navigate to the next thread in the parent search buffer."
   (if (= (point) (notmuch-show-message-top))
       (notmuch-show-goto-message-previous)
     (notmuch-show-move-to-message-top))
-  (notmuch-show-mark-read)
+  (notmuch-show-mark-seen)
   (notmuch-show-message-adjust))
 
 (defun notmuch-show-next-open-message (&optional pop-at-end)
@@ -1749,7 +1749,7 @@ to show, nil otherwise."
 		(not (notmuch-show-message-visible-p))))
     (if r
 	(progn
-	  (notmuch-show-mark-read)
+	  (notmuch-show-mark-seen)
 	  (notmuch-show-message-adjust))
       (if pop-at-end
 	  (notmuch-show-next-thread)
@@ -1764,7 +1764,7 @@ to show, nil otherwise."
 		(not (notmuch-show-get-prop :match))))
     (if r
 	(progn
-	  (notmuch-show-mark-read)
+	  (notmuch-show-mark-seen)
 	  (notmuch-show-message-adjust))
       (goto-char (point-max)))))
 
@@ -1777,7 +1777,7 @@ to show, nil otherwise."
   "Move to the first open message and mark it read"
   (goto-char (point-min))
   (if (notmuch-show-message-visible-p)
-      (notmuch-show-mark-read)
+      (notmuch-show-mark-seen)
     (notmuch-show-next-open-message))
   (when (eobp)
     ;; There are no matched non-excluded messages so open all matched
@@ -1786,7 +1786,7 @@ to show, nil otherwise."
     (force-window-update)
     (goto-char (point-min))
     (if (notmuch-show-message-visible-p)
-	(notmuch-show-mark-read)
+	(notmuch-show-mark-seen)
       (notmuch-show-next-open-message))))
 
 (defun notmuch-show-previous-open-message ()
@@ -1796,7 +1796,7 @@ to show, nil otherwise."
 		  (notmuch-show-goto-message-previous)
 		(notmuch-show-move-to-message-top))
 	      (not (notmuch-show-message-visible-p))))
-  (notmuch-show-mark-read)
+  (notmuch-show-mark-seen)
   (notmuch-show-message-adjust))
 
 (defun notmuch-show-view-raw-message ()
-- 
1.7.9.1

[PATCH WIP v2 5/6] emacs: show: add an update seen function to post-command-hook

[Mark Walters]

Add a function stub for updating seen messages to the
post-command-hook. This dummy function gets called with parameters the
start and end of the current window and can decide what to mark seen
based on that.

Since this is in the post-command-hook it should get called after most
user actions (exceptions include user resizing the window) so it
should be possible to make sure the seen status gets updated whether
the user uses notmuch commands like next-message or normal emacs
commands like scroll-up.

It also removes all of the old mark read/seen points to give a clean
slate for testing new mark read/seen algorithms.
---
 emacs/notmuch-show.el |   32 +++++++++++++++++---------------
 1 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 261c2e3..6b6e94a 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1142,6 +1142,8 @@ function is used."
   (let ((inhibit-read-only t))
 
     (notmuch-show-mode)
+    (add-hook 'post-command-hook #'notmuch-show-command-hook nil t)
+
     ;; Don't track undo information for this buffer
     (set 'buffer-undo-list t)
 
@@ -1585,6 +1587,16 @@ we are not marking them."
   (notmuch-show-mark-all-seen-read not-mark)
   (notmuch-kill-this-buffer))
 
+(defun notmuch-show-do-seen (start end)
+  "Update seen status for all messages between start and end."
+  )
+
+(defun notmuch-show-command-hook ()
+  (when (eq major-mode 'notmuch-show-mode)
+    ;; We need to redisplay to get window-start and window-end correct.
+    (redisplay)
+    (notmuch-show-do-seen (window-start) (window-end))))
+
 ;; Functions for getting attributes of several messages in the current
 ;; thread.
 
@@ -1720,9 +1732,7 @@ If a prefix argument is given and this is the last message in the
 thread, navigate to the next thread in the parent search buffer."
   (interactive "P")
   (if (notmuch-show-goto-message-next)
-      (progn
-	(notmuch-show-mark-seen)
-	(notmuch-show-message-adjust))
+      (notmuch-show-message-adjust)
     (if pop-at-end
 	(notmuch-show-next-thread)
       (goto-char (point-max)))))
@@ -1733,7 +1743,6 @@ thread, navigate to the next thread in the parent search buffer."
   (if (= (point) (notmuch-show-message-top))
       (notmuch-show-goto-message-previous)
     (notmuch-show-move-to-message-top))
-  (notmuch-show-mark-seen)
   (notmuch-show-message-adjust))
 
 (defun notmuch-show-next-open-message (&optional pop-at-end)
@@ -1748,9 +1757,7 @@ to show, nil otherwise."
     (while (and (setq r (notmuch-show-goto-message-next))
 		(not (notmuch-show-message-visible-p))))
     (if r
-	(progn
-	  (notmuch-show-mark-seen)
-	  (notmuch-show-message-adjust))
+	(notmuch-show-message-adjust)
       (if pop-at-end
 	  (notmuch-show-next-thread)
 	(goto-char (point-max))))
@@ -1763,9 +1770,7 @@ to show, nil otherwise."
     (while (and (setq r (notmuch-show-goto-message-next))
 		(not (notmuch-show-get-prop :match))))
     (if r
-	(progn
-	  (notmuch-show-mark-seen)
-	  (notmuch-show-message-adjust))
+	(notmuch-show-message-adjust)
       (goto-char (point-max)))))
 
 (defun notmuch-show-open-if-matched ()
@@ -1776,8 +1781,7 @@ to show, nil otherwise."
 (defun notmuch-show-goto-first-wanted-message ()
   "Move to the first open message and mark it read"
   (goto-char (point-min))
-  (if (notmuch-show-message-visible-p)
-      (notmuch-show-mark-seen)
+  (unless (notmuch-show-message-visible-p)
     (notmuch-show-next-open-message))
   (when (eobp)
     ;; There are no matched non-excluded messages so open all matched
@@ -1785,8 +1789,7 @@ to show, nil otherwise."
     (notmuch-show-mapc 'notmuch-show-open-if-matched)
     (force-window-update)
     (goto-char (point-min))
-    (if (notmuch-show-message-visible-p)
-	(notmuch-show-mark-seen)
+    (unless (notmuch-show-message-visible-p)
       (notmuch-show-next-open-message))))
 
 (defun notmuch-show-previous-open-message ()
@@ -1796,7 +1799,6 @@ to show, nil otherwise."
 		  (notmuch-show-goto-message-previous)
 		(notmuch-show-move-to-message-top))
 	      (not (notmuch-show-message-visible-p))))
-  (notmuch-show-mark-seen)
   (notmuch-show-message-adjust))
 
 (defun notmuch-show-view-raw-message ()
-- 
1.7.9.1

[PATCH WIP v2 6/6] emacs: show: make `seen' mean user viewed whole message

[Mark Walters]

This changes `seen' to mean that the user viewed `enough' of the whole
message: more precisely, a message is deemed seen if the top of the
message and either the bottom of the message or a point at least some
customisable number of lines into the message have each been visible
in the buffer at some point.

This is placed into the post-command-hook infrastructure introudced in
the previous patch.
---
 emacs/notmuch-show.el |   75 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 73 insertions(+), 2 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 6b6e94a..a57d8fb 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -211,6 +211,19 @@ For example, if you wanted to remove an \"unread\" tag and add a
   :type '(repeat string)
   :group 'notmuch-show)
 
+(defcustom notmuch-show-seen-lines-needed 0.75
+  "Control which messages get marked seen.
+
+A message is marked seen if both the top of the message and a
+point far \"enough\" down in the message have each been visible
+in the buffer at some point. This parameter controls the
+definition of enough.  Seeing the bottom of message is always
+deemed enough, but additionally...it an integer n then at least n
+lines of message must be visible in the window. If it is a float
+x then at least that proportion of the window must contain the
+message."
+  :type 'number
+  :group 'notmuch-show)
 
 (defmacro with-current-notmuch-show-message (&rest body)
   "Evaluate body with current buffer set to the text of current message"
@@ -1587,9 +1600,67 @@ we are not marking them."
   (notmuch-show-mark-all-seen-read not-mark)
   (notmuch-kill-this-buffer))
 
+(defun notmuch-show-update-seen (top-or-bottom)
+  "Update seen status of current message
+
+Mark that we have seen the TOP-OR-BOTTOM of current message."
+  (let* ((current (notmuch-show-get-prop :seen-local))
+	 new)
+    (unless (eq current 'both)
+      (if (eq top-or-bottom 'top)
+	  (if (eq current 'bottom)
+	      (setq new 'both)
+	    (setq new 'top))
+	(if (eq current 'top)
+	    (setq new 'both)
+	  (setq new 'bottom)))
+      (unless (eq current new)
+	(notmuch-show-set-prop :seen-local new))
+      (when (eq new 'both)
+	(notmuch-show-mark-seen)
+	(message "marking seen %s" (current-time))))))
+
+(defun notmuch-show-do-message-seen (start end)
+  "Update seen status for the current message.
+
+A message is seen if both the top and enough of the rest of the
+message have been visible in the buffer.  Enough means either the
+bottom of the message or a point in the message more than
+LINES-NEEDED lines into the message. LINES-NEEDED is
+`notmuch-show-seen-lines-needed` if that is an integer and that
+times the current window height if it is a float."
+  (let* ((lines-needed (if (integerp notmuch-show-seen-lines-needed)
+			   notmuch-show-seen-lines-needed
+			 (truncate (* notmuch-show-seen-lines-needed (window-body-height)))))
+	 (top (notmuch-show-message-top))
+	 (bottom (notmuch-show-message-bottom)))
+    (when (notmuch-show-message-visible-p)
+      (when (>= top start)
+	(notmuch-show-update-seen 'top))
+      (when (or (<= bottom end)
+		(> (count-screen-lines top end) lines-needed))
+	(notmuch-show-update-seen 'bottom)))))
+
+
 (defun notmuch-show-do-seen (start end)
-  "Update seen status for all messages between start and end."
-  )
+  "Update seen status for all messages between start and end.
+
+A message is seen if both the top and enough of the rest of the
+message have been visible in the buffer. See
+`notmuch-show-do-message-seen` for the definition of enough. Seen
+is a buffer local property. The unread status is removed from all
+seen messages when the user quits the show buffer."
+  (save-excursion
+    (goto-char start)
+    (while (and (or (notmuch-show-do-message-seen start end) t)
+		(< (notmuch-show-message-bottom) end)
+		(notmuch-show-goto-message-next)))
+
+    ;; This is a work around because emacs gives weird answers for
+    ;; window-end if the buffer ends with invisible text.
+    (when (and (pos-visible-in-window-p (point-max))
+	       (notmuch-show-message-visible-p))
+      (notmuch-show-update-seen 'bottom))))
 
 (defun notmuch-show-command-hook ()
   (when (eq major-mode 'notmuch-show-mode)
-- 
1.7.9.1

Re: [PATCH WIP v2 0/5] emacs: show: redesign unread/read logic

[Jani Nikula]

On Sun, 01 Dec 2013, Mark Walters <markwalters1009@gmail.com> wrote:
> This is further wip of the message at
> id:1385285551-5158-1-git-send-email-markwalters1009@gmail.com
> see there for some discussion of the design.
>
> This series is still definitely wip: one reason for posting is so
> people can play with different strategies for marking read
> easily. (As WIP tree's unread handling is broken and the tests need updating.)
>
> The series consists of three parts: the first 4 patches add the notion
> of seen: this means the user has seen the message but the message has
> typically not been marked read yet. The seen messages are marked read
> when the user quits the show buffer unless the user quits with
> prefix-arg quit. In all cases an informative message is shown.
>
> The fifth patch adds a psot-command-hook stub for updating the seen
> status. This seems a natural place to do the update as it means
> however the user navugates around the buffer (eg next-message or
> page-down etc) the update gets done.
>
> This is intended to be an easy place for other people to try out their
> own mark read strategies.
>
> The final patch implements something pretty close to what I would like
> for marking seen/read. A message is deemed seen provided the user has
> seen the top of the message, and has seen either the bottom of the
> message or a point at least some customisable number of lines into the
> message. The customisable number of lines can either be a fixed number
> e.g. 20, or a number depending on the height of the current window
> e.g. the default is 3/4 of the window height.
>
> The idea is a message seen if the user has seen the entire message, or
> enough of it they have to have noticed it. The figure of 3/4 also
> means that the notmuch commands like next-message which place the top
> of the message at the top of the window automatically mark the message
> seen as either the whole message or at least one window full must be
> visible.
>
> I would be very grateful for any comments on whether this behaves as
> people would expect, what they would want instead etc

Hi Mark, thanks for working on this.

I had tons of mail reading to catch up, so this was a good opportunity
to try the patches. I'll try to be objective and constructive next, but
up front, just so there's no doubt: I don't like it.

I think my issues boil down to the series containing two pretty
significant changes at once: how to decide if a message was read and
when to apply the tag changes to reflect that.

I found it confusing that messages were not being tagged -unread while I
was viewing the thread. I found it even more confusing to get a message
"Marked N messages read" on quitting show view with no feedback on
*which* messages were read, and often the N didn't feel right
either. And I think that's the problem: I wanted to see the new
heuristics on deciding whether a message was read in action, but I got
zero immediate feedback on it!

My suggestion is to drop the delay in tag changes for now, and focus on
the part that decides whether a message was read or not. Do the tag
changes immediately when you consider a message "seen". I think this way
we get a better feel of how well the heuristics really work, and we can
make it just right. I think that's the bug in we currently have in
notmuch, and delaying the tag changes doesn't contribute to fixing
it. Indeed I think the delay makes it *harder* to fix.

Afterwards, we could add the delayed tag changes (although hopefully as
an option) if desired. And keeping that in mind, AFAICT you wouldn't
need to rework your patches all that much.

How does that sound?


BR,
Jani.