104195 - rewrap destroys quotes

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Boris 'pi' Piwinger]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.4) Gecko/20010913

Write a reply mail where the quoted text is longer than wrap column. Use
Options/Rewrap. This will also apply to quoted text. So we have long quoted
lines followed by short unquoted lines. This is evil.

pi

Comment 1

[Akkana Peck]

This would definitely be evil.  The whole point of rewrap is that it's supposed
to do the right thing with quotes.  But I can't reproduce this -- it works for
me.  Can you give some more details on exactly what the message looks like (and
does it come from text or html) and exactly how you get into this mode?

Comment 2

[Boris 'pi' Piwinger]

OK, let me elaborate on the details. I use only the text editor. I answer to an
e-mail or posting (no difference here). The quoted lines (with the quotation
mark) are longer than the 70 characters I set as line length. For some (unclear)
reason the line breaking for my text does no longer work and lines become very
long. So I press Rewrap. Now my text is broken correctly, but also the quotes
are broken to fit 70 charaters.

Please let me know which details I could provide to clarify the problem.

In the meantime I switched to the new version, the problem is still around:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.5) Gecko/20011012

pi

Comment 3

[Akkana Peck]

Wait, I'm really confused.  Rewrap is for quoted text -- that's the whole point
of it, to rewrap quoted lines that are too long or short (e.g. from people who
send out mail with wrapping off entirely, or with their line length set too
long).  There's no need to rewrap the text you type; the edit window keeps it
wrapped no matter what.  If the new lines you type somehow aren't wrapping, then
that's a different bug (on plaintext editing), not a rewrap bug.  (There is an
existing bug, assigned to jfrancis, where if you break a quoted section and
start typing, or click immediately underneath quoted text and start typing, your
lines won't be broken.  That's the only case I know of where your typed lines
don't break correctly, though.)

Comment 4

[Boris 'pi' Piwinger]

Sorry, I did not make myself not clear enough (not being a native speaker can be
tough;-).

> Rewrap is for quoted text -- that's the whole point of it,
> to rewrap quoted lines that are too long or short

Well, that completely fails. See screenshots before and after which I'll post in
a minute. You have a comb-like structure (like Outbreak Excess produces).

> There's no need to rewrap the text you type;

In that case there is. Anyways, that text is wrapped using the command (as I
would expect), as is also seen on the screenshot.

> If the new lines you type somehow aren't wrapping, then
> that's a different bug (on plaintext editing), not a rewrap bug.

Right, that was not the point of my report. If you have the number for that bug,
this would be helpful.

Also note a third thing in the screenshots. The text doubled. I have never seen
that before. But between taking the shots I have done nothing but applying
options / rewrap.

pi

Comment 5

[Boris 'pi' Piwinger]

Attachment: Screenshot before applying options / rewrap

Comment 6

[Boris 'pi' Piwinger]

Attachment: Screenshot after applying options / rewrap

Comment 7

[Akkana Peck]

That really shouldn't be happening (and it doesn't happen for me, but I just
discovered that I do have quotesPreformatted set to the wrong value).  Can you
check your prefs (prefs.js in your profile directory) and see if there's a line
there setting a pref called editor.quotesPreformatted ?  Also, is your
mailnews.wraplength set to anything other than the default?

(I'm trying to test this myself with various values of
editor.quotesPreformatted, but apparently plaintext quoting is broken in the
build I have, so I'll have to wait a few days 'til that gets fixed.)  If
quotesInPre were being handled backward, that would explain this, but the code
looks right ...

Comment 8

[Boris 'pi' Piwinger]

> Can you check your prefs and see if there's a line there
> setting a pref called editor.quotesPreformatted ?

No such entry.

> Also, is your mailnews.wraplength set to anything other than the default?

I don't know the default, but it is 70 for me.

pi

Comment 9

[Akkana Peck]

I'm finally able to reproduce this on my own build, and debug it.  

The problem was that the algorithm sometimes made lines one word too long.  It
looks at a line and decides whether to break without looking at the length of
the first word on the next line; if that word is too long, we can exceed the
line length.  I have a patch for that, which should fix your problem.  I also
fixed some debug prints that were made longer than they should be in one of the
string API reorgs, and noticed some places where we were getting double spaces
inserted into the line, so I fixed those too.

Patch coming.  Looking for review and sr.  Kathy, any chance you can review
this?  Or suggest someone else?  Kin, can you sr?

Comment 10

[Akkana Peck]

Attachment: Patch, should fix the problem

Comment 11

[kinmoz]

Comment on attachment 56197 [details] [diff] [review]
Patch, should fix the problem

This may just be my paranoia, but...


+      // Skip over initial spaces:
+      while (nsCRT::IsAsciiSpace(tString[posInString]))
+        ++posInString;


