[Patch v3 0/3] emacs: show: redesign unread/read logic

[Patch v3 0/3] emacs: show: redesign unread/read logic

[Mark Walters]

This is the latest version of the redesign of the unread/read
logic. Now that the tag update code is in this is relatively simple.

The previous version (which included an early version of the tag
update code) is at
id:1387065197-15776-1-git-send-email-markwalters1009@gmail.com

The series consists of three patches. The first makes the test suite
run the post-command-hooks. This is bremner's version from
id:1390578744-30516-1-git-send-email-david@tethera.net (which is much
nicer than my original version). This patch has been reviewed by Tomi
in id:m21tzxns8p.fsf@guru.guru-group.fi and me in
id:87d2jizjlu.fsf@qmul.ac.uk

The second patch adds a stub function linked to the post-command-hook
to mark messages read. This is a convenient place to test other
possible logics.

The third patch adds my attempt at a plausible logic. I find it works
very well: it usually does both what I expect and what I want.

Somewhat surprisingly for a moderately significant change all tests
pass unchanged.

Finally, the tests do not pass after Patch 2 but before Patch 3. I
have left these two patches split to make it easy for other people to
test other unread logic, but we might want to merge the two when
commiting.

Best wishes

Mark







Mark Walters (3):
  test: make test_emacs call post-command-hook
  emacs: show: add an update seen function to post-command-hook
  emacs: show: make `seen' mean user viewed whole message

 emacs/notmuch-show.el |   91 +++++++++++++++++++++++++++++++++++++++++--------
 test/test-lib.el      |   11 ++++++
 test/test-lib.sh      |    2 +-
 3 files changed, 88 insertions(+), 16 deletions(-)

-- 
1.7.10.4

[Patch v3 1/3] test: make test_emacs call post-command-hook

[Mark Walters]

From: David Bremner <david@tethera.net>

The unread/read changes will use the post-command-hook. test_emacs
does not call the post-command-hook. This adds a notmuch-test-progn
which takes a list of commands as argument and executes them in turn
but runs the post-command-hook after each one.

The caller can batch operations (ie to stop post-command-hook from
being interleaved) by wrapping the batch of operations inside a progn.

We also explicitly run the post-command-hook before getting the output
from a test; this makes sense as this will be a place the user would
be seeing the information.
---
 test/test-lib.el |   11 +++++++++++
 test/test-lib.sh |    2 +-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/test/test-lib.el b/test/test-lib.el
index 437f83f..36afe63 100644
--- a/test/test-lib.el
+++ b/test/test-lib.el
@@ -52,11 +52,13 @@ (defun notmuch-test-wait ()
 
 (defun test-output (&optional filename)
   "Save current buffer to file FILENAME.  Default FILENAME is OUTPUT."
+  (notmuch-post-command)
   (write-region (point-min) (point-max) (or filename "OUTPUT")))
 
 (defun test-visible-output (&optional filename)
   "Save visible text in current buffer to file FILENAME.  Default
 FILENAME is OUTPUT."
+  (notmuch-post-command)
   (let ((text (visible-buffer-string)))
     (with-temp-file (or filename "OUTPUT") (insert text))))
 
@@ -166,6 +168,15 @@ (defun notmuch-test-expect-equal (output expected)
      (t
       (notmuch-test-report-unexpected output expected)))))
 
+(defun notmuch-post-command ()
+  (run-hooks 'post-command-hook))
+
+(defmacro notmuch-test-progn (&rest body)
+  (cons 'progn
+	(mapcar
+	 (lambda (x) `(prog1 ,x (notmuch-post-command)))
+	 body)))
+
 ;; For historical reasons, we hide deleted tags by default in the test
 ;; suite
 (setq notmuch-tag-deleted-formats
diff --git a/test/test-lib.sh b/test/test-lib.sh
index 8697d6a..e6403e5 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -1139,7 +1139,7 @@ test_emacs () {
 	rm -f OUTPUT
 	touch OUTPUT
 
-	${TEST_EMACSCLIENT} --socket-name="$EMACS_SERVER" --eval "(progn $@)"
+	${TEST_EMACSCLIENT} --socket-name="$EMACS_SERVER" --eval "(notmuch-test-progn $@)"
 }
 
 test_python() {
-- 
1.7.10.4

[Patch v3 2/3] 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 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index f6ca827..1a7de85 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1142,6 +1142,8 @@ (defun notmuch-show-build-buffer ()
   (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)
 
@@ -1530,6 +1532,16 @@ (defun notmuch-show-mark-read (&optional unread)
     (apply 'notmuch-show-tag-message
 	   (notmuch-tag-change-list notmuch-show-mark-read-tags unread))))
 
+(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.
 
@@ -1665,9 +1677,7 @@ (defun notmuch-show-next-message (&optional pop-at-end)
 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-message-adjust))
+      (notmuch-show-message-adjust)
     (if pop-at-end
 	(notmuch-show-next-thread)
       (goto-char (point-max)))))
