[PATCH 0/4] Quoting HTML-only emails in replies redux

[PATCH 0/4] Quoting HTML-only emails in replies redux

[Adam Wolfe Gordon]

From: Adam Wolfe Gordon <awg@xvx.ca>

Hi everyone,

This is a rework of my previous patch series adding support for replying to HTML
email in the emacs interface
(id:1322671241-23438-1-git-send-email-awg+notmuch@xvx.ca).

It was suggested on IRC that a more general solution would be to add a JSON
format to notmuch reply, and then have the emacs client parse the JSON to create
an appropriate reply. This patchset implements that.

The previous patches had an issue with emails that contained both HTML and
plaintext parts, where all the parts would end up quoted. This version avoids
that problem, since the emacs interface can easily check whether there are
plaintext parts and avoid quoting HTML parts if there are.

There should probably be some customize variables for this in emacs, to control
(for example) whether to quote HTML parts and whether to prefer HTML or
plaintext parts for quoting. Any suggestions for what should be customizable
would be appreciated.

I know Jani is currently working on some reply-related patches (the reply-all
vs. reply-one set). These changes probably won't merge cleanly with those
changes, so some care might be required. If his changes are pushed first, I'll
happily rebase and send a new set.

Thanks in advance for any reviews.

Adam Wolfe Gordon (4):
  test: Add broken test for the new JSON reply format.
  reply: Add a JSON reply format.
  man: Update notmuch-reply man page for JSON format.
  emacs: Use the new JSON reply format.

 emacs/notmuch-mua.el     |   62 +++++++++---
 man/man1/notmuch-reply.1 |    5 +
 notmuch-reply.c          |  269 +++++++++++++++++++++++++++++++++++++++-------
 test/emacs               |    1 +
 test/multipart           |    7 ++
 5 files changed, 292 insertions(+), 52 deletions(-)

-- 
1.7.5.4

[PATCH 1/4] test: Add broken test for the new JSON reply format.

[Adam Wolfe Gordon]

From: Adam Wolfe Gordon <awg@xvx.ca>

---
 test/multipart |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/test/multipart b/test/multipart
index f83526b..f5ebf04 100755
--- a/test/multipart
+++ b/test/multipart
@@ -589,6 +589,13 @@ Non-text part: text/html
 EOF
 test_expect_equal_file OUTPUT EXPECTED
 
+test_begin_subtest "'notmuch reply' to a multipart message with json format"
+notmuch reply --format=json 'id:87liy5ap00.fsf@yoom.home.cworth.org' >OUTPUT
+cat <<EOF >EXPECTED
+[{ "reply": { "headers": { "from": "Notmuch Test Suite <test_suite@notmuchmail.org>", "to": "Carl Worth <cworth@cworth.org>, cworth@cworth.org", "subject": "Re: Multipart message", "in-reply-to": "<87liy5ap00.fsf@yoom.home.cworth.org>", "references": " <87liy5ap00.fsf@yoom.home.cworth.org>"} }, "original": { "headers": { "from": "Carl Worth <cworth@cworth.org>", "to": "cworth@cworth.org", "cc": "", "subject": "Multipart message", "date": "Fri, 05 Jan 2001 15:43:57 +0000", "in-reply-to": "", "references": "" }, "body": [ { "content-type": "text/html", "content": "<p>This is an embedded message, with a multipart/alternative part.</p>\n"}, { "content-type": "text/plain", "content": "This is an embedded message, with a multipart/alternative part.\n"}, { "content-type": "text/plain", "content": "And this message is signed.\n\n-Carl\n"}, {} ] } }, {} ]
+EOF
+test_expect_equal_file OUTPUT EXPECTED
+
 test_begin_subtest "'notmuch show --part' does not corrupt a part with CRLF pair"
 notmuch show --format=raw --part=3 id:base64-part-with-crlf > crlf.out
 echo -n -e "\xEF\x0D\x0A" > crlf.expected
-- 
1.7.5.4

[PATCH 2/4] reply: Add a JSON reply format.

[Adam Wolfe Gordon]

From: Adam Wolfe Gordon <awg@xvx.ca>

This new JSON format for replies includes headers generated for a reply
message as well as the headers and all text parts of the original message.
Using this data, a client can intelligently create a reply. For example,
the emacs client will be able to create replies with quoted HTML parts by
parsing the HTML parts using w3m.
---
 notmuch-reply.c |  269 +++++++++++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 230 insertions(+), 39 deletions(-)

diff --git a/notmuch-reply.c b/notmuch-reply.c
index f8d5f64..82df396 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -30,6 +30,15 @@ reply_headers_message_part (GMimeMessage *message);
 static void
 reply_part_content (GMimeObject *part);
 
+static void
+reply_part_start_json (GMimeObject *part, int *part_count);
+
+static void
+reply_part_content_json (GMimeObject *part);
+
+static void
+reply_part_end_json (GMimeObject *part);
+
 static const notmuch_show_format_t format_reply = {
     "",
 	"", NULL,
@@ -46,6 +55,22 @@ static const notmuch_show_format_t format_reply = {
     ""
 };
 
+static const notmuch_show_format_t format_json = {
+    "",
+	"", NULL,
+	    "", NULL, NULL, "",
+	    "",
+	        reply_part_start_json,
+	        NULL,
+	        NULL,
+	        reply_part_content_json,
+	        reply_part_end_json,
+	        "",
+	    "",
+	"", "",
+    ""
+};
+
 static void
 show_reply_headers (GMimeMessage *message)
 {
@@ -147,6 +172,78 @@ reply_part_content (GMimeObject *part)
     }
 }
 
+static void
+reply_part_start_json (GMimeObject *part, unused(int *part_count))
+{
+    GMimeContentType *content_type = g_mime_object_get_content_type (GMIME_OBJECT (part));
+    GMimeContentDisposition *disposition = g_mime_object_get_content_disposition (part);
+
+    if (g_mime_content_type_is_type (content_type, "text", "*") &&
+	(!disposition ||
+	 strcmp (disposition->disposition, GMIME_DISPOSITION_INLINE) == 0))
+    {
+	printf("{ ");
+    }
+}
+
+static void
+reply_part_end_json (GMimeObject *part)
+{
+    GMimeContentType *content_type = g_mime_object_get_content_type (GMIME_OBJECT (part));
+    GMimeContentDisposition *disposition = g_mime_object_get_content_disposition (part);
+
+    if (g_mime_content_type_is_type (content_type, "text", "*") &&
+	(!disposition ||
+	 strcmp (disposition->disposition, GMIME_DISPOSITION_INLINE) == 0))
+	printf ("}, ");
+}
+
+static void
+reply_part_content_json (GMimeObject *part)
+{
+    GMimeContentType *content_type = g_mime_object_get_content_type (GMIME_OBJECT (part));
+    GMimeContentDisposition *disposition = g_mime_object_get_content_disposition (part);
+
+    void *ctx = talloc_new (NULL);
+
+    /* We only care about inline text parts for reply purposes */
+    if (g_mime_content_type_is_type (content_type, "text", "*") &&
+	(!disposition ||
+	 strcmp (disposition->disposition, GMIME_DISPOSITION_INLINE) == 0))
+    {
+	GMimeStream *stream_memory = NULL, *stream_filter = NULL;
+	GMimeDataWrapper *wrapper;
+	GByteArray *part_content;
+	const char *charset;
+
+	printf("\"content-type\": %s, \"content\": ",
+	       json_quote_str(ctx, g_mime_content_type_to_string(content_type)));
+
+	charset = g_mime_object_get_content_type_parameter (part, "charset");
+	stream_memory = g_mime_stream_mem_new ();
+	if (stream_memory) {
+	    stream_filter = g_mime_stream_filter_new(stream_memory);
+	    if (charset) {
+		g_mime_stream_filter_add(GMIME_STREAM_FILTER(stream_filter),
+					 g_mime_filter_charset_new(charset, "UTF-8"));
+	    }
+	}
+	wrapper = g_mime_part_get_content_object (GMIME_PART (part));
+	if (wrapper && stream_filter)
+	    g_mime_data_wrapper_write_to_stream (wrapper, stream_filter);
+	part_content = g_mime_stream_mem_get_byte_array (GMIME_STREAM_MEM (stream_memory));
+
+	printf("%s", json_quote_chararray(ctx, (char *) part_content->data, part_content->len));
+
+	if (stream_filter)
+	    g_object_unref(stream_filter);
+	if (stream_memory)
+	    g_object_unref(stream_memory);
+    }
+
+    talloc_free (ctx);
+}
+
 /* Is the given address configured as one of the user's "personal" or
  * "other" addresses. */
 static int