Any chance this could exceed nextNewLine or length? Do we need some code here 
that checks for that condition, and perhaps does the right thing in terms of the 
outer loop?


+        // Trim trailing spaces:
+        PRInt32 lastRealChar = nextNewline;
+        while (nsCRT::IsAsciiSpace(tString[lastRealChar-1]))
+          --lastRealChar;


Same here, is there any possibility that lastRealChar can become zero resulting 
in a -1 lookup?

Comment 12

[Akkana Peck]

I think it's unlikely (would only happen if the line was all spaces, but then
going back you'd hit the newline at the end of the previous line) but it's
possible, and the checks are a good idea in any case, and don't cost much.  New
patch on the way, with checks (which also converts more of those debug
statements that unnecessarily used intermediate variables) but otherwise it's
the same.

Comment 13

[Akkana Peck]

Attachment: Patch with additional checks for overruns

Comment 14

[kinmoz]

Comment on attachment 56276 [details] [diff] [review]
Patch with additional checks for overruns

sr=[e-mail]

Comment 15

[Akkana Peck]

I'll try to get this reviewed and in for 0.9.6.

Comment 16

[Kathleen :Brade]

Comment on attachment 56276 [details] [diff] [review]
Patch with additional checks for overruns

r=brade

Comment 17

[Akkana Peck]

Sorry -- I caught a bad flu and wasn't able to get this in in time for the
milestone.  It'll go into the next open tree I see, though.

Comment 18

[Akkana Peck]

Fixed.

Comment 19

[sujay]

Boris, can you please help verify this one? thanks..let us know if it looks
fixed on your end...

Comment 20

[Boris 'pi' Piwinger]

I will check, but my admin won't install a version before 0.9.6.

pi

Comment 21

[Boris 'pi' Piwinger]

Sorry, it is still broken. I'll again post before/after shots. Please ignore the
rectangle in the upper right corner of the second image.

pi

Comment 22

[Boris 'pi' Piwinger]

Attachment: Screenshot before applying options / rewrap (0.9.6)

Comment 23

[Boris 'pi' Piwinger]

Attachment: Screenshot after applying options / rewrap (0.9.6)

Comment 24

[David Hallowell]

Akkana says that the milestone 0.9.6 was missed and this was checked in
afterwards, this means the fix will be in the latest nightly builds or you'll
have to wait till 0.9.7 to verify if your admin has a policy of only installing
releases.

Comment 25

[Akkana Peck]

Marking fixed for now; please reopen if it doesn't work in 0.9.7.

Comment 26

[michael wendell]

Verifying on build 2001121803.  If anyone is still seeing this, please reopen.

Comment 27

[Boris 'pi' Piwinger]

Sorry for the long silence. I did not have 0.9.7 to verify, now I have 0.9.8.
The fix is not completely correct. My test looked like this (hopefully the lines
stay correct here):

>> 012345678 012345678 012345678 012345678 012345678 012345678 012345678
>> 012345678 012345678 012345678 012345678 012345678 012345678 012345678
>> 012345678 012345678 012345678 012345678 012345678 012345678 012345678
>> 012345678 012345678 012345678 012345678 012345678 012345678 012345678
>> 012345678 012345678 012345678 012345678 012345678 012345678 012345678
>> 012345678 012345678 012345678 012345678 012345678 012345678 012345678
>> 012345678 012345678 012345678 012345678 012345678 012345678 012345678

I created a reply and applied Rewrap, in the editor it looked like:

>>> 012345678 012345678 012345678 012345678 012345678 012345678
>>> 012345678 012345678 012345678 012345678 012345678 012345678
>>> 012345678 012345678 012345678 012345678 012345678 012345678
>>> 012345678 012345678 012345678 012345678 012345678 012345678
>>> 012345678 012345678 012345678 012345678 012345678 012345678
>>> 012345678 012345678 012345678 012345678 012345678 012345678
>>> 012345678 012345678 012345678 012345678 012345678 012345678
012345678
>>> 012345678 012345678 012345678 012345678 012345678 012345678

The mail actually looked like this when received:

>>> 012345678 012345678 012345678 012345678 012345678 012345678
>>> 012345678 012345678 012345678 012345678 012345678 012345678
>>> 012345678 012345678 012345678 012345678 012345678 012345678
>>> 012345678 012345678 012345678 012345678 012345678 012345678
>>> 012345678 012345678 012345678 012345678 012345678 012345678
>>> 012345678 012345678 012345678 012345678 012345678 012345678
>>> 012345678 012345678 012345678 012345678 012345678 012345678 012345678
>>> 012345678 012345678 012345678 012345678 012345678 012345678

Not really correct. Reopen or different bug?

pi

Comment 28

[Akkana Peck]

Different bug.  It's related to some existing ones, but I don't think anything
covers quite this case.  Please file it, against me.