Date: Sat, 4 Feb 2012 16:19:09 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
Subject: Re: [PATCH 1/2] show: Convert text format to the new self-recursive
	style
Message-ID: <20120204211909.GN10898@mit.edu>
References: <1327560926-22401-1-git-send-email-amdragon@mit.edu>
	<1327560926-22401-2-git-send-email-amdragon@mit.edu>
	<87obtkojpd.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87obtkojpd.fsf@gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: tomi.ollila@iki.fi, notmuch@notmuchmail.org
Precedence: list

Quoth Dmitry Kurochkin on Jan 31 at  3:26 am:
> On Thu, 26 Jan 2012 01:55:25 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> > This is all code movement and a smidgen of glue.  This moves the
> > existing text formatter code into one self-recursive function, but
> > doesn't change any of the logic.  The next patch will actually take
> > advantage of what the new structure has to offer.
> > 
> > Note that this patch retains format_headers_message_part_text because
> > it is also used by the raw format.
> > ---
> >  notmuch-show.c |  270 +++++++++++++++++++++++++++++---------------------------
> >  1 files changed, 139 insertions(+), 131 deletions(-)
> > 
> > diff --git a/notmuch-show.c b/notmuch-show.c
> > index dec799c..6a890b2 100644
> > --- a/notmuch-show.c
> > +++ b/notmuch-show.c
> > @@ -21,40 +21,17 @@
> >  #include "notmuch-client.h"
> >  
> >  static void
> > -format_message_text (unused (const void *ctx),
> > -		     notmuch_message_t *message,
> > -		     int indent);
> > -static void
> > -format_headers_text (const void *ctx,
> > -		     notmuch_message_t *message);
> > -
> > -static void
> >  format_headers_message_part_text (GMimeMessage *message);
> >  
> >  static void
> > -format_part_start_text (GMimeObject *part,
> > -			int *part_count);
> > -
> > -static void
> > -format_part_content_text (GMimeObject *part);
> > -
> > -static void
> > -format_part_end_text (GMimeObject *part);
> > +format_part_text (const void *ctx, mime_node_t *node,
> > +		  int indent, const notmuch_show_params_t *params);
> >  
> >  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",
> > -	        format_part_start_text,
> > -	        NULL,
> > -	        NULL,
> > -	        format_part_content_text,
> > -	        format_part_end_text,
> > -	        "",
> > -	    "\fbody}\n",
> > -	"\fmessage}\n", "",
> > -    ""
> > +    .message_set_start = "",
> > +    .part = format_part_text,
> > +    .message_set_sep = "",
> > +    .message_set_end = ""
> 
> I guess I missed this during the first review.  I think we should
> support NULL values for message_set_* members (in a separate patch, I
> guess).  This would allow us to explicitly initialize only part member
> in the above code.

I wouldn't want to support this without supporting it for all of the
string members of notmuch_show_format_t, which turns out to be a
fairly big change.  At the end of the show rewrite, all of these other
string members will go away, so I'll add support for just these being
NULL at that point.

> Looks good otherwise.
> 
> Regards,
>   Dmitry