From: Jameson Graef Rollins <jrollins@finestructure.net>
To: Felix Geller <fgeller@gmail.com>, notmuch@notmuchmail.org
Subject: Re: [PATCH] Emacs: Add variable to toggle message indentation in a
	thread
In-Reply-To: <m2r55xi4yr.fsf@gmail.com>
References: <m2r55xi4yr.fsf@gmail.com>
User-Agent: Notmuch/0.5-353-gb44d56e (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Mon, 11 Jul 2011 09:32:45 -0700
Message-ID: <87aackwzf6.fsf@servo.factory.finestructure.net>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
	micalg=pgp-sha256; protocol="application/pgp-signature"
Precedence: list

--=-=-=

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.

--=-=-=
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iQIcBAEBCAAGBQJOGyWtAAoJEO00zqvie6q8oWgP/0EThDHhwlBdBeDQSY2jzILY
GKUXROMAv20qQFjgzho3rDznz09OpVKPPTbpZuqbF3PuLMhwwDCWbOYEddZm+3ph
r3P+L7Xi6esx2TrUrTL7pgrGYGlMX3xrLZLjc2E7a+GY9UO1NODliiXCff93G311
1kzDZhPV6s8MjvvsIvD9aH4bbjV3iz+7fkMMcZV1qHXUQskik/TLETUKuJx1bmRz
fMrX7fssALCpCNIZFgPdkrhOvU4DUes5AjukWWompE6AwRSdDE9WV5O5P8xSWCb4
M4Ptgo8kMEj2IHNvcf1MJtvZgybt2+oX82CxFLQnBvVGj3H/HqXK+ZrTMW8UlwTu
5Q2LYELNSv1CilNXye43f/b93dlYLmUwM9RO11i1Gd1e97Nu2Taz7SDD3/M/NAx4
+IcumUgDSvx/u+4QDggE9th0ftQ1+KLaoCjukEbl1VYzn6orisJ4GamnmveuRpjK
B4uV9KheIVbRABN/ExxiTRQLp0aHEsNIdeHTQu2p8E+APiyqey3NkDJeKoRk6xqy
MCEpDy1ReRu74BkfBjAe0UNyvJZmhDk4ni/vGJ/r5Mt8cMYd6iMQpRp4EgguBCdg
SpP7HamxSkMpn/WToIv0/o5EVOoRIL10J6WK1Q+ZGTK0tsCgqZqgrCro8eWeJEfs
ujSVxTYHWHmzvd+tcJRe
=KHj4
-----END PGP SIGNATURE-----
--=-=-=--