1230968 - At editing ISO-2022-JP email which has long lines with "Edit As New Message", the long lines have extra spaces

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: longline-ja-JP.eml

STR:

1. Save the attachment as a .eml file
2. Import it to Daily.
3. Select the email.
4. Right click and choose "Edit As New Message"

Expected Result:

The message doesn't include extra spaces in the long line.

Actual Result:

The message includes (unnecessary) extra spaces in the long line.


I guess that this is not a regression actually if such email came from other MUAs. However, this is *new*, this becomes reproducible *only* with Thunderbird.

Comment 1

[Jorg K (CEST = GMT+2)]

Thanks for the report.

OK, the message you present is encoded in ISO-2022-JP, it is flowed with delsp=yes.

It can't be a regression, since those messages weren't possible in the past. Before ISO-2022-JP messages were forcibly chopped at 36 characters (72 bytes). As you correctly point out, the software never worked which such a message so the bug is pre-existing and could have produced with a message from anther e-mail client.

Looks like the "Edit message as new" doesn't deal with the delsp=yes correctly, that is, it doesn't remove the space again. It also doesn't suppress the wrapping, so you get extra spaces and extra wrapping.

I'll look into it.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: Extra hunks from bug 26734.

These are the two hunks from Hiro's fix to bug 26734 which I didn't carry forward.

When applied, the extra spaces go away, but the lines are still broken at 50 characters. 50, how strange.

See
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=26734&attachment=8537118
for the previous review.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: Extra hunks from bug 26734 - nits fixed.

Carrying forward Joshua's r+ from bug 26734 comment #115.
These are the two hunks from Hiro's original patch. I have fixed the nits from the original review:
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=26734&attachment=8537118

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e8e138ffe390

Comment 4

[Jorg K (CEST = GMT+2)]

OK, the unwanted wrapping is due to bug 1231017 which I just raised. Even in TB 38.4 editing a flowed message loses the flowing. It becomes pretty obvious with this Japanese example as well.

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 8696509 [details] [diff] [review]
Extra hunks from bug 26734 - nits fixed.

OK, removing the "Part 1" from the name, since this is it.

Once again, this code was reviewed and approved here
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=26734&attachment=8537118
in January 2015 and it fixes the problem reported in this bug.

I'm not comfortable landing something I don't understand, so please Joshua, take another look at it.

Perhaps you can explain what Hiro had in mind. Apparently he calls MimeInlineTextPlainFlowed_parse_line which does some extra delsp=yes processing but also a whole lot of other stuff.

Comment 6

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+1) from comment #5)
> Perhaps you can explain what Hiro had in mind. Apparently he calls
> MimeInlineTextPlainFlowed_parse_line which does some extra delsp=yes
> processing but also a whole lot of other stuff.

Actually, it doesn't do a "whole lot of other stuff", it just does
  return obj->options->decompose_file_output_fn(line, ...)
and this call was previously executed in MimeMessage_parse_line.

So I'm more comfortable with what appears a small refactoring.

I'd be interested to know why this code runs for forward and "edit as new", exactly the cases from bug 1231017 where the flowing doesn't work, but not for opening/viewing the message or replying to it, where the flowing does work.

Comment 7

[Joshua Cranmer [:jcranmer]]

(In reply to Jorg K (GMT+1) from comment #6)
> I'd be interested to know why this code runs for forward and "edit as new",
> exactly the cases from bug 1231017 where the flowing doesn't work, but not
> for opening/viewing the message or replying to it, where the flowing does
> work.

If the code you're referring to is the new lines in mimemsg.cpp, then you're only executing that code when obj->options->decompose_file_p is in effect, and that is an option that's set to indicate the use of drafts. I had really detailed notes on all of the MIME options once, but that went up in smoke several months ago, and I've not had the heart to go back and wade through libmime again.

Comment 8

[Jorg K (CEST = GMT+2)]

Let's land this. It's not our favourite piece of code, but it does the job in this case and doesn't cause problems, see comment #3.

Comment 9

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/99eb42a1fd3dba3a2cfaec4bb7a84bf36ebe3c1d
Bug 1230968 - Two hunks from bug 26734, part 3: Need to parse format=flowed message for editing. r=jcranmer

Comment 11

[Jorg K (CEST = GMT+2)]

This fix also fixes issues reported in bug 1230971 for forwarded messages.

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 8696494 [details]
longline-ja-JP.eml

Attaching EML messages as text/plain makes them readable in Bugzilla ;-)