456053 - Quoted text receives hard line breaks in f=f plain-text replies

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[rsx11m]

When replying to a flowed plain-text message in plain-text mode, without
disabling flowed format (f=f) support, the quoted text should be flowed as
well. On trunk, the trailing spaces are removed when sending, thus "hard"
line breaks result for the quote. New text added as well as replying in HTML mode (despite down-conversion to plain text when sending) are fine.

Steps to reproduce:
1. Switch to plain-text composition, make sure f=f is enabled.
2. Write yourself a long e-mail to cause f=f wrapping.
3. Verify that the message is displayed in flowed formatting.
4. Reply to yourself, verify that trailing spaces are present while composing.
5. After sending, the quote in the reply is not flowed.
6. The message source indicates that the trailing spaces were removed.

I've also tried adding spaces manually to the quote when composing, or
replying to a message which in turn has a quote that contains the correct trailing spaces. This is potentially a regression from bug 125928, which was fixed by bug 215068, potentially removing trailing spaces in plain-text compositions now too aggressively.

Observed for Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.1b1pre) Gecko/20080919072627 SeaMonkey/2.0a1pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080919031028 Shredder/3.0b1pre when replying from IMAP accounts or local folders. Trailing spaces are retained on branch versions, the problem was observed on neither Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.17) Gecko/20080829 SeaMonkey/1.1.12 nor Thunderbird 2.0.0.17 (Windows/20080914). No existing bug found.

Comment 1

[rsx11m]

> This is potentially a regression from bug 125928, which was
> fixed by bug 215068, potentially removing trailing spaces in plain-text
> compositions now too aggressively.

On a second thought, those bugs are on HTML composition, whereas the observed behavior here applies to plain-text composition only. While the underlying mechanisms may be the same, please feel free to change dependencies if this doesn't match. 

> I've also tried adding spaces manually to the quote when composing, or
> replying to a message which in turn has a quote that contains the correct
> trailing spaces.

Meaning, this didn't resolve anything and all trailing spaces (manually added
of from previous replies) were removed in plain-text mode.

Comment 2

[Andriy Tkachuk]

Attachment: patch1.0

Please try patch attached. It works for me. Thank you.

BTW - the patch includes the fix to bug 448198 as well (they are very close).

Comment 3

[rsx11m]

Andriy, thanks for the quick fix. This affects the DOM backend, as it seems, thus I don't think I would be able to anticipate any issues this may imply for other components. If you are confident that the patch works, I'd recommend to put it up for review and see if the reviewers have any comments on possible side effects or request more testing.

Comment 4

[rsx11m]

Are you going to flag your patch for review? Otherwise, nothing will happen...

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to comment #0)
> This is potentially a regression from bug 125928, which was
> fixed by bug 215068, potentially removing trailing spaces in plain-text
> compositions now too aggressively.
> 
> Observed for Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.1b1pre)
> Gecko/20080919072627 SeaMonkey/2.0a1pre and Mozilla/5.0 (Windows; U; Windows NT
> 5.1; en-US; rv:1.9.1b1pre) Gecko/20080919031028 Shredder/3.0b1pre when replying
> from IMAP accounts or local folders.

Confirming which bug broke this, or at least narrowing a regression timeframe, would help to validate the proper fix...

Comment 6

[rsx11m]

Per comment #2, this has already been confirmed implicitly by Andriy. If it's easier, this could be resolved as part of bug 448198 rather than separately.

Comment 7

[David Ascher (:davida)]

Andriy, is your patch still applicable?  If so, you should ask for an explicit review.  I'd suggest magnus (mkmelin), as he's also reviewing your other patch in 448198.

(I don't think this blocks tb3, but it looks like a good thing)

Comment 8

[Brett Leber]

Should this regression block tb3? I'm not sure how deeply it bothers other users, but it bothers me (I'm a big proponent of format=flowed). Once my mail client inserts hard line breaks and another user's client does the same, it's a slippery slope before we have hard-to-read messages with a proliferation of orphan lines (long line followed by a line with a few words, followed by a long line...). I'd appreciate an update from Andriy.

Comment 9

[rsx11m]

The related bug 448198 has been taken off the TB3 blocker list today, based on the fact that this is touching Gecko Core code and therefore would need to get into 1.9.1 after their release candidates have started. Also, any patch would need to come with a test (see bug 448198 comment #32 for complete discussion).

I'm wondering if a solution for the reply problem here would be sufficiently self-contained that it might be handled independent from the other bug, judging from the patch this may be the case (but nevertheless pose the same issue with review and approval requirements).

Comment 10

[rsx11m]

(In reply to comment #7)
> Andriy, is your patch still applicable?  If so, you should ask for an explicit
> review.  I'd suggest magnus (mkmelin), as he's also reviewing your other patch
> in 448198.

The patch still applies against comm-central/mozilla-1.9.1, and I don't see anything which would prevent it from also applying to mozilla-central. While this would bitrot the patch for bug 448198 by implementing part of it, this indeed seems to be straight-forward enough to be handled independently from
the other bug. It would be good to fix this for 3.0, with or without 448198.

Comment 11

[Serge Gautherie (:sgautherie)]

(In reply to comment #10)
> While
> this would bitrot the patch for bug 448198 by implementing part of it, this
> indeed seems to be straight-forward enough to be handled independently from
> the other bug.

(Yes, it looks like it would be easier to start with this bug, before bug 448198.)

Comment 12

[chicagolovr]

I don't know if this appropriate or not, because it's focused on TB, but this has been a problem for me since something changed the handling of format=flowed, some time ago.  It is still a problem in SM 2.0 [WinXpSp2]. no extensions.

FWIW, I use HTML in e-mail messages and text only in newsgroups.

Comment 13

[rsx11m]

It's a MailNews Core bug, thus it affects equally Thunderbird and SeaMonkey.

Comment 14

[chicagolovr]

Thanks... I notice I did not say specifically, that my problem is with posting in newsgroups in plain text only.  My experience is the same as Brett Leber's [in newsgroups].

Comment 15

[rsx11m]

The current patch would certainly help by reinstating the behavior on TB 2.0
and SM 1.x, so that may be the fastest way to go. On the other hand, time permitting, extending the logic whether or not to apply f=f to the quoted text may additionally be based on the format of the original message, thus avoiding issues with preformatted/non-f=f text in the source:

  if nsIDocumentEncoder::OutputFormatFlowed is set &&
     ((original message is plain text && original format is f=f) ||
      (original message is HTML && original style is !preformat))
    retain trailing spaces in quote;
  else
    remove trailing spaces from quote;

under the condition that the format details on the replied-to message are available at this level and it is fairly straight-forward to do.

Comment 19

[rsx11m]

Andriy, assuming that you are no longer working on this bug, I'll reset the status back to NEW and to the default assignee. I don't see any comment from
you either here nor in the parent bug for more than a year now...

Hopefully someone else can take it from here.

Comment 20

[Andriy Tkachuk]

Ok with me. And sorry...

Comment 21

[rsx11m]

Hi again, I thought that you had disappeared for good. Thanks for drafting the patch and to get a solution started. I'm hoping for 1.9.3 (TB 3.2) now as the freezes for 1.9.2 are probably in effect already.

Comment 22

[mike.easter]

I recently filed bug 544471 and was told that it was considered to be a duplicate of this bug.

I've read some of the commentary in several bug reports related to format=flowed, some of which go on in the comments for quite a number of years.

I'm an advocate of proper f=f functionality and I was pretty happy with the way Tbird2 worked with some reservations, but I'm very unhappy with the regression of Tbird3 in f=f.

I don't know the first thing about what to do with the patch or whether or not it is appropriate for Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2pre) Gecko/20100119 Lanikai/3.1a1

My current intention is to continue to use Tbird 2.0.0.23 on my linux installs and to patch my WinXP Lanikai/3.1a1 (and also my XP Tbird 3.0.1) if that is possible with this patch and if someone will tell me exactly how to use what is available here   patch1.0 (1.46 KB, patch) 2008-09-22 00:55 PDT, Andriy Tkachuk - Index: content/base/src/nsPlainTextSerializer.cpp

I think I also understand that this problem/regression is going to be very difficult to solve comprehensively because it involves the significant upgrades to the core since Tbird2.

Comment 23

[rsx11m]

There are two problems: One is to provide a patch which resolves the issue, and the one Andriy posted seems to revert to the TB 2.0 behavior. The other task is to provide a set of tests to avoid future regressions of the code, i.e., a list of all scenarios "with input (A) and settings (B), the output (C) is expected" would be needed for which /someone/ can write respective tests. At least for the trailing spaces issue here, it appears to be the more significant problem to come up with a proper test set. The related bug 448198 is substantially more complicated and covers other encoding issues, tricky to solve and to test for.

If you have some deeper insights in how RFC 3676 is supposed to be applied in the various cases, and if you can contribute such an A/B/C list of scenarios, it would sure be appreciated here and in bug 448198.

> and to patch my WinXP Lanikai/3.1a1 (and also my XP Tbird 3.0.1) if that is
> possible with this patch and if someone will tell me exactly how to use what is
> available here

Since this affects a C++ source file, there is no simple way to just modify an existing executable with that patch. Thus, you would have to recompile it from the sources. Look at https://developer.mozilla.org/en/Simple_Thunderbird_build providing some general instructions on how to get the source and compile it.
Use "comm-central" for a 3.2 build, "comm-1.9.1" for a 3.0 build. Once you have the sources, go into the "mozilla/" directory and apply the posted patch with "patch -p0 < bug456053fix.patch" (you will get an error if it has bitrotted, but maybe you are lucky). Then configure and compile as shown in the article.

Comment 24

[Alessandro Vesely]

(In reply to comment #23)
> provide a set of tests to avoid future regressions of the code, i.e., a list
> of all scenarios "with input (A) and settings (B), the output (C) is expected"
> would be needed for which /someone/ can write respective tests. At least for
> the trailing spaces issue here, it appears to be the more significant problem
> to come up with a proper test set.

I'm not aware of what tests already exist, but IMHO they should only concern space-stuffing. RFC 3676 says that an agent should "trim spaces before user-inserted hard line breaks", but think this should be done /before/ sending, i.e. as the user types. According to the least surprise principle, only unambiguous transformations, such as space stuffing, should take place after the user hit send. Otherwise, a "preview" function would be needed  --really a nuisance for plain text.

Perhaps it may be useful to display a sign after any end-of-line spaces. "Edit/Select all" already allows the user to see whether spaces are present, but not while typing.

The only workaround I've found is to unwrap quoted text onto a single long line.

BTW, TB3 also introduced breaking lines at "/" and maybe some other characters. Particularly inconvenient for URLs, which I'd prefer to remain on a single line. RFC 3676 allows them. While not discussed, the RFC's production rules also provide for fixed lines in the middle of f=f bodies, for example for tables or ascii-art. (The "Toggle Word Wrap" add-on only works on the whole message.)

Comment 25

[mike.easter]

I don't really follow or understand the developer discussions here; but as regards a 'test', I can very easily see/ test/ demonstrate/ illustrate/ (for my own viewing) what is wrong with v.3 in terms of stripping trailing spaces.

My experiences are with news: 

If I reply to a message with v.2 which message has a citation which has trailing spaces present (I can see those trailing spaces in the existing message by using ctrl-U to show the source and I can also see those same trailing spaces in the citation of my reply), I can see that v.2 maintains all of those trailing spaces in my reply.

If I reply to a message with v.3 which message has a citation with trailing spaces, I can see that v.3 strips all of those trailing spaces as I edit my reply.  Anyone developing an editing program for f=f with a runtime function would be able to quickly see that same effect.

Naturally neither v.2 nor v.3 can 'recreate' any missing or stripped trailing spaces either caused by the original agent posting without or caused by v.3 stripping some f=f agent's trailing spaces.

The issue of 'space stuffing' is of course not related to trailing spaces but leading spaces - and space stuffing issues are IMO a very minor issue when contrasted with the huge major problem of v.3 stripping all of those trailing spaces.

v.2 also has problems or a feature with f=f which are seen when using the 'rewrap' function during editing.  A rewrapped citation is stripped of its trailing spaces.

v.2 also lacks the feature of v.3 to be able to reply to selected text of a previous message.  Of course, when replying to selected text of a previous message, v.3 will strip the existing trailing spaces from the selected text.

Comment 26

[rsx11m]

Dan, Blake, Serge: Can you comment on what /exactly/ is required for the tests, not in the technical sense but with respect to the scenarios needed?

Comment 27

[mike.easter]

I'm noticing that this bug is persistent in messages posted by Shredder/3.2a1pre Mnenhy/0.7.6.666.

The specific bug of which I am speaking is when Shredder replies to a format=flowed message which of course has trailing spaces within a paragraph -- Shredder noncompliantly strips all of the trailing spaces and destroys what would have otherwise have been an ongoing f=f conversation.

Comment 28

[mike.easter]

Version 3.1 RC2 windows

The problem persists in this version.

When editing a message with f=f quotes, the trailing spaces in the cited material are retained, still there, during the editing process, but when the message is sent, the received message has those cited lines' trailing spaces stripped somehow.

That is baffling to me.

The proper handling of f=f can be seen in every version of Eudora since 4.1 except the new E8 based on Tbird.  It can also be seen in open source newsreaders such as mutt which uses vim or vi for its editor.  That is, vim knows format=flowed.  And of course it was handled correctly in Tbird 2.

Why don't the Tbird developers use the editor code or algorithms for f=f plaintext which is well established?  Or restore the compliance which was present in Tb2?  Why should one have to go thru' some kind of html hocus pocus to get plaintext f=f?

Comment 29

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 339746 [details] [diff] [review]
patch1.0

Patch still applies!

$ patch -p0 --dry-run < ~/Desktop/p456053.diff 
patching file content/base/src/nsPlainTextSerializer.cpp
Hunk #1 succeeded at 1647 (offset -44 lines).

Comment 30

[mike.easter]

Version 3.1.7 windows

The problem persists.

When replying to a f=f message which has trailing spaces, the trailing spaces are stripped, which is non-compliant.

Comment 31

[Alessandro Vesely]

With Thunderbird 6 the problem still persists.

I've got used to set

  mailnews.send_plaintext_flowed false

and reflow (Ctrl-R) paragraphs as needed when replying.  This setting also avoids inconsistent space-stuffing when manually indenting for block-quote or ascii-art in messages destined to lists (see Bug 609908 Comment 3.)

Comment 32

[Blake Winton (:bwinton) (:☕️)]

(In reply to rsx11m from comment #26)
> Dan, Blake, Serge: Can you comment on what /exactly/ is required for the
> tests, not in the technical sense but with respect to the scenarios needed?

Hmm, I wonder why I didn't see this before…

For the tests, as Philor says in bug 448198 comment 37, what we need are a list of various strings, and what the spec says they should turn into.  (Or, "a single comprehensive list saying that "- --" in format X quoted into format Y must be output as "(whatever it should be)" and in format Y quoted into format Y must be whatever else"

We can probably find someone to turn that large list into code that will automatically test our editor, and hopefully then get someone to finish writing the patch that actually fixes all the cases.

Thanks,
Blake.

Comment 33

[mike.easter]

With Thunderbird 8 the problem persists, Tbird strips trailing spaces even if they are manually placed there in quoted text.  Why would it do that?

Tbird 'respects' trailing spaces in unquoted body content and/but/then strips trailing spaces in quoted content. That means it has to be doing that stripping with a 'positive' action, 'intentionally'. I don't understand. It distinguishes the quoted text from the unquoted text and strips the trailing spaces from quoted but not original, unquoted.

Other newsreaders which have less numbers of developers can perform f=f properly, such as Opera, MesNews, Gravity, maybe Claws.  Why can't Tbird be fixed so that it will work again like it did in Tb2?


Mike Easter

Comment 35

[rsx11m]

Bug 1302674 contains a couple of test cases if needed.

Comment 36

[Miguel Negrão]

This issue imho makes thunderbird unusable for format=flowed plain text workflow. 

I can confirm that not removing the trailing space in the quoted text would fix this. That is generating instead of

> 1 test testtesttest test testtesttest test testtesttest test\n\r
> testtesttest test testtesttest test testtesttest test testtesttest test\n\r
> testtesttest test testtesttest test testtesttest testEND.\n\r

this:

> 1 test testtesttest test testtesttest test testtesttest testSPACE\n\r
> testtesttest test testtesttest test testtesttest test testtesttest testSPACE\n\r
> testtesttest test testtesttest test testtesttest testEND.\n\r

The original example fixed by hand flows the quoted text correctly in thunderbird. See new attch in 1302674.

Comment 37

[Jorg K (CEST = GMT+2)]

Attachment: patch1.0 (refreshed).

I've refreshed the patch just to see what it does.
It causes absolute havoc, so a clear f-.

Comment 38

[Jorg K (CEST = GMT+2)]

Attachment: patch1.0 (refreshed).

Sorry, I got this wrong. Now it's properly refreshed and still causes havoc.

Comment 39

[rsx11m]

Thanks, I was about to unbitrot and try the patch myself but you beat me to that.

So, is this bug supposed to go to Core/Serializers instead of MailNews Core?
That would be the apparent choice unless we can solve it "locally" for TB/SM.

Comment 40

[Jorg K (CEST = GMT+2)]

Attachment: Manually corrected message with spaced added.

(In reply to rsx11m from comment #39)
> Thanks, I was about to unbitrot and try the patch myself but you beat me to
> that.
Please check my work, I make mistakes. But as far as I can see, the patch is no good at all.

> So, is this bug supposed to go to Core/Serializers instead of MailNews Core?
> That would be the apparent choice unless we can solve it "locally" for TB/SM.
That depends on where the problem is. I know for a fact that Mailnews does space manipulation, for example here:
https://dxr.mozilla.org/comm-central/rev/c482d8a2cd6178a9e5990c07b6c7c09dc63d919d/mailnews/compose/src/nsMsgCompose.cpp#4558

I haven't looked at it at all. Let's see.

Say you start with a flowed message (lines shortened, "|" not part of the message):
|test test test |
|test test test |
|test test test |
|test test test |

When you reply, you see this on the screen and in the sent message:
|> test test test|
|> test test test|
|> test test test|
|> test test test|

OK, I added the spaces by hand in the composition window and sent the message. In the sent message, I still see the same, trailing spaces removed.

Next I edited the sent message (.eml file) to add the spaces (see attachment). Loading the message back into TB, it behaves OK, so I suppose that's the desired result.

Oh, I just noticed. If you do a HTML reply instead of a plaintext reply and chose delivery format plaintext, then it all works, the resulting plaintext message has a space at the end of each line.

Comment 41

[Miguel Negrão]

I'm really clueless about the details, but why does thunderbird not do as geary and apple mail:

|Content-Type: text/plain; charset=utf-8; format=flowed
|Content-Transfer-Encoding: quoted-printable
|
|
|
|> 1 test testtesttest test testtesttest test testtesttest test=20|
|> testtesttest test testtesttest test testtesttest test testtesttest=20|
|> test testtesttest test testtesttest test testtesttest testEND.|

Is there a technical reason not to encode the message this way ?

Comment 42

[Jorg K (CEST = GMT+2)]

Yes, I make mistakes. I was running that on my test profile where I had mailnews.wraplength set to 0 so I saw some unexpected effects. With that corrected, there is no havoc caused by the patch, it simply doesn't work.

The patch tries to suppress trimming
  stringpart.Trim(" ", false, true, true);
in some cases. Well, I printed out 'stringpart' and it has no trailing space to start with, so trimming or not makes no difference.

Maybe the patch worked at some stage in 2008, but now it doesn't. Maybe this .Trim() is part of the problem. In fact, if I add the spaces manually to the compose window and then run with the patch, the added spaces are not trimmed any more and you get the desired result.

So I can see at least two problems here:
1) Trailing spaces already missing in the compose window.
2) If added manually, they are trimmed away, which the patch seems to address.

Now
-      if (mFlags & nsIDocumentEncoder::OutputFormatFlowed) {
+      if ((mFlags & nsIDocumentEncoder::OutputFormatFlowed) &&
+          mCiteQuoteLevel == 0 && stringpart[0] != '>') {
         if ((outputLineBreak || !spacesOnly) && ...
seems to suppress that trimming when the string begins with a '>'. But why the mCiteQuoteLevel == 0? What should happen to lines like:
|>> this is the reply to a reply |
|>> which should still be flowed.|

Try HTML mail sent as plaintext and see that it works fine there. See bottom of comment #40.

Also, you can't get a patch through review that you can't explain to the reviewer. "It works for me." (comment #2) is not a good explanation. I know of a case where someone sent a one-line patch which got landed and caused many years of regressions until I ripped out that one-line hack and fixed the problem correctly (details in demand).

Also, I'm not sure that outputLineBreak isn't simply wrong in this case and the fix would be correcting it further up.

In summary, this would need a good few hours/days of thorough analysis since there is no one in Mozilla or the TB team who understands the plaintext encoder.

Comment 43

[Jorg K (CEST = GMT+2)]

(In reply to Miguel Negrão from comment #41)
> I'm really clueless about the details, but why does thunderbird not do as
> geary and apple mail:
> |Content-Type: text/plain; charset=utf-8; format=flowed
> |Content-Transfer-Encoding: quoted-printable
We don't need quoted-printable.

This
|> 1 test testtesttest test testtesttest test testtesttest test |
|> testtesttest test testtesttest test testtesttest test testtesttest |
|> test testtesttest test testtesttest test testtesttest testEND.|
would be fine.

> Is there a technical reason not to encode the message this way ?
There is no technical reason. It's called a bug. A software fault. In a module that is un-owned and unmaintained. If you want to fix it or pay for someone to fix it, step forward. This is free software, you get what you pay for or do yourself.

Comment 44

[Miguel Negrão]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #43)

> > Is there a technical reason not to encode the message this way ?
> There is no technical reason. It's called a bug. A software fault. In a
> module that is un-owned and unmaintained. If you want to fix it or pay for
> someone to fix it, step forward. This is free software, you get what you pay
> for or do yourself.

I think you misunderstood me, I meant to ask if there was a technical reason not to use quoted-printable, but from your answer I see quoted-printable is not needed, and is probably not related to this issue.
Yes, I'm aware of how free software works, I help maintain free software too.

Comment 45

[rsx11m]

The quoted-printable CTE is an ancient workaround from those times where some SMTP servers couldn't handle 8-bit data but only strict 7-bit ASCII. It is no longer needed for essentially all current servers that are out there. The reason why it still sticks around is mainly to provide a (wrong) way to break sent lines to adhere to a line limit without using the f=f mechanism (see bug 387687).

Comment 46

[Miguel Negrão]

>Now
>-      if (mFlags & nsIDocumentEncoder::OutputFormatFlowed) {
>+      if ((mFlags & nsIDocumentEncoder::OutputFormatFlowed) &&
>+          mCiteQuoteLevel == 0 && stringpart[0] != '>') {
>         if ((outputLineBreak || !spacesOnly) && ...
>seems to suppress that trimming when the string begins with a '>'. But why the mCiteQuoteLevel == 0? >What should happen to lines like:
>|>> this is the reply to a reply |
>|>> which should still be flowed.|

I think the spec says that flowing of quoted lines of any depth is mostly the same as non quoted lines, except that a change in quote depth should also have hard break (without space before CRLF)).

https://www.ietf.org/rfc/rfc2646.txt

   On reception, if a change in quoting depth occurs on a flowed line,
   this is an improperly formatted message.  The receiver SHOULD handle
   this error by using the 'quote-depth-wins' rule, which is to ignore
   the flowed indicator and treat the line as fixed.  That is, the
   change in quote depth ends the paragraph.

   For example, consider the following sequence of lines (using '*' to
   indicate a soft line break, i.e., SP CRLF, and '#' to indicate a hard
   line break, i.e., CRLF):

      > Thou villainous ill-breeding spongy dizzy-eyed*
      > reeky elf-skinned pigeon-egg!*     <--- problem ---<
      >> Thou artless swag-bellied milk-livered*
      >> dismal-dreaming idle-headed scut!#
      >>> Thou errant folly-fallen spleeny reeling-ripe*
      >>> unmuzzled ratsbane!#
      >>>> Henceforth, the coding style is to be strictly*
      >>>> enforced, including the use of only upper case.#
      >>>>> I've noticed a lack of adherence to the coding*
      >>>>> styles, of late.#
      >>>>>> Any complaints?#

   The second line ends in a soft line break, even though it is the last
   line of the one-deep quote block.  The question then arises as to how
   this line should be interpreted, considering that the next line is
   the first line of the two-deep quote block.

Comment 47

[Jorg K (CEST = GMT+2)]

(In reply to rsx11m from comment #45)
> The quoted-printable CTE is an ancient workaround from those times where
> some SMTP servers couldn't handle 8-bit data but only strict 7-bit ASCII.
Just for the record, TB also encodes in QP in some cases:
https://dxr.mozilla.org/comm-central/search?q=isUsingQP&redirect=false

Comment 48

[Alessandro Vesely]

Thank you guys for the good work you're after.  Please document thoroughly how f=f implementation is designed.  For example, has it ever been a design feature to prevent users from generating quotations by simply typing '>'?

(In reply to Jorg K (GMT+2, PTO during summer) from comment #47)
> (In reply to rsx11m from comment #45)
> > The quoted-printable CTE is an ancient workaround from those times where
> > some SMTP servers couldn't handle 8-bit data but only strict 7-bit ASCII.
> Just for the record, TB also encodes in QP in some cases:
> https://dxr.mozilla.org/comm-central/search?q=isUsingQP&redirect=false

For another use case, 7-bit is required by DKIM specs, at least in theory, thereby implying QP or base64:
https://tools.ietf.org/html/rfc6376#section-5.3

Comment 49

[Jorg K (CEST = GMT+2)]

I'll take a further look at this now.

Comment 50

[Jorg K (CEST = GMT+2)]

Attachment: Patch M-C part.

I think the patch for M-C should just be this. If we find a ">" at the beginning of the line, don't trim the line. I'm not sure about mCiteQuoteLevel, I dumped it out and in my testing it always came out as 0. So I don't know why Andriy Tkachuk was testing it for zero in his original patch.

There is still a C-C Mailnews patch necessary to add the spaces to the compose window (see comment #42 "two problems").

Comment 51

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2) from comment #50)
> There is still a C-C Mailnews patch necessary to add the spaces to the
> compose window (see comment #42 "two problems").
Not right. Mailnews digs out the trailing spaces from the message alright, but they got lost here:
https://dxr.mozilla.org/comm-central/rev/9bd937fc0c12bc6b04247f0da5e1f140bd1441de/mailnews/base/util/nsMsgUtils.cpp#2485
I'm onto it in case anyone cares and follows this bug.

Comment 52

[Jorg K (CEST = GMT+2)]

Comment on attachment 8795331 [details] [diff] [review]
Patch M-C part.

One line review.

What we're trying to achieve is pretty obvious. If we have a line 
|> this is a line ending with a space |
where the space does the format=flowed for us, we don't want to trim that space away.

Comment 53

[Jorg K (CEST = GMT+2)]

Attachment: Patch C-C part.

The call to ConvertToPlainText() was changed in bug 1230970:
https://hg.mozilla.org/comm-central/rev/3852741463e7#l1.26
I have to admit that I got it wrong. No one noticed since there was a second problem in M-C. So here I'm correcting the C-C part. Basically we still need to pass in the "flowed" parameter correctly.

Comment 54

[Jorg K (CEST = GMT+2)]

Attachment: Patch C-C part.

(Patch was missing the ---a and +++b lines, so here's a good one.)

Comment 55

[Miguel Negrão]

(In reply to Jorg K (GMT+2) from comment #51)
> I'm onto it in case anyone cares and follows this bug.

I'm not contributing at the moment but I definitely care and follow this bug. This affects some 70% of the emails I send... :-).

Comment 56

[Jorg K (CEST = GMT+2)]

Comment on attachment 8795331 [details] [diff] [review]
Patch M-C part.

Magnus' review queue is pretty long, so let's shift this to Aceman.

Comment 57

[Jorg K (CEST = GMT+2)]

Comment on attachment 8795738 [details] [diff] [review]
Patch C-C part.

Changes in this code:

First change (bug 653342):
https://hg.mozilla.org/comm-central/rev/c6c9a8e486b7#l8.88

Second change (bug 1230970):
https://hg.mozilla.org/comm-central/rev/3852741463e7#l1.26

So now I'm re-establishing a bit of what UseFormatFlowed()/GetSerialiserFlags() used to do. We need to pass whether it's flowed or not.

Comment 58

[Jorg K (CEST = GMT+2)]

Before anyone asks: Test is coming ;-)

Comment 59

[Jorg K (CEST = GMT+2)]

Attachment: C-C Test.

Comment 60

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

Comment on attachment 8795331 [details] [diff] [review]
Patch M-C part.

I don't like you to add comment in the if condition. Instead, could you create 2 static inline method named as IsQuotedLine(const nsAString&) or something similar? Then, use it here, <https://dxr.mozilla.org/mozilla-central/rev/b1d60f2f68c7cccc96fcf9a2075bb430a500a0f2/dom/base/nsPlainTextSerializer.cpp#1585> and <https://dxr.mozilla.org/mozilla-central/rev/b1d60f2f68c7cccc96fcf9a2075bb430a500a0f2/dom/base/nsPlainTextSerializer.cpp#1663>? Then, they must be easier to understand.

For example,

static inline bool
IsQuotedLine(const nsAString& aLine)
{
  return !aLine.IsEmpty() && aLine.First()== char16_t('>');
}


And could you add automated test into dom/base/test/TestPlainTextSerializer.cpp?

Comment 61

[Jorg K (CEST = GMT+2)]

Attachment: Patch M-C part (v2).

Masayuki-san, thanks for the quick turnaround and your input. I hope I implemented your suggestions the way you had them in mind.

Comment 62

[Jorg K (CEST = GMT+2)]

Attachment: Patch M-C part (v2).

(Oops, forgot to refresh the patch before attaching it.)

Comment 63

[Jorg K (CEST = GMT+2)]

Attachment: Patch M-C part (v2).

(Sorry about the noise. Added missing space before ==)

Comment 64

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

Comment on attachment 8796168 [details] [diff] [review]
Patch M-C part (v2).

Thank you very much!

Comment 65

[Jorg K (CEST = GMT+2)]

Masayuki-san, thanks again for the quick turnaround.

Aleth, could you push the M-C part to inbound.

Comment 66

[aleth [:aleth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/18faa8438eebdf4b25f37b54f645087585b7f5fa
Bug 456053 - Don't trim trailing space of quoted lines. r=masayuki

Comment 67

[Jorg K (CEST = GMT+2)]

Comment on attachment 8796168 [details] [diff] [review]
Patch M-C part (v2).

Too late for feedback, already on the way to M-C ;-)

Comment 68

[Jorg K (CEST = GMT+2)]

Attachment: Patch C-C part (v1b).

Updated comment.

Comment 69

[:aceman]

Comment on attachment 8796168 [details] [diff] [review]
Patch M-C part (v2).

Review of attachment 8796168 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsPlainTextSerializer.h
@@ +111,5 @@
>    }
>  
> +  inline bool IsQuotedLine(const nsAString& aLine)
> +  {
> +    return !aLine.IsEmpty() && aLine.First() == char16_t('>');

I don't like such cast, there surely is a method on nsAString to check a character the the beginning. But if this is the style in m-c then no problem.

Comment 70

[:aceman]

Comment on attachment 8795846 [details] [diff] [review]
C-C Test.

Review of attachment 8795846 [details] [diff] [review]:
-----------------------------------------------------------------

Would it make sense to use this same test file and run it through the same actions but not with format flowed set and check the spaces were removed?

::: mail/test/mozmill/composition/format-flowed.eml
@@ +1,2 @@
> +To: [e-mail]
> +From: [e-mail]

test.com seems to be a valid domain. Please try @test.invalid .

::: mail/test/mozmill/composition/test-reply-format-flowed.js
@@ +50,5 @@
> +
> +  // Check for a single line that contains text and make sure there is a
> +  // space at the end. Cater for both CRLF and LF line endings.
> +  if (!messageContent.includes
> +      ("> text text text text text text text text text text text text text text \n")

Can you test also a newline on the start of the line so you match a full line and not some substring?
E.g. messageContent.includes("\n> text ....")

@@ +54,5 @@
> +      ("> text text text text text text text text text text text text text text \n")
> +      &&
> +      !messageContent.includes
> +      ("> text text text text text text text text text text text text text text \r\n")) {
> +    assert_true(false, "Expected line not found in message.");

assert_true(false) ? Can this be rewritten?

E.g.
let expectedContents = messageContent.includes() || messageContent.includes()
assert_true(expectedContents, ...);

Comment 71

[:aceman]

Comment on attachment 8796234 [details] [diff] [review]
Patch C-C part (v1b).

Review of attachment 8796234 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +2959,5 @@
> +    if (pPrefBranch) {
> +      pPrefBranch->GetBoolPref("mailnews.send_plaintext_flowed", &flowed);
> +    }
> +
> +    ConvertToPlainText(flowed,

Should we check the return value here? Does it make sense to continue if this fails?

Comment 72

[Jorg K (CEST = GMT+2)]

Attachment: Patch C-C part (v1c).

Checking return value.

Comment 73

[Jorg K (CEST = GMT+2)]

Attachment: Patch C-C part (v1c).

(forgot refresh.)

Comment 74

[Jorg K (CEST = GMT+2)]

Attachment: C-C Test (v2).

I hope this satisfies you. IIRC, messages have CRLF on all platforms, so no need to test for both.

Comment 75

[:aceman]

Comment on attachment 8796342 [details] [diff] [review]
C-C Test (v2).

Review of attachment 8796342 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks. Do not forget to change the r= line in the other patch.

Comment 76

[Jorg K (CEST = GMT+2)]

Thanks for the quick review and the hint, much appreciated.
I also changed NS_ENSURE_SUCCESS(rv,rv); to NS_ENSURE_SUCCESS(rv, rv);
This will land when the M-C part has been merged.

Comment 77

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/18faa8438eeb

Comment 78

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/bc84637f9388
https://hg.mozilla.org/comm-central/rev/decaccbe1d8a

Comment 79

[Jorg K (CEST = GMT+2)]

Hmm, https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=decaccbe1d8ab52ceb093be2a84b06c252d143a4 crashed on every single platform but Linux 64 opt. And they all crash in the new test test-reply-format-flowed.js.

Test passes nicely locally.

Comment 80

[Jorg K (CEST = GMT+2)]

I talked to Aceman on IRC. "Send later" seems to cause crashes, see bug 1304092.
The symptom is:
ERROR: GetDiskSpaceAvailable: STATFS call FAILED.
Call to GetDiskSpaceAvailable FAILED!

Aceman suggested to create the outbox before (quote):
because the problem is that the backend does not create the file automatically fast enough.

Comment 81

[Jorg K (CEST = GMT+2)]

Attachment: 456053-followup.patch

Follow-up to avoid crashes due to bug 1298081.

Comment 82

[aleth [:aleth]]

(In reply to Jorg K (GMT+2) from comment #80)
> I talked to Aceman on IRC. "Send later" seems to cause crashes, see bug
> 1304092.
> The symptom is:
> ERROR: GetDiskSpaceAvailable: STATFS call FAILED.
> Call to GetDiskSpaceAvailable FAILED!
> 
> Aceman suggested to create the outbox before (quote):
> because the problem is that the backend does not create the file
> automatically fast enough.

This sounds suspiciously similar to our current #1 mozmill failure in Bug 1298081...

Comment 83

[aleth [:aleth]]

(In reply to Jorg K (GMT+2) from comment #81)
> Created attachment 8796718 [details] [diff] [review]
> 456053-followup.patch
> 
> Follow-up to avoid crashes due to bug 1298081.

oops, midaired :-)

Comment 84

[Jorg K (CEST = GMT+2)]

Attachment: 456053-followup2.patch

As discussed on IRC. Using the Drafts folder instead.

Many other tests create it beforehand as well.
https://dxr.mozilla.org/comm-central/search?q=create_folder(%22Drafts%22%2C+%5BCi.nsMsgFolderFlags.Drafts%5D)%3B&redirect=false

Comment 85

[Jorg K (CEST = GMT+2)]

Aceman: Two for the price of one, you chose.

Aleth: Working on getting the tree green again. Just waiting for review here and in bug 1306630.

Comment 86

[Jorg K (CEST = GMT+2)]

Please "autoland" chosen solution.

Comment 87

[:aceman]

I can't decide yet, because the Outbox version still crashes on Windows and OS X.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d18389d6386794bc555850b2f0cee74a1607df0f

I've started another try run with the Draft version.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=17a9d304110a4022806e59f3324191b2b2039dd4

Comment 88

[Jorg K (CEST = GMT+2)]

Attachment: 456053-followup2.patch

Declaration of draftsFolder; was missing, grrr.

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

Comment 89

[Jorg K (CEST = GMT+2)]

Comment on attachment 8796718 [details] [diff] [review]
456053-followup.patch

Let's switch to drafts since the try is green.

Comment 90

[Jorg K (CEST = GMT+2)]

I wanted to land this now, but here comes Aceman with another try. He really wants to know whether we need to create the Drafts folder beforehand ;-)

https://hg.mozilla.org/try-comm-central/rev/c3d732138d606ac7b9220a43f90e99304ddad4d4

Comment 91

[:aceman]

Comment on attachment 8796785 [details] [diff] [review]
456053-followup2.patch

Review of attachment 8796785 [details] [diff] [review]:
-----------------------------------------------------------------

It seems the version without creating Drafts also worked :)
Anyway, I observed the TB profile while the test is running and the Outbox (Unsent messages) was created automatically at the start of the test even if we do not use it now. So I do not understand why it is then claiming to not exist for the disk space check. But that is the job for bug 1298081.

So let's land this now.

Comment 92

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #91)
> So let's land this now.
Thanks, and let's document the changeset here:
https://hg.mozilla.org/comm-central/rev/26fbb38687e5