--- /dev/null
+Return-Path: <pieter@praet.org>\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 DF237431FB6\r
+ for <notmuch@notmuchmail.org>; Wed, 22 Feb 2012 10:44:20 -0800 (PST)\r
+X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
+X-Spam-Flag: NO\r
+X-Spam-Score: -0.7\r
+X-Spam-Level: \r
+X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5\r
+ tests=[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 8s+-APUknbXF for <notmuch@notmuchmail.org>;\r
+ Wed, 22 Feb 2012 10:44:20 -0800 (PST)\r
+Received: from mail-we0-f181.google.com (mail-we0-f181.google.com\r
+ [74.125.82.181]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
+ (No client certificate requested)\r
+ by olra.theworths.org (Postfix) with ESMTPS id A6A8A431FAE\r
+ for <notmuch@notmuchmail.org>; Wed, 22 Feb 2012 10:44:19 -0800 (PST)\r
+Received: by werp13 with SMTP id p13so265640wer.26\r
+ for <notmuch@notmuchmail.org>; Wed, 22 Feb 2012 10:44:18 -0800 (PST)\r
+Received-SPF: pass (google.com: domain of pieter@praet.org designates\r
+ 10.180.92.227 as permitted sender) client-ip=10.180.92.227; \r
+Authentication-Results: mr.google.com;\r
+ spf=pass (google.com: domain of pieter@praet.org\r
+ designates 10.180.92.227 as permitted sender)\r
+ smtp.mail=pieter@praet.org\r
+Received: from mr.google.com ([10.180.92.227])\r
+ by 10.180.92.227 with SMTP id cp3mr38259690wib.13.1329936258372\r
+ (num_hops = 1); Wed, 22 Feb 2012 10:44:18 -0800 (PST)\r
+Received: by 10.180.92.227 with SMTP id cp3mr31648878wib.13.1329936258296;\r
+ Wed, 22 Feb 2012 10:44:18 -0800 (PST)\r
+Received: from localhost ([109.131.181.26])\r
+ by mx.google.com with ESMTPS id fw5sm30967066wib.0.2012.02.22.10.44.16\r
+ (version=TLSv1/SSLv3 cipher=OTHER);\r
+ Wed, 22 Feb 2012 10:44:17 -0800 (PST)\r
+From: Pieter Praet <pieter@praet.org>\r
+To: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>,\r
+ Notmuch Mail <notmuch@notmuchmail.org>\r
+Subject: Re: [PATCH] emacs: make `notmuch-show-open-or-close-all' toggle\r
+ visibility\r
+In-Reply-To: <87wr7r80re.fsf@gmail.com>\r
+References: <1327469139-1968-1-git-send-email-pieter@praet.org>\r
+ <87wr7r80re.fsf@gmail.com>\r
+User-Agent: Notmuch/0.11.1+210~g6afc43e (http://notmuchmail.org) Emacs/23.3.1\r
+ (x86_64-unknown-linux-gnu)\r
+Date: Wed, 22 Feb 2012 19:41:58 +0100\r
+Message-ID: <87vcmy90cp.fsf@praet.org>\r
+MIME-Version: 1.0\r
+Content-Type: text/plain; charset=us-ascii\r
+X-Gm-Message-State:\r
+ ALoCoQkFw+6mWgJZIVQc4Qgmlmm/OB2b46syy0+nbr+nqwkWRLQKnsSUfaBm/mXjynUiA+s7hY+6\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: Wed, 22 Feb 2012 18:44:21 -0000\r
+\r
+On Mon, 13 Feb 2012 14:51:17 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:\r
+> Hi Pieter.\r
+> \r
+> On Wed, 25 Jan 2012 06:25:39 +0100, Pieter Praet <pieter@praet.org> wrote:\r
+> > * emacs/notmuch-show.el (notmuch-show-open-or-close-all):\r
+> > Rename to `notmuch-show-toggle-all-messages', and make it toggle\r
+> > visibility of all messages based on the visibility of the current\r
+> > message, instead of setting visibility based on whether or not a\r
+> > prefix arg was supplied.\r
+> > \r
+> > Same functionality, less effort (reaching for 'C-u' is a pain)...\r
+> > \r
+> > ---\r
+> > emacs/notmuch-show.el | 22 ++++++++++++----------\r
+> > 1 files changed, 12 insertions(+), 10 deletions(-)\r
+> > \r
+> > diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el\r
+> > index e6a5b31..2d17f74 100644\r
+> > --- a/emacs/notmuch-show.el\r
+> > +++ b/emacs/notmuch-show.el\r
+> > @@ -1050,8 +1050,8 @@ thread id. If a prefix is given, crypto processing is toggled."\r
+> > (define-key map "p" 'notmuch-show-previous-open-message)\r
+> > (define-key map (kbd "DEL") 'notmuch-show-rewind)\r
+> > (define-key map " " 'notmuch-show-advance-and-archive)\r
+> > - (define-key map (kbd "M-RET") 'notmuch-show-open-or-close-all)\r
+> > (define-key map (kbd "RET") 'notmuch-show-toggle-message)\r
+> > + (define-key map (kbd "M-RET") 'notmuch-show-toggle-all-messages)\r
+> \r
+> Should the function name include "visible" or "visibility" to make it\r
+> clear what is toggled? E.g. `notmuch-show-toggle-visibility-all'.\r
+> \r
+> Also, consider changing "all-messages" to just "all" or "thread". That\r
+> seems to be more consistent with other functions.\r
+>\r
+\r
+Good point, but we also have `notmuch-show-toggle-message' and\r
+`notmuch-show-toggle-headers', so `notmuch-show-toggle-visibility-all'\r
+would imply that both messages as well as headers are toggled.\r
+\r
+Also, `notmuch-show-toggle-visibility-thread' sounds to me like\r
+it would toggle the thread itself instead of the messages of\r
+which it is composed, so my personal expectation would be that\r
+it just blanks the entire buffer. (what's in a name....)\r
+\r
+How about renaming the relevant functions like so:\r
+- `notmuch-show-toggle-headers' -> `notmuch-show-toggle-visibility-headers'\r
+- `notmuch-show-toggle-message' -> `notmuch-show-toggle-visibility-message'\r
+- `notmuch-show-open-or-close-all' -> `notmuch-show-toggle-visibility-messages'\r
+\r
+> > (define-key map "#" 'notmuch-show-print-message)\r
+> > map)\r
+> > "Keymap for \"notmuch show\" buffers.")\r
+> > @@ -1502,16 +1502,18 @@ the result."\r
+> > (not (plist-get props :message-visible))))\r
+> > (force-window-update))\r
+> > \r
+> > -(defun notmuch-show-open-or-close-all ()\r
+> > - "Set the visibility all of the messages in the current thread.\r
+> > -By default make all of the messages visible. With a prefix\r
+> > -argument, hide all of the messages."\r
+> > +(defun notmuch-show-toggle-all-messages ()\r
+> > + "Toggle the visibility of all messages in the current thread.\r
+> > +If the current message is visible, hide all messages -- and vice versa."\r
+> > (interactive)\r
+> > - (save-excursion\r
+> > - (goto-char (point-min))\r
+> > - (loop do (notmuch-show-message-visible (notmuch-show-get-message-properties)\r
+> > - (not current-prefix-arg))\r
+> > - until (not (notmuch-show-goto-message-next))))\r
+> > + (let ((toggle (notmuch-show-message-visible-p)))\r
+> \r
+> Please rename "toggle" to "visible-p". That would make it more clear\r
+> what the variable means, and is consistent with\r
+> `notmuch-show-message-visible-p'.\r
+>\r
+\r
+AFAIK the '-p' suffix is "reserved" for predicate functions, and\r
+using it for a variable could be confusing. But I'm not aware of\r
+any guidelines on indicating the variable type except when it\r
+stores one or more functions [1,2]...\r
+\r
+Perhaps we could call it `visible-bool' ?\r
+\r
+Anyways, I've gone with your suggestion: `visible-p' it is...\r
+\r
+> > + (save-excursion\r
+> > + (goto-char (point-min))\r
+> > + (loop do (notmuch-show-message-visible\r
+> > + (notmuch-show-get-message-properties)\r
+> > + (not toggle))\r
+> > + until (not (notmuch-show-goto-message-next)))))\r
+> \r
+> A new `notmuch-show-mapc' function was introduced in a recent commit.\r
+> Please use it here instead of a custom loop.\r
+>\r
+\r
+Nice!\r
+\r
+> > + (recenter-top-bottom 1)\r
+> \r
+> There was no `recenter-top-bottom' call before. Why is it needed now?\r
+> It seems like an independent change and, if it is needed, would be\r
+> better as a separate patch. At the very least, it's worth mentioning in\r
+> the preamble and perhaps in a comment.\r
+>\r
+\r
+It ensures that the message being uncollapsed is put properly in view\r
+(instead of starting somewhere in the middle of the buffer) whilst also\r
+making it obvious that/if/when there's previous messages in the thread\r
+(due to its argument being 1 instead of 0).\r
+\r
+I thought about using `notmuch-show-message-adjust' instead, but that\r
+obscures the fact that there's previous messages.\r
+\r
+As it's quite essential in making the function DTRT, I've opted to\r
+clarify it in a comment as well as the commit message instead of\r
+splitting it out into a separate patch.\r
+\r
+> Regards,\r
+> Dmitry\r
+> \r
+> > (force-window-update))\r
+> > \r
+> > (defun notmuch-show-next-button ()\r
+> > -- \r
+> > 1.7.8.1\r
+> > \r
+> > _______________________________________________\r
+> > notmuch mailing list\r
+> > notmuch@notmuchmail.org\r
+> > http://notmuchmail.org/mailman/listinfo/notmuch\r
+\r
+So... I've expanded the test suite to cover everything I might be\r
+breaking, renamed the toggle functions to be consistent, and addressed\r
+all your comments in some way or another. I've also thrown in a bonus\r
+patch which is *not* meant to be applied (WIP, should eventually provide\r
+functionality similar to `notmuch-search-filter{,-by-tag}').\r
+\r
+Patches follow.\r
+\r
+\r
+Peace\r
+\r
+-- \r
+Pieter\r
+\r
+[1] http://www.gnu.org/software/emacs/manual/html_node/elisp/Coding-Conventions.html\r
+[2] http://www.gnu.org/software/emacs/manual/html_node/elisp/Hooks.html\r
projects / notmuch-archives.git / commitdiff