Closed
Bug 104195
Opened 23 years ago
Closed 23 years ago
rewrap destroys quotes
Categories
(Core :: DOM: Editor, defect)
Tracking
()
VERIFIED
FIXED
mozilla0.9.7
People
(Reporter: 3.14, Assigned: akkzilla)
References
Details
Attachments
(6 files)
27.16 KB,
image/png
|
Details | |
31.85 KB,
image/png
|
Details | |
3.39 KB,
patch
|
Details | Diff | Splinter Review | |
4.60 KB,
patch
|
Brade
:
review+
kinmoz
:
superreview+
|
Details | Diff | Splinter Review |
27.71 KB,
image/png
|
Details | |
26.84 KB,
image/png
|
Details |
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
Assignee | ||
Comment 1•23 years ago
|
||
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?
Assignee: kin → akkana
Reporter | ||
Comment 2•23 years ago
|
||
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
Assignee | ||
Comment 3•23 years ago
|
||
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.)
Reporter | ||
Comment 4•23 years ago
|
||
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
Reporter | ||
Comment 5•23 years ago
|
||
Reporter | ||
Comment 6•23 years ago
|
||
Assignee | ||
Comment 7•23 years ago
|
||
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 ...
Reporter | ||
Comment 8•23 years ago
|
||
> 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
Assignee | ||
Comment 9•23 years ago
|
||
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?
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 10•23 years ago
|
||
Comment 11•23 years ago
|
||
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?
Assignee | ||
Comment 12•23 years ago
|
||
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.
Assignee | ||
Comment 13•23 years ago
|
||
Comment 14•23 years ago
|
||
Comment on attachment 56276 [details] [diff] [review] Patch with additional checks for overruns sr=kin@netscape.com
Attachment #56276 -
Flags: superreview+
Assignee | ||
Comment 15•23 years ago
|
||
I'll try to get this reviewed and in for 0.9.6.
Target Milestone: --- → mozilla0.9.6
Comment 16•23 years ago
|
||
Comment on attachment 56276 [details] [diff] [review] Patch with additional checks for overruns r=brade
Attachment #56276 -
Flags: review+
Assignee | ||
Comment 17•23 years ago
|
||
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.
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Assignee | ||
Comment 18•23 years ago
|
||
Fixed.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 19•23 years ago
|
||
Boris, can you please help verify this one? thanks..let us know if it looks fixed on your end...
Reporter | ||
Comment 20•23 years ago
|
||
I will check, but my admin won't install a version before 0.9.6. pi
Reporter | ||
Comment 21•23 years ago
|
||
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 22•23 years ago
|
||
Reporter | ||
Comment 23•23 years ago
|
||
Comment 24•23 years ago
|
||
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.
Assignee | ||
Comment 25•23 years ago
|
||
Marking fixed for now; please reopen if it doesn't work in 0.9.7.
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Comment 26•23 years ago
|
||
Verifying on build 2001121803. If anyone is still seeing this, please reopen.
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 27•23 years ago
|
||
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
Assignee | ||
Comment 28•23 years ago
|
||
Different bug. It's related to some existing ones, but I don't think anything covers quite this case. Please file it, against me.
You need to log in
before you can comment on or make changes to this bug.
Description
•