Closed
Bug 995797
Opened 11 years ago
Closed 11 years ago
attribution line customization: basic customization of email reply header text using placeholders/variables
Categories
(MailNews Core :: Composition, enhancement)
MailNews Core
Composition
Tracking
(Not tracked)
VERIFIED
FIXED
Thunderbird 31.0
People
(Reporter: eu, Assigned: aceman)
References
(Depends on 1 open bug, )
Details
(Keywords: intl)
Attachments
(1 file, 4 obsolete files)
28.44 KB,
patch
|
mkmelin
:
review+
jsbruner
:
ui-review+
|
Details | Diff | Splinter Review |
Hi.
An user of Thunderbird in Galician wrote a mail to the Ubuntu's list asking us by a improvement in a translation. I have searched this string in Transvision and I think that are the below:
## reply header in composeMsg
## <author> wrote:
mailnews.reply_header_authorwrote=%s wrote
mailnews.reply_header_ondate=On %s
## reply header in composeMsg
## user specified
mailnews.reply_header_originalmessage=-------- Original Message --------
Concretly this are in the file:: mail ⊃ chrome ⊃ messenger ⊃ messengercompose ⊃ composeMsgs.properties
The search in Transvision: http://transvision.mozfr.org/?repo=release&sourcelocale=en-US&locale=gl&search_type=entities&recherche=mailnews.reply_header_ondate
I have checked as the string is translated in others languages and I think that I can not do anything. I explain me:
mailnews.reply_header_ondate=On %s
In Galician is translated as "En %s"
An example of what the users see now:
"En 16/01/14 22:19"
He propose us the next:
"O 16/01/14 ás 22:19"
I could change the "En" by "O", like this person says and it must be so. But I can not or I do not know as write something between the date and the time.
The "%s" variable writes the date and the time followed. Is there some way of changing that?
After I talked with Pascal, he thinks the same than me. There will be to change the source code to create two variables. Would it be this possible?
Best regards.
Yes this can be improved. The change would be useful to more languages than those mentioned.
The whole constructing of the reply header by concatenating "mailnews.reply_header_ondate + mailnews.reply_header_separator + mailnews.reply_header_authorwrote + mailnews.reply_header_colon" is hacky and not really localizable. The strings should contain whole sentences or labels to be properly localizable.
Assignee: nobody → acelists
Keywords: intl
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
So this could do it and allow localizers much better flexibility.
Attachment #8408645 -
Flags: ui-review?(josiah)
Attachment #8408645 -
Flags: review?(neil)
Attachment #8408645 -
Flags: review?(mkmelin+mozilla)
Attachment #8408645 -
Flags: review?(iann_bugzilla)
Comment 3•11 years ago
|
||
Comment on attachment 8408645 [details] [diff] [review]
patch
>+ composeBundle->GetStringFromName(MOZ_UTF16("mailnews.reply_header_authorwrotesingle"), getter_Copies(defaultValue));
>+ NS_GetLocalizedUnicharPreferenceWithDefault(prefBranch, "mailnews.reply_header_authorwrotesingle",
>+ defaultValue, reply_header_authorwrote);
Localised preferences already get the default value from the bundle, no need to duplicate effort.
>+## LOCALIZATION NOTE (mailnews.reply_header_ondateauthorwrote): 1st %s is date, 2nd %s is time, 3rd %s is author
>+mailnews.reply_header_ondateauthorwrote=On %s %s, %s wrote:
>+## LOCALIZATION NOTE (mailnews.reply_header_authorwroteondate): 1st %s is author, 2nd %s is date, 3rd %s is time
>+mailnews.reply_header_authorwroteondate=%s wrote on %s %s:
Hmm, can we do something like %1$s here so that the order can be changed?
So this looks more robust and changing of %s to %X$S (removing smprintf) allows to fix at least bug 334053.
Attachment #8408645 -
Attachment is obsolete: true
Attachment #8408645 -
Flags: ui-review?(josiah)
Attachment #8408645 -
Flags: review?(neil)
Attachment #8408645 -
Flags: review?(mkmelin+mozilla)
Attachment #8408645 -
Flags: review?(iann_bugzilla)
Attachment #8409429 -
Flags: review?(neil)
Attachment #8409429 -
Flags: review?(mkmelin+mozilla)
Attachment #8409429 -
Flags: review?(josiah)
Attachment #8409429 -
Flags: review?(iann_bugzilla)
Comment on attachment 8409429 [details] [diff] [review]
patch v2
Review of attachment 8409429 [details] [diff] [review]:
-----------------------------------------------------------------
::: mailnews/compose/src/nsMsgCompose.cpp
@@ +2219,5 @@
> switch (replyHeaderType)
> {
> + case 0: // No reply header at all (actually the "---- original message ----" string,
> + // which is kinda misleading. TODO: Should there be a "really no header" option?
> + mCitePrefix.AssignLiteral(replyHeaderOriginalmessage);
Make this .Assign() ...
Comment 6•11 years ago
|
||
Actually it looks as if nsTextFormatter::smprintf does support %2$S-type entries.
Ok, but there would still be the problem of bug 334053. And checking for the placeholders ourselves enables to skip getting the data that is not needed (autor, date, time).
Comment 8•11 years ago
|
||
(In reply to aceman from comment #7)
> Ok, but there would still be the problem of bug 334053. And checking for the
> placeholders ourselves enables to skip getting the data that is not needed
> (author, date, time).
Fair enough, although I think using #1 placeholders might be less confusing than %1$s.
Comment 9•11 years ago
|
||
Comment on attachment 8409429 [details] [diff] [review]
patch v2
>+ // If fetching any of the preferences fails,
>+ // we return early with header_type = 0 meaning "no header".
>+ rv = NS_GetUnicharPreferenceWithDefault(prefBranch, "mailnews.reply_header_locale", EmptyString(), reply_header_locale);
>+ NS_ENSURE_SUCCESS(rv, rv);
NS_GetUnicharPreferenceWithDefault never fails (I don't know why it bothers to return NS_OK).
>+ rv = prefBranch->GetIntPref("mailnews.reply_header_type", reply_header_type);
>+ return rv;
Or just return prefBranch->GetIntPref
>+ int32_t replyHeaderType;
>+ nsAutoString replyHeaderLocale;
>+ nsString replyHeaderAuthorWrote;
>+ nsString replyHeaderOnDateAuthorWrote;
>+ nsString replyHeaderAuthorWroteOnDate;
I wonder whether we should call GetReplyHeaderInfo here.
>+ case 4: // TODO: implement user specified header
(In theory we already get this via changing the pref. Or a future bug could make cases 0-3 read the string bundle directly, and only case 4 would read a pref.)
>+ citingHeader = true;
>+ headerDate = false;
I would rename citingHeader to headerAuthor for consistency.
>+ nsAutoCString citeAuthor;
>+ ExtractName(EncodedHeader(author), citeAuthor);
Use the nsString version of ExtractName.
>+ if (NS_FAILED(rv))
>+ replyHeaderOriginalmessage.AssignLiteral("---");
Won't this be confusing? (Signature is "-- ".)
Assignee | ||
Comment 10•11 years ago
|
||
It seems .properties files use both formats so yes, as these strings are to be edited by users, let's make them easier.
Attachment #8409429 -
Attachment is obsolete: true
Attachment #8409429 -
Flags: review?(neil)
Attachment #8409429 -
Flags: review?(mkmelin+mozilla)
Attachment #8409429 -
Flags: review?(josiah)
Attachment #8409429 -
Flags: review?(iann_bugzilla)
Attachment #8409834 -
Flags: ui-review?(josiah)
Attachment #8409834 -
Flags: review?(neil)
Attachment #8409834 -
Flags: review?(mkmelin+mozilla)
Attachment #8409834 -
Flags: review?(iann_bugzilla)
Assignee | ||
Comment 11•11 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #9)
> NS_GetUnicharPreferenceWithDefault never fails (I don't know why it bothers
> to return NS_OK).
Well, there is some bad indentation in that function. It may theoretically fail in the NS_ENSURE_ARG() line. And we never know what it is changed to in the future. I think it is a guideline to check for return values if a function returns nsresult.
> >+ case 4: // TODO: implement user specified header
> (In theory we already get this via changing the pref. Or a future bug could
> make cases 0-3 read the string bundle directly, and only case 4 would read a
> pref.)
Bug 107884 is about what the user defined header could be. I'll add the reference into the comment.
> >+ citingHeader = true;
> >+ headerDate = false;
> I would rename citingHeader to headerAuthor for consistency.
It is not the same. headerDate is a subset of citingHeader. That is why there are comments saying that.
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #8409834 -
Attachment is obsolete: true
Attachment #8409834 -
Flags: ui-review?(josiah)
Attachment #8409834 -
Flags: review?(neil)
Attachment #8409834 -
Flags: review?(mkmelin+mozilla)
Attachment #8409834 -
Flags: review?(iann_bugzilla)
Attachment #8409879 -
Flags: review?(neil)
Comment 13•11 years ago
|
||
Comment on attachment 8409879 [details] [diff] [review]
patch v4
Should be #1, #2, #3, not %... r=me with that fixed.
Attachment #8409879 -
Flags: review?(neil) → review+
Assignee | ||
Comment 14•11 years ago
|
||
OK, thanks.
Attachment #8409879 -
Attachment is obsolete: true
Attachment #8410425 -
Flags: ui-review?(josiah)
Attachment #8410425 -
Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Component: Message Compose Window → Composition
Product: Thunderbird → MailNews Core
Comment 15•11 years ago
|
||
Comment on attachment 8410425 [details] [diff] [review]
patch v5
Review of attachment 8410425 [details] [diff] [review]:
-----------------------------------------------------------------
Looks ok to me. r=mkmelin
Attachment #8410425 -
Flags: review?(mkmelin+mozilla) → review+
Updated•11 years ago
|
Attachment #8410425 -
Flags: ui-review?(josiah) → ui-review+
Comment 17•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 31.0
Comment 18•10 years ago
|
||
I'm aware that this is already fixed, but I'd just like to express my appreciation for this change.
I've been wanting the ability to customize attribution lines further, including specifically the ability to insert words in between the date and the timestamp, for years. I'd considered trying to write it up myself, but I suspected that any such change would be rejected on grounds of "should be an add-on".
This isn't the full level of customizability I've envisioned (in that it doesn't allow arbitrary selection of which variable elements to put where and in what order), but it's more than enough for what I'll actually use. Thank you very much for adding this!
Assignee | ||
Comment 19•10 years ago
|
||
Then that is probably bug 107884.
Comment 20•10 years ago
|
||
Can somebody summarize for us Addon authors what the new identifier is, and which old identifiers are deprecated?
Assignee | ||
Comment 21•10 years ago
|
||
prefs |
From the patch:
Removed prefs/strings:
-mailnews.reply_header_authorwrote=%s wrote
-mailnews.reply_header_ondate=On %s
-pref("mailnews.reply_header_separator", ", ");
-pref("mailnews.reply_header_colon", ":");
New prefs:
+## LOCALIZATION NOTE (mailnews.reply_header_authorwrotesingle): #1 is author (name of person replying to)
+mailnews.reply_header_authorwrotesingle=#1 wrote:
+## LOCALIZATION NOTE (mailnews.reply_header_ondateauthorwrote): #1 is author, #2 is date, #3 is time
+mailnews.reply_header_ondateauthorwrote=On #2 #3, #1 wrote:
+## LOCALIZATION NOTE (mailnews.reply_header_authorwroteondate): #1 is author, #2 is date, #3 is time
+mailnews.reply_header_authorwroteondate=#1 wrote on #2 #3:
These are defined as strings to localize in composeMsgs.properties. However they are also prefs with the same names and you can change the words in them and reorder the #1 #2 #3 as you wish. That is the improvement against old state that hardcoded the order of author, date and time in the resulting string.
We just predefine these 3 strings/formats for easier choosing (via the mailnews.reply_header_type) and Seamonkey even has UI for picking from these formats.
Comment 22•9 years ago
|
||
RFE |
Is there any chance to add a placeholder (#4) for e-mail address to get a format like this?
"name <mail@mail.com>" ("#1 <#4>")
Comment 23•8 years ago
|
||
RFE |
Sorry to comment on such an old bug, but...
Is it possible to get the Day of the Week to show in the string, ie:
"On Mon, mm/dd/yyyy at #:## AM, Name <email> wrote:"
?
Also - where are these strings fully documented, I can't seem to find that anywhere.
Thanks
Comment 24•4 months ago
•
|
||
💡 IMO very useful enhancement ideas above:
- comment #22: see bug 140377
- comment #23: There may be an existing bug for this, but haven't found it.
(In reply to Charles from comment #23)
where are these strings fully documented, I can't seem to find that anywhere. Thanks
see bug 1009585#c14 and URL below
Type: defect → enhancement
Summary: Possible improvement in the text of the headers of reply in the emails → attribution line customization: basic customization of email reply header text using placeholders/variables
You need to log in
before you can comment on or make changes to this bug.
Description
•