@@ -1678,7 +1688,6 @@ (defun notmuch-show-previous-message ()
   (if (= (point) (notmuch-show-message-top))
       (notmuch-show-goto-message-previous)
     (notmuch-show-move-to-message-top))
-  (notmuch-show-mark-read)
   (notmuch-show-message-adjust))
 
 (defun notmuch-show-next-open-message (&optional pop-at-end)
@@ -1693,9 +1702,7 @@ (defun notmuch-show-next-open-message (&optional pop-at-end)
     (while (and (setq r (notmuch-show-goto-message-next))
 		(not (notmuch-show-message-visible-p))))
     (if r
-	(progn
-	  (notmuch-show-mark-read)
-	  (notmuch-show-message-adjust))
+	(notmuch-show-message-adjust)
       (if pop-at-end
 	  (notmuch-show-next-thread)
 	(goto-char (point-max))))
@@ -1708,9 +1715,7 @@ (defun notmuch-show-next-matching-message ()
     (while (and (setq r (notmuch-show-goto-message-next))
 		(not (notmuch-show-get-prop :match))))
     (if r
-	(progn
-	  (notmuch-show-mark-read)
-	  (notmuch-show-message-adjust))
+	(notmuch-show-message-adjust)
       (goto-char (point-max)))))
 
 (defun notmuch-show-open-if-matched ()
@@ -1721,8 +1726,7 @@ (defun notmuch-show-open-if-matched ()
 (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-read)
+  (unless (notmuch-show-message-visible-p)
     (notmuch-show-next-open-message))
   (when (eobp)
     ;; There are no matched non-excluded messages so open all matched
@@ -1730,8 +1734,7 @@ (defun notmuch-show-goto-first-wanted-message ()
     (notmuch-show-mapc 'notmuch-show-open-if-matched)
     (force-window-update)
     (goto-char (point-min))
-    (if (notmuch-show-message-visible-p)
-	(notmuch-show-mark-read)
+    (unless (notmuch-show-message-visible-p)
       (notmuch-show-next-open-message))))
 
 (defun notmuch-show-previous-open-message ()
@@ -1741,7 +1744,6 @@ (defun notmuch-show-previous-open-message ()
 		  (notmuch-show-goto-message-previous)
 		(notmuch-show-move-to-message-top))
 	      (not (notmuch-show-message-visible-p))))
-  (notmuch-show-mark-read)
   (notmuch-show-message-adjust))
 
 (defun notmuch-show-view-raw-message ()
-- 
1.7.10.4

[Patch v3 3/3] 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 introduced in
the previous patch.
---
 emacs/notmuch-show.el |   63 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 61 insertions(+), 2 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 1a7de85..1abf24b 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -211,6 +211,20 @@ (defcustom notmuch-show-mark-read-tags '("-unread")
   :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.
+
+Enough means either the bottom of the message or a point in the
+message more than LINES-NEEDED lines into the message, where
+LINES-NEEDED is this variable if this variable is an integer and
+this variable times the current window height if it is a float."
+  :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"
@@ -1532,9 +1546,54 @@ (defun notmuch-show-mark-read (&optional unread)
     (apply 'notmuch-show-tag-message
 	   (notmuch-tag-change-list notmuch-show-mark-read-tags unread))))
 
+(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)))
+    (unless (or (eq current 'both) (eq current top-or-bottom))
+      (if (not current)
+	  (notmuch-show-set-prop :seen top-or-bottom)
+	(notmuch-show-set-prop :seen 'both)
+	(notmuch-show-mark-read)))))
+
+(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.  See the
+`notmuch-show-seen-lines-needed' for the definition of `enough'."
+  (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.
+
+We mark the top (bottom) of a message seen if the top (enough of
+the rest of the message) respectively have been visible in the
+buffer. See the `notmuch-show-seen-lines-needed' for the
+definition of `enough'. When both the top and bottom have been
+seen we mark the message read."
+  (save-excursion
+    (goto-char start)
+    (notmuch-show-do-message-seen start end)
+    (while (and (< (notmuch-show-message-bottom) end)
+		(notmuch-show-goto-message-next))
+      (notmuch-show-do-message-seen start end))
+    ;; 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.10.4

Re: [Patch v3 0/3] emacs: show: redesign unread/read logic

[David Edmondson]

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

On Tue, Mar 25 2014, Mark Walters wrote:
> The third patch adds my attempt at a plausible logic. I find it works
> very well: it usually does both what I expect and what I want.

Whilst I think that the patch is well done, I don't like the resulting
behaviour. That is a personal preference, of course. At the moment it
doesn't seem that there is a way to accept this patch and (optionally)
retain the existing behaviour. Do you have any thoughts on how we might
achieve that?

I don't have a strong preference for the default behaviour, though I
suspect that something closer to the current behaviour (where a message
is marked seen when it becomes the current message[1]) would better
match user expectation (but this is an opinion, not something founded in
fact).

Footnotes: 
[1]  Modulo bugs.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 310 bytes --]

Re: [Patch v3 0/3] emacs: show: redesign unread/read logic

[Mark Walters]

On Wed, 07 May 2014, David Edmondson <dme@dme.org> wrote:
> On Tue, Mar 25 2014, Mark Walters wrote:
>> The third patch adds my attempt at a plausible logic. I find it works
>> very well: it usually does both what I expect and what I want.
>
> Whilst I think that the patch is well done, I don't like the resulting
> behaviour. That is a personal preference, of course. At the moment it
> doesn't seem that there is a way to accept this patch and (optionally)
> retain the existing behaviour. Do you have any thoughts on how we might
> achieve that?

Thanks for looking and for the feedback. I should emphasise to everyone
that I would like all feedback whether positive or negative!

> I don't have a strong preference for the default behaviour, though I
> suspect that something closer to the current behaviour (where a message
> is marked seen when it becomes the current message[1]) would better
> match user expectation (but this is an opinion, not something founded in
> fact).

Just for the record I will detail what happens currently and then I have
a couple questions about your suggestion.

A message is marked read if:

1) if you navigate to a message using n/p (next/prev open message) 

2) if you navigate to it using N/P (next/prev message) regardless of
whether the message is open or closed.

3) if you go to it using n.s.next-matching-message (not bound by
default) whether message is open or closed.

4) when you enter a buffer and notmuch goes to the first open message.

but not marked read in cases like:

1) opening a message

