[PATCH v3 0/5] Quoting HTML emails in reply

[Adam Wolfe Gordon]

Hi everyone,

Thanks to all who reviewed bits of my previous series [1]. This version
contains:

* The latest version of the emacs patch [2], rebased against the current
  master.
* An updated version of the patch to notmuch-reply.c to address Jani's
  review comments from yesterday.
* The additional emacs patch I sent previously [3] to use the existing
  message-citation-line-format customization variable, as suggested by
  Gregor Zattler [4].

I'd be happy to hear more reviews, and would especially like if someone
can test this out with a variety of emails. My mail is almost exclusively
HTML-only, with a bit of plaintext-only, so I've tested very little with
multipart messages.

[1] id:"[hidden email]"
[2] id:"[hidden email]"
[3] id:"[hidden email]"
[4] id:"[hidden email]"

Adam Wolfe Gordon (5):
  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: Use message-citation-line-format in reply

 emacs/notmuch-lib.el     |    8 ++
 emacs/notmuch-mua.el     |  108 +++++++++++++------
 man/man1/notmuch-reply.1 |    5 +
 notmuch-reply.c          |  271 +++++++++++++++++++++++++++++++++++++++-------
 test/emacs               |    2 +-
 test/multipart           |    7 ++
 6 files changed, 328 insertions(+), 73 deletions(-)

--
1.7.5.4

_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch

[Adam Wolfe Gordon]

---
 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:[hidden email]' >OUTPUT
+cat <<EOF >EXPECTED
+[{ "reply": { "headers": { "from": "Notmuch Test Suite <[hidden email]>", "to": "Carl Worth <[hidden email]>, [hidden email]", "subject": "Re: Multipart message", "in-reply-to": "<[hidden email]>", "references": " <[hidden email]>"} }, "original": { "headers": { "from": "Carl Worth <[hidden email]>", "to": "[hidden email]", "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

_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch

[Adam Wolfe Gordon]

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 |  271 +++++++++++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 231 insertions(+), 40 deletions(-)

diff --git a/notmuch-reply.c b/notmuch-reply.c
index 0f682db..b4c2426 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)
 {
@@ -86,6 +111,17 @@ reply_headers_message_part (GMimeMessage *message)
     printf ("> Date: %s\n", g_mime_message_get_date_as_string (message));
 }
 
+static notmuch_bool_t
+reply_check_part_type (GMimeObject *part, const char *type, const char *subtype,
+       const char *disposition)
+{
+    GMimeContentType *content_type = g_mime_object_get_content_type (GMIME_OBJECT (part));
+    GMimeContentDisposition *part_disposition = g_mime_object_get_content_disposition (part);
+
+    return (g_mime_content_type_is_type (content_type, type, subtype) &&
+    (!part_disposition ||
+     strcmp (part_disposition->disposition, disposition) == 0));
+}
 
 static void
 reply_part_content (GMimeObject *part)
@@ -147,6 +183,63 @@ reply_part_content (GMimeObject *part)
     }
 }
 
+static void
+reply_part_start_json (GMimeObject *part, unused (int *part_count))
+{
+    if (reply_check_part_type (part, "text", "*", GMIME_DISPOSITION_INLINE))
+ printf ("{ ");
+}
+
+static void
+reply_part_end_json (GMimeObject *part)
+{
+    if (reply_check_part_type (part, "text", "*", GMIME_DISPOSITION_INLINE))
+ printf ("}, ");
+}
+
+static void
+reply_part_content_json (GMimeObject *part)
+{
+    GMimeContentType *content_type = g_mime_object_get_content_type (GMIME_OBJECT (part));
+    void *ctx = talloc_new (NULL);
+
+    /* We only care about inline text parts for reply purposes */
+    if (reply_check_part_type (part, "text", "*", GMIME_DISPOSITION_INLINE)) {
+ GMimeDataWrapper *wrapper;
+ GByteArray *part_content;
+
+ printf ("\"content-type\": %s, \"content\": ",
+       json_quote_str (ctx, g_mime_content_type_to_string (content_type)));
+
+ wrapper = g_mime_part_get_content_object (GMIME_PART (part));
+ if (wrapper) {
+    const char *charset = g_mime_object_get_content_type_parameter (part, "charset");
+    GMimeStream *stream_memory = g_mime_stream_mem_new ();
+    if (stream_memory) {
+ GMimeStream *stream_filter = NULL;
+ 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"));
+ }
+
+ if (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));
+    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
@@ -505,6 +598,61 @@ 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,
+     notmuch_bool_t reply_all)
+{
+    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, reply_all);
+
+    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,
@@ -515,8 +663,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);
@@ -525,62 +671,103 @@ 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, reply_all);
 
- 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);
- }
+ if (!reply)
+    continue;
 
- from_addr = add_recipients_from_message (reply, config, message,
- reply_all);
+ show_reply_headers (reply);
 
- if (from_addr == NULL)
-    from_addr = guess_from_received_header (config, message);
+ g_object_unref (G_OBJECT (reply));
+ reply = NULL;
 
- if (from_addr == NULL)
-    from_addr = notmuch_config_get_user_primary_email (config);
+ printf ("On %s, %s wrote:\n",
+ notmuch_message_get_header (message, "date"),
+ notmuch_message_get_header (message, "from"));
 
- 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);
+ show_message_body (message, format, params);
 
- in_reply_to = talloc_asprintf (ctx, "<%s>",
-     notmuch_message_get_message_id (message));
+ notmuch_message_destroy (message);
+    }
+    return 0;
+}
 
- g_mime_object_set_header (GMIME_OBJECT (reply),
-  "In-Reply-To", in_reply_to);
+static int
+notmuch_reply_format_json(void *ctx,
+  notmuch_config_t *config,
+  notmuch_query_t *query,
+  unused (notmuch_show_params_t *params),
+  notmuch_bool_t reply_all)
+{
+    GMimeMessage *reply;
+    notmuch_messages_t *messages;
+    notmuch_message_t *message;
+    const notmuch_show_format_t *format = &format_json;
 
- 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);
+    const char *reply_headers[] = {"from", "to", "subject", "in-reply-to", "references"};
+    const char *orig_headers[] = {"from", "to", "cc", "subject", "date", "in-reply-to", "references"};
+    unsigned int hidx;
 
