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

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

[Adam Wolfe Gordon]

Hi everyone,

Here is a new version of my HTML reply series [1], addressing Autin's review
comments. The reply code now shares more with the similar notmuch-show code,
which is a good thing.

Reviews welcome, but I'm hoping this can be in 0.13.

-- Adam

[1] id:"1335056093-17621-1-git-send-email-awg+notmuch@xvx.ca"

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

 emacs/notmuch-lib.el  |   19 +++++++++++++++++++
 emacs/notmuch-mua.el  |   13 ++++++++-----
 emacs/notmuch-show.el |   19 +------------------
 test/emacs            |   26 ++++++++++++++++++++++++++
 4 files changed, 54 insertions(+), 23 deletions(-)

-- 
1.7.5.4

[PATCH v2 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 38decd0..5f238d9 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 v2 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. We re-use some code from notmuch-show
(notmuch-show-mm-display-part-inline), which has been moved to
notmuch-lib.el.

Mark the test for this feature as not broken.
---
 emacs/notmuch-lib.el  |   19 +++++++++++++++++++
 emacs/notmuch-mua.el  |   13 ++++++++-----
 emacs/notmuch-show.el |   19 +------------------
 test/emacs            |    1 -
 4 files changed, 28 insertions(+), 24 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 6907a5f..7fa441a 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -21,6 +21,8 @@
 
 ;; This is an part of an emacs-based interface to the notmuch mail system.
 
+(require 'mm-view)
+(require 'mm-decode)
 (eval-when-compile (require 'cl))
 
 (defvar notmuch-command "notmuch"
@@ -237,6 +239,23 @@ the given type."
   (or (plist-get part :content)
       (notmuch-get-bodypart-internal (notmuch-id-to-query (plist-get msg :id)) nth process-crypto)))
 
+(defun notmuch-mm-display-part-inline (msg part nth content-type process-crypto)
+  "Use the mm-decode/mm-view functions to display a part in the
+current buffer, if possible."
+  (let ((display-buffer (current-buffer)))
+    (with-temp-buffer
+      (let* ((charset (plist-get part :content-charset))
+	     (handle (mm-make-handle (current-buffer) `(,content-type (charset . ,charset)))))
+	;; If the user wants the part inlined, insert the content and
+	;; test whether we are able to inline it (which includes both
+	;; capability and suitability tests).
+	(when (mm-inlined-p handle)
+	  (insert (notmuch-get-bodypart-content msg part nth process-crypto))
+	  (when (mm-inlinable-p handle)
+	    (set-buffer display-buffer)
+	    (mm-display-part handle)
+	    t))))))
+
 ;; Converts a plist of headers to an alist of headers. The input plist should
 ;; have symbols of the form :Header as keys, and the resulting alist will have
 ;; symbols of the form 'Header as keys.
diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
index 87bd88d..88d7505 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,12 @@ list."
 	else if (notmuch-match-content-type (plist-get part :content-type) "text/*")
 	  collect part))
 
+(defun notmuch-mua-insert-quotable-part (message part)
+  (notmuch-mm-display-part-inline message part (plist-get part :id)
+				  (plist-get part :content-type)
+				  notmuch-show-process-crypto)
+  (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 +176,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/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 37f0ebb..d318430 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -524,23 +524,6 @@ message at DEPTH in the current thread."
     (let ((handle (mm-make-handle (current-buffer) (list content-type))))
       (mm-interactively-view-part handle))))
 
-(defun notmuch-show-mm-display-part-inline (msg part nth content-type)
-  "Use the mm-decode/mm-view functions to display a part in the
-current buffer, if possible."
-  (let ((display-buffer (current-buffer)))
-    (with-temp-buffer
-      (let* ((charset (plist-get part :content-charset))
-	     (handle (mm-make-handle (current-buffer) `(,content-type (charset . ,charset)))))
-	;; If the user wants the part inlined, insert the content and
-	;; test whether we are able to inline it (which includes both
-	;; capability and suitability tests).
-	(when (mm-inlined-p handle)
-	  (insert (notmuch-get-bodypart-content msg part nth notmuch-show-process-crypto))
-	  (when (mm-inlinable-p handle)
-	    (set-buffer display-buffer)
-	    (mm-display-part handle)
-	    t))))))
-
 (defun notmuch-show-multipart/*-to-list (part)
   (mapcar (lambda (inner-part) (plist-get inner-part :content-type))
 	  (plist-get part :content)))
@@ -785,7 +768,7 @@ current buffer, if possible."
 (defun notmuch-show-insert-part-*/* (msg part content-type nth depth declared-type)
   ;; This handler _must_ succeed - it is the handler of last resort.
   (notmuch-show-insert-part-header nth content-type declared-type (plist-get part :filename))
-  (notmuch-show-mm-display-part-inline msg part nth content-type)
+  (notmuch-mm-display-part-inline msg part nth content-type notmuch-show-process-crypto)
   t)
 
 ;; Functions for determining how to handle MIME parts.
diff --git a/test/emacs b/test/emacs
index 5f238d9..a615b39 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

[PATCH v2.1 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. We re-use some code from notmuch-show
(notmuch-show-mm-display-part-inline), which has been moved to
notmuch-lib.el.

Mark the test for this feature as not broken.
---
I had removed the narrowing from this patch to check whether it was
actually necessary (it is), and forgot to put it back in. Here is 
a corrected patch.

 emacs/notmuch-lib.el  |   19 +++++++++++++++++++
 emacs/notmuch-mua.el  |   15 ++++++++++-----
 emacs/notmuch-show.el |   19 +------------------
 test/emacs            |    1 -
 4 files changed, 30 insertions(+), 24 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 6907a5f..7fa441a 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -21,6 +21,8 @@
 
 ;; This is an part of an emacs-based interface to the notmuch mail system.
 
+(require 'mm-view)
+(require 'mm-decode)
 (eval-when-compile (require 'cl))
 
 (defvar notmuch-command "notmuch"
@@ -237,6 +239,23 @@ the given type."
   (or (plist-get part :content)
       (notmuch-get-bodypart-internal (notmuch-id-to-query (plist-get msg :id)) nth process-crypto)))
 
+(defun notmuch-mm-display-part-inline (msg part nth content-type process-crypto)
+  "Use the mm-decode/mm-view functions to display a part in the
+current buffer, if possible."
+  (let ((display-buffer (current-buffer)))
+    (with-temp-buffer
+      (let* ((charset (plist-get part :content-charset))
+	     (handle (mm-make-handle (current-buffer) `(,content-type (charset . ,charset)))))
+	;; If the user wants the part inlined, insert the content and
+	;; test whether we are able to inline it (which includes both
+	;; capability and suitability tests).
+	(when (mm-inlined-p handle)
+	  (insert (notmuch-get-bodypart-content msg part nth process-crypto))
+	  (when (mm-inlinable-p handle)
+	    (set-buffer display-buffer)
+	    (mm-display-part handle)
+	    t))))))
+
 ;; Converts a plist of headers to an alist of headers. The input plist should
 ;; have symbols of the form :Header as keys, and the resulting alist will have
 ;; symbols of the form 'Header as keys.
diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
index 87bd88d..fc7ae07 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,14 @@ 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))
+    (notmuch-mm-display-part-inline message part (plist-get part :id)
+				    (plist-get part :content-type)
+				    notmuch-show-process-crypto)
+    (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 +178,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/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 37f0ebb..d318430 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -524,23 +524,6 @@ message at DEPTH in the current thread."
     (let ((handle (mm-make-handle (current-buffer) (list content-type))))
       (mm-interactively-view-part handle))))
 
-(defun notmuch-show-mm-display-part-inline (msg part nth content-type)
-  "Use the mm-decode/mm-view functions to display a part in the
-current buffer, if possible."
-  (let ((display-buffer (current-buffer)))
-    (with-temp-buffer
-      (let* ((charset (plist-get part :content-charset))
-	     (handle (mm-make-handle (current-buffer) `(,content-type (charset . ,charset)))))
-	;; If the user wants the part inlined, insert the content and
-	;; test whether we are able to inline it (which includes both
-	;; capability and suitability tests).
-	(when (mm-inlined-p handle)
-	  (insert (notmuch-get-bodypart-content msg part nth notmuch-show-process-crypto))
-	  (when (mm-inlinable-p handle)
-	    (set-buffer display-buffer)
-	    (mm-display-part handle)
-	    t))))))
-
 (defun notmuch-show-multipart/*-to-list (part)
   (mapcar (lambda (inner-part) (plist-get inner-part :content-type))
 	  (plist-get part :content)))
@@ -785,7 +768,7 @@ current buffer, if possible."
 (defun notmuch-show-insert-part-*/* (msg part content-type nth depth declared-type)
   ;; This handler _must_ succeed - it is the handler of last resort.
   (notmuch-show-insert-part-header nth content-type declared-type (plist-get part :filename))
-  (notmuch-show-mm-display-part-inline msg part nth content-type)
+  (notmuch-mm-display-part-inline msg part nth content-type notmuch-show-process-crypto)
   t)
 
 ;; Functions for determining how to handle MIME parts.
diff --git a/test/emacs b/test/emacs
index 5f238d9..a615b39 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 v2 0/2] Replying to HTML-only emails, v2

[Austin Clements]

LGTM.

Re: [PATCH v2 0/2] Replying to HTML-only emails, v2

[David Bremner]

Adam Wolfe Gordon <awg+notmuch@xvx.ca> writes:

> Hi everyone,
>
> Here is a new version of my HTML reply series [1], addressing Autin's review
> comments. The reply code now shares more with the similar notmuch-show code,
> which is a good thing.
>

I pushed this series, but it seems to have a test failure on the
buildbot (it's fine on my Debian wheezey box)

http://buildbot.notmuchmail.org/builders/master/builds/198/steps/tests/logs/problems

Maybe you and Tom could try to figure out what is different about his
environment?

d

Re: [PATCH v2 0/2] Replying to HTML-only emails, v2

[Adam Wolfe Gordon]

On Sun, May 6, 2012 at 6:12 AM, David Bremner <david@tethera.net> wrote:
> I pushed this series, but it seems to have a test failure on the
> buildbot (it's fine on my Debian wheezey box)
>
> http://buildbot.notmuchmail.org/builders/master/builds/198/steps/tests/logs/problems
>
> Maybe you and Tom could try to figure out what is different about his
> environment?

The failure is on the new HTML reply test, and the difference in the
output is just indentation - the quoted HTML text has an extra three
spaces at the beginning of each line. So, it's almost certainly a
difference in emacs versions or packages. I'm running Ubuntu Oneiric
with emacs 23.3.

It looks like the relevant variable in mm is
mm-text-html-renderer-alist, which defines various methods of parsing
HTML. For me it's:

((w3 . mm-inline-text-html-render-with-w3)
 (w3m . mm-inline-text-html-render-with-w3m)
 (w3m-standalone . mm-inline-text-html-render-with-w3m-standalone)
 (links mm-inline-render-with-file mm-links-remove-leading-blank
"links" "-dump" file)
 (lynx mm-inline-render-with-stdin nil "lynx" "-dump" "-force_html"
"-stdin" "-nolist")
 (html2text mm-inline-render-with-function html2text))

I get the behavior seen on the buildbot if the only option available
on my machine is lynx, so I'm guessing that's what's happening.

I'm not sure what the best option is for fixing this. A few possible options:

* Only run the test if links, w3m, or w3m.el is available.
* Make the test accept differences in whitespace.
* Try to produce HTML for the test that renders the same in every method.

Opinions from anyone?

-- Adam

[PATCH] test: Force reply to use html2text for consistency

[Adam Wolfe Gordon]

The output of the HTML reply test in the emacs suite can vary
depending on which HTML renderers are installed on the machine running
the tests. The renderer that is always available is emacs's builtin
html2text function. In order to get consistency, force the test to use
html2text even if other renderers are available.
---

As discussed on IRC, here's a patch that forces the HTML reply test to use
html2text, and adjusts the output accordingly. This should fix the test
failure on the buildbot.

 test/emacs |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/test/emacs b/test/emacs
index a615b39..e9f954c 100755
--- a/test/emacs
+++ b/test/emacs
@@ -447,7 +447,7 @@ test_expect_equal_file OUTPUT EXPECTED
 test_begin_subtest "Reply within emacs to an html-only message"
 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 '()))
+test_emacs "(let ((message-hidden-headers '()) (mm-text-html-renderer 'html2text))
 	    (notmuch-show \"id:${gen_msg_id}\")
 	    (notmuch-show-reply)
 	    (test-output))"
@@ -463,10 +463,7 @@ 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?
+> Hi,This is an HTML test message.OK?
 EOF
 test_expect_equal_file OUTPUT EXPECTED
 
-- 
1.7.5.4

Re: [PATCH] test: Force reply to use html2text for consistency

[David Bremner]

Adam Wolfe Gordon <awg+notmuch@xvx.ca> writes:

> The output of the HTML reply test in the emacs suite can vary
> depending on which HTML renderers are installed on the machine running
> the tests. The renderer that is always available is emacs's builtin
> html2text function. In order to get consistency, force the test to use
> html2text even if other renderers are available.

pushed, and it seems to replicate the test results on the buildbot. If
html2text ever gets less crappy I guess we'll have to redo the test
data.

d

Re: [PATCH] test: Force reply to use html2text for consistency

[Tomi Ollila]

On Mon, May 07 2012, David Bremner <david@tethera.net> wrote:

> Adam Wolfe Gordon <awg+notmuch@xvx.ca> writes:
>
>> The output of the HTML reply test in the emacs suite can vary
>> depending on which HTML renderers are installed on the machine running
>> the tests. The renderer that is always available is emacs's builtin
>> html2text function. In order to get consistency, force the test to use
>> html2text even if other renderers are available.
>
> pushed, and it seems to replicate the test results on the buildbot. If
> html2text ever gets less crappy I guess we'll have to redo the test
> data.

If html2text ever gets less crappy then we face the same problem -- users
with different emacs get different rendering output making tests fail
for some users.

Then we can do "(let ... (mm-text-html-renderer 'mm-insert-part))

to get html inserted unmodified; excerpt from test output using above:

 -> Hi,This is an HTML test message.OK?
 +> Hi,<br />This is an <b>HTML</b> test message.<br /><br />OK?

>
> d

Tomi