2) viewing or entering a message using other notmuch navigation such as
notmuch-show-advance and friends (bound to space)

3) viewing or entering a message using arrow keys, page-up page-down,
ctrl-v mouse clicks etc

Personally, I think marking a closed message read is a bug, and not
marking it read when opening it is too (at least in many cases).  The
other problem with the current approach (in my view) is that if you try
to use the navigation commands non-interactively then messages end up
being marked read, even if they are never displayed to the user. Linking
into the post-command-hook means that this should "just work".

Questions:  What does it mean for a message to be the current message?
Is it just point being in the message? Would you be happy with a message
being marked read when point entered the message? That could be done
from the post-command-hook infrastructure to fix some of the problems
mentioned above.

I think that is likely that people will disagree on how they want this
to work so I would like to try and make it customisable so I would
definitely be interested to see if I can get the behaviour you would
like from this infrastructure (or something similar).

Best wishes

Mark

Re: [Patch v3 0/3] emacs: show: redesign unread/read logic

[David Edmondson]

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

On Wed, May 07 2014, Mark Walters wrote:
> A message is marked read if:
>
> 1) if you navigate to a message using n/p (next/prev open message) 
>
> 2) if you navigate to it using N/P (next/prev message) regardless of
> whether the message is open or closed.
>
> 3) if you go to it using n.s.next-matching-message (not bound by
> default) whether message is open or closed.
>
> 4) when you enter a buffer and notmuch goes to the first open message.
>
> but not marked read in cases like:
>
> 1) opening a message
>
> 2) viewing or entering a message using other notmuch navigation such as
> notmuch-show-advance and friends (bound to space)

My experience is that this removes the 'unread' tag.

> 3) viewing or entering a message using arrow keys, page-up page-down,
> ctrl-v mouse clicks etc
>
> Personally, I think marking a closed message read is a bug,

Agreed.

> and not marking it read when opening it is too

