From: David Edmondson <dme@dme.org>
To: notmuch@notmuchmail.org
Subject: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of
	notmuch-show-mode.
Date: Fri, 23 Dec 2011 18:41:52 +0000
Message-Id: <1324665712-2419-1-git-send-email-dme@dme.org>
In-Reply-To: <id:"1324553312-10972-1-git-send-email-dme@dme.org">
References: <id:"1324553312-10972-1-git-send-email-dme@dme.org">
Precedence: list

The advance/rewind functions had become complex, which made it hard to
determine how they are expected to behave. Re-implement them simply in
order to poll user-experience and expectation.
---

Switched back to using `previous-single-char-property-change' now that
Aaron explained it. Fix a bug rewinding when the start of the current
message is visible.

 emacs/notmuch-show.el |  132 +++++++++++++++++++++++++++----------------------
 1 files changed, 73 insertions(+), 59 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 46525aa..e914ce1 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1156,38 +1156,56 @@ 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)
+    ;; We are at the end of the buffer - 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 searching backwards from
+   ;; `notmuch-show-message-bottom' for changes in the `invisible'
+   ;; property until we find a non-invisible character. When we find
+   ;; such a character we test to see whether it is visible in the
+   ;; window.
+   ;;
+   ;; Properties change between characters - the return value of
+   ;; `previous-single-char-property-change' points to the first
+   ;; character _inside_ the region with the `invisible' property
+   ;; set. To allow for this we step backwards one character upon
+   ;; finding the start of the invisible region.
+
+   ((> (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 to show more of
+    ;; it.
+    (scroll-up)
+    nil)
+
+   (t
+    ;; All of the current message has been seen - show the start of
+    ;; the next open message.
+    (notmuch-show-next-open-message)
+    nil)))
 
 (defun notmuch-show-advance-and-archive ()
   "Advance through thread and archive.
@@ -1201,44 +1219,40 @@ 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))
+      ;; 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))
+      ;; 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))
+      ;; The start of the current message is visible in the window.
+      ;;
+      ;; If the cursor is not at the start of the current 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