[PATCH] notmuch/emacs: Observe the charset of encoded parts, where known.

[PATCH] notmuch/emacs: Observe the charset of encoded parts, where known.

[David Edmondson]

Add the charset of encoded parts to the JSON output of 'notmuch -show'
when it is known. Observe the encoding when rendering parts in emacs.
---

Domo_ discovered that w3m was being called with an incorrect charset
to render text/html parts. This fixes that.

 emacs/notmuch-show.el |    3 ++-
 notmuch-show.c        |    3 +++
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 5502efd..0354a8e 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -328,7 +328,8 @@ message at DEPTH in the current thread."
 current buffer, if possible."
   (let ((display-buffer (current-buffer)))
     (with-temp-buffer
-      (let ((handle (mm-make-handle (current-buffer) (list content-type))))
+      (let* ((charset (plist-get part :content-charset))
+	     (handle (mm-make-handle (current-buffer) `(,content-type (charset . ,charset)))))
 	(if (and (mm-inlinable-p handle)
 		 (mm-inlined-p handle))
 	    (let ((content (notmuch-show-get-bodypart-content msg part nth)))
diff --git a/notmuch-show.c b/notmuch-show.c
index 0200b9c..3c9c610 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -657,6 +657,7 @@ static void
 format_part_content_json (GMimeObject *part)
 {
     GMimeContentType *content_type = g_mime_object_get_content_type (GMIME_OBJECT (part));
+    const char *content_charset = g_mime_object_get_content_type_parameter (GMIME_OBJECT (part), "charset");
     GMimeStream *stream_memory = g_mime_stream_mem_new ();
     const char *cid = g_mime_object_get_content_id (part);
     void *ctx = talloc_new (NULL);
@@ -664,6 +665,8 @@ format_part_content_json (GMimeObject *part)
 
     printf (", \"content-type\": %s",
 	    json_quote_str (ctx, g_mime_content_type_to_string (content_type)));
+    if (content_charset != NULL)
+	printf (", \"content-charset\": %s", json_quote_str (ctx, content_charset));
 
     if (cid != NULL)
 	    printf(", \"content-id\": %s", json_quote_str (ctx, cid));
-- 
1.7.7.3

Re: [PATCH] notmuch/emacs: Observe the charset of encoded parts, where known.

[Tomi Ollila]

On Wed, 11 Jan 2012 10:50:01 +0000, David Edmondson <dme@dme.org> wrote:
> Add the charset of encoded parts to the JSON output of 'notmuch -show'
> when it is known. Observe the encoding when rendering parts in emacs.
> ---
> 
> Domo_ discovered that w3m was being called with an incorrect charset
> to render text/html parts. This fixes that.

+1, unconditionally :)

> 
>  emacs/notmuch-show.el |    3 ++-
>  notmuch-show.c        |    3 +++
>  2 files changed, 5 insertions(+), 1 deletions(-)
> 

Tomi

Re: [PATCH] notmuch/emacs: Observe the charset of encoded parts, where known.

[Dmitry Kurochkin]

-1

I already tried to solve the above problem using a more general approach
(output all content-type parameters, not just charset) [1].  There was a
lengthy discussion on IRC about it and it was rejected.  The consensus
was that we need to make some more substantial changes to JSON and raw
output formats to properly fix the issue (and the issue is more general
than what this patch fixes, other content-type parameters are useful for
renderers as well and should be included in the output).

One particular issue with your patch is that it adds (incorrect) charset
parameter to plain/text parts which are converted to UTF-8 in JSON
output.

I am planning to work on a proper fix for this issue, but decided to
postpone it until Austin's rewrite of notmuch show is complete.

Regards,
  Dmitry

[1] id:"1321659905-24367-1-git-send-email-dmitry.kurochkin@gmail.com"

Re: [PATCH] notmuch/emacs: Observe the charset of encoded parts, where known.

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]

On Wed, 11 Jan 2012 22:34:45 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> -1

Which puts me back to 0 :-(

> One particular issue with your patch is that it adds (incorrect) charset
> parameter to plain/text parts which are converted to UTF-8 in JSON
> output.

Patches that break things should obviously not be accepted...

> I already tried to solve the above problem using a more general approach
> (output all content-type parameters, not just charset) [1].  There was a
> lengthy discussion on IRC about it and it was rejected.  The consensus
> was that we need to make some more substantial changes to JSON and raw
> output formats to properly fix the issue (and the issue is more general
> than what this patch fixes, other content-type parameters are useful for
> renderers as well and should be included in the output).

...but a useful point fix should not be blocked by theoretical future
work.

> I am planning to work on a proper fix for this issue, but decided to
> postpone it until Austin's rewrite of notmuch show is complete.

If the UTF8 text/plain part issue can be resolved, would you be happier
to accept this as an interim fix whilst we wait for the more complete
solution?

Nothing in the patch (so far) should make your proposed changes any
harder, so I'm not sure what the problem would be.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] notmuch/emacs: Observe the charset of encoded parts, where known.

[Dmitry Kurochkin]

Hi David.

On Thu, 12 Jan 2012 12:00:14 +0000, David Edmondson <dme@dme.org> wrote:
> On Wed, 11 Jan 2012 22:34:45 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > -1
> 
> Which puts me back to 0 :-(
> 
> > One particular issue with your patch is that it adds (incorrect) charset
> > parameter to plain/text parts which are converted to UTF-8 in JSON
> > output.
> 
> Patches that break things should obviously not be accepted...
> 
> > I already tried to solve the above problem using a more general approach
> > (output all content-type parameters, not just charset) [1].  There was a
> > lengthy discussion on IRC about it and it was rejected.  The consensus
> > was that we need to make some more substantial changes to JSON and raw
> > output formats to properly fix the issue (and the issue is more general
> > than what this patch fixes, other content-type parameters are useful for
> > renderers as well and should be included in the output).
> 
> ...but a useful point fix should not be blocked by theoretical future
> work.
> 

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 am planning to work on a proper fix for this issue, but decided to
> > postpone it until Austin's rewrite of notmuch show is complete.
> 
> If the UTF8 text/plain part issue can be resolved, would you be happier
> to accept this as an interim fix whilst we wait for the more complete
> solution?
> 

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.

* Add charset parameter for text/html parts only.

* Use `mail-header-parse-content-type' to parse content-type instead of
  contructing the list for `mm-make-handle' manually.

* Add a proper XXX comment to notmuch-show code.

I cannot say I would be happy about this patch after these changes.  It
would be a temporary hack anyway.  But I would withdraw my -1 and let
others decide.  I would like to see Jameson and Austin input on this
one.

Regards,
  Dmitry

[1] id:"1321659905-24367-1-git-send-email-dmitry.kurochkin@gmail.com"

> Nothing in the patch (so far) should make your proposed changes any
> harder, so I'm not sure what the problem would be.

Re: [PATCH] notmuch/emacs: Observe the charset of encoded parts, where known.

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 2329 bytes --]

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.

> * 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.

> 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?

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] notmuch/emacs: Observe the charset of encoded parts, where known.

[Dmitry Kurochkin]

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

[PATCH v2] notmuch/emacs: Observe the charset of text/html parts, where known.

[David Edmondson]

Add the charset of text/html parts to the JSON output of 'notmuch
-show' when it is known. Observe the encoding when rendering such
parts in emacs.
---

Add the charset only for text/html parts to avoid incorrectly
declaring the charset for JSON inline text/plain parts (as they have
already been converted to UTF-8).

 emacs/notmuch-show.el |    3 ++-
 notmuch-show.c        |   19 ++++++++++++++-----
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 5502efd..0354a8e 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -328,7 +328,8 @@ message at DEPTH in the current thread."
 current buffer, if possible."
   (let ((display-buffer (current-buffer)))
     (with-temp-buffer
-      (let ((handle (mm-make-handle (current-buffer) (list content-type))))
+      (let* ((charset (plist-get part :content-charset))
+	     (handle (mm-make-handle (current-buffer) `(,content-type (charset . ,charset)))))
 	(if (and (mm-inlinable-p handle)
 		 (mm-inlined-p handle))
 	    (let ((content (notmuch-show-get-bodypart-content msg part nth)))
diff --git a/notmuch-show.c b/notmuch-show.c
index 0200b9c..3d05da5 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -675,13 +675,22 @@ format_part_content_json (GMimeObject *part)
 	    printf (", \"filename\": %s", json_quote_str (ctx, filename));
     }
 
-    if (g_mime_content_type_is_type (content_type, "text", "*") &&
-	!g_mime_content_type_is_type (content_type, "text", "html"))
+    if (g_mime_content_type_is_type (content_type, "text", "*"))
     {
-	show_text_part_content (part, stream_memory);
-	part_content = g_mime_stream_mem_get_byte_array (GMIME_STREAM_MEM (stream_memory));
+	if (g_mime_content_type_is_type (content_type, "text", "html"))
+	{
+	    const char *content_charset = g_mime_object_get_content_type_parameter (GMIME_OBJECT (part), "charset");
+
+	    if (content_charset != NULL)
+		printf (", \"content-charset\": %s", json_quote_str (ctx, content_charset));
+	}
+	else
+	{
+	    show_text_part_content (part, stream_memory);
+	    part_content = g_mime_stream_mem_get_byte_array (GMIME_STREAM_MEM (stream_memory));
 
-	printf (", \"content\": %s", json_quote_chararray (ctx, (char *) part_content->data, part_content->len));
+	    printf (", \"content\": %s", json_quote_chararray (ctx, (char *) part_content->data, part_content->len));
+	}
     }
     else if (g_mime_content_type_is_type (content_type, "multipart", "*"))
     {
-- 
1.7.7.3

Re: [PATCH v2] notmuch/emacs: Observe the charset of text/html parts, where known.

[Austin Clements]

Quoth David Edmondson on Jan 12 at  1:31 pm:
> Add the charset of text/html parts to the JSON output of 'notmuch
> -show' when it is known. Observe the encoding when rendering such
> parts in emacs.

This seems like a fine interim solution to me, though I'd like to see
a comment above the change in format_part_content_json explaining why
we include the content charset for text/html and not for anything
else.  Perhaps something like,

/* For non-HTML text/* parts, we include the content in the JSON.
 * Since JSON must be Unicode, we handle charset decoding here and do
 * not report a charset to the caller.  For text/html parts, we do not
 * include the content.  If a caller is interested in text/html parts,
 * it should retrieve them separately.  Since this makes charset
 * decoding the responsibility on the caller, we report the charset
 * for text/html parts. */

Re: [PATCH v2] notmuch/emacs: Observe the charset of text/html parts, where known.

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 555 bytes --]

On Thu, 12 Jan 2012 13:49:59 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> Quoth David Edmondson on Jan 12 at  1:31 pm:
> > Add the charset of text/html parts to the JSON output of 'notmuch
> > -show' when it is known. Observe the encoding when rendering such
> > parts in emacs.
> 
> This seems like a fine interim solution to me, though I'd like to see
> a comment above the change in format_part_content_json explaining why
> we include the content charset for text/html and not for anything
> else.

Okay, thanks. New patch tomorrow.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

[PATCH v3] notmuch/emacs: Observe the charset of text/html parts, where known.

[David Edmondson]

Add the charset of text/html parts to the JSON output of 'notmuch
-show' when it is known. Observe the encoding when rendering such
parts in emacs.
---

Commentary added.

 emacs/notmuch-show.el |    3 ++-
 notmuch-show.c        |   28 +++++++++++++++++++++++-----
 2 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 5502efd..0354a8e 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -328,7 +328,8 @@ message at DEPTH in the current thread."
 current buffer, if possible."
   (let ((display-buffer (current-buffer)))
     (with-temp-buffer
-      (let ((handle (mm-make-handle (current-buffer) (list content-type))))
+      (let* ((charset (plist-get part :content-charset))
+	     (handle (mm-make-handle (current-buffer) `(,content-type (charset . ,charset)))))
 	(if (and (mm-inlinable-p handle)
 		 (mm-inlined-p handle))
 	    (let ((content (notmuch-show-get-bodypart-content msg part nth)))
diff --git a/notmuch-show.c b/notmuch-show.c
index 0200b9c..87a1c90 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -675,13 +675,31 @@ format_part_content_json (GMimeObject *part)
 	    printf (", \"filename\": %s", json_quote_str (ctx, filename));
     }
 
-    if (g_mime_content_type_is_type (content_type, "text", "*") &&
-	!g_mime_content_type_is_type (content_type, "text", "html"))
+    if (g_mime_content_type_is_type (content_type, "text", "*"))
     {
-	show_text_part_content (part, stream_memory);
-	part_content = g_mime_stream_mem_get_byte_array (GMIME_STREAM_MEM (stream_memory));
+	/* For non-HTML text/* parts, we include the content in the
+	 * JSON. Since JSON must be Unicode, we handle charset
+	 * decoding here and do not report a charset to the caller.
+	 * For text/html parts, we do not include the content. If a
+	 * caller is interested in text/html parts, it should retrieve
+	 * them separately and they will not be decoded. Since this
+	 * makes charset decoding the responsibility on the caller, we
+	 * report the charset for text/html parts.
+	 */
+	if (g_mime_content_type_is_type (content_type, "text", "html"))
+	{
+	    const char *content_charset = g_mime_object_get_content_type_parameter (GMIME_OBJECT (part), "charset");
+
+	    if (content_charset != NULL)
+		printf (", \"content-charset\": %s", json_quote_str (ctx, content_charset));
+	}
+	else
+	{
+	    show_text_part_content (part, stream_memory);
+	    part_content = g_mime_stream_mem_get_byte_array (GMIME_STREAM_MEM (stream_memory));
 
-	printf (", \"content\": %s", json_quote_chararray (ctx, (char *) part_content->data, part_content->len));
+	    printf (", \"content\": %s", json_quote_chararray (ctx, (char *) part_content->data, part_content->len));
+	}
     }
     else if (g_mime_content_type_is_type (content_type, "multipart", "*"))
     {
-- 
1.7.7.3

Re: [PATCH v3] notmuch/emacs: Observe the charset of text/html parts, where known.

[Tomi Ollila]

On Fri, 13 Jan 2012 09:44:46 +0000, David Edmondson <dme@dme.org> wrote:
> Add the charset of text/html parts to the JSON output of 'notmuch
> -show' when it is known. Observe the encoding when rendering such
> parts in emacs.
> ---
> 
> Commentary added.

+1

Tomi

Re: [PATCH v3] notmuch/emacs: Observe the charset of text/html parts, where known.

[Austin Clements]

Quoth David Edmondson on Jan 13 at  9:44 am:
> Add the charset of text/html parts to the JSON output of 'notmuch
> -show' when it is known. Observe the encoding when rendering such
> parts in emacs.
> ---
> 
> Commentary added.
> 
>  emacs/notmuch-show.el |    3 ++-
>  notmuch-show.c        |   28 +++++++++++++++++++++++-----
>  2 files changed, 25 insertions(+), 6 deletions(-)

LGTM.

Re: [PATCH v3] notmuch/emacs: Observe the charset of text/html parts, where known.

[David Bremner]

On Fri, 13 Jan 2012 09:44:46 +0000, David Edmondson <dme@dme.org> wrote:
> Add the charset of text/html parts to the JSON output of 'notmuch
> -show' when it is known. Observe the encoding when rendering such
> parts in emacs.
> ---

pushed