Note: There are a few cases of duplicates in user autocompletion which are being worked on.

the preference "mailnews.reply_header_originalmessage" in about:config is ignored for forwarded messages

RESOLVED FIXED in Thunderbird 14.0

Status

MailNews Core
MIME
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: bo liang, Assigned: aceman)

Tracking

Trunk
Thunderbird 14.0
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [testday-20110603], URL)

Attachments

(2 attachments, 4 obsolete attachments)

52.33 KB, image/png
Details
4.55 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US) AppleWebKit/534.10 (KHTML, like Gecko) Chrome/8.0.552.224 Safari/534.10
Build Identifier: 3.1.7

The problem be happened on windowsXP and Ubuntu.
About:config "mailnews.reply_header_originalmessage" is invalid!

please see the screen capturing:
http://blog.chinaunix.net/photo/98822_101224141637.png



Reproducible: Always

Steps to Reproduce:
1. change the value of "mailnews.reply_header_originalmessage" from "原始消息" to "Orginal Message"
2. Forward one mail

Actual Results:  
mail header text :"原始消息"

Expected Results:  
mail header text :"Orginal Message"

Thunderbird Setup 3.1.7.exe
Language:Chinese simplified
(Reporter)

Comment 1

7 years ago
Created attachment 499630 [details]
Screen capturing

Screen capturing
Does it changes if you restart Thunderbird ?
(Reporter)

Comment 3

7 years ago
(In reply to comment #2)
> Does it changes if you restart Thunderbird ?

I already restarted Thunderbird but "mailnews.reply_header_originalmessage" still invalided.
Thunderbird(linux) and Thunderbird(windows) is same.(invalided)
Component: Message Compose Window → Preferences
QA Contact: message-compose → preferences
(Assignee)

Comment 4

6 years ago
I can confirm this on 3.1.10 english version. The value of the preferrence is not taken into account. The delimiter is always the fixed string "---Original message---".
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hardware: x86 → All
Summary: About:config "mailnews.reply_header_originalmessage" is invalid! → the preferrence "mailnews.reply_header_originalmessage" in about:config is ignored
Whiteboard: [testday-20110603]
Version: unspecified → 3.1
(Assignee)

Comment 5

6 years ago
Wayne, WADA, does this work for any of you?
(Assignee)

Comment 6

6 years ago
This was added in Bug 70842.

This looks like the function what generates the message and should use the pref: http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#2234

I'll try to look why it does not work.
Assignee: nobody → acelists
Depends on: 70842
Version: 3.1 → Trunk
(Assignee)

Comment 7

6 years ago
The change in bug 70842 seems to be only from message reply.
The forward case is taken from a different spot in MIME mimedrft.cpp.
I am not sure we want to hook it on the same preference.
But I can try to prepare the infrastructure.
Component: Preferences → MIME
Product: Thunderbird → MailNews Core
QA Contact: preferences → mime
(Assignee)

Comment 8

6 years ago
Created attachment 596486 [details] [diff] [review]
patch proposal

So I currently hooked onto the existing pref of mailnews.reply_header_originalmessage . I get its value in the same way as http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp . I am not yet familiar with these ns*String games, but these conversions worked for me.
Attachment #596486 - Flags: feedback?(squibblyflabbetydoo)
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
Summary: the preferrence "mailnews.reply_header_originalmessage" in about:config is ignored → the preference "mailnews.reply_header_originalmessage" in about:config is ignored for forwarded messages

Comment 9

6 years ago
Comment on attachment 596486 [details] [diff] [review]
patch proposal

I'm probably not the best person to look at this, since I don't know much about composition. In any case, this seems ok, assuming you update the patch to use the proper string types (it leaks memory now).
Attachment #596486 - Flags: feedback?(squibblyflabbetydoo)
aceman for composition bienvenu or Ian.
(Assignee)

Comment 11

6 years ago
Comment on attachment 596486 [details] [diff] [review]
patch proposal

U can't use proper string types, as I have written:(
Attachment #596486 - Flags: feedback?(iann_bugzilla)

Comment 12

6 years ago
Comment on attachment 596486 [details] [diff] [review]
patch proposal

I'm not strong enough on best way to do strings in C++, try David
Attachment #596486 - Flags: feedback?(iann_bugzilla) → feedback?(dbienvenu)

Comment 13

6 years ago
Comment on attachment 596486 [details] [diff] [review]
patch proposal

Neil's the expert on this string stuff...

but the static buffer for a return value is ugly - better to use an nsCString temp variable, and have MimeGetReplyHeaderOriginalMessage fill that in, IMO. And then do NS_MsgSACat(&newBody, tempString.get())
Attachment #596486 - Flags: feedback?(dbienvenu) → feedback?(neil)

Comment 14

6 years ago
Here's a guide on string usage in Mozilla: https://developer.mozilla.org/En/Mozilla_internal_string_guide

As Bienvenu implies, I think you'll want to make your new function look like:

  nsresult MimeGetReplyHeaderOriginalMessage(nsAString &str) { ... }

Hopefully that will give you a nudge in the right direction. :)
Comment on attachment 596486 [details] [diff] [review]
patch proposal

Well, this is par for the course for MIME; I can't spot any leaks, so f+...

>+  NS_GetLocalizedUnicharPreferenceWithDefault(nsnull, "mailnews.reply_header_originalmessage",  tmpString2, tmpString3);
Although I would have inlined the conversion something like this:
NS_GetLocalizedUnicharPreferenceWithDefault(nsnull,
    "mailnews.reply_header_originalmessage",
     NS_ConvertUTF8toUTF16(tmpString1), tmpString3);
