From: Mark Walters <markwalters1009@gmail.com>
To: Jani Nikula <jani@nikula.org>, notmuch@notmuchmail.org
Subject: Re: [PATCH] emacs: insert quotable parts in reply as they are
	displayed in show view
In-Reply-To: <1376901495-17349-1-git-send-email-jani@nikula.org>
References: <1376901495-17349-1-git-send-email-jani@nikula.org>
User-Agent: Notmuch/0.15.2+192~g8222af3 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Fri, 23 Aug 2013 15:50:28 +0100
Message-ID: <87ob8oh363.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


Hi

On Mon, 19 Aug 2013, Jani Nikula <jani@nikula.org> wrote:
> In reply, insert quotable parts using notmuch-show-insert-bodypart
> instead of calling notmuch-mm-display-part-inline directly to render
> the quoted parts as they are rendered in show view.
>
> The notable change is that replies to text/calendar parts quote the
> pretty printed output of icalendar-import-buffer rather than the ugly
> raw vcalendar.

In general I like this. 

I am slightly worried whether this changes replies to text/html message:
from code following I think it does not but I don't have enough
text/html messages to check this. (Note unless the user customises
notmuch-multipart/alternative-discouraged it definitely does not affect
multipart messages with a text/html alternative)

If someone who does have text/html messages could test that would be
very helpful.

Otherwise just a couple of nits in the code:

> ---
>  emacs/notmuch-mua.el  |    7 ++++---
>  emacs/notmuch-show.el |    9 ++++++---
>  2 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
> index 2baae5f..24b0d0f 100644
> --- a/emacs/notmuch-mua.el
> +++ b/emacs/notmuch-mua.el
> @@ -130,9 +130,10 @@ list."
>  (defun notmuch-mua-insert-quotable-part (message part)
>    (save-restriction
>      (narrow-to-region (point) (point))
> -    (notmuch-mm-display-part-inline message part (plist-get part :id)
> -				    (plist-get part :content-type)
> -				    notmuch-show-process-crypto)
> +    ;; We don't want hooks, such as notmuch-wash-*, to be run on the
> +    ;; quotable part.
> +    (let ((notmuch-show-insert-text/plain-hook nil))
> +      (notmuch-show-insert-bodypart message part 0 nil t))
>      (goto-char (point-max))))
>  
>  ;; There is a bug in emacs 23's message.el that results in a newline

notmuch-show-insert-bodypart might need to be declared in notmuch-mua.el
as I seem to get a compile warning.

> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 82b70ba..1fb48aa 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -874,7 +874,7 @@ message at DEPTH in the current thread."
>        ;; showable this returns nil.
>        (notmuch-show-create-part-overlays button part-beg part-end))))
>  
> -(defun notmuch-show-insert-bodypart (msg part depth &optional hide)
> +(defun notmuch-show-insert-bodypart (msg part depth &optional hide quote)
>    "Insert the body part PART at depth DEPTH in the current thread.
>  
>  If HIDE is non-nil then initially hide this part."
> @@ -887,8 +887,11 @@ If HIDE is non-nil then initially hide this part."
>  			content-type))
>  	 (nth (plist-get part :id))
>  	 (beg (point))
> -	 ;; We omit the part button for the first (or only) part if this is text/plain.
> -	 (button (unless (and (string= mime-type "text/plain") (<= nth 1))
> +	 ;; We omit the part button for the first (or only) part if
> +	 ;; this is text/plain, or we're inserting the part for
> +	 ;; quoting in reply.
> +	 (button (unless (or quote
> +			     (and (string= mime-type "text/plain") (<= nth 1)))


Since quote is the name of a lisp function I would prefer some other name
for the variable. It is correct code as is but my eye sort of slipped
over the initial "or" and assumed quote was the function.

Best wishes

Mark

>  		   (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename))))
>  	 (content-beg (point)))
>  
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch