Received-SPF: pass (google.com: domain of pieter@praet.org designates
	10.180.92.227 as permitted sender) client-ip=10.180.92.227; 
From: Pieter Praet <pieter@praet.org>
To: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>,
	Notmuch Mail <notmuch@notmuchmail.org>
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
Precedence: list

On Mon, 13 Feb 2012 14:51:17 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> Hi Pieter.
> 
> On Wed, 25 Jan 2012 06:25:39 +0100, Pieter Praet <pieter@praet.org> 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