From: Austin Clements <amdragon@MIT.EDU>
To: Adam Wolfe Gordon <awg+notmuch@xvx.ca>, notmuch@notmuchmail.org
Subject: Re: [PATCH 2/2] emacs: Correctly quote non-text/plain parts in reply
In-Reply-To: <1335056093-17621-3-git-send-email-awg+notmuch@xvx.ca>
References: <1335056093-17621-1-git-send-email-awg+notmuch@xvx.ca>
	<1335056093-17621-3-git-send-email-awg+notmuch@xvx.ca>
User-Agent: Notmuch/0.12+132~gf2f390b (http://notmuchmail.org) Emacs/23.3.1
	(i486-pc-linux-gnu)
Date: Fri, 04 May 2012 15:05:07 -0400
Message-ID: <87obq3ahss.fsf@awakening.csail.mit.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

Personally I think the narrowing trick is clever, but I worry that it
assumes too much about how mm-display-part works, since mm-display-part
just takes a buffer in the handle.  Is there a reason this doesn't
simply use notmuch-show-mm-display-part-inline?  Something like
(untested)

(defun notmuch-mua-insert-quotable-part (message part)
  (notmuch-show-mm-display-part-inline 
   message part (plist-get part :id) (plist-get part :content-type)))

You might not even need notmuch-mua-insert-quotable-part.  (Why does
notmuch-show-mm-display-part-inline take all of those redundant
arguments?)

If there's a reason that doesn't work, I still think it would be better
to use a temporary buffer; something like (untested)

(defun notmuch-mua-insert-quotable-part (message part)
  (let ((orig-buffer (current-buffer)))
    (notmuch-with-temp-part-buffer message (plist-get part :id)
      (let ((handle ...))
        (with-current-buffer orig-buffer
           (mm-display-part handle))))))

In general, I feel like the reply code should share more structure with
the notmuch-show code.  I worry that the quoted text people wind up with
may not resemble the text they saw in the show buffer because the two
code paths use different rules.  But addressing that (if it's
addressable) should be done in a later series.

On Sat, 21 Apr 2012, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:
> Quote non-text parts nicely by displaying them with mm-display-part
> before calling message-cite-original to quote them. HTML-only emails
> can now be quoted correctly.
>
> Mark the test for this feature as not broken.
> ---
>  emacs/notmuch-mua.el |   20 +++++++++++++++-----
>  test/emacs           |    1 -
>  2 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
> index 87bd88d..f7af789 100644
> --- a/emacs/notmuch-mua.el
> +++ b/emacs/notmuch-mua.el
> @@ -21,6 +21,7 @@
>  
>  (require 'json)
>  (require 'message)
> +(require 'mm-view)
>  (require 'format-spec)
>  
>  (require 'notmuch-lib)
> @@ -90,6 +91,19 @@ list."
>  	else if (notmuch-match-content-type (plist-get part :content-type) "text/*")
>  	  collect part))
>  
> +(defun notmuch-mua-insert-quotable-part (message part)
> +  (save-restriction
> +    (narrow-to-region (point) (point))
> +    (insert (notmuch-get-bodypart-content message part
> +					  (plist-get part :id)
> +					  notmuch-show-process-crypto))
> +    (let ((handle (mm-make-handle (current-buffer)
> +				  (list (plist-get part :content-type))))
> +	  (end-of-orig (point-max)))
> +      (mm-display-part handle)
> +      (kill-region (point-min) end-of-orig))
> +    (goto-char (point-max))))
> +
>  ;; There is a bug in emacs 23's message.el that results in a newline
>  ;; not being inserted after the References header, so the next header
>  ;; is concatenated to the end of it. This function fixes the problem,
> @@ -169,11 +183,7 @@ list."
>  	;; Get the parts of the original message that should be quoted; this includes
>  	;; all the text parts, except the non-preferred ones in a multipart/alternative.
>  	(let ((quotable-parts (notmuch-mua-get-quotable-parts (plist-get original :body))))
> -	  (mapc (lambda (part)
> -		  (insert (notmuch-get-bodypart-content original part
> -							(plist-get part :id)
> -							notmuch-show-process-crypto)))
> -		quotable-parts))
> +	  (mapc (apply-partially 'notmuch-mua-insert-quotable-part original) quotable-parts))
>  
>  	(set-mark (point))
>  	(goto-char start)
> diff --git a/test/emacs b/test/emacs
> index e648f80..579844f 100755
> --- a/test/emacs
> +++ b/test/emacs
> @@ -445,7 +445,6 @@ EOF
>  test_expect_equal_file OUTPUT EXPECTED
>  
>  test_begin_subtest "Reply within emacs to an html-only message"
> -test_subtest_known_broken
>  add_message '[content-type]="text/html"' \
>  	    '[body]="Hi,<br />This is an <b>HTML</b> test message.<br /><br />OK?"'
>  test_emacs "(let ((message-hidden-headers '()))
> -- 
> 1.7.5.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch