From: Pieter Praet <pieter@praet.org>
To: Austin Clements <amdragon@MIT.EDU>
Subject: Re: [PATCH] emacs: logically group def{custom,face}s
In-Reply-To: <20120114180828.GD1801@mit.edu>
References: <87ty3ypsgp.fsf@praet.org>
	<1326531898-17356-1-git-send-email-pieter@praet.org>
	<20120114180828.GD1801@mit.edu>
User-Agent: Notmuch/0.11+78~g6c58370 (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-unknown-linux-gnu)
Date: Mon, 16 Jan 2012 11:34:53 +0100
Message-ID: <871ur02awi.fsf@praet.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Notmuch Mail <notmuch@notmuchmail.org>
Precedence: list

On Sat, 14 Jan 2012 13:08:28 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> Quoth Pieter Praet on Jan 14 at 10:04 am:
> > To allow for expansion whilst keeping everything tidy and organized,
> > move all defcustom/defface variables to the following subgroups,
> > defined in notmuch-lib.el:
> > 
> > - Hello
> > - Search
> > - Show
> > - Send
> > - Crypto
> > - Hooks
> > - External Commands
> > - Appearance
> > 
> > As an added benefit, defcustom keyword args are now consistently
> > in order of appearance @ defcustom's docstring (OCD much?).
> 
> Thanks for doing this. [...]

You're very welcome!

> [...] I recently went into customize-group notmuch
> and was overwhelmed by the pile of options presented.
> 

Same here :)

> > diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> > index 0f856bf..f6f48e9 100644
> > --- a/emacs/notmuch-lib.el
> > +++ b/emacs/notmuch-lib.el
> > @@ -28,17 +28,58 @@
> >    "Notmuch mail reader for Emacs."
> >    :group 'mail)
> >  
> 
> Group docstrings aren't generally of the form "Options concerning
> ..."; they just jump into what they concern.  E.g.,
> 
> Convenience : Convenience features for faster editing.
> 
> Calendar Hooks : Calendar hooks.
> 

Couldn't agree more!  I must admit, I was rather apathetic at the
time due to a lack of sleep.

> Also, all but one of the tags you give the groups would be
> automatically derived by Emacs, so you can remove those.
> 

How very convenient.  Removed 'em all: the one @ `notmuch-external'
was pretty pointless as well.

> > +(defgroup notmuch-hello nil
> > +  "Options concerning `notmuch-hello-mode'."
> > +  :tag "Notmuch Hello"
> > +  :group 'notmuch)
> 
> Perhaps "Overview of saved searches, tags, etc."
> 
> > +
> > +(defgroup notmuch-search nil
> > +  "Options concerning `notmuch-search-mode'."
> > +  :tag "Notmuch Search"
> > +  :group 'notmuch)
> 
> "Searching and sorting mail"?
> 
> > +
> > +(defgroup notmuch-show nil
> > +  "Options concerning `notmuch-show-mode'."
> > +  :tag "Notmuch Show"
> > +  :group 'notmuch)
> 
> "Showing messages and threads"?
> 
> > +
> > +(defgroup notmuch-send nil
> > +  "Options concerning the sending of messages."
> > +  :tag "Notmuch Send"
> > +  :group 'notmuch)
> 
> "Sending messages from Notmuch"?
> 
> We should probably link to the 'message group, perhaps by adding
>   :link '(custom-group-link message)
> here or maybe to the notmuch group itself.  Unfortunately, I don't
> think you can actually add a group to another group after it's been
> defined (though I could be wrong).
> 

Agreed.  I've added `notmuch-send' to the `message' group.

> > +
> > +(defgroup notmuch-crypto nil
> > +  "Options concerning the processing and fontification of
> > +cryptographic MIME parts in `notmuch-show-mode'."
> > +  :tag "Notmuch Crypto"
> > +  :group 'notmuch)
> 
> "Processing and display of cryptographic MIME parts"?  (You also don't
> want the docstring to be too long, given how it's displayed.)
> 
> > +
> > +(defgroup notmuch-hooks nil
> > +  "Run custom code on well-defined occasions."
> > +  :tag "Notmuch Hooks"
> > +  :group 'notmuch)
> > +
> > +(defgroup notmuch-external nil
> > +  "Run more custom code on different well-defined occasions."
> > +  :tag "Notmuch External Commands"
> > +  :group 'notmuch)
> 
> Oof!  It's okay to be a little redundant in the docstring.  Core Emacs
> options do it.  "External commands"?
> 
> > +
> > +(defgroup notmuch-appearance nil
> > +  "Options concerning how Notmuch looks."
> > +  :tag "Notmuch Appearance"
> > +  :group 'notmuch)
> 
> "How Notmuch looks"?
> 
> I worry that notmuch-appearance is a catch-all that most options
> arguably fit in to.  In particular, some notmuch-show options are also
> in this group and some aren't and it's not clear to me what the rule
> is.
> 
> Perhaps this should be notmuch-faces and limited to just faces (and
> maybe options that aren't technically faces but that affect face
> selection)?  Then the grouping rule would be obvious, like it is for
> all of the other groups.

That was my original intention, but due to `notmuch-hello-logo-background',
I decided to dump `notmuch-show-logo' in there as well, necessitating a
broader designation.  Fixed!

Patch follows.


Peace

-- 
Pieter