On Thu, 19 Jan 2012 10:46:54 -0700, Adam Wolfe Gordon <awg+notmuch at xvx.ca>
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
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
