Problem: composing emails when thread_view.preferred_type = html

[yrashk]

When thread_view.preferred_type is set to html, composing a plain text email ends up in wiping out message body after exiting from the editor.

When preferred_type is reverted back to plain, everything works as expected, but we lose the ability to see emails in the mailbox as HTML by default.

Here's a test that confirms this:

  BOOST_AUTO_TEST_CASE (compose_test_body_preferred_html)
  {
    using Astroid::ComposeMessage;
    using Astroid::Message;
    setup ();
    const_cast<ptree&>(astroid->config("thread_view")).put ("preferred_type", "html");

    ComposeMessage * c = new ComposeMessage ();

    ustring bdy = "This is test: æøå.\n > testing\ntesting\n...";

    LOG (trace) << "cm: writing utf-8 text to message body: " << bdy;
    c->body << bdy;

    c->build ();
    c->finalize ();
    ustring fn = c->write_tmp ();

    delete c;

    Message m (fn);

    ustring rbdy = m.viewable_text (false);

    BOOST_CHECK_MESSAGE (bdy == rbdy, "message reading produces the same output as compose message input");

    unlink (fn.c_str ());

    teardown ();

  }

This behaviour happens in Chunk where Chunk queries the configuration and if the type is preferred it sets the preferred property on a chunk to true. This, in turn, is used in Message which is in turn used by ComposeMessage. Since no chunk is marked as preferred, none is used.

Proposed solution: if no chunks is found to be preferred, pick one.

P.S. I am trying to produce a patch to fix this, but this might take a bit of time (the fact that I never really used C++ is not helping me!)

[gauteh]

Hi, thanks! Really good to have a test-case. Would be awesome with a patch! Yurii Rashkovskii writes on July 30, 2018 20:42:

This behaviour happens in Chunk where Chunk queries the configuration and if the type is preferred it sets the `preferred` property on a chunk to `true`. This, in turn, is used in `Message` which is in turn used by `ComposeMessage`. Since no chunk is marked as preferred, none is used. Proposed solution: if no chunks is found to be preferred, pick one. P.S. I am trying to produce a patch to fix this, but this might take a bit of time (the fact that I never really used C++ is not helping me!)

Similar to what you suggest I think the issue is in `Message::viewable_text (...)`. There is no HTML part and it tries to return HTML when that is preferred. If I am correct that function is used only in forward_message and reply_message as well as in compose_message. And every time bool html == false it should always return the plain/text part.

[yrashk]

I've checked, viewable_text is also used in src/modes/thread_view

So far, this is the patch that seems to fix the issue:

diff --git a/src/compose_message.cc b/src/compose_message.cc
index b6a1a25..68b45c2 100644
--- a/src/compose_message.cc
+++ b/src/compose_message.cc
@@ -283,7 +283,7 @@ namespace Astroid {
 
     set_subject (msg.subject);
 
-    body << msg.viewable_text (false);
+    body << msg.viewable_text (false, true);
 
   }
 
