[PATCH 0/2] Replying to HTML-only emails

[PATCH 0/2] Replying to HTML-only emails

[Adam Wolfe Gordon]

Hi all,

My recent reply enhancements were originally intended to allow proper
quoting of HTML-only email in reply. While the final version was a big
improvement on reply in general, it didn't actually acheive this goal.

So, this series finishes that work, using mm-display-part to render the
content of the original message correctly before inserting it into the reply
message. I've also added a test for this feature.

I'm not 100% happy with how the code looks for this, but I couldn't figure
out a way to make it work without the kind of ugly narrow-to-region. If
anyone has suggestions here I'd like to hear them.

Adam Wolfe Gordon (2):
  test: Replying to an HTML-only message in emacs
  emacs: Correctly quote non-text/plain parts in reply

 emacs/notmuch-mua.el |   20 +++++++++++++++-----
 test/emacs           |   26 ++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 5 deletions(-)

-- 
1.7.5.4

[PATCH 1/2] test: Replying to an HTML-only message in emacs

[Adam Wolfe Gordon]

With the latest reply infrastructure, we should be able to nicely
quote HTML-only emails. But currently emacs quotes the raw HTML
instead of parsing it first. This commit adds a test for this case.

This test currently marked as broken.
---
 test/emacs |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/test/emacs b/test/emacs
index c7510e9..e648f80 100755
--- a/test/emacs
+++ b/test/emacs
@@ -444,6 +444,33 @@ Alex Botero-Lowry <alex.boterolowry@gmail.com> writes:
 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 '()))
+	    (notmuch-show \"id:${gen_msg_id}\")
+	    (notmuch-show-reply)
+	    (test-output))"
+sed -i -e 's,^User-Agent: Notmuch/.* Emacs/.*,User-Agent: Notmuch/XXX Emacs/XXX,' OUTPUT
+cat <<EOF >EXPECTED
+From: Notmuch Test Suite <test_suite@notmuchmail.org>
+To: 
+Subject: Re: Reply within emacs to an html-only message
+In-Reply-To: <${gen_msg_id}>
+Fcc: ${MAIL_DIR}/sent
+References: <${gen_msg_id}>
+User-Agent: Notmuch/XXX Emacs/XXX
+--text follows this line--
+Notmuch Test Suite <test_suite@notmuchmail.org> writes:
+
+> Hi,
+> This is an HTML test message.
+>
+> OK?
+EOF
+test_expect_equal_file OUTPUT EXPECTED
+
 test_begin_subtest "Quote MML tags in reply"
 message_id='test-emacs-mml-quoting@message.id'
 add_message [id]="$message_id" \
-- 
1.7.5.4

[PATCH 2/2] emacs: Correctly quote non-text/plain parts in reply

[Adam Wolfe Gordon]

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

Re: [PATCH 2/2] emacs: Correctly quote non-text/plain parts in reply

[Mark Walters]

On Sun, 22 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.

My instinct would have been to do this with a temporary buffer rather
than the narrowing but I am *definitely* too much of a lisp beginner to
say that either is better.

(I think notmuch-show-mm-display-part-inline uses the temporary buffer
approach.)

Anyway, as it is, it looks correct and seems to work! (and is obviously
useful functionality)

Best wishes

Mark

> 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

Re: [PATCH 1/2] test: Replying to an HTML-only message in emacs

[Austin Clements]

Does the test output depend on the HTML renderer used?  (And should
there be a test dependency on w3m or something?  I must admit, I don't
know how mm's HTML rendering works.)

On Sat, 21 Apr 2012, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:
> With the latest reply infrastructure, we should be able to nicely
> quote HTML-only emails. But currently emacs quotes the raw HTML
> instead of parsing it first. This commit adds a test for this case.
>
> This test currently marked as broken.
> ---
>  test/emacs |   27 +++++++++++++++++++++++++++
>  1 files changed, 27 insertions(+), 0 deletions(-)
>
> diff --git a/test/emacs b/test/emacs
> index c7510e9..e648f80 100755
> --- a/test/emacs
> +++ b/test/emacs
> @@ -444,6 +444,33 @@ Alex Botero-Lowry <alex.boterolowry@gmail.com> writes:
>  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 '()))
> +	    (notmuch-show \"id:${gen_msg_id}\")
> +	    (notmuch-show-reply)
> +	    (test-output))"
> +sed -i -e 's,^User-Agent: Notmuch/.* Emacs/.*,User-Agent: Notmuch/XXX Emacs/XXX,' OUTPUT
> +cat <<EOF >EXPECTED
> +From: Notmuch Test Suite <test_suite@notmuchmail.org>
> +To: 
> +Subject: Re: Reply within emacs to an html-only message
> +In-Reply-To: <${gen_msg_id}>
> +Fcc: ${MAIL_DIR}/sent
> +References: <${gen_msg_id}>
> +User-Agent: Notmuch/XXX Emacs/XXX
> +--text follows this line--
> +Notmuch Test Suite <test_suite@notmuchmail.org> writes:
> +
> +> Hi,
> +> This is an HTML test message.
> +>
> +> OK?
> +EOF
> +test_expect_equal_file OUTPUT EXPECTED
> +
>  test_begin_subtest "Quote MML tags in reply"
>  message_id='test-emacs-mml-quoting@message.id'
>  add_message [id]="$message_id" \
> -- 
> 1.7.5.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 2/2] emacs: Correctly quote non-text/plain parts in reply

[Austin Clements]

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

Re: [PATCH 1/2] test: Replying to an HTML-only message in emacs

[Adam Wolfe Gordon]

On Fri, May 4, 2012 at 12:47 PM, Austin Clements <amdragon@mit.edu> wrote:
> Does the test output depend on the HTML renderer used?  (And should
> there be a test dependency on w3m or something?  I must admit, I don't
> know how mm's HTML rendering works.)

That's a good question. HTML output does depend on what renderer is
used, but I think the test message is simple enough that they'll all
produce the same output. I believe mm prefers w3m.el, and falls back
to using an external w3m binary, and then emacs's built-in HTML
parsing (I'm significantly less sure whether the last part is true).

I've tried removing w3m and w3m.el from my system, and the test still
passes, so I think we're OK.

-- Adam

Re: [PATCH 2/2] emacs: Correctly quote non-text/plain parts in reply

[Adam Wolfe Gordon]

On Fri, May 4, 2012 at 1:05 PM, Austin Clements <amdragon@mit.edu> wrote:
> 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?)

This almost works - patch forthcoming. The trouble with just using
notmuch-show-mm-display-part-inline is that it doesn't move the point
to the end of the text, which we rely on for message-cite-original to
quote the right region. But, I can use
notmuch-show-mm-display-part-inline (moved to the lib) by narrowing
first, and setting the point after using it. If we don't narrow, the
point ends up after the user's signature (which is inserted by message
mode before we insert the quoted text), and the signature ends up
being quoted.

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

Yeah, I agree with this. Maybe before 0.14 I will try to do some
refactoring to share code between reply and show, at least in emacs.

-- Adam