This is a spin-off of bug 124099. When you apply rewrap to quoted text (as shown by the blue coloring) it becomes black, i.e., is no longer treated as quote, even though the quote symbols remain in place. Using format flawed those quote symbols get space stuffed, but this is bug 140884. IOW: This bug blocks bug 140884. pi

I think this will be straightforward; if it turns out to be more tricky, I might have to bump the milestone out to 1.3.

Akkana, can you do it for 1.2b? It's pretty annoying to have those space stuffed quotes, which are a consequence of this. pi

I have a fix. The code's not terribly pretty: I added an API to nsIEditorMailSupport, but because of the inconsistency of the InsertAsQuotation API returning the inserted node, which makes no sense if more than one node was inserted, I had to add some tricky code around that (I hope the comments are adequate to explain what's happening). The rest of this is nice, though: after the routine is called, quoted blocks magically turn blue even if they weren't before. :-) The only place I'm currently actually calling the new routine is from rewrap (but 110949 concerns making Edit Draft call it, and its dependency tree points to another bug suggesting that Edit Msg as New should also call it), but this is useful enough that I wonder if the plaintext paste code should consider calling this as well. (It does have a danger, though: that innocent lines starting with > will always be treated as quotes.) Cc'ing Seth because he wanted the fix for bug 110949, Joe since he's been involved with the plaintext quote fracas, Kathy because she should know about the API change, and Daniel in case he has any worries about how this will interact with format=flowed -- I don't think it will cause any problems there, but Daniel might spot something I don't. Joe, can you review? You're probably the best person for it, though if Daniel has time I'd love to get his blessing on this. And if anyone sees a cleaner way I could have added this (re the InsertAsQuotation returned node issue), please speak up.

Comment on attachment 100773 [details] [diff] [review] Add new API for insertTextWithQuotes, and call it from rewrap my only comment is that "InsertTextWithQuotes" is not consistent with the other apis (and not as clear to me); perhaps InsertTextWithQuotations or something similar? (or is that too long)

Comment on attachment 100773 [details] [diff] [review] Add new API for insertTextWithQuotes, and call it from rewrap I don't think this will affect anything related to f=f.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b) Gecko/2002100214 (plus patch) does work:-)) pi

If bug 110949 is EDITORBASE, then this one (on which it depends) should also be considered.

Kathy, I wondered whether anyone would object to that inconsistency when I wrote it ... I've changed it to be "insertTextWithQuotations" instead of "WithQuotes". Seeking review ...

Comment on attachment 101414 [details] [diff] [review] Changed the name in line with Kathy's comment r=brade

Seth, can you sr this fix? Thanks!

I don't understand aAddCites. I would have thought the call to >+ rv = InsertAsPlaintextQuotation(curHunk, PR_FALSE, >+ getter_AddRefs(dummyNode)); in nsHTMLEditor::InsertTextWithQuotations(const nsAString &aStringToInsert) would have wanted PR_TRUE intead, to get the blue text for the portions that are quotes? What am I missing?

Joe: + * @param aAddCites Whether to prepend extra ">" to each line + * (usually true, unless those characters + * have already been added.) In the line you mention, we have just inspected the chunk we're about to insert and determined that it already has at least one level of ">" at the beginning of each line. Hence, we're going to treat it as quoted while inserting, but we don't want to add an extra layer of "> " to the existing string: we're inserting *as* quoted (because it already has the cite string in it), not quoting it (quoting it, e.g. insert/paste as quotation, is when we add the cite string ourselves, which is not what this new method is for). Is there some way I can reword the comments to make this clearer? Maybe something like this just before the line you mention? // Pass aAddCites=false to make sure we don't add extra ">" // quote levels to what's already there.

ok, thanks for the explanation. I understand that we don't want the extra ">". But I'm still confused about the actual cite. I thouhg before rewrapping was causing us to lose the "blue". Doesn't that mean the cite itself was gone? How does this fix that. Sorry if I'm being thick here. If I understand this I'll be able to not break it in the future! :-)

