On Mon, 11 Jul 2011 09:32:45 -0700, Jameson Graef Rollins <jrollins@finestructure.net> wrote:

> On Mon, 11 Jul 2011 10:42:04 +0200, Felix Geller <fgeller@gmail.com> wrote:
> > I added a variable to toggle message indentation in Emacs.
> 
> Hi, Felix.  Thanks for submitting this patch.  I think it's a good idea.
> I have a couple of comments below, a couple of which echo what Dmitry
> has already pointed out.
> 
> > diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> 
> This patch doesn't include a commit log, which is something we generally
> require.  The preferred way to send patches is with git format-patch or
> send-email, both of which format patches in such a way that they can be
> immediately applied to a git repo, including with the commit log.

Ok, tried again :) 

I attached two commits. One that includes the changes (which have most
comments incorporated, only that I stick to when rather than if) and one
that includes a test. I still can't run the tests myself, I attached an
excerpt of what errors come up for the emacs subset, so I haven't tested
the test itself :( However, it is a simple copy of an existing one, but
I use a let to change the value of the new variable and adapted one of
the existing expected outputs to lack indentation. I'd be grateful if
someone could test it...

Didn't know about format-patch, thanks :) 


Cheers,
Felix


>
> 
> > +(defcustom notmuch-show-indent-messages-in-thread nil
> > +  "Should the messages in a thread be indented according to their respective depth in the thread?"
> > +  :group 'notmuch
> > +  :type 'boolean)
> 
> I agres with Dmitry that this should default to 't', to be consistent
> with the current default behavior.
> 
> > -    (insert (notmuch-show-spaces-n depth)
> > +    (insert (if notmuch-show-indent-messages-in-thread
> > +		(notmuch-show-spaces-n depth)
> > +	      "")
> 
> I also agree with Dmitry's suggestion here to use the following slightly
> simpler construct:
> 
>  (if notmuch-show-indent-messages-in-thread
>      (insert (notmuch-show-spaces-n depth)))
> 
> Finally, as Dmitry also points out, you'll almost certainly need to
> construct a test for this feature, since it constitutes a pretty big
> formatting change.  It should probably test for both cases of the
> customization variable.  Check out the tests in tests/emacs for
> guidance.
> 
> hth.
> 
> jamie.
Non-text part: application/pgp-signature