--- /dev/null
+Return-Path: <dmitry.kurochkin@gmail.com>\r
+X-Original-To: notmuch@notmuchmail.org\r
+Delivered-To: notmuch@notmuchmail.org\r
+Received: from localhost (localhost [127.0.0.1])\r
+ by olra.theworths.org (Postfix) with ESMTP id 14C5B431FD0\r
+ for <notmuch@notmuchmail.org>; Fri, 23 Dec 2011 11:02:22 -0800 (PST)\r
+X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
+X-Spam-Flag: NO\r
+X-Spam-Score: -0.799\r
+X-Spam-Level: \r
+X-Spam-Status: No, score=-0.799 tagged_above=-999 required=5\r
+ tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1,\r
+ FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
+Received: from olra.theworths.org ([127.0.0.1])\r
+ by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
+ with ESMTP id GjCE4GT8NFVb for <notmuch@notmuchmail.org>;\r
+ Fri, 23 Dec 2011 11:02:20 -0800 (PST)\r
+Received: from mail-wi0-f181.google.com (mail-wi0-f181.google.com\r
+ [209.85.212.181]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
+ (No client certificate requested)\r
+ by olra.theworths.org (Postfix) with ESMTPS id 42B7F431FB6\r
+ for <notmuch@notmuchmail.org>; Fri, 23 Dec 2011 11:02:20 -0800 (PST)\r
+Received: by wibhq2 with SMTP id hq2so4080069wib.26\r
+ for <notmuch@notmuchmail.org>; Fri, 23 Dec 2011 11:02:19 -0800 (PST)\r
+DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma;\r
+ h=from:to:subject:in-reply-to:references:user-agent:date:message-id\r
+ :mime-version:content-type;\r
+ bh=wGaACLCj5B47Gfp94MLc7XVeZ8HbuoxB81o01sh2Xac=;\r
+ b=u3Lr/uhC0gL7n5RkhWITu2dWDNW/3Rmi8W/n9THHPzPMSCCj8rQ2t2f7ZHmFzxy7RO\r
+ 0bHXrLvGxqRep7lWqzghUMw0KgpctyYO+SJHg9nW/P7BUlDlFJYyXDz3OPmbzDmJAFR0\r
+ Wr1WDNXDTPtDkjUcv0pO395FXDsPkpP60VUoY=\r
+Received: by 10.180.79.35 with SMTP id g3mr34690292wix.19.1324666939111;\r
+ Fri, 23 Dec 2011 11:02:19 -0800 (PST)\r
+Received: from localhost ([91.144.186.21])\r
+ by mx.google.com with ESMTPS id k5sm33967196wiz.9.2011.12.23.11.02.17\r
+ (version=TLSv1/SSLv3 cipher=OTHER);\r
+ Fri, 23 Dec 2011 11:02:18 -0800 (PST)\r
+From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>\r
+To: David Edmondson <dme@dme.org>, notmuch@notmuchmail.org\r
+Subject: Re: [RFC][PATCH v4] emacs: Re-implement advance/rewind functions of\r
+ notmuch-show-mode.\r
+In-Reply-To: <1324665712-2419-1-git-send-email-dme@dme.org>\r
+References: <id:"1324553312-10972-1-git-send-email-dme@dme.org">\r
+ <1324665712-2419-1-git-send-email-dme@dme.org>\r
+User-Agent: Notmuch/0.10.2+116~g4c449d4 (http://notmuchmail.org) Emacs/23.3.1\r
+ (x86_64-pc-linux-gnu)\r
+Date: Fri, 23 Dec 2011 23:01:33 +0400\r
+Message-ID: <87ipl7kt82.fsf@gmail.com>\r
+MIME-Version: 1.0\r
+Content-Type: text/plain; charset=us-ascii\r
+X-BeenThere: notmuch@notmuchmail.org\r
+X-Mailman-Version: 2.1.13\r
+Precedence: list\r
+List-Id: "Use and development of the notmuch mail system."\r
+ <notmuch.notmuchmail.org>\r
+List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
+ <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
+List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
+List-Post: <mailto:notmuch@notmuchmail.org>\r
+List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
+List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
+ <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
+X-List-Received-Date: Fri, 23 Dec 2011 19:02:22 -0000\r
+\r
+Hi David.\r
+\r
+On Fri, 23 Dec 2011 18:41:52 +0000, David Edmondson <dme@dme.org> wrote:\r
+> The advance/rewind functions had become complex, which made it hard to\r
+> determine how they are expected to behave. Re-implement them simply in\r
+> order to poll user-experience and expectation.\r
+> ---\r
+> \r
+> Switched back to using `previous-single-char-property-change' now that\r
+> Aaron explained it. Fix a bug rewinding when the start of the current\r
+> message is visible.\r
+> \r
+\r
+I did not do a proper review. But here are few comments:\r
+\r
+* Revert changes to notmuch-show-advance-and-archive.\r
+\r
+* Can we split this in two patches? One for rewind and another for\r
+ advance.\r
+\r
+* Does this patch change the behavior of the functions or is it just\r
+ meant to simplify the code? If it is the former, it would be really\r
+ nice to have tests for it.\r
+\r
+Regards,\r
+ Dmitry\r
+\r
+> emacs/notmuch-show.el | 132 +++++++++++++++++++++++++++----------------------\r
+> 1 files changed, 73 insertions(+), 59 deletions(-)\r
+> \r
+> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el\r
+> index 46525aa..e914ce1 100644\r
+> --- a/emacs/notmuch-show.el\r
+> +++ b/emacs/notmuch-show.el\r
+> @@ -1156,38 +1156,56 @@ Some useful entries are:\r
+> ;; Commands typically bound to keys.\r
+> \r
+> (defun notmuch-show-advance ()\r
+> - "Advance through thread.\r
+> + "Advance through the current thread.\r
+> \r
+> -If the current message in the thread is not yet fully visible,\r
+> -scroll by a near screenful to read more of the message.\r
+> +Scroll the current message if the end of it is not visible,\r
+> +otherwise move to the next message.\r
+> \r
+> -Otherwise, (the end of the current message is already within the\r
+> -current window), advance to the next open message."\r
+> +Return `t' if we are at the end of the last message, otherwise\r
+> +`nil'."\r
+> (interactive)\r
+> - (let* ((end-of-this-message (notmuch-show-message-bottom))\r
+> - (visible-end-of-this-message (1- end-of-this-message))\r
+> - (ret nil))\r
+> - (while (invisible-p visible-end-of-this-message)\r
+> - (setq visible-end-of-this-message\r
+> - (previous-single-char-property-change visible-end-of-this-message\r
+> - 'invisible)))\r
+> - (cond\r
+> - ;; Ideally we would test `end-of-this-message' against the result\r
+> - ;; of `window-end', but that doesn't account for the fact that\r
+> - ;; the end of the message might be hidden.\r
+> - ((and visible-end-of-this-message\r
+> - (> visible-end-of-this-message (window-end)))\r
+> - ;; The bottom of this message is not visible - scroll.\r
+> - (scroll-up nil))\r
+> -\r
+> - ((not (= end-of-this-message (point-max)))\r
+> - ;; This is not the last message - move to the next visible one.\r
+> - (notmuch-show-next-open-message))\r
+> -\r
+> - (t\r
+> - ;; This is the last message - change the return value\r
+> - (setq ret t)))\r
+> - ret))\r
+> + (cond\r
+> + ((eobp)\r
+> + ;; We are at the end of the buffer - move to the next thread.\r
+> + t)\r
+> +\r
+> + ;; Ideally we would simply do:\r
+> + ;; \r
+> + ;; ((> (notmuch-show-message-bottom) (window-end))\r
+> + ;; \r
+> + ;; here, but that fails if the trailing text in the buffer is\r
+> + ;; invisible (`window-end' returns the last _visible_ character,\r
+> + ;; which can then be smaller than `notmuch-show-message-bottom').\r
+> + ;;\r
+> + ;; So we need to find the last visible character of the message. We\r
+> + ;; do this by searching backwards from\r
+> + ;; `notmuch-show-message-bottom' for changes in the `invisible'\r
+> + ;; property until we find a non-invisible character. When we find\r
+> + ;; such a character we test to see whether it is visible in the\r
+> + ;; window.\r
+> + ;;\r
+> + ;; Properties change between characters - the return value of\r
+> + ;; `previous-single-char-property-change' points to the first\r
+> + ;; character _inside_ the region with the `invisible' property\r
+> + ;; set. To allow for this we step backwards one character upon\r
+> + ;; finding the start of the invisible region.\r
+> +\r
+> + ((> (let ((visible-bottom (notmuch-show-message-bottom)))\r
+> + (while (invisible-p visible-bottom)\r
+> + (setq visible-bottom (max (point-min)\r
+> + (1- (previous-single-char-property-change\r
+> + visible-bottom 'invisible)))))\r
+> + visible-bottom) (window-end))\r
+> + ;; The end of this message is not visible - scroll to show more of\r
+> + ;; it.\r
+> + (scroll-up)\r
+> + nil)\r
+> +\r
+> + (t\r
+> + ;; All of the current message has been seen - show the start of\r
+> + ;; the next open message.\r
+> + (notmuch-show-next-open-message)\r
+> + nil)))\r
+> \r
+> (defun notmuch-show-advance-and-archive ()\r
+> "Advance through thread and archive.\r
+> @@ -1201,44 +1219,40 @@ from each message), kills the buffer, and displays the next\r
+> thread from the search from which this thread was originally\r
+> shown."\r
+> (interactive)\r
+> - (if (notmuch-show-advance)\r
+> - (notmuch-show-archive-thread)))\r
+> + (when (notmuch-show-advance)\r
+> + (notmuch-show-archive-thread)))\r
+> \r
+> (defun notmuch-show-rewind ()\r
+> - "Backup through the thread, (reverse scrolling compared to \\[notmuch-show-advance-and-archive]).\r
+> + "Move backwards through a thread, the counterpart to \\[notmuch-show-advance-and-archive]."\r
+> \r
+> -Specifically, if the beginning of the previous email is fewer\r
+> -than `window-height' lines from the current point, move to it\r
+> -just like `notmuch-show-previous-message'.\r
+> -\r
+> -Otherwise, just scroll down a screenful of the current message.\r
+> -\r
+> -This command does not modify any message tags, (it does not undo\r
+> -any effects from previous calls to\r
+> -`notmuch-show-advance-and-archive'."\r
+> (interactive)\r
+> - (let ((start-of-message (notmuch-show-message-top))\r
+> - (start-of-window (window-start)))\r
+> + (let ((start-of-message (notmuch-show-message-top)))\r
+> (cond\r
+> - ;; Either this message is properly aligned with the start of the\r
+> - ;; window or the start of this message is not visible on the\r
+> - ;; screen - scroll.\r
+> - ((or (= start-of-message start-of-window)\r
+> - (< start-of-message start-of-window))\r
+> + ((= start-of-message (point))\r
+> + ;; The cursor is at the start of the current message - move to\r
+> + ;; the previous open message.\r
+> + (notmuch-show-previous-open-message))\r
+> +\r
+> + ((< start-of-message (window-start))\r
+> + ;; The start of the current message is not visible - scroll\r
+> + ;; down.\r
+> (scroll-down)\r
+> - ;; If a small number of lines from the previous message are\r
+> - ;; visible, realign so that the top of the current message is at\r
+> - ;; the top of the screen.\r
+> - (if (<= (count-screen-lines (window-start) start-of-message)\r
+> - next-screen-context-lines)\r
+> - (progn\r
+> - (goto-char (notmuch-show-message-top))\r
+> - (notmuch-show-message-adjust)))\r
+> - ;; Move to the top left of the window.\r
+> - (goto-char (window-start)))\r
+> + ;; If the start of the current message became visible, align it\r
+> + ;; with the top of the window.\r
+> + (when (> start-of-message (window-start))\r
+> + (goto-char start-of-message)\r
+> + (notmuch-show-message-adjust)))\r
+> +\r
+> + ((>= start-of-message (window-start))\r
+> + ;; The start of the current message is visible in the window.\r
+> + ;;\r
+> + ;; If the cursor is not at the start of the current message,\r
+> + ;; move it there, but do not adjust or scroll the display.\r
+> + (goto-char start-of-message))\r
+> +\r
+> (t\r
+> ;; Move to the previous message.\r
+> - (notmuch-show-previous-message)))))\r
+> + (notmuch-show-previous-open-message)))))\r
+> \r
+> (defun notmuch-show-reply (&optional prompt-for-sender)\r
+> "Reply to the current message."\r
+> -- \r
+> 1.7.7.3\r
+> \r
+> _______________________________________________\r
+> notmuch mailing list\r
+> notmuch@notmuchmail.org\r
+> http://notmuchmail.org/mailman/listinfo/notmuch\r
projects / notmuch-archives.git / commitdiff