From: Aaron Ecay <aaronecay@gmail.com>
To: Austin Clements <amdragon@MIT.EDU>, David Edmondson <dme@dme.org>
Subject: Re: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of
	notmuch-show-mode.
In-Reply-To: <20111225010635.GG1927@mit.edu>
References: <id:"1324553312-10972-1-git-send-email-dme@dme.org">
	<1324665712-2419-1-git-send-email-dme@dme.org>
	<20111225010635.GG1927@mit.edu>
User-Agent: Notmuch/0.10.1+56~gd709fd6 (http://notmuchmail.org)
	Emacs/24.0.92.3 (i386-apple-darwin10.8.0)
Date: Sun, 25 Dec 2011 23:11:27 -0500
Message-ID: <m2zkegt1jk.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain
Cc: notmuch@notmuchmail.org
Precedence: list

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