From: David Edmondson <dme@dme.org>
To: notmuch@notmuchmail.org
Subject: [RFC][PATCH v3] emacs: Re-implement advance/rewind functions of
	notmuch-show-mode.
Date: Thu, 22 Dec 2011 12:38:38 +0000
Message-Id: <1324557518-19269-1-git-send-email-dme@dme.org>
In-Reply-To: <1324556204-15700-1-git-send-email-dme@dme.org>
References: <1324556204-15700-1-git-send-email-dme@dme.org>
Precedence: list

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