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

[RFC][PATCH v4] 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 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

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

[Dmitry Kurochkin]

Hi David.

On Fri, 23 Dec 2011 18:41:52 +0000, David Edmondson <dme@dme.org> wrote:
> 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.
> 

I did not do a proper review.  But here are few comments:

* Revert changes to notmuch-show-advance-and-archive.

* Can we split this in two patches?  One for rewind and another for
  advance.

* Does this patch change the behavior of the functions or is it just
  meant to simplify the code?  If it is the former, it would be really
  nice to have tests for it.

Regards,
  Dmitry

>  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
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

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

[Austin Clements]

Awesome.  This looks significantly cleaner.  I think this is worth
pushing for the comment you added to notmuch-show-advance alone.

This definitely changes the behavior of rewind, but other than one
case I point out below, I think what you have now is much closer to an
inverse of advance.  It would be nice to have tests for rewind (looks
like we don't have any right now), but it would seem counterproductive
to write tests for the current rewind only to rewrite them for this
rewind.

A few comments inline.

Quoth David Edmondson on Dec 23 at  6:41 pm:
> 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:
> +   ;; 

Tailing whitespace.

> +   ;; 	((> (notmuch-show-message-bottom) (window-end))
> +   ;; 

More trailing whitespace.

> +   ;; 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))

This will jump to the beginning of the previous message if I'm at the
beginning of a message.  I would expect rewind to show me the end of
the previous message in this case.

Other than this, I think the logic all makes sense, though I'm not
positive I was able to exercise all of the branches.

> +
> +     ((< 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."

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

[Aaron Ecay]

On Sat, 24 Dec 2011 20:06:35 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> Awesome.  This looks significantly cleaner.  I think this is worth
> pushing for the comment you added to notmuch-show-advance alone.

+1

> Quoth David Edmondson on Dec 23 at  6:41 pm:
> > 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:
> > +   ;; 
> 
> Tailing whitespace.
> 
> > +   ;; 	((> (notmuch-show-message-bottom) (window-end))
> > +   ;; 
> 
> More trailing whitespace.
> 
> > +   ;; 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))

Can this (let...) be lifted out of the (cond...)?  IMO it is very
confusing to be doing non-trivial computation in the test portion of a
cond form.

> > +    ;; 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))
> 
> This will jump to the beginning of the previous message if I'm at the
> beginning of a message.  I would expect rewind to show me the end of
> the previous message in this case.

Agreed.  I would like to see this case move back one screenful of text or
to the previous beginning-of-message, whichever is shorter.  Something
like this should do the trick (replacing the notmuch-show-prev-msg call):

(let ((last-msg-point (save-excursion
			      (notmuch-show-goto-message-previous)
			      (point))))
	(scroll-down)
	(if (> last-msg-point (window-start))
	    (goto-char last-msg-point)
	  (goto-char (window-start)))
	(notmuch-show-message-adjust))

Thanks,

-- 
Aaron Ecay

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

[David Edmondson]

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

On Fri, 23 Dec 2011 23:01:33 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> * Revert changes to notmuch-show-advance-and-archive.

