From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
To: David Edmondson <dme@dme.org>, notmuch@notmuchmail.org
Subject: Re: [PATCH] notmuch/emacs: Observe the charset of encoded parts,
	where known.
In-Reply-To: <cunr4z5kmmu.fsf@hotblack-desiato.hh.sledj.net>
References: <1326279001-28427-1-git-send-email-dme@dme.org>
	<87k44ym6ka.fsf@gmail.com>
	<cun8vldm8q9.fsf@hotblack-desiato.hh.sledj.net>
	<878vldgg3b.fsf@gmail.com>
	<cunr4z5kmmu.fsf@hotblack-desiato.hh.sledj.net>
User-Agent: Notmuch/0.10.2+135~gb811a3c (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Thu, 12 Jan 2012 18:53:45 +0400
Message-ID: <8762ghgefa.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

On Thu, 12 Jan 2012 14:42:49 +0000, David Edmondson <dme@dme.org> wrote:
> On Thu, 12 Jan 2012 18:17:44 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > I think there is a record of useful features and fixes that were not
> > accepted to notmuch because of some implementation issues.  And
> > interested people were using them in private repos for years.  (I do not
> > say that it is always the right thing to do, or that it is the right
> > thing in this particular case.)
> 
> I agree that this has happened. I think that it's a failure of the
> project that it has become common, necessary and generally accepted.
> 
> > I would like to see the following changes:
> > 
> > * Properly handle charset with parameters in Emacs UI.  Currently it is
> >   broken by your patch in one place at least:
> >   `notmuch-show-handlers-for' would produce incorrect results for
> >   content-type string with parameters.  In my patch [1] I did parse the
> >   charset at top level and then changed all usages of it accordingly.
> >   Making `notmuch-show-handlers-for' smarter about parameters may be
> >   sufficient, but I would like to see some more details on why adding
> >   parameters to content-type string does not break Emacs UI code in
> >   other places.
> 
> Your patch modifies the output of 'notmuch show' such that it included
> the full value of the content-type header, which means that it is
> necessary to parse it more carefully in emacs to discover and (as
> necessary) remove the parameters. The patch I posted doesn't do this,
> preferring to pass the charset (if any) as a supplementary parameter and
> leave the content-type as-is. This distinction means that the patch I
> posted isn't broken in the way that you describe.
> 

Sorry, I should have better look at the code.

> > * Add charset parameter for text/html parts only.
> 
> Version 2 of the patch does this.
> 
> > * Use `mail-header-parse-content-type' to parse content-type instead of
> >   contructing the list for `mm-make-handle' manually.
> 
> That's not required, see above.
> 
> > * Add a proper XXX comment to notmuch-show code.
> 
> I'm happy to do that.
> 
> > I cannot say I would be happy about this patch after these changes.
> 
> Can you say why? I agree that it is not a solution to all problems, but
> it is a workable solution to a specific problem.
> 

At the very least, because I did not really review the code, as you
probably understood from my poor comments :)

I do not have a strong preference here.  If others do, I prefer to leave
it for them to decide.

> > It would be a temporary hack anyway.
> 
> Agreed. Do you have any idea when you might be able to spend time on the
> better approach?

I hope to work on this once Austin's notmuch show rewrite is done.  It
is progressing, but I do not have any estimations.

Regards,
  Dmitry