From: Jani Nikula <jani@nikula.org>
To: Austin Clements <amdragon@MIT.EDU>, notmuch@notmuchmail.org
Subject: Re: [PATCH v5 2/2] show: Introduce mime_node formatter callback
In-Reply-To: <1327361590-19273-3-git-send-email-amdragon@mit.edu>
References: <1326918507-28033-1-git-send-email-amdragon@mit.edu>
	<1327361590-19273-1-git-send-email-amdragon@mit.edu>
	<1327361590-19273-3-git-send-email-amdragon@mit.edu>
User-Agent: Notmuch/0.11+76~g1de742d (http://notmuchmail.org) Emacs/23.3.1
	(i686-pc-linux-gnu)
Date: Tue, 24 Jan 2012 20:26:18 +0200
Message-ID: <87fwf59ctx.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: tomi.ollila@iki.fi
Precedence: list

On Mon, 23 Jan 2012 18:33:10 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> This callback is the gateway to the new mime_node_t-based formatters.
> This maintains backwards compatibility so the formatters can be
> transitioned one at a time.  Once all formatters are converted, the
> formatter structure can be reduced to only message_set_{start,sep,end}
> and part, most of show_message can be deleted, and all of
> show-message.c can be deleted.
> ---
>  notmuch-client.h |    6 ++++++
>  notmuch-reply.c  |    2 +-
>  notmuch-show.c   |   21 +++++++++++++++++----
>  3 files changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/notmuch-client.h b/notmuch-client.h
> index abfe5d3..093c789 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -62,8 +62,14 @@
>  #define STRINGIFY(s) STRINGIFY_(s)
>  #define STRINGIFY_(s) #s
>  
> +struct mime_node;
> +struct notmuch_show_params;
> +
>  typedef struct notmuch_show_format {
>      const char *message_set_start;
> +    void (*part) (const void *ctx,
> +		  struct mime_node *node, int indent,
> +		  const struct notmuch_show_params *params);
>      const char *message_start;
>      void (*message) (const void *ctx,
>  		     notmuch_message_t *message,
> diff --git a/notmuch-reply.c b/notmuch-reply.c
> index bf67960..f55b1d2 100644
> --- a/notmuch-reply.c
> +++ b/notmuch-reply.c
> @@ -31,7 +31,7 @@ static void
>  reply_part_content (GMimeObject *part);
>  
>  static const notmuch_show_format_t format_reply = {
> -    "",
> +    "", NULL,
>  	"", NULL,
>  	    "", NULL, reply_headers_message_part, ">\n",
>  	    "",
> diff --git a/notmuch-show.c b/notmuch-show.c
> index 682aa71..dec799c 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -42,7 +42,7 @@ static void
>  format_part_end_text (GMimeObject *part);
>  
>  static const notmuch_show_format_t format_text = {
> -    "",
> +    "", NULL,
>  	"\fmessage{ ", format_message_text,
>  	    "\fheader{\n", format_headers_text, format_headers_message_part_text, "\fheader}\n",
>  	    "\fbody{\n",
> @@ -89,7 +89,7 @@ static void
>  format_part_end_json (GMimeObject *part);
>  
>  static const notmuch_show_format_t format_json = {
> -    "[",
> +    "[", NULL,
>  	"{", format_message_json,
>  	    "\"headers\": {", format_headers_json, format_headers_message_part_json, "}",
>  	    ", \"body\": [",
> @@ -110,7 +110,7 @@ format_message_mbox (const void *ctx,
>  		     unused (int indent));
>  
>  static const notmuch_show_format_t format_mbox = {
> -    "",
> +    "", NULL,
>          "", format_message_mbox,
>              "", NULL, NULL, "",
>              "",
> @@ -129,7 +129,7 @@ static void
>  format_part_content_raw (GMimeObject *part);
>  
>  static const notmuch_show_format_t format_raw = {
> -    "",
> +    "", NULL,
>  	"", NULL,
>  	    "", NULL, format_headers_message_part_text, "\n",
>              "",
> @@ -850,6 +850,19 @@ show_message (void *ctx,
>  	      int indent,
>  	      notmuch_show_params_t *params)
>  {
> +    if (format->part) {
> +	void *local = talloc_new (ctx);
> +	mime_node_t *root, *part;
> +
> +	if (mime_node_open (local, message, params->cryptoctx, params->decrypt,
> +			    &root) == NOTMUCH_STATUS_SUCCESS &&
> +	    (part = mime_node_seek_dfs (root, (params->part < 0 ?
> +					       0 : params->part))))
> +	    format->part (local, part, indent, params);

I know you did this because Dmitry asked to remove the goto, but I'd
argue this is *much* less readable than the original with goto. There's
just way too much going on in the if. Two calls, an assignment, a
ternary operator. KISS.

BR,
Jani.





> +	talloc_free (local);
> +	return;
> +    }
> +
>      if (params->part <= 0) {
>  	fputs (format->message_start, stdout);
>  	if (format->message)
> -- 
> 1.7.7.3
>