Possible improvement in the text of the headers of reply in the emails

RESOLVED FIXED in Thunderbird 31.0

Status

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: eu, Assigned: aceman)

Tracking

(Depends on 1 bug, Blocks 1 bug, {intl})

Trunk
Thunderbird 31.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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
Posted 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
Posted 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 "-- ".)
Posted 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.
Posted 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+
Posted 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
https://hg.mozilla.org/comm-central/rev/2d5ceaf7ac97
Status: ASSIGNED → RESOLVED
Closed: 5 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
You need to log in before you can comment on or make changes to this bug.