Closed Bug 92331 Opened 24 years ago Closed 23 years ago

plain text reply indenting isn't ending lines correctly with values other than 78 chars

Categories

(MailNews Core :: Composition, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.8

People

(Reporter: blizzard, Assigned: mozeditor)

References

Details

(Whiteboard: EDITORBASE; fix checked in but not enabled)

Attachments

(2 files, 4 obsolete files)

Build is on Jul 25, 2001. Indenting as part of a reply is really hosed. The words on the ends of lines are set to the next line and replies look awful. I'll attach a screenshot.
Attached image image (deleted) —
Whiteboard: critical for 0.9.3
OK, this is because I had "wrap plan text messages" to 72 instead of 78.
Summary: plain text reply indenting isn't ending lines correctly → plain text reply indenting isn't ending lines correctly with values other than 78 chars
No, please don't set your wrap column artificially long just so that text looks right. Ugh, that will change the wrapping on what you actually send so it'll be too long in other people's windows (at least when it's quoted). This is an artifact of the fix for bug 69638 (and I noted when I proposed the fix that this would happen, but all respondants were of the opinion that that was okay and wouldn't bother anyone). It should wrap okay on send, just looks bad in the mail compose window. I think we can get around this by putting a moz-pre-wrap style (or some similar style which will cause wrapping) on the span tag, as commented in the other bug. I've been meaning to play around with that, but nobody seemed to care so it's been low priority. This should probably be a dup of that bug (or maybe we should make this be the current bug and I should close that one).
Well, if it does look OK on send I can't tell as the end user. From what I can tell what it looks like when it's in my window is what it's going to look like when it shows up on the other end. This _really_ needs to get fixed. It confused the hell out of me and I'm pretty tolerant. :) Isn't 78 the default?
72 is the default. At 78, your messages will wrap on an 80-column screen as soon as one person quotes them, whereas 72 leaves a comfortable margin (easier on the eye and can be quoted a few levels deep without wrapping). I agree about the wysiwyg-ness, which is why I raised the issue in the other bug. We should try the style thing and see if it helps. Want to be the guinea pig to test it if I write it? Grabbing this bug since it's related to the other one.
Assignee: ducarroz → akkana
I'll be more than happy to test it.
Whiteboard: critical for 0.9.3 → want for 0.9.3
I just noticed the "want for 0.9.3" SWB. I'm confused now; I thought we were talking about the trunk, but isn't the branch what's going to go out as 0.9.3? The bug 69638 fix I mentioned landed on the trunk but not the branch. AFAIK, the branch's handling of plaintext mail (including wrapping) hasn't changed since several weeks ago when Joe landed his plaintext fixes which fixed a lot of caret-motion bugs. Is this bug on the trunk or the branch (or both), and is it changed if you toggle the pref mentioned in that bgu?
Target Milestone: --- → mozilla0.9.4
Attaching a short patch which fixes the wrapping problem (long quoted lines no longer appear wrapped). Unfortunately, although the horizontal scrollbar trough appears in the compose window when the window is too wide, the scrollbar thumb doesn't appear. Resizing doesn't help. Arrowing around in the text does scroll as expected. This is a layout or xpfe bug. My suggestion is to go ahead and check this in, then we can file the bug on the scrollbar thumb not showing up. Most people won't be editing the ends of quoted lines anyway (and if they did, they'd probably use arrow keys or resize the window first). Looking for r= and sr= and testing from plaintext mail people.
Status: NEW → ASSIGNED
Whiteboard: want for 0.9.3 → FIX IN HAND, NEED r= sr=
Attached patch Patch: add a pre style on the span (obsolete) (deleted) — Splinter Review
I'm going to give this patch a spin today.
The missing horizontal scrollbar is bug 82635, unrelated to this bug.
The first problem that I noticed with this patch is that if you break in the middle of a quoted block to reply to part of a message it doesn't automatically wrap the line as you continue to type.
That's not good news. That means that the edit rules aren't correctly moving the caret outside of the quoted block when it's split. And this patch doesn't affect that at all; it only affects the way the block is rendered when you're in the compose window. In other words, if it isn't wrapping with this patch, then it probably wouldn't have wrapped at output time before the patch (which IMO is worse, since you wouldn't know there was a problem until you send). If that's really true, that suggests that we should flip the default value of the pref back until the edit rules problem is solved -- sending out unwrapped stuff while showing it to the user as though it's going to be wrapped is Bad. (It also means that this bug probably has to go back to Joe for edit rules work.) Comments solicited.
I have verified that if I apply the patch then the problem occurs. If I remove the patch the problem doesn't occur and I can split quotes until the cows come home.
This is the bug that is on the top of my most-hated-bug-list and I know that it's keeping shaver from using the tip, too. Marking critical for 0.9.4.
Whiteboard: FIX IN HAND, NEED r= sr= → FIX IN HAND, NEED r= sr=; critical for 0.9.4
Keywords: nsdogfood
Blizzard, last time I talked to you about this bug, we tentatively agreed that flipping the pref to go back to using pre was probably the right thing to do, but you were going to test more and report in the bug. I've been waiting for you to comment before doing anything. My advice is to flip the pref and go back to the previous behavior. In other words, Index: editor.js =================================================================== RCS file: /cvsroot/mozilla/modules/libpref/src/init/editor.js,v retrieving revision 3.17 diff -r3.17 editor.js 61c61 < pref("editor.quotesPreformatted", false); --- > pref("editor.quotesPreformatted", true); If someone wants to r and sr this change I'll be happy to check it in.
r=mcafee for flipping the pref
sr=blizzard It seems to work here.
Assignee: akkana → jfrancis
Status: ASSIGNED → NEW
Whiteboard: FIX IN HAND, NEED r= sr=; critical for 0.9.4 → critical for 0.9.4
Checked in the pref change. Handing back to Joe -- this needs edit rules love if we want to get the span thing working.
I'm going to assume the milestone doesn't apply to this bug but to the partial fix which has already landed. putting in 095 bucket.
Status: NEW → ASSIGNED
Whiteboard: critical for 0.9.4
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Attached patch patch for modules (obsolete) (deleted) — Splinter Review
Attached patch patch for editor (obsolete) (deleted) — Splinter Review
Above to patches get you most of the way there. I need to add some special casing for when you "split" a mailcite right at the front of it, or right at the end of it. Oddly enough, this patch unmasks a layout bug: you will get a funky duplicated (and selected) text frame if you split a mailcite right at the end of the cite. Anyway, give these a try and see if this is what you want.
Whiteboard: partial fix in hand
Whiteboard: partial fix in hand → EDITORBASE; 1 day; partial fix in hand
Blocks: 101632
Target Milestone: mozilla0.9.5 → mozilla0.9.6
QA Contact: sheelar → esther
Blocks: 108120
Blocks: 108194
No longer blocks: 108120
097
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Attached patch updated diffs to editor (obsolete) (deleted) — Splinter Review
this also contains 2 other bug fixes, for 101342 and 104499
Attachment #44449 - Attachment is obsolete: true
Attachment #48061 - Attachment is obsolete: true
Attachment #48062 - Attachment is obsolete: true
Oh, man. I want this for 0.9.7. How safe is it?
can you test? i haven't been able to test the latest changes. i can start that in an hour or two, and i think i have kin lined up for review. But if you can apply patch and do some testing that would help things a lot.
Unfortunately, no, I can't. I don't have a build tree that I can apply it in and build it in any reasonable amount of time.
fix checked in but not turned on. to enable, set the pref (see the libpref patch above). Will turn on in 098 if testing goes well.
Whiteboard: EDITORBASE; 1 day; partial fix in hand → EDITORBASE; fix checked in but not enalbed
Whiteboard: EDITORBASE; fix checked in but not enalbed → EDITORBASE; fix checked in but not enabled
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Attachment #61282 - Attachment is obsolete: true
Help! I need testing of this so that we can turn on the pref and make this fix kick in. To test simply apply the last (and only non-obsolete) patch to your tree and build. Or alternatively, just set the "editor.quotesPreformatted" pref to false. Then do plaintext mail composition, replying to a message, and expirement with typing enter/return inside of mailquotes, and also at the very beginning and end of mailquotes. See if you get the desired behavior and report here if you don't.
I will download today's release build and set the preference and test it out in my daily use for you.
I've been running with this for the last couple of days and I haven't seen anything bad as a result so far.
Blocks: 115520
ok, I'll land this asap (waiting for open tree)
rest of patch has landed on trunk
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
I think this made bug 119850 appear. The pref change triggers code in nsPlainTextSerializer.
Using build 20020322 on winxp, 20020318 on mac and linux. Verified
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: