User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10 ( .NET CLR 3.5.30729) Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.9) Gecko/20100915 Lightning/1.0b2 Thunderbird/3.1.4 The message editor which is opened by "Edit As New" seems to use the character encoding of the current (selected) message, not the message to be edited. Reproducible: Always Steps to Reproduce: 1. Save two messages with different character encoding. e.g. ISO-2022-JP for the message1 and UTF-8 for the message2. 2. Click message1 in the message list. The contents of the message1 will be displayed with correct character encoding. 3. Right click (without selection) message2 in the message list, and choose "Edit As New." Message2 will be opened with the message editor, but it will be displayed with incorrect character encoding.

Confirmed with Tb 3.1.4. - Problem occurs on "Edit As New" and "Forward". No problem with "Reply (All)". - Auto Detect=On/Off is irrelevant. - charset of Content-Type(or no charset) itself is irrelevant. Note: If any test mail has different charset from "default charset of mail composition", used charset is shown at window title of any compose window. It makes phenomenon clearler.

Is this a regression ?

> Thunderbird version 0.6+ (20050201) Forward : Problem is observed Edit As New : Problem is not observed > Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.10) Gecko/20100512 Thunderbird/3.0.5 Forward : Problem is not observed Edit As New : Problem is observed > Thunderbird 3.1.4 Forward : Problem is observed Edit As New : Problem is observed It looks for me old problem of UI. By "open context menu at different mail", selection is temporarily moved. By click of "Forward" or "Edit As New", selection is switch back. "Current mail" for "opening composion window" depends on timing. As controlling around UI is frequently changed, different phenomenon is observed with different Tb version.

It looks for me simlar issue to next problem o UI. > Bug 553859 Rebuild-Index via Right Click only(no explicit folder selection by Left Click) > produces mismatch between "selected folder at UI" and "highlight'ed folder at folder pane"

Phenomenon at compose window depended on Tb version. > Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.10) Gecko/20100512 Thunderbird/3.0.5 Edit As New : Problem is observed Subject : Not garbled by wrong charset Mail body : Not garbled by wrong charset > Thunderbird 3.1.4 Forward / Edit As New : Problem is observed Subject : Not garbled by wrong charset Mail body : Garbled by wrong charset > Mozilla/5.0 (Windows NT 5.1; rv:2.0b6pre) Gecko/20100906 Thunderbird/3.2a1pre Forward / Edit As New : Problem is observed Subject : Garbled by wrong charset Mail body : Garbled by wrong charset Because mail data was not garbled by wrong charset due to this bug till Tb 3.0, users weren't aware of this bug until Tb 3.1 was released. Mail was probably sent in UTF-8 if wrong charset was set due to this bug till Tb 3.0.

I'm observing a similar behavior (not sure if it's exactly the same, though): When pressing Ctrl+E (for "Edit message as new") from the main window (i.e. cursor bar is on the message), the compose window that comes up SOMETIMES has all special characters broken (i.e. wrong encoding). When this happens, I close it and press Ctrl+E again; and usually, after a few attempts, it finally works. Very annoying, but hard to reproduce - so I can't specify the exact circumstances at the moment :-(

I can confirm the "random nature" of this problem. Sometimes it seems to work, sometimes not. It might have to do with the info above regarding it depending on the encoding of "the current (selected) message". I'm not completely sure what that means though, since it would seem to me that if you right-click on a mail (in order to select "Edit As New..." or "Forward"), it will always itself be "the current (selected) message", but apparently it still loads with the wrong encoding when doing this? Anyway, one piece of helpful information might be that if you first open the mail in question, and THEN select "Forward" directly in the window of this mail (i.e. with the forward button in this window), it WILL get the right encoding, while if you right-click the same email directly from the mailbox (e.g. "Sent" or "Inbox") and select "Forward" from this pop-up menu, it will NOT work (i.e. the encoding will be incorrect).

Maybe I've found a way to reproduce the problem reliably. See bug #739063 comment #3.