Comment on attachment 101414 [details] [diff] [review] Changed the name in line with Kathy's comment 1) + PRBool curHunkIsQuoted = (aStringToInsert.First() == '>'); is it always '>'? should we do what you are doing in nsInternetCiter.cpp: static PRUnichar gt ('>'); 2) + + nsAString::const_iterator hunkStart, strEnd; + aStringToInsert.BeginReading(hunkStart); + aStringToInsert.EndReading(strEnd); + + // Loop over lines: + nsresult rv; + nsAString::const_iterator lineStart (hunkStart); + while (lineStart != strEnd) + { + // Search for the end of this line. + // NOTE! We only look for DOM newlines (\n). + // It would be nice to look for \r as well, but is it worth + // the performance hit of doing two separate searches each time + // since there's no longer a FindChars method? + PRBool found = FindCharInReadable('\n', lineStart, strEnd); + PRBool quoted = PR_FALSE; + if (found) + { + // if there's another newline, lineStart now points there. + // Loop over any consecutive newline chars: + nsAString::const_iterator firstNewline (lineStart); + while (*lineStart == '\n' || *lineStart == '\r') + ++lineStart; + quoted = (*lineStart == '>'); + if (quoted == curHunkIsQuoted) + continue; the second bit of code just looks for '>' following a \r or \n. we probably don't need to worry about a false positive there (do we?), but what about the first bit of code? what if \r is the proper line ending? I'm nervous about CR/LF issues, since it varies per platform and then if it comes from the wire (like imap) or from a file (local mail). or we safe, because we already converted properly before this point?

I'm perfectly happy defining a constant to hold the '>' character, if you like that better. I'll make that change. Regarding the newlines: it is an error to have a \r inside the dom, so strings are supposed to be converted to \n newlines before being inserted. Is it likely that mailnews isn't doing that substitution already? Our string interfaces don't give me a way to look for either \r or \n; to do that I either have to search for one, then search for the other (a significant performance hit since in most cases there will be no \r and the search will have to loop to the end of the message each time through) or else write FindCharsInReadable myself (which really should be in string classes, not reimplemented by each string user who needs it! but I can't find any string people right now to ask about why this functionality was removed).

> I'm perfectly happy defining a constant to hold the '>' character, if you like > that better. I'll make that change. it's up to you. I take it there is an RFC that says '>' is the required character. so perhaps making it a constant (and citing the RFC) is the way to go. over email, akk tells me that \r is illegal in the dom. she suggested: >(Maybe I should put one FindCharInReadable for \r at the very beginning, >and assert if it ever happens? It's definitely something we should know >about if it's happening; returns in the dom might lead to weird undefined >behavior from the editor or other parts of the code.) This sounds like a good idea. Your code should assume no \r, but we add some #ifdef DEBUG code that might not be fast, but that checks and asserts. this won't slow down opt builds, but will allow us developers to know if things change and our assumptions are incorrect. can you make a new patch with these changes, and then I'll sr?

Here are the changes you requested (hey, that sounds like a spammer ...) I don't cite an RFC, because I don't know what the relevant one is. Does RFC822 mention quote characters? RFC2646 certainly mentions them and discusses their use, but it's not the purpose of the RFC. Certainly there are other types of quoting (we support one other model, the aol-popularized Seth: >> perhaps making it a constant (and citing the RFC) is the way to go. << but it's not documented anywhere so I doubt anyone knows about it or uses it). But clearly the dominant internet-style ">" quoting is what mozilla encourages, so that's what this patch handles. Aside from quoting an RFC, I think I've addressed all your points. Joe: > ok, thanks for the explanation. I understand that we don't want the extra ">". > But I'm still confused about the actual cite. I thouhg before rewrapping was > causing us to lose the "blue". Doesn't that mean the cite itself was gone? How > does this fix that. The "blue" comes from wrapping in a special <div> or <pre> block. The "cite string" referred to in this patch is the character ">" prepended at the beginning of each line, which is independant of whether a block is wrapped in a special div or pre. We have the ">" characters at the beginning, and rewrap deletes them or adds new ones as appropriate (that's transparent to edit rules or mailnews code) so we magically have them after the rewrap too, and don't have to do anything about it. But we do have to do something about the special div/pre, because it's included in the selection at the beginning and thus will be deleted when the replacement text comes in, so if we want the replacement to have the special div/pre, we have to allow for adding it in (which is what this bug covers). Any clearer?

since \r is illegal, do we still need: while (*lineStart == '\n' || *lineStart == '\r') should we just be checking for \n?

Generally I check for \r when it's cheap to do, just in case, but omit it when it's expensive. I wouldn't insist on leaving that check in, though; either way is all right with me.

Comment on attachment 102090 [details] [diff] [review] Changes requested by Seth ok, i sobered up

Seth convinced me that it was better to remove the \r since the earlier assertion should catch improper returns that somehow get into the dom, at least in debug builds. (Which shouldn't happen anyway, unless rogue code modifies the dom without going through mailnews, editor or parser.)

Comment on attachment 102211 [details] [diff] [review] Remove the extra check for \r as requested by Seth r=jfrancis, sr=sspitzer thanks for patiently addressing my comments.

Fix is in!

This commit have added a "might be used uninitialized" warning on Brad TBox: +editor/libeditor/html/nsHTMLDataTransfer.cpp:1632 + `nsresult rv' might be used uninitialized in this function Indeed, if 'while (lineStart != strEnd)' loop is never executed (e.g. lineStart == strEnd from the beginning), nsHTMLEditor::InsertTextWithQuotations will return an uninitialized nsresult. P.S. See bug 59652 fro more on "might be used uninitialized" warnings.

Aleksey: thanks for the alert! I thought my compiler usually showed me those, but it wasn't showing me that one. I've checked in a fix initializing rv to NS_OK.

Patch works in Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b) Gecko/20021011. pi

tested using a trunk build from 20030407 on win2K. I set the edit mode to plaintext, selected to reply to a message that had been replied to a few times, selected to reply. Highlighted some of the quoted text and selected Edit|Rewrap -- and it work as expected. Verified