From: Pieter Praet Date: Wed, 22 Feb 2012 18:41:58 +0000 (+0100) Subject: Re: [PATCH] emacs: make `notmuch-show-open-or-close-all' toggle visibility X-Git-Url: http://git.tremily.us/gitweb.cgi?a=commitdiff_plain;h=7bd297ec74d14d2ef4ce9bc3f8f8b7024e28faf2;p=notmuch-archives.git Re: [PATCH] emacs: make `notmuch-show-open-or-close-all' toggle visibility --- diff --git a/44/2a251d872b2622f4fbfd50fa280f64a71921e7 b/44/2a251d872b2622f4fbfd50fa280f64a71921e7 new file mode 100644 index 000000000..dcc3033bd --- /dev/null +++ b/44/2a251d872b2622f4fbfd50fa280f64a71921e7 @@ -0,0 +1,216 @@ +Return-Path: +X-Original-To: notmuch@notmuchmail.org +Delivered-To: notmuch@notmuchmail.org +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