DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma;
	h=mime-version:sender:in-reply-to:references:date
	:x-google-sender-auth:message-id:subject:from:to:cc:content-type;
	b=HH9kCuE/rNTBVy3hUy1SywVh3gLBkpXQCo4DMr5dT+Clyxn7oowUypUxH3ON+5/4xf
	CVOfZO6DXGCkl1zXQqizoChzlzzv4fwI0cRje5ym9mCh7gmqqYvtnRi/D9OI8Hky10+J
	X2pyBigRIyZY5aaFhtcI/y7EFlg4PEAk+692w=
MIME-Version: 1.0
Sender: amdragon@gmail.com
In-Reply-To: <871uzmwtng.fsf@gilead.invalid>
References: <87fwoath2s.fsf@gilead.home.box>
	<BANLkTinKS5LbQsZuK8HyCzj+Lo4xqa2Sfg@mail.gmail.com>
	<871uznqeox.fsf@tredergarh.home.box>
	<BANLkTin0+O=mubHF7JNpw4u3DQDQJQ5zbw@mail.gmail.com>
	<874o4iwwp5.fsf@gilead.invalid> <871uzmwtng.fsf@gilead.invalid>
Date: Wed, 25 May 2011 15:11:16 -0400
Message-ID: <BANLkTikAB0mogUuS-nT_-i51LBdUBHCz3g@mail.gmail.com>
Subject: Re: [PATCH] emacs: Make the queries used in the all-tags section
From: Austin Clements <amdragon@mit.edu>
To: Daniel Schoepe <daniel.schoepe@googlemail.com>
Content-Type: text/plain; charset=ISO-8859-1
Cc: notmuch@notmuchmail.org
Precedence: list

On Wed, May 25, 2011 at 1:56 PM, Daniel Schoepe
<daniel.schoepe@googlemail.com> wrote:
> I accidentally used `filter' in the previous patch which isn't defined
> by default during runtime, updated version in the attachment.

Cool.  My inner parser is happy.

A few comments on the code:

> +(defcustom notmuch-hello-tag-list-make-query nil
> +  "Function or string to generate queries for the all tags list.
> +
> +This variable controls which query results are shown for each tag
> +in the \"all tags\" list. It can be nil (for default behaviour,
> +displaying all messages for a tag), a string that is used as a
> +filter for messages having that tag (equivalent to \"tag:TAG
> +and (THIS-VARIABLE)\", or a function that is given a tag and
> +should return the query that is to be used for the tag. If it
> +returns nil, the corresponding tag will be hidden."
> +  :type '(choice (const nil :tag "tag:TAG") string function)
> +  :group 'notmuch)

At least in Emacs 23.3.1, it has to be (const :tag "tag:TAG" nil).  I
didn't think the order mattered, but the tag didn't display otherwise.
 It would also be good to give descriptive tags to the other choices.

It would be more consistent if the function form of this also returned
a filter expression, rather than a whole expression.  This also
simplifies the documentation.

So, perhaps something like

(defcustom notmuch-hello-tag-list-counts nil
  "Method for generating counts displayed in the all tags list.

This variable controls the query used to generate counts for each
tag in the \"all tags\" list.  If nil, the tag list will count
all messages with each tag.  This can be a query string that will
filter the messages counted for each tag.  Finally, this can be a
function that will be called for each tag and should return a
filter query for that tag, or nil to hide the tag."
  :type '(choice (const :tag "Count all messages" nil)
                 (const :tag "Count unread messages" "tag:unread")
                 (const :tag "Custom filter" string)
                 (const :tag "Custom filter function" function))
  :group 'notmuch)

> +(defun notmuch-hello-generate-tag-alist ()
> +  "Return an alist from tags to queries to display in the all-tags section."
> +  (notmuch-filter
> +   'cdr
> +   (mapcar '(lambda (tag)

You don't need the quote before a lambda form.  (You can also change
'cdr to #'cdr to further hint the compiler, though it appears to not
matter in this case.)

> +(defun notmuch-filter (pred lst)
> +  "Return a list containing all elements from LST that satisfy PRED."

notmuch-remove-if-not would be more canonical (yeah, it's unwieldy,
blame Common Lisp).  Also, since Elisp doesn't do tail-recursion, the
standard way to define a remove-if-not function is

(defun notmuch-remove-if-not (predicate list)
  "Return a copy of LIST with all items not satisfying PREDICATE removed."
  (let (out)
    (while list
      (when (funcall predicate (car list))
        (push (car list) out))
      (setq list (cdr list)))
    (nreverse out)))

(Why oh why Elisp hasn't just made remove-if and remove-if-not
standard I don't know; they're redefined all over the Elisp code base.
 Any everybody's afraid to use cl-seq's remove-if-not because it's so
ridiculously complicated.)