@@ -476,6 +573,59 @@ guess_from_received_header (notmuch_config_t *config, notmuch_message_t *message
     return NULL;
 }
 
+static GMimeMessage *
+create_reply_message(void *ctx,
+		     notmuch_config_t *config,
+		     notmuch_message_t *message)
+{
+    const char *subject, *from_addr = NULL;
+    const char *in_reply_to, *orig_references, *references;
+
+    /* The 1 means we want headers in a "pretty" order. */
+    GMimeMessage *reply = g_mime_message_new (1);
+    if (reply == NULL) {
+	fprintf (stderr, "Out of memory\n");
+	return NULL;
+    }
+
+    subject = notmuch_message_get_header (message, "subject");
+    if (subject) {
+	if (strncasecmp (subject, "Re:", 3))
+	    subject = talloc_asprintf (ctx, "Re: %s", subject);
+	g_mime_message_set_subject (reply, subject);
+    }
+
+    from_addr = add_recipients_from_message (reply, config, message);
+
+    if (from_addr == NULL)
+	from_addr = guess_from_received_header (config, message);
+
+    if (from_addr == NULL)
+	from_addr = notmuch_config_get_user_primary_email (config);
+
+    from_addr = talloc_asprintf (ctx, "%s <%s>",
+				 notmuch_config_get_user_name (config),
+				 from_addr);
+    g_mime_object_set_header (GMIME_OBJECT (reply),
+			      "From", from_addr);
+
+    in_reply_to = talloc_asprintf (ctx, "<%s>",
+				   notmuch_message_get_message_id (message));
+
+    g_mime_object_set_header (GMIME_OBJECT (reply),
+			      "In-Reply-To", in_reply_to);
+
+    orig_references = notmuch_message_get_header (message, "references");
+    references = talloc_asprintf (ctx, "%s%s%s",
+				  orig_references ? orig_references : "",
+				  orig_references ? " " : "",
+				  in_reply_to);
+    g_mime_object_set_header (GMIME_OBJECT (reply),
+			      "References", references);
+
+    return reply;
+}
+
 static int
 notmuch_reply_format_default(void *ctx,
 			     notmuch_config_t *config,
@@ -485,8 +635,6 @@ notmuch_reply_format_default(void *ctx,
     GMimeMessage *reply;
     notmuch_messages_t *messages;
     notmuch_message_t *message;
-    const char *subject, *from_addr = NULL;
-    const char *in_reply_to, *orig_references, *references;
     const notmuch_show_format_t *format = &format_reply;
 
     for (messages = notmuch_query_search_messages (query);
@@ -495,61 +643,102 @@ notmuch_reply_format_default(void *ctx,
     {
 	message = notmuch_messages_get (messages);
 
-	/* The 1 means we want headers in a "pretty" order. */
-	reply = g_mime_message_new (1);
-	if (reply == NULL) {
-	    fprintf (stderr, "Out of memory\n");
-	    return 1;
-	}
+	reply = create_reply_message(ctx, config, message);
 
-	subject = notmuch_message_get_header (message, "subject");
-	if (subject) {
-	    if (strncasecmp (subject, "Re:", 3))
-		subject = talloc_asprintf (ctx, "Re: %s", subject);
-	    g_mime_message_set_subject (reply, subject);
-	}
+	show_reply_headers (reply);
 
-	from_addr = add_recipients_from_message (reply, config, message);
+	g_object_unref (G_OBJECT (reply));
+	reply = NULL;
 
-	if (from_addr == NULL)
-	    from_addr = guess_from_received_header (config, message);
+	printf ("On %s, %s wrote:\n",
+		notmuch_message_get_header (message, "date"),
+		notmuch_message_get_header (message, "from"));
 
-	if (from_addr == NULL)
-	    from_addr = notmuch_config_get_user_primary_email (config);
+	show_message_body (message, format, params);
 
-	from_addr = talloc_asprintf (ctx, "%s <%s>",
-				     notmuch_config_get_user_name (config),
-				     from_addr);
-	g_mime_object_set_header (GMIME_OBJECT (reply),
-				  "From", from_addr);
+	notmuch_message_destroy (message);
+    }
+    return 0;
+}
 
-	in_reply_to = talloc_asprintf (ctx, "<%s>",
-			     notmuch_message_get_message_id (message));
+static int
+notmuch_reply_format_json(void *ctx,
+			  notmuch_config_t *config,
+			  notmuch_query_t *query,
+			  unused (notmuch_show_params_t *params))
+{
+    GMimeMessage *reply;
+    notmuch_messages_t *messages;
+    notmuch_message_t *message;
+    const notmuch_show_format_t *format = &format_json;
 
-	g_mime_object_set_header (GMIME_OBJECT (reply),
-				  "In-Reply-To", in_reply_to);
+    const char *reply_headers[] = {"from", "to", "subject", "in-reply-to", "references"};
+    const int n_reply_headers = 5;
+    const char *orig_headers[] = {"from", "to", "cc", "subject", "date", "in-reply-to", "references"};
+    const int n_orig_headers = 7;
+    int hidx;
 
-	orig_references = notmuch_message_get_header (message, "references");
-	references = talloc_asprintf (ctx, "%s%s%s",
-				      orig_references ? orig_references : "",
-				      orig_references ? " " : "",
-				      in_reply_to);
-	g_mime_object_set_header (GMIME_OBJECT (reply),
-				  "References", references);
+    /* Start array of reply objects */
+    printf ("[");
 
-	show_reply_headers (reply);
+    for (messages = notmuch_query_search_messages (query);
+	 notmuch_messages_valid (messages);
+	 notmuch_messages_move_to_next (messages))
+    {
+	/* Start a reply object */
+	printf ("{ \"reply\": { \"headers\": { ");
+
+	message = notmuch_messages_get (messages);
+
+	reply = create_reply_message(ctx, config, message);
+
+	for (hidx = 0; hidx < n_reply_headers; hidx += 1)
+	{
+	    if (hidx > 0) {
+		printf (", ");
+	    }
+
+	    printf ("%s: %s", json_quote_str(ctx, reply_headers[hidx]),
+		    json_quote_str (ctx, g_mime_object_get_header (GMIME_OBJECT (reply), reply_headers[hidx])));
+	}
 
 	g_object_unref (G_OBJECT (reply));
 	reply = NULL;
 
-	printf ("On %s, %s wrote:\n",
-		notmuch_message_get_header (message, "date"),
-		notmuch_message_get_header (message, "from"));
+	/* Done the headers for the reply, which has no body parts */
+	printf ("} }");
+
+	/* Start the original */
+	printf (", \"original\": { \"headers\": { ");
+
+	for (hidx = 0; hidx < n_orig_headers; hidx += 1)
+	{
+	    if (hidx > 0) {
+		printf (", ");
+	    }
+
+	    printf ("%s: %s", json_quote_str(ctx, orig_headers[hidx]),
+		    json_quote_str (ctx, notmuch_message_get_header (message, orig_headers[hidx])));
+	}
+
+	/* End headers */
+	printf (" }, \"body\": [ ");
 
+	/* Show body parts */
 	show_message_body (message, format, params);
 
 	notmuch_message_destroy (message);
+
+	/* Done the original */
+	printf ("{} ] }");
+
+	/* End the reply object. */
+	printf (" }, ");
     }
+
+    /* End array of reply objects */
+    printf ("{} ]\n");
+
     return 0;
 }
 
@@ -640,6 +829,8 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
 		reply_format_func = notmuch_reply_format_default;
 	    } else if (strcmp (opt, "headers-only") == 0) {
 		reply_format_func = notmuch_reply_format_headers_only;
+	    } else if (strcmp (opt, "json") == 0) {
+		reply_format_func = notmuch_reply_format_json;
 	    } else {
 		fprintf (stderr, "Invalid value for --format: %s\n", opt);
 		return 1;
-- 
1.7.5.4

[PATCH 3/4] man: Update notmuch-reply man page for JSON format.

[Adam Wolfe Gordon]

From: Adam Wolfe Gordon <awg@xvx.ca>

---
 man/man1/notmuch-reply.1 |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/man/man1/notmuch-reply.1 b/man/man1/notmuch-reply.1
index 099d808..240dfed 100644
--- a/man/man1/notmuch-reply.1
+++ b/man/man1/notmuch-reply.1
@@ -41,6 +41,11 @@ include
 .BR default
 Includes subject and quoted message body.
 .TP
+.BR json
+Produces JSON output containing headers for a reply message and the
+headers and text parts of the original message. This output can be used
+by a client to create a reply message intelligently.
+.TP
 .BR headers\-only
 Only produces In\-Reply\-To, References, To, Cc, and Bcc headers.
 .RE
-- 
1.7.5.4

[PATCH 4/4] emacs: Use the new JSON reply format.

[Adam Wolfe Gordon]

From: Adam Wolfe Gordon <awg@xvx.ca>

Using the new JSON reply format allows emacs to quote HTML parts
nicely by first parsing them with w3m, then quoting them. This is
very useful for users who regularly receive HTML-only email.

The behavior for messages that contain plain text parts should be
unchanged, except that an additional quoted line is added to the end
of the reply message.  The test has been updated to reflect this.
---
 emacs/notmuch-mua.el |   62 +++++++++++++++++++++++++++++++++++++++----------
 test/emacs           |    1 +
 2 files changed, 50 insertions(+), 13 deletions(-)

diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
index 7114e48..7f894cb 100644
--- a/emacs/notmuch-mua.el
+++ b/emacs/notmuch-mua.el
@@ -19,6 +19,7 @@
 ;;
 ;; Authors: David Edmondson <dme@dme.org>
 
+(require 'json)
 (require 'message)
 
 (require 'notmuch-lib)
@@ -71,27 +72,62 @@ list."
 	    (push header message-hidden-headers)))
 	notmuch-mua-hidden-headers))
 
+(defun w3m-region (start end)) ;; From `w3m.el'.
+(defun notmuch-mua-quote-part (part)
+  (with-temp-buffer
+    (insert part)
+    (message-mode)
+    (fill-region (point-min) (point-max))
+    (goto-char (point-min))
+    (perform-replace "^" "> " nil t nil)
+    (set-buffer-modified-p nil)
+    (buffer-substring (point-min) (point-max))))
+(defun notmuch-mua-parse-html-part (part)
+  (with-temp-buffer
+    (insert part)
+    (w3m-region (point-min) (point-max))
+    (set-buffer-modified-p nil)
+    (buffer-substring (point-min) (point-max))))
 (defun notmuch-mua-reply (query-string &optional sender)
-  (let (headers
+  (let (reply
+	original
+	headers
 	body
-	(args '("reply")))
+	(args '("reply" "--format=json")))
     (if notmuch-show-process-crypto
 	(setq args (append args '("--decrypt"))))
     (setq args (append args (list query-string)))
-    ;; This make assumptions about the output of `notmuch reply', but
-    ;; really only that the headers come first followed by a blank
-    ;; line and then the body.
+    ;; Get the reply object as JSON, and parse it into an elisp object.
     (with-temp-buffer
       (apply 'call-process (append (list notmuch-command nil (list t t) nil) args))
       (goto-char (point-min))
-      (if (re-search-forward "^$" nil t)
-	  (save-excursion
-	    (save-restriction
-	      (narrow-to-region (point-min) (point))
-	      (goto-char (point-min))
-	      (setq headers (mail-header-extract)))))
-      (forward-line 1)
-      (setq body (buffer-substring (point) (point-max))))
+      (setq reply (aref (json-read) 0)))
+
+    ;; Get the list of headers
+    (setq headers (cdr (assq 'headers (assq 'reply reply))))
+    ;; Construct the body of the reply.
+    (setq original (cdr (assq 'original reply)))
+
+    ;; Start with the prelude, based on the headers of the original message.
+    (let ((original-headers (cdr (assq 'headers original))))
+      (setq body (format "On %s, %s wrote:\n"
+			 (cdr (assq 'date original-headers))
+			 (cdr (assq 'from original-headers)))))
+
+    ;; Extract the body parts and construct a reasonable quoted body.
+    (let* ((body-parts (cdr (assq 'body original)))
+	   (find-parts (lambda (type) (delq nil (mapcar (lambda (part)
+							  (if (string= (cdr (assq 'content-type part)) type)
+							      (cdr (assq 'content part))))
+							body-parts))))
+	   (plain-parts (apply find-parts '("text/plain")))
+	   (html-parts (apply find-parts '("text/html"))))
+      
+      (if (not (null plain-parts))
+	  (mapc (lambda (part) (setq body (concat body (notmuch-mua-quote-part part)))) plain-parts)
+	(mapc (lambda (part) (setq body (concat body (notmuch-mua-quote-part (notmuch-mua-parse-html-part part))))) html-parts)))
+    (setq body (concat body "\n"))
+	
     ;; If sender is non-nil, set the From: header to its value.
     (when sender
       (mail-header-set 'from sender headers))
diff --git a/test/emacs b/test/emacs
index a06c223..fe501da 100755
--- a/test/emacs
+++ b/test/emacs
@@ -270,6 +270,7 @@ Fcc: $(pwd)/mail/sent
 --text follows this line--
 On 01 Jan 2000 12:00:00 -0000, Notmuch Test Suite <test_suite@notmuchmail.org> wrote:
 > This is a test that messages are sent via SMTP
+> 
 EOF
 test_expect_equal_file OUTPUT EXPECTED
 
-- 
1.7.5.4

Re: [PATCH 4/4] emacs: Use the new JSON reply format.

[Aaron Ecay]

Adam,

One comment below.

On Sun,  8 Jan 2012 00:52:42 -0700, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:
> From: Adam Wolfe Gordon <awg@xvx.ca>
> 
> Using the new JSON reply format allows emacs to quote HTML parts
> nicely by first parsing them with w3m, then quoting them. This is
> very useful for users who regularly receive HTML-only email.
> 
> The behavior for messages that contain plain text parts should be
> unchanged, except that an additional quoted line is added to the end
> of the reply message.  The test has been updated to reflect this.
> ---
>  emacs/notmuch-mua.el |   62 +++++++++++++++++++++++++++++++++++++++----------
>  test/emacs           |    1 +
>  2 files changed, 50 insertions(+), 13 deletions(-)
> 
> diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
> index 7114e48..7f894cb 100644
> --- a/emacs/notmuch-mua.el
> +++ b/emacs/notmuch-mua.el
> @@ -19,6 +19,7 @@
>  ;;
>  ;; Authors: David Edmondson <dme@dme.org>
>  
> +(require 'json)
>  (require 'message)
>  
>  (require 'notmuch-lib)
> @@ -71,27 +72,62 @@ list."
>  	    (push header message-hidden-headers)))
>  	notmuch-mua-hidden-headers))
>  
> +(defun w3m-region (start end)) ;; From `w3m.el'.

What is the purpose of the above line?  If it is to make the compiler
aware of the function, you should use ‘declare-function’ instead.  Defun
will erase the original definition of the w3m-region function.

-- 
Aaron Ecay

Re: [PATCH 0/4] Quoting HTML-only emails in replies redux

[Aaron Ecay]

On Sun,  8 Jan 2012 00:52:38 -0700, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:

[...]

> 
> There should probably be some customize variables for this in emacs, to control
> (for example) whether to quote HTML parts and whether to prefer HTML or
> plaintext parts for quoting. Any suggestions for what should be customizable
> would be appreciated.

I think two variables should suffice: one (boolean) to control whether
to quote standalone text/html parts, and one to control which parts of a
multipart/alternative part to quote.  The latter should take possible
values 'text, 'html, and 'both.  This requires the emacs reply
functionality to distinguish between html parts that are part of a
multipart/alternative and those which are not, which (AFAICT) the
current code doesn’t do.

I haven’t tested the patch yet, but it looks very promising.  Thanks!

-- 
Aaron Ecay

Re: [PATCH 4/4] emacs: Use the new JSON reply format.

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 3952 bytes --]

On Sun,  8 Jan 2012 00:52:42 -0700, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:
> +(defun w3m-region (start end)) ;; From `w3m.el'.
> +(defun notmuch-mua-quote-part (part)
> +  (with-temp-buffer
> +    (insert part)
> +    (message-mode)
> +    (fill-region (point-min) (point-max))
> +    (goto-char (point-min))
> +    (perform-replace "^" "> " nil t nil)
> +    (set-buffer-modified-p nil)
> +    (buffer-substring (point-min) (point-max))))

Couldn't all of this be done directly in the reply buffer?

> +(defun notmuch-mua-parse-html-part (part)
> +  (with-temp-buffer
> +    (insert part)
> +    (w3m-region (point-min) (point-max))
> +    (set-buffer-modified-p nil)
> +    (buffer-substring (point-min) (point-max))))

Blank lines between functions are the house style (as much as there is
such a thing).

Using w3m means that you should `require' it. What happens when a user
doesn't have it? (Either the elisp or the command.)

>  (defun notmuch-mua-reply (query-string &optional sender)
> -  (let (headers
> +  (let (reply
> +	original
> +	headers
>  	body
> -	(args '("reply")))
> +	(args '("reply" "--format=json")))

Initialised variables are generally shown before uninitialised. I know
that you didn't do this 'wrong' and that it's an unrelated change, but
it should be fixed. (To the people who say that should be a separate
patch I say 'meh' - life is too short.)

>      (if notmuch-show-process-crypto
>  	(setq args (append args '("--decrypt"))))
>      (setq args (append args (list query-string)))
> -    ;; This make assumptions about the output of `notmuch reply', but
> -    ;; really only that the headers come first followed by a blank
> -    ;; line and then the body.
> +    ;; Get the reply object as JSON, and parse it into an elisp object.
>      (with-temp-buffer
>        (apply 'call-process (append (list notmuch-command nil (list t t) nil) args))
>        (goto-char (point-min))
> -      (if (re-search-forward "^$" nil t)
> -	  (save-excursion
> -	    (save-restriction
> -	      (narrow-to-region (point-min) (point))
> -	      (goto-char (point-min))
> -	      (setq headers (mail-header-extract)))))
> -      (forward-line 1)
> -      (setq body (buffer-substring (point) (point-max))))
> +      (setq reply (aref (json-read) 0)))
> +
> +    ;; Get the list of headers
> +    (setq headers (cdr (assq 'headers (assq 'reply reply))))
> +    ;; Construct the body of the reply.
> +    (setq original (cdr (assq 'original reply)))

The scope of (at least) `headers' and `original' could be tightened by
moving them down to the following `let' (and converting it to `let*').

> +
> +    ;; Start with the prelude, based on the headers of the original message.
> +    (let ((original-headers (cdr (assq 'headers original))))
> +      (setq body (format "On %s, %s wrote:\n"
> +			 (cdr (assq 'date original-headers))
> +			 (cdr (assq 'from original-headers)))))
> +
> +    ;; Extract the body parts and construct a reasonable quoted body.
> +    (let* ((body-parts (cdr (assq 'body original)))
> +	   (find-parts (lambda (type) (delq nil (mapcar (lambda (part)
> +							  (if (string= (cdr (assq 'content-type part)) type)
> +							      (cdr (assq 'content part))))
> +							body-parts))))

`find-parts' might be useful in other places. Maybe add it to notmuch-lib.el?

> +	   (plain-parts (apply find-parts '("text/plain")))
> +	   (html-parts (apply find-parts '("text/html"))))
> +      
> +      (if (not (null plain-parts))
> +	  (mapc (lambda (part) (setq body (concat body (notmuch-mua-quote-part part)))) plain-parts)
> +	(mapc (lambda (part) (setq body (concat body (notmuch-mua-quote-part (notmuch-mua-parse-html-part part))))) html-parts)))

If you have an 'else' clause, why test '(if (not ..' ?

> +    (setq body (concat body "\n"))
> +	

If it already ends with a carriage return, why do this?

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 0/4] Quoting HTML-only emails in replies redux

[Adam Wolfe Gordon]

Thanks for the suggestions.  Specific comments inline:

On Sun, Jan 8, 2012 at 18:36, Aaron Ecay <aaronecay@gmail.com> wrote:
>> There should probably be some customize variables for this in emacs, to control
>> (for example) whether to quote HTML parts and whether to prefer HTML or
>> plaintext parts for quoting. Any suggestions for what should be customizable
>> would be appreciated.
>
> I think two variables should suffice: one (boolean) to control whether
> to quote standalone text/html parts, and one to control which parts of a
> multipart/alternative part to quote.  The latter should take possible
> values 'text, 'html, and 'both.  This requires the emacs reply
> functionality to distinguish between html parts that are part of a
> multipart/alternative and those which are not, which (AFAICT) the
> current code doesn’t do.

The first one I think is obvious, and easy to add.  My previous
version actually had such an option, but I didn't bother with it this
time in the interest of getting the patch out.  I'll add this in and
send a new version.

Regarding the second suggestion, you're right that the current code
doesn't differentiate between text parts that are part of a
multipart/alternative and ones that aren't.  However, it does only
include parts with inline disposition, so it shouldn't end up quoting
stuff intended as attachments.  My thinking in the current operation
was to keep most of the complexity (i.e. going through the MIME parts
to pick out the relevant ones) in the C code, so that it doesn't have
to be implemented in each client - and also so I didn't have to
implement it in emacs lisp ;-).  Do you have some ideas for a JSON
format that's more complex than the current one (to include the
information necessary for making decisions about what to quote), but
less complex than something like notmuch show --format=json (which
would require the client to descend the MIME tree completely to create
a reply)?

A third customization option I was thinking about is a way to control
the format of the first line of the body (On $date, $person wrote).  I
think it would be fairly simple to let people specify an arbitrary
format using any of the headers of the original message, so you would
set it to something like "On %date%, %from% wrote", or "Quoth %from%
(%date%)".  This might be particularly useful to users who correspond
normally in a language other than English.  Anyone have thoughts on
whether this is worth implementing?

> I haven’t tested the patch yet, but it looks very promising.  Thanks!

Thanks for the suggestions - let me know if you have a chance to try it out.

-- 
Adam Wolfe Gordon

Re: [PATCH 4/4] emacs: Use the new JSON reply format.

[Adam Wolfe Gordon]

On Sun, Jan 8, 2012 at 18:27, Aaron Ecay <aaronecay@gmail.com> wrote:
>> +(defun w3m-region (start end)) ;; From `w3m.el'.
>
> What is the purpose of the above line?  If it is to make the compiler
> aware of the function, you should use ‘declare-function’ instead.  Defun
> will erase the original definition of the w3m-region function.

Indeed, it's to make the compiler aware of the function.  I followed
the example of notmuch-show.el (defvar w3m-current-buffer, line 391),
but it sounds like declare-function is a better way to accomplish
this.  I haven't written a whole lot of emacs lisp, so thanks for the
tip.

Given David's comments about requiring w3m instead it might be moot,
but if not I'll make this change for the next version.

Re: [PATCH 4/4] emacs: Use the new JSON reply format.

[Adam Wolfe Gordon]

Hi David,

Thanks for the review.  Most of the things you've suggested are easy
changes, and I think obvious improvements, so I'll change them for the
next version.  A bit of discussion on the more involved things below:

On Mon, Jan 9, 2012 at 01:50, David Edmondson <dme@dme.org> wrote:
> On Sun,  8 Jan 2012 00:52:42 -0700, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:
>> +(defun w3m-region (start end)) ;; From `w3m.el'.
>> +(defun notmuch-mua-quote-part (part)
>> +  (with-temp-buffer
>> +    (insert part)
>> +    (message-mode)
>> +    (fill-region (point-min) (point-max))
>> +    (goto-char (point-min))
>> +    (perform-replace "^" "> " nil t nil)
>> +    (set-buffer-modified-p nil)
>> +    (buffer-substring (point-min) (point-max))))
>
> Couldn't all of this be done directly in the reply buffer?

Indeed, it could, I just hadn't thought of it.  I'll do this for the
next version.

> Using w3m means that you should `require' it. What happens when a user
> doesn't have it? (Either the elisp or the command.)

This was my initial thought, but when I looked at notmuch-show.el,
which uses w3m features, I noticed that it doesn't have a require.  To
be clear, this patch requires w3m.el (not just the w3m binary), which
I don't think anything else in notmuch does.

In the previous version I had a customize variable specifying whether
to quote HTML parts, which meant that if the user could set the
customize variable to false and everything would work without w3m.el.
I'd like not to introduce a new prerequisite, so if there's a way to
make w3m.el optional that would be my preference.  Can you provide
some guidance on this?

-- 
Adam Wolfe Gordon

Re: [PATCH 4/4] emacs: Use the new JSON reply format.

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 1502 bytes --]

On Mon, 9 Jan 2012 19:10:48 -0700, Adam Wolfe Gordon <awg@xvx.ca> wrote:
> > Using w3m means that you should `require' it. What happens when a user
> > doesn't have it? (Either the elisp or the command.)
> 
> This was my initial thought, but when I looked at notmuch-show.el,
> which uses w3m features, I noticed that it doesn't have a require.  To
> be clear, this patch requires w3m.el (not just the w3m binary), which
> I don't think anything else in notmuch does.
> 
> In the previous version I had a customize variable specifying whether
> to quote HTML parts, which meant that if the user could set the
> customize variable to false and everything would work without w3m.el.
> I'd like not to introduce a new prerequisite, so if there's a way to
> make w3m.el optional that would be my preference.  Can you provide
> some guidance on this?

My suggestion would be to move the `html to text' functionality to a new
.el, as we might have more than one way to achieve it (emacs 24 has
`shr', for example).

Have `notmuch-mua.el' use a generically named function to perform the
transformation from html to text and that function should determine the
best way to achieve it.

Testing for `w3m.el' is relatively easy (`require' it and check for
error). Testing for `w3m' itself can be done using some code similar to
`notmuch-address-locate-command' (search the list - it's not yet
integrated), which is itself just copied from w3m (and should end up in
`notmuch-lib.el').

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 4/4] emacs: Use the new JSON reply format.

[Dmitry Kurochkin]

Hi Adam.

On Mon, 9 Jan 2012 19:10:48 -0700, Adam Wolfe Gordon <awg@xvx.ca> wrote:
> Hi David,
> 
> Thanks for the review.  Most of the things you've suggested are easy
> changes, and I think obvious improvements, so I'll change them for the
> next version.  A bit of discussion on the more involved things below:
> 
> On Mon, Jan 9, 2012 at 01:50, David Edmondson <dme@dme.org> wrote:
> > On Sun,  8 Jan 2012 00:52:42 -0700, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:
> >> +(defun w3m-region (start end)) ;; From `w3m.el'.
> >> +(defun notmuch-mua-quote-part (part)
> >> +  (with-temp-buffer
> >> +    (insert part)
> >> +    (message-mode)
> >> +    (fill-region (point-min) (point-max))
> >> +    (goto-char (point-min))
> >> +    (perform-replace "^" "> " nil t nil)
> >> +    (set-buffer-modified-p nil)
> >> +    (buffer-substring (point-min) (point-max))))
> >
> > Couldn't all of this be done directly in the reply buffer?
> 
> Indeed, it could, I just hadn't thought of it.  I'll do this for the
> next version.
> 
> > Using w3m means that you should `require' it. What happens when a user
> > doesn't have it? (Either the elisp or the command.)
> 
> This was my initial thought, but when I looked at notmuch-show.el,
> which uses w3m features, I noticed that it doesn't have a require.  To
> be clear, this patch requires w3m.el (not just the w3m binary), which
> I don't think anything else in notmuch does.
> 
> In the previous version I had a customize variable specifying whether
> to quote HTML parts, which meant that if the user could set the
> customize variable to false and everything would work without w3m.el.
> I'd like not to introduce a new prerequisite, so if there's a way to
> make w3m.el optional that would be my preference.  Can you provide
> some guidance on this?
> 

I did not follow the rest of the discussion, so sorry if I missed
something obvious.  But why can't we render HTML parts in replies the
same way we do in notmuch-show (using `mm-display-part')?  That should
not introduce a w3m.el requirement, would use the same renderer as
configured for show and hence would produce consistent output in show
and reply.

Regards,
  Dmitry

> -- 
> Adam Wolfe Gordon
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 4/4] emacs: Use the new JSON reply format.

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 529 bytes --]

On Thu, 12 Jan 2012 12:33:58 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> I did not follow the rest of the discussion, so sorry if I missed
> something obvious.  But why can't we render HTML parts in replies the
> same way we do in notmuch-show (using `mm-display-part')?  That should
> not introduce a w3m.el requirement, would use the same renderer as
> configured for show and hence would produce consistent output in show
> and reply.

Agreed, that would be good. It could be done as a second stage, though.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 4/4] emacs: Use the new JSON reply format.

[Tomi Ollila]

On Thu, 12 Jan 2012 12:07:40 +0000, David Edmondson <dme@dme.org> wrote:
> On Thu, 12 Jan 2012 12:33:58 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > I did not follow the rest of the discussion, so sorry if I missed
> > something obvious.  But why can't we render HTML parts in replies the
> > same way we do in notmuch-show (using `mm-display-part')?  That should
> > not introduce a w3m.el requirement, would use the same renderer as
> > configured for show and hence would produce consistent output in show
> > and reply.
> 
> Agreed, that would be good. It could be done as a second stage, though.

For the record. I built emacs 23.3 from source and after installation
by default there is no w3m module available.

Related thing: by default the value of (emacs variable)
mm-text-html-renderer is resolved as follows (IIRC):

   if w3m module can be loaded value will be 'w3m
   if no w3m is available but w3m binary exists, value will be 'w3m-standalone
   else it gets some other value (if any).

Tomi

Re: [PATCH 4/4] emacs: Use the new JSON reply format.

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 1004 bytes --]

On Thu, 12 Jan 2012 15:36:07 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> On Thu, 12 Jan 2012 12:07:40 +0000, David Edmondson <dme@dme.org> wrote:
> > On Thu, 12 Jan 2012 12:33:58 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > > I did not follow the rest of the discussion, so sorry if I missed
> > > something obvious.  But why can't we render HTML parts in replies the
> > > same way we do in notmuch-show (using `mm-display-part')?  That should
> > > not introduce a w3m.el requirement, would use the same renderer as
> > > configured for show and hence would produce consistent output in show
> > > and reply.
> > 
> > Agreed, that would be good. It could be done as a second stage, though.
> 
> For the record. I built emacs 23.3 from source and after installation
> by default there is no w3m module available.

Currently it's broken for everyone. With the proposed changes it will be
fixed for some people. Further improvements will, of course, be
possible.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 2/4] reply: Add a JSON reply format.

[Jani Nikula]

On Sun,  8 Jan 2012 00:52:40 -0700, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:
> From: Adam Wolfe Gordon <awg@xvx.ca>
> 
> This new JSON format for replies includes headers generated for a reply
> message as well as the headers and all text parts of the original message.
> Using this data, a client can intelligently create a reply. For example,
> the emacs client will be able to create replies with quoted HTML parts by
> parsing the HTML parts using w3m.

Hi Adam, this is a drive-by-review on some things I spotted, but can't
say I would've thought the whole thing through. I'm pretty ignorant
about MIME parts etc. Please find comments inline.

The reply-to-sender set was just merged. You'll have conflicts both here
and in emacs code, but they should be trivial to sort out.


BR,
Jani.


> ---
>  notmuch-reply.c |  269 +++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 230 insertions(+), 39 deletions(-)
> 
> diff --git a/notmuch-reply.c b/notmuch-reply.c
> index f8d5f64..82df396 100644
> --- a/notmuch-reply.c
> +++ b/notmuch-reply.c
> @@ -30,6 +30,15 @@ reply_headers_message_part (GMimeMessage *message);
>  static void
>  reply_part_content (GMimeObject *part);
>  
> +static void
> +reply_part_start_json (GMimeObject *part, int *part_count);
> +
> +static void
> +reply_part_content_json (GMimeObject *part);
> +
> +static void
> +reply_part_end_json (GMimeObject *part);
> +

I know there are existing forward declarations like this, but would it
be much trouble to arrange the code so that they were not needed at all?

>  static const notmuch_show_format_t format_reply = {
>      "",
>  	"", NULL,
> @@ -46,6 +55,22 @@ static const notmuch_show_format_t format_reply = {
>      ""
>  };
>  
> +static const notmuch_show_format_t format_json = {
> +    "",
> +	"", NULL,
> +	    "", NULL, NULL, "",
> +	    "",
> +	        reply_part_start_json,
> +	        NULL,
> +	        NULL,
> +	        reply_part_content_json,
> +	        reply_part_end_json,
> +	        "",
> +	    "",
> +	"", "",
> +    ""
> +};
> +
>  static void
>  show_reply_headers (GMimeMessage *message)
>  {
> @@ -147,6 +172,78 @@ reply_part_content (GMimeObject *part)
>      }
>  }
>  
> +static void
> +reply_part_start_json (GMimeObject *part, unused(int *part_count))
> +{
> +    GMimeContentType *content_type = g_mime_object_get_content_type (GMIME_OBJECT (part));
> +    GMimeContentDisposition *disposition = g_mime_object_get_content_disposition (part);
> +
> +    if (g_mime_content_type_is_type (content_type, "text", "*") &&
> +	(!disposition ||
> +	 strcmp (disposition->disposition, GMIME_DISPOSITION_INLINE) == 0))
> +    {
> +	printf("{ ");
> +    }
> +}
> +
> +static void
> +reply_part_end_json (GMimeObject *part)
> +{
> +    GMimeContentType *content_type = g_mime_object_get_content_type (GMIME_OBJECT (part));
> +    GMimeContentDisposition *disposition = g_mime_object_get_content_disposition (part);
> +
> +    if (g_mime_content_type_is_type (content_type, "text", "*") &&
> +	(!disposition ||
> +	 strcmp (disposition->disposition, GMIME_DISPOSITION_INLINE) == 0))
> +	printf ("}, ");
> +}

The two functions above, while small, are almost identical. Please move
the common stuff to a common helper, and you can have something like
this:

	if (the_common_function (part))
		printf ("}, ");

> +
> +static void
> +reply_part_content_json (GMimeObject *part)
> +{
> +    GMimeContentType *content_type = g_mime_object_get_content_type (GMIME_OBJECT (part));
> +    GMimeContentDisposition *disposition = g_mime_object_get_content_disposition (part);
> +
> +    void *ctx = talloc_new (NULL);
> +
> +    /* We only care about inline text parts for reply purposes */
> +    if (g_mime_content_type_is_type (content_type, "text", "*") &&
> +	(!disposition ||
> +	 strcmp (disposition->disposition, GMIME_DISPOSITION_INLINE) == 0))

Oh, you can use the common helper here too.

> +    {
> +	GMimeStream *stream_memory = NULL, *stream_filter = NULL;

No need to initialize stream_memory here.

> +	GMimeDataWrapper *wrapper;
> +	GByteArray *part_content;
> +	const char *charset;
> +
> +	printf("\"content-type\": %s, \"content\": ",
> +	       json_quote_str(ctx, g_mime_content_type_to_string(content_type)));

The style in notmuch is to have a space before the opening brace in
function calls. Check elsewhere also. I always forget that too. :)

> +
> +	charset = g_mime_object_get_content_type_parameter (part, "charset");

AFAICT charset declaration and the above call could be moved inside "if
(stream_memory)" below.

> +	stream_memory = g_mime_stream_mem_new ();
> +	if (stream_memory) {
> +	    stream_filter = g_mime_stream_filter_new(stream_memory);
> +	    if (charset) {
> +		g_mime_stream_filter_add(GMIME_STREAM_FILTER(stream_filter),
> +					 g_mime_filter_charset_new(charset, "UTF-8"));
> +	    }
> +	}
> +	wrapper = g_mime_part_get_content_object (GMIME_PART (part));
> +	if (wrapper && stream_filter)
> +	    g_mime_data_wrapper_write_to_stream (wrapper, stream_filter);
> +	part_content = g_mime_stream_mem_get_byte_array (GMIME_STREAM_MEM (stream_memory));

You only need charset, stream_memory and stream_filter if wrapper is
true. You could perhaps include all that stuff within "if (wrapper)".

> +
> +	printf("%s", json_quote_chararray(ctx, (char *) part_content->data, part_content->len));
> +
> +	if (stream_filter)
> +	    g_object_unref(stream_filter);
> +	if (stream_memory)
> +	    g_object_unref(stream_memory);
> +    }
> +
> +    talloc_free (ctx);
> +}
> +
>  /* Is the given address configured as one of the user's "personal" or
>   * "other" addresses. */
>  static int
> @@ -476,6 +573,59 @@ guess_from_received_header (notmuch_config_t *config, notmuch_message_t *message
>      return NULL;
>  }
>  
> +static GMimeMessage *
> +create_reply_message(void *ctx,
> +		     notmuch_config_t *config,
> +		     notmuch_message_t *message)
> +{
> +    const char *subject, *from_addr = NULL;
> +    const char *in_reply_to, *orig_references, *references;
> +
> +    /* The 1 means we want headers in a "pretty" order. */
> +    GMimeMessage *reply = g_mime_message_new (1);
> +    if (reply == NULL) {
> +	fprintf (stderr, "Out of memory\n");
> +	return NULL;
> +    }
> +
> +    subject = notmuch_message_get_header (message, "subject");
> +    if (subject) {
> +	if (strncasecmp (subject, "Re:", 3))
> +	    subject = talloc_asprintf (ctx, "Re: %s", subject);
> +	g_mime_message_set_subject (reply, subject);
> +    }
> +
> +    from_addr = add_recipients_from_message (reply, config, message);
> +
> +    if (from_addr == NULL)
> +	from_addr = guess_from_received_header (config, message);
> +
> +    if (from_addr == NULL)
> +	from_addr = notmuch_config_get_user_primary_email (config);
> +
> +    from_addr = talloc_asprintf (ctx, "%s <%s>",
> +				 notmuch_config_get_user_name (config),
> +				 from_addr);
> +    g_mime_object_set_header (GMIME_OBJECT (reply),
> +			      "From", from_addr);
> +
> +    in_reply_to = talloc_asprintf (ctx, "<%s>",
> +				   notmuch_message_get_message_id (message));
> +
> +    g_mime_object_set_header (GMIME_OBJECT (reply),
> +			      "In-Reply-To", in_reply_to);
> +
> +    orig_references = notmuch_message_get_header (message, "references");
> +    references = talloc_asprintf (ctx, "%s%s%s",
> +				  orig_references ? orig_references : "",
> +				  orig_references ? " " : "",
> +				  in_reply_to);
> +    g_mime_object_set_header (GMIME_OBJECT (reply),
> +			      "References", references);
> +
> +    return reply;
> +}

If you want to, you could make review easier by first having a patch
that abstracts the above in the original code, with no other
modifications. Then you could introduce new functionality that utilizes
it later. But this is no big deal in such a small abstraction.

> +
>  static int
>  notmuch_reply_format_default(void *ctx,
>  			     notmuch_config_t *config,
> @@ -485,8 +635,6 @@ notmuch_reply_format_default(void *ctx,
>      GMimeMessage *reply;
>      notmuch_messages_t *messages;
>      notmuch_message_t *message;
> -    const char *subject, *from_addr = NULL;
> -    const char *in_reply_to, *orig_references, *references;
>      const notmuch_show_format_t *format = &format_reply;
>  
>      for (messages = notmuch_query_search_messages (query);
> @@ -495,61 +643,102 @@ notmuch_reply_format_default(void *ctx,
>      {
>  	message = notmuch_messages_get (messages);
>  
> -	/* The 1 means we want headers in a "pretty" order. */
> -	reply = g_mime_message_new (1);
> -	if (reply == NULL) {
> -	    fprintf (stderr, "Out of memory\n");
> -	    return 1;
> -	}
> +	reply = create_reply_message(ctx, config, message);

You should check the return value.

>  
> -	subject = notmuch_message_get_header (message, "subject");
> -	if (subject) {
> -	    if (strncasecmp (subject, "Re:", 3))
> -		subject = talloc_asprintf (ctx, "Re: %s", subject);
> -	    g_mime_message_set_subject (reply, subject);
> -	}
> +	show_reply_headers (reply);
>  
> -	from_addr = add_recipients_from_message (reply, config, message);
> +	g_object_unref (G_OBJECT (reply));
> +	reply = NULL;
>  
> -	if (from_addr == NULL)
> -	    from_addr = guess_from_received_header (config, message);
> +	printf ("On %s, %s wrote:\n",
> +		notmuch_message_get_header (message, "date"),
> +		notmuch_message_get_header (message, "from"));
>  
> -	if (from_addr == NULL)
> -	    from_addr = notmuch_config_get_user_primary_email (config);
> +	show_message_body (message, format, params);
>  
> -	from_addr = talloc_asprintf (ctx, "%s <%s>",
> -				     notmuch_config_get_user_name (config),
> -				     from_addr);
> -	g_mime_object_set_header (GMIME_OBJECT (reply),
> -				  "From", from_addr);
> +	notmuch_message_destroy (message);
> +    }
> +    return 0;
> +}
>  
> -	in_reply_to = talloc_asprintf (ctx, "<%s>",
> -			     notmuch_message_get_message_id (message));
> +static int
> +notmuch_reply_format_json(void *ctx,
> +			  notmuch_config_t *config,
> +			  notmuch_query_t *query,
> +			  unused (notmuch_show_params_t *params))
> +{
> +    GMimeMessage *reply;
> +    notmuch_messages_t *messages;
> +    notmuch_message_t *message;
> +    const notmuch_show_format_t *format = &format_json;
>  
> -	g_mime_object_set_header (GMIME_OBJECT (reply),
> -				  "In-Reply-To", in_reply_to);
> +    const char *reply_headers[] = {"from", "to", "subject", "in-reply-to", "references"};
> +    const int n_reply_headers = 5;

Drop this, and use ARRAY_SIZE (reply_headers) instead.

> +    const char *orig_headers[] = {"from", "to", "cc", "subject", "date", "in-reply-to", "references"};
> +    const int n_orig_headers = 7;

Ditto.

> +    int hidx;
>  
> -	orig_references = notmuch_message_get_header (message, "references");
> -	references = talloc_asprintf (ctx, "%s%s%s",
> -				      orig_references ? orig_references : "",
> -				      orig_references ? " " : "",
> -				      in_reply_to);
> -	g_mime_object_set_header (GMIME_OBJECT (reply),
> -				  "References", references);
> +    /* Start array of reply objects */
> +    printf ("[");
>  
> -	show_reply_headers (reply);
> +    for (messages = notmuch_query_search_messages (query);
> +	 notmuch_messages_valid (messages);
> +	 notmuch_messages_move_to_next (messages))
> +    {
> +	/* Start a reply object */
> +	printf ("{ \"reply\": { \"headers\": { ");
> +
> +	message = notmuch_messages_get (messages);
> +
> +	reply = create_reply_message(ctx, config, message);

Check return value.

> +
> +	for (hidx = 0; hidx < n_reply_headers; hidx += 1)

hidx++ is the pattern in such for loops.

> +	{
> +	    if (hidx > 0) {
> +		printf (", ");
> +	    }

You could just compare "if (hidx)", and also drop the curly braces.

> +
> +	    printf ("%s: %s", json_quote_str(ctx, reply_headers[hidx]),
> +		    json_quote_str (ctx, g_mime_object_get_header (GMIME_OBJECT (reply), reply_headers[hidx])));
> +	}
>  
>  	g_object_unref (G_OBJECT (reply));
>  	reply = NULL;
>  
> -	printf ("On %s, %s wrote:\n",
> -		notmuch_message_get_header (message, "date"),
> -		notmuch_message_get_header (message, "from"));
> +	/* Done the headers for the reply, which has no body parts */
> +	printf ("} }");
> +
> +	/* Start the original */
> +	printf (", \"original\": { \"headers\": { ");
> +
> +	for (hidx = 0; hidx < n_orig_headers; hidx += 1)

hidx++

> +	{
> +	    if (hidx > 0) {
> +		printf (", ");
> +	    }

Same as above.

> +
> +	    printf ("%s: %s", json_quote_str(ctx, orig_headers[hidx]),
> +		    json_quote_str (ctx, notmuch_message_get_header (message, orig_headers[hidx])));
> +	}
> +
> +	/* End headers */
> +	printf (" }, \"body\": [ ");
>  
> +	/* Show body parts */
>  	show_message_body (message, format, params);
>  
>  	notmuch_message_destroy (message);
> +
> +	/* Done the original */
> +	printf ("{} ] }");
> +
> +	/* End the reply object. */
> +	printf (" }, ");
>      }
> +
> +    /* End array of reply objects */
> +    printf ("{} ]\n");
> +
>      return 0;
>  }
>  
> @@ -640,6 +829,8 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
>  		reply_format_func = notmuch_reply_format_default;
>  	    } else if (strcmp (opt, "headers-only") == 0) {
>  		reply_format_func = notmuch_reply_format_headers_only;
> +	    } else if (strcmp (opt, "json") == 0) {
> +		reply_format_func = notmuch_reply_format_json;
>  	    } else {
>  		fprintf (stderr, "Invalid value for --format: %s\n", opt);
>  		return 1;
> -- 
> 1.7.5.4
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 0/4] Quoting HTML-only emails in replies redux

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 359 bytes --]

Given that we're now doing a bunch of work in emacs as part of the reply
setup, why not just grab the content of the original message from the
show buffer and quote that?

The last time that approach was discussed Carl was against it because it
moved the emacs UI away from the behaviour of the CLI, but it seems that
we're already heading in that direction.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 4/4] emacs: Use the new JSON reply format.

[Adam Wolfe Gordon]

Hi Dmitry,

On Thu, Jan 12, 2012 at 01:33, Dmitry Kurochkin
<dmitry.kurochkin@gmail.com> wrote:
> I did not follow the rest of the discussion, so sorry if I missed
> something obvious.  But why can't we render HTML parts in replies the
> same way we do in notmuch-show (using `mm-display-part')?  That should
> not introduce a w3m.el requirement, would use the same renderer as
> configured for show and hence would produce consistent output in show
> and reply.

Thanks for the suggestion - I've implemented this now and I think it's
definitely the best way to go.  New patches coming soon with all the
suggested changes.

-- 
Adam Wolfe Gordon

Re: [PATCH 0/4] Quoting HTML-only emails in replies redux

[Aaron Ecay]

On Sun, 15 Jan 2012 09:26:59 +0000, David Edmondson <dme@dme.org> wrote:
> Given that we're now doing a bunch of work in emacs as part of the reply
> setup, why not just grab the content of the original message from the
> show buffer and quote that?
> 
> The last time that approach was discussed Carl was against it because it
> moved the emacs UI away from the behaviour of the CLI, but it seems that
> we're already heading in that direction.

I have been watching this patch series with interest, because it seemed
that when it landed it would be a good time for me to begin work on a
patch to allow notmuch to function like other emacs MUAs in constructing
the reply buffer internally to emacs, rather than through notmuch.  This
allows (at least) three things:
- Greater flexibility in the construction of address lists.  For example,
  there are some email lists where I want replies to list mail to go only
  to the list, not also to the original sender.  Additionally, I like to
  reply from my university address if colleagues write to my Gmail one.
  If a lisp function is generating the replies, it can be made to run a
  hook allowing users to insert these or other custom behaviors.
- The same reasoning as above, applied to signatures.  (different ones
  for different recipients)
- There exists at least one emacs package (supercite) which allows
  customization of the quoting of email replies.  This automates the
  “Firstname>” style quotes one sometimes sees, as well as many other
  possiblities.  It defines a way for emacs MUAs to construct reply
  buffers to cooperate with it, which many of the big emacs MUAs obey
  (Gnus and Wanderlust certainly do).  This is explained in the “hints
  to MUA authors” section of the supercite manual (distributed with
  Emacs).

So, a +1 from me on this idea, from a different perspective.

-- 
Aaron Ecay

Re: [PATCH 0/4] Quoting HTML-only emails in replies redux

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 2329 bytes --]

On Mon, 16 Jan 2012 02:38:38 -0500, Aaron Ecay <aaronecay@gmail.com> wrote:
> On Sun, 15 Jan 2012 09:26:59 +0000, David Edmondson <dme@dme.org> wrote:
> > Given that we're now doing a bunch of work in emacs as part of the reply
> > setup, why not just grab the content of the original message from the
> > show buffer and quote that?
> > 
> > The last time that approach was discussed Carl was against it because it
> > moved the emacs UI away from the behaviour of the CLI, but it seems that
> > we're already heading in that direction.
> 
> I have been watching this patch series with interest, because it seemed
> that when it landed it would be a good time for me to begin work on a
> patch to allow notmuch to function like other emacs MUAs in constructing
> the reply buffer internally to emacs, rather than through notmuch.  This
> allows (at least) three things:
> - Greater flexibility in the construction of address lists.  For example,
>   there are some email lists where I want replies to list mail to go only
>   to the list, not also to the original sender.

Is there a mechanistic way to determine the correct behaviour in this
respect? I suspect that it's exactly the kind of thing that Carl wanted
to be included in 'notmuch' itself, so that other UIs can benefit.

>   Additionally, I like to
>   reply from my university address if colleagues write to my Gmail one.
>   If a lisp function is generating the replies, it can be made to run a
>   hook allowing users to insert these or other custom behaviors.
> - The same reasoning as above, applied to signatures.  (different ones
>   for different recipients)

You can do both of these things today using `message-send-hook' (I do).

> - There exists at least one emacs package (supercite) which allows
>   customization of the quoting of email replies.  This automates the
>   “Firstname>” style quotes one sometimes sees, as well as many other
>   possiblities.  It defines a way for emacs MUAs to construct reply
>   buffers to cooperate with it, which many of the big emacs MUAs obey
>   (Gnus and Wanderlust certainly do).  This is explained in the “hints
>   to MUA authors” section of the supercite manual (distributed with
>   Emacs).

I dislike supercite, so no support from me in that direction :-)

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 0/4] Quoting HTML-only emails in replies redux

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 619 bytes --]

On Mon, 16 Jan 2012 08:39:30 +0000, David Edmondson <dme@dme.org> wrote:
> Is there a mechanistic way to determine the correct behaviour in this
> respect? I suspect that it's exactly the kind of thing that Carl wanted
> to be included in 'notmuch' itself, so that other UIs can benefit.

Agreed.  I think it's generally better to get the functionality we want
in the CLI if we can, so that all UIs benefit.  Emacs is still certainly
the most actively developed UI, and it's gotten quite complicated with
quite a few special features, but to the extent that we can put
new functionality into the CLI we should.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH 0/4] Quoting HTML-only emails in replies redux

[Aaron Ecay]

On Mon, 16 Jan 2012 08:39:30 +0000, David Edmondson <dme@dme.org> wrote:
> On Mon, 16 Jan 2012 02:38:38 -0500, Aaron Ecay <aaronecay@gmail.com> wrote:
> > - Greater flexibility in the construction of address lists.  For example,
> >   there are some email lists where I want replies to list mail to go only
> >   to the list, not also to the original sender.
> 
> Is there a mechanistic way to determine the correct behaviour in this
> respect? I suspect that it's exactly the kind of thing that Carl wanted
> to be included in 'notmuch' itself, so that other UIs can benefit.

I think it requires some amount of configuration, but it can be done
sensibly.  I am much more proficient with Elisp than with C, and
Emacs has prejudiced me towards solutions that allow me to have a
Turing-complete configuration language.  :)

I think a good starting point for thinking about mailing lists is what
Mutt does:
http://www.mutt.org/doc/manual/manual-4.html#using_lists

Notmuch at the CLI/C code level could aim for a comparable level of
expressiveness, and I think it would suffice for most people (including
me).

[...]
> 
> You can do both of these things today using `message-send-hook' (I
> do).

I avoided that, as it seemed to me that just before the message is sent
is too late to be doing these things (I’d like to see confirmation when
writing the message that the address/signature changes were applied
correctly).  But “M-x apropos RET message hook RET” shows that there are
some earlier points to hook into as well.  Thanks.

> I dislike supercite, so no support from me in that direction :-)

One advantage of supercite is that it allows non-English speakers to set
up the “On X, X wrote” line as they prefer.  Notmuch’s current approach
(a hard-coded C string) is the opposite of internationalized.  So it
would be nice to support some customization of that as well, even if we
don’t go the supercite route.

-- 
Aaron Ecay

Re: [PATCH 0/4] Quoting HTML-only emails in replies redux

[Adam Wolfe Gordon]

On Mon, Jan 16, 2012 at 15:31, Aaron Ecay <aaronecay@gmail.com> wrote:
> One advantage of supercite is that it allows non-English speakers to set
> up the “On X, X wrote” line as they prefer.  Notmuch’s current approach
> (a hard-coded C string) is the opposite of internationalized.  So it
> would be nice to support some customization of that as well, even if we
> don’t go the supercite route.

Note that with my patch, the "On X, X wrote" string is no longer
hardcoded in the CLI (at least when using the JSON reply format).  I
intend to make it configurable in emacs, though right now it is
hardcoded in notmuch-mua.el.

-- 
Adam Wolfe Gordon