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

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Enrique Estévez Fernández]

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.

Comment 1

[:aceman]

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.

Comment 2

[:aceman]

Attachment: patch

So this could do it and allow localizers much better flexibility.

Comment 3

[[e-mail]]

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?

Comment 4

[:aceman]

Attachment: patch v2

So this looks more robust and changing of %s to %X$S (removing smprintf) allows to fix at least bug 334053.

Comment 5

[:aceman]

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

[[e-mail]]

Actually it looks as if nsTextFormatter::smprintf does support %2$S-type entries.

Comment 7

[:aceman]

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

[[e-mail]]

(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

[[e-mail]]

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 "-- ".)

Comment 10

[:aceman]

Attachment: patch v3

It seems .properties files use both formats so yes, as these strings are to be edited by users, let's make them easier.

Comment 11

[:aceman]

(In reply to [e-mail] 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.

Comment 12

[:aceman]

Attachment: patch v4

Comment 13

[[e-mail]]

Comment on attachment 8409879 [details] [diff] [review]
patch v4

Should be #1, #2, #3, not %... r=me with that fixed.

Comment 14

[:aceman]

Attachment: patch v5

OK, thanks.

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 8410425 [details] [diff] [review]
patch v5

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

Looks ok to me. r=mkmelin

Comment 16

[:aceman]

Thanks!

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/2d5ceaf7ac97

Comment 18

[Andrew Buehler]

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!

Comment 19

[:aceman]

Then that is probably bug 107884.

Comment 20

[Axel Grude [:realRaven]]

Can somebody summarize for us Addon authors what the new identifier is, and which old identifiers are deprecated?

Comment 21

[:aceman]

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

[Hubga]

Is there any chance to add a placeholder (#4) for e-mail address to get a format like this?
"name <[e-mail]>" ("#1 <#4>")

Comment 23

[Charles]

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