Agreed.

> (at least in many cases).

I would be happy with just these fixed (i.e. the current behaviour with
those two bug fixes). My typical use is to move around a thread using
Space, Backspace, n, p, N and P with RET, M-RET and C-u M-RET to
manipulate open/closed state (i.e. not the normal emacs movement
commands to move).

> The other problem with the current approach (in my view) is that if
> you try to use the navigation commands non-interactively then messages
> end up being marked read, even if they are never displayed to the
> user.

In what cases does this happen? (Not arguing, just not fully
understanding.)

> Linking into the post-command-hook means that this should "just work".
>
> Questions:  What does it mean for a message to be the current message?
> Is it just point being in the message?

This makes sense to me, other than perhaps "point being in an _open_
message". I don't want moving point through a closed message with C-n to
remove the 'unread' tag.

> Would you be happy with a message being marked read when point entered
> the message? That could be done from the post-command-hook
> infrastructure to fix some of the problems mentioned above.
>
> I think that is likely that people will disagree on how they want this
> to work so I would like to try and make it customisable so I would
> definitely be interested to see if I can get the behaviour you would
> like from this infrastructure (or something similar).
>
> Best wishes
>
> Mark

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 310 bytes --]

Re: [Patch v3 0/3] emacs: show: redesign unread/read logic

[Mark Walters]

On Thu, 08 May 2014, David Edmondson <dme@dme.org> wrote:
> On Wed, May 07 2014, Mark Walters wrote:
>> A message is marked read if:
>>
>> 1) if you navigate to a message using n/p (next/prev open message) 
>>
>> 2) if you navigate to it using N/P (next/prev message) regardless of
>> whether the message is open or closed.
>>
>> 3) if you go to it using n.s.next-matching-message (not bound by
>> default) whether message is open or closed.
>>
>> 4) when you enter a buffer and notmuch goes to the first open message.
>>
>> but not marked read in cases like:
>>
>> 1) opening a message
>>
>> 2) viewing or entering a message using other notmuch navigation such as
>> notmuch-show-advance and friends (bound to space)
>
> My experience is that this removes the 'unread' tag.

Sorry I was wrong here. This one does mark it read.

>> 3) viewing or entering a message using arrow keys, page-up page-down,
>> ctrl-v mouse clicks etc

This is perhaps a key one: should the above mark it read.

>> Personally, I think marking a closed message read is a bug,
>
> Agreed.
>
>> and not marking it read when opening it is too
>
> Agreed.
>
>> (at least in many cases).
>
> I would be happy with just these fixed (i.e. the current behaviour with
> those two bug fixes). My typical use is to move around a thread using
> Space, Backspace, n, p, N and P with RET, M-RET and C-u M-RET to
> manipulate open/closed state (i.e. not the normal emacs movement
> commands to move).
>
>> The other problem with the current approach (in my view) is that if
>> you try to use the navigation commands non-interactively then messages
>> end up being marked read, even if they are never displayed to the
>> user.
>
> In what cases does this happen? (Not arguing, just not fully
> understanding.)

I had a series for implementing filtering (ie limiting the open
messages) in show where it was the main cause of my stopping. See
id:20120429005736.GK2704@mit.edu for details of the problems.

>> Linking into the post-command-hook means that this should "just work".
>>
>> Questions:  What does it mean for a message to be the current message?
>> Is it just point being in the message?
>
> This makes sense to me, other than perhaps "point being in an _open_
> message". I don't want moving point through a closed message with C-n to
> remove the 'unread' tag.

Oh yes I completely agree with that.

Best wishes

Mark

Re: [Patch v3 0/3] emacs: show: redesign unread/read logic

[David Edmondson]

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

On Thu, May 08 2014, Mark Walters wrote:
>>> 3) viewing or entering a message using arrow keys, page-up page-down,
>>> ctrl-v mouse clicks etc
>
> This is perhaps a key one: should the above mark it read.

I would be happy to have that be the case.

>>> The other problem with the current approach (in my view) is that if
>>> you try to use the navigation commands non-interactively then messages
>>> end up being marked read, even if they are never displayed to the
>>> user.
>>
>> In what cases does this happen? (Not arguing, just not fully
>> understanding.)
>
> I had a series for implementing filtering (ie limiting the open
> messages) in show where it was the main cause of my stopping. See
> id:20120429005736.GK2704@mit.edu for details of the problems.

Understood. I agree that would be broken.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 310 bytes --]