diff --git a/src/modes/forward_message.cc b/src/modes/forward_message.cc
index a47adc1..ca6a2fc 100644
--- a/src/modes/forward_message.cc
+++ b/src/modes/forward_message.cc
@@ -67,7 +67,7 @@ namespace Astroid {
         quoted << "Cc: " << AddressList(msg->cc()).str () << endl;
       quoted << endl;
 
-      string vt = msg->viewable_text(false);
+      string vt = msg->viewable_text(false, true);
       quoted << vt;
 
       body = ustring(quoted.str());
diff --git a/src/modes/reply_message.cc b/src/modes/reply_message.cc
index c3b673d..3221d60 100644
--- a/src/modes/reply_message.cc
+++ b/src/modes/reply_message.cc
@@ -56,7 +56,7 @@ namespace Astroid {
     quoted  << quoting_a.raw ()
             << endl;
 
-    string vt = msg->viewable_text(false);
+    string vt = msg->viewable_text(false, true);
     stringstream sstr (vt);
     while (sstr.good()) {
       string line;
diff --git a/tests/test_composed_message.cc b/tests/test_composed_message.cc
index e401dd0..9da7932 100644
--- a/tests/test_composed_message.cc
+++ b/tests/test_composed_message.cc
@@ -8,6 +8,7 @@
 # include "account_manager.hh"
 # include "utils/address.hh"
 # include "utils/ustring_utils.hh"
+# include "db.hh"
 
 BOOST_AUTO_TEST_SUITE(Composing)
 
@@ -54,6 +55,38 @@ BOOST_AUTO_TEST_SUITE(Composing)
 
   }
 
+  BOOST_AUTO_TEST_CASE (compose_test_body_preferred_html)
+  {
+    using Astroid::ComposeMessage;
+    using Astroid::Message;
+    setup ();
+    const_cast<ptree&>(astroid->config("thread_view")).put ("preferred_type", "html");
+
+    ComposeMessage * c = new ComposeMessage ();
+
+    ustring bdy = "This is test: æøå.\n > testing\ntesting\n...";
+
+    LOG (trace) << "cm: writing utf-8 text to message body: " << bdy;
+    c->body << bdy;
+
+    c->build ();
+    c->finalize ();
+    ustring fn = c->write_tmp ();
+
+    delete c;
+
+    ComposeMessage * m = new ComposeMessage ();
+    m->load_message ("id", fn);
+    ustring rbdy(m->body.str());
+
+    BOOST_CHECK_MESSAGE (bdy == rbdy, "message reading produces the same output as compose message input");
+
+    unlink (fn.c_str ());
+
+    teardown ();
+
+  }
+
   BOOST_AUTO_TEST_CASE (compose_test_references)
   {
     using Astroid::ComposeMessage;

What do you think? I've also confirmed the intended behavior in the application, it does now preserve the plain text of the message and sends it correctly but it does NOT show it in the preview before sending (any clues on how to fix that? I suspect that this is also related to the fact that html is preferred but there's obviously none). Previously, neither preview nor actual sent message contained the authored body.

[gauteh]

Yurii Rashkovskii writes on July 31, 2018 3:59:

I've checked, `viewable_text` is also used in `src/modes/thread_view`

Nice catch. It is used for yanking text to the clipboard here, a fix here would probably still work as expected - but we should probably have `fallback_html = true`.

So far, this is the patch that seems to fix the issue: ``` diff --git a/src/compose_message.cc b/src/compose_message.cc index b6a1a25..68b45c2 100644 --- a/src/compose_message.cc +++ b/src/compose_message.cc @@ -283,7 +283,7 @@ namespace Astroid { set_subject (msg.subject); - body << msg.viewable_text (false); + body << msg.viewable_text (false, true); ``` What do you think? I've also confirmed the intended behavior in the application, it does now preserve the plain text of the message and sends it correctly **but** it does NOT show it in the preview before sending (any clues on how to fix that? I suspect that this is also related to the fact that html is preferred but there's obviously none). Previously, neither preview nor actual sent message contained the authored body.

I think that this patch will sometimes put HTML in the text/plain part. Below is a quick patch for always choosing text/plain when html == false. I have not tested this properly, and probably some more fixes are necessary, but I think it addresses the root cause of the problem? ``` diff --git a/src/chunk.cc b/src/chunk.cc index 044aa376..762a8475 100644 --- a/src/chunk.cc +++ b/src/chunk.cc @@ -238,6 +238,10 @@ namespace Astroid { } + bool Chunk::is_content_type (const char * major, const char * minor) { + return (mime_object != NULL) && g_mime_content_type_is_type (content_type, major, minor); + } + ustring Chunk::viewable_text (bool html = true, bool verbose) { if (isencrypted && !crypt->decrypted) { if (verbose) { @@ -258,7 +262,7 @@ namespace Astroid { LOG (debug) << "chunk: body: part"; - if (g_mime_content_type_is_type (content_type, "text", "plain")) { + if (is_content_type ("text", "plain")) { LOG (debug) << "chunk: plain text (out html: " << html << ")"; GMimeDataWrapper * content = g_mime_part_get_content ( @@ -328,7 +332,7 @@ namespace Astroid { content_stream = filter_stream; - } else if (g_mime_content_type_is_type (content_type, "text", "html")) { + } else if (is_content_type ("text", "html")) { LOG (debug) << "chunk: html text"; GMimeDataWrapper * content = g_mime_part_get_content ( diff --git a/src/chunk.hh b/src/chunk.hh index eaea150d..30d224b0 100644 --- a/src/chunk.hh +++ b/src/chunk.hh @@ -30,6 +30,7 @@ namespace Astroid { ustring content_id; ustring get_content_type (); + bool is_content_type (const char* major, const char* minor); ustring viewable_text (bool, bool verbose = false); diff --git a/src/message_thread.cc b/src/message_thread.cc index c1a968da..9ec9fda1 100644 --- a/src/message_thread.cc +++ b/src/message_thread.cc @@ -316,13 +316,13 @@ namespace Astroid { bool use = false; if (c->siblings.size() >= 1) { - if (c->preferred) { + if (c->is_content_type ("text", html ? "html" : "plain")) { use = true; } else { /* check if there are any other preferred */ if (all_of (c->siblings.begin (), c->siblings.end (), - [](refptr<Chunk> c) { return (!c->preferred); })) { + [html](refptr<Chunk> c) { return !c->is_content_type ("text", html ? "html" : "plain"); })) { use = true; } else { use = false; @@ -333,7 +333,7 @@ namespace Astroid { } if (use) { - if (c->viewable && (c->preferred || html || fallback_html)) { + if (c->viewable && (c->is_content_type ("text", html ? "html" : "plain") || fallback_html)) { body += c->viewable_text (html); } ```

[gauteh]

Gaute Hope writes on July 31, 2018 7:37:

Yurii Rashkovskii writes on July 31, 2018 3:59: > What do you think? I've also confirmed the intended behavior in the application, it does now preserve the plain text of the message and sends it correctly **but** it does NOT show it in the preview before sending (any clues on how to fix that? I suspect that this is also related to the fact that html is preferred but there's obviously none). Previously, neither preview nor actual sent message contained the authored body. > I think that this patch will sometimes put HTML in the text/plain part. Below is a quick patch for always choosing text/plain when html == false. I have not tested this properly, and probably some more fixes are necessary, but I think it addresses the root cause of the problem?