Why? (I mean, because the change is poor or just that it's unrelated or
because I didn't mention it)

> * Can we split this in two patches?  One for rewind and another for
>   advance.

I'll think about that. Is there a specific reason? I'm not particularly
in favour of splitting things just for the sake of it.

> * Does this patch change the behavior of the functions or is it just
>   meant to simplify the code?  If it is the former, it would be really
>   nice to have tests for it.

I believe that it changes the behaviour. I'll write tests.

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

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

[David Edmondson]

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

On Sat, 24 Dec 2011 20:06:35 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> Awesome.  This looks significantly cleaner.  I think this is worth
> pushing for the comment you added to notmuch-show-advance alone.

Thanks.

> This definitely changes the behavior of rewind, but other than one
> case I point out below, I think what you have now is much closer to an
> inverse of advance.  It would be nice to have tests for rewind (looks
> like we don't have any right now), but it would seem counterproductive
> to write tests for the current rewind only to rewrite them for this
> rewind.

I'll write some tests.

> Tailing whitespace.

Will fix.

> > +     ((= 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))
> 
> This will jump to the beginning of the previous message if I'm at the
> beginning of a message.  I would expect rewind to show me the end of
> the previous message in this case.

That would definitely more closely be the inverse of how advance works,
but is it the most useful behaviour?

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

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

[David Edmondson]

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

On Sun, 25 Dec 2011 23:11:27 -0500, Aaron Ecay <aaronecay@gmail.com> wrote:
> > > +   ((> (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))
> 
> Can this (let...) be lifted out of the (cond...)?  IMO it is very
> confusing to be doing non-trivial computation in the test portion of a
> cond form.

It ends up a long way from where it's used, diluting the value of the
comment. I do like the current layout, but what if it was (something
like):

   ((let ((visible-bottom (1- (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)

That would seem more palatable, perhaps.

> Agreed.  I would like to see this case move back one screenful of text or
> to the previous beginning-of-message, whichever is shorter.

See previous comment - I agreed that it's not symmetric - just wonder
which is more useful behaviour.

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

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

[Dmitry Kurochkin]

Hi David.

On Mon, 26 Dec 2011 10:46:13 +0000, David Edmondson <dme@dme.org> wrote:
> On Fri, 23 Dec 2011 23:01:33 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > * Revert changes to notmuch-show-advance-and-archive.
> 
> Why? (I mean, because the change is poor or just that it's unrelated or
> because I didn't mention it)
> 

Because it is unrelated.

And can you please explain why `when' is better than `if' here?  Then I
will know which one to use the next time :)

> > * Can we split this in two patches?  One for rewind and another for
> >   advance.
> 
> I'll think about that. Is there a specific reason? I'm not particularly
> in favour of splitting things just for the sake of it.
> 

Because they are independent and can be split.  And it is easier to
review (and work in general, I suppose) with two smaller patches than
with a single bigger one.

Though, since you got two other reviews already, you can just ignore
this.

> > * Does this patch change the behavior of the functions or is it just
> >   meant to simplify the code?  If it is the former, it would be really
> >   nice to have tests for it.
> 
> I believe that it changes the behaviour. I'll write tests.

Thanks.

Regards,
  Dmitry

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

[David Edmondson]

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

On Mon, 26 Dec 2011 15:09:55 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> Hi David.
> 
> On Mon, 26 Dec 2011 10:46:13 +0000, David Edmondson <dme@dme.org> wrote:
> > On Fri, 23 Dec 2011 23:01:33 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > > * Revert changes to notmuch-show-advance-and-archive.
> > 
> > Why? (I mean, because the change is poor or just that it's unrelated or
> > because I didn't mention it)
> > 
> 
> Because it is unrelated.

Understood. For me this fell inside the 'trivial other change' boundary.

> And can you please explain why `when' is better than `if' here?  Then I
> will know which one to use the next time :)

`if' allows only a single statement for `then', which results in code like:

     (if foo
        (progn
          (this)
          (that)
          (theother)))

so if there is no `else' clause I've been preferring:

      (when foo
        (this)
        (that)
        (theother))

but that's obviously personal and not important in this specific case.

> > > * Can we split this in two patches?  One for rewind and another for
> > >   advance.
> > 
> > I'll think about that. Is there a specific reason? I'm not particularly
> > in favour of splitting things just for the sake of it.
> > 
> 
> Because they are independent and can be split.  And it is easier to
> review (and work in general, I suppose) with two smaller patches than
> with a single bigger one.

Your git-fu is obviously much stronger than mine. :-) Rebasing (groups
of) patches takes more of my time and is more error prone than I'd like.

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

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

[Jameson Graef Rollins]

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

On Mon, 26 Dec 2011 22:00:21 +0000, David Edmondson <dme@dme.org> wrote:
> Understood. For me this fell inside the 'trivial other change' boundary.

Fwiw, I don't remember there ever being such a distinction.  I think
we've always insisted that unrelated changes should be excluded.  As a
general rule, I think all patches should be as atomic as they can
possibly be.  I would much rather have more smaller, atomic patches than
fewer longer, composite ones.

> > And can you please explain why `when' is better than `if' here?  Then I
> > will know which one to use the next time :)
> 
> `if' allows only a single statement for `then', which results in code like:
> 
>      (if foo
>         (progn
>           (this)
>           (that)
>           (theother)))
> 
> so if there is no `else' clause I've been preferring:
> 
>       (when foo
>         (this)
>         (that)
>         (theother))
> 
> but that's obviously personal and not important in this specific case.

That's good.  I like it.  The if construction always annoyed me a bit
for this reason.  The when construction is definitely cleaner.

jamie.

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

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

[David Edmondson]

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

On Mon, 26 Dec 2011 14:24:11 -0800, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> On Mon, 26 Dec 2011 22:00:21 +0000, David Edmondson <dme@dme.org> wrote:
> > Understood. For me this fell inside the 'trivial other change' boundary.
> 
> Fwiw, I don't remember there ever being such a distinction.  I think
> we've always insisted that unrelated changes should be excluded.

I didn't mean to claim that the project had such a rule, just that I did.

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

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

[David Edmondson]

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

On Mon, 26 Dec 2011 10:49:46 +0000, David Edmondson <dme@dme.org> wrote:
> > > +     ((= 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))
> > 
> > This will jump to the beginning of the previous message if I'm at the
> > beginning of a message.  I would expect rewind to show me the end of
> > the previous message in this case.
> 
> That would definitely more closely be the inverse of how advance works,
> but is it the most useful behaviour?

I thought about this a bit more (mostly because I don't want to write
tests for one behaviour and then have to change them - writing tests for
emacs with the current test suite is painful).

If you want to go back a page you can use M-v. The whole point of
binding DEL to something in `notmuch-show-mode' is that it implement
something other than the vanilla behaviour. Simply showing the previous
page (the equivalent of M-v) adds no value.

Hence, I'd like to keep the (jump back a full message) behaviour in the
patch.

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

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

[Aaron Ecay]

On Wed, 28 Dec 2011 15:22:45 +0000, David Edmondson <dme@dme.org> wrote:
> I thought about this a bit more (mostly because I don't want to write
> tests for one behaviour and then have to change them - writing tests for
> emacs with the current test suite is painful).
> 
> If you want to go back a page you can use M-v. The whole point of
> binding DEL to something in `notmuch-show-mode' is that it implement
> something other than the vanilla behaviour. Simply showing the previous
> page (the equivalent of M-v) adds no value.

If you want to jump back to the previous message, you can press `p'.
(And M-v is a chord whereas DEL and p are a simple keystroke, so it’s
arguably maximally convenient to duplicate a chord command on a single
key rather than duplicating a single key on another single key.)

The way I look at it, notmuch has two sets of movement keys – n/p and
SPC/DEL.  The former moves by messages, and the latter by screenfuls
(with the added complication that the screenful movement commands also
stop at intervening message boundaries).  I’d prefer to maintain that
symmetry.

-- 
Aaron Ecay

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

[Jameson Graef Rollins]

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

On Wed, 28 Dec 2011 13:04:02 -0500, Aaron Ecay <aaronecay@gmail.com> wrote:
> The way I look at it, notmuch has two sets of movement keys – n/p and
> SPC/DEL.  The former moves by messages, and the latter by screenfuls
> (with the added complication that the screenful movement commands also
> stop at intervening message boundaries).  I’d prefer to maintain that
> symmetry.

agreed.

jamie.

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

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

[David Edmondson]

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

Okay, I'll buy it. Patch version 5 in a while...

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

[PATCH 1/2] 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 how they are expected to behave. Re-implement them simply
(!) in order to poll user-experience and expectation.
---

Rework re-wind in light of discussion.

 emacs/notmuch-show.el |  156 +++++++++++++++++++++++++++++++------------------
 1 files changed, 99 insertions(+), 57 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 5502efd..60af88b 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1151,39 +1151,57 @@ 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
-	    (max (point-min)
-		 (1- (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 `(1-
+   ;; 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 and also at the start
+   ;; of the search.
+
+   ((let ((visible-bottom (1- (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,40 +1219,64 @@ shown."
       (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.
+      (let ((start-of-previous (save-excursion
+				 (while (and (notmuch-show-goto-message-previous)
+					     (not (notmuch-show-message-visible-p))))
+				 (notmuch-show-message-top))))
+	;; If the start of the previous open message is visible on
+	;; screen, move the cursor there, but do not adjust or scroll
+	;; the display.
+	(if (> start-of-previous (window-start))
+	    (goto-char start-of-previous)
+
+	  ;; Otherwise, the start of the previous open message is
+	  ;; _not_ visible on screen.
+	  ;;
+	  ;; Scroll the window to show (some (more) of) the previous
+	  ;; message and move up into it.
+	  (scroll-down)
+	  (forward-line -1)
+
+	  ;; If the start of the previous message became visible on
+	  ;; screen due to the scrolling, align it with the top of the
+	  ;; window.
+	  (if (> start-of-previous (window-start))
+	      (progn
+		(goto-char start-of-previous)
+		(notmuch-show-message-adjust))
+	    ;; Otherwise leave the cursor at the start of the window.
+	    (goto-char (window-start))))))
+
+     ((< 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)
+      ;; If the start of the current message became visible, align it
+      ;; with the top of the window.
+      (if (> start-of-message (window-start))
 	  (progn
-	    (goto-char (notmuch-show-message-top))
-	    (notmuch-show-message-adjust)))
-      ;; Move to the top left of the window.
-      (goto-char (window-start)))
+	    (goto-char start-of-message)
+	    (notmuch-show-message-adjust))
+	;; Otherwise leave the cursor at the start of the window.
+	(goto-char (window-start))))
+
+     ((>= start-of-message (window-start))
+      ;; The start of the current message is visible in the window.
+      ;;
+      ;; Move the cursor to the start of the current message, 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

[PATCH 2/2] test: Add tests for advance/rewind.

[David Edmondson]

---

These tests pass when run interactively (via dtach/run_emacs and in a
normal session) with both emacs23 and emacs24. They pass when run via
the test suite with emacs24, but emacs23 behaves differently (the
result of scrolling is different). I've yet to figure out why.

 emacs/notmuch-test.el |  128 +++++++++++++++++++++++++++++++++++++++++++++++++
 test/emacs            |   13 +++++
 2 files changed, 141 insertions(+), 0 deletions(-)
 create mode 100644 emacs/notmuch-test.el

diff --git a/emacs/notmuch-test.el b/emacs/notmuch-test.el
new file mode 100644
index 0000000..307adfa
--- /dev/null
+++ b/emacs/notmuch-test.el
@@ -0,0 +1,128 @@
+;; notmuch-test.el --- testing the emacs interface
+;;
+;; Copyright © David Edmondson
+;;
+;; This file is part of Notmuch.
+;;
+;; Notmuch is free software: you can redistribute it and/or modify it
+;; under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+;;
+;; Notmuch is distributed in the hope that it will be useful, but
+;; WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+;; General Public License for more details.
+;;
+;; You should have received a copy of the GNU General Public License
+;; along with Notmuch.  If not, see <http://www.gnu.org/licenses/>.
+;;
+;; Authors: David Edmondson <dme@dme.org>
+
+(defvar notmuch-test-tests '(notmuch-test-show-advance-simple
+			     notmuch-test-show-advance-twice
+			     notmuch-test-show-advance-rewind
+			     notmuch-test-show-advance-twice-rewind
+			     notmuch-test-show-advance-not-eobp
+			     notmuch-test-show-advance-eobp)
+  "A list of tests.")
+
+(defun notmuch-test-all ()
+  "Wrapper for ease of test development."
+  (let ((result (get-buffer-create "*notmuch-test*")))
+    (set-buffer result)
+    (erase-buffer)
+    (dolist (test notmuch-test-tests)
+      (insert (format "%40S: %S\n"
+		      test
+		      (save-excursion
+			(funcall test)))))
+    (switch-to-buffer result)))
+
+;;
+
+(defun notmuch-test-show-advance-simple ()
+  ;; Find a particular thread.
+  (notmuch-show "id:20091117190054.GU3165@dottiness.seas.harvard.edu")
+  ;; Ensure that all messages are open.
+  (notmuch-show-open-or-close-all)
+  (redisplay t)
+  ;; Advance.
+  (notmuch-show-advance)
+  (redisplay t)
+  ;; The first message in the thread is longer than a screen, so we
+  ;; should just have scrolled normally.
+  (if (= (line-number-at-pos) 19)
+      t
+    (cons (line-number-at-pos) 19)))
+
+(defun notmuch-test-show-advance-twice ()
+  ;; Find a particular thread.
+  (notmuch-show "id:20091117190054.GU3165@dottiness.seas.harvard.edu")
+  ;; Ensure that all messages are open.
+  (notmuch-show-open-or-close-all)
+  (redisplay t)
+  ;; Advance twice.
+  (notmuch-show-advance)
+  (redisplay t)
+  (notmuch-show-advance)
+  (redisplay t)
+  ;; The first message in the thread is shorter than two screens, so
+  ;; we should be at the start of the second message.
+  (if (= (line-number-at-pos) 41)
+      t
+    (cons (line-number-at-pos) 41)))
+
+(defun notmuch-test-show-advance-rewind ()
+  ;; Find a particular thread.
+  (notmuch-show "id:20091117190054.GU3165@dottiness.seas.harvard.edu")
+  ;; Ensure that all messages are open.
+  (notmuch-show-open-or-close-all)
+  (redisplay t)
+  ;; Advance.
+  (notmuch-show-advance)
+  (redisplay t)
+  ;; Rewind.
+  (notmuch-show-rewind)
+  (redisplay t)
+  ;; Should be back at the start.
+  (if (= (line-number-at-pos) 1)
+      t
+    (cons (line-number-at-pos) 1)))
+
+(defun notmuch-test-show-advance-twice-rewind ()
+  ;; Find a particular thread.
+  (notmuch-show "id:20091117190054.GU3165@dottiness.seas.harvard.edu")
+  ;; Ensure that all messages are open.
+  (notmuch-show-open-or-close-all)
+  (redisplay t)
+  ;; Advance twice.
+  (notmuch-show-advance)
+  (redisplay t)
+  (notmuch-show-advance)
+  (redisplay t)
+  ;; Rewind.
+  (notmuch-show-rewind)
+  (redisplay t)
+  ;; The first message in the thread is shorter than two screens, so
+  ;; we should be in the middle of it.
+  (if (= (line-number-at-pos) 17)
+      t
+    (cons (line-number-at-pos) 17)))
+
+(defun notmuch-test-show-advance-not-eobp ()
+  ;; Find a particular thread.
+  (notmuch-show "id:20091117190054.GU3165@dottiness.seas.harvard.edu")
+  ;; From here `advance' should never cause us to leave the buffer.
+  (not (notmuch-show-advance)))
+
+(defun notmuch-test-show-advance-eobp ()
+  ;; Find a particular thread.
+  (notmuch-show "id:20091117190054.GU3165@dottiness.seas.harvard.edu")
+  (goto-char (point-max))
+  ;; From here `advance' should always cause us to leave the buffer.
+  (notmuch-show-advance))
+
+;;
+
+(provide 'notmuch-test)
diff --git a/test/emacs b/test/emacs
index ca82445..a1f1abc 100755
--- a/test/emacs
+++ b/test/emacs
@@ -514,4 +514,17 @@ counter=$(test_emacs \
 )
 test_expect_equal "$counter" 2
 
+for i in \
+    show-advance-simple \
+    show-advance-twice \
+    show-advance-rewind \
+    show-advance-twice-rewind \
+    show-advance-not-eobp \
+    show-advance-eobp
+do
+    test_begin_subtest "notmuch $i"
+    result=$(test_emacs '(require (quote notmuch-test)) (notmuch-test-'$i')')
+    test_expect_equal "$result" t
+done
+
 test_done
-- 
1.7.7.3

Re: [PATCH 2/2] test: Add tests for advance/rewind.

[David Edmondson]

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

What's the general feeling about this approach to producing tests for
the emacs UI? (That is, code the test in a .el file and call the
relevant function(s) from the test harness.)

It makes it simpler to develop and maintain the test (because you can do
more work with traditional emacs support for editing elisp), but might
make interpreting failures more difficult (the test harness mostly just
reports 'failed').

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

Re: [PATCH 2/2] test: Add tests for advance/rewind.

[Jameson Graef Rollins]

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

On Thu, 29 Dec 2011 16:05:39 +0000, David Edmondson <dme@dme.org> wrote:
> What's the general feeling about this approach to producing tests for
> the emacs UI? (That is, code the test in a .el file and call the
> relevant function(s) from the test harness.)

I think it's a nice idea.  It seems much cleaner than writing ever more
complicated code in the command line.  There was another recent idea to
actually write the python tests in python, if I remember correctly.  It
all seems to make sense to me.  Bash is convenient, but there's no
reason we need to be monogamous, especially when there are ways, with
your technique, to call individual "embedded" tests from the command
line and still take advantage of the bells and whistles of full test
infrastructure.

> It makes it simpler to develop and maintain the test (because you can do
> more work with traditional emacs support for editing elisp), but might
> make interpreting failures more difficult (the test harness mostly just
> reports 'failed').

It is really nice to see diffs on failure, actually, to get a sense of
what exactly went wrong.  Is it possible to have some sort of standard
report to stdout that could provide more info?

jamie.

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

Re: [PATCH 2/2] test: Add tests for advance/rewind.

[David Edmondson]

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

On Fri, 06 Jan 2012 09:10:17 -0800, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> > It makes it simpler to develop and maintain the test (because you can do
> > more work with traditional emacs support for editing elisp), but might
> > make interpreting failures more difficult (the test harness mostly just
> > reports 'failed').
> 
> It is really nice to see diffs on failure, actually, to get a sense of
> what exactly went wrong.

Agreed.

> Is it possible to have some sort of standard report to stdout that
> could provide more info?

A little syntactic sugar will improve things I expect (have to avoid
re-implementing ert though). I'll have a play.

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