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

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect)

Description

[Christopher Blizzard (:blizzard)]

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.

Comment 1

[Christopher Blizzard (:blizzard)]

Attachment: image

Comment 2

[Christopher Blizzard (:blizzard)]

OK, this is because I had "wrap plan text messages" to 72 instead of 78.

Comment 3

[Akkana Peck]

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).

Comment 4

[Christopher Blizzard (:blizzard)]

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?

Comment 5

[Akkana Peck]

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.

Comment 6

[Christopher Blizzard (:blizzard)]

I'll be more than happy to test it.

Comment 7

[Akkana Peck]

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?

Comment 8

[Akkana Peck]

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.

Comment 9

[Akkana Peck]

Attachment: Patch: add a pre style on the span

Comment 10

[Christopher Blizzard (:blizzard)]

I'm going to give this patch a spin today.

Comment 11

[Akkana Peck]

The missing horizontal scrollbar is bug 82635, unrelated to this bug.

Comment 12

[Christopher Blizzard (:blizzard)]

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.

Comment 13

[Akkana Peck]

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.

Comment 14

[Christopher Blizzard (:blizzard)]

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.

Comment 15

[Christopher Blizzard (:blizzard)]

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.

Comment 16

[Akkana Peck]

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.

Comment 17

[Chris McAfee]

r=mcafee for flipping the pref

Comment 18

[Christopher Blizzard (:blizzard)]

sr=blizzard

It seems to work here.

Comment 19

[Akkana Peck]

Checked in the pref change.
Handing back to Joe -- this needs edit rules love if we want to get the span
thing working.

Comment 20

[Joe Francis]

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.

Comment 21

[Joe Francis]

Attachment: patch for modules

Comment 22

[Joe Francis]

Attachment: patch for editor

Comment 23

[Joe Francis]

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.

Comment 24

[Joe Francis]

097

Comment 25

[Joe Francis]

Attachment: updated diffs to editor

this also contains 2 other bug fixes, for 101342 and 104499

Comment 26

[Joe Francis]

Attachment: updated diff for modules/libpref/src/init

Comment 27

[Christopher Blizzard (:blizzard)]

Oh, man.  I want this for 0.9.7.  How safe is it?

Comment 28

[Joe Francis]

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.

Comment 29

[Christopher Blizzard (:blizzard)]

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.

Comment 30

[Joe Francis]

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.

Comment 31

[Joe Francis]

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.

Comment 32

[lchiang]

I will download today's release build and set the preference and test it out in
my daily use for you.

Comment 33

[Christopher Blizzard (:blizzard)]

I've been running with this for the last couple of days and I haven't seen
anything bad as a result so far.

Comment 34

[Joe Francis]

ok, I'll land this asap (waiting for open tree)

Comment 35

[Joe Francis]

rest of patch has landed on trunk

Comment 36

[Daniel Bratell]

I think this made bug 119850 appear. The pref change triggers code in
nsPlainTextSerializer.

Comment 37

[esther]

Using build 20020322 on winxp, 20020318 on mac and linux.
Verified