Re: [PATCH] Emacs: Add variable to toggle message indentation in a thread

[Jameson Graef Rollins <jrollins@finestructure.net>]

[-- Attachment #1: Type: text/plain, Size: 1588 bytes --]

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.

> +(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.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]