121898 - Rewrap wraps similar lines inconsistently

Status: RESOLVED WORKSFORME

(MailNews Core :: Composition, defect)

Description

[kinmoz]

Launch mail plain text compose and type in the following line with the '>' 
character, followed by a return:

> Now is the time for all good men to come to the aid of their country.


Now use the shift-up-arrow key and ctrl-c to copy the line. Now do a select all 
(ctrl-a) and paste (ctrl-v) the text twice. You should end up with something 
like:

> Now is the time for all good men to come to the aid of their country.
> Now is the time for all good men to come to the aid of their country.

followed by a blank line. Now if you continually select options->rewrap from the 
menus, you will notice that the 1st line never wraps, the 2nd one will wrap and 
unwrap with every other selection of "rewrap" from the menus, and blank lines 
and empty lines with ">" chars appear.

It starts looking something like this after several rewraps:

> Now is the time for all good men to come to the aid of their country. 
> Now is the time for all good men to come to the aid of their
> country.

> 

> 

> 

>

Comment 1

[Akkana Peck]

Attachment: A fix

Here's a fix, which cleans up several related problems.  Here's an explanation
of the changes:
1. Add a routine IsSpace which can check for any whitespace (including
newline), and check it between wraps. We were in many cases adding extra spaces
at the ends of lines and between the cite string and the cited text, which was
causing inconsistent results in wrapping.
2. Clean up the handling of whitespace after the cite string.
3. Make the wrapping behavior more consistent.	Previously, when it was
considering strings at the end of the line, it would usually find a hit looking
backwards from prospective eol; but when starting on a new line, the line
breaker wouldn't find that hit, so we then looked forward (exceeding the
nominal line length).  Now, we don't do the look forward if we're already past
the beginning of the line; instead, we break the line and continue the inner
loop.  This fixes the "get longer/get shorter/get longer" behavior seen in the
example in this bug.
x. Removed a bunch of #ifdefed printfs because they were ugly and weren't that
helpful in debugging.

Comment 2

[Charles Manske]

Comment on attachment 75660 [details] [diff] [review]
A fix

r=cmanske

Comment 3

[kinmoz]

Comment on attachment 75660 [details] [diff] [review]
A fix

sr=[e-mail]

+static PRUnichar nbsp (0xa0);


I think there may be a platform difference on the char code used for an  
... I noticed that the intl guys added the following code to the netscape
spellchecker code ... I'm not sure if you have to do something similar or not:

    #ifdef XP_MAC
    #define IS_NBSP_CHAR(c) (((unsigned char)0xca)==(c))
    #else
    #define IS_NBSP_CHAR(c) (((unsigned char)0xa0)==(c))
    #endif


+      nsAutoString sub (Substring(tString, posInString, breakPt));
+      // skip newlines or whitespace at the end of the string
+      PRInt32 subend = sub.Length();
+      while (subend > 0 && IsSpace(sub[subend-1]))
+	 --subend;
+      sub.Left(sub, subend);


If sub contains nothing but spaces and subend is zero at the end of the loop,
Left() will truncate sub to an empty string right?

Comment 4

[Akkana Peck]

> If sub contains nothing but spaces and subend is zero at the end of the loop,
> Left() will truncate sub to an empty string right?

That's my understanding.  It calls Substring(sub, 0, 0) and at that point I get
lost in the string code ...

Comment 5

[Asa Dotzler [:asa]]

Comment on attachment 75660 [details] [diff] [review]
A fix

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 6

[Akkana Peck]

Fix is in.

Comment 7

[esther]

Using build 20020328 on winxp, linux and mac I don't see the get longer/get
shorter problem with each rewrap as stated in the original statement but what I
do see now is that the 2nd line wraps with an extra white space between the >
and N. Example below:

> Now is the time for all good men to come to the aid of their country.
>  Now is the time for all good men to come to the aid of their
>country.

I can't tell if this is what is expected after this fix.  Also note: If I type
or paste that same line 6 times then do the rewrap option, it performs the
rewrap on the first 2 lines as the example shows.  Clicking rewrap option again
rewraps lines 3 & 4, again rewraps line 5&6. 

This isn't what I would have expected.  Do you want a new bug or should I reopen
this one.

Comment 8

[esther]

reopening for review of current results.

Comment 9

[Akkana Peck]

Moving this off to 1.0.1 to fix the remaining problems -- it's better than it
was, and further changes aren't likely to be approved for 1.0.

Comment 10

[Pádraig Brady]

This is so annoying! Why can't it be fixed? (still in RC1)

In summary what I notice is replying to a mail, I get
(the cursor position is indicated with |).

> origonal
> quoted text

|

Now hitting down-arrow goes down another line?
Also putting the cursor at the end of the "quoted text"
line and hitting return puts 2 blank lines in!

Comment 11

[Akkana Peck]

[e-mail]: what you're describing doesn't seem to have anything to
do with rewrap.  I think you want bug 83378.

Comment 12

[scottputterman]

changing to nsbeta1- for Mach V per ADT.

Comment 13

[Akkana Peck]

Clarifying summary to reflect the current problem, since the original one was fixed.

Comment 14

[John Malmberg]

Rewrap mangles hand formatted text such as source code listings.

Of course the main reason that I use the rewrap function is because of a bug in
the plain text composer that sometimes causes new lines not to be wrapped when
they are typed in.

Comment 15

[Akkana Peck]

> Rewrap mangles hand formatted text such as source code listings.

So don't rewrap that part.  If you make a selection, rewrap will only apply to
the selection.

> because of a bug in
> the plain text composer that sometimes causes new lines not to be wrapped 

jfrancis has been working on that, and things should be better; for one thing,
it's now easy to see when you get stuck inside a quote block, because it has a
different color from the rest of the message.

Comment 16

[Akkana Peck]

Not sure when I'll get to this, and it's not clear anyone really cares much
about the remaining consistency problem.

Comment 17

[Tom Hundt]

>it's not clear anyone really cares much
>about the remaining consistency problem.

I use rewrap EVERY DAY and hence care about it very much.  

Comment 18

[Akkana Peck]

Tom: Is there a particular symptom you're seeing that's causing problems?  The
current bug as described by Esther in comment 7 is that if you run rewrap
multiple times on the same text, or once over multiple identical lines, it will
wrap at slightly different character positions (I think the wrap column gets off
by one or two characters after getting confused somehow by whitespace, but was
never able to pin it down farther than that).  Is this actually causing a
problem in normal email, or is there a related problem that's more serious? 

Comment 20

[Paolo Bonzini]

Right now, here are the results after N rewraps:

N=0

> Now is the time for all good men to come to the aid of their country.
> Now is the time for all good men to come to the aid of their country.


N=1

> Now is the time for all good men to come to the aid of their
> country. Now is the time for all good men to come to the aid of their
> country.

The sentences fit one line, but probably rewrap is discouraging wrapping at the end of a sentence?

N=2,4,...

> Now is the time for all good men to come to the aid of their country.
> Now is the time for all good men to come to the aid of their 
> country.

This is inconsistent with N=1.

N=3,5,...

> Now is the time for all good men to come to the aid of their
> country. Now is the time for all good men to come to the aid of their
>  country.

This is bug 191881.

Comment 21

[Hartmut Welpmann [:welpy-cw]]

The issue from the description has been fixed long ago (see comment 6). For the problem discussed in comments 7 and onward there is bug 191881.