Attachment #596486 - Flags: feedback?(neil) → feedback+
(Assignee)

Comment 16

6 years ago
Thanks, I'll try to find out what you are talking about :)
The static return string is just copied from the previously used function (above mine) so as not to break expectations of the callers.
(Assignee)

Comment 17

6 years ago
Neil, so why is squib seeing leaks?

Comment 18

6 years ago
Actually, I don't think it does leak, but it is weird.
(Assignee)

Comment 19

6 years ago
Created attachment 600786 [details] [diff] [review]
patch v2

Thanks for the hints guys.
What about this?
Attachment #596486 - Attachment is obsolete: true
Attachment #600786 - Flags: review?(neil)
Attachment #600786 - Flags: feedback?(squibblyflabbetydoo)
Comment on attachment 600786 [details] [diff] [review]
patch v2

>+    NS_ConvertUTF8toUTF16(MimeGetStringByID(MIME_FORWARDED_MESSAGE_HTML_USER_WROTE)));
Where did the Adopt go? I believe it was needed to avoid a leak.

>+  retString.Assign(NS_ConvertUTF16toUTF8(tmpRetString));
Nit: Use CopyUTF16toUTF8 instead.
(Assignee)

Comment 21

6 years ago
It changed to Assign...

Adopt is not even mentioned on
https://developer.mozilla.org/En/Mozilla_internal_string_guide
so I don't know when it must be used (I used it for literals only).
(Assignee)

Comment 22

6 years ago
Created attachment 601085 [details] [diff] [review]
patch v3

Changing Assign to Adopt didn't work.
So let's try CopyUTF8 too there.
Attachment #600786 - Attachment is obsolete: true
Attachment #601085 - Flags: review?(neil)
Attachment #601085 - Flags: feedback?(squibblyflabbetydoo)
Attachment #600786 - Flags: review?(neil)
Attachment #600786 - Flags: feedback?(squibblyflabbetydoo)
(In reply to aceman from comment #22)
> Changing Assign to Adopt didn't work.
Well, you can't assign or adopt directly into your destination because you have a UTF8/UTF16 mismatch, so you need to adopt into a temporary string.

I'm not sure where it's documented but so you know, Adopt is used when you have a string created through NS_Alloc/NS_Strdup/ToXXXString and you want your ns(C)String to automatically NS_Free it for you so that you don't leak it.

Comment 24

5 years ago
Comment on attachment 601085 [details] [diff] [review]
patch v3

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

I think this is ok, at least string-wise. I'm assuming this pref is used in some wholly-different part of the code for replies?
Attachment #601085 - Flags: feedback?(squibblyflabbetydoo) → feedback+
(Assignee)

Comment 25

5 years ago
Yes, see comment 7.
Comment on attachment 601085 [details] [diff] [review]
patch v3

>+  CopyUTF8toUTF16(MimeGetStringByID(MIME_FORWARDED_MESSAGE_HTML_USER_WROTE), defaultValue);
Still waiting for you to adopt the return value of MimeGetStringByID...
Attachment #601085 - Flags: review?(neil) → review-
(Assignee)

Comment 27

5 years ago
Created attachment 610971 [details] [diff] [review]
patch v4
Attachment #601085 - Attachment is obsolete: true
Attachment #610971 - Flags: review?(neil)
Comment on attachment 610971 [details] [diff] [review]
patch v4

>+nsresult
You always return NS_OK, so you might as well make this void anyway.

>+  nsString defaultValue;
>+  CopyUTF8toUTF16(tmpString, defaultValue);
Actually this is better written as NS_ConvertUTF8toUTF16 defaultValue(tmpString); but see also below.

>+  if (NS_FAILED(rv))
>+    tmpRetString.Assign(defaultValue);
NS_GetLocalizedUnicharPreferenceWithDefault always returns NS_OK, so you don't need this (or indeed rv). (This also means that you only use defaultValue once, so you can inline it with NS_ConvertUTF8toUTF16(tmpString).)

>     NS_MsgSACopy(&(newBody), "<HTML><BODY><BR><BR>");
> 
>+
Nit: unnecessary duplicate blank line.
Attachment #610971 - Flags: review?(neil) → review+
(Assignee)

Comment 29

5 years ago
Created attachment 611044 [details] [diff] [review]
patch v5

I wonder how many iterations we can get on such a short patch :)
Attachment #610971 - Attachment is obsolete: true
Attachment #611044 - Flags: review?(neil)

Updated

5 years ago
Attachment #611044 - Flags: review?(neil) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
http://hg.mozilla.org/comm-central/rev/573b4c79043b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0

Comment 31

5 years ago
Aaa. Why is reply_header_originalmessage used also for forwarded messages? It should be used only for reply messages, as the name tells. With this patch I am now getting also forwarded messages formatted in the same way as replies. And this is not correct in my configuration.

I believe there should be a separate configuration for forwarded messages and that it was correct behavior that reply_header_originalmessage was not used for forwarded messages.

Comment 32

5 years ago
There was a similar bug 224811.
(Assignee)

Updated

5 years ago
Duplicate of this bug: 224811
(Assignee)

Comment 34

5 years ago
That seems reasonable. Could you file a new bug with your request, blocking this one?

Updated

5 years ago
Depends on: 776438

Comment 35

5 years ago
Done.
You need to log in before you can comment on or make changes to this bug.