[RFC][PATCH] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[RFC][PATCH] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[David Edmondson]

The advance/rewind functions had become complex, which made it hard to
determine who they are expected to behave. Re-implement them simply in
order to poll user-experience and expectation.
---

This is intended to be for discussion!

The current rewind implementation didn't behave as I expected, so I
re-wrote it. The current advance implementation doesn't work if an
open message within a thread ends with hidden text (e.g. a signature)
(I suspect that relates do the end of the message and the signature
having different invisible properties, but didn't check that) and was
complicated, so I wrote a simpler version.

My aim was not to replicate the current behaviour perfectly, rather to
produce something that did what seemed reasonable. Your mileage may
vary.

This doesn't currently pass the test suite. We might decide that's a
bug in the suite, of course.

 emacs/notmuch-show.el |  104 +++++++++++++++++++++----------------------------
 1 files changed, 45 insertions(+), 59 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 46525aa..ab1e89f 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1156,38 +1156,29 @@ Some useful entries are:
 ;; Commands typically bound to keys.
 
 (defun notmuch-show-advance ()
-  "Advance through thread.
+  "Advance through the current thread.
 
-If the current message in the thread is not yet fully visible,
-scroll by a near screenful to read more of the message.
+Scroll the current message if the end of it is not visible,
+otherwise move to the next message.
 
-Otherwise, (the end of the current message is already within the
-current window), advance to the next open message."
+Return `t' if we are at the end of the last message, otherwise
+`nil'."
   (interactive)
-  (let* ((end-of-this-message (notmuch-show-message-bottom))
-	 (visible-end-of-this-message (1- end-of-this-message))
-	 (ret nil))
-    (while (invisible-p visible-end-of-this-message)
-      (setq visible-end-of-this-message
-	    (previous-single-char-property-change visible-end-of-this-message
-						  'invisible)))
-    (cond
-     ;; Ideally we would test `end-of-this-message' against the result
-     ;; of `window-end', but that doesn't account for the fact that
-     ;; the end of the message might be hidden.
-     ((and visible-end-of-this-message
-	   (> visible-end-of-this-message (window-end)))
-      ;; The bottom of this message is not visible - scroll.
-      (scroll-up nil))
-
-     ((not (= end-of-this-message (point-max)))
-      ;; This is not the last message - move to the next visible one.
-      (notmuch-show-next-open-message))
-
-     (t
-      ;; This is the last message - change the return value
-      (setq ret t)))
-    ret))
+  (cond
+   ((eobp)
+    ;; If we are at the end of the buffer then move to the next
+    ;; thread.
+    t)
+
+   ((> (notmuch-show-message-bottom) (window-end))
+    ;; The end of this message is not visible - scroll.
+    (scroll-up)
+    nil)
+
+   (t
+    ;; Show the start of the next message.
+    (notmuch-show-next-open-message)
+    nil)))
 
 (defun notmuch-show-advance-and-archive ()
   "Advance through thread and archive.
@@ -1201,44 +1192,39 @@ from each message), kills the buffer, and displays the next
 thread from the search from which this thread was originally
 shown."
   (interactive)
-  (if (notmuch-show-advance)
-      (notmuch-show-archive-thread)))
+  (when (notmuch-show-advance)
+    (notmuch-show-archive-thread)))
 
 (defun notmuch-show-rewind ()
-  "Backup through the thread, (reverse scrolling compared to \\[notmuch-show-advance-and-archive]).
+  "Move backwards through a thread, the counterpart 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
-just like `notmuch-show-previous-message'.
-
-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-and-archive'."
   (interactive)
-  (let ((start-of-message (notmuch-show-message-top))
-	(start-of-window (window-start)))
+  (let ((start-of-message (notmuch-show-message-top)))
     (cond
-      ;; Either this message is properly aligned with the start of the
-      ;; window or the start of this message is not visible on the
-      ;; screen - scroll.
-     ((or (= start-of-message start-of-window)
-	  (< start-of-message start-of-window))
+     ((= start-of-message (point))
+      ;; If the cursor is at the start of the current message, move to
+      ;; the previous open message.
+      (notmuch-show-previous-open-message))
+
+     ((< start-of-message (window-start))
+      ;; If the start of the current message is not visible, scroll
+      ;; down.
       (scroll-down)
-      ;; If a small number of lines from the previous message are
-      ;; visible, realign so that the top of the current message is at
-      ;; the top of the screen.
-      (if (<= (count-screen-lines (window-start) start-of-message)
-	      next-screen-context-lines)
-	  (progn
-	    (goto-char (notmuch-show-message-top))
-	    (notmuch-show-message-adjust)))
-      ;; Move to the top left of the window.
-      (goto-char (window-start)))
+      ;; If the start of the current message became visible, align it
+      ;; with the top of the window.
+      (when (> start-of-message (window-start))
+	(goto-char start-of-message)
+	(notmuch-show-message-adjust)))
+
+     ((> start-of-message (window-start))
+      ;; If the cursor is not at the start of the current (visible)
+      ;; message, move it there, but do not adjust or scroll the
+      ;; display.
+      (goto-char start-of-message))
+
      (t
       ;; Move to the previous message.
-      (notmuch-show-previous-message)))))
+      (notmuch-show-previous-open-message)))))
 
 (defun notmuch-show-reply (&optional prompt-for-sender)
   "Reply to the current message."
-- 
1.7.7.3

[RFC][PATCH v2] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[David Edmondson]

The advance/rewind functions had become complex, which made it hard to
determine who they are expected to behave. Re-implement them simply in
order to poll user-experience and expectation.
---

Re-introduce the detection of invisible trailers. Using
`previous-single-char-property-change' just didn't work in my testing,
so back to the original approach. Dmitry: can you explain why you
needed to change it?

 emacs/notmuch-show.el |  126 ++++++++++++++++++++++++++-----------------------
 1 files changed, 67 insertions(+), 59 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 46525aa..258814e 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1156,38 +1156,51 @@ Some useful entries are:
 ;; Commands typically bound to keys.
 
 (defun notmuch-show-advance ()
-  "Advance through thread.
+  "Advance through the current thread.
 
-If the current message in the thread is not yet fully visible,
-scroll by a near screenful to read more of the message.
+Scroll the current message if the end of it is not visible,
+otherwise move to the next message.
 
-Otherwise, (the end of the current message is already within the
-current window), advance to the next open message."
+Return `t' if we are at the end of the last message, otherwise
+`nil'."
   (interactive)
-  (let* ((end-of-this-message (notmuch-show-message-bottom))
-	 (visible-end-of-this-message (1- end-of-this-message))
-	 (ret nil))
-    (while (invisible-p visible-end-of-this-message)
-      (setq visible-end-of-this-message
-	    (previous-single-char-property-change visible-end-of-this-message
-						  'invisible)))
-    (cond
-     ;; Ideally we would test `end-of-this-message' against the result
-     ;; of `window-end', but that doesn't account for the fact that
-     ;; the end of the message might be hidden.
-     ((and visible-end-of-this-message
-	   (> visible-end-of-this-message (window-end)))
-      ;; The bottom of this message is not visible - scroll.
-      (scroll-up nil))
-
-     ((not (= end-of-this-message (point-max)))
-      ;; This is not the last message - move to the next visible one.
-      (notmuch-show-next-open-message))
-
-     (t
-      ;; This is the last message - change the return value
-      (setq ret t)))
-    ret))
+  (cond
+   ((eobp)
+    ;; If we are at the end of the buffer then move to the next
+    ;; thread.
+    t)
+
+   ;; Ideally we would simply do:
+   ;; 
+   ;; 	((> (notmuch-show-message-bottom) (window-end))
+   ;; 
+   ;; here, but that fails if the trailing text in the buffer is
+   ;; invisible (`window-end' returns the last visible character,
+   ;; which can then be smaller than `notmuch-show-message-bottom').
+   ;;
+   ;; So we need to find the last _visible_ character of the
+   ;; message. We do this by checking the invisibility of the
+   ;; characters from `notmuch-show-message-bottom' towards the start
+   ;; of the message. When we find a non-invisible character, we test
+   ;; to see whether it is visible in the window (i.e. less than
+   ;; `window-end').
+
+   ((let* ((visible-bottom (notmuch-show-message-bottom))
+	   (visible-bottom 
+	    (save-excursion
+	      (goto-char visible-bottom)
+	      (while (invisible-p (point))
+		(backward-char))
+	      (point))))
+      (> visible-bottom (window-end)))
+    ;; The end of this message is not visible - scroll.
+    (scroll-up)
+    nil)
+
+   (t
+    ;; Show the start of the next message.
+    (notmuch-show-next-open-message)
+    nil)))
 
 (defun notmuch-show-advance-and-archive ()
   "Advance through thread and archive.
@@ -1201,44 +1214,39 @@ from each message), kills the buffer, and displays the next
 thread from the search from which this thread was originally
 shown."
   (interactive)
-  (if (notmuch-show-advance)
-      (notmuch-show-archive-thread)))
+  (when (notmuch-show-advance)
+    (notmuch-show-archive-thread)))
 
 (defun notmuch-show-rewind ()
-  "Backup through the thread, (reverse scrolling compared to \\[notmuch-show-advance-and-archive]).
+  "Move backwards through a thread, the counterpart 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
-just like `notmuch-show-previous-message'.
-
-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-and-archive'."
   (interactive)
-  (let ((start-of-message (notmuch-show-message-top))
-	(start-of-window (window-start)))
+  (let ((start-of-message (notmuch-show-message-top)))
     (cond
-      ;; Either this message is properly aligned with the start of the
-      ;; window or the start of this message is not visible on the
-      ;; screen - scroll.
-     ((or (= start-of-message start-of-window)
-	  (< start-of-message start-of-window))
+     ((= start-of-message (point))
+      ;; If the cursor is at the start of the current message, move to
+      ;; the previous open message.
+      (notmuch-show-previous-open-message))
+
+     ((< start-of-message (window-start))
+      ;; If the start of the current message is not visible, scroll
+      ;; down.
       (scroll-down)
-      ;; If a small number of lines from the previous message are
-      ;; visible, realign so that the top of the current message is at
-      ;; the top of the screen.
-      (if (<= (count-screen-lines (window-start) start-of-message)
-	      next-screen-context-lines)
-	  (progn
-	    (goto-char (notmuch-show-message-top))
-	    (notmuch-show-message-adjust)))
-      ;; Move to the top left of the window.
-      (goto-char (window-start)))
+      ;; If the start of the current message became visible, align it
+      ;; with the top of the window.
+      (when (> start-of-message (window-start))
+	(goto-char start-of-message)
+	(notmuch-show-message-adjust)))
+
+     ((> start-of-message (window-start))
+      ;; If the cursor is not at the start of the current (visible)
+      ;; message, move it there, but do not adjust or scroll the
+      ;; display.
+      (goto-char start-of-message))
+
      (t
       ;; Move to the previous message.
-      (notmuch-show-previous-message)))))
+      (notmuch-show-previous-open-message)))))
 
 (defun notmuch-show-reply (&optional prompt-for-sender)
   "Reply to the current message."
-- 
1.7.7.3

[RFC][PATCH v3] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[David Edmondson]

The advance/rewind functions had become complex, which made it hard to
determine who they are expected to behave. Re-implement them simply in
order to poll user-experience and expectation.
---

This one passes the test suite and, consequently, works better when
the last open message in a thread has trailing invisible text.

 emacs/notmuch-show.el |  124 +++++++++++++++++++++++++-----------------------
 1 files changed, 65 insertions(+), 59 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 46525aa..9fec499 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1156,38 +1156,49 @@ Some useful entries are:
 ;; Commands typically bound to keys.
 
 (defun notmuch-show-advance ()
-  "Advance through thread.
+  "Advance through the current thread.
 
-If the current message in the thread is not yet fully visible,
-scroll by a near screenful to read more of the message.
+Scroll the current message if the end of it is not visible,
+otherwise move to the next message.
 
-Otherwise, (the end of the current message is already within the
-current window), advance to the next open message."
+Return `t' if we are at the end of the last message, otherwise
+`nil'."
   (interactive)
-  (let* ((end-of-this-message (notmuch-show-message-bottom))
-	 (visible-end-of-this-message (1- end-of-this-message))
-	 (ret nil))
-    (while (invisible-p visible-end-of-this-message)
-      (setq visible-end-of-this-message
-	    (previous-single-char-property-change visible-end-of-this-message
-						  'invisible)))
-    (cond
-     ;; Ideally we would test `end-of-this-message' against the result
-     ;; of `window-end', but that doesn't account for the fact that
-     ;; the end of the message might be hidden.
-     ((and visible-end-of-this-message
-	   (> visible-end-of-this-message (window-end)))
-      ;; The bottom of this message is not visible - scroll.
-      (scroll-up nil))
-
-     ((not (= end-of-this-message (point-max)))
-      ;; This is not the last message - move to the next visible one.
-      (notmuch-show-next-open-message))
-
-     (t
-      ;; This is the last message - change the return value
-      (setq ret t)))
-    ret))
+  (cond
+   ((eobp)
+    ;; If we are at the end of the buffer then move to the next
+    ;; thread.
+    t)
+
+   ;; Ideally we would simply do:
+   ;; 
+   ;; 	((> (notmuch-show-message-bottom) (window-end))
+   ;; 
+   ;; here, but that fails if the trailing text in the buffer is
+   ;; invisible (`window-end' returns the last visible character,
+   ;; which can then be smaller than `notmuch-show-message-bottom').
+   ;;
+   ;; So we need to find the last _visible_ character of the
+   ;; message. We do this by checking the invisibility of the
+   ;; characters from `notmuch-show-message-bottom'-1 towards the start
+   ;; of the message. When we find a non-invisible character, we test
+   ;; to see whether it is visible in the window.
+
+   ((let ((visible-bottom 
+	   (save-excursion
+	     (goto-char (1- (notmuch-show-message-bottom)))
+	     (while (invisible-p (point))
+	       (backward-char))
+	     (point))))
+      (> visible-bottom (window-end)))
+    ;; The end of this message is not visible - scroll.
+    (scroll-up)
+    nil)
+
+   (t
+    ;; Show the start of the next message.
+    (notmuch-show-next-open-message)
+    nil)))
 
 (defun notmuch-show-advance-and-archive ()
   "Advance through thread and archive.
@@ -1201,44 +1212,39 @@ from each message), kills the buffer, and displays the next
 thread from the search from which this thread was originally
 shown."
   (interactive)
-  (if (notmuch-show-advance)
-      (notmuch-show-archive-thread)))
+  (when (notmuch-show-advance)
+    (notmuch-show-archive-thread)))
 
 (defun notmuch-show-rewind ()
-  "Backup through the thread, (reverse scrolling compared to \\[notmuch-show-advance-and-archive]).
+  "Move backwards through a thread, the counterpart 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
-just like `notmuch-show-previous-message'.
-
-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-and-archive'."
   (interactive)
-  (let ((start-of-message (notmuch-show-message-top))
-	(start-of-window (window-start)))
+  (let ((start-of-message (notmuch-show-message-top)))
     (cond
-      ;; Either this message is properly aligned with the start of the
-      ;; window or the start of this message is not visible on the
-      ;; screen - scroll.
-     ((or (= start-of-message start-of-window)
-	  (< start-of-message start-of-window))
+     ((= start-of-message (point))
+      ;; If the cursor is at the start of the current message, move to
+      ;; the previous open message.
+      (notmuch-show-previous-open-message))
+
+     ((< start-of-message (window-start))
+      ;; If the start of the current message is not visible, scroll
+      ;; down.
       (scroll-down)
-      ;; If a small number of lines from the previous message are
-      ;; visible, realign so that the top of the current message is at
-      ;; the top of the screen.
-      (if (<= (count-screen-lines (window-start) start-of-message)
-	      next-screen-context-lines)
-	  (progn
-	    (goto-char (notmuch-show-message-top))
-	    (notmuch-show-message-adjust)))
-      ;; Move to the top left of the window.
-      (goto-char (window-start)))
+      ;; If the start of the current message became visible, align it
+      ;; with the top of the window.
+      (when (> start-of-message (window-start))
+	(goto-char start-of-message)
+	(notmuch-show-message-adjust)))
+
+     ((> start-of-message (window-start))
+      ;; If the cursor is not at the start of the current (visible)
+      ;; message, move it there, but do not adjust or scroll the
+      ;; display.
+      (goto-char start-of-message))
+
      (t
       ;; Move to the previous message.
-      (notmuch-show-previous-message)))))
+      (notmuch-show-previous-open-message)))))
 
 (defun notmuch-show-reply (&optional prompt-for-sender)
   "Reply to the current message."
-- 
1.7.7.3

Re: [RFC][PATCH v2] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[Aaron Ecay]

David,

Would the problem you had with previous-s-c-prop-change be fixed by the
patch to the original function I sent in the thread starting at
id:"m2y5u5cykp.fsf@kcals.intra.maillard.im" ?

-- 
Aaron Ecay

Re: [RFC][PATCH v2] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[David Edmondson]

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

On Thu, 22 Dec 2011 09:29:55 -0500, Aaron Ecay <aaronecay@gmail.com> wrote:
> Would the problem you had with previous-s-c-prop-change be fixed by the
> patch to the original function I sent in the thread starting at
> id:"m2y5u5cykp.fsf@kcals.intra.maillard.im" ?

I think so, yes. Re-writing the new version I posted to use
`previous-single-char-property-value' in the way that you describe:

   ((let ((visible-bottom (notmuch-show-message-bottom)))
      (while (invisible-p visible-bottom)
	(setq visible-bottom (max (point-min)
				  (1- (previous-single-char-property-change
				       visible-bottom 'invisible)))))
      (> visible-bottom (window-end)))
    ;; The end of this message is not visible - scroll.
    (scroll-up)
    nil)

seems to work in quick testing.

I'd vote to integrate your change if it seems correct to everyone else,
then I'll argue for a re-write separately.

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

Re: [RFC][PATCH v2] emacs: Re-implement advance/rewind functions of notmuch-show-mode.

[Xavier Maillard]

On Thu, 22 Dec 2011 09:29:55 -0500, Aaron Ecay <aaronecay@gmail.com> wrote:

> Would the problem you had with previous-s-c-prop-change be fixed by the
> patch to the original function I sent in the thread starting at
> id:"m2y5u5cykp.fsf@kcals.intra.maillard.im" ?

AFAIK it does.

/Xavier