I think the problem stems from the assumption that `Chunk::preffered` always equalled "text/plain" earlier. Now it is possible to configure that to also mean "text/html". But there are still spots in the code that relies on this old assumption. When composing we want the "text/plain" part when we call `viewable_text (html = false)`. There is a special case for processing markdown, but still the input text is "text/plain". When forwarding / replying we are also currently only able to quote "text/plain" (or attach the whole message as a MIME-message).

[yrashk]

I can confirm that your patch works as well, however it still doesn't show the text in the preview pane before sending -- but the body is not lost (it can be seen by re-entering into the editor).

Any thoughts on how we can make the preview work?

[gauteh]

Maybe the plain part is not shown when there is no html part and html is preferred? I think this is an issue in either page client or tvextension. tir. 31. jul. 2018 kl. 08:10 skrev Yurii Rashkovskii < notifications@github.com>:

…

I can confirm that your patch works as well, however it still doesn't show the text in the preview pane before sending -- but the body is not lost (it can be seen by re-entering into the editor). Any thoughts on how we can make the preview work? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#532 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADd-7V8b8kuPIdHZ9rJqcID4CadeZdAks5uL_TngaJpZM4Vm9Us> .

[gauteh]

Gaute Hope writes on July 31, 2018 8:38:

Maybe the plain part is not shown when there is no html part and html is preferred? I think this is an issue in either page client or tvextension.

ThreadView (the preview) does not use the same logic to construct the viewable text as Message::viewable_text (...). The ThreadView needs to handle the spefic chunks and parts (and thus calls `Chunk::viewable_text (..)` directly), while ComposeMessage/ForwardMessage/ReplyMessage/etc just need the viewable plain text (with no regard for attachments or MIME structure).

[gauteh]

Gaute Hope writes on July 31, 2018 8:51:

Gaute Hope writes on July 31, 2018 8:38: > Maybe the plain part is not shown when there is no html part and html is > preferred? I think this is an issue in either page client or tvextension. ThreadView (the preview) does not use the same logic to construct the viewable text as Message::viewable_text (...). The ThreadView needs to handle the spefic chunks and parts (and thus calls `Chunk::viewable_text (..)` directly), while ComposeMessage/ForwardMessage/ReplyMessage/etc just need the viewable plain text (with no regard for attachments or MIME structure).

Hi Yurii, did you have a chance to look anymore into this? Cheers, Gaute

[yrashk]

Hi Gaute, Not yet, been busy with other things, but I definitely want to solve it as it clearly affects my workflow :) We just need to sit down and figure out the exact changes to viewable text usage to make this work together with your last suggested patch.

…

On Wed, Aug 8, 2018, 11:27 PM Gaute Hope ***@***.***> wrote: Gaute Hope writes on July 31, 2018 8:51: > Gaute Hope writes on July 31, 2018 8:38: >> Maybe the plain part is not shown when there is no html part and html is >> preferred? I think this is an issue in either page client or tvextension. > > ThreadView (the preview) does not use the same logic to construct the > viewable text as Message::viewable_text (...). The ThreadView needs to > handle the spefic chunks and parts (and thus calls `Chunk::viewable_text > (..)` directly), while ComposeMessage/ForwardMessage/ReplyMessage/etc > just need the viewable plain text (with no regard for attachments or > MIME structure). > Hi Yurii, did you have a chance to look anymore into this? Cheers, Gaute — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#532 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAABxLJw0YdXO2mXvDSkJtOiCmnuX5elks5uOxGKgaJpZM4Vm9Us> .

[gauteh]

Yurii Rashkovskii writes on August 8, 2018 18:34:

Hi Gaute, Not yet, been busy with other things, but I definitely want to solve it as it clearly affects my workflow :) We just need to sit down and figure out the exact changes to viewable text usage to make this work together with your last suggested patch.

Hi Yurii, can you check if #545 fixes the issue? When replying to an HTML only message you will get an empty quote.

[davvil]

#545 solves the issue for me.

[gauteh]

Thanks, closing for now.

[gauteh]

Hm, noticed that I made some other changes.. needs some work before merge.