- show_reply_headers (reply);
+    /* Start array of reply objects */
+    printf ("[");
+
+    for (messages = notmuch_query_search_messages (query);
+ notmuch_messages_valid (messages);
+ notmuch_messages_move_to_next (messages))
+    {
+ message = notmuch_messages_get (messages);
+ reply = create_reply_message (ctx, config, message, reply_all);
+ if (!reply)
+    continue;
+
+ /* Start a reply object */
+ printf ("{ \"reply\": { \"headers\": { ");
+
+ for (hidx = 0; hidx < ARRAY_SIZE (reply_headers); hidx++)
+ {
+    if (hidx)
+ 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 < ARRAY_SIZE (orig_headers); hidx++)
+ {
+    if (hidx)
+ 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;
 }
 
@@ -646,6 +833,7 @@ notmuch_reply_format_headers_only(void *ctx,
 
 enum {
     FORMAT_DEFAULT,
+    FORMAT_JSON,
     FORMAT_HEADERS_ONLY,
 };
 
@@ -666,6 +854,7 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
     notmuch_opt_desc_t options[] = {
  { NOTMUCH_OPT_KEYWORD, &format, "format", 'f',
   (notmuch_keyword_t []){ { "default", FORMAT_DEFAULT },
+  { "json", FORMAT_JSON },
   { "headers-only", FORMAT_HEADERS_ONLY },
   { 0, 0 } } },
  { NOTMUCH_OPT_KEYWORD, &reply_all, "reply-to", 'r',
@@ -684,6 +873,8 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
 
     if (format == FORMAT_HEADERS_ONLY)
  reply_format_func = notmuch_reply_format_headers_only;
+    else if (format == FORMAT_JSON)
+ reply_format_func = notmuch_reply_format_json;
     else
  reply_format_func = notmuch_reply_format_default;
 
--
1.7.5.4

_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch

[Adam Wolfe Gordon]

---
 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 5160ece..ea7f87b 100644
--- a/man/man1/notmuch-reply.1
+++ b/man/man1/notmuch-reply.1
@@ -43,6 +43,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

_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch

[Adam Wolfe Gordon]

Using the new JSON reply format allows emacs to quote HTML
parts nicely by using mm-display-part to turn them into displayable
text, 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.
---
 emacs/notmuch-lib.el |    8 ++++
 emacs/notmuch-mua.el |   95 +++++++++++++++++++++++++++++++++-----------------
 2 files changed, 71 insertions(+), 32 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 9242537..9863d69 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.
 
+(eval-when-compile (require 'cl))
+
 (defvar notmuch-command "notmuch"
   "Command to run the notmuch binary.")
 
@@ -160,6 +162,12 @@ the user hasn't set this variable with the old or new value."
   (list 'when (< emacs-major-version 23)
  form))
 
+(defun notmuch-parts-filter-by-type (parts type)
+  "Return a list of message parts with the given type"
+  (loop for part across parts
+ if (string= (cdr (assq 'content-type part)) type)
+ collect (cdr (assq 'content part))))
+
 ;; Compatibility functions for versions of emacs before emacs 23.
 ;;
 ;; Both functions here were copied from emacs 23 with the following copyright:
diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
index 023645e..5ae0ccf 100644
--- a/emacs/notmuch-mua.el
+++ b/emacs/notmuch-mua.el
@@ -19,6 +19,7 @@
 ;;
 ;; Authors: David Edmondson <[hidden email]>
 
+(require 'json)
 (require 'message)
 
 (require 'notmuch-lib)
@@ -72,49 +73,79 @@ list."
     (push header message-hidden-headers)))
  notmuch-mua-hidden-headers))
 
+(defun notmuch-mua-insert-part-quoted (part)
+  (let ((start (point))
+ limit)
+    (insert part)
+    (setq limit (point-marker))
+    (goto-char start)
+    (while (re-search-forward "\\(^\\)[^$]" (marker-position limit) 0)
+      (replace-match "> " nil nil nil 1))
+    (set-buffer-modified-p nil)))
+
+(defun notmuch-mua-parse-html-part (part)
+  (with-temp-buffer
+    (insert part)
+    (let ((handle (mm-make-handle (current-buffer) (list "text/html")))
+  (end-of-orig (point-max)))
+      (mm-display-part handle)
+      (delete-region (point-min) end-of-orig)
+      (fill-region (point-min) (point-max))
+      (buffer-substring (point-min) (point-max)))))
+
 (defun notmuch-mua-reply (query-string &optional sender reply-all)
-  (let (headers
- body
- (args '("reply")))
+  (let ((args '("reply" "--format=json"))
+ reply
+ body)
     (if notmuch-show-process-crypto
  (setq args (append args '("--decrypt"))))
     (if reply-all
  (setq args (append args '("--reply-to=all")))
       (setq args (append args '("--reply-to=sender"))))
     (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))))
-    ;; If sender is non-nil, set the From: header to its value.
-    (when sender
-      (mail-header-set 'from sender headers))
-    (let
- ;; Overlay the composition window on that being used to read
- ;; the original message.
- ((same-window-regexps '("\\*mail .*")))
-      (notmuch-mua-mail (mail-header 'to headers)
- (mail-header 'subject headers)
- (message-headers-to-generate headers t '(to subject))))
-    ;; insert the message body - but put it in front of the signature
-    ;; if one is present
-    (goto-char (point-max))
-    (if (re-search-backward message-signature-separator nil t)
+      (setq reply (aref (json-read) 0)))
+
+    ;; Start with the prelude, based on the headers of the original message.
+    (let* ((original (cdr (assq 'original reply)))
+   (headers (cdr (assq 'headers (assq 'reply reply))))
+   (original-headers (cdr (assq 'headers original)))
+   (body-parts (cdr (assq 'body original)))
+   (plain-parts (notmuch-parts-filter-by-type body-parts "text/plain"))
+   (html-parts (notmuch-parts-filter-by-type body-parts "text/html")))
+
+      ;; If sender is non-nil, set the From: header to its value.
+      (when sender
+ (mail-header-set 'from sender headers))
+      (let
+  ;; Overlay the composition window on that being used to read
+  ;; the original message.
+  ((same-window-regexps '("\\*mail .*")))
+ (notmuch-mua-mail (mail-header 'to headers)
+  (mail-header 'subject headers)
+  (message-headers-to-generate headers t '(to subject))))
+      ;; insert the message body - but put it in front of the signature
+      ;; if one is present
+      (goto-char (point-max))
+      (if (re-search-backward message-signature-separator nil t)
   (forward-line -1)
-      (goto-char (point-max)))
-    (insert body)
-    (push-mark))
-  (set-buffer-modified-p nil)
+ (goto-char (point-max)))
+
+      (insert (format "On %s, %s wrote:\n"
+      (cdr (assq 'date original-headers))
+      (cdr (assq 'from original-headers))))
+
+      (if plain-parts
+  (mapc (lambda (part) (notmuch-mua-insert-part-quoted part)) plain-parts)
+ (mapc (lambda (part)
+ (notmuch-mua-insert-part-quoted (notmuch-mua-parse-html-part part)))
+      html-parts))
+
+      (push-mark))
+    (set-buffer-modified-p nil))
 
   (message-goto-body))
 
--
1.7.5.4

_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch

[Adam Wolfe Gordon]

Instead of using a static citation line for the first line of the
reply message, use the customizable one defined by message-mode.
This makes it easy for users to customize the reply style, and
retains consistency for users with existing message-mode
customizations.
---
 emacs/notmuch-mua.el |   19 ++++++++++++++++---
 test/emacs           |    2 +-
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
index 5ae0ccf..e485d93 100644
--- a/emacs/notmuch-mua.el
+++ b/emacs/notmuch-mua.el
@@ -134,9 +134,22 @@ list."
   (forward-line -1)
  (goto-char (point-max)))
 
-      (insert (format "On %s, %s wrote:\n"
-      (cdr (assq 'date original-headers))
-      (cdr (assq 'from original-headers))))
+      (let* ((quoth message-citation-line-format)
+     (case-fold-search nil)
+     (full-from (cdr (assq 'from original-headers)))
+     (from-addr (car (mail-header-parse-address full-from)))
+     (from-name (cdr (mail-header-parse-address full-from)))
+     (first-name (car (split-string from-name)))
+     (last-name (append (cdr (split-string from-name))))
+     (time (date-to-time (cdr (assq 'date original-headers)))))
+
+ (setq quoth (replace-regexp-in-string "%f" full-from quoth t t))
+ (setq quoth (replace-regexp-in-string "%n" from-addr quoth t t))
+ (setq quoth (replace-regexp-in-string "%N" from-name quoth t t))
+ (setq quoth (replace-regexp-in-string "%F" first-name quoth t t))
+ (setq quoth (replace-regexp-in-string "%L" last-name quoth t t))
+ (setq quoth (format-time-string quoth time))
+ (insert quoth))
 
       (if plain-parts
   (mapc (lambda (part) (notmuch-mua-insert-part-quoted part)) plain-parts)
diff --git a/test/emacs b/test/emacs
index ac47b16..3f59b23 100755
--- a/test/emacs
+++ b/test/emacs
@@ -268,7 +268,7 @@ Subject: Re: Testing message sent via SMTP
 In-Reply-To: <XXX>
 Fcc: $(pwd)/mail/sent
 --text follows this line--
-On 01 Jan 2000 12:00:00 -0000, Notmuch Test Suite <[hidden email]> wrote:
+On Sat, Jan 01 2000, Notmuch Test Suite wrote:
 > This is a test that messages are sent via SMTP
 EOF
 test_expect_equal_file OUTPUT EXPECTED
--
1.7.5.4

_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch

[Aaron Ecay-2]

On Thu, 19 Jan 2012 10:46:57 -0700, Adam Wolfe Gordon <[hidden email]> wrote:

> Instead of using a static citation line for the first line of the
> reply message, use the customizable one defined by message-mode.
> This makes it easy for users to customize the reply style, and
> retains consistency for users with existing message-mode
> customizations.
> ---
>  emacs/notmuch-mua.el |   19 ++++++++++++++++---
>  test/emacs           |    2 +-
>  2 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
> index 5ae0ccf..e485d93 100644
> --- a/emacs/notmuch-mua.el
> +++ b/emacs/notmuch-mua.el
> @@ -134,9 +134,22 @@ list."
>    (forward-line -1)
>   (goto-char (point-max)))
>  
> -      (insert (format "On %s, %s wrote:\n"
> -      (cdr (assq 'date original-headers))
> -      (cdr (assq 'from original-headers))))
> +      (let* ((quoth message-citation-line-format)
> +     (case-fold-search nil)
> +     (full-from (cdr (assq 'from original-headers)))
> +     (from-addr (car (mail-header-parse-address full-from)))
> +     (from-name (cdr (mail-header-parse-address full-from)))
> +     (first-name (car (split-string from-name)))
> +     (last-name (append (cdr (split-string from-name))))
> +     (time (date-to-time (cdr (assq 'date original-headers)))))
> +
> + (setq quoth (replace-regexp-in-string "%f" full-from quoth t t))
> + (setq quoth (replace-regexp-in-string "%n" from-addr quoth t t))
> + (setq quoth (replace-regexp-in-string "%N" from-name quoth t t))
> + (setq quoth (replace-regexp-in-string "%F" first-name quoth t t))
> + (setq quoth (replace-regexp-in-string "%L" last-name quoth t t))
> + (setq quoth (format-time-string quoth time))
> + (insert quoth))

Shouldn’t this just use message-insert-formatted-citation-line?

Another approach you might take with this patch series is to look at
the message-cite-original function (which I just discovered as I was
plumbing around in message.el looking for the function to format the
citation line).  I think that what one does to use this fn is to put
the original message text into the reply buffer (unquoted), set point
and mark to encompass it, then call the fn.  It automatically handles
inserting the quotes, and has some customization options (stripping
signatures from replies, customizable quote character instead of “>”,
...).

The message-cite-original function also adds escape characters to the
cookies that message-mode uses to indicate sign/encrypt/attach
directives.  I think notmuch exposes files on the user’s computer to
others, if a user can be tricked into replying to a message with an
attachment cookie and not stripping the cookie from the reply text.  So
to mitigate this, whatever reply mechanism winds up being used should
call mml-quote-region on the reply text (as message-cite-original does).

I just sent a patch to the list to do this in the current version of
notmuch, which should show up in
id:"[hidden email]" .

--
Aaron Ecay
_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch

[Adam Wolfe Gordon]

On Thu, Jan 19, 2012 at 11:45, Aaron Ecay <[hidden email]> wrote:
> Shouldn’t this just use message-insert-formatted-citation-line?

Yes, good idea.  I just tried this and it almost works.  The only
issue is that the default message-mode-citation-line-format has a
newline at the end, and this function inserts an *additional* newline,
so we end up with a blank line before the beginning of the quoted
text.  This is fixable by the user, of course, but it means the
default out-of-the-box setup will create funny-looking replies, which
is probably bad.  Thoughts?

> Another approach you might take with this patch series is to look at
> the message-cite-original function (which I just discovered as I was
> plumbing around in message.el looking for the function to format the
> citation line).  I think that what one does to use this fn is to put
> the original message text into the reply buffer (unquoted), set point
> and mark to encompass it, then call the fn.  It automatically handles
> inserting the quotes, and has some customization options (stripping
> signatures from replies, customizable quote character instead of “>”,
> ...).
>
> The message-cite-original function also adds escape characters to the
> cookies that message-mode uses to indicate sign/encrypt/attach
> directives.  I think notmuch exposes files on the user’s computer to
> others, if a user can be tricked into replying to a message with an
> attachment cookie and not stripping the cookie from the reply text.  So
> to mitigate this, whatever reply mechanism winds up being used should
> call mml-quote-region on the reply text (as message-cite-original does).

I've also tried using message-cite-original to create the reply body,
and it also almost works.  The issue, again, is one of defaults.  In
addition to the blank line I mentioned above, the default
message-citation-line-function inserts the "plain" citation line "Foo
<[hidden email]> writes:" instead of the formatted one.  This is a big
change from the current notmuch default.

If everyone's OK with this and willing to customize it themselves,
then I'm happy to go with this solution, but I'm pretty reluctant to
change the default in such a significant way.
_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch

[Aaron Ecay-2]

On Thu, 19 Jan 2012 21:46:46 -0700, Adam Wolfe Gordon <[hidden email]> wrote:

> On Thu, Jan 19, 2012 at 11:45, Aaron Ecay <[hidden email]> wrote:
> > Shouldn’t this just use message-insert-formatted-citation-line?
>
> Yes, good idea.  I just tried this and it almost works.  The only
> issue is that the default message-mode-citation-line-format has a
> newline at the end, and this function inserts an *additional* newline,
> so we end up with a blank line before the beginning of the quoted
> text.  This is fixable by the user, of course, but it means the
> default out-of-the-box setup will create funny-looking replies, which
> is probably bad.  Thoughts?

(let ((message-citation-line-format
       (remove ?\n message-citation-line-format)))
  ...)

(Or, if you think someone might have a newline other than at the end of
the string, you could do something a little more complicated to remove
only a newline at the end.)

Or you could introduce a new defcustom
‘notmuch-message-citation-line-format’ (with newline-less default), and
let-bind m-l-c-f to that value.  But that is pretty ugly – we don’t want
to have to “wrap” every variable whose default we don’t like.

> I've also tried using message-cite-original to create the reply body,
> and it also almost works.  The issue, again, is one of defaults.  In
> addition to the blank line I mentioned above, the default
> message-citation-line-function inserts the "plain" citation line "Foo
> <[hidden email]> writes:" instead of the formatted one.  This is a big
> change from the current notmuch default.
>
> If everyone's OK with this and willing to customize it themselves,
> then I'm happy to go with this solution, but I'm pretty reluctant to
> change the default in such a significant way.

I’m personally of the opinion that notmuch should just say “the mail
composition facility is provided by message mode (here is the
documentation on customizing it)”.  Message mode is 8,000 lines of
elisp.  We have the choice to:
- write our own message composition mode
- wrap (as explained above) the default message-mode variables we don’t
  like in notmuch-prefixed variants, with suitable let-bindings.
- use only the parts of message-mode that we like
- pass composition off to message mode

The first option just doesn’t make sense.  The second is also a little
nuts.  The third is what we are trying now, but it’s painful – the
message mode code has its own structure and its own defaults, which
don’t always agree with notmuch’s.  I am in favor of the fourth –
notmuch’s elisp code should pass data to message mode in the most low
level form it can, and let message mode do any extra work in the way
it already does.  And users should customize message composition via
message mode, not notmuch.  There’s a tradeoff to this approach – by
controlling more within notmuch, we can have nicer defaults at the
expense of more brittle code and/or fewer user-visible customizations.

This is not in any way a criticism of your work (which is great) –
you’re right that you need “permission” to uproot the defaults, and I’m
advocating for it to be given.

One possible step that might ease the transition pain could be for
notmuch’s emacs interface to have a configuration file (similar to
Wanderlust’s ~/.wl; I believe Gnus also uses a ~/.gnus).  The idea is
that this file contains elisp code, and is loaded by notmuch the first
time any notmuch-related commands are invoked by the user.  If the file
does not exist, notmuch could create it with default content that sets
message-citation-function, message-citation-line-format,
message-yank-prefix (to get rid of the ugly default whereby message-mode
indents the original message by four spaces instead of inserting “> ”),
etc.  If there is interest in this approach, I’d be happy to work on a
patch for it.

I’ve sort of stumped for this idea before
(id:"[hidden email]") and it didn’t exactly get rave reviews.
So I’ll shut up if it’s really not something people want to see.

I’ll close with an example of a nice feature that message mode has
(which I’ve been really wanting since the reply keybindings changed)
that notmuch would get for free if it hooked into message mode better:
the function message-widen-reply takes a reply-to-sender message and
makes it reply-to-all.

--
Aaron Ecay
_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch

[David Edmondson]

On Fri, 20 Jan 2012 00:53:39 -0500, Aaron Ecay <[hidden email]> wrote:
> I’m personally of the opinion that notmuch should just say “the mail
> composition facility is provided by message mode (here is the
> documentation on customizing it)”.

In general, +1.

> One possible step that might ease the transition pain could be for
> notmuch’s emacs interface to have a configuration file (similar to
> Wanderlust’s ~/.wl; I believe Gnus also uses a ~/.gnus).  The idea is
> that this file contains elisp code, and is loaded by notmuch the first
> time any notmuch-related commands are invoked by the user.

This is how my own configuration is stored (in ~/.notmuch.el).

> I’ll close with an example of a nice feature that message mode has
> (which I’ve been really wanting since the reply keybindings changed)
> that notmuch would get for free if it hooked into message mode better:
> the function message-widen-reply takes a reply-to-sender message and
> makes it reply-to-all.

That would require a bunch of work on our side to prepare the data that
message-mode uses, but would indeed be nice.

_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch

[Adam Wolfe Gordon]

Erk, forgot to reply-all.  Aaron might get this twice.

On Thu, Jan 19, 2012 at 22:53, Aaron Ecay <[hidden email]> wrote:
> (let ((message-citation-line-format
>       (remove ?\n message-citation-line-format)))
>  ...)
>
> (Or, if you think someone might have a newline other than at the end of
> the string, you could do something a little more complicated to remove
> only a newline at the end.)

I did something like this initially, to make the test pass, but my
thought is that some people might intend for that newline to be there
and we shouldn't be removing it.  Perhaps I'm being overly sensitive
to people with odd setups ;-).

> Or you could introduce a new defcustom
> ‘notmuch-message-citation-line-format’ (with newline-less default), and
> let-bind m-l-c-f to that value.  But that is pretty ugly – we don’t want
> to have to “wrap” every variable whose default we don’t like.

Agreed, not a good solution.

> I’m personally of the opinion that notmuch should just say “the mail
> composition facility is provided by message mode (here is the
> documentation on customizing it)”.  Message mode is 8,000 lines of
> elisp.  We have the choice to:
> - write our own message composition mode
> - wrap (as explained above) the default message-mode variables we don’t
>  like in notmuch-prefixed variants, with suitable let-bindings.
> - use only the parts of message-mode that we like
> - pass composition off to message mode
>
> The first option just doesn’t make sense.  The second is also a little
> nuts.  The third is what we are trying now, but it’s painful – the
> message mode code has its own structure and its own defaults, which
> don’t always agree with notmuch’s.  I am in favor of the fourth –
> notmuch’s elisp code should pass data to message mode in the most low
> level form it can, and let message mode do any extra work in the way
> it already does.  And users should customize message composition via
> message mode, not notmuch.  There’s a tradeoff to this approach – by
> controlling more within notmuch, we can have nicer defaults at the
> expense of more brittle code and/or fewer user-visible customizations.
>
> This is not in any way a criticism of your work (which is great) –
> you’re right that you need “permission” to uproot the defaults, and I’m
> advocating for it to be given.

I think we're on the same page here - I definitely prefer to have
notmuch-mua use existing emacs functionality wherever it makes sense.
The only question in my mind is how ugly we're willing to let the
defaults be in order to leverage existing stuff.

> One possible step that might ease the transition pain could be for
> notmuch’s emacs interface to have a configuration file (similar to
> Wanderlust’s ~/.wl; I believe Gnus also uses a ~/.gnus).  The idea is
> that this file contains elisp code, and is loaded by notmuch the first
> time any notmuch-related commands are invoked by the user.  If the file
> does not exist, notmuch could create it with default content that sets
> message-citation-function, message-citation-line-format,
> message-yank-prefix (to get rid of the ugly default whereby message-mode
> indents the original message by four spaces instead of inserting “> ”),
> etc.  If there is interest in this approach, I’d be happy to work on a
> patch for it.

This would be a good way to get around the ugly defaults problem, and
it would also be an easy way for people to share their notmuch/emacs
setups.  I already have my setup in a file separate from my normal
emacs config, and I run `emacs -q -l ~/.emacs-notmuch -f notmuch` so
it doesn't load my normal, programming-oriented setup.

One additional issue I noticed this morning with using
message-cite-original: it doesn't wrap/fill the quoted message.  I'm
guessing there's a way to make message-cite-original do this, but I
haven't figured it out.
_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch

[Tomi Ollila-2]

On Fri, 20 Jan 2012 10:22:09 -0700, Adam Wolfe Gordon <[hidden email]> wrote:
> Erk, forgot to reply-all.  Aaron might get this twice.

Pick this:  [hidden email]  (and add to to/cc) next time you forgot
to press 'R' (that's what I do :)

>
> On Thu, Jan 19, 2012 at 22:53, Aaron Ecay <[hidden email]> wrote:
>
> > One possible step that might ease the transition pain could be for
> > notmuch’s emacs interface to have a configuration file (similar to
> > Wanderlust’s ~/.wl; I believe Gnus also uses a ~/.gnus).  The idea is
> > that this file contains elisp code, and is loaded by notmuch the first
> > time any notmuch-related commands are invoked by the user.  If the file
> > does not exist, notmuch could create it with default content that sets
> > message-citation-function, message-citation-line-format,
> > message-yank-prefix (to get rid of the ugly default whereby message-mode
> > indents the original message by four spaces instead of inserting “> ”),
> > etc.  If there is interest in this approach, I’d be happy to work on a
> > patch for it.

Yes, that would be good -- then there is default file everyone can be
directed to...

> This would be a good way to get around the ugly defaults problem, and
> it would also be an easy way for people to share their notmuch/emacs
> setups.  I already have my setup in a file separate from my normal
> emacs config, and I run `emacs -q -l ~/.emacs-notmuch -f notmuch` so
> it doesn't load my normal, programming-oriented setup.

I used to do just that -- but then I cannot enter M-x notmuch in my
emacs for sending email from that particular emacs. Now I have something
like:

(autoload 'notmuch "~/local/my-notmuch" "Notmuchmail" t)

Where first lines add load path for notmuch, then (require 'notmuch)
and then stars loading configuration...

The question, in case that configuration file is added, where is it
located: ~/.notmuch (to add yet another file there), or into
.notmuch/ directory or XDG -like .config/notmuch/ (config.el ?)

See: id:"[hidden email]"

Tomi
_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch

[Adam Wolfe Gordon]

Use message-mode's message-cite-original function to create the
quoted body for reply messages. In order to make this act like the
existing notmuch defaults, you will need to set the following in
your emacs configuration:

message-citation-line-format "On %a, %d %b %Y, %f wrote:"
message-citation-line-function 'message-insert-formatted-citation-line

The test has been updated to reflect the (ugly) emacs default.
---

Here is an alternate version of the patch, which uses message-cite-original.

I suggest people try out this version and see if the behavior is
acceptable with some configuration tweaks. If it is, then we can
work on implementing the notmuch-emacs config file idea, and go
with this version. As I mentioned, the one thing I haven't figured
out how to do with configuration is make message-cite-original fill
the quoted message. This would probably be a dealbreaker for me, but
I suspect it can be done somehow with the right combination of hooks.

 emacs/notmuch-mua.el |   32 +++++++++++++++++++-------------
 test/emacs           |    3 ++-
 2 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
index 5ae0ccf..45c314d 100644
--- a/emacs/notmuch-mua.el
+++ b/emacs/notmuch-mua.el
@@ -21,6 +21,7 @@
 
 (require 'json)
 (require 'message)
+(require 'format-spec)
 
 (require 'notmuch-lib)
 (require 'notmuch-address)
@@ -134,19 +135,24 @@ list."
   (forward-line -1)
  (goto-char (point-max)))
 
-      (insert (format "On %s, %s wrote:\n"
-      (cdr (assq 'date original-headers))
-      (cdr (assq 'from original-headers))))
-
-      (if plain-parts
-  (mapc (lambda (part) (notmuch-mua-insert-part-quoted part)) plain-parts)
- (mapc (lambda (part)
- (notmuch-mua-insert-part-quoted (notmuch-mua-parse-html-part part)))
-      html-parts))
-
-      (push-mark))
-    (set-buffer-modified-p nil))
-
+      (let ((from (cdr (assq 'from original-headers)))
+    (date (cdr (assq 'date original-headers)))
+    (start (point)))
+
+ (insert "From: " from "\n")
+ (insert "Date: " date "\n\n")
+      
+ (if plain-parts
+    (mapc 'insert plain-parts)
+  (mapc (lambda (part)
+  (insert (notmuch-mua-parse-html-part part)))
+ html-parts))
+ (push-mark)
+ (goto-char start)
+ (message-cite-original))))
+
+  (push-mark)
+  (set-buffer-modified-p nil)
   (message-goto-body))
 
 (defun notmuch-mua-forward-message ()
diff --git a/test/emacs b/test/emacs
index ac47b16..aecbf41 100755
--- a/test/emacs
+++ b/test/emacs
@@ -268,7 +268,8 @@ Subject: Re: Testing message sent via SMTP
 In-Reply-To: <XXX>
 Fcc: $(pwd)/mail/sent
 --text follows this line--
-On 01 Jan 2000 12:00:00 -0000, Notmuch Test Suite <[hidden email]> wrote:
+Notmuch Test Suite <[hidden email]> writes:
+
 > This is a test that messages are sent via SMTP
 EOF
 test_expect_equal_file OUTPUT EXPECTED
--
1.7.5.4

_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch

[Mark Walters]

On Thu, 19 Jan 2012 10:46:54 -0700, Adam Wolfe Gordon <[hidden email]> wrote:
> 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 this is only a preliminary look so far as I read the code. Note this
is the first time I have tried reviewing a substantial chunk of code so
sorry for any stupidities on my part!

Best wishes

Mark

>  notmuch-reply.c |  271 +++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 231 insertions(+), 40 deletions(-)
>
> diff --git a/notmuch-reply.c b/notmuch-reply.c
> index 0f682db..b4c2426 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)
>  {
> @@ -86,6 +111,17 @@ reply_headers_message_part (GMimeMessage *message)
>      printf ("> Date: %s\n", g_mime_message_get_date_as_string (message));
>  }
>  
> +static notmuch_bool_t
> +reply_check_part_type (GMimeObject *part, const char *type, const char *subtype,
> +       const char *disposition)
> +{
> +    GMimeContentType *content_type = g_mime_object_get_content_type (GMIME_OBJECT (part));
> +    GMimeContentDisposition *part_disposition = g_mime_object_get_content_disposition (part);
> +
> +    return (g_mime_content_type_is_type (content_type, type, subtype) &&
> +    (!part_disposition ||
> +     strcmp (part_disposition->disposition, disposition) == 0));
> +}
>  
>  static void
>  reply_part_content (GMimeObject *part)
> @@ -147,6 +183,63 @@ reply_part_content (GMimeObject *part)
>      }
>  }
>  
> +static void
> +reply_part_start_json (GMimeObject *part, unused (int *part_count))
> +{
> +    if (reply_check_part_type (part, "text", "*", GMIME_DISPOSITION_INLINE))
> + printf ("{ ");
> +}
> +
> +static void
> +reply_part_end_json (GMimeObject *part)
> +{
> +    if (reply_check_part_type (part, "text", "*", GMIME_DISPOSITION_INLINE))
> + printf ("}, ");
> +}
> +
> +static void
> +reply_part_content_json (GMimeObject *part)
> +{
> +    GMimeContentType *content_type = g_mime_object_get_content_type (GMIME_OBJECT (part));
> +    void *ctx = talloc_new (NULL);
> +
> +    /* We only care about inline text parts for reply purposes */
> +    if (reply_check_part_type (part, "text", "*", GMIME_DISPOSITION_INLINE)) {

This seems to be different from the logic in the text output: I think
that inlines all text/* regardless of disposition. I think the JSON
output should include at least as much as the text output as it is easy
for the caller to discard parts.

> + GMimeDataWrapper *wrapper;
> + GByteArray *part_content;
> +
> + printf ("\"content-type\": %s, \"content\": ",
> +       json_quote_str (ctx, g_mime_content_type_to_string (content_type)));
> +
> + wrapper = g_mime_part_get_content_object (GMIME_PART (part));
> + if (wrapper) {
> +    const char *charset = g_mime_object_get_content_type_parameter (part, "charset");
> +    GMimeStream *stream_memory = g_mime_stream_mem_new ();
> +    if (stream_memory) {
> + GMimeStream *stream_filter = NULL;
> + 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"));
> + }
> +
> + if (stream_filter) {

should the if (charset) block be inside the if (stream_filter) block?

> +    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));
> +    g_object_unref (stream_filter);
> + }
> +    }
> +
> +    if (stream_memory)
> + g_object_unref (stream_memory);
> + }
> +    }
> +
> +    talloc_free (ctx);

Does wrapper need to a free/unref somewhere?

> +}
> +
>  /* Is the given address configured as one of the user's "personal" or
>   * "other" addresses. */
>  static int
> @@ -505,6 +598,61 @@ 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,
> +     notmuch_bool_t reply_all)
> +{
> +    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, reply_all);
> +
> +    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,
> @@ -515,8 +663,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);
> @@ -525,62 +671,103 @@ 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, reply_all);
>  
> - 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);
> - }
> + if (!reply)
> +    continue;
>  
> - from_addr = add_recipients_from_message (reply, config, message,
> - reply_all);
> + show_reply_headers (reply);
>  
> - if (from_addr == NULL)
> -    from_addr = guess_from_received_header (config, message);
> + g_object_unref (G_OBJECT (reply));
> + reply = NULL;
>  
> - if (from_addr == NULL)
> -    from_addr = notmuch_config_get_user_primary_email (config);
> + printf ("On %s, %s wrote:\n",
> + notmuch_message_get_header (message, "date"),
> + notmuch_message_get_header (message, "from"));
>  
> - 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);
> + show_message_body (message, format, params);
>  
> - in_reply_to = talloc_asprintf (ctx, "<%s>",
> -     notmuch_message_get_message_id (message));
> + notmuch_message_destroy (message);
> +    }
> +    return 0;
> +}
>  
> - g_mime_object_set_header (GMIME_OBJECT (reply),
> -  "In-Reply-To", in_reply_to);
> +static int
> +notmuch_reply_format_json(void *ctx,
> +  notmuch_config_t *config,
> +  notmuch_query_t *query,
> +  unused (notmuch_show_params_t *params),
> +  notmuch_bool_t reply_all)
> +{
> +    GMimeMessage *reply;
> +    notmuch_messages_t *messages;
> +    notmuch_message_t *message;
> +    const notmuch_show_format_t *format = &format_json;
>  
> - 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);
> +    const char *reply_headers[] = {"from", "to", "subject", "in-reply-to", "references"};
> +    const char *orig_headers[] = {"from", "to", "cc", "subject", "date", "in-reply-to", "references"};
> +    unsigned int hidx;
>  
> - show_reply_headers (reply);
> +    /* Start array of reply objects */
> +    printf ("[");
> +
> +    for (messages = notmuch_query_search_messages (query);
> + notmuch_messages_valid (messages);
> + notmuch_messages_move_to_next (messages))
> +    {
> + message = notmuch_messages_get (messages);
> + reply = create_reply_message (ctx, config, message, reply_all);
> + if (!reply)
> +    continue;
> +
> + /* Start a reply object */
> + printf ("{ \"reply\": { \"headers\": { ");
> +
> + for (hidx = 0; hidx < ARRAY_SIZE (reply_headers); hidx++)
> + {

Nit: I think the preferred style is brace on the same line as the for loop.

> +    if (hidx)
> + 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 ("} }");

If replying to multiple messages (such as a whole thread) you get
multiple sets of "new headers". I think that probably is not what is
wanted but its still better than the weird things the text version
does. Might be worth putting a comment. [What I think should happen is
that a union of all the headers from all these is taken throwing away
duplicate addresses but that is obviously not part of this patch set]

> + /* Start the original */
> + printf (", \"original\": { \"headers\": { ");
> +
> + for (hidx = 0; hidx < ARRAY_SIZE (orig_headers); hidx++)
> + {
> +    if (hidx)
> + 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;
>  }
>  
> @@ -646,6 +833,7 @@ notmuch_reply_format_headers_only(void *ctx,
>  
>  enum {
>      FORMAT_DEFAULT,
> +    FORMAT_JSON,
>      FORMAT_HEADERS_ONLY,
>  };
>  
> @@ -666,6 +854,7 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
>      notmuch_opt_desc_t options[] = {
>   { NOTMUCH_OPT_KEYWORD, &format, "format", 'f',
>    (notmuch_keyword_t []){ { "default", FORMAT_DEFAULT },
> +  { "json", FORMAT_JSON },
>    { "headers-only", FORMAT_HEADERS_ONLY },
>    { 0, 0 } } },
>   { NOTMUCH_OPT_KEYWORD, &reply_all, "reply-to", 'r',
> @@ -684,6 +873,8 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
>  
>      if (format == FORMAT_HEADERS_ONLY)
>   reply_format_func = notmuch_reply_format_headers_only;
> +    else if (format == FORMAT_JSON)
> + reply_format_func = notmuch_reply_format_json;
>      else
>   reply_format_func = notmuch_reply_format_default;
>  
> --
> 1.7.5.4
>
> _______________________________________________
> notmuch mailing list
> [hidden email]
> http://notmuchmail.org/mailman/listinfo/notmuch

_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch

[Mark Walters]

On Thu, 19 Jan 2012 10:46:56 -0700, Adam Wolfe Gordon <[hidden email]> wrote:
> Using the new JSON reply format allows emacs to quote HTML
> parts nicely by using mm-display-part to turn them into displayable
> text, 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.

Hi I have tried using this patch and it did give an identical emacs
buffer when replying to text/plain or multipart/alternative messages
except for two things:

the old one put a line in the reply for each attachment not being
included in the reply (eg "Attachment: file.pdf (application/pdf)" ) and
the new one does not.

and your version does not include text/plain attachments with
disposition attachment (obviously as the cli part does not).

[Note I am not saying whether either of these correct or not, just that
they are changes.]

It also worked nicely on the html only messages I tried it on.

I have not reviewed the lisp yet; I will try and have a look at it but I
am a lisp beginner.

Best wishes

Mark

PS Sorry I sent this from my unsubscribed address first: since I am
using your patch-set my recent reply From: modification wasn't there!
_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch

[Mark Walters]

On Sun, 05 Feb 2012 11:50:12 +0000, Mark Walters <[hidden email]> wrote:

> On Thu, 19 Jan 2012 10:46:54 -0700, Adam Wolfe Gordon <[hidden email]> wrote:
> > 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 this is only a preliminary look so far as I read the code. Note this
> is the first time I have tried reviewing a substantial chunk of code so
> sorry for any stupidities on my part!

After Austin's show modifications (commit 7430a42) I needed the
following patch which is probably trivial but I was only guessing based
on the other change to notmuch-reply Austin made at the time.

Best wishes

Mark

diff --git a/notmuch-reply.c b/notmuch-reply.c
index 9aefce8..1c62b54 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -56,7 +56,7 @@ static const notmuch_show_format_t format_reply = {
 };
 
 static const notmuch_show_format_t format_json = {
-    "",
+    "", NULL,
  "", NULL,
     "", NULL, NULL, "",
     "",


_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch

[Adam Wolfe Gordon]

Thanks for the review. The style nits are things I missed in my
previous cleanup, so thanks for pointing them out. I should probably
run uncrustify and see if it complains about anything else.

The other points are definitely up for discussion, and some are areas
where I was unsure to start with. Discussion inline:

On Sun, Feb 5, 2012 at 04:50, Mark Walters <[hidden email]> wrote:
>> +    /* We only care about inline text parts for reply purposes */
>> +    if (reply_check_part_type (part, "text", "*", GMIME_DISPOSITION_INLINE)) {
>
> This seems to be different from the logic in the text output: I think
> that inlines all text/* regardless of disposition. I think the JSON
> output should include at least as much as the text output as it is easy
> for the caller to discard parts.

Indeed, the text output includes all text/* parts except for
text/html, regardless of disposition. My thought was that it doesn't
really make sense to quote an attachment, or at least it's not the
behavior I would expect. But, perhaps it makes more sense to include
all the text parts, with their dispositions, and let the MUA decide
what it wants to quote. If anyone has thoughts on this I'm happy to
hear them.

> Does wrapper need to a free/unref somewhere?

The text format doesn't free or unref wrapper, so I followed its
example. But, I'm not a gmime expert, and I agree intuitively that it
should be freed somehow. Can anyone enlighten me?

> If replying to multiple messages (such as a whole thread) you get
> multiple sets of "new headers". I think that probably is not what is
> wanted but its still better than the weird things the text version
> does. Might be worth putting a comment. [What I think should happen is
> that a union of all the headers from all these is taken throwing away
> duplicate addresses but that is obviously not part of this patch set]

I've never been sure about what the intended behavior is when replying
to multiple messages in the CLI. My thought was that it should create
a reply to each message, so an MUA could iterate over them allowing
you to compose replies to multiple messages. But, I've never wanted or
used such a feature, so I'm agnostic on whether it's right. The emacs
MUA (at least with my patch) ignores all but the first reply object in
the array, my assumption being that reply only operates on multiple
messages by accident.

Does anyone use reply with multiple messages? If so, what semantics do
you expect?
_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch

[Dmitry Kurochkin]

On Sun, 5 Feb 2012 12:42:12 -0700, Adam Wolfe Gordon <[hidden email]> wrote:

> Thanks for the review. The style nits are things I missed in my
> previous cleanup, so thanks for pointing them out. I should probably
> run uncrustify and see if it complains about anything else.
>
> The other points are definitely up for discussion, and some are areas
> where I was unsure to start with. Discussion inline:
>
> On Sun, Feb 5, 2012 at 04:50, Mark Walters <[hidden email]> wrote:
> >> +    /* We only care about inline text parts for reply purposes */
> >> +    if (reply_check_part_type (part, "text", "*", GMIME_DISPOSITION_INLINE)) {
> >
> > This seems to be different from the logic in the text output: I think
> > that inlines all text/* regardless of disposition. I think the JSON
> > output should include at least as much as the text output as it is easy
> > for the caller to discard parts.
>
> Indeed, the text output includes all text/* parts except for
> text/html, regardless of disposition. My thought was that it doesn't
> really make sense to quote an attachment, or at least it's not the
> behavior I would expect. But, perhaps it makes more sense to include
> all the text parts, with their dispositions, and let the MUA decide
> what it wants to quote. If anyone has thoughts on this I'm happy to
> hear them.
>
> > Does wrapper need to a free/unref somewhere?
>
> The text format doesn't free or unref wrapper, so I followed its
> example. But, I'm not a gmime expert, and I agree intuitively that it
> should be freed somehow. Can anyone enlighten me?
>
> > If replying to multiple messages (such as a whole thread) you get
> > multiple sets of "new headers". I think that probably is not what is
> > wanted but its still better than the weird things the text version
> > does. Might be worth putting a comment. [What I think should happen is
> > that a union of all the headers from all these is taken throwing away
> > duplicate addresses but that is obviously not part of this patch set]
>
> I've never been sure about what the intended behavior is when replying
> to multiple messages in the CLI. My thought was that it should create
> a reply to each message, so an MUA could iterate over them allowing
> you to compose replies to multiple messages. But, I've never wanted or
> used such a feature, so I'm agnostic on whether it's right. The emacs
> MUA (at least with my patch) ignores all but the first reply object in
> the array, my assumption being that reply only operates on multiple
> messages by accident.
>

In some cases, notmuch CLI insists that the search query matches exactly
one message (e.g. notmuch show for parts).  IMO the same behavior makes
sense for notmuch reply.

Regards,
  Dmitry

> Does anyone use reply with multiple messages? If so, what semantics do
> you expect?
> _______________________________________________
> notmuch mailing list
> [hidden email]
> http://notmuchmail.org/mailman/listinfo/notmuch
_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch

[Austin Clements]

Quoth Adam Wolfe Gordon on Jan 19 at 10:46 am:
> 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.

Sorry for coming late to the party.  I really like this idea, but it
seems like your implementation is duplicating a lot of the work of
notmuch show.  This makes me wonder if it would be better to return
reply header information in the JSON (which is definitely the way to
go) but to fetch the part body from the UI via show (and maybe reuse
some of the show-mode logic, if it makes sense to do so).  If this has
already been discussed, just point me at the thread and I'll catch
myself up.
_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch

[Adam Wolfe Gordon]

On Sun, Feb 5, 2012 at 20:44, Austin Clements <[hidden email]> wrote:
> Sorry for coming late to the party.  I really like this idea, but it
> seems like your implementation is duplicating a lot of the work of
> notmuch show.  This makes me wonder if it would be better to return
> reply header information in the JSON (which is definitely the way to
> go) but to fetch the part body from the UI via show (and maybe reuse
> some of the show-mode logic, if it makes sense to do so).  If this has
> already been discussed, just point me at the thread and I'll catch
> myself up.

Thanks for taking a look. Dmitry noted on IRC that inlining the HTML
in JSON could cause issues with non-UTF8 character sets. Right now I'm
working on essentially what you've suggested - having the CLI produce
only headers, and then using show to get the quotable body.

Something else that was mentioned on IRC is using some of the notmuch
show logic to produce the show JSON format as part of reply. I looked
into this, but it would take some serious refactoring (to make the
show JSON stuff accessible to reply), and since emacs will need to end
up calling show anyway, I'm not sure it's worth it. I do like the idea
of different CLI commands being able to produce standardized formats
through some shared interface, I'm just not sure it's necessary here,
and not sure what the interface should look like.
_______________________________________________
notmuch mailing list
[hidden email]
http://notmuchmail.org/mailman/listinfo/notmuch