From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by olra.theworths.org (Postfix) with ESMTP id 53590431FD0 for ; Sat, 24 Dec 2011 17:05:41 -0800 (PST) X-Virus-Scanned: Debian amavisd-new at olra.theworths.org X-Spam-Flag: NO X-Spam-Score: -0.7 X-Spam-Level: X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5 tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled Received: from olra.theworths.org ([127.0.0.1]) by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id TynwI3CCRu3v for ; Sat, 24 Dec 2011 17:05:40 -0800 (PST) Received: from dmz-mailsec-scanner-2.mit.edu (DMZ-MAILSEC-SCANNER-2.MIT.EDU [18.9.25.13]) by olra.theworths.org (Postfix) with ESMTP id 482EB431FB6 for ; Sat, 24 Dec 2011 17:05:40 -0800 (PST) X-AuditID: 1209190d-b7f576d0000008c4-7d-4ef676e2995b Received: from mailhub-auth-2.mit.edu ( [18.7.62.36]) by dmz-mailsec-scanner-2.mit.edu (Symantec Messaging Gateway) with SMTP id 16.E4.02244.2E676FE4; Sat, 24 Dec 2011 20:05:38 -0500 (EST) Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103]) by mailhub-auth-2.mit.edu (8.13.8/8.9.2) with ESMTP id pBP15cOc019573; Sat, 24 Dec 2011 20:05:38 -0500 Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91]) (authenticated bits=0) (User authenticated as amdragon@ATHENA.MIT.EDU) by outgoing.mit.edu (8.13.6/8.12.4) with ESMTP id pBP15b1p029641 (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NOT); Sat, 24 Dec 2011 20:05:37 -0500 (EST) Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.77) (envelope-from ) id 1RecXr-0004Qn-Jr; Sat, 24 Dec 2011 20:06:35 -0500 Date: Sat, 24 Dec 2011 20:06:35 -0500 From: Austin Clements To: David Edmondson Subject: Re: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of notmuch-show-mode. Message-ID: <20111225010635.GG1927@mit.edu> References: <1324665712-2419-1-git-send-email-dme@dme.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1324665712-2419-1-git-send-email-dme@dme.org> User-Agent: Mutt/1.5.21 (2010-09-15) X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFmpileLIzCtJLcpLzFFi42IRYrdT0X1U9s3PoHeCqsW+O1uYLK7fnMns wOSx6/lfJo9nq24xBzBFcdmkpOZklqUW6dslcGWcWR9ZcMGh4urJe0wNjJcNuxg5OSQETCQ2 LexmhrDFJC7cW8/WxcjFISSwj1Fi+eONjBDOBkaJ9ub/zBDOSSaJKY1noMqWMEpceH+QBaSf RUBVYu2/X+wgNpuAhsS2/csZQWwRAUWJ/99WgMWZBaQlvv1uZgKxhQUSJW4tngxkc3DwCmhL XJqrABIWEiiUOHjyNNhJvAKCEidnPmGBaNWSuPHvJVg5yJjl/zhAwpwC1hJtzT/ANokKqEhM ObmNbQKj0Cwk3bOQdM9C6F7AyLyKUTYlt0o3NzEzpzg1Wbc4OTEvL7VI10gvN7NELzWldBMj KKg5JXl3ML47qHSIUYCDUYmHt3HpFz8h1sSy4srcQ4ySHExKorxGxd/8hPiS8lMqMxKLM+KL SnNSiw8xSnAwK4nwaiYBlfOmJFZWpRblw6SkOViUxHlVtd75CQmkJ5akZqemFqQWwWRlODiU JHhLgdErJFiUmp5akZaZU4KQZuLgBBnOAzQ8BKSGt7ggMbc4Mx0if4pRUUqcVx0kIQCSyCjN g+uFJZ1XjOJArwjzyoJU8QATFlz3K6DBTECDY4xAri4uSURISTUwth45+2+Szbmw6kfba1tP Z36xPta3zz6p+eir0vfWni1nf7/ZEnas1TImqt1RyP53YxHnmtCtM66XlG3z3195/ukaC673 h/45SFznys85IhvuoaS9Jmfi3OpjvzoVa+Ue5Hw/aPP3fc23q78qb0/MlzP9EpM7wXnDwhID 9tzjgWJpDv95eGwfKLEUZyQaajEXFScCAHTzGV8VAwAA Cc: notmuch@notmuchmail.org X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: "Use and development of the notmuch mail system." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 25 Dec 2011 01:05:41 -0000 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."