From: Ethan Glasser-Camp <ethan.glasser.camp@gmail.com>
To: David Edmondson <dme@dme.org>, notmuch@notmuchmail.org
Subject: Re: [PATCH v2] emacs: Add more processing of displayed headers.
In-Reply-To: <1328542748-19530-2-git-send-email-dme@dme.org>
References: <1327052612-1040-1-git-send-email-dme@dme.org>
	<1328542748-19530-1-git-send-email-dme@dme.org>
	<1328542748-19530-2-git-send-email-dme@dme.org>
User-Agent: Notmuch/0.14+45~g6ea9330 (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Fri, 12 Oct 2012 15:11:11 -0400
Message-ID: <87sj9ja4kw.fsf@betacantrips.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

Hi! Just going through the patch queue.

This is definitely a nice effect, but I'm not sure of the approach. It
doesn't indent the message's tags, and it doesn't work when you resize the
window. (You can get some very ugly wrapping if you put your mind to
it.)

Is there no better way to do this using visual-line-mode? I know that
the rest of notmuch uses hard filling, but that's no reason to make a
bad situation worse. It looks like you can put wrap-prefix text
properties all over, as done in the adaptive-wrap package:

http://elpa.gnu.org/packages/adaptive-wrap-0.1.el

Slightly more nit-picky comments below.

David Edmondson <dme@dme.org> writes:

> -(defvar notmuch-show-markup-headers-hook '(notmuch-show-colour-headers)
> +(defcustom notmuch-show-markup-headers-hook '(notmuch-show-colour-headers
> +					      notmuch-show-fill-headers
> +					      notmuch-show-indent-continuations)
>    "A list of functions called to decorate the headers listed in
> -`notmuch-message-headers'.")
> +`notmuch-message-headers'."
> +  :type 'hook
> +  :options '(notmuch-show-colour-headers
> +	     notmuch-show-fill-headers
> +	     notmuch-show-indent-continuations)
> +  :group 'notmuch-show)

This hook is not normal because it takes an argument, and so should have
a name ending in -hooks or -functions. Also, since it's a defcustom now,
it should probably have a better explanation of how it works, that it
takes an argument, and what that argument means.

It seems extremely dicey to me that you can put
notmuch-show-indent-continuations in this list before, or without,
notmuch-show-fill-headers.

> +(defun notmuch-show-fill-headers (depth)
> +  "Wrap the text of the current headers."
> +
> +  ;; '-5' to allow for the indentation code.
> +  (let ((fill-column (- (window-width) depth 5)))

It took me a little while to figure out what this meant. How about,
"underfill by 5 so that inserting indentation doesn't cause more
wrapping"? Is it possible to be smart enough to let

> +(defun notmuch-show-indent-continuations (depth)
> +  "Indent any continuation lines."
> +  (goto-char (point-min))
> +  (while (not (eobp))
> +    (if (not (looking-at "^[A-Za-z][-A-Za-z0-9]*:"))
> +	;; Four spaces tends to work well with 'To' and 'Cc' headers.
> +	(insert "    "))
> +    (forward-line)))

I'm not crazy about this but I'm not sure I can say why exactly. Why
can't we just run indent-rigidly over the whole thing?

The comment isn't terribly useful. Only those headers? "Tends to work
well"?

>      ;; Override `notmuch-message-headers' to force `From' to be
>      ;; displayed.
>      (let ((notmuch-message-headers '("From" "Subject" "To" "Cc" "Date")))
> -      (notmuch-show-insert-headers (plist-get message :headers)))
> +      (notmuch-show-insert-headers (plist-get message :headers) 0))

This took me a long while to figure out, especially because it looks
like the depth is being passed normally to notmuch-show-insert-bodypart,
just below. A comment like "depth = 0 because we reindent below" would
have been really helpful. A good test message is
id:"87ocabvp0y.fsf@wsheee.2x.cz".

Ethan