"Open Draft"/"Forward"/"Edit As New"/"Reply" creates message composition with incorrect character encoding if messages wasn't viewed before. Uses charset from previously viewed message.

RESOLVED FIXED in Thunderbird 47.0

Status

Thunderbird
Folder and Message Lists
RESOLVED FIXED
7 years ago
5 months ago

People

(Reporter: Iwasa Kazmi, Assigned: Jorg K (GMT+2))

Tracking

({dataloss})

unspecified
Thunderbird 47.0
x86
Windows XP
dataloss
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird45 fixed, thunderbird46 fixed, thunderbird47 fixed)

Details

Attachments

(1 attachment, 12 obsolete attachments)

4.03 KB, patch
Magnus Melin
: review+
Jorg K (GMT+2)
: approval-comm-aurora+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.

Updated

7 years ago
Duplicate of this bug: 610329
Summary: "Edit As New" opens a message with incorrect character encoding → "Edit As New" opens a message with incorrect character encoding ("Forward"/Edit As New" of context menu by right click of a maiil without explicit mail selection at Thread Pane. Charset of currently selected mail is used by mail composition window)
Blocks: 597821

Comment 7

6 years ago
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 :-(

Comment 8

6 years ago
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).
Duplicate of this bug: 692264
Blocks: 701694

Comment 10

5 years ago
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!
Summary: "Edit As New" opens a message with incorrect character encoding ("Forward"/Edit As New" of context menu by right click of a maiil without explicit mail selection at Thread Pane. Charset of currently selected mail is used by mail composition window) → "Forward"/"Edit As New" from context menu creates message composition with incorrect character encoding (if right-click without explicit prior message selection from Thread Pane, charset of currently selected mail is used for draft - bogus focus!)
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).
Duplicate of this bug: 779517

Comment 14

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

Comment 15

4 years ago
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!

Comment 16

4 years ago
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.

Comment 17

4 years ago
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.

Comment 18

4 years ago
follow-up kindly requested

Comment 19

3 years ago
"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???

Comment 20

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

Comment 21

2 years ago
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.

Comment 22

2 years ago
Bug still present in 38.4.0
(Assignee)

Updated

a year ago
Duplicate of this bug: 644780
(Assignee)

Comment 24

a year ago
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.
(Assignee)

Updated

a year ago
No longer blocks: 701694
(Assignee)

Updated

a year ago
Duplicate of this bug: 701694
(Assignee)

Comment 26

a year ago
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.
(Assignee)

Updated

a year ago
Flags: needinfo?(neil)
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Updated

a year ago
Summary: "Forward"/"Edit As New" from context menu creates message composition with incorrect character encoding (if right-click without explicit prior message selection from Thread Pane, charset of currently selected mail is used for draft - bogus focus!) → "Open Draft"/"Forward"/"Edit As New" creates message composition with incorrect character encoding if messages wasn't viewed before. Uses charset from previously viewed message.
(Assignee)

Comment 27

a year ago
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?

Comment 28

a year ago
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.
(Assignee)

Comment 29

a year ago
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.

Comment 30

a year ago
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
(Assignee)

Comment 31

a year ago
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.
(Assignee)

Comment 32

a year ago
Created attachment 8705061 [details] [diff] [review]
Possible solution (v1).

OK, I'm storing the message URL with the charset to be able to compare it on retrieval. What do you think, gentlemen?
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Flags: needinfo?(neil)
Flags: needinfo?(mkmelin+mozilla)
Attachment #8705061 - Flags: feedback?(neil)
Attachment #8705061 - Flags: feedback?(mkmelin+mozilla)

Comment 33

a year ago
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.
Attachment #8705061 - Flags: feedback?(mkmelin+mozilla) → feedback-
(Assignee)

Comment 34

a year ago
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.
Keywords: dataloss

Comment 35

a year ago
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.
(Assignee)

Comment 36

a year ago
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.

Comment 37

a year ago
(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?
(Assignee)

Comment 38

a year ago
(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-.

Comment 39

a year ago
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.
(Assignee)

Comment 40

a year ago
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.
Summary: "Open Draft"/"Forward"/"Edit As New" creates message composition with incorrect character encoding if messages wasn't viewed before. Uses charset from previously viewed message. → "Open Draft"/"Forward"/"Edit As New"/"Reply" creates message composition with incorrect character encoding if messages wasn't viewed before. Uses charset from previously viewed message.
(Assignee)

Comment 41

a year ago
Created attachment 8706346 [details] [diff] [review]
Better possible solution (v2)

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
Attachment #8705061 - Attachment is obsolete: true
Attachment #8705061 - Flags: feedback?(neil)
(Assignee)

Comment 42

a year ago
(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.
(Assignee)

Comment 43

a year ago
Created attachment 8706400 [details] [diff] [review]
WIP: Better possible solution (v3).

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.
Attachment #8706346 - Attachment is obsolete: true
Attachment #8706400 - Flags: feedback?(mkmelin+mozilla)

Comment 44

a year ago
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
(Assignee)

Comment 45

a year ago
(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.
(Assignee)

Comment 46

a year ago
Created attachment 8706599 [details] [diff] [review]
WIP: Better possible solution (v4).

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://"
Attachment #8706400 - Attachment is obsolete: true
Attachment #8706400 - Flags: feedback?(mkmelin+mozilla)
Attachment #8706599 - Flags: feedback?(mkmelin+mozilla)
(Assignee)

Comment 47

a year ago
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://".

Comment 48

a year ago
(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?
(Assignee)

Comment 50

a year ago
(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.
Flags: needinfo?(rkent)
(Assignee)

Comment 51

a year ago
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"?
(Assignee)

Updated

a year ago
Attachment #8706599 - Flags: feedback?(mkmelin+mozilla)
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.
Flags: needinfo?(rkent)
(Assignee)

Comment 53

a year ago
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.
(Assignee)

Updated

a year ago
Depends on: 1239658
(Assignee)

Comment 54

a year ago
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.
(Assignee)

Comment 55

a year ago
Created attachment 8707985 [details] [diff] [review]
Possible solution (v5).

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.
Attachment #8706599 - Attachment is obsolete: true
Attachment #8707985 - Flags: feedback?(mkmelin+mozilla)
(Assignee)

Comment 56

a year ago
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.
(Assignee)

Comment 59

a year ago
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.
Flags: needinfo?(rkent)
"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.
Flags: needinfo?(rkent)
(Assignee)

Comment 61

a year ago
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.
(Assignee)

Comment 63

a year ago
(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?
Flags: needinfo?(rkent)
(Assignee)

Comment 64

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

Comment 65

a year ago
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.
(Assignee)

Comment 66

a year ago
Created attachment 8709391 [details] [diff] [review]
Possible solution (v6).

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.
Attachment #8707985 - Attachment is obsolete: true
Attachment #8707985 - Flags: feedback?(mkmelin+mozilla)
Attachment #8709391 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 67

a year ago
Created attachment 8709398 [details] [diff] [review]
Possible solution (v6a).

Oops, cut&paste error.
Attachment #8709391 - Attachment is obsolete: true
Attachment #8709391 - Flags: review?(mkmelin+mozilla)
Attachment #8709398 - Flags: review?(mkmelin+mozilla)
(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.
Flags: needinfo?(rkent)
(Assignee)

Comment 69

a year ago
(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 70

a year ago
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;
Attachment #8709398 - Flags: review?(mkmelin+mozilla) → review-
(Assignee)

Comment 71

a year ago
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
(Assignee)

Comment 72

a year ago
Created attachment 8710144 [details] [diff] [review]
New approach (v1).

Take a look at this, please.
Attachment #8709398 - Attachment is obsolete: true
Attachment #8710144 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 73

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

Comment 74

a year ago
(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)

Comment 75

a year ago
(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.
(Assignee)

Comment 76

a year ago
(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.

Comment 77

a year ago
(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.
(Assignee)

Comment 78

a year ago
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.
(Assignee)

Comment 79

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

Comment 80

a year ago
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.
(Assignee)

Comment 81

a year ago
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.

Comment 82

a year ago
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.
(Assignee)

Comment 83

a year ago
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.
(Assignee)

Comment 84

a year ago
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.
(Assignee)

Comment 85

a year ago
Created attachment 8711363 [details] [diff] [review]
New approach (v2).

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.
Attachment #8710144 - Attachment is obsolete: true
Attachment #8710144 - Flags: review?(mkmelin+mozilla)
Attachment #8711363 - Flags: review?(mkmelin+mozilla)

Comment 86

a year ago
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.
Attachment #8711363 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 87

a year ago
(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.
(Assignee)

Comment 88

a year ago
Created attachment 8712051 [details] [diff] [review]
New approach (v3).

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.
Attachment #8711363 - Attachment is obsolete: true
Attachment #8712051 - Flags: review?(mkmelin+mozilla)

Comment 89

a year ago
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
Attachment #8712051 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 90

a year ago
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.
(Assignee)

Comment 91

a year ago
Created attachment 8714026 [details] [diff] [review]
New approach (v3a), dump() removed, nits fixed.

Carrying forward Magnus' r+.
Attachment #8712051 - Attachment is obsolete: true
Attachment #8714026 - Flags: review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed
(Assignee)

Comment 92

a year ago
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.
Attachment #8714026 - Flags: approval-comm-beta?
Attachment #8714026 - Flags: approval-comm-aurora+
(Assignee)

Comment 93

a year ago
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.
(Assignee)

Comment 94

a year ago
Created attachment 8714036 [details] [diff] [review]
New approach (v3b), dump() removed, nits fixed.

More elegant like this.
Attachment #8714026 - Attachment is obsolete: true
Attachment #8714026 - Flags: approval-comm-beta?
Attachment #8714036 - Flags: review+
(Assignee)

Comment 95

a year ago
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);
Attachment #8714036 - Flags: review+ → review?(mkmelin+mozilla)
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 96

a year ago
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
Attachment #8714036 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed
(Assignee)

Comment 97

a year ago
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.
Attachment #8714036 - Flags: approval-comm-beta?
Attachment #8714036 - Flags: approval-comm-aurora+

Comment 98

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

Updated

a year ago
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 47.0

Comment 99

a year ago
Had to back this out for multiple mozmill failures. https://hg.mozilla.org/comm-central/rev/54355e275f92
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

a year ago
Attachment #8714036 - Flags: approval-comm-aurora+
(Assignee)

Comment 100

a year ago
Created attachment 8716778 [details] [diff] [review]
New approach (v3c)

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.
Attachment #8714036 - Attachment is obsolete: true
Attachment #8714036 - Flags: approval-comm-beta?
Attachment #8716778 - Flags: review?(mkmelin+mozilla)

Comment 101

a year ago
Comment on attachment 8716778 [details] [diff] [review]
New approach (v3c)

Review of attachment 8716778 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM! r=mkmelin
Attachment #8716778 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 102

a year ago
We're good now, try came out green.
Keywords: checkin-needed

Comment 103

a year ago
https://hg.mozilla.org/comm-central/rev/6051de19ee59
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Assignee)

Comment 104

a year ago
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.
Attachment #8716778 - Flags: approval-comm-beta?
Attachment #8716778 - Flags: approval-comm-aurora+
(Assignee)

Comment 105

a year ago
Aurora (TB 46):
https://hg.mozilla.org/releases/comm-aurora/rev/ed5ff7b03a43
status-thunderbird45: --- → affected
status-thunderbird46: --- → fixed
status-thunderbird47: --- → fixed
Comment on attachment 8716778 [details] [diff] [review]
New approach (v3c)

http://hg.mozilla.org/releases/comm-beta/rev/0e6bd151c024
Attachment #8716778 - Flags: approval-comm-beta? → approval-comm-beta+

Updated

a year ago
status-thunderbird45: affected → fixed
(Assignee)

Updated

a year ago
Duplicate of this bug: 555693
Depends on: 1323377
Depends on: 1324264
No longer depends on: 1324264
(Assignee)

Updated

5 months ago
Depends on: 1325967
You need to log in before you can comment on or make changes to this bug.