Closed Bug 621264 Opened 11 years ago Closed 10 years ago

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

Categories

(MailNews Core :: MIME, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 14.0

People

(Reporter: bo_00, Assigned: aceman)

References

()

Details

(Whiteboard: [testday-20110603])

Attachments

(2 files, 4 obsolete files)

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
Attached image Screen capturing
Screen capturing
Does it changes if you restart Thunderbird ?
(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
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
Wayne, WADA, does this work for any of you?
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
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
Attached patch patch proposal (obsolete) — Splinter Review
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)
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 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.
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 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 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)
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+
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.
Neil, so why is squib seeing leaks?
Actually, I don't think it does leak, but it is weird.
Attached patch patch v2 (obsolete) — Splinter Review
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.
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).
Attached patch patch v3 (obsolete) — Splinter Review
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 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+
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-
Attached patch patch v4 (obsolete) — Splinter Review
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+
Attached patch patch v5Splinter Review
I wonder how many iterations we can get on such a short patch :)
Attachment #610971 - Attachment is obsolete: true
Attachment #611044 - Flags: review?(neil)
Attachment #611044 - Flags: review?(neil) → review+
Keywords: checkin-needed
http://hg.mozilla.org/comm-central/rev/573b4c79043b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
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.
There was a similar bug 224811.
Duplicate of this bug: 224811
That seems reasonable. Could you file a new bug with your request, blocking this one?
Depends on: 776438
Done.
You need to log in before you can comment on or make changes to this bug.