Closed Bug 43388 Opened 25 years ago Closed 25 years ago

|InsertAsQuotation| not flowed aware

Categories

(Core :: DOM: Editor, defect, P3)

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: BenB, Assigned: akkzilla)

References

Details

(Whiteboard: [nsbeta3+][p:3])

Reproduce: 1. Open a format=flowed msg with a correct, flowed quote (most lines look like ">[SPACE][some text][SPACE]"). 2. Reply to it using the plain text composer. 3. Send 4. Read the sent msg in Mozilla. Actual result: - After step 2, you can see a space between the first ">" and the ">" from the quoted msg. This is the bug. - After step 4, you see the quote of level 2 flowing, *including* the ">"s, i.e. you see | > bla bla bla bla bla > bla bla bla | bla bla > bla bla bla ("|" is the <blockquote> bar). This is the result of the bug. Expected result: - After step 2, you see all ">"s marking quotes (and *only* them) without any spaces between them. After them, there must be a space. - After step 4, you see also the quote of level 2 marked with a bar, i.e. | | bla bla bla bla bla bla bla bla | | bla bla bla bla bla (the content of the quote can (and should) wrap). Implementation suggestion: IIRC, |InsertAsQuotation| takes a format param. Check for a |format=flowed| param in there. If true, prepend ">", if the first char is ">", otherwise "> ". Do *not* change the behaviour for quoting fixed plain text (otherwise you might mark something as being a quote which isn't one).
Keywords: nsbeta2
No, InsertAsQuotation doesn't take a format argument, just a flag indicating whether it's inserting html or plaintext. I'm not clear here: what does the string being inserted look like before it's handed to the editor? How do I reproduce this? I tried sending a plaintext mail to myself, then replying to that, then replying to that ... all the replies looked fine. I don't see any preference in the mail/news pref panels that look like they're related to turning on flowed. Better yet, can you tell me how to reproduce it by giving me a test string that I can just copy and then do "Paste as Quotation" into the plaintext editor?
Paste As Quotation <example> >This is a testmail too se if you have gotten everything correct. This >is a long line that should be rewrapped if there's not enough room for >it. Since it is quite long now, I guess this is enough, but I'll write >a little more just in case you have a small font or very wide window. </example> in the plain text composer, sending this and reading with Mozilla shows the bug for me, in both a 2000-06-04 nightly and a custom built of mine from 2000-06-13 with related changes (found the bug during testing them).
Note the spaces at the end of all lines but the last one.
I don't have any possibilities to test anything, but doesn't InsertAsQuotation use the format=flowed mime converter to convert the mail from text to html? I thought that converter removed the '>' and replaced them by <blockquote>:s. In that case ther shouldn't be any problem.
Daniel, yes, the HTML composer works fine. We're speaking about the plaintext composer. Press SHIFT (or whatever it is on your platform) during reply to get it (or change your prefs). No blockquotes possible there.
<example quality=better> > This is a testmail too se if you have gotten everything correct. This > is a long line that should be rewrapped if there's not enough room for > it. Since it is quite long now, I guess this is enough, but I'll write > a little more just in case you have a small font or very wide window. </example>
As I said, I can't test anything. That will have to wait until I'm settled in my new apartement. So, what way does the mail travel? Guess: Raw mail (text/plain format=flowed) -> format=flowed Mime-converter (html) -> nsHTMLToTXTSinkStream (text format=flowed) -> InsertAsQuotation (text with '>', still format=flowed and with the trailing spaces) -> format=flowed Mime-converter (html) Then I think I understand what the problem is, and I've been aware of it for some time. The problem (as I sees it) is that Mozilla has a plaintext composer which is used when composing emails. The decision was made that it should contain only plain text and we can't (easily and correct) represent quoted flowed text with plain text. We would have to use block quoting to preserve the flow and they aren't compatible with the plain text composer. I would like to see the plain text composer dropped completely from mail composing. It can't do anything you can't do in the full composer window, and only causes problems. How do you, for instance, do if you suddenly in that composer window decides that the long mail you've written would benefit from some bold text and it wouldn't be a problem to send the mail as text/html or mixed. You can't. I don't know of any other MUA:s that have this strict distinction. To solve this bug you probably have to skip the flowed quoting unless someone decides that we can accept blockquotes (displayed with a vertical bar) in the plain text composer. Alternative number one (the bad but simple one) is easily solved if my assumption about the way the text travels is correct. Just don't send the format=flowed flag into nsHTMLToTXTSinkStream. A hack (if the other way is to hard) is to strip all trailing whitespaces in |InsertAsQuotation|.
Daniel, the plaintext composer is an elementary feature of the product. There are people (I guess akk is one of them) who just don't like HTML mails, never want to send them and want to control in detail what they send out. Then there are people (like myself) you found the HTML composer being by far to wierd, buggy => unpredictable => unusable. I wouldn't be here, if 4.x has had no plaintext composer, because I wouldn't have used Messenger 4.x. So, considering the audience, you're in a pretty bad shape to propose to remove the plaintext composer :). I don't know how exactly the current data flow is, but the idea is not to involve any conversion, if not absolutely necessary. So, that plan (see bug 34941) is to let the data path look like: 1. Original mail in RFC822 format 2. -> InsertAsQuotation(format="text/plain; format=flowed") 3. -> HTML-escaping (because the plain text composer is technically a HTML editor operating in a <pre wrap>, IIRC). This should *not* be done by libmime. This should work fine already. 4. -> plain text composer 5. -> HTML-pre wrap-content->TXT. This might be done in nsHTMLToTXTConv. 6. -> Send No HTML involved. That's how plain text users want it to be. But this doesn't invalidate flowed. We can still generate a correct inital, flowed msg (i.e. with correct ">"s and trailing spaces). If the user changes this, that's IMO his/her responsibility (this is exactly this level of control those users want, I think). The new content is edited in a <pre wrap>, so is does flow, when it is in the plain text composer. We should maintain this flow by adding necessary trailing spaces in step 5. I think, this already works fine. Yes, this is an inconsistency: in the quote, the user sees and can edit the "on the wire" format, while this is not true for new content. Does anybody have a suggestion? akk? I think, this bug should be fixed in step 2.
> No HTML involved OK, no *real* HTML, <pre> doesn't count.
Well, if the only argument against removing the plain text composer is that the full composer window is buggy, I would suggest focusing on fixing those bugs instead of trying to maintain two composers, one buggy and one inadequate. The spaces at the end of the line is an implementation detail with format=flowed and is nothing a user should have to care about. We should never depend on trailing spaces that a user has had the ability to touch (add, remove or turn upside down). That's why the plain text composer window doesn't work with quoted flowed text as it is now.
Daniel, No, bugs in the HTML composer were *not* the only argument. As I outlined above, there are also users who don't trust software (i.e. *you* and *me*) and want to control exactly what goes out, down to a trailing space. So, it is fine to let the user edit the trailing spaces. They *do* to care about these details. These users are typically very "advanced" ones. If they mess it up: Their own fault. OTOH, they can workaround possible bugs in our software (and there most likely will be some). In short: Plain text composer and HTML composer have a completely different philosophy: In the HTML composer, the user is in dummy mode and the software does the right thing; in the plain text composer, the software is in dummy mode and the user does the right thing.
Oh no! We must never let the user edit the spaces that decides if the line will be interpreted as a flowed line. The user of the plain text composer windows will not only be, what you describes as, advanced users. It's way too easy to switch to it for us to make such assumptions. Adding a space at the end of a line by mistake (very easily done and totally invisible for the user) and then sending the mail will cause very strange results. I would also say that only extremely advanced users has even the slightest idea of what a trailing space in a format=flowed mail means. If the goal of the plain text composer window is to be able to layout a mail character by character then we shouldn't use format=flowed for it at all. For people with extreme need of control I can recommend: telnet <your mailserver> 25 but then they will still not know what mailservers and the receiving party will do to the mail.
akk, plaintext composer master, your call. I don't see any chance to preserve the flowing information unless we show the trailing spaces to the user. Being able to wrap quotes was one of the main ideas of format=flowed (see spec). I would like not to disable this feature for the plain text composer.
I agree with Ben that we should preserve the plaintext composer, because a lot of people still want it, for more control over what they're typing (e.g. you can paste in an ascii table or ascii art and not have the newlines removed). Personally, I use plaintext compose in 4.x, but in Mozilla I use html compose, then convert to plaintext on send. I agree with Daniel that I don't see any way that we can have the plaintext editor support flowed quotes. It does have rewrap (though that feature is buggy and needs work) and users can always rewrap quoted material when they need to. What I think should happen here is basically what Daniel suggests: if we're going to quote in a plaintext compose window, then we should convert the message to plaintext without the flowed flag, so it has appropriate carets and newlines before it's passed to InsertAsQuotation, and the message will no longer be flowed (though if flowing is on, the new text that the user types will be flowed on output). Incidentally, I still can't reproduce this bug in the editor or compose windows. I can bring up a plaintext compose window, copy Ben's suggested text and do control-middlemouse to paste-as-quotation, the result looks fine. If I edit it to add a couple of lines, and do Output Text, it still looks fine. If I then send the message to myself, if I view the message in 4.x, it shows up as a plaintext message and it still looks fine. However, if I view the message in Mozilla, it looks rather like Ben described it: the inner level of quotation uses "> " and isn't all at the beginning of a line, the outer level appears as a blue bar (html quote). But if I reply to this in a plaintext compose window, it looks fine, and if I send the result, that looks fine too. It's only when viewed in the mozilla mail window that it looks bad. Am I seeing the wrong bug, or not seeing Ben's bug? The bug on displaying mail in the mail window is clearly not mine, and it seems like plaintext compose is doing the right thing as far as I've been able to test it.
akk, you#re seeing exactly the bug I described. As I outlines in the description in the beginning, this *is* a composition bug: We're sending out malformed quote tags. Please see the spec (sorry). The reason why you see it only in the view is that currently only our viewer understands format=flowed. I bet, you'll see the same result in Eudora. However you fix this, please not that the datapath before InsertAsQuotation will change with bug 34941, so don't rely on the current (RFC822->)libmime->(HTML->) nsHTMLToTXTConv->(plaintext->)InsertAsQuotation path.
Implementation suggestion: I still think, we should use my implementation suggestion as described in the beginning. If you keep the current malformed (with a space between the first and alll following) quote tags, the inner ones will be considered content by the reader and it will show them to the user. This would be bad, even if they wouldn't flow, because this would be the only case where users see them: They don't see them for HTML, correct format=flowed and not even plain text (due to my bug 31906). (OK, Euroda users might send Mozilla users similar mails, if they replies to real plain text, but this is a really far out edge case now.) But if you don't want the quote to flow, any code (possibly also InsertAsQuotation, I dunno) has to remove the trailing spaces before insertion into the composer.
Putting on [nsbeta2+] radar for beta2 fix.
Whiteboard: [nsbeta2+]
I still don't understand the problem. The text looks fine to me. Is the problem that the text in the plaintext compose window is not legal flowed, yet we're sending it as flowed? How should it be different from the way it actually looks? I don't understand your implementation suggestion, because I don't understand what's wrong with what the editor is doing. Is it that our output looks flowed to some clients because of the spaces at the ends of the lines? What if we just removed the spaces at the end of all the quoted lines?
Status: NEW → ASSIGNED
Target Milestone: --- → M17
Sorry, I missed the part where Ben said just that. Okay, I can easily make InsertAsQuotation remove trailing spaces, if that would fix this bug.
> Is it that our output > looks flowed to some clients because of the spaces at the ends of the lines? Yes, but, following the spec, it *is* flowed (not just looks like that). > What if we just removed the spaces at the end of all the quoted lines? That would still leave the ">" is the content. You also need to remove the space between the ">"s. That's what I proposed.
That would still leave the ">" is the content. You also need to remove the space between the ">"s. That's what I proposed. Why do we have to do that? There is nothing wrong with "> > >" in format=flowed mail. It would be treated as the test "> >" quoted once. If we remove the spaces between the '>':s we would get ">>>" which is the empty line quoted three times which is not quite the same. I wonder if that's what we want. Unless we're ourselves inserting the "> ".
> There is nothing wrong with "> > >" That would be a quoted plain text msg. But we're speaking about quoting flowed msgs, e.g. the msg we quoted looked like ">> the is the first line of a paragraph " and we produce "> >> the is the first line of a paragraph ", failing to carry over the "quoted" information and making ">> " content, which it is not.
Better to not produce the extra space at the first place. We only need that space if we're putting a quote in front of text. Handwritten patch to editor/base/nsInternetCiter.cpp (line ~84): while (i < length) { if (uch == newline) - aOutString.AppendWithConversion("> "); + aOutString.AppendWithConversion(">"); uch = aInString[i++]; + + // Insert a space if we are quoting unquoted text, to make + // it readable. + if(uch != '>') + aOutString.AppendWithConversion(" "); + aOutString += uch; }
Yes, that's exactly what I had in mind, but only for format=flowed.
Only for quoting flowed msgs, that is.
As for removing trailing spaces in quotes, maybe we could make if preffable, possibly honoring the pref "mail.IKnowWhatIDo(TM)" :).
Okay, I've boned up on the spec (which I should have done much earlier -- sorry) and you're right, Ben, I should fix the nsInternetCiter and make it flowed aware; either make it notice trailing spaces, or pass in a flowed argument to it (I hope I can get by with the former and don't require the latter). It also has to have its quotation-level handling fixed, and output ">>>" instead of "> > >" (though "> > >" would have been legal in the non-flowed case, wouldn't it?) Ben's fix will fix the second problem, but it also needs to handle removing the quotes then wrapping, and I want to fix the quoting-level detection code while I'm at it, since RFC 2646 is quite specific about how this should be done, and my old code doesn't do anything like that and is known to be broken in any case.
> I should fix the nsInternetCiter and make it flowed aware IMO, format=flowed should have it's own class (at least from a logical POV). Format=flowed is quite specific and explicit about quoting, while plain text quoting is mostly tradition. > either make it notice trailing spaces, or pass in a flowed argument We have the information flowed/non-flowed unambiguously - why guess? > I want to fix the quoting-level detection code Use mozITXTToHTMLConv::CiteLevelTXT and you're set. It detects plain text quotes and format=flowed quotes are just a special case here, because you don't know anymore, if the quote was format=flowed or plain text.
> We have the information flowed/non-flowed unambiguously - why guess? My thought was that the spec talks about allowing mixing flowed and non-flowed lines in the same document, so the quoting software should behave accordingly: do flowed quoting on flowed lines, otherwise do what we currently do (except for omitting the spaces between the quote characters, and fixing the bugs in quote level detection).
> My thought was that the spec talks about allowing mixing flowed and non-flowed > lines in the same document All non-flowed lines are fixed. This includes - lines intended to be fixed (e.g. ascii-art) - lines quoted from a fixed-width format (e.g. normal plain text) - paragraphs fitting in one line . This is IMO the largest weakness of RFC2646. > do flowed quoting on flowed lines, otherwise do what we currently do Note: <quote src="ftp://venera.isi.edu/in-notes/rfc2646.txt"> 4.5. Quoting [...] However, the line > > Exit, Stage Left is different. It has a quote-depth of one, and a content of "> Exit, Stage Left". </quote> You might still decide to recognize the quoted plain text quotes (i.e. assume quote-level 2 in the above example) *for the rewrap feature*, since this feature is usually only invoked, if things are completely broken in the first place.
> - paragraphs fitting in one line - flowed (!) paragraphs fitting in one line > *for the rewrap feature* (and only for that)
added fix by date in status
Whiteboard: [nsbeta2+] → [nsbeta2+][8/2]
Moving from [nsbeta2+] to [nsbeta2-] per todays Composer QA Beta2 Status review.
Whiteboard: [nsbeta2+][8/2] → [nsbeta2-]
Keywords: nsbeta3
Keywords: nsbeta2 → correctness
Whiteboard: [nsbeta2-]
Target Milestone: M17 → M18
setting to nsbeta3+
Whiteboard: nsbeta3+
setting priority in status whiteboard
Priority: P3 → P4
Whiteboard: nsbeta3+ → [nsbeta3+][p:4]
Priority: P4 → P3
Whiteboard: [nsbeta3+][p:4] → [nsbeta3+][p:3]
Ben asked me to stick my two cents in here ... I don't pretend to fully understand this bug (I get a headache just reading it). But apparently one way it could be fixed would be to strip trailing spaces from (non-flowed) lines -- something which Daniel (judging by his comments on 2000-06-22) doesn't like the sound of. I'm the FAQ maintainer for a group which could be expected to care about this issue quite a lot, <news:alt.ascii-art>. And I can say quite confidently that removing trailing spaces at sending time would do much more good for us than it would do harm. Often when doing the sort of layout-y things that the plaintext composer allows, posters insert/leave trailing spaces by mistake, and this makes the picture suffer from unnecessary `wrapping damage' when viewed in a window which is slightly narrower than the composition window of the author. In contrast, I have *never* seen a message where trailing spaces were inserted intentionally. If this is irrelevant, just ignore me. :-)
> I can say quite confidently that removing trailing spaces at sending time > would do much more good for us than it would do harm. Be aware that this will loose the flowing information in quotes, i.e. quotes won't flow anymore. (Since you said on .mail-news that this were be broken.) I see no other way to preserve this info.
also, I know of at least one significant trailing space in mail, the space in the dash-dash-space sequence: -- Ian
hixie, that gets special treatment anyway.
I said, remove trailing spaces from *non-flowed* lines. So quotes would still flow, assuming RFC 2646 caters for that.
Matthew, and how do you tell the difference? If you mean "flowed = wraps at composer window width", then quotes count as fixed, because Gecko considers ">" being content (the closest layout thing Gecko knows are vertical bars) and would let them also flow. If you mean "flowed = has trailing spaces", this would be my inital proposal, which has the downside that we cannot tell at sending time, who or what inserted them (i.e. user-inserted trailing spaces would make that line flowed).
I don't think it's at all likely that at a user is going to insert trailing spaces in quoted text when they reply to it. Snip quoted text? Sure. Insert `[...]' in it? Maybe. But insert trailing spaces? Nah. I think the benefit (flowed quotes) far outweighs the risk (an occasional single errantly-flowed line of quoted text).
So, you're proposing special-casing: While sending, leave trailing spaces in quotes, but strip them for the rest? My only objection would be that this "modality" is confusing for the user, but otherwise, this sounds good to me. Comments?
See my comment of 2000-06-22 07:13 rather than forcing me to say it alla again.
Catching up on the latest comments preparatory to actually writing code ... Thanks, Matthew, for joining the discussion and giving us an eye into the ascii-art community! That's a lot better than speculating what they might or might not prefer. Reading back, I think there are three fixable issues here: 1. Nested quotes are quoted wrong by the citer, and when rewrapped, are rewrapped wrong. I'll look into calling Ben's stream class for fixing this. 2. We want to treat the plaintext editor as a wysiwyg tool for folks who know exactly what they want their message to look like; this means that the output shouldn't be flowed. I think this should apply to quotes as well (there's always rewrap, if needed) but I'm not sure we have consensus on that (Matthew, does it sound okay to send everything as fixed, including quotes?) This means work in the output sink and may require another output flag (or perhaps the output sink should just key off whether there's a whitespace: -moz-pre-wrap style node on the body?) 3. We may also want the citer to strip trailing newlines when quoting material, so that when the user quotes (or does paste-as-quotation) from material which might have trailing spaces for some other reason (e.g. ascii art) it doesn't get misinterpreted as flowed. OTOH, if we do step 2, this won't matter because the spaces will be stripped at output time. Comments? I want to make sure I'm doing the right thing before I start writing code -- I expect that the three of you understand this better than I do.
> 1. Nested quotes are quoted wrong by the citer I think, Daniel's patch will do it for quoting format=flowed, but it shouldn't be used for format=fixed, because you'd so some kind of quote recognition then (what happens, if the line starts with ">From"?). > and when rewrapped, are > rewrapped wrong. I'll look into calling Ben's stream class for fixing this. Isn't rewrap another bug? > plaintext editor [...] the output shouldn't be flowed. Eh? The plaintext editor does put out flowed for normal text since a year or so. Do you want to change that now? More comments tomorrow, possibly.
> We want to treat the plaintext editor as a wysiwyg tool for folks who know > exactly what they want their message to look like; this means that the output > shouldn't be flowed ... Matthew, does it sound okay to send everything as > fixed, including quotes? That sounds (to my technically deficient mind) like you're proposing removing all send-time support for format=flowed altogether. Which is not what anyone wants, I don't think. Here's what I suggest, at send time: * Remove all trailing spaces (from both quoted and directly-entered lines). * Insert trailing spaces to signify format=flowed where appropriate -- in both directly-entered lines, and quoted lines which were flowed when they were inserted into the message. The only case where that would break, I think, is where I construct my own quotation using the `>' and Enter keys, because Mozilla wouldn't know that I intended it to be a quotation. But I see no way around that, short of doing what Daniel suggests and replacing the plaintext composer with a `plain text' mode in the HTML composer -- a mode where the only formatting available is a `Quoted' paragraph format, to insert >s where necessary.
> * Remove all trailing spaces (from both quoted and directly-entered lines). > * Insert trailing spaces to signify format=flowed where appropriate -- in both > directly-entered lines, and quoted lines which were flowed when they were > inserted into the message. Matthew, again: How do you tell that the quote was flowed, if you remove the trailing spaces? That's the whole problem here. Also, changing only the functions at sending is not sufficient: The quoting is broken also. If we quote a format=flowed msg, we should preserve the qoute information for all quotes, including nested ones. This requires the space between the first, inserted ">" and the following, qouted ">"s to be removed (see Daniel's patch), but for format=flowed only.
> Matthew, again: How do you tell that the quote was flowed, if you remove the > trailing spaces? Because while the quote is sitting in the composition window, it has a <div class="-moz-flowed">...</div> (or whatever) enclosing it, doesn't it? If it doesn't (if the plaintext composer doesn't contain any markup at all), then I don't see how this bug can possibly be fixed. What does Eudora do in the same situation? Has RFC 2646 specified behavior here which is actually impossible to achieve in a plaintext editor?
Eudora uses blockquote-style composing. More interesting might be to look at hotmail which can't use that method. They leave the trailing SPACE at quoted lines and when making a mail from the data in the text box they assume that quoted lines (lines beginning with a '>') and ending with a space is flowed. That is the same as us I think. For unquoted lines they do a little differently. There, they strip triling SPACE so writing a SPACE at the end of a line won't automagically make that line flowed. I think Hotmail's approach is reasonable. Quotes are seldom handwritten so one may assume that trailing spaces are preserved. If we don't do as Eudora, I vote for the Hotmail way. That is: * To leave spaces at the end of quoted lines to signal flow. * To strip trailing spaces at the end of composed lines. This is not perfect, but will let as many as possible benefit from format=flowed while keeping side effects quite low. I'm not sure what the differences between this and what we do today are, but I'm sure someone can illuminate us.
> If it doesn't (if the plaintext composer doesn't contain any markup at all), > then I don't see how this bug can possibly be fixed. Well, see the original description - this bug is a real bug and has to be fixed anyhow. Personally, I like your suggestion to preserve trailing spaces in quotes only best. > What does Eudora do in the same situation? I think, it doesn't have a plaintext editor.
Ops, didn't saw Daniel's comment. Looks like we have a winner! I like Matthew's suggestion, Daniel likes it, Hotmail uses it, Akk doesn't seem to have a strong opinion here (correct me, if I'm wrong). (I didn't know that Hotmail supports flowed at all. Does it really send it out? Would be cool...) 1. So, my inital suggestion (in the description of the bug) happens to still stand. You can take Daniel's code (see patch above), but only for quoting format=flowed. If quoting format=fixed, leave as is. This might mean to have to carry the content-type around, which is a non-trivial change :(. 2. However, in the meantime, somebody changed the code (or is it my prefs?), so that it strips trailing spaces during quoting. (This means that this bug as initially described doesn't appear anymore, but also means that quotes don't flow. But is that still nsbeta3+ worthy?) Change that back. 3. The additional problem raised in that bug - trailing spaces in new content make lines flow - seems to have disappeared as well. In this case, it has been fixed as suggested: Trailing spaces in new text are stripped, while they are preserved in quotes. Akk, while fixing this bug, please verify these statements.
Okay, just to be sure I'm clear, here's what I think the consensus is: change the plaintext output sink so that when we're outputting plaintext (i.e. the body style is some variant of pre), - don't strip trailing spaces in quoted text (we think it's stripping them now, though nobody remembers doing that intentionally) - do strip trailing spaces in unquoted text (we think it's not stripping them now). I'll need to write a text to make sure this is working as expected. I'm not sure how to test it "live", though, especially since I think we still have differences between platforms in whether mozilla sends/displays flowed or not (I think that bug was futured, but I can't find it to confirm). I'd welcome suggestions and a test case for live testing, and please let me know if I'm missing anything here.
Akk, and don't forget the InternetCiter. The plaintext output sink might be involved 2 times, right? Original, quoted HTML msg -> nshtmltotxtsinkstream (with the original msg) -> internetciter -> plaintext editor (<pre>) -> nshtmltotxtsinkstream (with the editor content, i.e. the quoted and new text) -> send The first run of nshtmltotxtsinkstream must strip trailing spaces in the content and generate those for flowing paragraphs. The internet citer must be adjusted not to output "> " before already quoted content for format=flowed, because that would loose the quote information for nested quotes. The second run of nshtmltotxtsinkstream must be told not to strip trailing spaces for content in <pre> and beginning with ">" (this propably requires a new flag).
I've been trying to come up with a test case and to figure out what the code is currently doing. I've learned a couple of interesting things: 1. The plaintext outsink doesn't do any handling of plaintext quotes, only html quotes. It doesn't look for ">" or count the levels. This means that it doesn't know when it's in a quote. Hopefully I can just key off whether mCurrentLine[0] == '>', but that hasn't been working in my little tests because mCurrentLine doesn't actually correspond to the line breaks; it gathers up an appropriate number of words to fill a line, but the string itself has line breaks in it. 2. I also can't seem to get it to flow text. If I give TestOutput a file with lots of short lines ending with spaces, and specify the flowed flag, shouldn't it flow them together, or do we keep the user's ragged line lengths and rely on the MUA on the other end to flow the message (so people with non-flowing MUAs will see ragged line lengths). I'm getting very confused on how we're managing to flow at all (if we are), and why plaintext quotes aren't more screwed up than they are, and will need help on testing this.
Blocks: 51799
The format=flowed decoder, that parses quotes, is in mailnews/mime/src/mimetpfl.cpp. I guess you are working on the second of the two passes Ben mentions above. Then the thing to do is 1. To strip trailing spaces from lines not starting with a quote. 2. Not strip trailing spaces from lines starting with a quote. If the parser hands you chunks of text containing linebreaks and the output is not formatted, then maybe it's a little messy, since those chunks are output in one piece if my memory serves me correctly.
> The format=flowed decoder, that parses quotes, is in > mailnews/mime/src/mimetpfl.cpp. But, that is not used at all in our scenario, right? > I guess you are working on the second of the two passes Ben mentions above. (of nshtmltotxtsinkstream) Yes. Note that the stripping doesn't necessarily has to be done on the sink. It is some kind of a hack for the plaintext composer, so, I think, you could as well create any intermediate step that is only used for the plaintext mailnews composer.
Depends on: 52376
I'm still confused about my role here (I'm sorry, I must seem very dense, but it's hard when there doesn't seem to be an actual visible bug I can reproduce). Can someone please either 1. explain what the current visible symptom of this bug is -- how do I reproduce it? or 2. explain to me what change needs to be made where, e.g. "in nsInternetCiter::GetCiteString, always add trailing spaces if they aren't already there", or "in nsHTMLToTXTSinkStream, strip trailing spaces" or "in the sink stream, it's currently stripping spaces and it shouldn't be if the flowed flag is set". Every time I look at this and try to actually make any code changes beyond Daniel's patch to correct the nested quotes in the internet citer (that much I understand, and it will go in with my upcoming fix for rewrap) I find that I"m still confused about what's currently thought to be wrong.
Ben clarified for me on IRC: "current visible symptom of this bug": quote flowing msg using plaintext composer. send. read. quote won't flow. it should. So the problem, we think, is that if you have a flowed message in the mail window, and you reply to it in plaintext mail compose, by the time the text gets out of plaintext mail compose, it no longer has trailing spaces on the quoted stuff. We don't know at what point the spaces are getting stripped. This is the remaining bug (I just checked in a fix for the citer so it will do ">> " instead of "> > ") and the one I am working on now.
Here's the deal. The spaces are not getting added in the first pass through nsHTMLToTXTSinkStream, when mail calls it in order to save the message to pass to the editor. The spaces are not getting added because mail is not setting the OutputFormatFlowed flag in order to tell the sink stream to add the spaces. Rich, Ducarroz, is this intentional that we're not flowing quoted material?
#13 0x421044d2 in ConvertBufToPlainText (aConBuf=@0x8e49698, formatflowed=0) at nsMsgCompUtils.cpp:2008 #14 0x4210df90 in QuotingOutputStreamListener::ConvertToPlainText ( this=0x8e49688, formatflowed=0) at nsMsgCompose.cpp:1243 The reason that flowed is turned off turns out to be this clause at the end of UseFormatFlowed: // Just the check for charset left. // This is a raw check and might include charsets which could // use format=flowed and might exclude charsets which couldn't // use format=flowed. // // The problem is the SPACE format=flowed inserts at the end of // the line. Not all charsets like that. if( nsMsgI18Nmultibyte_charset(charset)) return PR_FALSE; Charset is "UTF-8". So for UTF-8, we're not allowing format=flowed at all. Why? lxr points to nhotta as the author of this code -- I will ask him.
Naoki is going to take a look. Naoki, here's what I'm doing to test this: using the mozilla plaintext compose window, send myself a message with a couple of short paragraphs (2-3 lines each, long enough that they might have to wrap). Then in mozilla mail, read this message, and reply to it in plaintext. (Shift-New and shift-reply will bring up the plaintext compose window if your normal setting is html.) Break in one of the routines mentioned, and note whether flowed is being sent.
Okay, I can reproduce this on my WinNT. I think for the quote operation, the message charset is after the quotation completes before that libmime internal charset (UTF-8) is used. nsMsgI18Nmultibyte_charset just return true for any multibyte charset. But for this case we do not have to include UTF-8. So we can check for UTF-8 before calling nsMsgI18Nmultibyte_charset and allow UTF-8 charset to be sent as a flowed message.
>the message charset is after the quotation completes I meant to say, the message charset is set after the quotation completes
Naoki: if I change the line in UseFormatFlowed to read as follows: if( nsCRT::strcmp(charset, "UTF-8") && nsMsgI18Nmultibyte_charset(charset)) return PR_FALSE; then flowing seems to work again (I can reply to a flowed message, and both the quoted text and my reply come out flowed when I read them in the mozilla mail window). Can you review this, or tell me if there would be a better way? Thanks.
Please use strcasecmp, otherwise looks good, r=nhotta.
Won't this break some asian languages that this code was supposed to fix? I guess, if the charset is utf-8 it could _really_ be a CJK charset that is damaged if inserting the space. Or is it already damaged by the insertion of the quote sign so this doesn't matter? Or is the space stripped somewhere later if it's CJK maybe.
Fix checked in. I think the idea was that if the message was in Japanese or other languages which would be problematical, the charset would be listed as something other than UTF-8. I hope that's true; otherwise we'll have to revisit this and find out why the charset isn't set correctly early enough.
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
verified in 9/15 build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.