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)

enhancement
Not set
normal

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)

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
Attached patch patch (obsolete) — Splinter Review
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 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?
Blocks: 275195
Attached patch patch v2 (obsolete) — Splinter Review
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)
Blocks: 334053
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() ...
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).
(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 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 "-- ".)
Attached patch patch v3 (obsolete) — Splinter Review
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)
(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.
Attached patch patch v4 (obsolete) — Splinter Review
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 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+
Attached patch patch v5Splinter Review
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 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+
Attachment #8410425 - Flags: ui-review?(josiah) → ui-review+
Thanks!
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 31.0
Depends on: 1009585
Blocks: 1021228
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!
Then that is probably bug 107884.
Can somebody summarize for us Addon authors what the new identifier is, and which old identifiers are deprecated?
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.
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>")
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

💡 IMO very useful enhancement ideas above:

 
(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

verified as working in TB 115

Status: RESOLVED → VERIFIED
See Also: 275195
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: