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

RESOLVED FIXED in Thunderbird 45.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: masayuki, Assigned: Jorg K (GMT+2))

Tracking

({jp-critical})

Trunk
Thunderbird 45.0
x86
Windows
jp-critical
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Created attachment 8696494 [details]
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.
(Assignee)

Comment 1

2 years ago
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.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Keywords: regression
Version: unspecified → Trunk
(Assignee)

Comment 2

2 years ago
Created attachment 8696504 [details] [diff] [review]
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.
(Assignee)

Comment 3

2 years ago
Created attachment 8696509 [details] [diff] [review]
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
Attachment #8696504 - Attachment is obsolete: true
Attachment #8696509 - Flags: review+
(Assignee)

Updated

2 years ago
Summary: At editing ISO-2022-JP email which has lone lines with "Edit As New Message", the lone lines have extra spaces → At editing ISO-2022-JP email which has long lines with "Edit As New Message", the long lines have extra spaces and are wrapped at 50 characters
(Assignee)

Comment 4

2 years ago
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.
Summary: At editing ISO-2022-JP email which has long lines with "Edit As New Message", the long lines have extra spaces and are wrapped at 50 characters → At editing ISO-2022-JP email which has long lines with "Edit As New Message", the long lines have extra spaces
(Assignee)

Comment 5

2 years ago
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.
Attachment #8696509 - Attachment description: Part 1: Extra hunks from bug 26734 - nits fixed. → Extra hunks from bug 26734 - nits fixed.
Attachment #8696509 - Flags: review?(Pidgeot18)
(Assignee)

Comment 6

2 years ago
(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.
(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.
Attachment #8696509 - Flags: review?(Pidgeot18) → review+
(Assignee)

Comment 8

2 years ago
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.
Keywords: checkin-needed

Comment 9

2 years ago
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

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 45.0
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1230971
(Assignee)

Comment 11

2 years ago
This fix also fixes issues reported in bug 1230971 for forwarded messages.
(Assignee)

Comment 12

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

Attaching EML messages as text/plain makes them readable in Bugzilla ;-)
Attachment #8696494 - Attachment mime type: message/rfc822 → text/plain
(Assignee)

Updated

11 months ago
Depends on: 1287126
You need to log in before you can comment on or make changes to this bug.