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