[RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode.
Aaron Ecay
aaronecay at gmail.com
Sun Dec 25 20:11:27 PST 2011
On Sat, 24 Dec 2011 20:06:35 -0500, Austin Clements <amdragon at 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
More information about the notmuch
mailing list