Well, this bug just adds to the endless saga of TB's well-known affinity to bogus-focus. Try this in your OS file manager: single-left-click file1 -> file1 is selected and focused right-click file2 -> file2 is selected *and* focused (but Thunderbird doesn't care about focus, hence stubbornly keeps focus on file1, i.e. msg1) Even for multiple selections (again, in your OS file manager): single-left-click file1 -> file1 is selected and focused hold Ctrl while single-left-click file2 -> file1 and file2 are selected, and *file2* has focus let go of Ctrl (2 files still selected) right-click on file1 -> file1 gets focus (!) (2 files still selected) ESC, then right-click on file 2 -> file2 gets focus (!) (2 files still selected) So even within a multiple selection, right-clicking on another part of selection is supposed to move focus there, while preserving selection (Thunderbird doesn't move the focus). Life could be so simple if we just did like all other applications: Focus follows mouse click. I don't see any exceptions for that in our design rules: https://developer.mozilla.org/en/XUL_Tutorial/Focus_and_Selection https://wiki.mozilla.org/XUL:Focus_Behaviour I realize that this might be a deliberate exception to preserve multiple selections and make them more stable. I'm not sure how many users really make complex selections and then, before acting on the selection, decide to take the risk and act on a single message outside of selection. And how are users supposed to find out? There is no precedence behaviour for this type of UI anywhere outside TB. Even if we actually want that extraordinary sticky selection behaviour, we still don't get it right: 1) We are inconsistent with ourselves. Behaviour is different for other lists, e.g. contacts pane in AB. 2) Dotted focus border must *always* indicate where keyboard input will be directed to. In case of context menu, all keyboard input goes to context menu. So dotted focus border, at least for as long as the context menu for single message is open, needs to be on that single affected message, not somewhere in the middle of nowhere of a temporarily hidden selection (because as long as that context menu is open, the old selection *cannot* get keyboard input!). Even when you right-click on drafts folder while there's selection in inbox, it's so confusing to see the focus border move to inbox while you've just right-clicked on drafts (which is also selected) - and each time I'm wondering which of the two will be affected by contextual actions!

In a nutshell, I find it very confusing how we break "Focus follows mouse click" rule - one of the most basic user interaction principles (regardless if left or right click).

Bug is still present in TB 17.0.4 in Windows Vista and Ubuntu 12.10.

Bug is still present in TB 17.0.8 in Windows 7. This bug is really annoying for reporting cases where previous sent mails are used for composing the new message. A fix to this would be very welcome!

Is there any progress? I seem to have the same problem (with forwarding previously sent messages). Seems to be random, and a TB restart helps sometimes. But it's hard to give consistent observations, as it doesn't affect me often. 17.0.8 here. Sorry to say, but this issue is seemingly very old. Hardly 'new'. Some (among many) web mentions of the problem: http://forums.mozillazine.org/viewtopic.php?f=39&t=1089335 https://getsatisfaction.com/mozilla_messaging/topics/broken_encoding_in_messages_forwarded_by_thunderbird_for_cyrillic_text https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/1166104 http://forums.mozillazine.org/viewtopic.php?p=11010395&sid=74935483f0008d8f029d4e6c0a528293#p11010395 (the solution given here doesn't work). Please correct me if I'm mistaken here. Thanks.

PS. No idea if it helps, but the last case I've encountered seems to be independent both from the action the forwarding is accomplished with, and the F8 panel status.

follow-up kindly requested

"Edit as new" as NEVER worked for me, EVER, literally for YEARS. It always seems to go into "plain text" e-mail mode and the composition toolbar does not show. Isn't this EVER going to be fixed???

An observation: When working on a draft, "Edit as new" MIGHT bring the error, but double-click MIGHT not bring it. Recent occurrence.

I looked at the code, the chain of calls for the forward action seems to be: thunderbird-31.5.0+build1/mail/base/content/mailWindowOverlay.js: function MsgForwardMessage(event) function MsgForwardAsInline(event) function composeMsgByType(aCompType, aEvent) thunderbird-31.5.0+build1/mail/base/content/mailCommands.js: function ComposeMessage(type, format, folder, messageArray) thunderbird-31.5.0+build1/mailnews/compose/src/nsMsgComposeService.cpp: nsMsgComposeService::OpenComposeWindow(const char *msgComposeWindowURL, nsIMsgDBHdr *origMsgHdr, const char *originalMsgURI, MSG_ComposeType type, MSG_ComposeFormat format, nsIMsgIdentity * aIdentity, nsIMsgWindow *aMsgWindow) nsresult nsMsgComposeService::LoadDraftOrTemplate(const nsACString& aMsgURI, nsMimeOutputType aOutType, nsIMsgIdentity * aIdentity, const char * aOriginalMsgURI, nsIMsgDBHdr * aOrigMsgHdr, bool aForwardInline, bool overrideComposeFormat, nsIMsgWindow *aMsgWindow) nsresult nsMsgComposeService::RunMessageThroughMimeDraft( const nsACString& aMsgURI, nsMimeOutputType aOutType, nsIMsgIdentity * aIdentity, const char * aOriginalMsgURI, nsIMsgDBHdr * aOrigMsgHdr, bool aForwardInline, const nsAString &aForwardTo, bool aOverrideComposeFormat, nsIMsgWindow *aMsgWindow) In RunMessageThroughMimeDraft there is an "if (aMsgWindow)" code block with the following comment: // if we are forwarding a message and that message used a charset over ride // then use that over ride charset instead of the charset specified in the message Commenting out that block I got the correct behaviour. So, I guess the alternatives are: 1) There is a bug in the if block. 2) The block must be removed. 3) The block shouldn't be run in this use case, therefore aMsgWindow should be NULL, then ComposeMessage should call OpenComposeWindow with a null msgWindow. 4) OpenComposeWindow is fine, and the bug is only solvable with substantially more work. My bet is on (3), not passing msgWindow to OpenComposeWindow, but I don't know if there are side effects. Please note, I am not a Mozilla contributor, I'm just counting on saving some work for any mantainer who could pick this bug.

Bug still present in 38.4.0

Bug still present in TB 45 (Earlybird). I've just reproduced it in bug 644780, comment #2. This is annoying. I just saw a case where quotes (ANSI 0x92, U+2019) got messed up due to this.

Julio, nice work. It's great to come to a bug where someone else has already done some investigation. I'm printing out here https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgComposeService.cpp#1619 what nsMsgWindow::GetMailCharacterSet() returns. And it returns the wrong thing. If you double-click a draft or use a message for "edit as new" or forward which has charset, say windows-1252, it may return UTF-8, if the previously *viewed* message was UTF-8. So that's the problem. nsMsgWindow::GetMailCharacterSet() only returns a member variable: https://dxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgWindow.cpp#275 If a message was never viewed, either because the previous pane was switched off or by tricky right-click without viewing the message (see bug 644780, comment #2), then what is returned may be wrong, since it comes from the previously viewed message. BTW, the override covers this case: If the user opens a message and then changes the text encoding to something else, then that user-set charset should override the message's own charset. In other words: Charset used to view overrides message's charset (regardless of edit draft, "edit as new", forward). The problem is that the charset used to view a different message is used as override. Your suggestion to remove or disable the whole block is sadly not a possibility, since it would destroy the function I've just described. The solution is to stop the override if the message wasn't viewed before. And that's the tricky bit. While I'm digging around a bit, I might as well ask the experts for some inspiration.

Magnus, Neil: I will store the the message URL here https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimemoz2.cpp#727 and here https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/nsStreamConverter.cpp#180 together with the charset. I'll change the GetMailCharacterSet() function to also return the message URL, so then I can check that the charset really applies to the right message. Or do you have a better idea?

I'd like to propose, that the above new description might be erroneous. I had cases when the problem occurred repeatedly (although not inevitably) regardless of if the message was viewed or not, and sometimes even restart of Thunderbird wouldn't help. Also, changing the text encoding wouldn't help in some cases.

Well, I can only fix the bug I can reproduce. It's clearly reproducible and my analysis also shows why the problem occurs. You might have a different problem. Once the bug is fixed, you can download a Daily build and see whether your problem is fixed, too.

Got the message. I'll check it. I was just suggesting narrowing the scope of the bug might not be desirable. Please also see my comments above for reference. BRG

Magnus, Neil: This was I foolish idea. I wanted to change GetMailCharacterSet() and SetMailCharacterSet() to pass the URL of the message so we don't get the charset for the wrong message. Turns out that these functions are linked to the mailCharacterSet attribute: https://dxr.mozilla.org/comm-central/source/mailnews/base/public/nsIMsgWindow.idl#63 and can't be changed. So I should wait for your answer. My aim is to store something that identifies the message that caused the charset to be stored, so it can be compared when the charset is retrieved.

OK, I'm storing the message URL with the charset to be able to compare it on retrieval. What do you think, gentlemen?

Comment on attachment 8705061 [details] [diff] [review] Possible solution (v1). Review of attachment 8705061 [details] [diff] [review]: ----------------------------------------------------------------- I don't have a solution, but this does look like a very hacky thing to do. I think somehow we just have to make the mailCharacterSet not lie.

I disagree. nsMsgWindow::SetMailCharacterSet() gets called "casually" by the MIME parser. It says: By the way, the message I just looked at had charset such and such. The underlying assumption is that the message which gets "edited as new", forwarded or used as a draft will be viewed before processing it. This assumption is false as this bug shows. So basically, the current state is a big hack. Registering which message was actually parsed to derive the charset makes things better. Saying "I don't like it" doesn't help. There is no good way to forget the set character set. I refer you to this comment: https://dxr.mozilla.org/comm-central/source/mailnews/base/public/nsIMsgWindow.idl#72 Same here. There is no good place to clear the charset. NB.: This is actually a dataloss issue and a very embarrassing one too. A friend of mind who drives without preview pane repeated his Christmas message and guess what, this bug shredded through a good part of his messages and they got sent with garbled content since the text was below a picture at the top so he didn't recheck that the text looked OK before sending.

Wadr, so what's the problem here? Is is a failure of imagination? Not knowing the system(s) well enough, how they work and interact? Not being particularly strong at debugging and problem solving? There are very few "simple" problems like this that cannot usually be fairly easily corrected. You have to right-click on an e-mail item in the list box (afaik there is no menu item) so you know what the source e-mail is, and then theoretically what charset it is. It seems relatively straight forward. Even if it's just a matter of passing the information needed down the line to the final "procedure" so it sets things correctly. Again, wadr, it cannot be THAT difficult of a problem to solve. This is just an e-mail client program. It doesn't get too much simpler than that. (that is, a program with any minimally decent features and functionality) Pardon me if I'm missing something, but I've been a "high level" programmer for too many years and have worked on programs this "complex" and MUCH more complex and complex systems of programs and way too much other stuff like that, so I know what it's like. Thanks for working on it at least.

Dear Bill Dee, <off-topic> With all due respect (wadr), I can't see that your comment is contributing anything. Let me give you some background: Thunderbird and Firefox have about 7 million lines of code. That makes them more complex than Libreoffice, yet they are loss complex than Linux. In general, web browsers are highly complex programs, and Thunderbird, a mail-client that is based on a web browser, is equally complex. If you look at all the functions Thunderbird offers, "just an e-mail client program" is quite inappropriate. Currently Thunderbird is maintained by a dedicated group of volunteers, who don't receive a cent for the countless hours of hard and very qualified work they invest. If you've worked on "much more complex" programs, why don't you join the team and fix the bug yourself. You an start your analysis here: https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgComposeService.cpp#1619 Your comments what a great guy you are and how easy this problem should be, don't help at all. Currently you have exactly two ways to guarantee that a bug gets fixed: First, you fix it yourself. Second, you hire a programmer who will fix. In the future, keeping in mind that Thunderbird is currently in transition from being part of Mozilla to establishing its own legal and financial home, there will be a system by which bugs can be prioritised by users. </off-topic> The cause of the problem has been analysed and a possible solution has been presented. Sadly the bug is in an area that is not well structured, that's why no developer has taken an interest until recently. Now the group of developers will decide whether there are better ways than the proposed solution and the bug will be fixed in due course. Kind regards, Jorg K.

(In reply to Jorg K (GMT+1) from comment #34) > I disagree. > > nsMsgWindow::SetMailCharacterSet() gets called "casually" by the MIME > parser. Looks to me (without testing) it would be called, mainly, from http://mxr.mozilla.org/comm-central/source/mailnews/mime/src/mimetext.cpp#362 - always when parsing the message line by line. > The underlying assumption is that the message which gets "edited as new", > forwarded or used as a draft will be viewed before processing it. This > assumption is false as this bug shows. Yes but it still needs to be parsed, right? > So basically, the current state is a big hack. Registering which message was > actually parsed to derive the charset makes things better. Well you still do need to get the real charset it should have, so I'm not sure I see the difference. What i mean is, if you need to get the real charset, why not always just use that?

(In reply to Magnus Melin from comment #37) > Looks to me (without testing) it would be called, mainly, from > http://mxr.mozilla.org/comm-central/source/mailnews/mime/src/mimetext. > cpp#362 - always when parsing the message line by line. Well, the function in question, SetMailCharacterSet() gets called twice: https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/nsStreamConverter.cpp#180 https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimemoz2.cpp#727 The second call is in SetMailCharacterSetToMsgWindow() which itself has a bunch more callers in MIME code. I don't really need to list them all, you can see them with one click: https://dxr.mozilla.org/comm-central/search?q=SetMailCharacterSet&redirect=false&case=false > Yes but it still needs to be parsed, right? One would assume so, otherwise it'd be hard to get it into the composition window ;-) I think this comment is besides the point. This bug is about the *override* charset: When you view a message and select a different charset for viewing, then this charset is used for forwarding, "edit-as-new" and "edit draft". It *overrides* the original charset of the message. Equally, if the folder has an override charset defined. Without question, all works well, if we don't attempt to *override* the charset. In comment #21 the friendly analyst already stated that when removing the code that does the overriding at https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgComposeService.cpp#1617 the problem goes away. Please read the second part of my comment #26 that explains this. BTW, the override is enabled here: https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgComposeService.cpp#522 > Well you still do need to get the real charset it should have, so I'm not > sure I see the difference. What i mean is, if you need to get the real > charset, why not always just use that? I don't understand this. We have the real charset, this is about *not* using the real charset and using a user set override instead. And that comes from the wrong message and there is no convenient way to clear it. So far, my solution is the only viable. When viewing a message which potentially sets the *override* charset, we also note for which message URL this was set. So then when we come to doing the override, we only use it, if it was set for the message in question. All I can offer is changing the name of the attribute from "urlForCharacterSet" to "urlForOverrideCharacterSet". BTW: The two calls to SetMailCharacterSet() cover the two cases: One is for viewing the message and determining the charset this way, the other call in the stream converter sets the override from the folder property. Please reconsider the f-.

I see. Maybe you could just reset msgWindow.charsetOverride to false on parsing? That way you wouldn’t apply the override in nsMsgComposeService::RunMessageThroughMimeDraft and there would be no problem.

Let's look at something else first: I've just tried whether this bug is also an issue for "Reply". It is. But it's even more twisted: https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#149 If you *change* the charset of the previously viewed message, then the reply will be in that encoding. Something else my patch would have to repair. Looking at the reply case makes me think. There the charset will only be carried over from the previously viewed message to the *reply*, if it was *changed* for the previously viewed message (setting the override), not always as for forward/edit-as-new/edit-draft. So removing this line: https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgComposeService.cpp#522 - aMsgWindow->SetCharsetOverride(true); + // Do NOT always set override. It gets set if the user changes the text encoding. + // aMsgWindow->SetCharsetOverride(true); will remove the charset carry-over from the previous message as long as the charset of the previous message isn't changed manually. So summarising: A one-line-change, removing the line above, fixes 95% of the problem. If you view message 1 and then reply-to/forward/edit-as-new/edit-draft message 2 (and not view it before), the charset will only be carried over erroneously if the charset of message 1 was changed/overridden. Note that a folder-set charset causes an override. There is no problem if the charset was not overridden. Note: This line is the only line added in bug 494912 (attachment 438064 [details] [diff] [review]), but I think it's wrong in today's code base and also not needed any more. So now we're down to fixing the remaining 5% of the bug: message 1 is viewed, charset is changed, message 2 is "used" without viewing and receives the previous override. Now back to answering your comment: (In reply to Magnus Melin from comment #39) > Maybe you could just reset msgWindow.charsetOverride to false on > parsing? That way you wouldn’t apply the override in > nsMsgComposeService::RunMessageThroughMimeDraft and there would be no > problem. Can you please elaborate. Parsing the message that will fill the compose window, right? That assumes that you only parse each message once. So if you first view it, it gets parsed, the override gets reset. If the user then overrides, or the folder overrides, then the flag gets set. If the user then "uses" the message, the override is used, assuming it's not parsed again. If they move on to a different message, that one gets parsed and the flag gets reset. Is that what you meant? I'd have to investigate that. It would be brilliant to find a convenient spot for your reset.

This takes Magnus' idea further of clearing the charset override flag. Sadly I haven't found a convenient spot to clear it, so setting the URL for the message will now clear the flag if it was set for a different message. Forward/edit-as-new/edit-draft works now and doesn't carry over the charset from a previously viewed message whose charset was manually changed or via the folder property. Reply still has the issue, I will look at this now. I also need to set the message URL when replying here: https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#1835

(In reply to Jorg K (GMT+1) from comment #41) > Sadly I haven't found a convenient spot to clear it, ... Let me elaborate. I tried to clear the flag in the parser, but in the genuine override case, the charset is set from the override before calling the parser. So clearing it in the parser is not what we want. Here some debug with the current patch: Viewing the message: ===== bridge_new_new_uri - URL(2) |mailbox-message://nobody@Local%20Folders/Henk%27s%20Christmas#876422| ===== bridge_new_new_uri |UTF-8|mailbox-message://nobody@Local%20Folders/Henk%27s%20Christmas#876422|0| ===== nsMsgWindow::SetUrlOfWindow - old |mailbox-message://nobody@Local%20Folders/Henk%27s%20Christmas#878311| ===== nsMsgWindow::SetUrlOfWindow - new |mailbox-message://nobody@Local%20Folders/Henk%27s%20Christmas#876422| ===== nsMsgWindow::SetUrlOfWindow - clearing override ===== nsMsgWindow::SetCharsetOverride: 0 ===== MIME - URL |mailbox-message://nobody@Local%20Folders/Henk%27s%20Christmas#876422| ===== nsMsgWindow::SetUrlOfWindow - old |mailbox-message://nobody@Local%20Folders/Henk%27s%20Christmas#876422| ===== nsMsgWindow::SetUrlOfWindow - new |mailbox-message://nobody@Local%20Folders/Henk%27s%20Christmas#876422| Manually setting the charset to something else: (happens here: https://dxr.mozilla.org/comm-central/source/mail/base/content/mailWindow.js#627) ===== nsMsgWindow::SetCharsetOverride: 1 ===== bridge_new_new_uri - URL(2) |mailbox-message://nobody@Local%20Folders/Henk%27s%20Christmas#876422| ===== MIME - URL |mailbox-message://nobody@Local%20Folders/Henk%27s%20Christmas#876422| ===== nsMsgWindow::SetUrlOfWindow - old |mailbox-message://nobody@Local%20Folders/Henk%27s%20Christmas#876422| ===== nsMsgWindow::SetUrlOfWindow - new |mailbox-message://nobody@Local%20Folders/Henk%27s%20Christmas#876422| Now forwarding that message: ===== RunMessageThroughMimeDraft |mailbox-message://nobody@local%20folders/Henk%27s%20Christmas#876422| ===== nsMsgWindow::SetUrlOfWindow - old |mailbox-message://nobody@Local%20Folders/Henk%27s%20Christmas#876422| ===== nsMsgWindow::SetUrlOfWindow - new |mailbox-message://nobody@local%20folders/Henk%27s%20Christmas#876422| ===== nsMsgWindow::GetCharsetOverride: 1 ===== bridge_new_new_uri - URL(1) |mailbox-message://nobody@Local%20Folders/Henk%27s%20Christmas#876422| Convinced? I'll now go and fix the reply case.

OK, this fixes the reply case: - View message 1 - Change the charset used for viewing - Reply to message 2 without viewing it first. Now uses the correct charset from message 2 and not the override from message 1. I spotted another problem: Whilst the replying to 2 uses the correct charset, the window title still shows the override charset from message 1. Terrible. Also, I've been testing on local messages so far. Using IMAP, this approach doesn't work (yet) since there is a mix of message URLs: View message: ===== bridge_new_new_uri - URL(2) |imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX%3E3261| ===== bridge_new_new_uri |ISO-8859-1|imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX%3E3261|0| ===== nsMsgWindow::SetUrlOfWindow - old |mailbox-message://nobody@Local%20Folders/Henk%27s%20Christmas#878309| ===== nsMsgWindow::SetUrlOfWindow - new |imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX%3E3261| ===== nsMsgWindow::SetUrlOfWindow - clearing override ===== nsMsgWindow::SetCharsetOverride: 0 ===== MIME - URL |imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX%3E3261| ===== nsMsgWindow::SetUrlOfWindow - old |imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX%3E3261| ===== nsMsgWindow::SetUrlOfWindow - new |imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX%3E3261| Change charset: ===== nsMsgWindow::SetCharsetOverride: 1 ===== bridge_new_new_uri - URL(2) |imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX%3E3261| ===== MIME - URL |imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX%3E3261| ===== nsMsgWindow::SetUrlOfWindow - old |imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX%3E3261| ===== nsMsgWindow::SetUrlOfWindow - new |imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX%3E3261| So far so good ... Forward (or Reply): ===== RunMessageThroughMimeDraft |imap-message://info%40hostalpancheta%2Ees@mail.hostalpancheta.es/INBOX#3261| ===== nsMsgWindow::SetUrlOfWindow - old |imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX%3E3261| ===== nsMsgWindow::SetUrlOfWindow - new |imap-message://info%40hostalpancheta%2Ees@mail.hostalpancheta.es/INBOX#3261| ===== nsMsgWindow::SetUrlOfWindow - clearing override ===== nsMsgWindow::GetCharsetOverride: 0 ===== bridge_new_new_uri - URL(1) |imap-message://info%40hostalpancheta.es@mail.hostalpancheta.es/INBOX#3261| So basically the URL initially began with "imap://" but then changed to "imap-message://". And the comparison fails and so the override gets cleared where it shouldn't. I'll stop here and wait for some feedback. If we were to continue this approach, I'd have to "unify" the message URLs and fix the reply window title.

Comment on attachment 8706400 [details] [diff] [review] WIP: Better possible solution (v3). Review of attachment 8706400 [details] [diff] [review]: ----------------------------------------------------------------- Hmm, another idea. I don't suppose we could just reset the charset override in the front-end? places http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#1938 ::: mailnews/base/public/nsIMsgWindow.idl @@ +60,5 @@ > void displayHTMLInMessagePane(in AString title, in AString body, in boolean clearMsgHdr); > > readonly attribute nsIPrompt promptDialog; > attribute ACString mailCharacterSet; > + attribute ACString urlOfWindow; I'd call it messageURL or something

(In reply to Magnus Melin from comment #44) > Hmm, another idea. I don't suppose we could just reset the charset override > in the front-end? places > http://mxr.mozilla.org/comm-central/source/mail/base/content/ > mailWindowOverlay.js#1938 I don't understand. We don't want to reset the override if the override was set for the message that is being used for composing (reply, forward, edit-as-new, edit-draft). We only want to reset it if it was set for another (previous) message. That's why me current approach takes note of the message being processed, if it it's different from the previous one, we clear the override. So you can use one message many times, forward it, reply to it, edit it as new, the override will hang around, until you move on to the next message. I have no better idea to notice when the message being "used" changes. > I'd call it messageURL or something Sure, but first we need to decide whether we're going to pursue this approach or whether you have a better idea. Frankly, having the messageURL handy in JS is not the worst thing that can happen. What do you know about the IMAP problem? How come the messageURL changes from "imap://" to "imap-message://"? I guess I will hunt down the other problem of the wrong charset being displayed in the composition window title myself.

OK, renamed the attribute to messageURL. Also, resetting the saved charset together with the override flag fixes the problem of the wrong charset being shown in the reply window's title. So the remaining issue is to make it work for IMAP where the URL changes from "imap://" to "imap-message://"

There is equally a problem when replying to a "saved" message. Here also, there seem to be two different message URLs in use. Some debug here: ==== nsMsgCompose::CreateMessage |file:///D:/Desktop/Hostal%20Cerrado.eml| ===== nsMsgWindow::SetMessageURL - old |mailbox:///D:/Desktop/Hostal%20Cerrado.eml| ===== nsMsgWindow::SetMessageURL - new |file:///D:/Desktop/Hostal%20Cerrado.eml| ===== nsMsgWindow::SetMessageURL - clearing override and charset ===== nsMsgWindow::GetCharsetOverride: 0 So basically the MIME code registers the message as "mailbox://", but the code that handles the reply uses "file://".

(In reply to Jorg K (GMT+1) from comment #45) > What do you know about the IMAP problem? How come the messageURL changes > from "imap://" to "imap-message://"? Not sure but I think the imap-message/mailbox-message/news-message uris are local notations, so they don't get confused with the remote urls wrt protocol handling.

The normal way to refer to a particular message is folder and key. Is there any reason you don;t propose that?

(In reply to Kent James (:rkent) from comment #49) > The normal way to refer to a particular message is folder and key. Is there > any reason you don;t propose that? Ignorance, I guess ;-) The places in the code I need to modify all handle a "URL/URI" (please look at the patch). As detailed on the mailing list, I get these variations: Local message, three variations: mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/Henk's%20Christmas?number=876422 mailbox-message://nobody@Local%20Folders/Henk%27s%20Christmas#876422 mailbox-message://nobody@local%20folders/Henk%27s%20Christmas#876422 <== Note: All lower case. IMAP message: imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX.Jordi%20Knobloch%3E139 imap-message://info%40hostalpancheta%2Ees@mail.hostalpancheta.es/INBOX/Jordi%20Knobloch#139 imap-message://info%40hostalpancheta.es@mail.hostalpancheta.es/INBOX/Jordi Knobloch#139 Saved message opened from OS folder: mailbox:///D:/Desktop/Hostal%20Cerrado.eml file:///D:/Desktop/Hostal%20Cerrado.eml What would be "folder" and "key" and what is a convenient way to extract them? I'm just checking, some of the places I want to tweak also have the nsIMsgDBHdr available. That has a method GetMessageKey, I wonder whether that would help. What about the case of a saved .eml file that is opened? That has not folder or key.

So what do I use? nsMsgKey k; aOrigMsgHdr->GetMessageKey(&k); nsCString n; nsCOMPtr<nsIMsgFolder> f; aOrigMsgHdr->GetFolder(getter_AddRefs(f)); f->GetFolderURL(n); printf ("===== RunMessageThroughMimeDraft - Folder+Key |%d|%s|\n", k, n.get()); Giving: ===== RunMessageThroughMimeDraft - Folder+Key |876422|mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/Henk's%20Christmas| or ===== RunMessageThroughMimeDraft - Folder+Key |139|imap://info%40hostalpancheta.es@mail.hostalpancheta.es/INBOX/Jordi%20Knobloch| Is that what you mean by "folder and key"? Or which method do you propose to use to get the folder? And what to I do with a "saved message"?

Jorg, I have not really been able to look deeply at your specific issue. My "folder and key" was a fairly standard answer that would apply to existing messages. Compose though is a different problem, and I cannot give you an answer without spending more time diving into your real issues. I would need to understand that different stages that the composed message goes through, and what survives that entire process. Perhaps folderURL + key is not the best for your usage. It really is only defined once the message is stored somewhere, and composed messages are not necessarily stored anywhere. I don't have more time today to look deeper at this.

For completeness, here is what Kent told me in a private message: === Well a url for messages is intended as a way to access the message, not as unique identifier. Hence you are seeing the different representations. I believe that the folder URL though is a valid unique identifier (ensured by RDF). So if I wanted to track a message, it would be folder URL and database key (64 bit integer) as the identifier. For a file message, I suppose the URL of the file would work, presumably with key = 0 (or unknown which ash a unique value?) There are many additional levels of complexity of this unfortunately. IMAP messages can have fake keys in the early processing steps, for example. So as is typical with our code, you may run into some strange edge cases. === I will pursue the folderURL + key approach a little further.

I decided to split out the main problem into bug 1239658. That will fix 95% of the problem here. I've been experimenting with tracking a message through the various steps: Opening it for viewing, changing the charset, using if for forward/"edit as new"/"edit draft"/reply. It's really hard to assign a unique identifier especially when it's "detached" from the original database message. As long as we have "mime_draft_data", we can relate it back to the database through the origMsgHdr member, but as soon as we're dealing with "mime_stream_data" that connection is lost: MDD: https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/nsStreamConverter.cpp#112 MSD: https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/nsStreamConverter.cpp#122 and also https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimemoz2.cpp#711 That's where the charset is set but we need to be able to tell for which original message. There are two more locations where we need to identify the message: https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp?from=GetTopmostMsgWindowCharacterSet# and https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgComposeService.cpp#1613 The URL being carried around changes as detailed in comment #50. There are differences between local messages and IMAP messages. Message opened from a file are yet another problem. In short, this may not make it into TB 45 since it also needs an interface change in nsIMsgWindow.idl.

OK, after a lot of fiddling I found a way to generate a unique URL. Local message: mailbox-message://nobody@Local%20Folders/Henk%27s%20Christmas#876422 IMAP: imap-message://info%40hostalpancheta.es@mail.hostalpancheta.es/INBOX/Jordi Knobloch#139 File: file:///D:/Desktop/Hostal%20Cerrado.eml So we should be able to track the message through the compose process and reset the override flag when moving on to another message.

If you want to test this: Replace: printf("===== nsMsgWindow::SetMessageURL - clearing override and charset\n"); with printf("===== nsMsgWindow::SetMessageURL - clearing override and charset\a\a\n"); And you'll hear a beep every time we move to a new message.

I looked over this for possible landing in TB 45, and I think we'll not do the interface change. Should we decide to port a future change to TB 45, we can do a branch-specific interface if needed, the porting would not be that extensive.

Comment on attachment 8707985 [details] [diff] [review] Possible solution (v5). Review of attachment 8707985 [details] [diff] [review]: ----------------------------------------------------------------- As a general comment, I am still skeptical about using message URL as a unique identifier. Is there any reason that you could not use message ID for this, even if you have to create that ID early? ::: mailnews/compose/src/nsMsgCompose.cpp @@ +155,5 @@ > nsCOMPtr<nsIMsgWindow> msgWindow; > mailSession->GetTopmostMsgWindow(getter_AddRefs(msgWindow)); > if (msgWindow) > { > + msgWindow->SetMessageURL(messageURL); Drive-by comment: You are adding a side effect to a getter, which is generally a bad idea. If you really need this change, then I would prefer if you simply eliminated this getter, which is only used in one place, and move its code into that one place.

Thanks for looking at this. Sadly I don't understand 90% of what you're saying ;-( (In reply to Kent James (:rkent) from comment #57) > I looked over this for possible landing in TB 45, and I think we'll not do > the interface change. Should we decide to port a future change to TB 45, we > can do a branch-specific interface if needed, the porting would not be that > extensive. Can you please explain. If you port an interface change (back?) to TB 45 at a later stage, that changes the interface and therefore upsets add-ons? I thought the idea is to land all interfaces and then not change them again. (In reply to Kent James (:rkent) from comment #58) > As a general comment, I am still skeptical about using message URL as a > unique identifier. Is there any reason that you could not use message ID for > this, even if you have to create that ID early? Well, the message URL is already passed around in parts of the system. As you can see in the patch, it can be used without much tweaking in most places. The only tweaking happens for saved messages (file://). Getting the URL from the "mime stream data" is a little tricky. Forwarding or replying to a message we would have to rely on its ID, which is not under our control. > ::: mailnews/compose/src/nsMsgCompose.cpp > @@ +155,5 @@ > > nsCOMPtr<nsIMsgWindow> msgWindow; > > mailSession->GetTopmostMsgWindow(getter_AddRefs(msgWindow)); > > if (msgWindow) > > { > > + msgWindow->SetMessageURL(messageURL); > Drive-by comment: You are adding a side effect to a getter, which is > generally a bad idea. If you really need this change, then I would prefer if > you simply eliminated this getter, which is only used in one place, and move > its code into that one place. Which getter? You call GetTopmostMsgWindowCharacterSet() a getter. It's the only place in the source file which knows of the message window. So I put my call in there. Surely I can get rid of the function and go like this as you suggested: // Get message window. nsCOMPtr<nsIMsgWindow> msgWindow; mailSession->GetTopmostMsgWindow(getter_AddRefs(msgWindow)); // Do some processing if we have a window. if (msgWindow) { // Let the window know the message URL of the message we're processing. // This will reset the charset and override if this was set for a previous message. msgWindow->SetMessageURL(messageURL); // Now retrieve the charset and the override. We can be sure it applies to the // message we're processing here. msgWindow->GetMailCharacterSet(charset); msgWindow->GetCharsetOverride(charsetOverride); } So how do we proceed with this? Message URL, yes or no? I thinks it's handy to have. Can your scepticism me dissolved? If so, I'm happy to only land this on TB 46 or later. I've spent so much time investigating here that I don't want to spend any more if this is not going ahead. Most of the problem is resolved by bug 1239658. To reproduce the bug here, you need to: 1) view message 1 2) actively change its charset 3) forward/reply to/edit draft/edit as new of message 2 without viewing it. Not a likely case. Without fixing bug 1239658, the second step was not necessary, the charset of message 1 was always carried over to message 2 due to the erroneous programmatically-set override. This made the problem much more likely.

"Can you please explain. If you port an interface change (back?) to TB 45 at a later stage, that changes the interface and therefore upsets add-ons? I thought the idea is to land all interfaces and then not change them again." That you do is to add a new interface, nsIMsgWindowESR45, that only has the new item added, and then you add that and QI to it when you need it when you need to deal with he new attribute. So you are adding a new interface, but not changing an old one. That is OK. "So how do we proceed with this? Message URL, yes or no? I thinks it's handy to have. Can your scepticism me dissolved?" Unfortunately we encounter this class of issue constantly, where something is convenient to use, so we conflate several usages. Here you are conflating "string used to access a message" with "unique message identifier". All of these accumulated conflations are what makes the code so hard to work with. The fact that you had to do changes to make this unique show that this was never intended. But my comments are very general, I have not made an attempt to really understand the root issue of what you are trying to do, and what the alternatives are. But you are taking what was a hack to begin with, and adding several new bad ideas to it (a side effect in a getter, conflating the usage of URL with unique identifier). TopMsgWindow is almost always a hack BTW, as I have have learned myself through bitter experience and regressions. "It's the only place in the source file which knows of the message window" is also evidence that something is not quite right here. It may not make sense to sort this out to figure out the proper way architecturally to do this, if you believe that the most common problem is solved. I have not made an attempt myself to understand the underlying issues, and I have more urgent issues as well.

OK, well, let me submit a new patch addressing the "drive by" (getter) issue tomorrow, and ask Magnus for his opinion. I still think "string used to access a message" (message URL) is a unique identifier under our control which is a valid identifier to tell two messages apart. That's all we need.

(In reply to Jorg K (GMT+1) from comment #61) > OK, well, let me submit a new patch addressing the "drive by" (getter) issue > tomorrow, and ask Magnus for his opinion. > > I still think "string used to access a message" (message URL) is a unique > identifier under our control which is a valid identifier to tell two > messages apart. That's all we need. It's not unique because it may contain other information about the message unrelated to its identity, such as the emitter type.

(In reply to Kent James (:rkent) from comment #62) > It's not unique because it may contain other information about the message > unrelated to its identity, such as the emitter type. I clearly don't know enough about this, so I don't understand your comment. "string used to access a message" must be unique enough to access the message, so how can it not be unique at least for the current session and as long as the mailbox doesn't get compacted or have it's keys changed? How would "contain[ing] other information about the message unrelated to its identity" would make it not unique?

Grammar derailed: How would "contain[ing] other information about the message unrelated to its identity" make it not unique?

OK, maybe a silly question: Surely if you were to store message size and message date, for example, then it wouldn't be unique. But we're talking about the message URL or URI, so the uniform resource *identifier*. How can an identifier not be unique? Also, the message URL I am proposing to use is basically "forder+key" which is what you suggested. Only that my approach uses one string and can handle file:// URLs at well.

This addresses Kent's drive by comment. I've also included a \a\a in a print statement when the override is reset. You should hear it every time you move to a new message. You shouldn't hear it when changing the charset for a message (setting the override) and then using the same message for forward/edit-draft/reply/edit-as-new.

Oops, cut&paste error.

(In reply to Jorg K (GMT+1) from comment #65) > OK, maybe a silly question: Surely if you were to store message size and > message date, for example, then it wouldn't be unique. > > But we're talking about the message URL or URI, so the uniform resource > *identifier*. How can an identifier not be unique? Also, the message URL I > am proposing to use is basically "forder+key" which is what you suggested. > Only that my approach uses one string and can handle file:// URLs at well. It is OK to use folder URL plus message key as a unique identifier (which could be saved as either a string URI or a folder URI plus message key) as long as this is generated as needed and you do not try to enforce this requirement upstream of your particular needs. (In reply to Jorg K (GMT+1) from comment #64) > How would "contain[ing] other information about the message unrelated to its > identity" make it not unique? URL1 = "mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/Henk's%20Christmas?number=876422" URL2 = "mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/Henk's%20Christmas?number=876422?emitter=js" Both refer to the same message, but URL1 != URL2 because the URL used to refer to a specific message is not unique. The MESSAGE is unique, but the URL is not. It can be made unique if you agree on certain restrictions (such as using the folder URL with an appended integer key in the URI reference field), but what I am opposed to is either assuming that those restrictions already exist in existing message URLs, or trying to change existing message URLS to impose your restrictions. Also a caution: URIs with references (that is a # in the string representation) can sometimes have that reference stripped if you pass it around Gecko, as sometimes Gecko makes the assumption that what you REALLY care about is the webpage, and the reference is not needed to locate a specific webpage uniquely. This would not be a problem if you just use nsIURI to generate a string, and only compare to that string, not try to use it in Necko.

(In reply to Kent James (:rkent) from comment #68) > It is OK to use folder URL plus message key as a unique identifier (which > could be saved as either a string URI or a folder URI plus message key) as > long as this is generated as needed and you do not try to enforce this > requirement upstream of your particular needs. Well, as you can see in the patch, I had to tweak five spots: 1) nsMsgCompose.cpp: The message URL/URI is already available, I just call nsIMsgWindow::SetMessageURL() on it. 2) nsMsgComposeService.cpp: The message URL/URI is already available, I just strip what's behind the "?" 3) nsStreamConverter.cpp (1): The message URL/URI is already available. 4) nsStreamConverter.cpp (2): See case 5) 5) mimemoz2.cpp: As case 4): A little tweak to get it via "channel->GetURI(getter_AddRefs(uri));" from the mime stream data. So if I understand your concern correctly: It is generated when needed, in fact in 3 of 5 cases it's there already, two cases need more processing. Nothing is changed upstream of these five spots.

Comment on attachment 8709398 [details] [diff] [review] Possible solution (v6a). Review of attachment 8709398 [details] [diff] [review]: ----------------------------------------------------------------- While it may work, this looks very fragile and error-prone. I just don't like that we expose a messageURL for internal need. I still wonder if it couldn't just be as simple as checking if the message selected is already displayed, and if not reset the override. So in ComposeMessage http://mxr.mozilla.org/comm-central/source/mail/base/content/mailCommands.js#154 something like if (gMessageDisplay.displayedMessage.messageKey != messageKey) msgWindow.charsetOverride = false;

This is so funny. Right above the line you pointed out, they actually use part of the message URL: ex : mailbox-message://john%2Edoe@pop.isp.invalid/Drafts#12345 So the very same identifier I'm suggesting to use is already used. Only that this is not correct. The message key DOES not identify the message, since two messages in two different folders can have the same key. Anyway, your suggestion doesn't fly, since debug if (messageArray.length == 1) { dump("==== Message |"+messageArray[0]+"|\n"); dump("==== Display |"+gMessageDisplay.displayedMessage.messageKey+"|\n"); } give this ==== Message |mailbox-message://nobody@Local%20Folders/Henk%27s%20Christmas#878309| ==== Display |878309| although I viewed a *different* message before clicking reply/forward. The only thing you can do is store the half-ID of the message to which the override applies here: https://dxr.mozilla.org/comm-central/source/mail/base/content/mailWindow.js#627

Take a look at this, please.

Sorry, mucked-up the commit comment. Intended: Bug 597369 - Remember which message the override applies to and reset if necessary. r=mkmelin.

(In reply to Jorg K (GMT+1) from comment #71) > Anyway, your suggestion doesn't fly, Hmm, I just tried it and it seems to work for me. This is what i did 1. View a UTF-8 msg, manually set it to latin1 2. Try reply/fwd/edit as new of another utf-8 message, without even viewing it first (thread pane context menu)

(In reply to Jorg K (GMT+1) from comment #71) > Only that this is not correct. The message key DOES not identify the > message, since two messages in two different folders can have the same key. While true, a bit unlikely you'd hit that case. Of course we could check the folder too to be 100% sure.

(In reply to Magnus Melin from comment #74) > Hmm, I just tried it and it seems to work for me. This is what i did > 1. View a UTF-8 msg, manually set it to latin1 > 2. Try reply/fwd/edit as new of another utf-8 message, without even viewing > it first (thread pane context menu) As you know, the latest bustage is just over, so I need to refresh my local build before I can give you a qualified answer here. Assuming that I wasn't blind when I wrote comment #71, I did this: I have a test folder with a couple of messages, one in windows-1252, one in UTF-8. The message pane is off. I open one message and set the charset to whatever, Russian, Arabic. I can't remember whether I closed the tab. I then forward the other message by clicking the forward button. Result: messageArray[0] and gMessageDisplay.displayedMessage.messageKey are the same, so I wouldn't reset the override. My patch on the other hand works in this case.

(In reply to Magnus Melin from comment #75) I’m not sure if the following tests anything more than you did, but as I did go to the trouble of documenting one sequence of tests (in a drafts folder) I’ll mention it again: It’s in bug #739063 comment #3.

What your describing in bug #739063 comment #3 falls into the 85% of the problem that is already fixed in bug 1239658. You can test that right now in an Earlybird build. The problem that was fixed is that the charset of a previously viewed message would be carried over onto another one, for example a draft that is edited again. The remaining 15% of the problem are that when you view a message, *manually change the text encoding* via the menu and then proceed to use a different message without viewing it first, then the charset gets carried over. None of your test cases mention the manually changing the text encoding, so (sorry) they are nor relevant to what needs to be done in this bug.

Better: they are not relevant to what remains to be done in this bug.

Ah yes, with the preview pane off. I think you could just always reset charsetOverride in folderDisplay displayMessageChanged (in addition) to get that to work.

Actually, what is wrong with my solution? When overriding the charset, store the message key for which this applies. function MailSetCharacterSet(aEvent) { if (aEvent.target.hasAttribute("charset")) { msgWindow.mailCharacterSet = aEvent.target.getAttribute("charset"); msgWindow.charsetOverride = true; + gMessageDisplay.keyForCharsetOverride = gMessageDisplay.displayedMessage.messageKey; } messenger.setDocumentCharset(msgWindow.mailCharacterSet); } (Note: Difference to the patch, changed keyForOverride to keyForCharsetOverride). Then when starting to compose: function ComposeMessage(type, format, folder, messageArray) { + if (messageArray.length == 1) { + if (typeof gMessageDisplay.keyForCharsetOverride == 'undefined' || + GetMsgKeyFromURI(messageArray[0]) != gMessageDisplay.keyForCharsetOverride) { + msgWindow.charsetOverride = false; + } + } This is crystal clear. Your proposal is more obfuscated and adding stuff to displayMessageChanged https://dxr.mozilla.org/comm-central/source/mail/base/content/folderDisplay.js#1359 doesn't make things clearer either. So please comment on my suggestion as I had requested before I pursue your suggestion further.

I don't think it's wrong per se, but if you'd to do that, you should add the property to messageDisplay.js. However, since msgWindow.charsetOverride will be different after you composed or not, it seems better to just have it reset always so that part is not in an undetermined state, should anyone actually want if for anything.

OK, two remarks: We've already discussed that your original approach doesn't fly with the message pane switched off. The approach of resetting the override in displayMessageChanged also doesn't quite fly, I think. This gets called an awful lot, in fact too much. Try this: With the message pane switched off, open a message, displayMessageChanged gets called. Then change the charset, then close the message to return to the message pane and forward the very same message. Oops, displayMessageChanged just got called again, which would kill the override. Sure, you could argue that you shouldn't close the message if you want to proceed with the override, but I don't see a reason why the override shouldn't live a little longer. Personally, I like my approach best. I will add a property as you said. Since there is no good point to reset the override, at least not it displayMessageChanged, whichever interested party needs to do their own checking, as the compose pipeline does.

This is messy. When opening a message from a file, this doesn't work: gMessageDisplay.keyForCharsetOverride = gMessageDisplay.displayedMessage.messageKey; I come to believe that resetting the override in displayMessageChanged is all we need. That way if the user opens a message, sets the override, then answers or forwards they are cool. If they close they message, the lose the override. If they change to a different message, they also lose the override. So the override is very short lived and that's OK. That should also work messages from a file. I'll try it.

Try this. I think this is all it needs. (Terrible, after so much work invested here, it comes down to one line.) To use the override, you need to view the message, change the text encoding and then immediately proceed to do the action you want. The override is very short lived.

Comment on attachment 8711363 [details] [diff] [review] New approach (v2). Review of attachment 8711363 [details] [diff] [review]: ----------------------------------------------------------------- You still need the ComposeMessage change to handle comment 74 too.

(In reply to Magnus Melin from comment #86) > You still need the ComposeMessage change to handle comment 74 too. You're right: The one liner doesn't work in a thread context.

This is becoming more complicated than the "Asian Crisis" ;-( Anyway, this works now and I've also tested it when opening and replying to/forwarding a saved message. I left the dump() statements in since I'm sure you'll want to test it.

Comment on attachment 8712051 [details] [diff] [review] New approach (v3). Review of attachment 8712051 [details] [diff] [review]: ----------------------------------------------------------------- r=mkmelin with some nits (and dumps removed of course) ::: mail/base/content/mailWindow.js @@ +626,5 @@ > msgWindow.mailCharacterSet = aEvent.target.getAttribute("charset"); > msgWindow.charsetOverride = true; > + gMessageDisplay.keyForCharsetOverride = > + (typeof gMessageDisplay.displayedMessage.messageKey == "undefined" ? > + null : gMessageDisplay.displayedMessage.messageKey); gMessageDisplay.displayedMessage.messageKey should never be undefined, afaikt? didn't test but even for an eml file id expect it to be null ::: mail/base/content/messageDisplay.js @@ +94,5 @@ > */ > displayedMessage: null, > + /** > + * The key of the message for which a charset override was last set. > + * Null if it was never set. Note that a stale value can hang around. nit: lowercase null

Thanks for the review. (In reply to Magnus Melin from comment #89) > > msgWindow.mailCharacterSet = aEvent.target.getAttribute("charset"); > > msgWindow.charsetOverride = true; > > + gMessageDisplay.keyForCharsetOverride = > > + (typeof gMessageDisplay.displayedMessage.messageKey == "undefined" ? > > + null : gMessageDisplay.displayedMessage.messageKey); + dump("==== In override |"+gMessageDisplay.displayedMessage.messageKey+"|\n"); + dump("==== In override |"+gMessageDisplay.keyForCharsetOverride+"|\n"); > gMessageDisplay.displayedMessage.messageKey should never be undefined, > afaikt? didn't test but even for an eml file id expect it to be null Debug says when opening a saved message and changing charset: ==== In override |undefined| ==== In override |null| > > + * The key of the message for which a charset override was last set. > > + * Null if it was never set. Note that a stale value can hang around. > nit: lowercase null Fixed. It was upper case since there was a full stop before.

Carrying forward Magnus' r+.

Comment on attachment 8714026 [details] [diff] [review] New approach (v3a), dump() removed, nits fixed. [Approval Request Comment] Regression caused by (bug #): No regression, was wrong from day one. User impact if declined: Nasty little bug where charset is carried over to other message. Testing completed (on c-c, etc.): Manual. Risk to taking this patch (and alternatives if risky): I don't consider it risky. A few lines of JS changes to reset the charset override at appropriate times.

Re. comment #90: Opening a saved message and dumping out the the attributes of gMessageDisplay.displayedMessage gives: ===== [object Object] key=|messageSize|, value=|1020| key=|author|, value=|=?UTF-8?Q?J=c3=b6rg_Knobloch?= <jorgk@jorgk.com>| key=|messageId|, value=|<7af61586-e385-da96-374d-0877cfce2120@jorgk.com>| key=|date|, value=|1453227762000000| and more, but no message key. So this is indeed undefined.

More elegant like this.

Comment on attachment 8714036 [details] [diff] [review] New approach (v3b), dump() removed, nits fixed. Sorry Magnus, as per comment #93, there is no "messageKey" property for opened saved messages. Could you please review this again: + gMessageDisplay.keyForCharsetOverride = + ("messageKey" in gMessageDisplay.displayedMessage ? + gMessageDisplay.displayedMessage.messageKey : null);

Comment on attachment 8714036 [details] [diff] [review] New approach (v3b), dump() removed, nits fixed. Review of attachment 8714036 [details] [diff] [review]: ----------------------------------------------------------------- Yes, better! r=mkmelin

Comment on attachment 8714036 [details] [diff] [review] New approach (v3b), dump() removed, nits fixed. [Approval Request Comment] Regression caused by (bug #): No regression, was wrong from day one. User impact if declined: Nasty little bug where charset is carried over to other message. Testing completed (on c-c, etc.): Manual. Risk to taking this patch (and alternatives if risky): I don't consider it risky. A few lines of JS changes to reset the charset override at appropriate times.

https://hg.mozilla.org/comm-central/rev/6cea02e28370cfd331f3fd36e6750d71bcd55440 Bug 597369 - Reset override when different message is displayed. r=mkmelin.

Had to back this out for multiple mozmill failures. https://hg.mozilla.org/comm-central/rev/54355e275f92

Sorry about the test failures. This bug is haunting me. In this patch I've changed + if (messageArray.length == 1) { to + if (messageArray && messageArray.length == 1) { since apparently the message array can be null, most likely for *new* messages ;-) Before I get into another embarrassing situation, here a try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9e4a825f9169 Can you please r+ again.

Comment on attachment 8716778 [details] [diff] [review] New approach (v3c) Review of attachment 8716778 [details] [diff] [review]: ----------------------------------------------------------------- LGTM! r=mkmelin

We're good now, try came out green.

Comment on attachment 8716778 [details] [diff] [review] New approach (v3c) [Approval Request Comment] Regression caused by (bug #): No regression, was wrong from day one. User impact if declined: Nasty little bug where charset is carried over to other message. Testing completed (on c-c, etc.): Manual. Risk to taking this patch (and alternatives if risky): I don't consider it risky. A few lines of JS changes to reset the charset override at appropriate times.

Aurora (TB 46): https://hg.mozilla.org/releases/comm-aurora/rev/ed5ff7b03a43

Comment on attachment 8716778 [details] [diff] [review] New approach (v3c) http://hg.mozilla.org/releases/comm-beta/rev/0e6bd151c024